From febaea6ff812a52ddab4fd76c60d00c1a5e056c3 Mon Sep 17 00:00:00 2001 From: Marcel Guzik Date: Mon, 26 Aug 2024 20:43:44 +0200 Subject: [PATCH] fixup! only use tedge-write if no write permissions --- Cargo.lock | 1 + crates/core/tedge_write/src/api.rs | 20 +- .../tedge_config_manager/Cargo.toml | 1 + .../tedge_config_manager/src/actor.rs | 196 ++++++++++++++---- .../configuration_operation.robot | 53 ++++- 5 files changed, 218 insertions(+), 53 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b700dac2df6..083be8a3575 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4002,6 +4002,7 @@ dependencies = [ "thiserror", "tokio", "toml 0.7.8", + "uzers", ] [[package]] diff --git a/crates/core/tedge_write/src/api.rs b/crates/core/tedge_write/src/api.rs index 5af924d1c47..cb8f13086c1 100644 --- a/crates/core/tedge_write/src/api.rs +++ b/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, - /// User which will become the new owner of the file. - pub user: Option<&'a str>, + /// New UID of the file. + pub uid: Option, - /// Group which will become the new owner of the file. - pub group: Option<&'a str>, + /// New GID of the file. + pub gid: Option, } 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 diff --git a/crates/extensions/tedge_config_manager/Cargo.toml b/crates/extensions/tedge_config_manager/Cargo.toml index e82a6d91a7e..4397b05b09e 100644 --- a/crates/extensions/tedge_config_manager/Cargo.toml +++ b/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"] } diff --git a/crates/extensions/tedge_config_manager/src/actor.rs b/crates/extensions/tedge_config_manager/src/actor.rs index aa58d395f3a..09670d4d3ce 100644 --- a/crates/extensions/tedge_config_manager/src/actor.rs +++ b/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,15 +427,24 @@ 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, @@ -431,41 +452,60 @@ impl ConfigManagerActor { ) -> Result { 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::() { + 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), diff --git a/tests/RobotFramework/tests/cumulocity/configuration/configuration_operation.robot b/tests/RobotFramework/tests/cumulocity/configuration/configuration_operation.robot index 84a7cb869c4..a819ad86dc0 100644 --- a/tests/RobotFramework/tests/cumulocity/configuration/configuration_operation.robot +++ b/tests/RobotFramework/tests/cumulocity/configuration/configuration_operation.robot @@ -30,7 +30,7 @@ Set Configuration when file does not exist Text file (Child Device) ${CHILD_SN} ${PARENT_SN}:device:${CHILD_SN} CONFIG1 /etc/config1.json ${CURDIR}/config1-version2.json 640 tedge:tedge delete_file_before=${true} Binary file (Child Device) ${CHILD_SN} ${PARENT_SN}:device:${CHILD_SN} CONFIG1_BINARY /etc/binary-config1.tar.gz ${CURDIR}/binary-config1.tar.gz 640 tedge:tedge delete_file_before=${true} -Set Configuration when file exists +Set Configuration when file exists and agent run normally [Tags] \#2972 [Template] Set Configuration from Device [Documentation] If the configuration file already exists, it should be overwritten, but owner and permissions @@ -40,6 +40,20 @@ Set Configuration when file exists Text file (Child Device) ${CHILD_SN} ${PARENT_SN}:device:${CHILD_SN} CONFIG1 /etc/config1.json ${CURDIR}/config1-version2.json 664 root:root delete_file_before=${false} Binary file (Child Device) ${CHILD_SN} ${PARENT_SN}:device:${CHILD_SN} CONFIG1_BINARY /etc/binary-config1.tar.gz ${CURDIR}/binary-config1.tar.gz 664 root:root delete_file_before=${false} +Set Configuration when file exists and tedge run by root + [Tags] \#2972 + [Template] Set Configuration from Device + [Documentation] If the configuration file already exists, it should be overwritten, but owner and permissions + ... should remain unchanged. If tedge-agent is run as root, it should not use tedge-agent for privilege elevation + Text file (Main Device) ${PARENT_SN} ${PARENT_SN} CONFIG1 /etc/config1.json ${CURDIR}/config1-version2.json 664 root:root delete_file_before=${false} + ... agent_as_root=${true} + Binary file (Main Device) ${PARENT_SN} ${PARENT_SN} CONFIG1_BINARY /etc/binary-config1.tar.gz ${CURDIR}/binary-config1.tar.gz 664 root:root delete_file_before=${false} + ... agent_as_root=${true} + Text file (Child Device) ${CHILD_SN} ${PARENT_SN}:device:${CHILD_SN} CONFIG1 /etc/config1.json ${CURDIR}/config1-version2.json 664 root:root delete_file_before=${false} + ... agent_as_root=${true} + Binary file (Child Device) ${CHILD_SN} ${PARENT_SN}:device:${CHILD_SN} CONFIG1_BINARY /etc/binary-config1.tar.gz ${CURDIR}/binary-config1.tar.gz 664 root:root delete_file_before=${false} + ... agent_as_root=${true} + Set configuration with broken url [Template] Set Configuration from URL Main Device ${PARENT_SN} ${PARENT_SN} CONFIG1 /etc/config1.json invalid://hellö.zip @@ -242,20 +256,43 @@ Set Configuration from Device ... ${permission} ... ${ownership} ... ${delete_file_before}=${true} + ... ${agent_as_root}=${false} IF ${delete_file_before} ThinEdgeIO.Set Device Context ${device} Execute Command rm -f ${device_file} END - Cumulocity.Set Device ${external_id} - ${config_url}= Cumulocity.Create Inventory Binary temp_file ${config_type} file=${file} - ${operation}= Cumulocity.Set Configuration ${config_type} url=${config_url} - ${operation}= Operation Should Be SUCCESSFUL ${operation} timeout=120 + # we check that when `tedge` user has permissions to the configuration file's parent directory, tedge-write is not + # used to deploy the configuration file but a normal write is used; we change path of tedge-write so that test fails + # if its attempted to be used, the test fails + IF ${agent_as_root} == ${true} + ThinEdgeIO.Set Device Context ${device} + Execute Command sed 's/User\=tedge/User\=root/' -i /lib/systemd/system/tedge-agent.service + Execute Command systemctl daemon-reload + Execute Command systemctl restart tedge-agent + Execute Command mv /usr/bin/tedge-write /usr/bin/tedge-write.bak + END - ThinEdgeIO.Set Device Context ${device} - File Checksum Should Be Equal ${device_file} ${file} - Path Should Have Permissions ${device_file} ${permission} ${ownership} + TRY + Cumulocity.Set Device ${external_id} + ${config_url}= Cumulocity.Create Inventory Binary temp_file ${config_type} file=${file} + ${operation}= Cumulocity.Set Configuration ${config_type} url=${config_url} + ${operation}= Operation Should Be SUCCESSFUL ${operation} timeout=120 + + ThinEdgeIO.Set Device Context ${device} + File Checksum Should Be Equal ${device_file} ${file} + Path Should Have Permissions ${device_file} ${permission} ${ownership} + + FINALLY + IF ${agent_as_root} == ${true} + ThinEdgeIO.Set Device Context ${device} + Execute Command mv /usr/bin/tedge-write.bak /usr/bin/tedge-write + Execute Command sed 's/User\=root/User\=tedge/' -i /lib/systemd/system/tedge-agent.service + Execute Command systemctl daemon-reload + Execute Command systemctl restart tedge-agent + END + END Set Configuration from URL [Arguments] ${test_desc} ${device} ${external_id} ${config_type} ${device_file} ${config_url}