refactor: Improve error reporting on sudo execution error

crates/common/tedge_config/src/lib.rs

@@ -1,5 +1,6 @@
 mod sudo;
 pub use sudo::SudoCommandBuilder;
+pub use sudo::SudoError;
 pub mod cli;
 mod system_toml;
 pub use system_toml::*;

crates/common/tedge_config/src/sudo.rs

@@ -1,5 +1,6 @@
 use std::ffi::OsStr;
 use std::process::Command;
+use std::process::Stdio;
 use std::sync::Arc;
 use tracing::warn;
 
@@ -65,4 +66,79 @@ impl SudoCommandBuilder {
             }
         }
     }
+
+    /// Ensure the command can be executed using sudo
+    ///
+    /// Be warned, that the command is actually executed.
+    pub fn ensure_command_succeeds<S: AsRef<OsStr>>(
+        &self,
+        program: &impl AsRef<OsStr>,
+        args: &Vec<S>,
+    ) -> Result<(), SudoError> {
+        let output = self
+            .command(program)
+            .args(args)
+            .stdout(Stdio::null())
+            .stderr(Stdio::piped())
+            .output();
+        match output {
+            Ok(output) if output.status.success() => Ok(()),
+            Ok(output) => {
+                tracing::error!(target: "sudo", "{} failed with stderr: <<EOF\n{}\nEOF",
+                    program.as_ref().to_string_lossy(),
+                    String::from_utf8_lossy(output.stderr.as_ref()));
+                match output.status.code() {
+                    Some(exit_code) => {
+                        if self.command_is_sudo_enabled(program, args) {
+                            Err(SudoError::ExecutionFailed(exit_code))
+                        } else {
+                            Err(SudoError::CannotSudo)
+                        }
+                    }
+                    None => Err(SudoError::ExecutionInterrupted),
+                }
+            }
+            Err(err) => Err(SudoError::CannotExecute(err)),
+        }
+    }
+
+    /// Check that sudo is enabled and the user authorized to run the command with sudo
+    ///
+    /// This is done by running `sudo --list <command> <args>`.
+    fn command_is_sudo_enabled<S: AsRef<OsStr>>(
+        &self,
+        program: &impl AsRef<OsStr>,
+        args: &Vec<S>,
+    ) -> bool {
+        if !self.enabled {
+            return false;
+        }
+        let Ok(sudo) = which::which(self.sudo_program.as_ref()) else {
+            return false;
+        };
+        let status = Command::new(sudo)
+            .arg("-n")
+            .arg("--list")
+            .arg(program)
+            .args(args)
+            .stdout(Stdio::null())
+            .stderr(Stdio::null())
+            .status();
+        matches!(status, Ok(status) if status.success())
+    }
+}
+
+#[derive(thiserror::Error, Debug)]
+pub enum SudoError {
+    #[error("The user has not been authorized to run the command with sudo")]
+    CannotSudo,
+
+    #[error(transparent)]
+    CannotExecute(#[from] std::io::Error),
+
+    #[error("The command returned a non-zero exit code: {0}")]
+    ExecutionFailed(i32),
+
+    #[error("The command has been interrupted by a signal")]
+    ExecutionInterrupted,
 }

crates/core/plugin_sm/src/plugin.rs

@@ -138,7 +138,7 @@ pub trait Plugin {
         if failed_updates.is_empty() {
             let outcome = self.update_list(&updates, command_log.as_deref_mut()).await;
             if let Err(err @ SoftwareError::UpdateListNotSupported(_)) = outcome {
-                info!("{err}");
+                info!(target: "SM plugins", "{err}");
                 for update in updates.iter() {
                     if let Err(error) = self
                         .apply(update, command_log.as_deref_mut(), download_path)
@@ -228,7 +228,7 @@ pub trait Plugin {
                     url: url.url().to_string(),
                 })
         {
-            error!("Download error: {err:#?}");
+            error!(target: "SM plugins", "Download error: {err:#?}");
             if let Some(ref mut logger) = command_log {
                 logger
                     .write(format!("error: {}\n", &err).as_bytes())

crates/core/plugin_sm/src/plugin_manager.rs

@@ -5,11 +5,9 @@ use camino::Utf8PathBuf;
 use std::borrow::Cow;
 use std::collections::BTreeMap;
 use std::fs;
-use std::io::ErrorKind;
 use std::io::{self};
 use std::path::Path;
 use std::path::PathBuf;
-use std::process::Stdio;
 use tedge_api::commands::CommandStatus;
 use tedge_api::commands::SoftwareListCommand;
 use tedge_api::commands::SoftwareUpdateCommand;
@@ -18,6 +16,7 @@ use tedge_api::SoftwareError;
 use tedge_api::SoftwareType;
 use tedge_api::DEFAULT;
 use tedge_config::SudoCommandBuilder;
+use tedge_config::SudoError;
 use tracing::error;
 use tracing::info;
 use tracing::warn;
@@ -106,7 +105,7 @@ impl ExternalPlugins {
             config_dir,
         };
         if let Err(e) = plugins.load().await {
-            warn!(
+            warn!(target: "SM plugins",
                 "Reading the plugins directory ({:?}): failed with: {e:?}",
                 &plugins.plugin_dir
             );
@@ -119,15 +118,15 @@ impl ExternalPlugins {
                     .by_software_type(default_plugin_type.as_str())
                     .is_none()
                 {
-                    warn!(
+                    warn!(target: "SM plugins",
                         "The configured default plugin: {} not found",
                         default_plugin_type
                     );
                 }
-                info!("Default plugin type: {}", default_plugin_type)
+                info!(target: "SM plugins", "Default plugin type: {}", default_plugin_type)
             }
             None => {
-                info!("Default plugin type: Not configured")
+                info!(target: "SM plugins", "Default plugin type: Not configured")
             }
         }
 
@@ -145,42 +144,28 @@ impl ExternalPlugins {
             let entry = maybe_entry?;
             let path = entry.path();
             if path.is_file() {
-                let mut command = self.sudo.command(&path);
-
-                match command
-                    .arg(LIST)
-                    .stdout(Stdio::null())
-                    .stderr(Stdio::null())
-                    .status()
-                {
-                    Ok(code) if code.success() => {
-                        info!("Plugin activated: {}", path.display());
+                match self.sudo.ensure_command_succeeds(&path, &vec![LIST]) {
+                    Ok(()) => {
+                        info!(target: "SM plugins", "Plugin activated: {}", path.display());
                     }
-
-                    // If the file is not executable or returned non 0 status code we assume it is not a valid and skip further processing.
-                    Ok(_) => {
-                        error!(
-                            "File {} in plugin directory does not support list operation and may not be a valid plugin, skipping.",
+                    Err(SudoError::CannotSudo) => {
+                        error!(target: "SM plugins",
+                            "Skipping {}: not properly configured to run with sudo",
                             path.display()
                         );
                         continue;
                     }
-
-                    Err(err) if err.kind() == ErrorKind::PermissionDenied => {
-                        error!(
-                            "File {} Permission Denied, is the file an executable?\n
-                            The file will not be registered as a plugin.",
+                    Err(SudoError::ExecutionFailed(_)) => {
+                        error!(target: "SM plugins",
+                            "Skipping {}: does not support list operation and may not be a valid plugin",
                             path.display()
                         );
                         continue;
                     }
-
                     Err(err) => {
-                        error!(
-                            "An error occurred while trying to run: {}: {}\n
-                            The file will not be registered as a plugin.",
-                            path.display(),
-                            err
+                        error!(target: "SM plugins",
+                            "Skipping {}: can not be launched as a plugin: {err}",
+                            path.display()
                         );
                         continue;
                     }

crates/extensions/tedge_log_manager/src/plugin_manager.rs

@@ -4,10 +4,9 @@ use crate::plugin::LIST;
 use camino::Utf8Path;
 use camino::Utf8PathBuf;
 use std::collections::BTreeMap;
-use std::io::ErrorKind;
-use std::process::Stdio;
 use std::sync::Arc;
 use tedge_config::SudoCommandBuilder;
+use tedge_config::SudoError;
 use tracing::error;
 use tracing::info;
 
@@ -39,7 +38,7 @@ impl ExternalPlugins {
                 Err(err) => {
                     error!(
                         target: "log plugins",
-                        "Failed to read log plugin directory {plugin_dir} due to: {err}, skipping"
+                        "Skipping directory {plugin_dir}: {err}"
                     );
                     continue;
                 }
@@ -50,7 +49,7 @@ impl ExternalPlugins {
                     Ok(entry) => entry,
                     Err(err) => {
                         error!(target: "log plugins",
-                            "Failed to read log plugin directory entry in {plugin_dir}: due to {err}, skipping",
+                            "Skipping directory entry in {plugin_dir}: {err}",
                         );
                         continue;
                     }
@@ -60,53 +59,38 @@ impl ExternalPlugins {
                     let Some(plugin_name) = path.file_name() else {
                         error!(
                             target: "log plugins",
-                            "Failed to extract log plugin name from {path}, skipping",
+                            "Skipping {path}: failed to extract plugin name",
                         );
                         continue;
                     };
                     if let Some(plugin) = self.plugin_map.get(plugin_name) {
                         info!(
                             target: "log plugins",
-                            "The log plugin {path} is overriden by {}, skipping",
+                            "Skipping {path}: overridden by {}",
                             plugin.path.display()
                         );
                         continue;
                     }
 
-                    let mut command = self.sudo.command(path);
-
-                    match command
-                        .arg(LIST)
-                        .stdout(Stdio::null())
-                        .stderr(Stdio::null())
-                        .status()
-                    {
-                        Ok(code) if code.success() => {
+                    match self.sudo.ensure_command_succeeds(&path, &vec![LIST]) {
+                        Ok(()) => {
                             info!(target: "log plugins", "Log plugin activated: {path}");
                         }
-
-                        // If the file is not executable or returned non 0 status code we assume it is not a valid log plugin and skip further processing.
-                        Ok(_) => {
+                        Err(SudoError::CannotSudo) => {
                             error!(target: "log plugins",
-                                "File {path} in log plugin directory does not support list operation and may not be a valid plugin, skipping."
+                                "Skipping {path}: not properly configured to run with sudo"
                             );
                             continue;
                         }
-
-                        Err(err) if err.kind() == ErrorKind::PermissionDenied => {
-                            error!(
-                                target: "log plugins",
-                                "File {path} Permission Denied, is the file an executable?\n
-                                The file will not be registered as a log plugin."
+                        Err(SudoError::ExecutionFailed(_)) => {
+                            error!(target: "log plugins",
+                             "Skipping {path}: does not support list operation and may not be a valid plugin"
                             );
                             continue;
                         }
-
                         Err(err) => {
-                            error!(
-                                target: "log plugins",
-                                "An error occurred while trying to run: {path}: {err}\n
-                                The file will not be registered as a log plugin."
+                            error!(target: "log plugins",
+                                "Skipping {path}: can not be launched as a plugin: {err}"
                             );
                             continue;
                         }

tests/RobotFramework/requirements/requirements.adapter-docker.txt

@@ -1 +1 @@
-robotframework-devicelibrary[docker] @ git+https://github.com/thin-edge/robotframework-devicelibrary.git@1.21.1
+robotframework-devicelibrary[docker] @ git+https://github.com/thin-edge/robotframework-devicelibrary.git@1.22.1

tests/RobotFramework/requirements/requirements.adapter-local.txt

@@ -1 +1 @@
-robotframework-devicelibrary[local] @ git+https://github.com/thin-edge/robotframework-devicelibrary.git@1.21.1
+robotframework-devicelibrary[local] @ git+https://github.com/thin-edge/robotframework-devicelibrary.git@1.22.1

tests/RobotFramework/requirements/requirements.adapter-ssh.txt

@@ -1,2 +1,2 @@
-robotframework-devicelibrary[ssh] @ git+https://github.com/thin-edge/robotframework-devicelibrary.git@1.21.1
+robotframework-devicelibrary[ssh] @ git+https://github.com/thin-edge/robotframework-devicelibrary.git@1.22.1
 robotframework-sshlibrary~=3.8.0

tests/RobotFramework/tests/cumulocity/log/log_operation_plugins.robot

@@ -134,7 +134,8 @@ Overriding a log plugin
     # Add an extra location for local log plugins
     Execute Command    mkdir -p /usr/local/tedge/log-plugins
     Execute Command    tedge config set log.plugin_paths '/usr/local/tedge/log-plugins,/usr/share/tedge/log-plugins'
-    Execute Command    cmd=echo 'tedge ALL = (ALL) NOPASSWD:SETENV: /usr/local/tedge/log-plugins/[a-zA-Z0-9]*' | sudo tee -a /etc/sudoers.d/tedge
+    Execute Command
+    ...    cmd=echo 'tedge ALL = (ALL) NOPASSWD:SETENV: /usr/local/tedge/log-plugins/[a-zA-Z0-9]*' | sudo tee -a /etc/sudoers.d/tedge
     Restart Service    tedge-agent
     Should Support Log File Types    fake_log::fake_plugin    includes=${True}
     # Override the fake plugin with a dummy plugin
@@ -158,6 +159,23 @@ Overriding a log plugin
     ...    ${operation}
     ...    expected_pattern=.*Dummy content.*
 
+Reporting sudo misconfiguration
+    # Add an extra location for local log plugins
+    # BUT failing to authorize tedge to run these plugins with sudo
+    Stop Service    tedge-agent
+    Execute Command    mkdir -p /etc/share/tedge/log-plugins
+    Execute Command    tedge config add log.plugin_paths /etc/share/tedge/log-plugins
+    ThinEdgeIO.Transfer To Device
+    ...    ${CURDIR}/plugins/dummy_plugin
+    ...    /etc/share/tedge/log-plugins/dummy_plugin
+    Execute Command    chmod a+x /etc/share/tedge/log-plugins/dummy_plugin
+
+    Start Service    tedge-agent
+    Service Logs Should Contain
+    ...    tedge-agent
+    ...    current_only=${True}
+    ...    text=ERROR log plugins: Skipping /etc/share/tedge/log-plugins/dummy_plugin: not properly configured to run with sudo
+
 Agent resilient to plugin dirs removal
     ${date_from}=    Get Unix Timestamp
     Execute Command    rm -rf /usr/share/tedge/log-plugins