From 454829f3a039452cd402e19a94b3c96a055a7c6e Mon Sep 17 00:00:00 2001 From: Russell Bryant Date: Wed, 3 Jun 2026 18:04:27 -0400 Subject: [PATCH 1/3] feat(cli): add --env flag to sandbox create and exec Add `--env KEY=VALUE` (repeatable) to `openshell sandbox create` and `openshell sandbox exec`, wiring user-specified environment variables into the existing `SandboxSpec.environment` and `ExecSandboxRequest.environment` protobuf fields which were previously only accessible via the SDK/gRPC API. Fix a bug where user-specified environment variables set via `SandboxSpec.environment` were silently dropped inside sandbox SSH sessions. The SSH handler's `apply_child_env()` calls `env_clear()` to strip supervisor-internal secrets before spawning user shells, but this also wiped container-level env vars including user-specified ones. This was introduced in 5fd4885a (2026-02-26, "feat(sandbox): VS Code Remote-SSH support with platform detection fix") which added `env_clear()` to fix a VS Code platform detection timeout caused by leaked supervisor env vars. The fix introduces an `OPENSHELL_USER_ENVIRONMENT` sidecar env var: compute drivers (Docker, Podman, Kubernetes, VM) serialize the user-specified environment as JSON into this variable at container creation time, the sandbox supervisor deserializes it at startup, and the SSH handler re-injects the values into child processes after `env_clear()`. User-specified vars are applied at lowest priority so proxy, TLS, and provider credentials still take precedence. Signed-off-by: Russell Bryant --- Cargo.lock | 1 + crates/openshell-cli/src/main.rs | 16 ++++ crates/openshell-cli/src/run.rs | 22 ++++-- .../sandbox_create_lifecycle_integration.rs | 77 +++++++++++++++++++ crates/openshell-core/src/sandbox_env.rs | 7 ++ crates/openshell-driver-docker/Cargo.toml | 1 + crates/openshell-driver-docker/src/lib.rs | 14 +++- .../openshell-driver-kubernetes/src/driver.rs | 7 ++ .../openshell-driver-podman/src/container.rs | 14 +++- crates/openshell-driver-vm/src/driver.rs | 11 ++- crates/openshell-sandbox/src/lib.rs | 9 +++ crates/openshell-sandbox/src/ssh.rs | 22 ++++++ 12 files changed, 191 insertions(+), 10 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 4bc657be3..d0cd77f85 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3482,6 +3482,7 @@ dependencies = [ "futures", "openshell-core", "serde", + "serde_json", "tar", "temp-env", "tempfile", diff --git a/crates/openshell-cli/src/main.rs b/crates/openshell-cli/src/main.rs index 21f15834b..802555410 100644 --- a/crates/openshell-cli/src/main.rs +++ b/crates/openshell-cli/src/main.rs @@ -1267,6 +1267,10 @@ enum SandboxCommands { #[arg(long = "label")] labels: Vec, + /// Environment variables to inject into the sandbox (KEY=VALUE format, repeatable). + #[arg(long = "env", value_name = "KEY=VALUE")] + envs: Vec, + /// Approval mode for agent-authored policy proposals. /// /// `manual` (default): every proposal lands in the draft inbox for @@ -1373,6 +1377,10 @@ enum SandboxCommands { #[arg(long, overrides_with = "tty")] no_tty: bool, + /// Environment variables to set for the command (KEY=VALUE format, repeatable). + #[arg(long = "env", value_name = "KEY=VALUE")] + envs: Vec, + /// Command and arguments to execute. #[arg(required = true, trailing_var_arg = true, allow_hyphen_values = true)] command: Vec, @@ -2549,6 +2557,7 @@ async fn main() -> Result<()> { auto_providers, no_auto_providers, labels, + envs, approval_mode, command, } => { @@ -2583,6 +2592,9 @@ async fn main() -> Result<()> { labels_map.insert(parts[0].to_string(), parts[1].to_string()); } + // Parse --env flags into a HashMap. + let env_map = run::parse_env_pairs(&envs)?; + // Parse --upload specs into [(local_path, sandbox_path, git_ignore)]. let upload_specs: Vec<(String, Option, bool)> = upload .iter() @@ -2629,6 +2641,7 @@ async fn main() -> Result<()> { tty_override, auto_providers_override, &labels_map, + &env_map, &approval_mode, &tls, )) @@ -2741,6 +2754,7 @@ async fn main() -> Result<()> { timeout, tty, no_tty, + envs, command, } => { let name = resolve_sandbox_name(name, &ctx.name)?; @@ -2752,6 +2766,7 @@ async fn main() -> Result<()> { } else { None // auto-detect }; + let env_map = run::parse_key_value_pairs(&envs, "--env")?; let exit_code = run::sandbox_exec_grpc( endpoint, &name, @@ -2759,6 +2774,7 @@ async fn main() -> Result<()> { workdir.as_deref(), timeout, tty_override, + &env_map, &tls, ) .await?; diff --git a/crates/openshell-cli/src/run.rs b/crates/openshell-cli/src/run.rs index 3b2adfc59..c570cfb3c 100644 --- a/crates/openshell-cli/src/run.rs +++ b/crates/openshell-cli/src/run.rs @@ -1709,6 +1709,7 @@ pub async fn sandbox_create( tty_override: Option, auto_providers_override: Option, labels: &HashMap, + environment: &HashMap, approval_mode: &str, tls: &TlsOptions, ) -> Result<()> { @@ -1785,6 +1786,7 @@ pub async fn sandbox_create( spec: Some(SandboxSpec { gpu: requested_gpu, gpu_device: gpu_device.unwrap_or_default().to_string(), + environment: environment.clone(), policy, providers: configured_providers, template, @@ -2587,6 +2589,7 @@ const MAX_STDIN_PAYLOAD: usize = 4 * 1024 * 1024; /// Execute a command in a running sandbox via gRPC, streaming output to the terminal. /// /// Returns the remote command's exit code. +#[allow(clippy::too_many_arguments, clippy::implicit_hasher)] pub async fn sandbox_exec_grpc( server: &str, name: &str, @@ -2594,6 +2597,7 @@ pub async fn sandbox_exec_grpc( workdir: Option<&str>, timeout_seconds: u32, tty_override: Option, + environment: &HashMap, tls: &TlsOptions, ) -> Result { let mut client = grpc_client(server, tls).await?; @@ -2648,8 +2652,15 @@ pub async fn sandbox_exec_grpc( .unwrap_or_else(|| std::io::stdin().is_terminal() && std::io::stdout().is_terminal()); if tty_override == Some(true) && std::io::stdin().is_terminal() { - return sandbox_exec_interactive_grpc(client, &sandbox, command, workdir, timeout_seconds) - .await; + return sandbox_exec_interactive_grpc( + client, + &sandbox, + command, + workdir, + timeout_seconds, + environment, + ) + .await; } // Make the streaming gRPC call. @@ -2658,7 +2669,7 @@ pub async fn sandbox_exec_grpc( sandbox_id: sandbox.object_id().to_string(), command: command.to_vec(), workdir: workdir.unwrap_or_default().to_string(), - environment: HashMap::new(), + environment: environment.clone(), timeout_seconds, stdin: stdin_payload, tty, @@ -3004,6 +3015,7 @@ async fn sandbox_exec_interactive_grpc( command: &[String], workdir: Option<&str>, timeout_seconds: u32, + environment: &HashMap, ) -> Result { use openshell_core::proto::{ExecSandboxInput, ExecSandboxWindowResize, exec_sandbox_input}; use tokio_stream::wrappers::ReceiverStream; @@ -3019,7 +3031,7 @@ async fn sandbox_exec_interactive_grpc( sandbox_id: sandbox.object_id().to_string(), command: command.to_vec(), workdir: workdir.unwrap_or_default().to_string(), - environment: HashMap::new(), + environment: environment.clone(), timeout_seconds, stdin: Vec::new(), tty: true, @@ -3837,7 +3849,7 @@ async fn auto_create_provider( Ok(()) } -fn parse_key_value_pairs(items: &[String], flag: &str) -> Result> { +pub fn parse_key_value_pairs(items: &[String], flag: &str) -> Result> { let mut map = HashMap::new(); for item in items { diff --git a/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs b/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs index 828dbd998..e1c3d8e89 100644 --- a/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs +++ b/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs @@ -794,6 +794,7 @@ async fn sandbox_create_keeps_command_sessions_by_default() { Some(false), Some(false), &HashMap::new(), + &HashMap::new(), "manual", &tls, ) @@ -836,6 +837,7 @@ async fn sandbox_create_sends_cpu_and_memory_limits_only() { Some(false), Some(false), &HashMap::new(), + &HashMap::new(), "manual", &tls, ) @@ -913,6 +915,7 @@ async fn sandbox_create_does_not_infer_command_providers_when_v2_enabled() { Some(true), Some(false), &HashMap::new(), + &HashMap::new(), "manual", &tls, ) @@ -970,6 +973,7 @@ async fn sandbox_create_returns_vm_error_without_waiting_for_timeout() { Some(false), Some(false), &HashMap::new(), + &HashMap::new(), "manual", &tls, ) @@ -1023,6 +1027,7 @@ async fn sandbox_create_keeps_waiting_while_vm_progress_arrives() { Some(false), Some(false), &HashMap::new(), + &HashMap::new(), "manual", &tls, ) @@ -1068,6 +1073,7 @@ async fn sandbox_create_times_out_when_only_logs_arrive() { Some(false), Some(false), &HashMap::new(), + &HashMap::new(), "manual", &tls, ) @@ -1109,6 +1115,7 @@ async fn sandbox_create_deletes_command_sessions_with_no_keep() { Some(false), Some(false), &HashMap::new(), + &HashMap::new(), "manual", &tls, ) @@ -1154,6 +1161,7 @@ async fn sandbox_create_deletes_shell_sessions_with_no_keep() { Some(true), Some(false), &HashMap::new(), + &HashMap::new(), "manual", &tls, ) @@ -1199,6 +1207,7 @@ async fn sandbox_create_keeps_sandbox_with_hidden_keep_flag() { Some(false), Some(false), &HashMap::new(), + &HashMap::new(), "manual", &tls, ) @@ -1244,6 +1253,7 @@ async fn sandbox_create_keeps_sandbox_with_forwarding() { Some(false), Some(false), &HashMap::new(), + &HashMap::new(), "manual", &tls, ) @@ -1252,3 +1262,70 @@ async fn sandbox_create_keeps_sandbox_with_forwarding() { assert!(deleted_names(&server).await.is_empty()); } + +#[tokio::test] +async fn sandbox_create_sends_environment_variables() { + let server = run_server().await; + let fake_ssh_dir = tempfile::tempdir().unwrap(); + let xdg_dir = tempfile::tempdir().unwrap(); + let _env = test_env(&fake_ssh_dir, &xdg_dir); + let tls = test_tls(&server); + install_fake_ssh(&fake_ssh_dir); + + let mut env_map = HashMap::new(); + env_map.insert("FOO".to_string(), "bar".to_string()); + env_map.insert("BAZ".to_string(), "qux=with=equals".to_string()); + + run::sandbox_create( + &server.endpoint, + Some("env-test"), + None, + "openshell", + None, + true, + false, + None, + None, + None, + None, + &[], + None, + None, + &["echo".to_string(), "OK".to_string()], + Some(false), + Some(false), + &HashMap::new(), + &env_map, + "manual", + &tls, + ) + .await + .expect("sandbox create should succeed"); + + let requests = create_requests(&server).await; + let environment = &requests[0] + .spec + .as_ref() + .expect("spec should be present") + .environment; + assert_eq!(environment.get("FOO").map(String::as_str), Some("bar")); + assert_eq!( + environment.get("BAZ").map(String::as_str), + Some("qux=with=equals") + ); + assert_eq!(environment.len(), 2); +} + +#[tokio::test] +async fn sandbox_create_env_rejects_invalid_format() { + let err = run::parse_key_value_pairs( + &["VALID=ok".to_string(), "NOEQUALSSIGN".to_string()], + "--env", + ) + .unwrap_err(); + let msg = format!("{err}"); + assert!( + msg.contains("--env") && msg.contains("NOEQUALSSIGN"), + "error should mention the flag and bad value, got: {msg}" + ); +} diff --git a/crates/openshell-core/src/sandbox_env.rs b/crates/openshell-core/src/sandbox_env.rs index 9ffbf79f0..1059c0d08 100644 --- a/crates/openshell-core/src/sandbox_env.rs +++ b/crates/openshell-core/src/sandbox_env.rs @@ -50,6 +50,13 @@ pub const SANDBOX_TOKEN: &str = "OPENSHELL_SANDBOX_TOKEN"; /// the token is held in process memory thereafter. pub const SANDBOX_TOKEN_FILE: &str = "OPENSHELL_SANDBOX_TOKEN_FILE"; +/// JSON-serialized map of user-specified environment variables. +/// +/// Set by compute drivers from `SandboxSpec.environment`. The sandbox +/// supervisor deserializes this at startup and injects the variables into +/// SSH child processes (which use `env_clear()` for security isolation). +pub const USER_ENVIRONMENT: &str = "OPENSHELL_USER_ENVIRONMENT"; + /// Path to the projected `ServiceAccount` JWT (Kubernetes driver). /// /// Used to bootstrap a gateway-minted JWT via `IssueSandboxToken`. Kubelet diff --git a/crates/openshell-driver-docker/Cargo.toml b/crates/openshell-driver-docker/Cargo.toml index 0cdc205ed..fb2a643ea 100644 --- a/crates/openshell-driver-docker/Cargo.toml +++ b/crates/openshell-driver-docker/Cargo.toml @@ -20,6 +20,7 @@ tokio-stream = { workspace = true } tracing = { workspace = true } bytes = { workspace = true } serde = { workspace = true } +serde_json = { workspace = true } bollard = { version = "0.20" } tar = "0.4" tempfile = "3" diff --git a/crates/openshell-driver-docker/src/lib.rs b/crates/openshell-driver-docker/src/lib.rs index 864d91f22..2d69a1d88 100644 --- a/crates/openshell-driver-docker/src/lib.rs +++ b/crates/openshell-driver-docker/src/lib.rs @@ -1643,10 +1643,20 @@ fn build_environment(sandbox: &DriverSandbox, config: &DockerDriverRuntimeConfig ]); if let Some(spec) = sandbox.spec.as_ref() { + let mut user_env = HashMap::new(); if let Some(template) = spec.template.as_ref() { - environment.extend(template.environment.clone()); + user_env.extend(template.environment.clone()); } - environment.extend(spec.environment.clone()); + user_env.extend(spec.environment.clone()); + if !user_env.is_empty() + && let Ok(json) = serde_json::to_string(&user_env) + { + environment.insert( + openshell_core::sandbox_env::USER_ENVIRONMENT.to_string(), + json, + ); + } + environment.extend(user_env); } environment.insert( diff --git a/crates/openshell-driver-kubernetes/src/driver.rs b/crates/openshell-driver-kubernetes/src/driver.rs index 830b85225..8e633e4a6 100644 --- a/crates/openshell-driver-kubernetes/src/driver.rs +++ b/crates/openshell-driver-kubernetes/src/driver.rs @@ -1482,6 +1482,13 @@ fn build_env_list( let mut env = existing_env.cloned().unwrap_or_default(); apply_env_map(&mut env, template_environment); apply_env_map(&mut env, spec_environment); + let mut user_env = template_environment.clone(); + user_env.extend(spec_environment.clone()); + if !user_env.is_empty() + && let Ok(json) = serde_json::to_string(&user_env) + { + upsert_env(&mut env, openshell_core::sandbox_env::USER_ENVIRONMENT, &json); + } apply_required_env( &mut env, sandbox_id, diff --git a/crates/openshell-driver-podman/src/container.rs b/crates/openshell-driver-podman/src/container.rs index 13f053e93..44f53a9ee 100644 --- a/crates/openshell-driver-podman/src/container.rs +++ b/crates/openshell-driver-podman/src/container.rs @@ -244,6 +244,7 @@ fn build_env( let mut env: BTreeMap = BTreeMap::new(); // 1. User-supplied environment (lowest priority). + let mut user_env: BTreeMap = BTreeMap::new(); if let Some(s) = spec { if !s.log_level.is_empty() { env.insert( @@ -252,14 +253,23 @@ fn build_env( ); } for (k, v) in &s.environment { - env.insert(k.clone(), v.clone()); + user_env.insert(k.clone(), v.clone()); } } if let Some(t) = template { for (k, v) in &t.environment { - env.insert(k.clone(), v.clone()); + user_env.insert(k.clone(), v.clone()); } } + if !user_env.is_empty() + && let Ok(json) = serde_json::to_string(&user_env) + { + env.insert( + openshell_core::sandbox_env::USER_ENVIRONMENT.into(), + json, + ); + } + env.extend(user_env); // 2. Required driver vars (highest priority -- always overwrite). env.insert( diff --git a/crates/openshell-driver-vm/src/driver.rs b/crates/openshell-driver-vm/src/driver.rs index 445905a1e..bd124abfb 100644 --- a/crates/openshell-driver-vm/src/driver.rs +++ b/crates/openshell-driver-vm/src/driver.rs @@ -3498,7 +3498,16 @@ fn build_guest_environment( ), ])); } - environment.extend(merged_environment(sandbox)); + let user_env = merged_environment(sandbox); + if !user_env.is_empty() + && let Ok(json) = serde_json::to_string(&user_env) + { + environment.insert( + openshell_core::sandbox_env::USER_ENVIRONMENT.to_string(), + json, + ); + } + environment.extend(user_env); environment.insert( openshell_core::sandbox_env::TELEMETRY_ENABLED.to_string(), openshell_core::telemetry::enabled_env_value().to_string(), diff --git a/crates/openshell-sandbox/src/lib.rs b/crates/openshell-sandbox/src/lib.rs index e9d8921b6..90ba4fdb7 100644 --- a/crates/openshell-sandbox/src/lib.rs +++ b/crates/openshell-sandbox/src/lib.rs @@ -424,6 +424,13 @@ pub async fn run_sandbox( ); let provider_env = provider_credentials.snapshot().child_env.clone(); + let user_environment: std::collections::HashMap = std::env::var( + openshell_core::sandbox_env::USER_ENVIRONMENT, + ) + .ok() + .and_then(|json| serde_json::from_str(&json).ok()) + .unwrap_or_default(); + // Create identity cache for SHA256 TOFU when OPA is active let identity_cache = opa_engine .as_ref() @@ -806,6 +813,7 @@ pub async fn run_sandbox( let netns_fd = ssh_netns_fd; let ca_paths = ca_file_paths.clone(); let provider_credentials_clone = provider_credentials.clone(); + let user_env_clone = user_environment.clone(); let (ssh_ready_tx, ssh_ready_rx) = tokio::sync::oneshot::channel(); @@ -819,6 +827,7 @@ pub async fn run_sandbox( proxy_url, ca_paths, provider_credentials_clone, + user_env_clone, ) .await { diff --git a/crates/openshell-sandbox/src/ssh.rs b/crates/openshell-sandbox/src/ssh.rs index 67fbc7e57..3eddc4b6d 100644 --- a/crates/openshell-sandbox/src/ssh.rs +++ b/crates/openshell-sandbox/src/ssh.rs @@ -107,6 +107,7 @@ pub async fn run_ssh_server( proxy_url: Option, ca_file_paths: Option<(PathBuf, PathBuf)>, provider_credentials: ProviderCredentialState, + user_environment: HashMap, ) -> Result<()> { let (listener, config, ca_paths) = match ssh_server_init(&listen_path, &ca_file_paths) { Ok(v) => { @@ -131,6 +132,7 @@ pub async fn run_ssh_server( let proxy_url = proxy_url.clone(); let ca_paths = ca_paths.clone(); let provider_credentials = provider_credentials.clone(); + let user_environment = user_environment.clone(); tokio::spawn(async move { if let Err(err) = handle_connection( @@ -142,6 +144,7 @@ pub async fn run_ssh_server( proxy_url, ca_paths, provider_credentials, + user_environment, ) .await { @@ -168,6 +171,7 @@ async fn handle_connection( proxy_url: Option, ca_file_paths: Option>, provider_credentials: ProviderCredentialState, + user_environment: HashMap, ) -> Result<()> { // Access is gated by the Unix-socket filesystem permissions (root-only), // not by an application-level preface. The supervisor bridges the @@ -190,6 +194,7 @@ async fn handle_connection( proxy_url, ca_file_paths, provider_credentials, + user_environment, ); russh::server::run_stream(config, stream, handler) .await @@ -217,10 +222,12 @@ struct SshHandler { proxy_url: Option, ca_file_paths: Option>, provider_credentials: ProviderCredentialState, + user_environment: HashMap, channels: HashMap, } impl SshHandler { + #[allow(clippy::too_many_arguments)] fn new( policy: SandboxPolicy, workdir: Option, @@ -228,6 +235,7 @@ impl SshHandler { proxy_url: Option, ca_file_paths: Option>, provider_credentials: ProviderCredentialState, + user_environment: HashMap, ) -> Self { Self { policy, @@ -236,6 +244,7 @@ impl SshHandler { proxy_url, ca_file_paths, provider_credentials, + user_environment, channels: HashMap::new(), } } @@ -458,6 +467,7 @@ impl russh::server::Handler for SshHandler { self.proxy_url.clone(), self.ca_file_paths.clone(), &self.provider_credentials.snapshot().child_env, + &self.user_environment, )?; let state = self.channels.get_mut(&channel).ok_or_else(|| { anyhow::anyhow!("subsystem_request on unknown channel {channel:?}") @@ -553,6 +563,7 @@ impl SshHandler { self.proxy_url.clone(), self.ca_file_paths.clone(), &provider_snapshot.child_env, + &self.user_environment, )?; state.pty_master = Some(pty_master); state.input_sender = Some(input_sender); @@ -570,6 +581,7 @@ impl SshHandler { self.proxy_url.clone(), self.ca_file_paths.clone(), &provider_snapshot.child_env, + &self.user_environment, )?; state.input_sender = Some(input_sender); } @@ -668,6 +680,7 @@ fn session_user_and_home(policy: &SandboxPolicy) -> (String, String) { } } +#[allow(clippy::too_many_arguments)] fn apply_child_env( cmd: &mut Command, session_home: &str, @@ -676,6 +689,7 @@ fn apply_child_env( proxy_url: Option<&str>, ca_file_paths: Option<&(PathBuf, PathBuf)>, provider_env: &HashMap, + user_environment: &HashMap, ) { let path = std::env::var("PATH").unwrap_or_else(|_| "/usr/local/bin:/usr/bin:/bin".into()); @@ -687,6 +701,10 @@ fn apply_child_env( .env("PATH", &path) .env("TERM", term); + for (key, value) in user_environment { + cmd.env(key, value); + } + if let Some(url) = proxy_url { for (key, value) in child_env::proxy_env_vars(url) { cmd.env(key, value); @@ -716,6 +734,7 @@ fn spawn_pty_shell( proxy_url: Option, ca_file_paths: Option>, provider_env: &HashMap, + user_environment: &HashMap, ) -> anyhow::Result<(std::fs::File, mpsc::Sender>)> { let winsize = Winsize { ws_row: to_u16(pty.row_height.max(1)), @@ -764,6 +783,7 @@ fn spawn_pty_shell( proxy_url.as_deref(), ca_file_paths.as_deref(), provider_env, + user_environment, ); cmd.stdin(stdin).stdout(stdout).stderr(stderr); @@ -879,6 +899,7 @@ fn spawn_pipe_exec( proxy_url: Option, ca_file_paths: Option>, provider_env: &HashMap, + user_environment: &HashMap, ) -> anyhow::Result>> { let mut cmd = command.map_or_else( || { @@ -909,6 +930,7 @@ fn spawn_pipe_exec( proxy_url.as_deref(), ca_file_paths.as_deref(), provider_env, + user_environment, ); cmd.stdin(Stdio::piped()) .stdout(Stdio::piped()) From a576536ed8ded587b1fcbeb9195a2a59d00d08b1 Mon Sep 17 00:00:00 2001 From: Russell Bryant Date: Thu, 4 Jun 2026 14:46:42 -0400 Subject: [PATCH 2/3] fix(sandbox): address PR review feedback for env var injection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - VM driver: restructure build_guest_environment to apply user env before required driver vars, preventing user-specified values from overriding OPENSHELL_ENDPOINT, PATH, and other critical vars. Matches the pattern already used by Docker/Podman/Kubernetes. - All drivers: insert OPENSHELL_USER_ENVIRONMENT sidecar after environment.extend(user_env) so a user-supplied key of the same name cannot clobber the JSON payload the supervisor parses. - Podman driver: fix template/spec env precedence — template env is now applied first with spec overwriting, matching the other drivers (spec is user-specified and should win over image defaults). - CLI: add parse_env_pairs() that rejects OPENSHELL_* prefixed keys with a clear error message. Both sandbox create and exec call sites updated to use it. - SSH handler: filter out OPENSHELL_* keys from user_environment before injecting into child processes. - Docs: add "Set Environment Variables" section to manage-sandboxes.mdx documenting --env for both create and exec, per-command override semantics, and the OPENSHELL_* reservation. - Tests: add sandbox_create_env_rejects_reserved_prefix test. - Fix rustfmt formatting in lib.rs, container.rs, driver.rs. --- crates/openshell-cli/src/main.rs | 2 +- crates/openshell-cli/src/run.rs | 12 +++ .../sandbox_create_lifecycle_integration.rs | 11 +++ crates/openshell-driver-docker/src/lib.rs | 2 +- .../openshell-driver-kubernetes/src/driver.rs | 6 +- .../openshell-driver-podman/src/container.rs | 18 ++-- crates/openshell-driver-vm/src/driver.rs | 98 +++++++++---------- crates/openshell-sandbox/src/lib.rs | 11 +-- crates/openshell-sandbox/src/ssh.rs | 4 +- docs/sandboxes/manage-sandboxes.mdx | 21 ++++ 10 files changed, 116 insertions(+), 69 deletions(-) diff --git a/crates/openshell-cli/src/main.rs b/crates/openshell-cli/src/main.rs index 802555410..4e8778c41 100644 --- a/crates/openshell-cli/src/main.rs +++ b/crates/openshell-cli/src/main.rs @@ -2766,7 +2766,7 @@ async fn main() -> Result<()> { } else { None // auto-detect }; - let env_map = run::parse_key_value_pairs(&envs, "--env")?; + let env_map = run::parse_env_pairs(&envs)?; let exit_code = run::sandbox_exec_grpc( endpoint, &name, diff --git a/crates/openshell-cli/src/run.rs b/crates/openshell-cli/src/run.rs index c570cfb3c..d1d8c3389 100644 --- a/crates/openshell-cli/src/run.rs +++ b/crates/openshell-cli/src/run.rs @@ -3868,6 +3868,18 @@ pub fn parse_key_value_pairs(items: &[String], flag: &str) -> Result Result> { + let map = parse_key_value_pairs(items, "--env")?; + for key in map.keys() { + if key.starts_with("OPENSHELL_") { + return Err(miette::miette!( + "--env keys starting with OPENSHELL_ are reserved; got '{key}'" + )); + } + } + Ok(map) +} + fn parse_credential_pairs(items: &[String]) -> Result> { let mut map = HashMap::new(); diff --git a/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs b/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs index e1c3d8e89..22786187c 100644 --- a/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs +++ b/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs @@ -1329,3 +1329,14 @@ async fn sandbox_create_env_rejects_invalid_format() { "error should mention the flag and bad value, got: {msg}" ); } + +#[tokio::test] +async fn sandbox_create_env_rejects_reserved_prefix() { + let err = run::parse_env_pairs(&["VALID=ok".to_string(), "OPENSHELL_SECRET=bad".to_string()]) + .unwrap_err(); + let msg = format!("{err}"); + assert!( + msg.contains("OPENSHELL_") && msg.contains("reserved"), + "error should mention reserved prefix, got: {msg}" + ); +} diff --git a/crates/openshell-driver-docker/src/lib.rs b/crates/openshell-driver-docker/src/lib.rs index 2d69a1d88..11ffab7d6 100644 --- a/crates/openshell-driver-docker/src/lib.rs +++ b/crates/openshell-driver-docker/src/lib.rs @@ -1648,6 +1648,7 @@ fn build_environment(sandbox: &DriverSandbox, config: &DockerDriverRuntimeConfig user_env.extend(template.environment.clone()); } user_env.extend(spec.environment.clone()); + environment.extend(user_env.clone()); if !user_env.is_empty() && let Ok(json) = serde_json::to_string(&user_env) { @@ -1656,7 +1657,6 @@ fn build_environment(sandbox: &DriverSandbox, config: &DockerDriverRuntimeConfig json, ); } - environment.extend(user_env); } environment.insert( diff --git a/crates/openshell-driver-kubernetes/src/driver.rs b/crates/openshell-driver-kubernetes/src/driver.rs index 8e633e4a6..fc1f74929 100644 --- a/crates/openshell-driver-kubernetes/src/driver.rs +++ b/crates/openshell-driver-kubernetes/src/driver.rs @@ -1487,7 +1487,11 @@ fn build_env_list( if !user_env.is_empty() && let Ok(json) = serde_json::to_string(&user_env) { - upsert_env(&mut env, openshell_core::sandbox_env::USER_ENVIRONMENT, &json); + upsert_env( + &mut env, + openshell_core::sandbox_env::USER_ENVIRONMENT, + &json, + ); } apply_required_env( &mut env, diff --git a/crates/openshell-driver-podman/src/container.rs b/crates/openshell-driver-podman/src/container.rs index 44f53a9ee..cdf074619 100644 --- a/crates/openshell-driver-podman/src/container.rs +++ b/crates/openshell-driver-podman/src/container.rs @@ -244,7 +244,13 @@ fn build_env( let mut env: BTreeMap = BTreeMap::new(); // 1. User-supplied environment (lowest priority). + // Template vars first, then spec overwrites (spec is user-specified). let mut user_env: BTreeMap = BTreeMap::new(); + if let Some(t) = template { + for (k, v) in &t.environment { + user_env.insert(k.clone(), v.clone()); + } + } if let Some(s) = spec { if !s.log_level.is_empty() { env.insert( @@ -256,20 +262,12 @@ fn build_env( user_env.insert(k.clone(), v.clone()); } } - if let Some(t) = template { - for (k, v) in &t.environment { - user_env.insert(k.clone(), v.clone()); - } - } + env.extend(user_env.clone()); if !user_env.is_empty() && let Ok(json) = serde_json::to_string(&user_env) { - env.insert( - openshell_core::sandbox_env::USER_ENVIRONMENT.into(), - json, - ); + env.insert(openshell_core::sandbox_env::USER_ENVIRONMENT.into(), json); } - env.extend(user_env); // 2. Required driver vars (highest priority -- always overwrite). env.insert( diff --git a/crates/openshell-driver-vm/src/driver.rs b/crates/openshell-driver-vm/src/driver.rs index bd124abfb..5338c576e 100644 --- a/crates/openshell-driver-vm/src/driver.rs +++ b/crates/openshell-driver-vm/src/driver.rs @@ -3450,55 +3450,10 @@ fn build_guest_environment( || guest_visible_openshell_endpoint(&config.openshell_endpoint), String::from, ); - let mut environment = HashMap::from([ - ("HOME".to_string(), "/root".to_string()), - ( - "PATH".to_string(), - "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin".to_string(), - ), - ("TERM".to_string(), "xterm".to_string()), - ( - openshell_core::sandbox_env::ENDPOINT.to_string(), - openshell_endpoint, - ), - ( - openshell_core::sandbox_env::SANDBOX_ID.to_string(), - sandbox.id.clone(), - ), - ( - openshell_core::sandbox_env::SANDBOX.to_string(), - sandbox.name.clone(), - ), - ( - openshell_core::sandbox_env::SSH_SOCKET_PATH.to_string(), - GUEST_SSH_SOCKET_PATH.to_string(), - ), - ( - openshell_core::sandbox_env::SANDBOX_COMMAND.to_string(), - "tail -f /dev/null".to_string(), - ), - ( - openshell_core::sandbox_env::LOG_LEVEL.to_string(), - openshell_core::driver_utils::sandbox_log_level(sandbox, &config.log_level), - ), - ]); - if config.requires_tls_materials() { - environment.extend(HashMap::from([ - ( - openshell_core::sandbox_env::TLS_CA.to_string(), - GUEST_TLS_CA_PATH.to_string(), - ), - ( - openshell_core::sandbox_env::TLS_CERT.to_string(), - GUEST_TLS_CERT_PATH.to_string(), - ), - ( - openshell_core::sandbox_env::TLS_KEY.to_string(), - GUEST_TLS_KEY_PATH.to_string(), - ), - ])); - } + // 1. User-supplied environment (lowest priority). let user_env = merged_environment(sandbox); + let mut environment: HashMap = HashMap::new(); + environment.extend(user_env.clone()); if !user_env.is_empty() && let Ok(json) = serde_json::to_string(&user_env) { @@ -3507,7 +3462,52 @@ fn build_guest_environment( json, ); } - environment.extend(user_env); + + // 2. Required driver vars (highest priority -- always overwrite). + environment.insert("HOME".to_string(), "/root".to_string()); + environment.insert( + "PATH".to_string(), + "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin".to_string(), + ); + environment.insert("TERM".to_string(), "xterm".to_string()); + environment.insert( + openshell_core::sandbox_env::ENDPOINT.to_string(), + openshell_endpoint, + ); + environment.insert( + openshell_core::sandbox_env::SANDBOX_ID.to_string(), + sandbox.id.clone(), + ); + environment.insert( + openshell_core::sandbox_env::SANDBOX.to_string(), + sandbox.name.clone(), + ); + environment.insert( + openshell_core::sandbox_env::SSH_SOCKET_PATH.to_string(), + GUEST_SSH_SOCKET_PATH.to_string(), + ); + environment.insert( + openshell_core::sandbox_env::SANDBOX_COMMAND.to_string(), + "tail -f /dev/null".to_string(), + ); + environment.insert( + openshell_core::sandbox_env::LOG_LEVEL.to_string(), + openshell_core::driver_utils::sandbox_log_level(sandbox, &config.log_level), + ); + if config.requires_tls_materials() { + environment.insert( + openshell_core::sandbox_env::TLS_CA.to_string(), + GUEST_TLS_CA_PATH.to_string(), + ); + environment.insert( + openshell_core::sandbox_env::TLS_CERT.to_string(), + GUEST_TLS_CERT_PATH.to_string(), + ); + environment.insert( + openshell_core::sandbox_env::TLS_KEY.to_string(), + GUEST_TLS_KEY_PATH.to_string(), + ); + } environment.insert( openshell_core::sandbox_env::TELEMETRY_ENABLED.to_string(), openshell_core::telemetry::enabled_env_value().to_string(), diff --git a/crates/openshell-sandbox/src/lib.rs b/crates/openshell-sandbox/src/lib.rs index 90ba4fdb7..231b588ea 100644 --- a/crates/openshell-sandbox/src/lib.rs +++ b/crates/openshell-sandbox/src/lib.rs @@ -424,12 +424,11 @@ pub async fn run_sandbox( ); let provider_env = provider_credentials.snapshot().child_env.clone(); - let user_environment: std::collections::HashMap = std::env::var( - openshell_core::sandbox_env::USER_ENVIRONMENT, - ) - .ok() - .and_then(|json| serde_json::from_str(&json).ok()) - .unwrap_or_default(); + let user_environment: std::collections::HashMap = + std::env::var(openshell_core::sandbox_env::USER_ENVIRONMENT) + .ok() + .and_then(|json| serde_json::from_str(&json).ok()) + .unwrap_or_default(); // Create identity cache for SHA256 TOFU when OPA is active let identity_cache = opa_engine diff --git a/crates/openshell-sandbox/src/ssh.rs b/crates/openshell-sandbox/src/ssh.rs index 3eddc4b6d..9db2bf97d 100644 --- a/crates/openshell-sandbox/src/ssh.rs +++ b/crates/openshell-sandbox/src/ssh.rs @@ -702,7 +702,9 @@ fn apply_child_env( .env("TERM", term); for (key, value) in user_environment { - cmd.env(key, value); + if !key.starts_with("OPENSHELL_") { + cmd.env(key, value); + } } if let Some(url) = proxy_url { diff --git a/docs/sandboxes/manage-sandboxes.mdx b/docs/sandboxes/manage-sandboxes.mdx index 8a154c1b3..27a414387 100644 --- a/docs/sandboxes/manage-sandboxes.mdx +++ b/docs/sandboxes/manage-sandboxes.mdx @@ -134,6 +134,27 @@ OpenShell allocates a TTY automatically when both stdin and stdout are terminals | `--timeout` | Command timeout in seconds. `0` disables the timeout. | | `--tty` | Force TTY allocation. | | `--no-tty` | Disable TTY allocation even when attached to a terminal. | +| `--env` | Set an environment variable for the command (`KEY=VALUE`, repeatable). | + +## Set Environment Variables + +Inject environment variables into the sandbox at creation time: + +```shell +openshell sandbox create --env API_KEY=sk-test --env DEBUG=1 -- my-agent +``` + +Variables set with `--env` are available to all processes in the sandbox, including interactive shells and exec commands. + +You can also set per-command environment variables with `sandbox exec`: + +```shell +openshell sandbox exec -n my-sandbox --env MY_VAR=hello -- printenv MY_VAR +``` + +Per-command variables override the sandbox-level environment for that command only. + +Environment variable names starting with `OPENSHELL_` are reserved and rejected by the CLI. ## Label a Sandbox From cf1baf98ef940037ed01b9cc474931b80fe219a7 Mon Sep 17 00:00:00 2001 From: Russell Bryant Date: Fri, 5 Jun 2026 08:29:01 -0400 Subject: [PATCH 3/3] fix(server): reject OPENSHELL_* env keys at API boundary Add server-side validation to reject environment variable keys starting with OPENSHELL_ in SandboxSpec.environment, SandboxTemplate.environment, and ExecSandboxRequest.environment. This closes the gap where SDK/gRPC callers (bypassing the CLI) could inject reserved keys that affect supervisor-internal state. The CLI already rejects these keys via parse_env_pairs, but the API boundary is the authoritative enforcement point since all clients (CLI, SDK, direct gRPC) pass through it. --- .../sandbox_create_lifecycle_integration.rs | 2 +- .../openshell-server/src/grpc/validation.rs | 70 +++++++++++++++++++ 2 files changed, 71 insertions(+), 1 deletion(-) diff --git a/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs b/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs index 22786187c..ab7039b8a 100644 --- a/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs +++ b/crates/openshell-cli/tests/sandbox_create_lifecycle_integration.rs @@ -1281,7 +1281,7 @@ async fn sandbox_create_sends_environment_variables() { Some("env-test"), None, "openshell", - None, + &[], true, false, None, diff --git a/crates/openshell-server/src/grpc/validation.rs b/crates/openshell-server/src/grpc/validation.rs index 83658ef7b..5680acd86 100644 --- a/crates/openshell-server/src/grpc/validation.rs +++ b/crates/openshell-server/src/grpc/validation.rs @@ -56,6 +56,7 @@ pub(super) fn validate_exec_request_fields(req: &ExecSandboxRequest) -> Result<( } reject_control_chars(value, &format!("environment value for '{key}'"))?; } + reject_reserved_env_keys(&req.environment, "environment")?; if !req.workdir.is_empty() { if req.workdir.len() > MAX_EXEC_WORKDIR_LEN { return Err(Status::invalid_argument(format!( @@ -125,10 +126,12 @@ pub(super) fn validate_sandbox_spec( MAX_MAP_VALUE_LEN, "spec.environment", )?; + reject_reserved_env_keys(&spec.environment, "spec.environment")?; // --- spec.template --- if let Some(ref tmpl) = spec.template { validate_sandbox_template(tmpl)?; + reject_reserved_env_keys(&tmpl.environment, "spec.template.environment")?; } // --- spec.policy serialized size --- @@ -235,6 +238,20 @@ pub(super) fn validate_string_map( Ok(()) } +fn reject_reserved_env_keys( + map: &std::collections::HashMap, + field_name: &str, +) -> Result<(), Status> { + for key in map.keys() { + if key.starts_with("OPENSHELL_") { + return Err(Status::invalid_argument(format!( + "{field_name} keys starting with OPENSHELL_ are reserved; got '{key}'" + ))); + } + } + Ok(()) +} + // --------------------------------------------------------------------------- // Provider field validation // --------------------------------------------------------------------------- @@ -886,6 +903,59 @@ mod tests { assert!(validate_sandbox_spec("my-sandbox", &spec).is_ok()); } + #[test] + fn validate_sandbox_spec_rejects_reserved_env_key() { + let spec = SandboxSpec { + environment: std::iter::once(("OPENSHELL_SECRET".to_string(), "val".to_string())) + .collect(), + ..Default::default() + }; + let err = validate_sandbox_spec("s", &spec).unwrap_err(); + assert!( + err.message().contains("OPENSHELL_") && err.message().contains("reserved"), + "expected reserved key error, got: {}", + err.message() + ); + } + + #[test] + fn validate_sandbox_spec_rejects_reserved_template_env_key() { + let spec = SandboxSpec { + template: Some(SandboxTemplate { + environment: std::iter::once(( + "OPENSHELL_ENDPOINT".to_string(), + "evil".to_string(), + )) + .collect(), + ..Default::default() + }), + ..Default::default() + }; + let err = validate_sandbox_spec("s", &spec).unwrap_err(); + assert!( + err.message().contains("OPENSHELL_") && err.message().contains("reserved"), + "expected reserved key error, got: {}", + err.message() + ); + } + + #[test] + fn validate_exec_request_rejects_reserved_env_key() { + let req = ExecSandboxRequest { + sandbox_id: "id".to_string(), + command: vec!["echo".to_string()], + environment: std::iter::once(("OPENSHELL_SANDBOX_ID".to_string(), "evil".to_string())) + .collect(), + ..Default::default() + }; + let err = validate_exec_request_fields(&req).unwrap_err(); + assert!( + err.message().contains("OPENSHELL_") && err.message().contains("reserved"), + "expected reserved key error, got: {}", + err.message() + ); + } + // ---- Provider field validation ---- fn one_credential() -> HashMap {