diff --git a/crates/openshell-sandbox/src/child_env.rs b/crates/openshell-sandbox/src/child_env.rs index 2e615b8f8b..014ded65a4 100644 --- a/crates/openshell-sandbox/src/child_env.rs +++ b/crates/openshell-sandbox/src/child_env.rs @@ -71,6 +71,18 @@ pub(crate) fn tls_env_vars( ] } +/// Serializes tests that read or write `OPENSHELL_DIRECT_TCP_*`. +/// +/// `std::env::set_var` / `remove_var` are unsafe because a concurrent read or +/// write of the environment is undefined behavior. Tests run in parallel by +/// default, and the tests in this module and in `sandbox::linux::netns` touch +/// the same variables, so every such test holds this lock for its whole body. +#[cfg(test)] +pub(crate) fn lock_direct_tcp_env() -> std::sync::MutexGuard<'static, ()> { + static LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + LOCK.lock().unwrap_or_else(std::sync::PoisonError::into_inner) +} + #[cfg(test)] mod tests { use super::*; @@ -79,8 +91,10 @@ mod tests { #[test] fn apply_proxy_env_includes_node_proxy_opt_in_and_local_bypass() { + let _env = lock_direct_tcp_env(); // Ensure no leftover env from other tests affects NO_PROXY - std::env::remove_var("OPENSHELL_DIRECT_TCP_HOSTS"); + // SAFETY: test-only; all tests touching these env vars hold lock_direct_tcp_env(). + unsafe { std::env::remove_var("OPENSHELL_DIRECT_TCP_HOSTS") }; let mut cmd = Command::new("/usr/bin/env"); cmd.stdin(Stdio::null()) @@ -102,11 +116,14 @@ mod tests { #[test] fn no_proxy_includes_direct_tcp_hosts() { - std::env::remove_var("OPENSHELL_DIRECT_TCP_ENDPOINTS"); - std::env::set_var( + let _env = lock_direct_tcp_env(); + // SAFETY: test-only; all tests touching these env vars hold lock_direct_tcp_env(). + unsafe { std::env::remove_var("OPENSHELL_DIRECT_TCP_ENDPOINTS") }; + // SAFETY: test-only; all tests touching these env vars hold lock_direct_tcp_env(). + unsafe { std::env::set_var( "OPENSHELL_DIRECT_TCP_HOSTS", "oauth2.googleapis.com,gmail.googleapis.com", - ); + ) }; let no_proxy = build_no_proxy(); assert_eq!( @@ -115,16 +132,20 @@ mod tests { ); // Clean up - std::env::remove_var("OPENSHELL_DIRECT_TCP_HOSTS"); + // SAFETY: test-only; all tests touching these env vars hold lock_direct_tcp_env(). + unsafe { std::env::remove_var("OPENSHELL_DIRECT_TCP_HOSTS") }; } #[test] fn no_proxy_includes_direct_tcp_endpoints() { - std::env::remove_var("OPENSHELL_DIRECT_TCP_HOSTS"); - std::env::set_var( + let _env = lock_direct_tcp_env(); + // SAFETY: test-only; all tests touching these env vars hold lock_direct_tcp_env(). + unsafe { std::env::remove_var("OPENSHELL_DIRECT_TCP_HOSTS") }; + // SAFETY: test-only; all tests touching these env vars hold lock_direct_tcp_env(). + unsafe { std::env::set_var( "OPENSHELL_DIRECT_TCP_ENDPOINTS", "10.0.1.215:5432, 10.0.1.215:6379 , db.internal:1025,", - ); + ) }; let no_proxy = build_no_proxy(); assert_eq!( @@ -132,7 +153,8 @@ mod tests { "127.0.0.1,localhost,::1,10.0.1.215,10.0.1.215,db.internal" ); - std::env::remove_var("OPENSHELL_DIRECT_TCP_ENDPOINTS"); + // SAFETY: test-only; all tests touching these env vars hold lock_direct_tcp_env(). + unsafe { std::env::remove_var("OPENSHELL_DIRECT_TCP_ENDPOINTS") }; } #[test] diff --git a/crates/openshell-sandbox/src/proxy.rs b/crates/openshell-sandbox/src/proxy.rs index b52cc60b90..8cd561a524 100644 --- a/crates/openshell-sandbox/src/proxy.rs +++ b/crates/openshell-sandbox/src/proxy.rs @@ -1916,10 +1916,15 @@ fn rewrite_forward_request( output.extend_from_slice(&raw[header_end..used]); } - // Fail-closed: scan for any remaining unresolved placeholders + // Fail-closed: scan for any remaining unresolved placeholders or + // provider-shaped aliases, including the percent-decoded request line. if secret_resolver.is_some() { let output_str = String::from_utf8_lossy(&output); - if output_str.contains(crate::secrets::PLACEHOLDER_PREFIX_PUBLIC) { + let request_line = output_str.split("\r\n").next().unwrap_or(""); + if crate::secrets::request_line_has_credential_marker(request_line) { + return Err(crate::secrets::UnresolvedPlaceholderError { location: "path" }); + } + if crate::secrets::contains_credential_marker(&output_str) { return Err(crate::secrets::UnresolvedPlaceholderError { location: "header" }); } } @@ -3170,6 +3175,59 @@ mod tests { assert!(!result_str.contains("openshell:resolve:env:ANTHROPIC_API_KEY")); } + fn github_alias_resolver() -> Option { + SecretResolver::from_provider_env( + [("GITHUB_TOKEN".to_string(), "github_pat_REAL".to_string())] + .into_iter() + .collect(), + ) + .1 + } + + #[test] + fn test_rewrite_resolves_provider_alias_auth_header() { + let resolver = github_alias_resolver(); + let raw = b"GET http://host/p HTTP/1.1\r\nHost: host\r\nAuthorization: token github_pat_OPENSHELL-RESOLVE-ENV-GITHUB_TOKEN\r\n\r\n"; + let result = rewrite_forward_request(raw, raw.len(), "/p", resolver.as_ref()) + .expect("should succeed"); + let result_str = String::from_utf8_lossy(&result); + assert!(result_str.contains("Authorization: token github_pat_REAL")); + assert!(!result_str.contains("OPENSHELL-RESOLVE-ENV-")); + } + + #[test] + fn test_rewrite_rejects_unknown_provider_alias() { + let resolver = github_alias_resolver(); + let raw = b"GET http://host/p HTTP/1.1\r\nHost: host\r\nAuthorization: token github_pat_OPENSHELL-RESOLVE-ENV-OTHER_TOKEN\r\n\r\n"; + let err = rewrite_forward_request(raw, raw.len(), "/p", resolver.as_ref()) + .expect_err("unresolved alias must fail closed"); + assert_eq!(err.location, "header"); + } + + #[test] + fn test_rewrite_rejects_provider_alias_in_path() { + let resolver = github_alias_resolver(); + let raw = b"GET http://host/p HTTP/1.1\r\nHost: host\r\n\r\n"; + assert!( + rewrite_forward_request( + raw, + raw.len(), + "/p?t=github_pat_OPENSHELL-RESOLVE-ENV-GITHUB_TOKEN", + resolver.as_ref(), + ) + .is_err() + ); + assert!( + rewrite_forward_request( + raw, + raw.len(), + "/p?t=github_pat_OPENSHELL%2DRESOLVE%2DENV%2DGITHUB_TOKEN", + resolver.as_ref(), + ) + .is_err() + ); + } + // --- Forward proxy SSRF defence tests --- // // The forward proxy handler uses the same SSRF logic as the CONNECT path: diff --git a/crates/openshell-sandbox/src/sandbox/linux/netns.rs b/crates/openshell-sandbox/src/sandbox/linux/netns.rs index bd24e647bc..3d315cf299 100644 --- a/crates/openshell-sandbox/src/sandbox/linux/netns.rs +++ b/crates/openshell-sandbox/src/sandbox/linux/netns.rs @@ -1152,31 +1152,40 @@ mod tests { #[test] fn test_parse_direct_tcp_hosts() { - std::env::set_var( + let _env = crate::child_env::lock_direct_tcp_env(); + // SAFETY: test-only; all tests touching these env vars hold lock_direct_tcp_env(). + unsafe { std::env::set_var( "OPENSHELL_DIRECT_TCP_HOSTS", "oauth2.googleapis.com, gmail.googleapis.com , ", - ); + ) }; let hosts = parse_direct_tcp_hosts(); assert_eq!(hosts, vec!["oauth2.googleapis.com", "gmail.googleapis.com"]); - std::env::remove_var("OPENSHELL_DIRECT_TCP_HOSTS"); + // SAFETY: test-only; all tests touching these env vars hold lock_direct_tcp_env(). + unsafe { std::env::remove_var("OPENSHELL_DIRECT_TCP_HOSTS") }; } #[test] fn test_parse_direct_tcp_hosts_empty() { - std::env::remove_var("OPENSHELL_DIRECT_TCP_HOSTS"); + let _env = crate::child_env::lock_direct_tcp_env(); + // SAFETY: test-only; all tests touching these env vars hold lock_direct_tcp_env(). + unsafe { std::env::remove_var("OPENSHELL_DIRECT_TCP_HOSTS") }; assert!(parse_direct_tcp_hosts().is_empty()); - std::env::set_var("OPENSHELL_DIRECT_TCP_HOSTS", ""); + // SAFETY: test-only; all tests touching these env vars hold lock_direct_tcp_env(). + unsafe { std::env::set_var("OPENSHELL_DIRECT_TCP_HOSTS", "") }; assert!(parse_direct_tcp_hosts().is_empty()); - std::env::remove_var("OPENSHELL_DIRECT_TCP_HOSTS"); + // SAFETY: test-only; all tests touching these env vars hold lock_direct_tcp_env(). + unsafe { std::env::remove_var("OPENSHELL_DIRECT_TCP_HOSTS") }; } #[test] fn test_parse_direct_tcp_endpoints_basic() { - std::env::set_var( + let _env = crate::child_env::lock_direct_tcp_env(); + // SAFETY: test-only; all tests touching these env vars hold lock_direct_tcp_env(). + unsafe { std::env::set_var( "OPENSHELL_DIRECT_TCP_ENDPOINTS", "10.0.1.215:5432, 10.0.1.215:6379 , db.internal:1025,", - ); + ) }; let eps = parse_direct_tcp_endpoints(); assert_eq!( eps, @@ -1186,30 +1195,38 @@ mod tests { DirectTcpEndpoint { host: "db.internal".into(), port: 1025 }, ] ); - std::env::remove_var("OPENSHELL_DIRECT_TCP_ENDPOINTS"); + // SAFETY: test-only; all tests touching these env vars hold lock_direct_tcp_env(). + unsafe { std::env::remove_var("OPENSHELL_DIRECT_TCP_ENDPOINTS") }; } #[test] fn test_parse_direct_tcp_endpoints_invalid_entries_skipped() { - std::env::set_var( + let _env = crate::child_env::lock_direct_tcp_env(); + // SAFETY: test-only; all tests touching these env vars hold lock_direct_tcp_env(). + unsafe { std::env::set_var( "OPENSHELL_DIRECT_TCP_ENDPOINTS", "host-no-port, :5432, host:abc, good.internal:8025", - ); + ) }; let eps = parse_direct_tcp_endpoints(); assert_eq!( eps, vec![DirectTcpEndpoint { host: "good.internal".into(), port: 8025 }] ); - std::env::remove_var("OPENSHELL_DIRECT_TCP_ENDPOINTS"); + // SAFETY: test-only; all tests touching these env vars hold lock_direct_tcp_env(). + unsafe { std::env::remove_var("OPENSHELL_DIRECT_TCP_ENDPOINTS") }; } #[test] fn test_parse_direct_tcp_endpoints_empty() { - std::env::remove_var("OPENSHELL_DIRECT_TCP_ENDPOINTS"); + let _env = crate::child_env::lock_direct_tcp_env(); + // SAFETY: test-only; all tests touching these env vars hold lock_direct_tcp_env(). + unsafe { std::env::remove_var("OPENSHELL_DIRECT_TCP_ENDPOINTS") }; assert!(parse_direct_tcp_endpoints().is_empty()); - std::env::set_var("OPENSHELL_DIRECT_TCP_ENDPOINTS", ""); + // SAFETY: test-only; all tests touching these env vars hold lock_direct_tcp_env(). + unsafe { std::env::set_var("OPENSHELL_DIRECT_TCP_ENDPOINTS", "") }; assert!(parse_direct_tcp_endpoints().is_empty()); - std::env::remove_var("OPENSHELL_DIRECT_TCP_ENDPOINTS"); + // SAFETY: test-only; all tests touching these env vars hold lock_direct_tcp_env(). + unsafe { std::env::remove_var("OPENSHELL_DIRECT_TCP_ENDPOINTS") }; } #[test] diff --git a/crates/openshell-sandbox/src/secrets.rs b/crates/openshell-sandbox/src/secrets.rs index a27537c910..dfb9c5c756 100644 --- a/crates/openshell-sandbox/src/secrets.rs +++ b/crates/openshell-sandbox/src/secrets.rs @@ -7,8 +7,16 @@ use std::fmt; const PLACEHOLDER_PREFIX: &str = "openshell:resolve:env:"; -/// Public access to the placeholder prefix for fail-closed scanning in other modules. -pub(crate) const PLACEHOLDER_PREFIX_PUBLIC: &str = PLACEHOLDER_PREFIX; + +/// Marker of a provider-shaped alias: `OPENSHELL-RESOLVE-ENV-`. +/// +/// Some clients validate the credential's shape before sending it (e.g. gh's +/// `--attach` only accepts `github_pat_` / `ghp_` / ... tokens) and cannot carry +/// the canonical `openshell:resolve:env:KEY` placeholder. They send an alias +/// such as `github_pat_OPENSHELL-RESOLVE-ENV-GITHUB_TOKEN` instead, which the +/// proxy resolves to the same secret as the canonical placeholder. +/// Ported from NVIDIA/OpenShell#1286 (header and request-line scope only). +const PROVIDER_ALIAS_MARKER: &str = "OPENSHELL-RESOLVE-ENV-"; /// Characters that are valid in an env var key name (used to extract /// placeholder boundaries within concatenated strings like path segments). @@ -16,6 +24,45 @@ fn is_env_key_char(b: u8) -> bool { b.is_ascii_alphanumeric() || b == b'_' } +/// Characters allowed in the shape prefix of a provider-shaped alias +/// (RFC 3986 unreserved characters). +fn is_alias_token_char(b: u8) -> bool { + b.is_ascii_alphanumeric() || matches!(b, b'_' | b'-' | b'.' | b'~') +} + +/// Extract the env key from a whole-token provider-shaped alias. +/// +/// The token must be `OPENSHELL-RESOLVE-ENV-` where the prefix is +/// non-empty and made of unreserved characters, and the key runs to the end of +/// the token. Anything else is not an alias. +fn alias_env_key(token: &str) -> Option<&str> { + let marker_start = token.find(PROVIDER_ALIAS_MARKER)?; + if marker_start == 0 { + return None; + } + if !token[..marker_start].bytes().all(is_alias_token_char) { + return None; + } + let key_start = marker_start + PROVIDER_ALIAS_MARKER.len(); + let key_end = token[key_start..] + .bytes() + .position(|b| !is_env_key_char(b)) + .map_or(token.len(), |p| key_start + p); + (key_end == token.len() && key_end > key_start).then_some(&token[key_start..key_end]) +} + +/// True if the text still carries a credential placeholder or alias marker. +/// Used by the fail-closed scans after rewriting. +pub(crate) fn contains_credential_marker(text: &str) -> bool { + text.contains(PLACEHOLDER_PREFIX) || text.contains(PROVIDER_ALIAS_MARKER) +} + +/// True if a request line carries a placeholder or alias marker in its raw or +/// percent-decoded form (F5 — encoded placeholder bypass). +pub(crate) fn request_line_has_credential_marker(line: &str) -> bool { + contains_credential_marker(line) || contains_credential_marker(&percent_decode(line)) +} + // --------------------------------------------------------------------------- // Error and result types // --------------------------------------------------------------------------- @@ -91,7 +138,15 @@ impl SecretResolver { /// Returns `None` if the placeholder is unknown or the resolved value /// contains prohibited control characters (CRLF, null byte). pub(crate) fn resolve_placeholder(&self, value: &str) -> Option<&str> { - let secret = self.by_placeholder.get(value).map(String::as_str)?; + let secret = match self.by_placeholder.get(value) { + Some(secret) => secret.as_str(), + None => { + let key = alias_env_key(value)?; + self.by_placeholder + .get(&placeholder_for_env_key(key)) + .map(String::as_str)? + } + }; match validate_resolved_secret(secret) { Ok(s) => Some(s), Err(reason) => { @@ -610,14 +665,14 @@ pub(crate) fn rewrite_http_header_block( // Fail-closed scan: check for any remaining unresolved placeholders // in both raw form and percent-decoded form of the output header block. let output_header = String::from_utf8_lossy(&output[..output.len().min(header_end + 256)]); - if output_header.contains(PLACEHOLDER_PREFIX) { + if contains_credential_marker(&output_header) { return Err(UnresolvedPlaceholderError { location: "header" }); } // Also check percent-decoded form of the request line (F5 — encoded placeholder bypass) let rewritten_rl = output_header.split("\r\n").next().unwrap_or(""); let decoded_rl = percent_decode(rewritten_rl); - if decoded_rl.contains(PLACEHOLDER_PREFIX) { + if contains_credential_marker(&decoded_rl) { return Err(UnresolvedPlaceholderError { location: "path" }); } @@ -646,6 +701,13 @@ pub(crate) fn rewrite_target_for_eval( target: &str, resolver: &SecretResolver, ) -> Result { + // Provider-shaped aliases are resolved in headers only. An alias in the + // target would never be resolved, so fail closed before it reaches OPA or + // the upstream (raw and percent-decoded forms). + if target.contains(PROVIDER_ALIAS_MARKER) || percent_decode(target).contains(PROVIDER_ALIAS_MARKER) { + return Err(UnresolvedPlaceholderError { location: "path" }); + } + if !target.contains(PLACEHOLDER_PREFIX) { // Also check percent-decoded form let decoded = percent_decode(target); @@ -1473,4 +1535,150 @@ mod tests { assert_eq!(result.resolved, "/bottok123/method?key=key456"); assert_eq!(result.redacted, "/bot[CREDENTIAL]/method?key=[CREDENTIAL]"); } + + // === Provider-shaped aliases (ported from NVIDIA/OpenShell#1286) === + // + // A client that validates the credential's shape before sending (e.g. gh's + // `--attach`, which only accepts `github_pat_` / `ghp_` / ... tokens) cannot + // carry the canonical `openshell:resolve:env:KEY` placeholder. Such a client + // sends `OPENSHELL-RESOLVE-ENV-KEY` instead, and the proxy must + // resolve it to the same secret as the canonical placeholder. + + fn github_resolver() -> SecretResolver { + let (_, resolver) = SecretResolver::from_provider_env( + [("GITHUB_TOKEN".to_string(), "github_pat_REAL".to_string())] + .into_iter() + .collect(), + ); + resolver.expect("resolver") + } + + #[test] + fn provider_alias_resolves_to_the_canonical_secret() { + let resolver = github_resolver(); + assert_eq!( + resolver.resolve_placeholder("github_pat_OPENSHELL-RESOLVE-ENV-GITHUB_TOKEN"), + Some("github_pat_REAL") + ); + } + + #[test] + fn provider_alias_in_token_scheme_header_is_rewritten() { + let resolver = github_resolver(); + assert_eq!( + rewrite_header_line( + "Authorization: token github_pat_OPENSHELL-RESOLVE-ENV-GITHUB_TOKEN", + &resolver, + ), + "Authorization: token github_pat_REAL" + ); + } + + #[test] + fn provider_alias_accepts_unreserved_prefix_characters() { + let resolver = github_resolver(); + assert_eq!( + resolver.resolve_placeholder("a.b~c-d_e9OPENSHELL-RESOLVE-ENV-GITHUB_TOKEN"), + Some("github_pat_REAL") + ); + } + + #[test] + fn provider_alias_with_unknown_key_is_not_resolved() { + let resolver = github_resolver(); + assert_eq!( + resolver.resolve_placeholder("github_pat_OPENSHELL-RESOLVE-ENV-OTHER_TOKEN"), + None + ); + } + + #[test] + fn provider_alias_requires_a_non_empty_prefix() { + let resolver = github_resolver(); + assert_eq!(resolver.resolve_placeholder("OPENSHELL-RESOLVE-ENV-GITHUB_TOKEN"), None); + } + + #[test] + fn provider_alias_rejects_prefix_with_reserved_characters() { + let resolver = github_resolver(); + assert_eq!( + resolver.resolve_placeholder("a/b:OPENSHELL-RESOLVE-ENV-GITHUB_TOKEN"), + None + ); + } + + #[test] + fn provider_alias_must_end_at_the_key() { + let resolver = github_resolver(); + assert_eq!( + resolver.resolve_placeholder("github_pat_OPENSHELL-RESOLVE-ENV-GITHUB_TOKEN!x"), + None + ); + assert_eq!(resolver.resolve_placeholder("github_pat_OPENSHELL-RESOLVE-ENV-"), None); + } + + #[test] + fn provider_alias_resolution_rejects_crlf_secret() { + let (_, resolver) = SecretResolver::from_provider_env( + [("GITHUB_TOKEN".to_string(), "bad\r\nX-Injected: 1".to_string())] + .into_iter() + .collect(), + ); + let resolver = resolver.expect("resolver"); + assert_eq!( + resolver.resolve_placeholder("github_pat_OPENSHELL-RESOLVE-ENV-GITHUB_TOKEN"), + None + ); + } + + #[test] + fn provider_alias_header_block_round_trip() { + let resolver = github_resolver(); + let raw = b"POST /user-attachments/assets?name=a.webm HTTP/1.1\r\nHost: uploads.github.com\r\nAuthorization: token github_pat_OPENSHELL-RESOLVE-ENV-GITHUB_TOKEN\r\nContent-Length: 4\r\n\r\nbody"; + let result = rewrite_http_header_block(raw, Some(&resolver)).expect("should resolve"); + let rewritten = String::from_utf8(result.rewritten).expect("utf8"); + assert!(rewritten.contains("Authorization: token github_pat_REAL\r\n")); + assert!(!rewritten.contains("OPENSHELL-RESOLVE-ENV-")); + assert!(rewritten.ends_with("\r\n\r\nbody")); + } + + #[test] + fn unresolved_provider_alias_in_header_fails_closed() { + let resolver = github_resolver(); + let raw = b"GET /user HTTP/1.1\r\nHost: api.github.com\r\nAuthorization: token github_pat_OPENSHELL-RESOLVE-ENV-OTHER_TOKEN\r\n\r\n"; + let err = rewrite_http_header_block(raw, Some(&resolver)).expect_err("must fail closed"); + assert_eq!(err.location, "header"); + } + + #[test] + fn provider_alias_in_request_line_fails_closed() { + let resolver = github_resolver(); + let raw = b"GET /x?t=github_pat_OPENSHELL-RESOLVE-ENV-GITHUB_TOKEN HTTP/1.1\r\nHost: api.github.com\r\n\r\n"; + assert!(rewrite_http_header_block(raw, Some(&resolver)).is_err()); + let encoded = b"GET /x%3Ft%3Dgithub_pat_OPENSHELL%2DRESOLVE%2DENV%2DGITHUB_TOKEN HTTP/1.1\r\nHost: api.github.com\r\n\r\n"; + assert!(rewrite_http_header_block(encoded, Some(&resolver)).is_err()); + } + + #[test] + fn rewrite_target_for_eval_rejects_provider_alias() { + // Aliases are resolved in headers only. One in the request target is + // never resolved, so it must fail closed instead of reaching OPA and upstream. + let resolver = github_resolver(); + let err = rewrite_target_for_eval("/x?t=github_pat_OPENSHELL-RESOLVE-ENV-GITHUB_TOKEN", &resolver) + .expect_err("alias in target must fail closed"); + assert_eq!(err.location, "path"); + assert!( + rewrite_target_for_eval("/x?t=github_pat_OPENSHELL%2DRESOLVE%2DENV%2DGITHUB_TOKEN", &resolver) + .is_err() + ); + } + + #[test] + fn canonical_placeholder_still_resolves_after_alias_support() { + let resolver = github_resolver(); + assert_eq!( + rewrite_header_line("Authorization: token openshell:resolve:env:GITHUB_TOKEN", &resolver), + "Authorization: token github_pat_REAL" + ); + } }