From 58ac48e1c84b162e9eb5cb43dc3d3facb979a436 Mon Sep 17 00:00:00 2001 From: bfjelds Date: Tue, 26 May 2026 09:09:58 -0700 Subject: [PATCH 1/6] feat(deps): add stdin piping support to Dependency Command wrapper Extend the Dependency::Command wrapper in osutils to support piping data to a child process's stdin. This maintains the wrapper's uniform output capture, error formatting, and tracing for all command executions. Changes: - Add stdin_data field and stdin()/with_stdin() builder methods to Command - Use a writer thread to avoid deadlocks when the child's output pipes fill - Handle BrokenPipe gracefully (child exit status takes precedence) - Annotate render_command() with '(with stdin)' for tracing without leaking payload contents - Rewrite hash_password() and set_password_via_chpasswd() in osmodifier to use the Dependency wrapper instead of raw std::process::Command - Add Cat test dependency and comprehensive stdin tests Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/osmodifier/src/users.rs | 80 ++++------------ crates/osutils/src/dependencies.rs | 144 +++++++++++++++++++++++++++-- 2 files changed, 153 insertions(+), 71 deletions(-) diff --git a/crates/osmodifier/src/users.rs b/crates/osmodifier/src/users.rs index 68fda4ab8..ba843c0d3 100644 --- a/crates/osmodifier/src/users.rs +++ b/crates/osmodifier/src/users.rs @@ -3,7 +3,7 @@ //! User management — create/update users, passwords, SSH keys, groups. -use std::{fs, io::Write, os::unix::fs::PermissionsExt, path::Path, process::Command}; +use std::{fs, os::unix::fs::PermissionsExt, path::Path}; use anyhow::{bail, Context, Error}; use log::{debug, info}; @@ -164,40 +164,16 @@ fn validate_shadow_value(value: &str) -> Result<(), Error> { } fn hash_password(plaintext: &str) -> Result { - // Use Dependency::Openssl to resolve the binary path for consistent - // detection, but use std::process::Command for stdin piping which - // the Dependency Command wrapper doesn't yet support. - let openssl_path = Dependency::Openssl - .path() - .context("openssl is required for password hashing")?; - - let mut child = Command::new(openssl_path) - .args(["passwd", "-6", "-stdin"]) - .stdin(std::process::Stdio::piped()) - .stdout(std::process::Stdio::piped()) - .stderr(std::process::Stdio::piped()) - .spawn() - .context("Failed to start openssl passwd")?; - - if let Some(mut stdin) = child.stdin.take() { - stdin - .write_all(plaintext.as_bytes()) - .context("Failed to write password to openssl stdin")?; - } - - let output = child - .wait_with_output() - .context("Failed to wait for openssl passwd")?; - - if !output.status.success() { - let stderr = String::from_utf8_lossy(&output.stderr); - bail!("openssl passwd failed: {stderr}"); - } - - Ok(String::from_utf8(output.stdout) - .context("openssl passwd produced non-UTF-8 output")? - .trim() - .to_string()) + let output = Dependency::Openssl + .cmd() + .with_arg("passwd") + .with_arg("-6") + .with_arg("-stdin") + .with_stdin(plaintext.as_bytes()) + .output_and_check() + .context("Failed to hash password with openssl")?; + + Ok(output.trim().to_string()) } fn create_user(user: &MICUser) -> Result<(), Error> { @@ -266,38 +242,16 @@ fn update_user_password(ctx: &OsModifierContext, username: &str, hash: &str) -> /// Set password on a newly created user via chpasswd -e (stdin), avoiding /// leaking the hash through /proc/cmdline. fn set_password_via_chpasswd(username: &str, hash: &str) -> Result<(), Error> { - // Use Dependency::Chpasswd to resolve the binary path for consistent - // detection, but use std::process::Command for stdin piping which - // the Dependency Command wrapper doesn't yet support. - let chpasswd_path = Dependency::Chpasswd - .path() - .context("chpasswd is required for setting user passwords")?; - debug!("Setting password for new user '{username}' via chpasswd"); let input = format!("{username}:{hash}\n"); - let mut child = Command::new(chpasswd_path) - .arg("-e") - .stdin(std::process::Stdio::piped()) - .stdout(std::process::Stdio::piped()) - .stderr(std::process::Stdio::piped()) - .spawn() - .context("Failed to start chpasswd")?; - - if let Some(mut stdin) = child.stdin.take() { - stdin - .write_all(input.as_bytes()) - .context("Failed to write to chpasswd stdin")?; - } - - let output = child - .wait_with_output() - .context("Failed to wait for chpasswd")?; + Dependency::Chpasswd + .cmd() + .with_arg("-e") + .with_stdin(input.into_bytes()) + .run_and_check() + .with_context(|| format!("Failed to set password for '{username}' via chpasswd"))?; - if !output.status.success() { - let stderr = String::from_utf8_lossy(&output.stderr); - bail!("chpasswd failed for '{username}': {stderr}"); - } Ok(()) } diff --git a/crates/osutils/src/dependencies.rs b/crates/osutils/src/dependencies.rs index e5e36c6ee..125db66fc 100644 --- a/crates/osutils/src/dependencies.rs +++ b/crates/osutils/src/dependencies.rs @@ -1,10 +1,10 @@ use std::{ borrow::Cow, ffi::{OsStr, OsString}, - io, + io::{self, Write}, os::unix::process::ExitStatusExt, path::PathBuf, - process::{Command as StdCommand, Output}, + process::{Command as StdCommand, Output, Stdio}, }; use log::trace; @@ -153,6 +153,8 @@ pub enum Dependency { #[cfg(test)] DoesNotExist, #[cfg(test)] + Cat, + #[cfg(test)] Echo, #[cfg(test)] False, @@ -207,6 +209,7 @@ impl Dependency { dependency: *self, args: vec![], envs: vec![], + stdin_data: None, } } } @@ -215,6 +218,7 @@ pub struct Command { dependency: Dependency, args: Vec, envs: Vec<(OsString, OsString)>, + stdin_data: Option>, } impl Command { @@ -249,6 +253,18 @@ impl Command { self } + /// Set data to pipe to the command's stdin. + pub fn stdin(&mut self, data: impl Into>) -> &mut Self { + self.stdin_data = Some(data.into()); + self + } + + /// Owned-builder variant of [`stdin`](Self::stdin). + pub fn with_stdin(mut self, data: impl Into>) -> Self { + self.stdin(data); + self + } + pub fn envs(&mut self, vars: I) -> &mut Command where I: IntoIterator, @@ -275,7 +291,7 @@ impl Command { } fn render_command(&self) -> String { - if self.args.is_empty() { + let base = if self.args.is_empty() { self.dependency.to_string() } else { format!( @@ -292,6 +308,11 @@ impl Command { .collect::>() .join(" "), ) + }; + if self.stdin_data.is_some() { + format!("{base} (with stdin)") + } else { + base } } @@ -301,12 +322,59 @@ impl Command { cmd.envs(self.envs.clone()); let rendered_command = self.render_command(); trace!("Executing '{rendered_command}'"); - let output = cmd - .output() - .map_err(|inner| DependencyError::CouldNotExecute { - dependency: self.dependency, - inner, + + let output = if let Some(ref stdin_data) = self.stdin_data { + cmd.stdin(Stdio::piped()); + cmd.stdout(Stdio::piped()); + cmd.stderr(Stdio::piped()); + + let mut child = + cmd.spawn() + .map_err(|inner| DependencyError::CouldNotExecute { + dependency: self.dependency, + inner, + })?; + + // Write stdin on a separate thread to avoid deadlock if the child + // fills its stdout/stderr pipe before consuming all input. + let child_stdin = child.stdin.take(); + let payload = stdin_data.clone(); + let writer = std::thread::spawn(move || -> io::Result<()> { + if let Some(mut handle) = child_stdin { + handle.write_all(&payload)?; + // handle is dropped here, closing stdin so the child sees EOF + } + Ok(()) + }); + + let output = child.wait_with_output().map_err(|inner| { + DependencyError::CouldNotExecute { + dependency: self.dependency, + inner, + } })?; + + // Collect stdin write result. BrokenPipe is not fatal — the child + // may have exited early, and its exit status / stderr carries the + // real diagnostic. + if let Err(write_err) = writer.join().unwrap_or(Ok(())) { + if write_err.kind() != io::ErrorKind::BrokenPipe && output.status.success() { + return Err(Box::new(DependencyError::CouldNotExecute { + dependency: self.dependency, + inner: write_err, + })); + } + } + + output + } else { + cmd.output() + .map_err(|inner| DependencyError::CouldNotExecute { + dependency: self.dependency, + inner, + })? + }; + let output = CommandOutput { rendered_command: rendered_command.clone(), dependency: self.dependency, @@ -500,4 +568,64 @@ mod tests { )); assert_eq!(output.explain_exit(), "exited with status: 1"); } + + #[test] + fn test_stdin_piped_to_cat() { + let output = Dependency::Cat + .cmd() + .with_stdin(b"hello from stdin".to_vec()) + .output_and_check() + .unwrap(); + assert_eq!(output, "hello from stdin"); + } + + #[test] + fn test_stdin_empty() { + let output = Dependency::Cat + .cmd() + .with_stdin(Vec::new()) + .output_and_check() + .unwrap(); + assert_eq!(output, ""); + } + + #[test] + fn test_stdin_with_failing_command() { + let result = Dependency::False + .cmd() + .with_stdin(b"ignored".to_vec()) + .output() + .unwrap(); + assert!(!result.success()); + assert!(matches!( + *result.check().unwrap_err(), + DependencyError::ExecutionFailed { .. } + )); + } + + #[test] + fn test_render_command_with_stdin() { + let cmd = Dependency::Cat.cmd().with_stdin(b"secret".to_vec()); + assert_eq!(cmd.render_command(), "cat (with stdin)"); + } + + #[test] + fn test_render_command_with_args_and_stdin() { + let cmd = Dependency::Echo + .cmd() + .with_arg("-n") + .with_stdin(b"data".to_vec()); + assert_eq!(cmd.render_command(), "echo -n (with stdin)"); + } + + #[test] + fn test_stdin_replaces_previous() { + let output = Dependency::Cat + .cmd() + .with_stdin(b"first".to_vec()) + .with_stdin(b"second".to_vec()) + .output_and_check() + .unwrap(); + assert_eq!(output, "second"); + } } From 0fe1a8400652b7325d2947edddc6bffb16ad76ba Mon Sep 17 00:00:00 2001 From: bfjelds Date: Tue, 26 May 2026 09:23:11 -0700 Subject: [PATCH 2/6] style: fix rustfmt formatting in dependencies.rs Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/osutils/src/dependencies.rs | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/crates/osutils/src/dependencies.rs b/crates/osutils/src/dependencies.rs index 125db66fc..3e4d5ccac 100644 --- a/crates/osutils/src/dependencies.rs +++ b/crates/osutils/src/dependencies.rs @@ -328,12 +328,12 @@ impl Command { cmd.stdout(Stdio::piped()); cmd.stderr(Stdio::piped()); - let mut child = - cmd.spawn() - .map_err(|inner| DependencyError::CouldNotExecute { - dependency: self.dependency, - inner, - })?; + let mut child = cmd + .spawn() + .map_err(|inner| DependencyError::CouldNotExecute { + dependency: self.dependency, + inner, + })?; // Write stdin on a separate thread to avoid deadlock if the child // fills its stdout/stderr pipe before consuming all input. @@ -347,12 +347,13 @@ impl Command { Ok(()) }); - let output = child.wait_with_output().map_err(|inner| { - DependencyError::CouldNotExecute { - dependency: self.dependency, - inner, - } - })?; + let output = + child + .wait_with_output() + .map_err(|inner| DependencyError::CouldNotExecute { + dependency: self.dependency, + inner, + })?; // Collect stdin write result. BrokenPipe is not fatal — the child // may have exited early, and its exit status / stderr carries the From fe967ec3edd764c555ec305d1282ec8adc171a07 Mon Sep 17 00:00:00 2001 From: bfjelds Date: Wed, 27 May 2026 10:54:25 -0700 Subject: [PATCH 3/6] fix: use thread::scope for stdin writer to avoid clone and surface panics Replace std::thread::spawn with std::thread::scope so the writer thread can borrow stdin_data directly (eliminating the clone) and panics are surfaced via .expect() rather than silently swallowed by unwrap_or. Addresses both Copilot review comments on PR #657. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/osutils/src/dependencies.rs | 69 +++++++++++++++++------------- 1 file changed, 40 insertions(+), 29 deletions(-) diff --git a/crates/osutils/src/dependencies.rs b/crates/osutils/src/dependencies.rs index 3e4d5ccac..a76b0f63c 100644 --- a/crates/osutils/src/dependencies.rs +++ b/crates/osutils/src/dependencies.rs @@ -335,39 +335,50 @@ impl Command { inner, })?; - // Write stdin on a separate thread to avoid deadlock if the child + // Write stdin on a scoped thread to avoid deadlock if the child // fills its stdout/stderr pipe before consuming all input. + // Using thread::scope lets us borrow stdin_data (no clone) and + // ensures panics are surfaced rather than silently swallowed. let child_stdin = child.stdin.take(); - let payload = stdin_data.clone(); - let writer = std::thread::spawn(move || -> io::Result<()> { - if let Some(mut handle) = child_stdin { - handle.write_all(&payload)?; - // handle is dropped here, closing stdin so the child sees EOF + let dep = self.dependency; + + std::thread::scope(|s| { + let writer = s.spawn(|| -> io::Result<()> { + if let Some(mut handle) = child_stdin { + handle.write_all(stdin_data)?; + // handle is dropped here, closing stdin so the child sees EOF + } + Ok(()) + }); + + let output = + child + .wait_with_output() + .map_err(|inner| DependencyError::CouldNotExecute { + dependency: dep, + inner, + })?; + + // Collect stdin write result. A panic in the writer thread will + // be re-raised here. BrokenPipe is not fatal — the child may + // have exited early, and its exit status / stderr carries the + // real diagnostic. + match writer.join().expect("stdin writer thread panicked") { + Ok(()) => {} + Err(write_err) => { + if write_err.kind() != io::ErrorKind::BrokenPipe + && output.status.success() + { + return Err(Box::new(DependencyError::CouldNotExecute { + dependency: dep, + inner: write_err, + })); + } + } } - Ok(()) - }); - - let output = - child - .wait_with_output() - .map_err(|inner| DependencyError::CouldNotExecute { - dependency: self.dependency, - inner, - })?; - - // Collect stdin write result. BrokenPipe is not fatal — the child - // may have exited early, and its exit status / stderr carries the - // real diagnostic. - if let Err(write_err) = writer.join().unwrap_or(Ok(())) { - if write_err.kind() != io::ErrorKind::BrokenPipe && output.status.success() { - return Err(Box::new(DependencyError::CouldNotExecute { - dependency: self.dependency, - inner: write_err, - })); - } - } - output + Ok(output) + })? } else { cmd.output() .map_err(|inner| DependencyError::CouldNotExecute { From 207f5a25f62aab9ed21911c23363b20f022a79ef Mon Sep 17 00:00:00 2001 From: bfjelds Date: Wed, 27 May 2026 11:01:38 -0700 Subject: [PATCH 4/6] style: fix rustfmt formatting on condition expression Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/osutils/src/dependencies.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/osutils/src/dependencies.rs b/crates/osutils/src/dependencies.rs index a76b0f63c..9d683890f 100644 --- a/crates/osutils/src/dependencies.rs +++ b/crates/osutils/src/dependencies.rs @@ -366,8 +366,7 @@ impl Command { match writer.join().expect("stdin writer thread panicked") { Ok(()) => {} Err(write_err) => { - if write_err.kind() != io::ErrorKind::BrokenPipe - && output.status.success() + if write_err.kind() != io::ErrorKind::BrokenPipe && output.status.success() { return Err(Box::new(DependencyError::CouldNotExecute { dependency: dep, From 5c478ef2487950323e8a717dc971151d769d293e Mon Sep 17 00:00:00 2001 From: bfjelds Date: Wed, 27 May 2026 11:17:51 -0700 Subject: [PATCH 5/6] fix: use strict UTF-8 decoding for openssl passwd output Switch hash_password() from output_and_check() (which uses lossy UTF-8 decoding) to raw_output_and_check() with explicit String::from_utf8(). This preserves the prior strict behavior where non-UTF-8 output becomes an error rather than silently producing a corrupted password hash. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/osmodifier/src/users.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/crates/osmodifier/src/users.rs b/crates/osmodifier/src/users.rs index ba843c0d3..617b53c52 100644 --- a/crates/osmodifier/src/users.rs +++ b/crates/osmodifier/src/users.rs @@ -164,16 +164,19 @@ fn validate_shadow_value(value: &str) -> Result<(), Error> { } fn hash_password(plaintext: &str) -> Result { - let output = Dependency::Openssl + let raw = Dependency::Openssl .cmd() .with_arg("passwd") .with_arg("-6") .with_arg("-stdin") .with_stdin(plaintext.as_bytes()) - .output_and_check() + .raw_output_and_check() .context("Failed to hash password with openssl")?; - Ok(output.trim().to_string()) + let stdout = String::from_utf8(raw.stdout) + .context("openssl passwd produced non-UTF-8 output")?; + + Ok(stdout.trim().to_string()) } fn create_user(user: &MICUser) -> Result<(), Error> { From d793dee3a9e323e4de6b35eab4a149a73dca27af Mon Sep 17 00:00:00 2001 From: bfjelds Date: Wed, 27 May 2026 11:23:42 -0700 Subject: [PATCH 6/6] style: fix rustfmt formatting on from_utf8 binding Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/osmodifier/src/users.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/osmodifier/src/users.rs b/crates/osmodifier/src/users.rs index 617b53c52..1b2222d00 100644 --- a/crates/osmodifier/src/users.rs +++ b/crates/osmodifier/src/users.rs @@ -173,8 +173,8 @@ fn hash_password(plaintext: &str) -> Result { .raw_output_and_check() .context("Failed to hash password with openssl")?; - let stdout = String::from_utf8(raw.stdout) - .context("openssl passwd produced non-UTF-8 output")?; + let stdout = + String::from_utf8(raw.stdout).context("openssl passwd produced non-UTF-8 output")?; Ok(stdout.trim().to_string()) }