Fix ssh option processing

cgwalters · cgwalters · commit bc79ff78f5e1 · 2025-09-26T15:47:46.000-04:00
I got bit badly by the problem that `ssh` always runs
a remote shell and basically just does the completely
wrong thing if you pass it multiple things
in argv.

While we're here fix things so libvirt and ephemeral
share more code.

Signed-off-by: Colin Walters &lt;walters@verbum.org&gt;
diff --git a/crates/kit/src/libvirt/ssh.rs b/crates/kit/src/libvirt/ssh.rs
@@ -245,36 +245,38 @@ impl LibvirtSshOpts {
         // Build SSH command
         let mut ssh_cmd = Command::new("ssh");
 
-        // Basic SSH options
+        // Add SSH key and port
         ssh_cmd
             .arg("-i")
             .arg(temp_key.path())
             .arg("-p")
-            .arg(ssh_config.ssh_port.to_string())
-            .args(["-o", "IdentitiesOnly=yes"])
-            .arg("-o")
-            .arg("PasswordAuthentication=no")
-            .arg("-o")
-            .arg("ConnectTimeout=30")
-            .arg("-o")
-            .arg("ServerAliveInterval=60");
-
-        // Host key checking
-        if !self.strict_host_keys {
-            ssh_cmd
-                .arg("-o")
-                .arg("StrictHostKeyChecking=no")
-                .arg("-o")
-                .arg("UserKnownHostsFile=/dev/null");
-        }
+            .arg(ssh_config.ssh_port.to_string());
+
+        // Apply common SSH options
+        let common_opts = crate::ssh::CommonSshOptions {
+            strict_host_keys: self.strict_host_keys,
+            connect_timeout: self.timeout,
+            server_alive_interval: 60,
+            log_level: "ERROR".to_string(),
+            extra_options: vec![],
+        };
+        common_opts.apply_to_command(&mut ssh_cmd);
 
         // Target host
         ssh_cmd.arg(format!("{}@127.0.0.1", self.user));
 
-        // Add command if specified
+        // Add command if specified - use the same argument escaping logic as container SSH
         if !self.command.is_empty() {
             ssh_cmd.arg("--");
-            ssh_cmd.args(&self.command);
+            if self.command.len() > 1 {
+                // Multiple arguments need proper shell escaping
+                let combined_command = crate::ssh::shell_escape_command(&self.command);
+                debug!("Combined escaped command: {}", combined_command);
+                ssh_cmd.arg(combined_command);
+            } else {
+                // Single argument can be passed directly
+                ssh_cmd.args(&self.command);
+            }
         }
 
         debug!("Executing SSH command: {:?}", ssh_cmd);
diff --git a/crates/kit/src/run_ephemeral_ssh.rs b/crates/kit/src/run_ephemeral_ssh.rs
@@ -166,7 +166,7 @@ pub fn run_ephemeral_ssh(opts: RunEphemeralSshOpts) -> Result<()> {
 
     // Execute SSH connection directly (no thread needed for this)
     // This allows SSH output to be properly forwarded to stdout/stderr
-    debug!("Connecting to SSH...");
+    debug!("Connecting to SSH with args: {:?}", opts.ssh_args);
     let status = ssh::connect_via_container_with_status(&container_name, opts.ssh_args)?;
     debug!("SSH connection completed");
 
diff --git a/crates/kit/src/ssh.rs b/crates/kit/src/ssh.rs
@@ -9,6 +9,19 @@ use tracing::debug;
 
 use crate::CONTAINER_STATEDIR;
 
+/// Combine multiple command arguments into a properly escaped shell command string
+///
+/// This is necessary because SSH protocol sends commands as strings, not argument arrays.
+/// When bcvk receives multiple arguments like ["/bin/sh", "-c", "echo hello; sleep 5"],
+/// they must be combined into a single string that will be correctly interpreted by the
+/// remote shell.
+///
+/// Uses the `shlex` crate for robust POSIX shell escaping.
+pub fn shell_escape_command(args: &[String]) -> String {
+    shlex::try_join(args.iter().map(|s| s.as_str()))
+        .expect("Command arguments should not contain null bytes")
+}
+
 /// Represents an SSH keypair with file paths and public key content
 #[derive(Debug, Clone)]
 pub struct SshKeyPair {
@@ -176,25 +189,76 @@ pub fn connect_via_container(container_name: &str, args: Vec<String>) -> Result<
 /// SSH connection configuration options
 #[derive(Debug, Clone)]
 pub struct SshConnectionOptions {
-    /// Connection timeout in seconds (default: 30)
-    pub connect_timeout: u32,
+    /// Common SSH options shared across implementations
+    pub common: CommonSshOptions,
     /// Enable/disable TTY allocation (default: true)
     pub allocate_tty: bool,
-    /// SSH log level (default: ERROR)
+    /// Suppress output to stdout/stderr (default: false)
+    pub suppress_output: bool,
+}
+
+/// Common SSH options that can be shared between different SSH implementations
+#[derive(Debug, Clone)]
+pub struct CommonSshOptions {
+    /// Use strict host key checking
+    pub strict_host_keys: bool,
+    /// SSH connection timeout in seconds
+    pub connect_timeout: u32,
+    /// Server alive interval in seconds
+    pub server_alive_interval: u32,
+    /// SSH log level
     pub log_level: String,
     /// Additional SSH options as key-value pairs
     pub extra_options: Vec<(String, String)>,
-    /// Suppress output to stdout/stderr (default: false)
-    pub suppress_output: bool,
 }
 
-impl Default for SshConnectionOptions {
+impl Default for CommonSshOptions {
     fn default() -> Self {
         Self {
+            strict_host_keys: false,
             connect_timeout: 30,
-            allocate_tty: true,
+            server_alive_interval: 60,
             log_level: "ERROR".to_string(),
             extra_options: vec![],
+        }
+    }
+}
+
+impl CommonSshOptions {
+    /// Apply these options to an SSH command
+    pub fn apply_to_command(&self, cmd: &mut std::process::Command) {
+        // Basic security options
+        cmd.args(["-o", "IdentitiesOnly=yes"]);
+        cmd.args(["-o", "PasswordAuthentication=no"]);
+        cmd.args(["-o", "KbdInteractiveAuthentication=no"]);
+        cmd.args(["-o", "GSSAPIAuthentication=no"]);
+
+        // Connection options
+        cmd.args(["-o", &format!("ConnectTimeout={}", self.connect_timeout)]);
+        cmd.args([
+            "-o",
+            &format!("ServerAliveInterval={}", self.server_alive_interval),
+        ]);
+        cmd.args(["-o", &format!("LogLevel={}", self.log_level)]);
+
+        // Host key checking
+        if !self.strict_host_keys {
+            cmd.args(["-o", "StrictHostKeyChecking=no"]);
+            cmd.args(["-o", "UserKnownHostsFile=/dev/null"]);
+        }
+
+        // Add extra SSH options
+        for (key, value) in &self.extra_options {
+            cmd.args(["-o", &format!("{}={}", key, value)]);
+        }
+    }
+}
+
+impl Default for SshConnectionOptions {
+    fn default() -> Self {
+        Self {
+            common: CommonSshOptions::default(),
+            allocate_tty: true,
             suppress_output: false,
         }
     }
@@ -204,10 +268,14 @@ impl SshConnectionOptions {
     /// Create options suitable for quick connectivity tests (short timeout, no TTY)
     pub fn for_connectivity_test() -> Self {
         Self {
-            connect_timeout: 2,
+            common: CommonSshOptions {
+                strict_host_keys: false,
+                connect_timeout: 2,
+                server_alive_interval: 60,
+                log_level: "ERROR".to_string(),
+                extra_options: vec![],
+            },
             allocate_tty: false,
-            log_level: "ERROR".to_string(),
-            extra_options: vec![],
             suppress_output: true,
         }
     }
@@ -251,25 +319,18 @@ pub fn connect_via_container_with_options(
         cmd.args(["exec", container_name, "ssh"]);
     }
 
-    // SSH key and security options
+    // SSH key
     let keypath = Utf8Path::new("/run/tmproot")
         .join(CONTAINER_STATEDIR.trim_start_matches('/'))
         .join("ssh");
     cmd.args(["-i", keypath.as_str()]);
-    cmd.args(["-o", "IdentitiesOnly=yes"]);
-    cmd.args(["-o", "PasswordAuthentication=no"]);
-    cmd.args(["-o", "KbdInteractiveAuthentication=no"]);
-    cmd.args(["-o", "GSSAPIAuthentication=no"]);
-    cmd.args(["-o", "StrictHostKeyChecking=no"]);
-    cmd.args(["-o", "UserKnownHostsFile=/dev/null"]);
-
-    // Configurable options
-    cmd.args(["-o", &format!("ConnectTimeout={}", options.connect_timeout)]);
-    cmd.args(["-o", &format!("LogLevel={}", options.log_level)]);
-
-    // Add extra SSH options
-    for (key, value) in &options.extra_options {
-        cmd.args(["-o", &format!("{}={}", key, value)]);
+
+    // Apply common SSH options
+    options.common.apply_to_command(&mut cmd);
+
+    // Container SSH always uses batch mode for non-interactive commands
+    if !options.allocate_tty {
+        cmd.args(["-o", "BatchMode=yes"]);
     }
 
     // Connect to VM via QEMU port forwarding on localhost
@@ -278,11 +339,31 @@ pub fn connect_via_container_with_options(
 
     // Add any additional arguments
     if !args.is_empty() {
+        debug!("Adding SSH arguments: {:?}", args);
         cmd.arg("--");
-        cmd.args(&args);
+
+        // If we have multiple arguments, we need to properly combine them into a single
+        // command string that will survive shell parsing on the remote side.
+        // This is because SSH protocol sends commands as strings, not argument arrays.
+        if args.len() > 1 {
+            // Combine arguments with proper shell escaping
+            let combined_command = shell_escape_command(&args);
+            debug!("Combined escaped command: {}", combined_command);
+            cmd.arg(combined_command);
+        } else {
+            // Single argument can be passed directly
+            cmd.args(&args);
+        }
     }
 
     debug!("Executing: podman {:?}", cmd.get_args().collect::<Vec<_>>());
+    debug!(
+        "Full command line: podman {}",
+        cmd.get_args()
+            .map(|s| s.to_string_lossy().to_string())
+            .collect::<Vec<_>>()
+            .join(" ")
+    );
 
     // Suppress output if requested (useful for connectivity testing)
     if options.suppress_output {
@@ -336,36 +417,68 @@ mod tests {
     fn test_ssh_connection_options() {
         // Test default options
         let default_opts = SshConnectionOptions::default();
-        assert_eq!(default_opts.connect_timeout, 30);
+        assert_eq!(default_opts.common.connect_timeout, 30);
         assert!(default_opts.allocate_tty);
-        assert_eq!(default_opts.log_level, "ERROR");
-        assert!(default_opts.extra_options.is_empty());
+        assert_eq!(default_opts.common.log_level, "ERROR");
+        assert!(default_opts.common.extra_options.is_empty());
         assert!(!default_opts.suppress_output);
 
         // Test connectivity test options
         let test_opts = SshConnectionOptions::for_connectivity_test();
-        assert_eq!(test_opts.connect_timeout, 2);
+        assert_eq!(test_opts.common.connect_timeout, 2);
         assert!(!test_opts.allocate_tty);
-        assert_eq!(test_opts.log_level, "ERROR");
-        assert!(test_opts.extra_options.is_empty());
+        assert_eq!(test_opts.common.log_level, "ERROR");
+        assert!(test_opts.common.extra_options.is_empty());
         assert!(test_opts.suppress_output);
 
         // Test custom options
         let mut custom_opts = SshConnectionOptions::default();
-        custom_opts.connect_timeout = 10;
+        custom_opts.common.connect_timeout = 10;
         custom_opts.allocate_tty = false;
-        custom_opts.log_level = "DEBUG".to_string();
+        custom_opts.common.log_level = "DEBUG".to_string();
         custom_opts
+            .common
             .extra_options
             .push(("ServerAliveInterval".to_string(), "30".to_string()));
 
-        assert_eq!(custom_opts.connect_timeout, 10);
+        assert_eq!(custom_opts.common.connect_timeout, 10);
         assert!(!custom_opts.allocate_tty);
-        assert_eq!(custom_opts.log_level, "DEBUG");
-        assert_eq!(custom_opts.extra_options.len(), 1);
+        assert_eq!(custom_opts.common.log_level, "DEBUG");
+        assert_eq!(custom_opts.common.extra_options.len(), 1);
         assert_eq!(
-            custom_opts.extra_options[0],
+            custom_opts.common.extra_options[0],
             ("ServerAliveInterval".to_string(), "30".to_string())
         );
     }
+
+    #[test]
+    fn test_shell_escape_command() {
+        // Single argument
+        assert_eq!(shell_escape_command(&["echo".to_string()]), "echo");
+
+        // Multiple simple arguments
+        assert_eq!(
+            shell_escape_command(&["/bin/sh".to_string(), "-c".to_string()]),
+            "/bin/sh -c"
+        );
+
+        // Arguments with special characters - shlex uses single quotes for POSIX compliance
+        let result = shell_escape_command(&[
+            "/bin/sh".to_string(),
+            "-c".to_string(),
+            "echo hello; sleep 5; echo world".to_string(),
+        ]);
+        // Verify that the result properly escapes the command with semicolons
+        assert!(result.starts_with("/bin/sh -c "));
+        assert!(result.contains("echo hello; sleep 5; echo world"));
+
+        // Test that shlex properly handles quotes and spaces
+        let result2 = shell_escape_command(&[
+            "echo".to_string(),
+            "hello world".to_string(),
+            "it's working".to_string(),
+        ]);
+        assert!(result2.contains("hello world"));
+        assert!(result2.contains("it's working"));
+    }
 }
