Merge pull request #3091 from Bravo555/refactor/atomic-write-module

Extract atomic write function into a module

crates/common/tedge_utils/src/atomic.rs

@@ -0,0 +1,125 @@
+//! Utilities for atomic writes.
+//!
+//! For deployment of configuration files, we need to create a file atomically because certain
+//! programs might watch configuration file for changes, so if it's not written atomically, then
+//! file might be only partially written and a program trying to read it may crash.
+//!
+//! Atomic write of a file consists of creating a temporary file in the same directory, filling it
+//! with correct content and permissions, and only then renaming the temporary into the destination
+//! filename. Because we're never actually writing into the file, we don't need to write permissions
+//! for the destination file, even if it exists. Instead we need only write/execute permissions to
+//! the directory file is located in unless the directory has a sticky bit set. Overwriting a file
+//! will also change its uid/gid/mode, if writing process euid/egid is different from file's
+//! uid/gid. To keep uid/gid the same, after the write we need to do `chown`, and to do it we need
+//! sudo.
+
+use std::io::ErrorKind;
+use std::io::Read;
+use std::os::unix::fs::fchown;
+use std::os::unix::fs::MetadataExt;
+use std::os::unix::fs::PermissionsExt;
+use std::path::Path;
+
+use anyhow::Context;
+
+/// Writes a file atomically and optionally sets its permissions.
+///
+/// Setting ownership of a file is a privileged operation so it needs to be run as root. If any of
+/// the filesystem operations fail due to not having permissions, the function will return an error.
+///
+/// If the file already exists, its content will be overwritten but its permissions will remain
+/// unchanged.
+pub fn write_file_atomic_set_permissions_if_doesnt_exist(
+    mut src: impl Read,
+    dest: impl AsRef<Path>,
+    permissions: &MaybePermissions,
+) -> anyhow::Result<()> {
+    let dest = dest.as_ref();
+
+    let target_permissions = target_permissions(dest, permissions)
+        .context("failed to compute target permissions of the file")?;
+
+    // TODO: create tests to ensure writes we expect are atomic
+    let mut tempfile = tempfile::Builder::new()
+        .permissions(std::fs::Permissions::from_mode(0o600))
+        .tempfile_in(dest.parent().context("invalid path")?)
+        .with_context(|| {
+            format!(
+                "could not create temporary file at '{}'",
+                dest.to_string_lossy()
+            )
+        })?;
+
+    std::io::copy(&mut src, &mut tempfile).context("failed to copy")?;
+
+    tempfile
+        .as_file()
+        .set_permissions(std::fs::Permissions::from_mode(target_permissions.mode))
+        .context("failed to set mode on the destination file")?;
+
+    fchown(
+        tempfile.as_file(),
+        Some(target_permissions.uid),
+        Some(target_permissions.gid),
+    )
+    .context("failed to change ownership of the destination file")?;
+
+    tempfile.as_file().sync_all()?;
+
+    tempfile
+        .persist(dest)
+        .context("failed to persist temporary file at destination")?;
+
+    Ok(())
+}
+
+/// Computes target permissions for the file.
+///
+/// - if file exists preserve current permissions
+/// - if it doesn't exist apply permissions from `permissions` if they are defined
+/// - set to root:root with default umask otherwise
+///
+/// # Errors
+/// - if desired user/group doesn't exist on the system
+/// - no permission to read destination file
+fn target_permissions(dest: &Path, permissions: &MaybePermissions) -> anyhow::Result<Permissions> {
+    let current_file_permissions = match std::fs::metadata(dest) {
+        Err(err) => match err.kind() {
+            ErrorKind::NotFound => None,
+            _ => return Err(err.into()),
+        },
+        Ok(p) => Some(p),
+    };
+
+    let uid = current_file_permissions
+        .as_ref()
+        .map(|p| p.uid())
+        .or(permissions.uid)
+        .unwrap_or(0);
+
+    let gid = current_file_permissions
+        .as_ref()
+        .map(|p| p.gid())
+        .or(permissions.gid)
+        .unwrap_or(0);
+
+    let mode = current_file_permissions
+        .as_ref()
+        .map(|p| p.mode())
+        .or(permissions.mode)
+        .unwrap_or(0o644);
+
+    Ok(Permissions { uid, gid, mode })
+}
+
+pub struct MaybePermissions {
+    pub uid: Option<u32>,
+    pub gid: Option<u32>,
+    pub mode: Option<u32>,
+}
+
+struct Permissions {
+    uid: u32,
+    gid: u32,
+    mode: u32,
+}

crates/common/tedge_utils/src/lib.rs

@@ -1,3 +1,4 @@
+pub mod atomic;
 pub mod file;
 pub mod fs;
 pub mod paths;

crates/core/tedge_write/Cargo.toml

@@ -18,6 +18,7 @@ clap.workspace = true
 nix.workspace = true
 path-clean.workspace = true
 tedge_config.workspace = true
+tedge_utils.workspace = true
 uzers.workspace = true
 which.workspace = true
 

crates/core/tedge_write/src/bin.rs

@@ -1,16 +1,10 @@
 // TODO: force `destination_path` to be the first argument in clap
 
-use std::fs;
-use std::io;
-use std::os::unix::fs::chown;
-use std::os::unix::fs::MetadataExt;
-use std::os::unix::fs::PermissionsExt;
-
 use anyhow::bail;
 use anyhow::Context;
-use camino::Utf8Path;
 use camino::Utf8PathBuf;
 use clap::Parser;
+use tedge_utils::atomic::MaybePermissions;
 
 /// A binary used for writing to files which `tedge` user does not have write permissions for, using
 /// sudo.
@@ -62,133 +56,34 @@ pub fn run(args: Args) -> anyhow::Result<()> {
         );
     }
 
-    let file_existed_before_write = target_filepath.is_file();
-
-    write_stdin_to_file_atomic(&target_filepath)?;
-
-    if file_existed_before_write {
-        return Ok(());
-    }
-
-    if args.user.is_some() || args.group.is_some() {
-        chown_by_user_and_group_name(
-            &target_filepath,
-            args.user.as_deref(),
-            args.group.as_deref(),
-        )
-        .context("Changing ownership of destination file failed")?;
-    }
-
-    if let Some(mode) = args.mode {
-        let mode = u32::from_str_radix(&mode, 8).context("Parsing mode failed")?;
-        let permissions = fs::Permissions::from_mode(mode);
-        fs::set_permissions(args.destination_path.as_std_path(), permissions)
-            .context("Could not set new permissions")?;
-    }
-
-    Ok(())
-}
+    let mode = args
+        .mode
+        .map(|m| u32::from_str_radix(&m, 8).with_context(|| format!("invalid mode: {m}")))
+        .transpose()?;
 
-/// Writes contents of stdin into a file atomically.
-///
-/// To write a file atomically, stdin is written into a temporary file, which is then renamed into the target file.
-/// Because rename is only atomic if both source and destination are on the same filesystem, the temporary file is
-/// located in the same directory as the target file.
-///
-/// Using [`io::copy`], data should be copied using Linux-specific syscalls for copying between file descriptors,
-/// without unnecessary copying to and from a userspace buffer.
-fn write_stdin_to_file_atomic(target_filepath: &Utf8Path) -> anyhow::Result<()> {
-    let temp_filepath = {
-        let Some(temp_filename) = target_filepath.file_name().map(|f| format!("{f}.tmp")) else {
-            bail!("Destination path {target_filepath} does not name a valid filename");
-        };
-        target_filepath.with_file_name(temp_filename)
-    };
-
-    // can fail if no permissions or temporary file already exists
-    let mut temp_file = fs::OpenOptions::new()
-        .write(true)
-        .create_new(true)
-        .open(temp_filepath.as_std_path())
-        .with_context(|| format!("Could not open temporary file `{temp_filepath}` for writing"))?;
-
-    // If the target file already exists, use the same permissions and uid/gid
-    if target_filepath.is_file() {
-        let target_metadata = target_filepath.metadata().with_context(|| {
-            format!("Could not fetch metadata of target file {target_filepath}")
-        })?;
-
-        let uid = target_metadata.uid();
-        let gid = target_metadata.gid();
-        chown(&temp_filepath, Some(uid), Some(gid))
-            .context("Could not set destination owner/group")?;
-
-        let target_permissions = target_metadata.permissions();
-        temp_file
-            .set_permissions(target_permissions)
-            .with_context(|| {
-                format!("Could not set permissions for temporary file {temp_filepath}")
-            })?;
-    }
-
-    let mut stdin = std::io::stdin().lock();
-    io::copy(&mut stdin, &mut temp_file)
-        .with_context(|| format!("Could not write to the temporary file `{temp_filepath}`"))?;
-
-    if let Err(e) = fs::rename(&temp_filepath, target_filepath)
-        .with_context(|| format!("Could not write to destination file `{target_filepath}`"))
-    {
-        let _ = fs::remove_file(&temp_filepath);
-        return Err(e);
-    }
-    Ok(())
-}
+    let uid = args
+        .user
+        .map(|u| uzers::get_user_by_name(&*u).with_context(|| format!("no such user: '{u}'")))
+        .transpose()?
+        .map(|u| u.uid());
 
-fn chown_by_user_and_group_name(
-    filepath: &Utf8Path,
-    user: Option<&str>,
-    group: Option<&str>,
-) -> anyhow::Result<()> {
-    // if user and group contain only digits, then they're ids
-    let user_id = user.and_then(|u| u.parse::<u32>().ok());
-    let group_id = group.and_then(|g| g.parse::<u32>().ok());
+    let gid = args
+        .group
+        .map(|g| uzers::get_group_by_name(&*g).with_context(|| format!("no such group: '{g}'")))
+        .transpose()?
+        .map(|g| g.gid());
 
-    let new_uid = match user {
-        Some(u) => {
-            if user_id.is_some() {
-                user_id
-            } else {
-                Some(
-                    uzers::get_user_by_name(u)
-                        .with_context(|| format!("User `{u}` does not exist"))?
-                        .uid(),
-                )
-            }
-        }
-        None => None,
-    };
+    // what permissions we want to set if the file doesn't exist
+    let permissions = MaybePermissions { uid, gid, mode };
 
-    let new_gid = match group {
-        Some(g) => {
-            if group_id.is_some() {
-                group_id
-            } else {
-                Some(
-                    uzers::get_group_by_name(g)
-                        .with_context(|| format!("Group `{g}` does not exist"))?
-                        .gid(),
-                )
-            }
-        }
-        None => None,
-    };
+    let src = std::io::stdin().lock();
 
-    nix::unistd::chown(
-        filepath.as_std_path(),
-        new_uid.map(nix::unistd::Uid::from_raw),
-        new_gid.map(nix::unistd::Gid::from_raw),
+    tedge_utils::atomic::write_file_atomic_set_permissions_if_doesnt_exist(
+        src,
+        &target_filepath,
+        &permissions,
     )
-    .with_context(|| format!("chown failed for file `{filepath}`"))?;
+    .with_context(|| format!("failed to write to destination file '{target_filepath}'"))?;
 
     Ok(())
 }