engineering: [osmodifier follow-up] add stdin piping support to Dependency Command wrapper

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,19 @@ fn validate_shadow_value(value: &str) -> Result<(), Error> {
 }
 
 fn hash_password(plaintext: &str) -> Result<String, Error> {
-    // 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 raw = Dependency::Openssl
+        .cmd()
+        .with_arg("passwd")
+        .with_arg("-6")
+        .with_arg("-stdin")
+        .with_stdin(plaintext.as_bytes())
+        .raw_output_and_check()
+        .context("Failed to hash password with openssl")?;
 
-    let output = child
-        .wait_with_output()
-        .context("Failed to wait for openssl passwd")?;
+    let stdout =
+        String::from_utf8(raw.stdout).context("openssl passwd produced non-UTF-8 output")?;
 
-    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())
+    Ok(stdout.trim().to_string())
 }
 
 fn create_user(user: &MICUser) -> Result<(), Error> {
@@ -266,38 +245,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(())
 }
 

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<OsString>,
     envs: Vec<(OsString, OsString)>,
+    stdin_data: Option<Vec<u8>>,
 }
 
 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<Vec<u8>>) -> &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<Vec<u8>>) -> Self {
+        self.stdin(data);
+        self
+    }
+
     pub fn envs<I, K, V>(&mut self, vars: I) -> &mut Command
     where
         I: IntoIterator<Item = (K, V)>,
@@ -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::<Vec<_>>()
                     .join(" "),
             )
+        };
+        if self.stdin_data.is_some() {
+            format!("{base} (with stdin)")
+        } else {
+            base
         }
     }
 
@@ -301,12 +322,70 @@ 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 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 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(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 +579,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");
+    }
 }