fixup! only use tedge-write if no write permissions

crates/core/tedge_write/src/api.rs

@@ -21,11 +21,11 @@ pub struct CopyOptions<'a> {
     /// Permission mode for the file, in octal form.
     pub mode: Option<u32>,
 
-    /// User which will become the new owner of the file.
-    pub user: Option<&'a str>,
+    /// New UID of the file.
+    pub uid: Option<u32>,
 
-    /// Group which will become the new owner of the file.
-    pub group: Option<&'a str>,
+    /// New GID of the file.
+    pub gid: Option<u32>,
 }
 
 impl<'a> CopyOptions<'a> {
@@ -66,11 +66,11 @@ impl<'a> CopyOptions<'a> {
         if let Some(mode) = self.mode {
             command.arg("--mode").arg(format!("{mode:o}"));
         }
-        if let Some(user) = self.user {
-            command.arg("--user").arg(user);
+        if let Some(user) = self.uid {
+            command.arg("--user").arg(user.to_string());
         }
-        if let Some(group) = self.group {
-            command.arg("--group").arg(group);
+        if let Some(group) = self.gid {
+            command.arg("--group").arg(group.to_string());
         }
 
         Ok(command)
@@ -99,8 +99,8 @@ mod tests {
             to: dest_path.as_path().try_into().unwrap(),
             sudo: SudoCommandBuilder::enabled(true),
             mode: None,
-            user: None,
-            group: None,
+            uid: None,
+            gid: None,
         };
 
         // when sudo not in path, start tedge-write without sudo

crates/extensions/tedge_config_manager/Cargo.toml

@@ -28,6 +28,7 @@ tedge_uploader_ext = { workspace = true }
 tedge_utils = { workspace = true }
 thiserror = { workspace = true }
 toml = { workspace = true }
+uzers = { workspace = true }
 
 [dev-dependencies]
 tedge_actors = { workspace = true, features = ["test-helpers"] }

crates/extensions/tedge_config_manager/src/actor.rs

@@ -1,3 +1,4 @@
+use anyhow::Context;
 use async_trait::async_trait;
 use camino::Utf8Path;
 use camino::Utf8PathBuf;
@@ -7,6 +8,9 @@ use log::info;
 use serde_json::json;
 use std::collections::HashMap;
 use std::io::ErrorKind;
+use std::os::unix::fs::chown;
+use std::os::unix::fs::MetadataExt;
+use std::os::unix::fs::PermissionsExt;
 use tedge_actors::fan_in_message_type;
 use tedge_actors::Actor;
 use tedge_actors::ChannelError;
@@ -31,9 +35,11 @@ use tedge_mqtt_ext::QoS;
 use tedge_mqtt_ext::Topic;
 use tedge_uploader_ext::UploadRequest;
 use tedge_uploader_ext::UploadResult;
-use tedge_utils::fs::AtomFileError;
+use tedge_utils::file::PermissionEntry;
+use tedge_utils::fs::atomically_write_file_sync;
 use tedge_write::CopyOptions;
 
+use crate::FileEntry;
 use crate::TedgeWriteStatus;
 
 use super::config::PluginConfig;
@@ -381,7 +387,9 @@ impl ConfigManagerActor {
         let response = match result {
             Ok(response) => response,
             Err(err) => {
-                let error_message = format!("config-manager failed downloading a file: {err}",);
+                let err =
+                    anyhow::Error::from(err).context("config-manager failed downloading a file");
+                let error_message = format!("{err:#}");
                 request.failed(&error_message);
                 error!("{}", error_message);
                 self.publish_command_status(ConfigOperation::Update(topic, request))
@@ -403,6 +411,10 @@ impl ConfigManagerActor {
                 error!("{}", error_message);
                 self.publish_command_status(ConfigOperation::Update(topic, request))
                     .await?;
+
+                // TODO: source temporary file should be cleaned up automatically
+                let _ = std::fs::remove_file(from);
+
                 return Ok(());
             }
         };
@@ -415,57 +427,85 @@ impl ConfigManagerActor {
         self.publish_command_status(ConfigOperation::Update(topic, request))
             .await?;
 
+        // TODO: source temporary file should be cleaned up automatically
+        let _ = std::fs::remove_file(from);
+
         Ok(())
     }
 
     /// Deploys the new version of the configuration file and returns the path under which it was
     /// deployed.
     ///
-    /// This function ensures that the configuration file under `dest` is overwritten by a new
-    /// version currently stored in a temporary directory under `src`. If we have no write
-    /// permissions, use tedge-write for permission elevation if `use_tedge_write` is enabled.
+    /// Ensures that the configuration file under `dest` is overwritten atomically by a new version
+    /// currently stored in a temporary directory.
+    ///
+    /// If the configuration file doesn't already exist, a new file with target permissions needs to
+    /// be created. If the configuration file already exists, its content should be overwritten, but
+    /// owner and mode should remain unchanged.
+    ///
+    /// If `use_tedge_write` is enabled, a `tedge-write` process is spawned when privilege elevation
+    /// is required.
     fn deploy_config_file(
         &self,
         from: &Utf8Path,
         config_type: &str,
     ) -> Result<Utf8PathBuf, ConfigManagementError> {
         let file_entry = self.plugin_config.get_file_entry_from_type(config_type)?;
 
-        let mode = file_entry.file_permissions.mode;
-        let user = file_entry.file_permissions.user.as_deref();
-        let group = file_entry.file_permissions.group.as_deref();
-
         let to = Utf8PathBuf::from(&file_entry.path);
-        let src_file = std::fs::File::open(from)?;
 
-        // try doing a regular copy, return if success or error other than permissions
-        debug!("deploying config file from '{from}' to '{to}'");
-        let err = match tedge_utils::fs::atomically_write_file_sync(&to, src_file) {
-            Ok(()) => return Ok(to),
-            Err(err) => {
-                let AtomFileError::WriteError { source, .. } = &err;
-                if source.kind() != ErrorKind::PermissionDenied {
-                    return Err(err.into());
-                }
-                err
-            }
-        };
-        // if we got permission denied and tedge-write is enabled, use it for privilege elevation
-        let TedgeWriteStatus::Enabled { sudo } = self.config.use_tedge_write.clone() else {
-            return Err(err.into());
+        let Err(err) = move_file_atomic_set_permissions_if_doesnt_exist(
+            from,
+            &to,
+            &file_entry.file_permissions,
+        ) else {
+            return Ok(to);
         };
 
-        debug!("using tedge-write for privilege elevation");
+        if let Some(io_error) = err.downcast_ref::<std::io::Error>() {
+            if io_error.kind() != ErrorKind::PermissionDenied {
+                return Err(err.into());
+            }
+        }
 
-        let options = CopyOptions {
-            from,
-            to: to.as_path(),
-            sudo,
-            mode,
-            user,
-            group,
-        };
-        options.copy()?;
+        match self.config.use_tedge_write.clone() {
+            TedgeWriteStatus::Disabled => {
+                return Err(err.into());
+            }
+
+            TedgeWriteStatus::Enabled { sudo } => {
+                let mode = file_entry.file_permissions.mode;
+
+                let entry_uid = if let Some(ref u) = file_entry.file_permissions.user {
+                    let uid = uzers::get_user_by_name(u)
+                        .with_context(|| format!("no such user: '{u}'"))?
+                        .uid();
+                    Some(uid)
+                } else {
+                    None
+                };
+
+                let entry_gid = if let Some(ref g) = file_entry.file_permissions.group {
+                    let gid = uzers::get_group_by_name(g)
+                        .with_context(|| format!("no such group: '{g}'"))?
+                        .gid();
+                    Some(gid)
+                } else {
+                    None
+                };
+
+                let options = CopyOptions {
+                    from,
+                    to: to.as_path(),
+                    sudo,
+                    mode,
+                    uid: entry_uid,
+                    gid: entry_gid,
+                };
+
+                options.copy()?;
+            }
+        }
 
         Ok(to)
     }
@@ -528,6 +568,92 @@ impl ConfigManagerActor {
     }
 }
 
+/// Writes a file atomically and optionally sets its permissions.
+///
+/// Setting permissions (owner and group) of a file is a privileged operation so it needs to be run
+/// as root. If the 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.
+///
+/// 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.
+///
+/// # Errors
+///
+/// - `src` doesn't exist
+/// - user or group doesn't exist
+/// - we have no write/execute permissions to the directory
+fn move_file_atomic_set_permissions_if_doesnt_exist(
+    src: &Utf8Path,
+    dest: &Utf8Path,
+    permissions: &PermissionEntry,
+) -> anyhow::Result<()> {
+    let current_file_permissions = match std::fs::metadata(dest) {
+        Err(err) => match err.kind() {
+            ErrorKind::PermissionDenied => return Err(err).context("no permissions"),
+            ErrorKind::NotFound => None,
+            _ => return Err(err).context("unexpected IO error"),
+        },
+        Ok(p) => Some(p),
+    };
+
+    let entry_mode = permissions.mode;
+
+    // map username and groupname to uid and gid
+    let entry_uid = if let Some(ref u) = permissions.user {
+        let uid = uzers::get_user_by_name(u)
+            .with_context(|| format!("no such user: '{u}'"))?
+            .uid();
+        Some(uid)
+    } else {
+        None
+    };
+
+    let entry_gid = if let Some(ref g) = permissions.group {
+        let gid = uzers::get_group_by_name(g)
+            .with_context(|| format!("no such group: '{g}'"))?
+            .gid();
+        Some(gid)
+    } else {
+        None
+    };
+
+    let target_mode = current_file_permissions
+        .as_ref()
+        .map(|p| p.mode())
+        .or(entry_mode);
+    let target_uid = current_file_permissions
+        .as_ref()
+        .map(|p| p.uid())
+        .or(entry_uid);
+    let target_gid = current_file_permissions
+        .as_ref()
+        .map(|p| p.gid())
+        .or(entry_gid);
+
+    let src_file = std::fs::File::open(src)?;
+    atomically_write_file_sync(dest, src_file)?;
+    std::fs::set_permissions(
+        dest,
+        std::fs::Permissions::from_mode(target_mode.unwrap_or(0o644)),
+    )?;
+    chown(dest, target_uid, target_gid)?;
+
+    Ok(())
+}
+
 #[derive(Debug, PartialEq, Eq, Clone)]
 pub enum ConfigOperation {
     Snapshot(Topic, ConfigSnapshotCmdPayload),