From 9b47113418afbf78362d382125bd2db281c17918 Mon Sep 17 00:00:00 2001 From: John Myers <9696606+johntmyers@users.noreply.github.com> Date: Sun, 15 Mar 2026 23:27:50 -0700 Subject: [PATCH 01/13] fix(sandbox): prevent overread request smuggling in L7 REST parser The HTTP/1.1 parser used a 1024-byte read buffer that could capture bytes from a pipelined second request. Those overflow bytes were forwarded upstream as body overflow without L7 policy evaluation, enabling request smuggling that bypasses per-request method/path enforcement. Replace multi-byte read with byte-at-a-time read_u8 that stops exactly at the CRLFCRLF header terminator. Add regression test proving two pipelined requests are parsed independently. Refs: #350 --- crates/openshell-sandbox/src/l7/rest.rs | 70 +++++++++++++++++++------ 1 file changed, 54 insertions(+), 16 deletions(-) diff --git a/crates/openshell-sandbox/src/l7/rest.rs b/crates/openshell-sandbox/src/l7/rest.rs index 6d83195e..61b026f5 100644 --- a/crates/openshell-sandbox/src/l7/rest.rs +++ b/crates/openshell-sandbox/src/l7/rest.rs @@ -51,9 +51,15 @@ impl L7Provider for RestProvider { } /// Parse one HTTP/1.1 request from the stream. +/// +/// Reads one byte at a time to stop exactly at the `\r\n\r\n` header +/// terminator. A multi-byte read could consume bytes belonging to a +/// subsequent pipelined request, and those overflow bytes would be +/// forwarded upstream without L7 policy evaluation -- a request +/// smuggling vulnerability. Byte-at-a-time overhead is negligible for +/// the typical 200-800 byte headers on L7-inspected REST endpoints. async fn parse_http_request(client: &mut C) -> Result> { let mut buf = Vec::with_capacity(4096); - let mut tmp = [0u8; 1024]; loop { if buf.len() > MAX_HEADER_BYTES { @@ -62,24 +68,19 @@ async fn parse_http_request(client: &mut C) -> Result n, + let byte = match client.read_u8().await { + Ok(b) => b, Err(e) if buf.is_empty() && is_benign_close(&e) => return Ok(None), + Err(e) if buf.is_empty() && e.kind() == std::io::ErrorKind::UnexpectedEof => { + return Ok(None); // Clean close before any data + } Err(e) => return Err(miette::miette!("{e}")), }; - if n == 0 { - if buf.is_empty() { - return Ok(None); // Clean connection close - } - return Err(miette!( - "Client disconnected mid-request after {} bytes", - buf.len() - )); - } - buf.extend_from_slice(&tmp[..n]); + buf.push(byte); - // Check for end of headers - if buf.windows(4).any(|w| w == b"\r\n\r\n") { + // Check for end of headers -- `ends_with` is sufficient because + // we append exactly one byte per iteration. + if buf.ends_with(b"\r\n\r\n") { break; } } @@ -109,7 +110,7 @@ async fn parse_http_request(client: &mut C) -> Result Date: Sun, 15 Mar 2026 23:30:43 -0700 Subject: [PATCH 02/13] fix(server): harden sandbox TLS secret volume permissions to 0400 Kubernetes secret volumes default to 0644, allowing the unprivileged sandbox user to read the mTLS client private key via the Landlock baseline /etc read set. A compromised sandbox could use the key to impersonate the control-plane client. Set defaultMode to 256 (octal 0400, owner-read only) on both the default and custom pod template paths. The supervisor reads TLS materials as root before forking, so this does not affect normal operation. Refs: #350 --- crates/openshell-server/src/sandbox/mod.rs | 42 +++++++++++++++++++--- 1 file changed, 38 insertions(+), 4 deletions(-) diff --git a/crates/openshell-server/src/sandbox/mod.rs b/crates/openshell-server/src/sandbox/mod.rs index 7b1272a8..d8eb0410 100644 --- a/crates/openshell-server/src/sandbox/mod.rs +++ b/crates/openshell-server/src/sandbox/mod.rs @@ -969,13 +969,14 @@ fn sandbox_template_to_k8s( serde_json::Value::Array(vec![serde_json::Value::Object(container)]), ); - // Add TLS secret volume. + // Add TLS secret volume. Mode 0400 (owner-read) prevents the + // unprivileged sandbox user from reading the mTLS private key. if !client_tls_secret_name.is_empty() { spec.insert( "volumes".to_string(), serde_json::json!([{ "name": "openshell-client-tls", - "secret": { "secretName": client_tls_secret_name } + "secret": { "secretName": client_tls_secret_name, "defaultMode": 256 } }]), ); } @@ -1047,7 +1048,8 @@ fn inject_pod_template( ); } - // Inject TLS volume at the pod spec level. + // Inject TLS volume at the pod spec level. Mode 0400 (owner-read) + // prevents the unprivileged sandbox user from reading the mTLS private key. if !client_tls_secret_name.is_empty() { let volumes = spec .entry("volumes") @@ -1055,7 +1057,7 @@ fn inject_pod_template( if let Some(volumes_arr) = volumes.as_array_mut() { volumes_arr.push(serde_json::json!({ "name": "openshell-client-tls", - "secret": { "secretName": client_tls_secret_name } + "secret": { "secretName": client_tls_secret_name, "defaultMode": 256 } })); } } @@ -2071,4 +2073,36 @@ mod tests { .expect("hostAliases should exist in custom pod template"); assert_eq!(host_aliases[0]["ip"], "192.168.65.2"); } + + #[test] + fn tls_secret_volume_uses_restrictive_default_mode() { + let template = SandboxTemplate::default(); + let pod_template = sandbox_template_to_k8s( + &template, + false, + "openshell/sandbox:latest", + "", + "sandbox-id", + "sandbox-name", + "https://gateway.example.com", + "0.0.0.0:2222", + "secret", + 300, + &std::collections::HashMap::new(), + "my-tls-secret", + "", + ); + + let volumes = pod_template["spec"]["volumes"] + .as_array() + .expect("volumes should exist"); + let tls_vol = volumes + .iter() + .find(|v| v["name"] == "openshell-client-tls") + .expect("TLS volume should exist"); + assert_eq!( + tls_vol["secret"]["defaultMode"], 256, // 0o400 + "TLS secret volume must use mode 0400 to prevent sandbox user from reading the private key" + ); + } } From fde0d7e7a2d12f7dad4f95a2846a141cf60d42f3 Mon Sep 17 00:00:00 2001 From: John Myers <9696606+johntmyers@users.noreply.github.com> Date: Sun, 15 Mar 2026 23:35:39 -0700 Subject: [PATCH 03/13] fix(sandbox): stop using cmdline paths for binary policy matching /proc/pid/cmdline is fully attacker-controlled (argv[0] can be set to any string via execve) and had no integrity verification, unlike exec.path and ancestors which are kernel-managed and get TOFU/SHA256 checks. The Rego policy used cmdline_paths as a grant-access signal, allowing any sandboxed process to claim the identity of an allowed binary and bypass network restrictions. Remove the cmdline exact-match rule and exclude cmdline_paths from the glob-match rule. Only exec.path and exec.ancestors (from /proc/pid/exe) are now used for binary identity. cmdline_paths remain in the OPA input for deny-reason diagnostics only. Refs: #350 --- .../openshell-sandbox/data/sandbox-policy.rego | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/crates/openshell-sandbox/data/sandbox-policy.rego b/crates/openshell-sandbox/data/sandbox-policy.rego index 7e9986ab..1fddcea2 100644 --- a/crates/openshell-sandbox/data/sandbox-policy.rego +++ b/crates/openshell-sandbox/data/sandbox-policy.rego @@ -129,22 +129,13 @@ binary_allowed(policy, exec) if { b.path == ancestor } -# Binary matching: cmdline exact path (script interpreters — e.g. node runs claude script). -# When /usr/local/bin/claude has shebang #!/usr/bin/env node, the exe is /usr/bin/node -# but cmdline contains /usr/local/bin/claude as an argv entry. -binary_allowed(policy, exec) if { - some b - b := policy.binaries[_] - not contains(b.path, "*") - cp := exec.cmdline_paths[_] - b.path == cp -} - -# Binary matching: glob pattern against path, any ancestor, or any cmdline path. +# Binary matching: glob pattern against exe path or any ancestor. +# NOTE: cmdline_paths are intentionally excluded — argv[0] is trivially +# spoofable via execve and must not be used as a grant-access signal. binary_allowed(policy, exec) if { some b in policy.binaries contains(b.path, "*") - all_paths := array.concat(array.concat([exec.path], exec.ancestors), exec.cmdline_paths) + all_paths := array.concat([exec.path], exec.ancestors) some p in all_paths glob.match(b.path, ["/"], p) } From 50bc50248de560550c17aa96c85360890220eed5 Mon Sep 17 00:00:00 2001 From: John Myers <9696606+johntmyers@users.noreply.github.com> Date: Sun, 15 Mar 2026 23:37:56 -0700 Subject: [PATCH 04/13] fix(sandbox): reject symlink and non-dir read_write paths before chown The supervisor runs as root and calls chown on each read_write path. Since chown follows symlinks, a malicious container image could place a symlink (e.g. /sandbox -> /etc/shadow) to trick the supervisor into transferring ownership of arbitrary files to the sandbox user. Add symlink_metadata (lstat) check before chown to reject symlinks and non-directory entries. The TOCTOU window is not exploitable because no untrusted child process has been forked yet at this point. Refs: #350 --- crates/openshell-sandbox/src/lib.rs | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/crates/openshell-sandbox/src/lib.rs b/crates/openshell-sandbox/src/lib.rs index 655acc86..877fb53c 100644 --- a/crates/openshell-sandbox/src/lib.rs +++ b/crates/openshell-sandbox/src/lib.rs @@ -1199,9 +1199,30 @@ fn prepare_filesystem(policy: &SandboxPolicy) -> Result<()> { None }; - // Create and chown each read_write path + // Create and chown each read_write path. + // + // SECURITY: use symlink_metadata (lstat) to inspect each path *before* + // calling chown. chown follows symlinks, so a malicious container image + // could place a symlink (e.g. /sandbox -> /etc/shadow) to trick the + // root supervisor into transferring ownership of arbitrary files. + // The TOCTOU window between lstat and chown is not exploitable because + // no untrusted process is running yet (the child has not been forked). for path in &policy.filesystem.read_write { - if !path.exists() { + // Check for symlinks or non-directory files before touching the path. + if let Ok(meta) = std::fs::symlink_metadata(path) { + if meta.file_type().is_symlink() { + return Err(miette::miette!( + "read_write path '{}' is a symlink — refusing to chown (potential privilege escalation)", + path.display() + )); + } + if !meta.file_type().is_dir() { + return Err(miette::miette!( + "read_write path '{}' exists but is not a directory — refusing to chown", + path.display() + )); + } + } else { debug!(path = %path.display(), "Creating read_write directory"); std::fs::create_dir_all(path).into_diagnostic()?; } From 2d3e643296ebea25bfed5ee419a4398f9f613fe2 Mon Sep 17 00:00:00 2001 From: John Myers <9696606+johntmyers@users.noreply.github.com> Date: Sun, 15 Mar 2026 23:42:42 -0700 Subject: [PATCH 05/13] fix(server): redact provider credentials in gRPC CRUD responses Provider CRUD RPCs (create, get, list, update) returned full Provider objects including plaintext credentials (API keys, secrets). Any authenticated client -- including sandbox workloads running untrusted code -- could read credentials for all providers. Add redact_provider_credentials helper that clears the credentials map before returning. Internal server paths (inference routing, sandbox env injection) read from the store directly and are unaffected. Update tests to verify redaction and assert persistence via direct store reads. Refs: #350 --- crates/openshell-server/src/grpc.rs | 70 ++++++++++++++++++++--------- 1 file changed, 48 insertions(+), 22 deletions(-) diff --git a/crates/openshell-server/src/grpc.rs b/crates/openshell-server/src/grpc.rs index 422b6463..a2c6a58f 100644 --- a/crates/openshell-server/src/grpc.rs +++ b/crates/openshell-server/src/grpc.rs @@ -3134,6 +3134,14 @@ fn hmac_sha256(key: &[u8], data: &[u8]) -> String { // Provider CRUD // --------------------------------------------------------------------------- +/// Strip credential values from a provider before returning it in a gRPC +/// response. Internal server paths (inference routing, sandbox env injection) +/// read credentials from the store directly and are unaffected. +fn redact_provider_credentials(mut provider: Provider) -> Provider { + provider.credentials.clear(); + provider +} + async fn create_provider_record( store: &crate::persistence::Store, mut provider: Provider, @@ -3169,7 +3177,7 @@ async fn create_provider_record( .await .map_err(|e| Status::internal(format!("persist provider failed: {e}")))?; - Ok(provider) + Ok(redact_provider_credentials(provider)) } async fn get_provider_record( @@ -3185,6 +3193,7 @@ async fn get_provider_record( .await .map_err(|e| Status::internal(format!("fetch provider failed: {e}")))? .ok_or_else(|| Status::not_found("provider not found")) + .map(redact_provider_credentials) } async fn list_provider_records( @@ -3201,7 +3210,7 @@ async fn list_provider_records( for record in records { let provider = Provider::decode(record.payload.as_slice()) .map_err(|e| Status::internal(format!("decode provider failed: {e}")))?; - providers.push(provider); + providers.push(redact_provider_credentials(provider)); } Ok(providers) @@ -3270,7 +3279,7 @@ async fn update_provider_record( .await .map_err(|e| Status::internal(format!("persist provider failed: {e}")))?; - Ok(updated) + Ok(redact_provider_credentials(updated)) } async fn delete_provider_record( @@ -3428,14 +3437,23 @@ mod tests { .await .unwrap(); assert_eq!(updated.id, provider_id); - // Updated credential has new value. + // Credentials are redacted in responses. + assert!( + updated.credentials.is_empty(), + "credentials must be redacted in gRPC responses" + ); + // Verify the store still has full credentials. + let stored: Provider = store + .get_message_by_name("gitlab-local") + .await + .unwrap() + .unwrap(); assert_eq!( - updated.credentials.get("API_TOKEN"), + stored.credentials.get("API_TOKEN"), Some(&"rotated-token".to_string()) ); - // Non-updated credential is preserved (not clobbered). assert_eq!( - updated.credentials.get("SECONDARY"), + stored.credentials.get("SECONDARY"), Some(&"secondary-token".to_string()) ); // Updated config has new value. @@ -3528,21 +3546,21 @@ mod tests { assert_eq!(updated.id, persisted.id); assert_eq!(updated.r#type, "nvidia"); - assert_eq!(updated.credentials.len(), 2); - assert_eq!( - updated.credentials.get("API_TOKEN"), - Some(&"token-123".to_string()) - ); - assert_eq!( - updated.credentials.get("SECONDARY"), - Some(&"secondary-token".to_string()) - ); + // Credentials are redacted in responses. + assert!(updated.credentials.is_empty()); assert_eq!(updated.config.len(), 2); assert_eq!( updated.config.get("endpoint"), Some(&"https://example.com".to_string()) ); assert_eq!(updated.config.get("region"), Some(&"us-west".to_string())); + // Verify the store still has full credentials. + let stored: Provider = store + .get_message_by_name("noop-test") + .await + .unwrap() + .unwrap(); + assert_eq!(stored.credentials.len(), 2); } #[tokio::test] @@ -3568,18 +3586,26 @@ mod tests { .await .unwrap(); - assert_eq!(updated.credentials.len(), 1); - assert_eq!( - updated.credentials.get("API_TOKEN"), - Some(&"token-123".to_string()) - ); - assert!(updated.credentials.get("SECONDARY").is_none()); + // Credentials are redacted in responses. + assert!(updated.credentials.is_empty()); assert_eq!(updated.config.len(), 1); assert_eq!( updated.config.get("endpoint"), Some(&"https://example.com".to_string()) ); assert!(updated.config.get("region").is_none()); + // Verify the store has the correct credential state (SECONDARY deleted). + let stored: Provider = store + .get_message_by_name("delete-key-test") + .await + .unwrap() + .unwrap(); + assert_eq!(stored.credentials.len(), 1); + assert_eq!( + stored.credentials.get("API_TOKEN"), + Some(&"token-123".to_string()) + ); + assert!(stored.credentials.get("SECONDARY").is_none()); } #[tokio::test] From 1a980f1d105765be4dd7ec894f2e1fdfc7e89fdc Mon Sep 17 00:00:00 2001 From: John Myers <9696606+johntmyers@users.noreply.github.com> Date: Sun, 15 Mar 2026 23:50:04 -0700 Subject: [PATCH 06/13] fix(sandbox): enforce non-root fallback when process user unset drop_privileges silently returned Ok(()) when both run_as_user and run_as_group were None, even when running as root. In local/dev mode policies are loaded from disk without passing through the server-side ensure_sandbox_process_identity normalization, so child processes could retain root and all capabilities (SYS_ADMIN, NET_ADMIN). When running as root with no process identity configured, fall back to sandbox:sandbox instead of no-oping. Non-root runtimes are unaffected. Refs: #350 --- crates/openshell-sandbox/src/process.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/crates/openshell-sandbox/src/process.rs b/crates/openshell-sandbox/src/process.rs index cb10b8ca..d3b4ecac 100644 --- a/crates/openshell-sandbox/src/process.rs +++ b/crates/openshell-sandbox/src/process.rs @@ -360,7 +360,18 @@ pub fn drop_privileges(policy: &SandboxPolicy) -> Result<()> { _ => None, }; + // If no user/group is configured and we are running as root, fall back to + // "sandbox:sandbox" instead of silently keeping root. This covers the + // local/dev-mode path where policies are loaded from disk and never pass + // through the server-side `ensure_sandbox_process_identity` normalization. + // For non-root runtimes, the no-op is safe -- we are already unprivileged. if user_name.is_none() && group_name.is_none() { + if nix::unistd::geteuid().is_root() { + let mut fallback = policy.clone(); + fallback.process.run_as_user = Some("sandbox".into()); + fallback.process.run_as_group = Some("sandbox".into()); + return drop_privileges(&fallback); + } return Ok(()); } From 8f188de5c33e2c1a6dec549d6e3e55a351d6d280 Mon Sep 17 00:00:00 2001 From: John Myers <9696606+johntmyers@users.noreply.github.com> Date: Sun, 15 Mar 2026 23:55:40 -0700 Subject: [PATCH 07/13] fix(sandbox): deny forward proxy for L7-configured endpoints The forward proxy path only performed L4 (endpoint) and allowed_ips checks. If an endpoint had L7 rules (method/path restrictions), a sandboxed process could bypass them by using HTTP_PROXY with plain http:// requests instead of CONNECT tunneling, since L7 inspection only runs in the CONNECT path. Add a guard in handle_forward_proxy that queries the endpoint's L7 config and returns 403 if any L7 rules are present, forcing traffic through the CONNECT path where per-request inspection happens. Refs: #350 --- crates/openshell-sandbox/src/proxy.rs | 28 +++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/crates/openshell-sandbox/src/proxy.rs b/crates/openshell-sandbox/src/proxy.rs index 6fe26b37..44234f66 100644 --- a/crates/openshell-sandbox/src/proxy.rs +++ b/crates/openshell-sandbox/src/proxy.rs @@ -1647,6 +1647,34 @@ async fn handle_forward_proxy( }; let policy_str = matched_policy.as_deref().unwrap_or("-"); + // 4b. Reject if the endpoint has L7 config — the forward proxy path does + // not perform per-request method/path inspection, so L7-configured + // endpoints must go through the CONNECT tunnel where inspection happens. + if query_l7_config(&opa_engine, &decision, &host_lc, port).is_some() { + info!( + dst_host = %host_lc, + dst_port = port, + method = %method, + path = %path, + binary = %binary_str, + policy = %policy_str, + action = "deny", + reason = "endpoint has L7 rules; use CONNECT", + "FORWARD", + ); + emit_denial_simple( + denial_tx, + &host_lc, + port, + &binary_str, + &decision, + "endpoint has L7 rules configured; forward proxy bypasses L7 inspection — use CONNECT", + "forward-l7-bypass", + ); + respond(client, b"HTTP/1.1 403 Forbidden\r\n\r\n").await?; + return Ok(()); + } + // 5. DNS resolution + SSRF defence (mirrors the CONNECT path logic). // - If allowed_ips is set: validate resolved IPs against the allowlist // (this is the SSRF override for private IP destinations). From 7b3da536dcad1b0ade24ae7a5b53ef73ac12e988 Mon Sep 17 00:00:00 2001 From: John Myers <9696606+johntmyers@users.noreply.github.com> Date: Sun, 15 Mar 2026 23:59:53 -0700 Subject: [PATCH 08/13] test(e2e): add regression test for forward proxy L7 bypass Verifies that the forward proxy path (plain http:// via HTTP_PROXY) returns 403 for endpoints with L7 rules configured, preventing sandboxed processes from bypassing per-request method/path enforcement by avoiding the CONNECT tunnel. Refs: #350 --- e2e/rust/tests/forward_proxy_l7_bypass.rs | 217 ++++++++++++++++++++++ 1 file changed, 217 insertions(+) create mode 100644 e2e/rust/tests/forward_proxy_l7_bypass.rs diff --git a/e2e/rust/tests/forward_proxy_l7_bypass.rs b/e2e/rust/tests/forward_proxy_l7_bypass.rs new file mode 100644 index 00000000..3e913607 --- /dev/null +++ b/e2e/rust/tests/forward_proxy_l7_bypass.rs @@ -0,0 +1,217 @@ +// SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +//! Regression test: the forward proxy path must reject requests to endpoints +//! that have L7 rules configured. Before the fix, plain `http://` requests +//! (which use the forward proxy, not CONNECT) bypassed per-request method/path +//! enforcement entirely. + +#![cfg(feature = "e2e")] + +use std::io::Write; +use std::process::Command; +use std::time::Duration; + +use openshell_e2e::harness::port::find_free_port; +use openshell_e2e::harness::sandbox::SandboxGuard; +use tempfile::NamedTempFile; +use tokio::time::{interval, timeout}; + +const TEST_SERVER_IMAGE: &str = "python:3.13-alpine"; + +struct DockerServer { + port: u16, + container_id: String, +} + +impl DockerServer { + async fn start() -> Result { + let port = find_free_port(); + let script = r#"from http.server import BaseHTTPRequestHandler, HTTPServer + +class Handler(BaseHTTPRequestHandler): + def do_GET(self): + self.send_response(200) + self.end_headers() + self.wfile.write(b'{"ok":true}') + def do_DELETE(self): + self.send_response(200) + self.end_headers() + self.wfile.write(b'{"ok":true}') + def log_message(self, format, *args): + pass + +HTTPServer(("0.0.0.0", 8000), Handler).serve_forever() +"#; + + let output = Command::new("docker") + .args([ + "run", + "--detach", + "--rm", + "-p", + &format!("{port}:8000"), + TEST_SERVER_IMAGE, + "python3", + "-c", + script, + ]) + .output() + .map_err(|e| format!("start docker test server: {e}"))?; + + let stdout = String::from_utf8_lossy(&output.stdout).trim().to_string(); + let stderr = String::from_utf8_lossy(&output.stderr).to_string(); + + if !output.status.success() { + return Err(format!( + "docker run failed (exit {:?}):\n{stderr}", + output.status.code() + )); + } + + let server = Self { + port, + container_id: stdout, + }; + server.wait_until_ready().await?; + Ok(server) + } + + async fn wait_until_ready(&self) -> Result<(), String> { + let container_id = self.container_id.clone(); + timeout(Duration::from_secs(30), async move { + let mut tick = interval(Duration::from_millis(500)); + loop { + tick.tick().await; + let output = Command::new("docker") + .args([ + "exec", + &container_id, + "python3", + "-c", + "import urllib.request; urllib.request.urlopen('http://127.0.0.1:8000', timeout=1).read()", + ]) + .output() + .ok(); + if output.is_some_and(|o| o.status.success()) { + return; + } + } + }) + .await + .map_err(|_| "docker test server did not become ready within 30s".to_string()) + } +} + +impl Drop for DockerServer { + fn drop(&mut self) { + let _ = Command::new("docker") + .args(["rm", "-f", &self.container_id]) + .output(); + } +} + +fn write_policy_with_l7_rules(port: u16) -> Result { + let mut file = NamedTempFile::new().map_err(|e| format!("create temp policy file: {e}"))?; + let policy = format!( + r#"version: 1 + +filesystem_policy: + include_workdir: true + read_only: + - /usr + - /lib + - /proc + - /dev/urandom + - /app + - /etc + - /var/log + read_write: + - /sandbox + - /tmp + - /dev/null + +landlock: + compatibility: best_effort + +process: + run_as_user: sandbox + run_as_group: sandbox + +network_policies: + test_l7: + name: test_l7 + endpoints: + - host: host.openshell.internal + port: {port} + protocol: rest + allowed_ips: + - "172.0.0.0/8" + rules: + - allow: + method: GET + path: /allowed + binaries: + - path: /usr/bin/curl + - path: /usr/bin/python* + - path: /usr/local/bin/python* +"# + ); + file.write_all(policy.as_bytes()) + .map_err(|e| format!("write temp policy file: {e}"))?; + file.flush() + .map_err(|e| format!("flush temp policy file: {e}"))?; + Ok(file) +} + +/// The forward proxy path (plain http:// via HTTP_PROXY) must return 403 for +/// endpoints with L7 rules, forcing clients through the CONNECT tunnel where +/// per-request method/path inspection actually happens. +#[tokio::test] +async fn forward_proxy_rejects_l7_configured_endpoint() { + let server = DockerServer::start() + .await + .expect("start docker test server"); + let policy = write_policy_with_l7_rules(server.port).expect("write custom policy"); + let policy_path = policy + .path() + .to_str() + .expect("temp policy path should be utf-8") + .to_string(); + + // Python script that tries a plain http:// request (forward proxy path). + // HTTP_PROXY is set automatically by the sandbox, so urllib will use the + // forward proxy for http:// URLs (not CONNECT). + let script = format!( + r#" +import urllib.request, urllib.error, json, sys +url = "http://host.openshell.internal:{port}/allowed" +try: + resp = urllib.request.urlopen(url, timeout=15) + print(json.dumps({{"status": resp.status, "error": None}})) +except urllib.error.HTTPError as e: + print(json.dumps({{"status": e.code, "error": str(e)}})) +except Exception as e: + print(json.dumps({{"status": -1, "error": str(e)}})) +"#, + port = server.port, + ); + + let guard = SandboxGuard::create(&[ + "--policy", + &policy_path, + "--", + "python3", + "-c", + &script, + ]) + .await + .expect("sandbox create"); + + // The forward proxy should return 403 because the endpoint has L7 rules. + assert!( + guard.create_output.contains("\"status\": 403"), + "expected 403 from forward proxy for L7-configured endpoint, got:\n{}", + guard.create_output + ); +} From 04525fe96887290cdecff8c215b8395453931c63 Mon Sep 17 00:00:00 2001 From: John Myers <9696606+johntmyers@users.noreply.github.com> Date: Mon, 16 Mar 2026 00:15:03 -0700 Subject: [PATCH 09/13] fix: update tests for cmdline path and TLS volume changes Update OPA tests to verify cmdline_paths no longer grant access (regression tests for the security fix). Fix formatting in TLS volume test. Refs: #350 --- crates/openshell-sandbox/src/opa.rs | 45 ++++++++++------------ crates/openshell-server/src/sandbox/mod.rs | 3 +- 2 files changed, 22 insertions(+), 26 deletions(-) diff --git a/crates/openshell-sandbox/src/opa.rs b/crates/openshell-sandbox/src/opa.rs index 705f0e62..a85b27e5 100644 --- a/crates/openshell-sandbox/src/opa.rs +++ b/crates/openshell-sandbox/src/opa.rs @@ -862,12 +862,10 @@ mod tests { let config = engine.query_sandbox_config().unwrap(); assert!(config.filesystem.include_workdir); assert!(config.filesystem.read_only.contains(&PathBuf::from("/usr"))); - assert!( - config - .filesystem - .read_write - .contains(&PathBuf::from("/tmp")) - ); + assert!(config + .filesystem + .read_write + .contains(&PathBuf::from("/tmp"))); } #[test] @@ -1098,9 +1096,11 @@ network_policies: } #[test] - fn cmdline_path_matches_script_binary() { - // Simulates: node runs /usr/local/bin/my-tool (a script with shebang) - // exe = /usr/bin/node, cmdline contains /usr/local/bin/my-tool + fn cmdline_path_does_not_grant_access() { + // Simulates: node runs /usr/local/bin/my-tool (a script with shebang). + // exe = /usr/bin/node, cmdline contains /usr/local/bin/my-tool. + // cmdline_paths are attacker-controlled (argv[0] spoofing) and must + // NOT be used as a grant-access signal. let cmdline_data = r" network_policies: script_test: @@ -1121,11 +1121,9 @@ network_policies: }; let decision = engine.evaluate_network(&input).unwrap(); assert!( - decision.allowed, - "Expected allow via cmdline path match, got deny: {}", - decision.reason + !decision.allowed, + "cmdline_paths must not grant network access (argv[0] is spoofable)" ); - assert_eq!(decision.matched_policy.as_deref(), Some("script_test")); } #[test] @@ -1156,7 +1154,7 @@ network_policies: } #[test] - fn cmdline_glob_pattern_matches() { + fn cmdline_glob_pattern_does_not_grant_access() { let glob_data = r#" network_policies: glob_test: @@ -1177,9 +1175,8 @@ network_policies: }; let decision = engine.evaluate_network(&input).unwrap(); assert!( - decision.allowed, - "Expected glob to match cmdline path, got deny: {}", - decision.reason + !decision.allowed, + "cmdline_paths must not match globs for granting access (argv[0] is spoofable)" ); } @@ -1190,10 +1187,10 @@ network_policies: let input = NetworkInput { host: "api.anthropic.com".into(), port: 443, - binary_path: PathBuf::from("/usr/bin/node"), + binary_path: PathBuf::from("/usr/local/bin/claude"), binary_sha256: "unused".into(), ancestors: vec![], - cmdline_paths: vec![PathBuf::from("/usr/local/bin/claude")], + cmdline_paths: vec![], }; let decision = engine.evaluate_network(&input).unwrap(); assert!( @@ -1227,12 +1224,10 @@ network_policies: let config = engine.query_sandbox_config().unwrap(); assert!(config.filesystem.include_workdir); assert!(config.filesystem.read_only.contains(&PathBuf::from("/usr"))); - assert!( - config - .filesystem - .read_write - .contains(&PathBuf::from("/tmp")) - ); + assert!(config + .filesystem + .read_write + .contains(&PathBuf::from("/tmp"))); assert_eq!(config.process.run_as_user.as_deref(), Some("sandbox")); assert_eq!(config.process.run_as_group.as_deref(), Some("sandbox")); } diff --git a/crates/openshell-server/src/sandbox/mod.rs b/crates/openshell-server/src/sandbox/mod.rs index d8eb0410..bb943c12 100644 --- a/crates/openshell-server/src/sandbox/mod.rs +++ b/crates/openshell-server/src/sandbox/mod.rs @@ -2101,7 +2101,8 @@ mod tests { .find(|v| v["name"] == "openshell-client-tls") .expect("TLS volume should exist"); assert_eq!( - tls_vol["secret"]["defaultMode"], 256, // 0o400 + tls_vol["secret"]["defaultMode"], + 256, // 0o400 "TLS secret volume must use mode 0400 to prevent sandbox user from reading the private key" ); } From 11e6cb82de497141a9a6c65e92e389a80898d3c8 Mon Sep 17 00:00:00 2001 From: John Myers <9696606+johntmyers@users.noreply.github.com> Date: Mon, 16 Mar 2026 00:28:14 -0700 Subject: [PATCH 10/13] test(e2e): update provider e2e tests for credential redaction Provider CRUD gRPC responses no longer include credential values. Update three e2e tests to assert credentials are empty in responses. Provider functionality is verified by existing e2e tests that check env var injection into sandboxes (which read from the store directly). Refs: #350 --- e2e/python/test_sandbox_providers.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/e2e/python/test_sandbox_providers.py b/e2e/python/test_sandbox_providers.py index 899b6e46..2cdba29c 100644 --- a/e2e/python/test_sandbox_providers.py +++ b/e2e/python/test_sandbox_providers.py @@ -280,8 +280,8 @@ def test_update_provider_preserves_unset_credentials_and_config( got = stub.GetProvider(openshell_pb2.GetProviderRequest(name=name)) p = got.provider - assert p.credentials["KEY_A"] == "rotated-a" - assert p.credentials["KEY_B"] == "val-b", "KEY_B should be preserved" + # Credentials are redacted in gRPC responses (security hardening). + assert len(p.credentials) == 0, "credentials must be redacted in gRPC responses" assert p.config["BASE_URL"] == "https://example.com", ( "config should be preserved" ) @@ -320,7 +320,8 @@ def test_update_provider_empty_maps_preserves_all( got = stub.GetProvider(openshell_pb2.GetProviderRequest(name=name)) p = got.provider - assert p.credentials["TOKEN"] == "secret" + # Credentials are redacted in gRPC responses (security hardening). + assert len(p.credentials) == 0, "credentials must be redacted in gRPC responses" assert p.config["URL"] == "https://api.example.com" finally: _delete_provider(stub, name) @@ -358,9 +359,8 @@ def test_update_provider_merges_config_preserves_credentials( got = stub.GetProvider(openshell_pb2.GetProviderRequest(name=name)) p = got.provider - assert p.credentials["API_KEY"] == "original-key", ( - "credentials should be untouched" - ) + # Credentials are redacted in gRPC responses (security hardening). + assert len(p.credentials) == 0, "credentials must be redacted in gRPC responses" assert p.config["ENDPOINT"] == "https://new.example.com" finally: _delete_provider(stub, name) From 9950592ff2397565ba189b290bf1a57e24ffd5bf Mon Sep 17 00:00:00 2001 From: John Myers <9696606+johntmyers@users.noreply.github.com> Date: Mon, 16 Mar 2026 00:39:41 -0700 Subject: [PATCH 11/13] chore: reformat for Rust 1.94 assert! macro style --- crates/openshell-sandbox/src/opa.rs | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/crates/openshell-sandbox/src/opa.rs b/crates/openshell-sandbox/src/opa.rs index a85b27e5..a86fd683 100644 --- a/crates/openshell-sandbox/src/opa.rs +++ b/crates/openshell-sandbox/src/opa.rs @@ -862,10 +862,12 @@ mod tests { let config = engine.query_sandbox_config().unwrap(); assert!(config.filesystem.include_workdir); assert!(config.filesystem.read_only.contains(&PathBuf::from("/usr"))); - assert!(config - .filesystem - .read_write - .contains(&PathBuf::from("/tmp"))); + assert!( + config + .filesystem + .read_write + .contains(&PathBuf::from("/tmp")) + ); } #[test] @@ -1224,10 +1226,12 @@ network_policies: let config = engine.query_sandbox_config().unwrap(); assert!(config.filesystem.include_workdir); assert!(config.filesystem.read_only.contains(&PathBuf::from("/usr"))); - assert!(config - .filesystem - .read_write - .contains(&PathBuf::from("/tmp"))); + assert!( + config + .filesystem + .read_write + .contains(&PathBuf::from("/tmp")) + ); assert_eq!(config.process.run_as_user.as_deref(), Some("sandbox")); assert_eq!(config.process.run_as_group.as_deref(), Some("sandbox")); } From c14b6946ac3820456dd5e57cb2d115f8682128aa Mon Sep 17 00:00:00 2001 From: John Myers <9696606+johntmyers@users.noreply.github.com> Date: Mon, 16 Mar 2026 00:50:34 -0700 Subject: [PATCH 12/13] fix(sandbox): make drop_privileges tests root-aware for CI CI runs as root but has no 'sandbox' user. The security fix correctly errors when running as root with no process identity and no fallback user available -- this is the intended behavior (refuse to run as root). Update tests to expect that error in root-without-sandbox-user environments instead of unconditionally asserting Ok. Refs: #350 --- crates/openshell-sandbox/src/process.rs | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/crates/openshell-sandbox/src/process.rs b/crates/openshell-sandbox/src/process.rs index d3b4ecac..b93d125a 100644 --- a/crates/openshell-sandbox/src/process.rs +++ b/crates/openshell-sandbox/src/process.rs @@ -533,7 +533,15 @@ mod tests { run_as_user: None, run_as_group: None, }); - assert!(drop_privileges(&policy).is_ok()); + if nix::unistd::geteuid().is_root() { + // As root, drop_privileges falls back to "sandbox:sandbox". + // If that user exists, it succeeds; if not (e.g. CI), it + // must error rather than silently keep root. + let has_sandbox = User::from_name("sandbox").ok().flatten().is_some(); + assert_eq!(drop_privileges(&policy).is_ok(), has_sandbox); + } else { + assert!(drop_privileges(&policy).is_ok()); + } } #[test] @@ -542,7 +550,12 @@ mod tests { run_as_user: Some(String::new()), run_as_group: Some(String::new()), }); - assert!(drop_privileges(&policy).is_ok()); + if nix::unistd::geteuid().is_root() { + let has_sandbox = User::from_name("sandbox").ok().flatten().is_some(); + assert_eq!(drop_privileges(&policy).is_ok(), has_sandbox); + } else { + assert!(drop_privileges(&policy).is_ok()); + } } #[test] From d5362783086a63286a0e0e1b79ddc6a7130d25e4 Mon Sep 17 00:00:00 2001 From: John Myers <9696606+johntmyers@users.noreply.github.com> Date: Mon, 16 Mar 2026 01:20:48 -0700 Subject: [PATCH 13/13] fix(sandbox): allow non-directory read_write entries like /dev/null The symlink guard incorrectly rejected all non-directory entries. Character devices like /dev/null are legitimate read_write paths used in sandbox policies. Only reject symlinks, which are the actual attack vector for the chown privilege escalation. Refs: #350 --- crates/openshell-sandbox/src/lib.rs | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/crates/openshell-sandbox/src/lib.rs b/crates/openshell-sandbox/src/lib.rs index 877fb53c..754c3be0 100644 --- a/crates/openshell-sandbox/src/lib.rs +++ b/crates/openshell-sandbox/src/lib.rs @@ -1208,7 +1208,8 @@ fn prepare_filesystem(policy: &SandboxPolicy) -> Result<()> { // The TOCTOU window between lstat and chown is not exploitable because // no untrusted process is running yet (the child has not been forked). for path in &policy.filesystem.read_write { - // Check for symlinks or non-directory files before touching the path. + // Check for symlinks before touching the path. Character/block devices + // (e.g. /dev/null) are legitimate read_write entries and must be allowed. if let Ok(meta) = std::fs::symlink_metadata(path) { if meta.file_type().is_symlink() { return Err(miette::miette!( @@ -1216,12 +1217,6 @@ fn prepare_filesystem(policy: &SandboxPolicy) -> Result<()> { path.display() )); } - if !meta.file_type().is_dir() { - return Err(miette::miette!( - "read_write path '{}' exists but is not a directory — refusing to chown", - path.display() - )); - } } else { debug!(path = %path.display(), "Creating read_write directory"); std::fs::create_dir_all(path).into_diagnostic()?;