Skip to content

Commit f5893f7

Browse files
didier-wenzekdidier-wenzek
committed
Improve error reporting on sudo execution error
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
1 parent 7e86b90 commit f5893f7

File tree

9 files changed

+132
-68
lines changed

9 files changed

+132
-68
lines changed

‎crates/common/tedge_config/src/lib.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
mod sudo;
22
pub use sudo::SudoCommandBuilder;
3+
pub use sudo::SudoError;
34
pub mod cli;
45
mod system_toml;
56
pub use system_toml::*;

‎crates/common/tedge_config/src/sudo.rs‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
use std::ffi::OsStr;
22
use std::process::Command;
3+
use std::process::Stdio;
34
use std::sync::Arc;
45
use tracing::warn;
56

@@ -65,4 +66,79 @@ impl SudoCommandBuilder {
6566
}
6667
}
6768
}
69+
70+
/// Ensure the command can be executed using sudo
71+
///
72+
/// Be warned, that the command is actually executed.
73+
pub fn ensure_command_succeeds<S: AsRef<OsStr>>(
74+
&self,
75+
program: &impl AsRef<OsStr>,
76+
args: &Vec<S>,
77+
) -> Result<(), SudoError> {
78+
let output = self
79+
.command(program)
80+
.args(args)
81+
.stdout(Stdio::null())
82+
.stderr(Stdio::piped())
83+
.output();
84+
match output {
85+
Ok(output) if output.status.success() => Ok(()),
86+
Ok(output) => {
87+
tracing::error!(target: "sudo", "{} failed with stderr: <<EOF\n{}\nEOF",
88+
program.as_ref().to_string_lossy(),
89+
String::from_utf8_lossy(output.stderr.as_ref()));
90+
match output.status.code() {
91+
Some(exit_code) => {
92+
if self.command_is_sudo_enabled(program, args) {
93+
Err(SudoError::ExecutionFailed(exit_code))
94+
} else {
95+
Err(SudoError::CannotSudo)
96+
}
97+
}
98+
None => Err(SudoError::ExecutionInterrupted),
99+
}
100+
}
101+
Err(err) => Err(SudoError::CannotExecute(err)),
102+
}
103+
}
104+
105+
/// Check that sudo is enabled and the user authorized to run the command with sudo
106+
///
107+
/// This is done by running `sudo --list <command> <args>`.
108+
fn command_is_sudo_enabled<S: AsRef<OsStr>>(
109+
&self,
110+
program: &impl AsRef<OsStr>,
111+
args: &Vec<S>,
112+
) -> bool {
113+
if !self.enabled {
114+
return false;
115+
}
116+
let Ok(sudo) = which::which(self.sudo_program.as_ref()) else {
117+
return false;
118+
};
119+
let status = Command::new(sudo)
120+
.arg("-n")
121+
.arg("--list")
122+
.arg(program)
123+
.args(args)
124+
.stdout(Stdio::null())
125+
.stderr(Stdio::null())
126+
.status();
127+
matches!(status, Ok(status) if status.success())
128+
}
129+
}
130+
131+
#[derive(thiserror::Error, Debug)]
132+
pub enum SudoError {
133+
#[error("The user has not been authorized to run the command with sudo")]
134+
CannotSudo,
135+
136+
#[error(transparent)]
137+
CannotExecute(#[from] std::io::Error),
138+
139+
#[error("The command returned a non-zero exit code: {0}")]
140+
ExecutionFailed(i32),
141+
142+
#[error("The command has been interrupted by a signal")]
143+
ExecutionInterrupted,
68144
}

‎crates/core/plugin_sm/src/plugin.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,7 @@ pub trait Plugin {
138138
if failed_updates.is_empty() {
139139
let outcome = self.update_list(&updates, command_log.as_deref_mut()).await;
140140
if let Err(err @ SoftwareError::UpdateListNotSupported(_)) = outcome {
141-
info!("{err}");
141+
info!(target: "SM plugins", "{err}");
142142
for update in updates.iter() {
143143
if let Err(error) = self
144144
.apply(update, command_log.as_deref_mut(), download_path)
@@ -228,7 +228,7 @@ pub trait Plugin {
228228
url: url.url().to_string(),
229229
})
230230
{
231-
error!("Download error: {err:#?}");
231+
error!(target: "SM plugins", "Download error: {err:#?}");
232232
if let Some(ref mut logger) = command_log {
233233
logger
234234
.write(format!("error: {}\n", &err).as_bytes())

‎crates/core/plugin_sm/src/plugin_manager.rs‎

Lines changed: 17 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,9 @@ use camino::Utf8PathBuf;
55
use std::borrow::Cow;
66
use std::collections::BTreeMap;
77
use std::fs;
8-
use std::io::ErrorKind;
98
use std::io::{self};
109
use std::path::Path;
1110
use std::path::PathBuf;
12-
use std::process::Stdio;
1311
use tedge_api::commands::CommandStatus;
1412
use tedge_api::commands::SoftwareListCommand;
1513
use tedge_api::commands::SoftwareUpdateCommand;
@@ -18,6 +16,7 @@ use tedge_api::SoftwareError;
1816
use tedge_api::SoftwareType;
1917
use tedge_api::DEFAULT;
2018
use tedge_config::SudoCommandBuilder;
19+
use tedge_config::SudoError;
2120
use tracing::error;
2221
use tracing::info;
2322
use tracing::warn;
@@ -106,7 +105,7 @@ impl ExternalPlugins {
106105
config_dir,
107106
};
108107
if let Err(e) = plugins.load().await {
109-
warn!(
108+
warn!(target: "SM plugins",
110109
"Reading the plugins directory ({:?}): failed with: {e:?}",
111110
&plugins.plugin_dir
112111
);
@@ -119,15 +118,15 @@ impl ExternalPlugins {
119118
.by_software_type(default_plugin_type.as_str())
120119
.is_none()
121120
{
122-
warn!(
121+
warn!(target: "SM plugins",
123122
"The configured default plugin: {} not found",
124123
default_plugin_type
125124
);
126125
}
127-
info!("Default plugin type: {}", default_plugin_type)
126+
info!(target: "SM plugins", "Default plugin type: {}", default_plugin_type)
128127
}
129128
None => {
130-
info!("Default plugin type: Not configured")
129+
info!(target: "SM plugins", "Default plugin type: Not configured")
131130
}
132131
}
133132

@@ -145,42 +144,28 @@ impl ExternalPlugins {
145144
let entry = maybe_entry?;
146145
let path = entry.path();
147146
if path.is_file() {
148-
let mut command = self.sudo.command(&path);
149-
150-
match command
151-
.arg(LIST)
152-
.stdout(Stdio::null())
153-
.stderr(Stdio::null())
154-
.status()
155-
{
156-
Ok(code) if code.success() => {
157-
info!("Plugin activated: {}", path.display());
147+
match self.sudo.ensure_command_succeeds(&path, &vec![LIST]) {
148+
Ok(()) => {
149+
info!(target: "SM plugins", "Plugin activated: {}", path.display());
158150
}
159-
160-
// If the file is not executable or returned non 0 status code we assume it is not a valid and skip further processing.
161-
Ok(_) => {
162-
error!(
163-
"File {} in plugin directory does not support list operation and may not be a valid plugin, skipping.",
151+
Err(SudoError::CannotSudo) => {
152+
error!(target: "SM plugins",
153+
"Skipping {}: not properly configured to run with sudo",
164154
path.display()
165155
);
166156
continue;
167157
}
168-
169-
Err(err) if err.kind() == ErrorKind::PermissionDenied => {
170-
error!(
171-
"File {} Permission Denied, is the file an executable?\n
172-
The file will not be registered as a plugin.",
158+
Err(SudoError::ExecutionFailed(_)) => {
159+
error!(target: "SM plugins",
160+
"Skipping {}: does not support list operation and may not be a valid plugin",
173161
path.display()
174162
);
175163
continue;
176164
}
177-
178165
Err(err) => {
179-
error!(
180-
"An error occurred while trying to run: {}: {}\n
181-
The file will not be registered as a plugin.",
182-
path.display(),
183-
err
166+
error!(target: "SM plugins",
167+
"Skipping {}: can not be launched as a plugin: {err}",
168+
path.display()
184169
);
185170
continue;
186171
}

‎crates/extensions/tedge_log_manager/src/plugin_manager.rs‎

Lines changed: 14 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,9 @@ use crate::plugin::LIST;
44
use camino::Utf8Path;
55
use camino::Utf8PathBuf;
66
use std::collections::BTreeMap;
7-
use std::io::ErrorKind;
8-
use std::process::Stdio;
97
use std::sync::Arc;
108
use tedge_config::SudoCommandBuilder;
9+
use tedge_config::SudoError;
1110
use tracing::error;
1211
use tracing::info;
1312

@@ -39,7 +38,7 @@ impl ExternalPlugins {
3938
Err(err) => {
4039
error!(
4140
target: "log plugins",
42-
"Failed to read log plugin directory {plugin_dir} due to: {err}, skipping"
41+
"Skipping directory {plugin_dir}: {err}"
4342
);
4443
continue;
4544
}
@@ -50,7 +49,7 @@ impl ExternalPlugins {
5049
Ok(entry) => entry,
5150
Err(err) => {
5251
error!(target: "log plugins",
53-
"Failed to read log plugin directory entry in {plugin_dir}: due to {err}, skipping",
52+
"Skipping directory entry in {plugin_dir}: {err}",
5453
);
5554
continue;
5655
}
@@ -60,53 +59,38 @@ impl ExternalPlugins {
6059
let Some(plugin_name) = path.file_name() else {
6160
error!(
6261
target: "log plugins",
63-
"Failed to extract log plugin name from {path}, skipping",
62+
"Skipping {path}: failed to extract plugin name",
6463
);
6564
continue;
6665
};
6766
if let Some(plugin) = self.plugin_map.get(plugin_name) {
6867
info!(
6968
target: "log plugins",
70-
"The log plugin {path} is overriden by {}, skipping",
69+
"Skipping {path}: overridden by {}",
7170
plugin.path.display()
7271
);
7372
continue;
7473
}
7574

76-
let mut command = self.sudo.command(path);
77-
78-
match command
79-
.arg(LIST)
80-
.stdout(Stdio::null())
81-
.stderr(Stdio::null())
82-
.status()
83-
{
84-
Ok(code) if code.success() => {
75+
match self.sudo.ensure_command_succeeds(&path, &vec![LIST]) {
76+
Ok(()) => {
8577
info!(target: "log plugins", "Log plugin activated: {path}");
8678
}
87-
88-
// 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.
89-
Ok(_) => {
79+
Err(SudoError::CannotSudo) => {
9080
error!(target: "log plugins",
91-
"File {path} in log plugin directory does not support list operation and may not be a valid plugin, skipping."
81+
"Skipping {path}: not properly configured to run with sudo"
9282
);
9383
continue;
9484
}
95-
96-
Err(err) if err.kind() == ErrorKind::PermissionDenied => {
97-
error!(
98-
target: "log plugins",
99-
"File {path} Permission Denied, is the file an executable?\n
100-
The file will not be registered as a log plugin."
85+
Err(SudoError::ExecutionFailed(_)) => {
86+
error!(target: "log plugins",
87+
"Skipping {path}: does not support list operation and may not be a valid plugin"
10188
);
10289
continue;
10390
}
104-
10591
Err(err) => {
106-
error!(
107-
target: "log plugins",
108-
"An error occurred while trying to run: {path}: {err}\n
109-
The file will not be registered as a log plugin."
92+
error!(target: "log plugins",
93+
"Skipping {path}: can not be launched as a plugin: {err}"
11094
);
11195
continue;
11296
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
robotframework-devicelibrary[docker] @ git+https://github.com/thin-edge/robotframework-devicelibrary.git@1.21.1
1+
robotframework-devicelibrary[docker] @ git+https://github.com/thin-edge/robotframework-devicelibrary.git@1.22.1

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
robotframework-devicelibrary[local] @ git+https://github.com/thin-edge/robotframework-devicelibrary.git@1.21.1
1+
robotframework-devicelibrary[local] @ git+https://github.com/thin-edge/robotframework-devicelibrary.git@1.22.1

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,2 @@
1-
robotframework-devicelibrary[ssh] @ git+https://github.com/thin-edge/robotframework-devicelibrary.git@1.21.1
1+
robotframework-devicelibrary[ssh] @ git+https://github.com/thin-edge/robotframework-devicelibrary.git@1.22.1
22
robotframework-sshlibrary~=3.8.0

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

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,8 @@ Overriding a log plugin
134134
# Add an extra location for local log plugins
135135
Execute Command mkdir -p /usr/local/tedge/log-plugins
136136
Execute Command tedge config set log.plugin_paths '/usr/local/tedge/log-plugins,/usr/share/tedge/log-plugins'
137-
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
137+
Execute Command
138+
... cmd=echo 'tedge ALL = (ALL) NOPASSWD:SETENV: /usr/local/tedge/log-plugins/[a-zA-Z0-9]*' | sudo tee -a /etc/sudoers.d/tedge
138139
Restart Service tedge-agent
139140
Should Support Log File Types fake_log::fake_plugin includes=${True}
140141
# Override the fake plugin with a dummy plugin
@@ -158,6 +159,23 @@ Overriding a log plugin
158159
... ${operation}
159160
... expected_pattern=.*Dummy content.*
160161

162+
Reporting sudo misconfiguration
163+
# Add an extra location for local log plugins
164+
# BUT failing to authorize tedge to run these plugins with sudo
165+
Stop Service tedge-agent
166+
Execute Command mkdir -p /etc/share/tedge/log-plugins
167+
Execute Command tedge config add log.plugin_paths /etc/share/tedge/log-plugins
168+
ThinEdgeIO.Transfer To Device
169+
... ${CURDIR}/plugins/dummy_plugin
170+
... /etc/share/tedge/log-plugins/dummy_plugin
171+
Execute Command chmod a+x /etc/share/tedge/log-plugins/dummy_plugin
172+
173+
Start Service tedge-agent
174+
Service Logs Should Contain
175+
... tedge-agent
176+
... current_only=${True}
177+
... text=ERROR log plugins: Skipping /etc/share/tedge/log-plugins/dummy_plugin: not properly configured to run with sudo
178+
161179
Agent resilient to plugin dirs removal
162180
${date_from}= Get Unix Timestamp
163181
Execute Command rm -rf /usr/share/tedge/log-plugins

0 commit comments

Comments
 (0)