From aaac12bfaa38d5145429a1fb448ae41ced1961a0 Mon Sep 17 00:00:00 2001 From: "Geoffrey Trousdale (they/them)" Date: Tue, 3 Nov 2026 12:39:57 -0500 Subject: [PATCH 1/3] feat(mxc): support sandbox exec on Windows `sandbox exec`, `connect`, and file transfer reach the sandbox through the supervisor's SSH server. On Windows that server was never started and the relay rejected SSH targets, so exec failed on both MXC backends. Supervisor: the relay and the SSH server run in the same process, so on Windows each relay is handed to the server as one half of an in-memory duplex stream, registered under the configured socket path. No socket or pipe is created, so nothing outside the supervisor can reach the server. The Unix socket path is unchanged. MXC driver: pass OPENSHELL_SSH_SOCKET_PATH so the supervisor starts its SSH server. MXC boundary: accept shell exec intent. The gateway sends a POSIX command string built by shell_escape; when it is that plain form, recover argv, environment, and working directory and spawn the program directly. Other shell syntax runs through cmd.exe /d /s /c. When a program cannot be started directly, retry through cmd.exe, as a POSIX shell would report "command not found" itself; this also resolves cmd built-ins. Without the retry the SSH client waits for an exit status that never arrives. PTY (ConPTY) remains unsupported, so `sandbox connect` is not enabled. Signed-off-by: Geoffrey Trousdale (they/them) --- crates/openshell-driver-mxc/src/driver.rs | 8 + crates/openshell-mxc-boundary/src/lib.rs | 2 + .../src/posix_command.rs | 222 ++++++++++++ crates/openshell-mxc-boundary/src/runtime.rs | 327 +++++++++++++++++- .../src/delegated.rs | 27 +- .../openshell-supervisor-process/src/lib.rs | 1 - .../openshell-supervisor-process/src/ssh.rs | 248 +++++++++++-- .../src/supervisor_session.rs | 8 +- crates/openshell-supervisor/src/lib.rs | 12 +- 9 files changed, 787 insertions(+), 68 deletions(-) create mode 100644 crates/openshell-mxc-boundary/src/posix_command.rs diff --git a/crates/openshell-driver-mxc/src/driver.rs b/crates/openshell-driver-mxc/src/driver.rs index b2ae367bfd..86b0f5dba3 100644 --- a/crates/openshell-driver-mxc/src/driver.rs +++ b/crates/openshell-driver-mxc/src/driver.rs @@ -42,6 +42,7 @@ const DEFAULT_IMAGE_SENTINEL: &str = "mxc:process-container"; const HOST_AUTH_BUNDLE_FILE: &str = "supervisor-auth.json"; /// Directory in the sandbox host state directory for the proxy CA files. const HOST_PROXY_TLS_DIR: &str = "proxy-tls"; +const HOST_SSH_SERVER_NAME: &str = "supervisor-ssh"; const HOST_RUNTIME_DESCRIPTOR_FILE: &str = "runtime-descriptor.json"; const BOUNDARY_CONFIG_FILE: &str = "boundary.json"; const BOUNDARY_TLS_CERT_FILE: &str = "sandbox.crt"; @@ -908,6 +909,13 @@ fn supervisor_command( openshell_core::sandbox_env::PROXY_TLS_DIR, context.host_state_dir.join(HOST_PROXY_TLS_DIR), ) + // Enables the supervisor's SSH server for exec, connect, and file + // transfer. On Windows the server is reached in process, so this path + // only names the server; no socket is created. + .env( + openshell_core::sandbox_env::SSH_SOCKET_PATH, + context.host_state_dir.join(HOST_SSH_SERVER_NAME), + ) .env( openshell_core::sandbox_env::LOG_LEVEL, openshell_core::driver_utils::sandbox_log_level(&context.sandbox, "info"), diff --git a/crates/openshell-mxc-boundary/src/lib.rs b/crates/openshell-mxc-boundary/src/lib.rs index d06070d79c..ff894dfcb5 100644 --- a/crates/openshell-mxc-boundary/src/lib.rs +++ b/crates/openshell-mxc-boundary/src/lib.rs @@ -16,6 +16,8 @@ pub mod backend; mod identity; #[cfg(target_os = "windows")] pub mod launch; +#[cfg(any(target_os = "windows", test))] +mod posix_command; #[cfg(target_os = "windows")] pub mod socket_owner; diff --git a/crates/openshell-mxc-boundary/src/posix_command.rs b/crates/openshell-mxc-boundary/src/posix_command.rs new file mode 100644 index 0000000000..0326f58dc2 --- /dev/null +++ b/crates/openshell-mxc-boundary/src/posix_command.rs @@ -0,0 +1,222 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +//! Recover argv from the POSIX command strings that reach a Windows boundary. +//! +//! The gateway and SSH server deliver `exec` requests as one POSIX shell +//! command string built by `shell_escape`: an optional `cd &&` prefix, +//! `KEY=` assignments, then the escaped argv. Windows has no POSIX +//! shell, and handing that string to `cmd.exe` would mangle its quoting. This +//! module parses exactly that form back into structured parts so the boundary +//! can spawn the program directly with the caller's original arguments. +//! Strings that use any other shell syntax are not parsed; the caller runs them +//! through `cmd.exe` instead. + +/// A command string reduced to the pieces the boundary spawns directly. +#[derive(Debug, PartialEq, Eq)] +pub struct PosixCommand { + pub workdir: Option, + pub env: Vec<(String, String)>, + pub argv: Vec, +} + +#[derive(Debug, PartialEq, Eq)] +enum Token { + Word { text: String, quoted: bool }, + And, +} + +/// Parse `[cd &&] [NAME=]... ...`. +/// +/// Returns `None` when the string uses shell syntax beyond that form +/// (pipes, redirection, variables, globbing, or command separators), or when +/// no program word remains. +pub fn parse(command: &str) -> Option { + let mut tokens = tokenize(command)?.into_iter().peekable(); + let workdir = if matches!(tokens.peek(), Some(Token::Word { text, quoted: false }) if text == "cd") + { + tokens.next(); + let Some(Token::Word { text, .. }) = tokens.next() else { + return None; + }; + if tokens.next() != Some(Token::And) { + return None; + } + Some(text) + } else { + None + }; + + let mut env = Vec::new(); + let mut argv = Vec::new(); + for token in tokens { + let Token::Word { text, quoted } = token else { + return None; + }; + if argv.is_empty() + && !quoted + && let Some((name, value)) = assignment(&text) + { + env.push((name, value)); + continue; + } + argv.push(text); + } + if argv.is_empty() { + return None; + } + Some(PosixCommand { workdir, env, argv }) +} + +/// An unquoted leading `NAME=` makes the word an environment assignment. +fn assignment(word: &str) -> Option<(String, String)> { + let (name, value) = word.split_once('=')?; + let mut chars = name.chars(); + let first = chars.next()?; + if !(first == '_' || first.is_ascii_alphabetic()) + || !chars.all(|c| c == '_' || c.is_ascii_alphanumeric()) + { + return None; + } + Some((name.to_string(), value.to_string())) +} + +/// Split on unquoted whitespace. A word's `quoted` flag records whether quoting +/// began before its first `=`, which keeps a quoted `'A=b'` an argument. +fn tokenize(command: &str) -> Option> { + let mut tokens = Vec::new(); + let mut chars = command.chars().peekable(); + loop { + while chars.peek().is_some_and(char::is_ascii_whitespace) { + chars.next(); + } + let Some(&first) = chars.peek() else { + return Some(tokens); + }; + if first == '&' { + chars.next(); + if chars.next() != Some('&') { + return None; + } + tokens.push(Token::And); + continue; + } + + let mut text = String::new(); + let mut quote_before_equals = false; + while let Some(&c) = chars.peek() { + if c.is_ascii_whitespace() { + break; + } + chars.next(); + match c { + '\'' => { + quote_before_equals |= !text.contains('='); + loop { + match chars.next()? { + '\'' => break, + other => text.push(other), + } + } + } + '"' => { + quote_before_equals |= !text.contains('='); + loop { + match chars.next()? { + '"' => break, + '\\' => match chars.next()? { + escaped @ ('"' | '\\') => text.push(escaped), + other => { + text.push('\\'); + text.push(other); + } + }, + '$' | '`' => return None, + other => text.push(other), + } + } + } + '\\' => text.push(chars.next()?), + '|' | '&' | ';' | '<' | '>' | '(' | ')' | '$' | '`' | '*' | '?' | '[' | '{' + | '}' | '~' | '#' => return None, + other => text.push(other), + } + } + tokens.push(Token::Word { + text, + quoted: quote_before_equals, + }); + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn argv(values: &[&str]) -> Vec { + values.iter().map(|value| (*value).to_string()).collect() + } + + #[test] + fn plain_arguments_round_trip() { + let parsed = parse("cmd.exe /d /c echo hello").expect("parsed"); + assert_eq!(parsed.argv, argv(&["cmd.exe", "/d", "/c", "echo", "hello"])); + assert_eq!(parsed.workdir, None); + assert!(parsed.env.is_empty()); + } + + #[test] + fn gateway_escaping_is_reversed() { + // shell_escape("it's a C:\\path") and an empty argument. + let parsed = parse(r#"'C:\Program Files\x.exe' 'it'"'"'s' '' plain"#).expect("parsed"); + assert_eq!( + parsed.argv, + argv(&[r"C:\Program Files\x.exe", "it's", "", "plain"]) + ); + } + + #[test] + fn workdir_and_environment_prefixes_are_extracted() { + let parsed = + parse(r"cd 'C:\work dir' && A=1 B='two words' tool.exe --flag").expect("parsed"); + assert_eq!(parsed.workdir.as_deref(), Some(r"C:\work dir")); + assert_eq!( + parsed.env, + vec![ + ("A".to_string(), "1".to_string()), + ("B".to_string(), "two words".to_string()) + ] + ); + assert_eq!(parsed.argv, argv(&["tool.exe", "--flag"])); + } + + #[test] + fn quoted_assignment_is_an_argument() { + let parsed = parse("'A=1' tool").expect("parsed"); + assert!(parsed.env.is_empty()); + assert_eq!(parsed.argv, argv(&["A=1", "tool"])); + } + + #[test] + fn assignment_after_program_is_an_argument() { + let parsed = parse("tool A=1").expect("parsed"); + assert_eq!(parsed.argv, argv(&["tool", "A=1"])); + } + + #[test] + fn shell_syntax_is_not_parsed() { + for command in [ + "echo a | more", + "echo a > out.txt", + "echo a; echo b", + "echo $HOME", + "echo *.txt", + "echo a && echo b", + "'unterminated", + "A=1", + "", + ] { + assert_eq!(parse(command), None, "{command}"); + } + } +} diff --git a/crates/openshell-mxc-boundary/src/runtime.rs b/crates/openshell-mxc-boundary/src/runtime.rs index 9a75b4281f..08eb49c759 100644 --- a/crates/openshell-mxc-boundary/src/runtime.rs +++ b/crates/openshell-mxc-boundary/src/runtime.rs @@ -878,6 +878,7 @@ impl BoundaryRuntime { ProcessKindWire::Main, program, args, + None, spec.workdir.clone(), environment, ) @@ -901,10 +902,10 @@ impl BoundaryRuntime { } async fn start_exec(&self, envelope: &RequestEnvelope, spec: ExecSpecWire) -> Response { - if spec.shell.is_some() || spec.runtime_helper.is_some() { + if spec.runtime_helper.is_some() { return guest_error( BoundaryErrorKind::Invalid, - "MXC does not support shell exec intent or runtime helpers", + "MXC does not support runtime helpers", ); } if spec.pty { @@ -913,6 +914,8 @@ impl BoundaryRuntime { "Windows ConPTY is not enabled for MXC exec", ); } + let shell_request = spec.shell.is_some(); + let launch = windows_launch(spec); { let mut requests = lock(&self.exec_requests); if !requests.insert(envelope.request_id.clone()) { @@ -930,20 +933,37 @@ impl BoundaryRuntime { let environment = workload_environment( &self.config.child_env, &lock(&self.provider_environment).1, - &spec.env, + &launch.env, &self.proxy_url, lock(&self.ca_paths).as_ref(), ); - let process = match ManagedProcess::spawn( + let retry = shell_request.then(|| cmd_retry(&launch)).flatten(); + let spawned = ManagedProcess::spawn( id.clone(), ProcessKindWire::Exec, - spec.program, - spec.args, - spec.workdir, - environment, + launch.program, + launch.args, + launch.raw_command_tail, + launch.workdir, + environment.clone(), ) - .await - { + .await; + let spawned = match (spawned, retry) { + (Err(_), Some(retry)) => { + ManagedProcess::spawn( + id.clone(), + ProcessKindWire::Exec, + retry.program, + retry.args, + retry.raw_command_tail, + retry.workdir, + environment, + ) + .await + } + (spawned, _) => spawned, + }; + let process = match spawned { Ok(process) => Arc::new(process), Err(error) => return guest_error(BoundaryErrorKind::Process, error), }; @@ -1182,6 +1202,123 @@ fn install_ca_environment( } } +/// The concrete Windows process an exec request resolves to. +#[derive(Debug, PartialEq, Eq)] +struct WindowsLaunch { + program: String, + args: Vec, + raw_command_tail: Option, + workdir: Option, + env: Vec<(String, String)>, +} + +/// Map an exec request onto a Windows process. +/// +/// Shell intent carries one POSIX command string. When it is the plain form +/// the gateway builds from an argv, the program runs directly with the original +/// arguments, environment, and working directory. Any other shell syntax runs +/// through `cmd.exe /d /s /c`, and a shell with no command starts `cmd.exe` +/// reading stdin. The login-shell flag has no Windows equivalent. +fn windows_launch(spec: ExecSpecWire) -> WindowsLaunch { + let ExecSpecWire { + program, + args, + shell, + env, + workdir, + .. + } = spec; + let Some(shell) = shell else { + return WindowsLaunch { + program, + args, + raw_command_tail: None, + workdir, + env, + }; + }; + let Some(command) = shell.command.filter(|command| !command.trim().is_empty()) else { + return WindowsLaunch { + program: "cmd.exe".to_string(), + args: vec!["/d".to_string(), "/q".to_string()], + raw_command_tail: None, + workdir, + env, + }; + }; + if let Some(parsed) = crate::posix_command::parse(&command) { + let mut argv = parsed.argv.into_iter(); + let program = argv.next().unwrap_or_default(); + let mut merged_env = env; + merged_env.extend(parsed.env); + return WindowsLaunch { + program, + args: argv.collect(), + raw_command_tail: None, + workdir: parsed.workdir.or(workdir), + env: merged_env, + }; + } + WindowsLaunch { + program: "cmd.exe".to_string(), + args: vec!["/d".to_string(), "/s".to_string(), "/c".to_string()], + raw_command_tail: Some(format!("\"{command}\"")), + workdir, + env, + } +} + +/// The `cmd.exe` launch used when a shell request's program cannot be started +/// directly, as a POSIX shell would report `command not found` itself. It also +/// resolves `cmd.exe` built-ins such as `echo`, `dir`, and `type`, and returns +/// cmd's own error text and exit code for a missing program. Without this, a +/// spawn failure leaves the SSH client waiting for an exit status that never +/// arrives. +fn cmd_retry(launch: &WindowsLaunch) -> Option { + if launch.raw_command_tail.is_some() { + return None; + } + let line = std::iter::once(launch.program.as_str()) + .chain(launch.args.iter().map(String::as_str)) + .map(quote_windows_argument) + .collect::>() + .join(" "); + Some(WindowsLaunch { + program: "cmd.exe".to_string(), + args: vec!["/d".to_string(), "/s".to_string(), "/c".to_string()], + raw_command_tail: Some(format!("\"{line}\"")), + workdir: launch.workdir.clone(), + env: launch.env.clone(), + }) +} + +/// Quote one argument with the Microsoft C runtime command-line rules. +fn quote_windows_argument(argument: &str) -> String { + if !argument.is_empty() && !argument.contains([' ', '\t', '"']) { + return argument.to_string(); + } + let mut quoted = String::from("\""); + let mut backslashes = 0_usize; + for c in argument.chars() { + match c { + '\\' => backslashes += 1, + '"' => { + quoted.push_str(&"\\".repeat(backslashes * 2 + 1)); + quoted.push('"'); + backslashes = 0; + } + other => { + quoted.push_str(&"\\".repeat(backslashes)); + quoted.push(other); + backslashes = 0; + } + } + } + quoted.push_str(&"\\".repeat(backslashes * 2)); + quoted.push('"'); + quoted +} + fn workload_environment( child: &HashMap, providers: &HashMap, @@ -1274,6 +1411,7 @@ impl ManagedProcess { kind: ProcessKindWire, program: String, args: Vec, + raw_command_tail: Option, workdir: Option, environment: HashMap, ) -> Result { @@ -1287,6 +1425,11 @@ impl ManagedProcess { .stdout(std::process::Stdio::piped()) .stderr(std::process::Stdio::piped()) .envs(environment); + // cmd.exe parses its own command line, so the text after /c must not be + // re-quoted with the C runtime rules that `Command::args` applies. + if let Some(tail) = raw_command_tail { + command.raw_arg(tail); + } if let Some(workdir) = workdir.filter(|path| !path.trim().is_empty()) { command.current_dir(workdir); } @@ -1690,6 +1833,168 @@ fn lock(mutex: &Mutex) -> std::sync::MutexGuard<'_, T> { #[cfg(test)] mod tests { use super::*; + use openshell_sandbox_backend::boundary_protocol::ShellSpecWire; + + fn shell_exec(command: Option<&str>) -> ExecSpecWire { + ExecSpecWire { + program: String::new(), + args: Vec::new(), + shell: Some(ShellSpecWire { + command: command.map(str::to_string), + login: true, + }), + runtime_helper: None, + env: vec![("FROM_REQUEST".to_string(), "1".to_string())], + workdir: Some(r"C:\default".to_string()), + pty: false, + } + } + + #[test] + fn plain_shell_command_runs_the_program_directly() { + let launch = windows_launch(shell_exec(Some( + r#"cd 'C:\work dir' && A=1 'C:\Program Files\tool.exe' 'it'"'"'s'"#, + ))); + assert_eq!(launch.program, r"C:\Program Files\tool.exe"); + assert_eq!(launch.args, vec!["it's".to_string()]); + assert_eq!(launch.workdir.as_deref(), Some(r"C:\work dir")); + assert_eq!(launch.raw_command_tail, None); + assert_eq!( + launch.env, + vec![ + ("FROM_REQUEST".to_string(), "1".to_string()), + ("A".to_string(), "1".to_string()) + ] + ); + } + + #[test] + fn shell_syntax_runs_through_cmd() { + let launch = windows_launch(shell_exec(Some("echo a | findstr a"))); + assert_eq!(launch.program, "cmd.exe"); + assert_eq!(launch.args, vec!["/d", "/s", "/c"]); + assert_eq!( + launch.raw_command_tail.as_deref(), + Some("\"echo a | findstr a\"") + ); + assert_eq!(launch.workdir.as_deref(), Some(r"C:\default")); + } + + #[test] + fn shell_without_command_starts_cmd() { + let launch = windows_launch(shell_exec(None)); + assert_eq!(launch.program, "cmd.exe"); + assert_eq!(launch.args, vec!["/d", "/q"]); + } + + #[test] + fn argv_exec_is_unchanged() { + let mut spec = shell_exec(None); + spec.shell = None; + spec.program = "tool.exe".to_string(); + spec.args = vec!["a b".to_string()]; + let launch = windows_launch(spec); + assert_eq!(launch.program, "tool.exe"); + assert_eq!(launch.args, vec!["a b".to_string()]); + assert_eq!(launch.raw_command_tail, None); + } + + #[tokio::test] + async fn missing_program_retries_through_cmd() { + let launch = windows_launch(shell_exec(Some("no-such-program.exe 'a b'"))); + let direct = ManagedProcess::spawn( + "retry-direct".to_string(), + ProcessKindWire::Exec, + launch.program.clone(), + launch.args.clone(), + None, + None, + HashMap::new(), + ) + .await; + assert!(direct.is_err(), "the program does not exist"); + let retry = cmd_retry(&launch).expect("parsed launches can retry"); + assert_eq!( + retry.raw_command_tail.as_deref(), + Some("\"no-such-program.exe \"a b\"\"") + ); + let process = ManagedProcess::spawn( + "retry-cmd".to_string(), + ProcessKindWire::Exec, + retry.program, + retry.args, + retry.raw_command_tail, + None, + HashMap::new(), + ) + .await + .expect("cmd starts"); + let status = tokio::time::timeout(Duration::from_secs(10), process.wait()) + .await + .expect("process exits"); + assert!( + matches!(status, ExitStatusWire::Exited(code) if code != 0), + "{status:?}" + ); + } + + #[test] + fn cmd_fallback_launches_are_not_retried() { + let launch = windows_launch(shell_exec(Some("echo a | findstr a"))); + assert_eq!(cmd_retry(&launch), None); + } + + #[test] + fn windows_arguments_are_quoted_for_the_c_runtime() { + assert_eq!(quote_windows_argument("plain"), "plain"); + assert_eq!(quote_windows_argument(""), "\"\""); + assert_eq!(quote_windows_argument("a b"), "\"a b\""); + assert_eq!(quote_windows_argument(r#"say "hi""#), r#""say \"hi\"""#); + assert_eq!( + quote_windows_argument(r"C:\dir with space\"), + r#""C:\dir with space\\""# + ); + } + + #[tokio::test] + async fn cmd_fallback_runs_shell_syntax() { + let launch = windows_launch(shell_exec(Some("echo first & echo second"))); + let process = ManagedProcess::spawn( + "cmd-fallback-test".to_string(), + ProcessKindWire::Exec, + launch.program, + launch.args, + launch.raw_command_tail, + None, + HashMap::new(), + ) + .await + .expect("launch cmd fallback"); + let status = tokio::time::timeout(Duration::from_secs(10), process.wait()) + .await + .expect("process exits"); + assert!(matches!(status, ExitStatusWire::Exited(0))); + let mut cursor = process.output.cursor(); + let output = tokio::time::timeout(Duration::from_secs(10), async { + let mut output = Vec::new(); + loop { + let event = cursor.recv().await.expect("retained output event"); + if event.channel == STREAM_STDOUT { + output.extend(event.payload); + } + if event.channel == STREAM_EXIT { + return output; + } + } + }) + .await + .expect("stdout is retained"); + let output = String::from_utf8_lossy(&output); + assert!( + output.contains("first") && output.contains("second"), + "{output}" + ); + } #[test] fn exec_environment_keeps_proxy_and_ca_with_refreshed_providers() { @@ -2009,6 +2314,7 @@ mod tests { "echo relay-free launch".to_string(), ], None, + None, HashMap::new(), ) .await @@ -2055,6 +2361,7 @@ mod tests { "(for /L %i in (1,1,2000) do @echo output-line) & echo last-error 1>&2".into(), ], None, + None, HashMap::new(), ) .await diff --git a/crates/openshell-supervisor-process/src/delegated.rs b/crates/openshell-supervisor-process/src/delegated.rs index 9f61c6e3e2..659b72e9a2 100644 --- a/crates/openshell-supervisor-process/src/delegated.rs +++ b/crates/openshell-supervisor-process/src/delegated.rs @@ -11,10 +11,8 @@ use miette::Result; use openshell_isolation_interface::contract::{ BoundaryExec, BoundaryLoopbackConnector, BoundaryProcess, }; -#[cfg(unix)] use openshell_ocsf::{ActivityId, AppLifecycleBuilder, SeverityId, StatusId, ocsf_emit}; -#[cfg(unix)] fn ocsf_ctx() -> &'static openshell_ocsf::EventContext { openshell_ocsf::ctx::ctx() } @@ -142,7 +140,6 @@ pub async fn start_boundary_access( } /// The optional SSH adapter is the only platform-specific access component. -#[cfg(unix)] async fn start_optional_ssh( socket_path: Option, shared_socket: bool, @@ -198,24 +195,6 @@ async fn start_optional_ssh( } } -#[cfg(not(unix))] -async fn start_optional_ssh( - socket_path: Option, - _shared_socket: bool, - _ca_file_paths: Option<(std::path::PathBuf, std::path::PathBuf)>, - _boundary_exec: Arc, - _port_forward: Arc, - _main_session: Arc, - _host_key: Option, -) -> Result>> { - if socket_path.is_some() { - return Err(miette::miette!( - "SSH access sockets are unsupported on this host" - )); - } - Ok(None) -} - /// Report the canonical process exit until the gateway acknowledges it. pub async fn report_main_process_exit( endpoint: &str, @@ -384,7 +363,7 @@ mod tests { #[cfg(not(unix))] #[tokio::test] - async fn explicit_unix_ssh_socket_is_rejected() { + async fn in_process_ssh_requires_a_host_key() { let boundary = Arc::new(AccessBoundary); let result = start_boundary_access( None, @@ -402,9 +381,9 @@ mod tests { assert!( result .err() - .expect("Unix SSH unsupported") + .expect("SSH without a host key must be rejected") .to_string() - .contains("unsupported") + .contains("host key is missing") ); } diff --git a/crates/openshell-supervisor-process/src/lib.rs b/crates/openshell-supervisor-process/src/lib.rs index df0f21a519..b2c9ef22fb 100644 --- a/crates/openshell-supervisor-process/src/lib.rs +++ b/crates/openshell-supervisor-process/src/lib.rs @@ -12,7 +12,6 @@ pub mod delegated; pub mod log_push; pub mod main_session; pub mod skills; -#[cfg(unix)] pub mod ssh; pub mod supervisor_session; diff --git a/crates/openshell-supervisor-process/src/ssh.rs b/crates/openshell-supervisor-process/src/ssh.rs index 58b3941df1..676b4b510d 100644 --- a/crates/openshell-supervisor-process/src/ssh.rs +++ b/crates/openshell-supervisor-process/src/ssh.rs @@ -6,16 +6,18 @@ mod input; mod upload; -#[cfg(test)] +#[cfg(all(test, unix))] mod exec_input_tests; -#[cfg(test)] +#[cfg(all(test, unix))] mod upload_tests; use crate::main_session::{MainOutput, MainSession}; #[cfg(unix)] use libc; -use miette::{IntoDiagnostic, Result}; +#[cfg(unix)] +use miette::IntoDiagnostic; +use miette::Result; use openshell_core::VERSION; use openshell_ocsf::{ ActionId, ActivityId, DispositionId, SeverityId, SshActivityBuilder, StatusId, ocsf_emit, @@ -25,9 +27,12 @@ use russh::server::{Auth, ChannelOpenHandle, Handle, Session}; use russh::{ChannelId, ChannelOpenFailure, Sig}; use std::borrow::Cow; use std::collections::HashMap; -use std::path::{Path, PathBuf}; +#[cfg(unix)] +use std::path::Path; +use std::path::PathBuf; use std::sync::Arc; use std::time::Duration; +#[cfg(unix)] use tokio::net::UnixListener; use tracing::warn; @@ -40,7 +45,7 @@ const SSH_PEER_TIMEOUT: Duration = Duration::from_mins(1); mod peer_stream; -#[cfg(test)] +#[cfg(all(test, unix))] #[path = "ssh/reconnect_tests.rs"] mod reconnect_tests; @@ -70,6 +75,7 @@ fn filter_main_detach_sequence(prefix_pending: &mut bool, data: &[u8]) -> (Vec, @@ -91,12 +97,7 @@ pub fn parse_host_key(key: Option<&openshell_core::jwt::SecretSshHostKey>) -> Re Ok(key) } -fn ssh_server_init( - listen_path: &Path, - ca_file_paths: &Option<(PathBuf, PathBuf)>, - shared_socket: bool, - host_key: PrivateKey, -) -> Result { +fn ssh_server_config(host_key: PrivateKey) -> Arc { let mut config = russh::server::Config { server_id: russh::SshId::Standard(Cow::Owned(format!("SSH-2.0-OpenShell_{VERSION}"))), auth_rejection_time: Duration::from_secs(1), @@ -106,8 +107,17 @@ fn ssh_server_init( ..Default::default() }; config.keys.push(host_key); + Arc::new(config) +} - let config = Arc::new(config); +#[cfg(unix)] +fn ssh_server_init( + listen_path: &Path, + ca_file_paths: &Option<(PathBuf, PathBuf)>, + shared_socket: bool, + host_key: PrivateKey, +) -> Result { + let config = ssh_server_config(host_key); let ca_paths = ca_file_paths.as_ref().map(|p| Arc::new(p.clone())); // A driver may place the supervisor in another container, so an explicitly @@ -153,6 +163,7 @@ fn ssh_server_init( Ok((listener, config, ca_paths)) } +#[cfg(unix)] #[allow(clippy::too_many_arguments, clippy::implicit_hasher)] pub async fn run_ssh_server( listen_path: PathBuf, @@ -241,8 +252,10 @@ pub async fn run_ssh_server( } } +#[cfg(unix)] const MAX_CONSECUTIVE_UNKNOWN_SSH_ACCEPT_ERRORS: u32 = 10; +#[cfg(unix)] #[derive(Debug, PartialEq)] enum SshAcceptAction { Terminal, @@ -252,6 +265,7 @@ enum SshAcceptAction { }, } +#[cfg(unix)] fn classify_ssh_accept_error( error: &std::io::Error, consecutive_resource_errors: &mut u32, @@ -333,18 +347,208 @@ fn classify_ssh_accept_error( } } +/// Windows has no socket hop between the relay and the SSH server: both run in +/// the supervisor process, so each accepted relay is handed to the server as +/// one half of an in-memory duplex stream. Nothing outside the supervisor +/// process can reach the server. The configured socket path is only the +/// registry key that pairs a relay with its server. +#[cfg(not(unix))] +mod in_process { + use std::collections::HashMap; + use std::io; + use std::path::{Path, PathBuf}; + use std::sync::{Mutex, OnceLock, PoisonError}; + use tokio::io::DuplexStream; + use tokio::sync::mpsc; + + const DUPLEX_BUFFER_BYTES: usize = 256 * 1024; + const ACCEPT_BACKLOG: usize = 64; + + type Acceptors = Mutex>>; + + fn acceptors() -> &'static Acceptors { + static ACCEPTORS: OnceLock = OnceLock::new(); + ACCEPTORS.get_or_init(|| Mutex::new(HashMap::new())) + } + + /// Removes the server's acceptor when the server task ends, so later relays + /// fail instead of queueing on a dead server. + pub(super) struct Registration { + key: PathBuf, + } + + impl Drop for Registration { + fn drop(&mut self) { + acceptors() + .lock() + .unwrap_or_else(PoisonError::into_inner) + .remove(&self.key); + } + } + + pub(super) fn register( + key: &Path, + ) -> Result<(Registration, mpsc::Receiver), String> { + let mut map = acceptors().lock().unwrap_or_else(PoisonError::into_inner); + if map.contains_key(key) { + return Err(format!( + "an SSH server is already registered for {}", + key.display() + )); + } + let (sender, receiver) = mpsc::channel(ACCEPT_BACKLOG); + map.insert(key.to_path_buf(), sender); + Ok(( + Registration { + key: key.to_path_buf(), + }, + receiver, + )) + } + + /// Open a connection to the in-process SSH server registered under `key`. + pub async fn connect(key: &Path) -> io::Result { + let sender = acceptors() + .lock() + .unwrap_or_else(PoisonError::into_inner) + .get(key) + .cloned() + .ok_or_else(|| { + io::Error::new( + io::ErrorKind::NotFound, + "supervisor SSH server is not running", + ) + })?; + let (client, server) = tokio::io::duplex(DUPLEX_BUFFER_BYTES); + sender.send(server).await.map_err(|_| { + io::Error::new( + io::ErrorKind::ConnectionRefused, + "supervisor SSH server stopped", + ) + })?; + Ok(client) + } +} + +#[cfg(not(unix))] +pub use in_process::connect as connect_in_process; + +#[cfg(all(test, not(unix)))] +mod in_process_tests { + use super::in_process; + use std::path::Path; + use tokio::io::{AsyncReadExt as _, AsyncWriteExt as _}; + + #[tokio::test] + async fn connections_reach_only_the_registered_server() { + let key = Path::new(r"C:\state\sandbox-a\supervisor-ssh"); + let (registration, mut accepted) = in_process::register(key).expect("register"); + assert!( + in_process::register(key).is_err(), + "a second server cannot claim the same key" + ); + + let mut client = in_process::connect(key).await.expect("connect"); + let mut server = accepted.recv().await.expect("server receives the stream"); + client.write_all(b"ping").await.unwrap(); + let mut buffer = [0_u8; 4]; + server.read_exact(&mut buffer).await.unwrap(); + assert_eq!(&buffer, b"ping"); + + let other = in_process::connect(Path::new(r"C:\state\sandbox-b\supervisor-ssh")).await; + assert_eq!( + other.err().map(|error| error.kind()), + Some(std::io::ErrorKind::NotFound) + ); + + drop(registration); + let stopped = in_process::connect(key).await; + assert_eq!( + stopped.err().map(|error| error.kind()), + Some(std::io::ErrorKind::NotFound), + "a stopped server no longer accepts connections" + ); + } +} + +#[cfg(not(unix))] +#[allow(clippy::too_many_arguments, clippy::implicit_hasher)] +pub async fn run_ssh_server( + listen_path: PathBuf, + ready_tx: tokio::sync::oneshot::Sender>, + _ca_file_paths: Option<(PathBuf, PathBuf)>, + _shared_socket: bool, + port_forward: Arc, + boundary_exec: Arc, + main_session: Option>, + host_key: PrivateKey, +) -> Result<()> { + let config = ssh_server_config(host_key); + let (_registration, mut accepted) = match in_process::register(&listen_path) { + Ok(registered) => { + let _ = ready_tx.send(Ok(())); + registered + } + Err(error) => { + let _ = ready_tx.send(Err(miette::miette!(error))); + return Ok(()); + } + }; + ocsf_emit!( + SshActivityBuilder::new(openshell_ocsf::ctx::ctx()) + .activity(ActivityId::Listen) + .severity(SeverityId::Informational) + .status(StatusId::Success) + .message("SSH server listening on the supervisor in-process transport") + .build() + ); + + while let Some(stream) = accepted.recv().await { + let config = config.clone(); + let port_forward = port_forward.clone(); + let boundary_exec = boundary_exec.clone(); + let main_session = main_session.clone(); + tokio::spawn(async move { + if let Err(err) = handle_connection( + stream, + config, + port_forward, + boundary_exec, + main_session, + SSH_PEER_TIMEOUT, + ) + .await + { + ocsf_emit!( + SshActivityBuilder::new(openshell_ocsf::ctx::ctx()) + .activity(ActivityId::Fail) + .severity(SeverityId::Low) + .status(StatusId::Failure) + .message(format!("SSH connection failed: {err}")) + .build() + ); + } + }); + } + Ok(()) +} + #[allow(clippy::too_many_arguments)] -async fn handle_connection( - stream: tokio::net::UnixStream, +async fn handle_connection( + stream: S, config: Arc, port_forward: Arc, boundary_exec: Arc, main_session: Option>, peer_timeout: Duration, -) -> Result<()> { - // Access is gated by the Unix-socket filesystem permissions (root-only), - // not by an application-level preface. The supervisor bridges the - // gateway's RelayStream directly into this socket. +) -> Result<()> +where + S: tokio::io::AsyncRead + tokio::io::AsyncWrite + Unpin + Send + 'static, +{ + // Access is gated by the Unix-socket filesystem permissions (root-only) or, + // on Windows, by the stream never leaving the supervisor process. There is + // no application-level preface. The supervisor bridges the gateway's + // RelayStream directly into this stream. ocsf_emit!( SshActivityBuilder::new(openshell_ocsf::ctx::ctx()) .activity(ActivityId::Open) @@ -352,7 +556,11 @@ async fn handle_connection( .disposition(DispositionId::Allowed) .severity(SeverityId::Informational) .status(StatusId::Success) - .message("SSH connection accepted on supervisor Unix socket") + .message(if cfg!(unix) { + "SSH connection accepted on supervisor Unix socket" + } else { + "SSH connection accepted on supervisor in-process transport" + }) .build() ); @@ -1530,7 +1738,7 @@ fn direct_tcpip_target( openshell_isolation_interface::contract::LoopbackTarget::new(ip, port).ok() } -#[cfg(test)] +#[cfg(all(test, unix))] #[allow( clippy::doc_markdown, reason = "Test documentation references protocol and API identifiers." diff --git a/crates/openshell-supervisor-process/src/supervisor_session.rs b/crates/openshell-supervisor-process/src/supervisor_session.rs index 898af8672d..19240230e6 100644 --- a/crates/openshell-supervisor-process/src/supervisor_session.rs +++ b/crates/openshell-supervisor-process/src/supervisor_session.rs @@ -968,15 +968,19 @@ async fn open_target( expected_ssh_peer_pid: Option, ) -> Result, Box> { #[cfg(not(unix))] - let _ = (ssh_socket_path, expected_ssh_peer_pid); + let _ = expected_ssh_peer_pid; match relay_open.target.as_ref() { Some(relay_open::Target::Tcp(target)) => open_tcp_target(target, port_forward).await, Some(relay_open::Target::Ssh(_)) | None => { if ssh_socket_path.as_os_str().is_empty() { return Err("SSH access is not configured for this supervisor".into()); } + // The SSH server runs in this process, so there is no peer to + // authenticate: the stream never leaves the supervisor. #[cfg(not(unix))] - return Err("SSH relay targets are unsupported by the Windows supervisor".into()); + return Ok(Box::new( + crate::ssh::connect_in_process(ssh_socket_path).await?, + )); #[cfg(unix)] { let runtime_path = crate::unix_socket::runtime_path(ssh_socket_path); diff --git a/crates/openshell-supervisor/src/lib.rs b/crates/openshell-supervisor/src/lib.rs index 212cdc5366..442c20bfac 100644 --- a/crates/openshell-supervisor/src/lib.rs +++ b/crates/openshell-supervisor/src/lib.rs @@ -769,7 +769,7 @@ async fn run_sandbox_with_backend( } = config; // The current client attachment adapter is SSH; retaining a process's I/O // internally does not make it reachable by a foreground client. - if await_main_process_attachment && (!cfg!(unix) || ssh_socket_path.is_none()) { + if await_main_process_attachment && ssh_socket_path.is_none() { return Err(miette::miette!( "foreground attachment to the main process requires a supported SSH access adapter; create the sandbox with --detach" )); @@ -819,7 +819,6 @@ async fn run_sandbox_with_backend( &auth_bundle, )?; let sandbox_bearer = openshell_core::grpc_client::install_supervisor_auth_bundle(&auth_bundle)?; - #[cfg(unix)] let ssh_host_key = if ssh_socket_path.is_some() { Some(openshell_supervisor_process::ssh::parse_host_key( auth_bundle.ssh_host_private_key.as_ref(), @@ -827,15 +826,6 @@ async fn run_sandbox_with_backend( } else { None }; - #[cfg(not(unix))] - let ssh_host_key = { - if ssh_socket_path.is_some() { - return Err(miette::miette!( - "SSH access sockets are unsupported on this host" - )); - } - None - }; // Startup joins the trace that created the sandbox when the driver passes // one, and ends once the access plane is up. let startup = tracing::info_span!( From 70acd7d313e6220cb14ec573dc305fdb4d83d531 Mon Sep 17 00:00:00 2001 From: "Geoffrey Trousdale (they/them)" Date: Wed, 4 Nov 2026 18:01:30 -0500 Subject: [PATCH 2/3] fix(mxc): run Windows sandbox commands as written - MXC boundary: pass the tail after `cmd.exe /c` verbatim (with /s and outer quotes) for both exec and the main process, because cmd.exe does not use CRT argument quoting. - MXC driver: honor a generic `create -- ` and default the working directory to a per-sandbox temp directory when the template sets none. - CLI/core: escape the SSH ProxyCommand for Windows and split Windows sandbox paths correctly. Signed-off-by: Geoffrey Trousdale (they/them) --- crates/openshell-cli/src/ssh.rs | 20 ++-- crates/openshell-core/src/forward.rs | 98 ++++++++++++++++++-- crates/openshell-driver-mxc/src/driver.rs | 77 ++++++++++++--- crates/openshell-mxc-boundary/src/runtime.rs | 76 ++++++++++++++- 4 files changed, 237 insertions(+), 34 deletions(-) diff --git a/crates/openshell-cli/src/ssh.rs b/crates/openshell-cli/src/ssh.rs index a576d5a788..fad8a750f5 100644 --- a/crates/openshell-cli/src/ssh.rs +++ b/crates/openshell-cli/src/ssh.rs @@ -14,8 +14,9 @@ use miette::{IntoDiagnostic, Report, Result, WrapErr}; use nix::sys::signal::Signal; use openshell_core::driver_mounts; use openshell_core::forward::{ - ForwardSpec, build_proxy_command, format_gateway_url, resolve_ssh_gateway, sandbox_ssh_args, - shell_escape, validate_ssh_session_response, write_forward_pid, + ForwardSpec, build_proxy_command, format_gateway_url, proxy_command_escape, + resolve_ssh_gateway, sandbox_ssh_args, shell_escape, validate_ssh_session_response, + write_forward_pid, }; use openshell_core::proto::{ CreateSshSessionRequest, GetSandboxRequest, SandboxPhase, SshRelayTarget, TcpForwardFrame, @@ -1334,8 +1335,11 @@ fn ssh_upload_command(proxy_command: &str) -> Command { /// `"/sandbox/sub/file"` -> `("/sandbox/sub", "file")` /// `"file.txt"` -> `(".", "file.txt")` fn split_sandbox_path(path: &str) -> (&str, &str) { - match path.rfind('/') { - Some(0) => ("/", &path[1..]), + match path.rfind(['/', '\\']) { + Some(0) => (&path[..1], &path[1..]), + Some(pos) if pos == 2 && path.as_bytes().get(1) == Some(&b':') => { + (&path[..=pos], &path[pos + 1..]) + } Some(pos) => (&path[..pos], &path[pos + 1..]), None => (".", path), } @@ -2126,13 +2130,13 @@ fn host_alias(name: &str, workspace: &str) -> String { fn render_ssh_config(gateway: &str, name: &str, workspace: &str) -> String { let exe = std::env::current_exe().expect("failed to resolve OpenShell executable"); - let exe = shell_escape(&exe.to_string_lossy()); + let exe = proxy_command_escape(&exe.to_string_lossy()); let proxy_cmd = format!( "{exe} ssh-proxy --gateway-name {} --name {} --workspace {}", - shell_escape(gateway), - shell_escape(name), - shell_escape(workspace), + proxy_command_escape(gateway), + proxy_command_escape(name), + proxy_command_escape(workspace), ); let host_alias = host_alias(name, workspace); format!( diff --git a/crates/openshell-core/src/forward.rs b/crates/openshell-core/src/forward.rs index c8e6c10131..e7f6caa06a 100644 --- a/crates/openshell-core/src/forward.rs +++ b/crates/openshell-core/src/forward.rs @@ -906,7 +906,7 @@ pub fn format_gateway_url(scheme: &str, host: &str, port: u16) -> String { format!("{scheme}://{}:{port}", bracket_ipv6_host(host)) } -/// Shell-escape a value for use inside a `ProxyCommand` string. +/// POSIX shell-escape a value for use in command strings. pub fn shell_escape(value: &str) -> String { if value.is_empty() { return "''".to_string(); @@ -923,11 +923,55 @@ pub fn shell_escape(value: &str) -> String { format!("'{escaped}'") } +/// Quote one argument for the Windows command-line parser used by +/// `CreateProcessW`-launched programs. +/// +/// OpenSSH executes `ProxyCommand` directly on Windows instead of passing it +/// through `/bin/sh`. Always quoting each argument keeps executable paths with +/// spaces intact. Backslashes immediately before a quote or the closing quote +/// are doubled according to the Windows C runtime argument parsing rules. +#[cfg(any(windows, test))] +fn windows_command_line_escape(value: &str) -> String { + let mut escaped = String::with_capacity(value.len() + 2); + escaped.push('"'); + let mut backslashes = 0; + for character in value.chars() { + if character == '\\' { + backslashes += 1; + continue; + } + let count = if character == '"' { + backslashes * 2 + 1 + } else { + backslashes + }; + escaped.extend(std::iter::repeat_n('\\', count)); + backslashes = 0; + escaped.push(character); + } + escaped.extend(std::iter::repeat_n('\\', backslashes * 2)); + escaped.push('"'); + escaped +} + +/// Escape one argument for the command-launch behavior OpenSSH uses for +/// `ProxyCommand` on the current host platform. +pub fn proxy_command_escape(value: &str) -> String { + #[cfg(not(windows))] + { + shell_escape(value) + } + #[cfg(windows)] + { + windows_command_line_escape(value) + } +} + /// Build the SSH `ProxyCommand` string used to tunnel to a sandbox. /// -/// Every interpolated argument is shell-escaped so that server-supplied values -/// (gateway URL, sandbox name, workspace, token, gateway name) cannot inject shell -/// metacharacters into the command that OpenSSH executes via `/bin/sh -c`. +/// On Unix, every interpolated argument is shell-escaped for the `/bin/sh -c` +/// invocation used by OpenSSH. On Windows, every argument is quoted for the +/// command line that OpenSSH launches through `CreateProcessW`. pub fn build_proxy_command( exe: &str, gateway_url: &str, @@ -938,12 +982,12 @@ pub fn build_proxy_command( ) -> String { format!( "{} ssh-proxy --gateway {} --sandbox {} --workspace {} --token {} --gateway-name {}", - shell_escape(exe), - shell_escape(gateway_url), - shell_escape(sandbox_name), - shell_escape(workspace), - shell_escape(token), - shell_escape(gateway_name), + proxy_command_escape(exe), + proxy_command_escape(gateway_url), + proxy_command_escape(sandbox_name), + proxy_command_escape(workspace), + proxy_command_escape(token), + proxy_command_escape(gateway_name), ) } @@ -1346,6 +1390,7 @@ mod tests { } } + #[cfg(not(windows))] #[test] fn build_proxy_command_escapes_shell_metacharacters() { // Attacker-controlled values in every escapable position. @@ -1369,6 +1414,7 @@ mod tests { assert!(!outside_single_quotes(&cmd).contains('\n')); } + #[cfg(not(windows))] #[test] fn build_proxy_command_empty_values_quote_rather_than_vanish() { // An empty value must become `''` rather than disappearing — otherwise @@ -1378,6 +1424,7 @@ mod tests { assert!(cmd.contains("--workspace ''")); } + #[cfg(not(windows))] #[test] fn build_proxy_command_safe_values_pass_through_unquoted() { let cmd = build_proxy_command( @@ -1411,8 +1458,39 @@ mod tests { } } + #[test] + fn windows_command_line_escape_round_trips_through_crt_parsing() { + assert_eq!(windows_command_line_escape(""), r#""""#); + assert_eq!( + windows_command_line_escape(r"C:\Program Files\openshell.exe"), + r#""C:\Program Files\openshell.exe""# + ); + assert_eq!(windows_command_line_escape(r#"a"b"#), r#""a\"b""#); + assert_eq!(windows_command_line_escape(r#"a\"b"#), r#""a\\\"b""#); + assert_eq!(windows_command_line_escape(r"dir\"), r#""dir\\""#); + assert_eq!(windows_command_line_escape(r"a\b"), r#""a\b""#); + } + + #[cfg(windows)] + #[test] + fn build_proxy_command_quotes_every_windows_argument() { + let cmd = build_proxy_command( + r"C:\Program Files\openshell.exe", + "gw", + "", + "ws & calc", + "tok", + "name", + ); + assert_eq!( + cmd, + r#""C:\Program Files\openshell.exe" ssh-proxy --gateway "gw" --sandbox "" --workspace "ws & calc" --token "tok" --gateway-name "name""# + ); + } + /// Helper: return the concatenation of characters that appear outside /// POSIX single-quoted runs. Used by the metacharacter assertions above. + #[cfg(not(windows))] fn outside_single_quotes(s: &str) -> String { let mut out = String::new(); let mut inside = false; diff --git a/crates/openshell-driver-mxc/src/driver.rs b/crates/openshell-driver-mxc/src/driver.rs index 86b0f5dba3..d77adbd9c2 100644 --- a/crates/openshell-driver-mxc/src/driver.rs +++ b/crates/openshell-driver-mxc/src/driver.rs @@ -1014,23 +1014,34 @@ async fn set_failed(context: &LifecycleContext, message: &str) { } fn sandbox_config(sandbox: &DriverSandbox) -> Result { - let config = sandbox + let driver_config = sandbox .spec .as_ref() .and_then(|spec| spec.template.as_ref()) - .and_then(|template| template.driver_config.as_ref()) - .ok_or_else(|| { - tonic::Status::invalid_argument( - "mxc requires template.driver_config.mxc with command and cwd", - ) - })?; - let config: MxcSandboxConfig = - serde_json::from_value(struct_to_json_value(config)).map_err(|error| { + .and_then(|template| template.driver_config.as_ref()); + let config = match driver_config { + // The MXC-specific `--driver-config-json` is the most deliberately + // targeted input, so it takes priority over the generic command. + Some(config) => serde_json::from_value(struct_to_json_value(config)).map_err(|error| { tonic::Status::invalid_argument(format!("invalid mxc driver_config: {error}")) - })?; + })?, + // The driver-agnostic `sandbox create -- ` syntax. The working + // directory also hosts boundary state, so it must be writable by the + // gateway user; use a per-sandbox directory under its temp folder. + None => MxcSandboxConfig { + command: sandbox + .spec + .as_ref() + .map(|spec| spec.command.clone()) + .unwrap_or_default(), + cwd: default_sandbox_cwd(&sandbox.id)?, + denied_paths: Vec::new(), + }, + }; if config.command.first().is_none_or(String::is_empty) { return Err(tonic::Status::invalid_argument( - "mxc driver_config.command must contain an executable", + "mxc sandbox command must contain an executable: set it with \ + `sandbox create -- ` or `--driver-config-json`", )); } let mut denied_paths = Vec::with_capacity(config.denied_paths.len()); @@ -1360,6 +1371,19 @@ fn path_key(path: &str) -> Vec { .collect() } +fn default_sandbox_cwd(sandbox_id: &str) -> Result { + let directory = std::env::temp_dir() + .join("openshell-mxc") + .join(safe_component(sandbox_id)?); + std::fs::create_dir_all(&directory).map_err(|error| { + tonic::Status::internal(format!( + "create default MXC working directory {}: {error}", + directory.display() + )) + })?; + Ok(directory.display().to_string()) +} + fn launch_authentication( sandbox: &DriverSandbox, ) -> Result { @@ -1668,6 +1692,37 @@ fn platform_event(sandbox_id: String, reason: &str, message: String) -> WatchSan // the driver. Keep lifecycle/security tests below during their migration. #[cfg(test)] mod tests { + #[test] + fn generic_command_is_used_without_driver_config() { + let sandbox = DriverSandbox { + id: "generic-command-sandbox".to_string(), + spec: Some(DriverSandboxSpec { + command: vec!["cmd.exe".into(), "/c".into(), "exit".into()], + ..Default::default() + }), + ..Default::default() + }; + let config = sandbox_config(&sandbox).expect("generic command is accepted"); + assert_eq!(config.command, vec!["cmd.exe", "/c", "exit"]); + let cwd = Path::new(&config.cwd); + assert!(cwd.is_absolute() && cwd.is_dir(), "{}", config.cwd); + assert!(cwd.ends_with("generic-command-sandbox")); + let _ = std::fs::remove_dir(cwd); + + let empty = DriverSandbox { + id: "empty-command-sandbox".to_string(), + spec: Some(DriverSandboxSpec::default()), + ..Default::default() + }; + let error = sandbox_config(&empty).expect_err("a command is required"); + assert!(error.message().contains("sandbox create -- ")); + let _ = std::fs::remove_dir( + std::env::temp_dir() + .join("openshell-mxc") + .join("empty-command-sandbox"), + ); + } + #[test] fn mxc_driver_command_reaches_supervisor_without_generic_command() { let command = vec![ diff --git a/crates/openshell-mxc-boundary/src/runtime.rs b/crates/openshell-mxc-boundary/src/runtime.rs index 08eb49c759..51873ae02f 100644 --- a/crates/openshell-mxc-boundary/src/runtime.rs +++ b/crates/openshell-mxc-boundary/src/runtime.rs @@ -873,12 +873,13 @@ impl BoundaryRuntime { ); let process_id = format!("{}:main:0", self.config.generation); let (program, args) = agent_command(spec.clone()); + let (args, raw_command_tail) = cmd_verbatim_tail(&program, args); let process = match ManagedProcess::spawn( process_id.clone(), ProcessKindWire::Main, program, args, - None, + raw_command_tail, spec.workdir.clone(), environment, ) @@ -1247,14 +1248,15 @@ fn windows_launch(spec: ExecSpecWire) -> WindowsLaunch { }; }; if let Some(parsed) = crate::posix_command::parse(&command) { - let mut argv = parsed.argv.into_iter(); - let program = argv.next().unwrap_or_default(); + let mut words = parsed.argv.into_iter(); + let program = words.next().unwrap_or_default(); let mut merged_env = env; merged_env.extend(parsed.env); + let (args, raw_command_tail) = cmd_verbatim_tail(&program, words.collect()); return WindowsLaunch { program, - args: argv.collect(), - raw_command_tail: None, + args, + raw_command_tail, workdir: parsed.workdir.or(workdir), env: merged_env, }; @@ -1268,6 +1270,26 @@ fn windows_launch(spec: ExecSpecWire) -> WindowsLaunch { } } +/// `cmd.exe /c` treats the rest of its command line as command text and does not +/// apply the C runtime quoting rules, so a quoted path such as `>"C:\x.txt"` +/// would reach it as `\"`. Pass everything after `/c` or `/k` verbatim, wrapped +/// in `/s` quotes, as a terminal would. Other programs keep their arguments. +fn cmd_verbatim_tail(program: &str, args: Vec) -> (Vec, Option) { + let is_cmd = Path::new(program) + .file_stem() + .is_some_and(|stem| stem.eq_ignore_ascii_case("cmd")); + let command_flag = args + .iter() + .position(|arg| arg.eq_ignore_ascii_case("/c") || arg.eq_ignore_ascii_case("/k")); + let (true, Some(flag)) = (is_cmd, command_flag) else { + return (args, None); + }; + let mut args = args; + let text = args.split_off(flag + 1).join(" "); + args.insert(flag, "/s".to_string()); + (args, Some(format!("\"{text}\""))) +} + /// The `cmd.exe` launch used when a shell request's program cannot be started /// directly, as a POSIX shell would report `command not found` itself. It also /// resolves `cmd.exe` built-ins such as `echo`, `dir`, and `type`, and returns @@ -1887,6 +1909,50 @@ mod tests { assert_eq!(launch.args, vec!["/d", "/q"]); } + #[test] + fn cmd_command_text_is_passed_verbatim() { + let launch = windows_launch(shell_exec(Some( + r#"'C:\Windows\System32\cmd.exe' /v:on /c '>"C:\dir\out.txt" echo !value!'"#, + ))); + assert_eq!(launch.program, r"C:\Windows\System32\cmd.exe"); + assert_eq!(launch.args, vec!["/v:on", "/s", "/c"]); + assert_eq!( + launch.raw_command_tail.as_deref(), + Some(r#"">"C:\dir\out.txt" echo !value!""#) + ); + assert_eq!(cmd_retry(&launch), None, "cmd launches are not retried"); + } + + #[tokio::test] + async fn cmd_writes_a_quoted_path() { + let directory = tempfile::tempdir().expect("temporary directory"); + let path = directory.path().join("out file.txt"); + let command = format!( + "cmd.exe /c {}", + openshell_core::forward::shell_escape(&format!(">\"{}\" echo written", path.display())) + ); + let launch = windows_launch(shell_exec(Some(&command))); + let process = ManagedProcess::spawn( + "cmd-quoted-path".to_string(), + ProcessKindWire::Exec, + launch.program, + launch.args, + launch.raw_command_tail, + None, + HashMap::new(), + ) + .await + .expect("cmd starts"); + let status = tokio::time::timeout(Duration::from_secs(10), process.wait()) + .await + .expect("process exits"); + assert!(matches!(status, ExitStatusWire::Exited(0)), "{status:?}"); + assert_eq!( + std::fs::read_to_string(&path).expect("file written").trim(), + "written" + ); + } + #[test] fn argv_exec_is_unchanged() { let mut spec = shell_exec(None); From 4115758dcb0a697da20964330d19df7e63ab22e2 Mon Sep 17 00:00:00 2001 From: "Geoffrey Trousdale (they/them)" Date: Wed, 4 Nov 2026 12:03:35 -0500 Subject: [PATCH 3/3] feat(mxc): restart stopped sandboxes StartSandbox returned Unimplemented. Stop already tears the runtime down completely (for an isolation session it also deprovisions the agent user), so start reruns the sandbox lifecycle from the stored spec with the fresh launch credentials the gateway supplies. Each start gets its own runtime generation and state directories; the previous generation's directories are removed. Host paths named by the policy, such as the workspace, persist across the restart. A start of a sandbox that is already starting or running succeeds without relaunching. The isolation-session agent user is not preserved across a restart: a new agent user is provisioned for each start. Also fix the Windows restart counter fixture: cmd includes the space before `&` in echoed text, so the counter read "2 " instead of "2". Signed-off-by: Geoffrey Trousdale (they/them) --- crates/openshell-driver-mxc/src/driver.rs | 113 ++++++++++++++++++++++ crates/openshell-driver-mxc/src/grpc.rs | 41 ++++++-- 2 files changed, 146 insertions(+), 8 deletions(-) diff --git a/crates/openshell-driver-mxc/src/driver.rs b/crates/openshell-driver-mxc/src/driver.rs index d77adbd9c2..0d31462aa2 100644 --- a/crates/openshell-driver-mxc/src/driver.rs +++ b/crates/openshell-driver-mxc/src/driver.rs @@ -539,6 +539,119 @@ impl MxcComputeBackend { Ok(()) } + /// Restart a stopped sandbox by running its lifecycle again with the fresh + /// launch credentials the gateway supplies for this start. + /// + /// Stop tears the runtime down completely; for an isolation session that + /// includes deprovisioning the agent user. Restart therefore provisions a + /// new runtime generation (and, for an isolation session, a new agent user) + /// from the stored sandbox spec. Host paths named by the policy, such as the + /// workspace, are unaffected by stop and so persist across the restart. + pub async fn start_sandbox( + &self, + sandbox_id: &str, + sandbox_name: &str, + fresh_launch_authentication: Vec, + ) -> Result<(), tonic::Status> { + if fresh_launch_authentication.is_empty() { + return Err(tonic::Status::invalid_argument( + "launch authentication is required to start an MXC sandbox", + )); + } + let gate = { + let registry = self.registry.lock().await; + let entry = registry.get(sandbox_id).ok_or_else(|| { + tonic::Status::not_found(format!("sandbox {sandbox_name} not found")) + })?; + if entry.sandbox.name != sandbox_name { + return Err(tonic::Status::failed_precondition( + "sandbox_id did not match sandbox_name", + )); + } + entry.lifecycle_gate.clone() + }; + let startup_guard = gate.lock_owned().await; + + let (mut sandbox, previous_host, previous_boundary) = { + let registry = self.registry.lock().await; + let entry = registry.get(sandbox_id).ok_or_else(|| { + tonic::Status::not_found(format!("sandbox {sandbox_name} not found")) + })?; + match entry.phase_state { + PhaseState::Stopped | PhaseState::Failed(_) => {} + // A retried start of a sandbox that is already up is not an error. + PhaseState::Running | PhaseState::Starting => return Ok(()), + } + ( + entry.sandbox.clone(), + entry.host_state_dir.clone(), + entry.boundary_state_dir.clone(), + ) + }; + if let Some(spec) = sandbox.spec.as_mut() { + spec.launch_authentication = fresh_launch_authentication; + } else { + return Err(tonic::Status::failed_precondition( + "stored MXC sandbox has no spec to restart", + )); + } + let launch = launch_authentication(&sandbox)?; + launch + .validate() + .map_err(|error| tonic::Status::failed_precondition(error.to_string()))?; + let sandbox_config = sandbox_config(&sandbox)?; + let generation = safe_component(launch.supervisor.runtime_generation.as_str())?.to_string(); + let host_state_dir = self + .config + .state_dir + .join(safe_component(sandbox_id)?) + .join(&generation); + let boundary_state_dir = PathBuf::from(&sandbox_config.cwd) + .join(".openshell-runtime") + .join(&generation); + if previous_host != host_state_dir { + cleanup_runtime_directory(&previous_host); + } + if previous_boundary != boundary_state_dir { + cleanup_runtime_directory(&previous_boundary); + } + + let starting = make_sandbox_with_condition( + &sandbox, + &condition("Ready", "False", "Starting", "MXC runtime restarting"), + false, + ); + { + let mut registry = self.registry.lock().await; + let entry = registry.get_mut(sandbox_id).ok_or_else(|| { + tonic::Status::not_found(format!("sandbox {sandbox_name} not found")) + })?; + entry.sandbox = starting.clone(); + entry.phase_state = PhaseState::Starting; + entry.shutdown_tx = None; + entry.terminated_rx = None; + entry.host_state_dir.clone_from(&host_state_dir); + entry.boundary_state_dir.clone_from(&boundary_state_dir); + } + let _ = self.watch_tx.send(sandbox_event(starting)); + let context = LifecycleContext { + invoker: self.invoker.clone(), + config: self.config.clone(), + gateway: self.gateway.clone(), + registry: self.registry.clone(), + watch_tx: self.watch_tx.clone(), + attribution: self.attribution.clone(), + sandbox, + sandbox_config, + generation, + host_state_dir, + boundary_state_dir, + policy_mapper: self.policy_mapper.clone(), + }; + tokio::spawn(async move { run_lifecycle(context, startup_guard).await }); + Ok(()) + } + pub async fn delete_sandbox( &self, sandbox_id: &str, diff --git a/crates/openshell-driver-mxc/src/grpc.rs b/crates/openshell-driver-mxc/src/grpc.rs index 4c21172c47..60b31012ab 100644 --- a/crates/openshell-driver-mxc/src/grpc.rs +++ b/crates/openshell-driver-mxc/src/grpc.rs @@ -127,11 +127,16 @@ impl ComputeDriver for ComputeDriverService { async fn start_sandbox( &self, - _request: Request, + request: Request, ) -> Result, Status> { - Err(Status::unimplemented( - "mxc driver does not support restarting stopped sandboxes", - )) + let req = request.into_inner(); + if req.sandbox_id.is_empty() || req.name.is_empty() { + return Err(Status::invalid_argument("sandbox_id and name are required")); + } + self.backend + .start_sandbox(&req.sandbox_id, &req.name, req.launch_authentication) + .await?; + Ok(Response::new(StartSandboxResponse::default())) } async fn delete_sandbox( @@ -185,7 +190,7 @@ mod tests { use crate::driver::MxcComputeConfig; #[tokio::test] - async fn start_sandbox_reports_one_shot_lifecycle() { + async fn start_sandbox_validates_the_request() { let service = ComputeDriverService::new(MxcComputeBackend::new( openshell_core::config::DEFAULT_GATEWAY_NAME, MxcComputeConfig::default(), @@ -194,10 +199,30 @@ mod tests { let error = service .start_sandbox(Request::new(StartSandboxRequest::default())) .await - .expect_err("MXC must not restart a stopped one-shot workload"); + .expect_err("a start without a sandbox identity is rejected"); + assert_eq!(error.code(), tonic::Code::InvalidArgument); - assert_eq!(error.code(), tonic::Code::Unimplemented); - assert!(error.message().contains("does not support restarting")); + let error = service + .start_sandbox(Request::new(StartSandboxRequest { + sandbox_id: "missing".to_string(), + name: "missing".to_string(), + launch_authentication: Vec::new(), + ..Default::default() + })) + .await + .expect_err("a start without fresh launch credentials is rejected"); + assert_eq!(error.code(), tonic::Code::InvalidArgument); + + let error = service + .start_sandbox(Request::new(StartSandboxRequest { + sandbox_id: "missing".to_string(), + name: "missing".to_string(), + launch_authentication: b"{}".to_vec(), + ..Default::default() + })) + .await + .expect_err("an unknown sandbox cannot be started"); + assert_eq!(error.code(), tonic::Code::NotFound); } #[tokio::test]