From 7a1f8f3a38a282362f55c326411b9b2e3adcc4ad Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Thu, 9 Jul 2026 12:18:49 +0000 Subject: [PATCH 1/7] feat(sandbox,gateway): route sandbox egress through corporate HTTP proxy - Chain sandbox egress through a corporate HTTP proxy so outbound traffic from within the sandbox respects the host proxy settings - Forward sandbox proxy environment variables to the generated Podman config so the proxy is applied consistently to Podman-managed workloads Signed-off-by: Philippe Martin --- architecture/sandbox.md | 10 + crates/openshell-driver-podman/README.md | 8 + crates/openshell-driver-podman/src/config.rs | 94 +++ .../openshell-driver-podman/src/container.rs | 98 +++ crates/openshell-driver-podman/src/driver.rs | 1 + crates/openshell-driver-podman/src/main.rs | 16 + .../openshell-supervisor-network/src/lib.rs | 1 + .../openshell-supervisor-network/src/proxy.rs | 81 ++- .../src/upstream_proxy.rs | 667 ++++++++++++++++++ docs/reference/gateway-config.mdx | 9 + tasks/scripts/gateway.sh | 9 + 11 files changed, 989 insertions(+), 5 deletions(-) create mode 100644 crates/openshell-supervisor-network/src/upstream_proxy.rs diff --git a/architecture/sandbox.md b/architecture/sandbox.md index 30d1bf4783..1a68a9fe4c 100644 --- a/architecture/sandbox.md +++ b/architecture/sandbox.md @@ -78,6 +78,16 @@ handled by the inference interception path: External inference endpoints that do not use `inference.local` are treated like ordinary network traffic and must be allowed by policy. +In proxy-required networks, the supervisor honors `HTTPS_PROXY`/`HTTP_PROXY`/ +`NO_PROXY` from its own environment: after policy and SSRF checks pass, it +chains the upstream dial through the corporate forward proxy with HTTP CONNECT +instead of connecting directly. `NO_PROXY` destinations, loopback, and +host-gateway aliases always dial directly. Only `http://` proxy URLs are +supported. Local DNS resolution and SSRF validation still run before the +proxied dial; the CONNECT target sent to the corporate proxy is the requested +hostname. The workload child's proxy variables are unaffected — they are +always rewritten to point at the local policy proxy. + ## Credentials Provider credentials are stored at the gateway and fetched by the supervisor at diff --git a/crates/openshell-driver-podman/README.md b/crates/openshell-driver-podman/README.md index c6a619f3d1..91141565ed 100644 --- a/crates/openshell-driver-podman/README.md +++ b/crates/openshell-driver-podman/README.md @@ -345,6 +345,14 @@ Podman resources after out-of-band container removal or label drift. | `OPENSHELL_PODMAN_TLS_CA` | `--podman-tls-ca` | unset | Host path to the CA certificate mounted for sandbox mTLS. | | `OPENSHELL_PODMAN_TLS_CERT` | `--podman-tls-cert` | unset | Host path to the client certificate mounted for sandbox mTLS. | | `OPENSHELL_PODMAN_TLS_KEY` | `--podman-tls-key` | unset | Host path to the client private key mounted for sandbox mTLS. | +| `OPENSHELL_SANDBOX_HTTPS_PROXY` | `--sandbox-https-proxy` | unset | Corporate forward proxy URL injected into sandbox containers as `HTTPS_PROXY`. The in-container supervisor chains policy-approved upstream dials through it with HTTP CONNECT. Only `http://` proxy URLs are supported. | +| `OPENSHELL_SANDBOX_HTTP_PROXY` | `--sandbox-http-proxy` | unset | Corporate forward proxy URL injected as `HTTP_PROXY` for plain HTTP requests. | +| `OPENSHELL_SANDBOX_NO_PROXY` | `--sandbox-no-proxy` | unset | Comma-separated `NO_PROXY` list (hostnames, domain suffixes, IPs, CIDRs) dialed directly instead of through the corporate proxy. | + +Through the gateway, the same settings are the `https_proxy`, `http_proxy`, and +`no_proxy` keys under `[openshell.drivers.podman]`; see +`docs/reference/gateway-config.mdx`. Per-sandbox environment values take +precedence over these operator defaults. ## Rootless-Specific Adaptations diff --git a/crates/openshell-driver-podman/src/config.rs b/crates/openshell-driver-podman/src/config.rs index def8e5f3dc..95970b0791 100644 --- a/crates/openshell-driver-podman/src/config.rs +++ b/crates/openshell-driver-podman/src/config.rs @@ -134,6 +134,21 @@ pub struct PodmanComputeConfig { /// Set to `0` to disable health checks entirely. /// Defaults to [`DEFAULT_HEALTH_CHECK_INTERVAL_SECS`] (10 seconds). pub health_check_interval_secs: u64, + /// Corporate forward proxy URL injected into sandbox containers as + /// `HTTPS_PROXY`/`https_proxy` (e.g. `http://proxy.corp.com:8080`). + /// + /// The in-container supervisor chains policy-approved TLS tunnels + /// through this proxy with HTTP CONNECT instead of dialing upstream + /// destinations directly. Only `http://` proxy URLs are supported. + /// Per-sandbox environment values take precedence. + pub https_proxy: Option, + /// Corporate forward proxy URL injected as `HTTP_PROXY`/`http_proxy`, + /// used for plain HTTP requests. See `https_proxy`. + pub http_proxy: Option, + /// Comma-separated `NO_PROXY` list injected alongside the proxy URLs + /// (e.g. `*.svc.cluster.local,10.0.0.0/8`). Destinations matching an + /// entry are dialed directly instead of through the corporate proxy. + pub no_proxy: Option, } pub const DEFAULT_HEALTH_CHECK_INTERVAL_SECS: u64 = 10; @@ -189,6 +204,35 @@ impl PodmanComputeConfig { Ok(()) } + /// Validate optional corporate proxy configuration. + /// + /// The in-container supervisor only supports `http://` forward proxies, + /// so reject other schemes at config time instead of silently ignoring + /// them inside every sandbox. + pub fn validate_proxy_config(&self) -> Result<(), crate::client::PodmanApiError> { + for (field, value) in [ + ("https_proxy", &self.https_proxy), + ("http_proxy", &self.http_proxy), + ] { + let Some(url) = value else { continue }; + let trimmed = url.trim(); + if trimmed.is_empty() { + return Err(crate::client::PodmanApiError::InvalidInput(format!( + "{field} must not be empty when set" + ))); + } + if let Some((scheme, _)) = trimmed.split_once("://") + && !scheme.eq_ignore_ascii_case("http") + { + return Err(crate::client::PodmanApiError::InvalidInput(format!( + "{field} uses unsupported scheme '{scheme}': only http:// forward \ + proxies are supported by the sandbox supervisor" + ))); + } + } + Ok(()) + } + /// Validate optional host gateway override. pub fn validate_host_gateway_ip(&self) -> Result<(), crate::client::PodmanApiError> { let trimmed = self.host_gateway_ip.trim(); @@ -262,6 +306,9 @@ impl Default for PodmanComputeConfig { sandbox_pids_limit: DEFAULT_SANDBOX_PIDS_LIMIT, enable_bind_mounts: false, health_check_interval_secs: DEFAULT_HEALTH_CHECK_INTERVAL_SECS, + https_proxy: None, + http_proxy: None, + no_proxy: None, } } } @@ -288,6 +335,10 @@ impl std::fmt::Debug for PodmanComputeConfig { "health_check_interval_secs", &self.health_check_interval_secs, ) + // Proxy URLs may embed credentials in userinfo; log presence only. + .field("https_proxy", &self.https_proxy.is_some()) + .field("http_proxy", &self.http_proxy.is_some()) + .field("no_proxy", &self.no_proxy) .finish() } } @@ -397,6 +448,49 @@ mod tests { }); } + // ── Proxy config validation ─────────────────────────────────────── + + #[test] + fn validate_proxy_config_accepts_unset_and_http() { + assert!( + PodmanComputeConfig::default() + .validate_proxy_config() + .is_ok() + ); + let cfg = PodmanComputeConfig { + https_proxy: Some("http://proxy.corp.com:8080".to_string()), + http_proxy: Some("proxy.corp.com:3128".to_string()), + no_proxy: Some("*.svc.cluster.local".to_string()), + ..PodmanComputeConfig::default() + }; + assert!(cfg.validate_proxy_config().is_ok()); + } + + #[test] + fn validate_proxy_config_rejects_non_http_schemes() { + for url in ["https://proxy:443", "socks5://proxy:1080"] { + let cfg = PodmanComputeConfig { + https_proxy: Some(url.to_string()), + ..PodmanComputeConfig::default() + }; + let err = cfg.validate_proxy_config().unwrap_err(); + assert!( + err.to_string().contains("unsupported scheme"), + "{url}: {err}" + ); + } + } + + #[test] + fn validate_proxy_config_rejects_empty_value() { + let cfg = PodmanComputeConfig { + http_proxy: Some(" ".to_string()), + ..PodmanComputeConfig::default() + }; + let err = cfg.validate_proxy_config().unwrap_err(); + assert!(err.to_string().contains("http_proxy"), "{err}"); + } + // ── TLS config validation ───────────────────────────────────────── #[test] diff --git a/crates/openshell-driver-podman/src/container.rs b/crates/openshell-driver-podman/src/container.rs index e4a0b79189..18f6080741 100644 --- a/crates/openshell-driver-podman/src/container.rs +++ b/crates/openshell-driver-podman/src/container.rs @@ -385,6 +385,25 @@ fn build_env( env.insert(openshell_core::sandbox_env::USER_ENVIRONMENT.into(), json); } + // 1.5. Operator-configured corporate egress proxy. The in-container + // supervisor reads these from its own environment and chains approved + // upstream dials through the corporate proxy; the workload child's proxy + // variables are rewritten separately to point at the local policy proxy. + // Inserted as defaults so per-sandbox user env wins. Both cases are set + // because proxy-aware tooling disagrees on which one it reads. + for (name, value) in [ + ("HTTPS_PROXY", &config.https_proxy), + ("https_proxy", &config.https_proxy), + ("HTTP_PROXY", &config.http_proxy), + ("http_proxy", &config.http_proxy), + ("NO_PROXY", &config.no_proxy), + ("no_proxy", &config.no_proxy), + ] { + if let Some(value) = value { + env.entry(name.into()).or_insert_with(|| value.clone()); + } + } + // 2. Required driver vars (highest priority -- always overwrite). env.insert( openshell_core::sandbox_env::SANDBOX.into(), @@ -1675,6 +1694,85 @@ mod tests { ); } + #[test] + fn container_spec_injects_operator_proxy_env() { + let sandbox = test_sandbox("test-id", "test-name"); + let mut config = test_config(); + config.https_proxy = Some("http://proxy.corp.com:8080".to_string()); + config.http_proxy = Some("http://proxy.corp.com:3128".to_string()); + config.no_proxy = Some("*.svc.cluster.local,10.0.0.0/8".to_string()); + + let spec = build_container_spec(&sandbox, &config); + let env_map = spec["env"].as_object().expect("env should be an object"); + + for key in ["HTTPS_PROXY", "https_proxy"] { + assert_eq!( + env_map.get(key).and_then(|v| v.as_str()), + Some("http://proxy.corp.com:8080"), + "{key} should carry the operator proxy URL" + ); + } + for key in ["HTTP_PROXY", "http_proxy"] { + assert_eq!( + env_map.get(key).and_then(|v| v.as_str()), + Some("http://proxy.corp.com:3128"), + "{key} should carry the operator proxy URL" + ); + } + for key in ["NO_PROXY", "no_proxy"] { + assert_eq!( + env_map.get(key).and_then(|v| v.as_str()), + Some("*.svc.cluster.local,10.0.0.0/8"), + "{key} should carry the operator NO_PROXY list" + ); + } + } + + #[test] + fn container_spec_omits_proxy_env_when_unconfigured() { + let sandbox = test_sandbox("test-id", "test-name"); + let spec = build_container_spec(&sandbox, &test_config()); + let env_map = spec["env"].as_object().expect("env should be an object"); + + for key in ["HTTPS_PROXY", "HTTP_PROXY", "NO_PROXY"] { + assert!( + !env_map.contains_key(key), + "{key} should be absent without operator proxy config" + ); + } + } + + #[test] + fn container_spec_user_env_overrides_operator_proxy() { + use openshell_core::proto::compute::v1::{DriverSandboxSpec, DriverSandboxTemplate}; + + let mut sandbox = test_sandbox("test-id", "test-name"); + sandbox.spec = Some(DriverSandboxSpec { + environment: std::collections::HashMap::from([( + "HTTPS_PROXY".to_string(), + "http://sandbox-specific:9999".to_string(), + )]), + template: Some(DriverSandboxTemplate::default()), + ..Default::default() + }); + let mut config = test_config(); + config.https_proxy = Some("http://proxy.corp.com:8080".to_string()); + + let spec = build_container_spec(&sandbox, &config); + let env_map = spec["env"].as_object().expect("env should be an object"); + + assert_eq!( + env_map.get("HTTPS_PROXY").and_then(|v| v.as_str()), + Some("http://sandbox-specific:9999"), + "per-sandbox HTTPS_PROXY should win over operator config" + ); + assert_eq!( + env_map.get("https_proxy").and_then(|v| v.as_str()), + Some("http://proxy.corp.com:8080"), + "unset lowercase variant still falls back to operator config" + ); + } + #[test] fn container_spec_required_labels_cannot_be_overridden() { use openshell_core::proto::compute::v1::{DriverSandboxSpec, DriverSandboxTemplate}; diff --git a/crates/openshell-driver-podman/src/driver.rs b/crates/openshell-driver-podman/src/driver.rs index cf639fa265..4207da08bd 100644 --- a/crates/openshell-driver-podman/src/driver.rs +++ b/crates/openshell-driver-podman/src/driver.rs @@ -185,6 +185,7 @@ impl PodmanComputeDriver { config.validate_tls_config()?; config.validate_runtime_limits()?; config.validate_host_gateway_ip()?; + config.validate_proxy_config()?; let client = PodmanClient::new(config.socket_path.clone()); diff --git a/crates/openshell-driver-podman/src/main.rs b/crates/openshell-driver-podman/src/main.rs index c57aff4277..116ad1553c 100644 --- a/crates/openshell-driver-podman/src/main.rs +++ b/crates/openshell-driver-podman/src/main.rs @@ -103,6 +103,19 @@ struct Args { /// Host path to the client private key for sandbox mTLS. #[arg(long, env = "OPENSHELL_PODMAN_TLS_KEY")] podman_tls_key: Option, + + /// Corporate forward proxy URL injected into sandbox containers as + /// `HTTPS_PROXY` for the supervisor's upstream dials (http:// only). + #[arg(long, env = "OPENSHELL_SANDBOX_HTTPS_PROXY")] + sandbox_https_proxy: Option, + + /// Corporate forward proxy URL injected as `HTTP_PROXY` (http:// only). + #[arg(long, env = "OPENSHELL_SANDBOX_HTTP_PROXY")] + sandbox_http_proxy: Option, + + /// Comma-separated `NO_PROXY` list injected alongside the proxy URLs. + #[arg(long, env = "OPENSHELL_SANDBOX_NO_PROXY")] + sandbox_no_proxy: Option, } #[tokio::main] @@ -137,6 +150,9 @@ async fn main() -> Result<()> { guest_tls_cert: args.podman_tls_cert, guest_tls_key: args.podman_tls_key, sandbox_pids_limit: args.sandbox_pids_limit, + https_proxy: args.sandbox_https_proxy, + http_proxy: args.sandbox_http_proxy, + no_proxy: args.sandbox_no_proxy, ..PodmanComputeConfig::default() }) .await diff --git a/crates/openshell-supervisor-network/src/lib.rs b/crates/openshell-supervisor-network/src/lib.rs index c6c1af5aed..ac0cb120a2 100644 --- a/crates/openshell-supervisor-network/src/lib.rs +++ b/crates/openshell-supervisor-network/src/lib.rs @@ -19,3 +19,4 @@ pub mod run; pub mod sigv4; mod spiffe_endpoint; mod token_grant; +pub mod upstream_proxy; diff --git a/crates/openshell-supervisor-network/src/proxy.rs b/crates/openshell-supervisor-network/src/proxy.rs index db6315dd86..ed71c4477c 100644 --- a/crates/openshell-supervisor-network/src/proxy.rs +++ b/crates/openshell-supervisor-network/src/proxy.rs @@ -7,6 +7,7 @@ use crate::identity::BinaryIdentityCache; use crate::l7::tls::ProxyTlsState; use crate::opa::{NetworkAction, OpaEngine, PolicyGenerationGuard}; use crate::policy_local::{POLICY_LOCAL_HOST, PolicyLocalContext}; +use crate::upstream_proxy::{self, UpstreamProxyConfig, UpstreamScheme}; use miette::{IntoDiagnostic, Result}; use openshell_core::activity::{ActivitySender, try_record_activity}; use openshell_core::denial::DenialEvent; @@ -240,6 +241,24 @@ impl ProxyHandle { ); } + // Corporate egress proxy configured on the supervisor's own + // environment (HTTPS_PROXY/HTTP_PROXY/NO_PROXY). Read once at startup; + // the workload cannot influence the supervisor's environment. + let upstream_proxy: Arc> = + Arc::new(UpstreamProxyConfig::from_env()); + if let Some(cfg) = upstream_proxy.as_ref() { + let event = openshell_ocsf::ConfigStateChangeBuilder::new(openshell_ocsf::ctx::ctx()) + .severity(SeverityId::Informational) + .status(StatusId::Success) + .state(openshell_ocsf::StateId::Enabled, "enabled") + .message(format!( + "Upstream corporate proxy enabled: {}", + cfg.summary() + )) + .build(); + ocsf_emit!(event); + } + let join = tokio::spawn(async move { // Wait for the OPA engine's symlink resolution reload to complete // before accepting connections. This prevents requests from @@ -275,6 +294,7 @@ impl ProxyHandle { let inf = inference_ctx.clone(); let policy_local = policy_local_ctx.clone(); let gw = trusted_host_gateway.clone(); + let up_proxy = upstream_proxy.clone(); let resolver = provider_credentials .as_ref() .and_then(ProviderCredentialState::resolver); @@ -296,6 +316,7 @@ impl ProxyHandle { inf, policy_local, gw, + up_proxy, resolver, dynamic_credentials, dtx, @@ -548,6 +569,7 @@ async fn handle_tcp_connection( inference_ctx: Option>, policy_local_ctx: Option>, trusted_host_gateway: Arc>, + upstream_proxy: Arc>, secret_resolver: Option>, dynamic_credentials: Option< Arc< @@ -602,6 +624,7 @@ async fn handle_tcp_connection( entrypoint_pid, policy_local_ctx, trusted_host_gateway, + upstream_proxy, secret_resolver, dynamic_credentials, denial_tx.as_ref(), @@ -1076,9 +1099,15 @@ async fn handle_tcp_connection( return Ok(()); } - let mut upstream = TcpStream::connect(validated_addrs.as_slice()) - .await - .into_diagnostic()?; + let mut upstream = dial_upstream( + &upstream_proxy, + UpstreamScheme::Https, + &host_lc, + port, + &validated_addrs, + ) + .await + .into_diagnostic()?; debug!( "handle_tcp_connection dns_resolve_and_tcp_connect: {}ms host={host_lc}", @@ -2768,6 +2797,33 @@ fn validate_declared_endpoint_resolved_addrs( Ok(()) } +/// Dial a validated upstream destination. +/// +/// Connects directly to the SSRF-checked resolved addresses, or chains +/// through the corporate proxy (HTTP CONNECT) when one is configured for +/// this destination via the supervisor's `HTTPS_PROXY`/`HTTP_PROXY` and not +/// excluded by `NO_PROXY`. Policy evaluation and SSRF validation must have +/// already succeeded; only the final TCP dial changes. +/// +/// The CONNECT target sent to the corporate proxy is the client-requested +/// hostname, so hostname-filtering proxies and split-horizon DNS at the +/// proxy keep working. +async fn dial_upstream( + upstream_proxy: &Option, + scheme: UpstreamScheme, + host_lc: &str, + port: u16, + addrs: &[SocketAddr], +) -> std::io::Result { + if let Some(endpoint) = upstream_proxy + .as_ref() + .and_then(|cfg| cfg.proxy_for(scheme, host_lc)) + { + return upstream_proxy::connect_via(endpoint, host_lc, port).await; + } + TcpStream::connect(addrs).await +} + /// Resolve a host:port using sandbox `/etc/hosts` first (when available), then /// reject if any resolved address is internal. /// @@ -3322,6 +3378,7 @@ async fn handle_forward_proxy( entrypoint_pid: Arc, policy_local_ctx: Option>, trusted_host_gateway: Arc>, + upstream_proxy: Arc>, secret_resolver: Option>, dynamic_credentials: Option< Arc< @@ -4336,8 +4393,21 @@ async fn handle_forward_proxy( return Ok(()); } - // 6. Connect upstream - let mut upstream = match TcpStream::connect(addrs.as_slice()).await { + // 6. Connect upstream. Host-gateway aliases always dial directly — the + // corporate proxy cannot reach the driver-injected host gateway. + let dial_result = if is_host_gateway_alias(&host_lc) { + TcpStream::connect(addrs.as_slice()).await + } else { + dial_upstream( + &upstream_proxy, + UpstreamScheme::Http, + &host_lc, + port, + &addrs, + ) + .await + }; + let mut upstream = match dial_result { Ok(s) => s, Err(e) => { let event = HttpActivityBuilder::new(openshell_ocsf::ctx::ctx()) @@ -8456,6 +8526,7 @@ network_policies: None, // inference_ctx None, // policy_local_ctx Arc::new(None), // trusted_host_gateway + Arc::new(None), // upstream_proxy None, // secret_resolver None, // dynamic_credentials Some(denial_tx), // denial_tx — positive allow/deny signal diff --git a/crates/openshell-supervisor-network/src/upstream_proxy.rs b/crates/openshell-supervisor-network/src/upstream_proxy.rs new file mode 100644 index 0000000000..e6023c7d84 --- /dev/null +++ b/crates/openshell-supervisor-network/src/upstream_proxy.rs @@ -0,0 +1,667 @@ +// SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +//! Upstream corporate proxy chaining for the sandbox egress proxy. +//! +//! In proxy-required enterprise networks (issue #1792) the supervisor cannot +//! dial policy-approved destinations directly: all outbound traffic must go +//! through a corporate forward proxy. This module reads the standard +//! `HTTPS_PROXY` / `HTTP_PROXY` / `ALL_PROXY` / `NO_PROXY` variables from the +//! supervisor's **own** environment (the workload child's proxy variables are +//! rewritten separately to point at the local policy proxy) and chains +//! approved connections through the corporate proxy with HTTP CONNECT. +//! +//! Scope and invariants: +//! - Only `http://` proxy URLs are supported. `https://` and SOCKS proxies +//! are ignored with a warning. +//! - Policy evaluation, DNS resolution, and SSRF checks run exactly as in the +//! direct-dial path; the corporate proxy only replaces the final TCP dial. +//! - `NO_PROXY` decides which destinations bypass the corporate proxy and +//! keep dialing directly (cluster-internal services, host gateway, etc.). +//! Loopback destinations always bypass the proxy. + +use std::io::{Error as IoError, ErrorKind}; +use std::net::IpAddr; +use std::time::Duration; + +use base64::Engine as _; +use tokio::io::{AsyncReadExt, AsyncWriteExt}; +use tokio::net::TcpStream; +use tracing::{debug, warn}; + +/// Upper bound on the corporate proxy's CONNECT response header block. +const MAX_CONNECT_RESPONSE_BYTES: usize = 8 * 1024; + +/// End-to-end budget for dialing the corporate proxy and completing the +/// CONNECT handshake. +const CONNECT_HANDSHAKE_TIMEOUT: Duration = Duration::from_secs(30); + +/// Which proxy variable applies to a destination. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum UpstreamScheme { + /// TLS-bound tunnels (client issued CONNECT): `HTTPS_PROXY`. + Https, + /// Plain HTTP forward-proxy requests: `HTTP_PROXY`. + Http, +} + +/// A parsed corporate proxy endpoint. +#[derive(Clone)] +pub struct ProxyEndpoint { + host: String, + port: u16, + /// Pre-computed `Basic ` header value from URL userinfo. + /// Never logged. + proxy_authorization: Option, +} + +impl ProxyEndpoint { + /// `host:port` label for logs. Excludes credentials. + #[must_use] + pub fn display_addr(&self) -> String { + format!("{}:{}", self.host, self.port) + } +} + +impl std::fmt::Debug for ProxyEndpoint { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("ProxyEndpoint") + .field("host", &self.host) + .field("port", &self.port) + .field("proxy_authorization", &self.proxy_authorization.is_some()) + .finish() + } +} + +/// One parsed `NO_PROXY` entry. +#[derive(Debug, Clone)] +enum NoProxyEntry { + /// `*` — bypass the proxy for every destination. + Wildcard, + /// Domain suffix match: `corp.com` matches `corp.com` and `x.corp.com`. + Domain(String), + /// Exact IP literal match. + Ip(IpAddr), + /// CIDR match against IP-literal destination hosts. + Cidr(ipnet::IpNet), +} + +/// Parsed `NO_PROXY` list. +#[derive(Debug, Clone, Default)] +struct NoProxy { + entries: Vec, +} + +impl NoProxy { + fn parse(raw: &str) -> Self { + let mut entries = Vec::new(); + for item in raw.split(',') { + let item = item.trim(); + if item.is_empty() { + continue; + } + if item == "*" { + entries.push(NoProxyEntry::Wildcard); + continue; + } + if let Ok(net) = item.parse::() { + entries.push(NoProxyEntry::Cidr(net)); + continue; + } + if let Ok(ip) = item.trim_matches(['[', ']']).parse::() { + entries.push(NoProxyEntry::Ip(ip)); + continue; + } + // Domain entry. Strip a `:port` qualifier (ports are ignored), + // then any leading `*.` or `.` so `.corp.com`, `*.corp.com`, and + // `corp.com` all behave identically. + let name = item.rsplit_once(':').map_or(item, |(name, port)| { + if port.chars().all(|c| c.is_ascii_digit()) { + name + } else { + item + } + }); + let name = name + .strip_prefix("*.") + .or_else(|| name.strip_prefix('.')) + .unwrap_or(name) + .to_ascii_lowercase(); + if !name.is_empty() { + entries.push(NoProxyEntry::Domain(name)); + } + } + Self { entries } + } + + /// Whether `host` (lowercase hostname or IP literal) must bypass the + /// corporate proxy. Loopback destinations always match. + fn matches(&self, host: &str) -> bool { + let host_ip = host.trim_matches(['[', ']']).parse::().ok(); + if host == "localhost" || host_ip.is_some_and(|ip| ip.is_loopback()) { + return true; + } + self.entries.iter().any(|entry| match entry { + NoProxyEntry::Wildcard => true, + NoProxyEntry::Domain(suffix) => { + host == suffix + || host + .strip_suffix(suffix) + .is_some_and(|prefix| prefix.ends_with('.')) + } + NoProxyEntry::Ip(ip) => host_ip == Some(*ip), + NoProxyEntry::Cidr(net) => host_ip.is_some_and(|ip| net.contains(&ip)), + }) + } +} + +/// Corporate proxy configuration read from the supervisor's environment. +#[derive(Debug, Clone)] +pub struct UpstreamProxyConfig { + https: Option, + http: Option, + no_proxy: NoProxy, +} + +impl UpstreamProxyConfig { + /// Read `HTTPS_PROXY` / `HTTP_PROXY` / `ALL_PROXY` / `NO_PROXY` (lowercase + /// variants take precedence) from the process environment. Returns `None` + /// when no usable proxy is configured. + #[must_use] + pub fn from_env() -> Option { + Self::from_lookup(|name| std::env::var(name).ok()) + } + + fn from_lookup(lookup: impl Fn(&str) -> Option) -> Option { + let var = |upper: &str, lower: &str| { + lookup(lower) + .or_else(|| lookup(upper)) + .filter(|v| !v.trim().is_empty()) + }; + let all = var("ALL_PROXY", "all_proxy"); + let https = var("HTTPS_PROXY", "https_proxy") + .or_else(|| all.clone()) + .and_then(|url| parse_proxy_url(&url, "HTTPS_PROXY")); + let http = var("HTTP_PROXY", "http_proxy") + .or(all) + .and_then(|url| parse_proxy_url(&url, "HTTP_PROXY")); + if https.is_none() && http.is_none() { + return None; + } + let no_proxy = NoProxy::parse(&var("NO_PROXY", "no_proxy").unwrap_or_default()); + Some(Self { + https, + http, + no_proxy, + }) + } + + /// The corporate proxy to use for `host`, or `None` when the destination + /// must be dialed directly. + #[must_use] + pub fn proxy_for(&self, scheme: UpstreamScheme, host: &str) -> Option<&ProxyEndpoint> { + if self.no_proxy.matches(host) { + return None; + } + match scheme { + UpstreamScheme::Https => self.https.as_ref(), + UpstreamScheme::Http => self.http.as_ref(), + } + } + + /// Credential-free summary for startup logging. + #[must_use] + pub fn summary(&self) -> String { + let fmt = |ep: Option<&ProxyEndpoint>| { + ep.map_or_else(|| "-".to_string(), ProxyEndpoint::display_addr) + }; + format!( + "https_proxy={} http_proxy={} no_proxy_entries={}", + fmt(self.https.as_ref()), + fmt(self.http.as_ref()), + self.no_proxy.entries.len() + ) + } +} + +/// Parse an `http://[user:pass@]host[:port]` proxy URL. Unsupported schemes +/// (TLS or SOCKS proxies) are rejected with a warning. +fn parse_proxy_url(raw: &str, var_name: &str) -> Option { + let raw = raw.trim(); + let rest = match raw.split_once("://") { + Some((scheme, rest)) => { + if !scheme.eq_ignore_ascii_case("http") { + warn!( + "{var_name} uses unsupported proxy scheme '{scheme}' \ + (only http:// proxies are supported); ignoring" + ); + return None; + } + rest + } + None => raw, + }; + // Drop any path component. + let rest = rest.split(['/', '?', '#']).next().unwrap_or(rest); + let (userinfo, authority) = match rest.rsplit_once('@') { + Some((userinfo, authority)) => (Some(userinfo), authority), + None => (None, rest), + }; + let Some((host, port)) = split_host_port(authority) else { + warn!("{var_name} has an invalid proxy address '{authority}'; ignoring"); + return None; + }; + let proxy_authorization = userinfo.map(|userinfo| { + let (user, pass) = userinfo.split_once(':').unwrap_or((userinfo, "")); + let credentials = format!("{}:{}", percent_decode(user), percent_decode(pass)); + format!( + "Basic {}", + base64::engine::general_purpose::STANDARD.encode(credentials) + ) + }); + Some(ProxyEndpoint { + host, + port, + proxy_authorization, + }) +} + +/// Split `host[:port]` (with optional `[v6]` brackets), defaulting to port 80. +fn split_host_port(authority: &str) -> Option<(String, u16)> { + if authority.is_empty() { + return None; + } + if let Some(v6_end) = authority.find(']') { + if !authority.starts_with('[') { + return None; + } + let host = authority[1..v6_end].to_string(); + let port = match authority[v6_end + 1..].strip_prefix(':') { + Some(port) => port.parse().ok()?, + None if authority[v6_end + 1..].is_empty() => 80, + None => return None, + }; + return Some((host, port)); + } + match authority.rsplit_once(':') { + Some((host, port)) if !host.contains(':') => Some((host.to_string(), port.parse().ok()?)), + Some(_) => None, // bare IPv6 without brackets is ambiguous + None => Some((authority.to_string(), 80)), + } +} + +/// Minimal percent-decoder for URL userinfo. +fn percent_decode(input: &str) -> String { + let bytes = input.as_bytes(); + let mut out = Vec::with_capacity(bytes.len()); + let mut i = 0; + while i < bytes.len() { + if bytes[i] == b'%' + && i + 2 < bytes.len() + && let (Some(hi), Some(lo)) = ( + (bytes[i + 1] as char).to_digit(16), + (bytes[i + 2] as char).to_digit(16), + ) + { + #[allow(clippy::cast_possible_truncation)] + out.push((hi * 16 + lo) as u8); + i += 3; + } else { + out.push(bytes[i]); + i += 1; + } + } + String::from_utf8_lossy(&out).into_owned() +} + +/// Open a tunnel to `host:port` through the corporate proxy with HTTP CONNECT. +/// +/// Returns the connected stream once the proxy answers 200; after that the +/// stream is a transparent byte pipe to the destination. +/// +/// The destination hostname (not a locally resolved IP) is sent in the +/// CONNECT target so hostname-filtering proxies keep working; local DNS +/// resolution and SSRF validation must already have happened at the call +/// site. +/// +/// # Errors +/// +/// Returns an error when the proxy is unreachable, the handshake times out, +/// or the proxy answers with a non-200 status. +pub async fn connect_via( + endpoint: &ProxyEndpoint, + host: &str, + port: u16, +) -> std::io::Result { + tokio::time::timeout( + CONNECT_HANDSHAKE_TIMEOUT, + connect_via_inner(endpoint, host, port), + ) + .await + .map_err(|_| { + IoError::new( + ErrorKind::TimedOut, + format!( + "upstream proxy {} CONNECT handshake timed out", + endpoint.display_addr() + ), + ) + })? +} + +async fn connect_via_inner( + endpoint: &ProxyEndpoint, + host: &str, + port: u16, +) -> std::io::Result { + let mut stream = TcpStream::connect((endpoint.host.as_str(), endpoint.port)).await?; + + let target = if host.contains(':') { + format!("[{host}]:{port}") + } else { + format!("{host}:{port}") + }; + let mut request = format!("CONNECT {target} HTTP/1.1\r\nHost: {target}\r\n"); + if let Some(auth) = &endpoint.proxy_authorization { + request.push_str("Proxy-Authorization: "); + request.push_str(auth); + request.push_str("\r\n"); + } + request.push_str("\r\n"); + stream.write_all(request.as_bytes()).await?; + + // Read the proxy's response header block. + let mut buf = vec![0u8; MAX_CONNECT_RESPONSE_BYTES]; + let mut used = 0; + loop { + if used == buf.len() { + return Err(IoError::other(format!( + "upstream proxy {} CONNECT response headers exceed {MAX_CONNECT_RESPONSE_BYTES} bytes", + endpoint.display_addr() + ))); + } + let n = stream.read(&mut buf[used..]).await?; + if n == 0 { + return Err(IoError::new( + ErrorKind::UnexpectedEof, + format!( + "upstream proxy {} closed the connection during CONNECT", + endpoint.display_addr() + ), + )); + } + used += n; + if buf[..used].windows(4).any(|win| win == b"\r\n\r\n") { + break; + } + } + + let response = String::from_utf8_lossy(&buf[..used]); + let status_line = response.lines().next().unwrap_or_default(); + let status_code = status_line + .split_whitespace() + .nth(1) + .and_then(|code| code.parse::().ok()); + match status_code { + Some(200) => { + debug!( + proxy = %endpoint.display_addr(), + target = %target, + "upstream proxy CONNECT tunnel established" + ); + Ok(stream) + } + Some(code) => Err(IoError::other(format!( + "upstream proxy {} refused CONNECT to {target}: HTTP {code}", + endpoint.display_addr() + ))), + None => Err(IoError::new( + ErrorKind::InvalidData, + format!( + "upstream proxy {} sent a malformed CONNECT response", + endpoint.display_addr() + ), + )), + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn config_from(pairs: &[(&str, &str)]) -> Option { + UpstreamProxyConfig::from_lookup(|name| { + pairs + .iter() + .find(|(k, _)| *k == name) + .map(|(_, v)| (*v).to_string()) + }) + } + + #[test] + fn no_env_yields_none() { + assert!(config_from(&[]).is_none()); + } + + #[test] + fn empty_values_yield_none() { + assert!(config_from(&[("HTTPS_PROXY", " "), ("HTTP_PROXY", "")]).is_none()); + } + + #[test] + fn https_proxy_parsed_with_port() { + let cfg = config_from(&[("HTTPS_PROXY", "http://proxy.corp.com:8080")]).unwrap(); + let ep = cfg + .proxy_for(UpstreamScheme::Https, "api.stripe.com") + .unwrap(); + assert_eq!(ep.display_addr(), "proxy.corp.com:8080"); + assert!(ep.proxy_authorization.is_none()); + assert!( + cfg.proxy_for(UpstreamScheme::Http, "api.stripe.com") + .is_none() + ); + } + + #[test] + fn lowercase_takes_precedence() { + let cfg = config_from(&[ + ("HTTPS_PROXY", "http://upper:1111"), + ("https_proxy", "http://lower:2222"), + ]) + .unwrap(); + let ep = cfg.proxy_for(UpstreamScheme::Https, "example.com").unwrap(); + assert_eq!(ep.display_addr(), "lower:2222"); + } + + #[test] + fn all_proxy_fills_both_schemes() { + let cfg = config_from(&[("ALL_PROXY", "http://proxy:3128")]).unwrap(); + assert!( + cfg.proxy_for(UpstreamScheme::Https, "example.com") + .is_some() + ); + assert!(cfg.proxy_for(UpstreamScheme::Http, "example.com").is_some()); + } + + #[test] + fn scheme_defaults_to_http_and_port_defaults_to_80() { + let cfg = config_from(&[("HTTP_PROXY", "proxy.corp.com")]).unwrap(); + let ep = cfg.proxy_for(UpstreamScheme::Http, "example.com").unwrap(); + assert_eq!(ep.display_addr(), "proxy.corp.com:80"); + } + + #[test] + fn tls_and_socks_proxies_rejected() { + assert!(config_from(&[("HTTPS_PROXY", "https://proxy:443")]).is_none()); + assert!(config_from(&[("HTTPS_PROXY", "socks5://proxy:1080")]).is_none()); + } + + #[test] + fn userinfo_becomes_basic_auth() { + let cfg = config_from(&[("HTTPS_PROXY", "http://user:p%40ss@proxy:8080")]).unwrap(); + let ep = cfg.proxy_for(UpstreamScheme::Https, "example.com").unwrap(); + let auth = ep.proxy_authorization.as_deref().unwrap(); + let expected = base64::engine::general_purpose::STANDARD.encode("user:p@ss"); + assert_eq!(auth, format!("Basic {expected}")); + } + + #[test] + fn debug_output_hides_credentials() { + let cfg = config_from(&[("HTTPS_PROXY", "http://user:secret@proxy:8080")]).unwrap(); + let debug = format!("{cfg:?}"); + assert!(!debug.contains("secret")); + assert!(!cfg.summary().contains("secret")); + } + + #[test] + fn ipv6_proxy_address_parses() { + let cfg = config_from(&[("HTTPS_PROXY", "http://[fd00::1]:8080")]).unwrap(); + let ep = cfg.proxy_for(UpstreamScheme::Https, "example.com").unwrap(); + assert_eq!(ep.display_addr(), "fd00::1:8080"); + } + + // -- NO_PROXY matching -- + + fn no_proxy_cfg(no_proxy: &str) -> UpstreamProxyConfig { + config_from(&[("HTTPS_PROXY", "http://proxy:8080"), ("NO_PROXY", no_proxy)]).unwrap() + } + + fn bypasses(cfg: &UpstreamProxyConfig, host: &str) -> bool { + cfg.proxy_for(UpstreamScheme::Https, host).is_none() + } + + #[test] + fn no_proxy_domain_suffix_matches_host_and_subdomains() { + let cfg = no_proxy_cfg("corp.com,other.example"); + assert!(bypasses(&cfg, "corp.com")); + assert!(bypasses(&cfg, "api.corp.com")); + assert!(!bypasses(&cfg, "notcorp.com")); + assert!(!bypasses(&cfg, "corp.com.evil.io")); + } + + #[test] + fn no_proxy_leading_dot_and_wildcard_prefix_are_equivalent() { + for entry in [".svc.cluster.local", "*.svc.cluster.local"] { + let cfg = no_proxy_cfg(entry); + assert!( + bypasses(&cfg, "kubernetes.default.svc.cluster.local"), + "{entry}" + ); + assert!(!bypasses(&cfg, "example.com"), "{entry}"); + } + } + + #[test] + fn no_proxy_cidr_matches_ip_literals() { + let cfg = no_proxy_cfg("10.96.0.0/12"); + assert!(bypasses(&cfg, "10.96.0.1")); + assert!(bypasses(&cfg, "10.100.20.30")); + assert!(!bypasses(&cfg, "10.200.0.9")); + assert!(!bypasses(&cfg, "93.184.216.34")); + } + + #[test] + fn no_proxy_exact_ip_matches() { + let cfg = no_proxy_cfg("192.168.1.5"); + assert!(bypasses(&cfg, "192.168.1.5")); + assert!(!bypasses(&cfg, "192.168.1.6")); + } + + #[test] + fn no_proxy_wildcard_bypasses_everything() { + let cfg = no_proxy_cfg("*"); + assert!(bypasses(&cfg, "example.com")); + } + + #[test] + fn no_proxy_ignores_port_qualifiers() { + let cfg = no_proxy_cfg("internal.corp:8443"); + assert!(bypasses(&cfg, "internal.corp")); + assert!(bypasses(&cfg, "svc.internal.corp")); + } + + #[test] + fn loopback_and_localhost_always_bypass() { + let cfg = no_proxy_cfg(""); + assert!(bypasses(&cfg, "localhost")); + assert!(bypasses(&cfg, "127.0.0.1")); + assert!(bypasses(&cfg, "::1")); + assert!(!bypasses(&cfg, "example.com")); + } + + // -- CONNECT handshake -- + + async fn fake_proxy( + response: &'static str, + ) -> (std::net::SocketAddr, tokio::task::JoinHandle) { + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + let handle = tokio::spawn(async move { + let (mut socket, _) = listener.accept().await.unwrap(); + let mut buf = vec![0u8; 4096]; + let mut used = 0; + loop { + let n = socket.read(&mut buf[used..]).await.unwrap(); + used += n; + if n == 0 || buf[..used].windows(4).any(|w| w == b"\r\n\r\n") { + break; + } + } + socket.write_all(response.as_bytes()).await.unwrap(); + String::from_utf8_lossy(&buf[..used]).into_owned() + }); + (addr, handle) + } + + fn endpoint_for(addr: std::net::SocketAddr, auth: Option<&str>) -> ProxyEndpoint { + ProxyEndpoint { + host: addr.ip().to_string(), + port: addr.port(), + proxy_authorization: auth.map(str::to_string), + } + } + + #[tokio::test] + async fn connect_via_success_sends_connect_and_auth() { + let (addr, handle) = fake_proxy("HTTP/1.1 200 Connection established\r\n\r\n").await; + let endpoint = endpoint_for(addr, Some("Basic dXNlcjpwYXNz")); + let stream = connect_via(&endpoint, "api.example.com", 443) + .await + .unwrap(); + drop(stream); + let request = handle.await.unwrap(); + assert!(request.starts_with("CONNECT api.example.com:443 HTTP/1.1\r\n")); + assert!(request.contains("Host: api.example.com:443\r\n")); + assert!(request.contains("Proxy-Authorization: Basic dXNlcjpwYXNz\r\n")); + } + + #[tokio::test] + async fn connect_via_rejects_non_200() { + let (addr, _handle) = + fake_proxy("HTTP/1.1 407 Proxy Authentication Required\r\n\r\n").await; + let endpoint = endpoint_for(addr, None); + let err = connect_via(&endpoint, "api.example.com", 443) + .await + .unwrap_err(); + assert!(err.to_string().contains("407"), "{err}"); + } + + #[tokio::test] + async fn connect_via_rejects_malformed_response() { + let (addr, _handle) = fake_proxy("garbage without status\r\n\r\n").await; + let endpoint = endpoint_for(addr, None); + let err = connect_via(&endpoint, "api.example.com", 443) + .await + .unwrap_err(); + assert_eq!(err.kind(), ErrorKind::InvalidData); + } + + #[tokio::test] + async fn connect_via_ipv6_target_is_bracketed() { + let (addr, handle) = fake_proxy("HTTP/1.1 200 OK\r\n\r\n").await; + let endpoint = endpoint_for(addr, None); + let _ = connect_via(&endpoint, "2001:db8::1", 443).await.unwrap(); + let request = handle.await.unwrap(); + assert!(request.starts_with("CONNECT [2001:db8::1]:443 HTTP/1.1\r\n")); + } +} diff --git a/docs/reference/gateway-config.mdx b/docs/reference/gateway-config.mdx index 6ed2bae858..90674a3ee4 100644 --- a/docs/reference/gateway-config.mdx +++ b/docs/reference/gateway-config.mdx @@ -354,6 +354,15 @@ sandbox_pids_limit = 2048 # but increase process churn (each check spawns a conmon subprocess). # Set to 0 to disable health checks entirely. Default: 10. health_check_interval_secs = 10 +# Corporate forward proxy for sandbox egress. When set, the in-container +# supervisor chains policy-approved upstream connections through this proxy +# with HTTP CONNECT instead of dialing destinations directly. Only http:// +# proxy URLs are supported. NO_PROXY entries (hostnames, domain suffixes, +# IPs, CIDRs) are dialed directly. Per-sandbox HTTPS_PROXY/HTTP_PROXY/NO_PROXY +# environment values take precedence over these defaults. +# https_proxy = "http://proxy.corp.com:8080" +# http_proxy = "http://proxy.corp.com:8080" +# no_proxy = "*.svc.cluster.local,10.0.0.0/8" ``` ### MicroVM diff --git a/tasks/scripts/gateway.sh b/tasks/scripts/gateway.sh index 11fbf9a059..53edff81d7 100644 --- a/tasks/scripts/gateway.sh +++ b/tasks/scripts/gateway.sh @@ -352,6 +352,15 @@ EOF if [[ -n "${GRPC_ENDPOINT}" ]]; then printf 'grpc_endpoint = "%s"\n' "${GRPC_ENDPOINT}" >>"${CONFIG_PATH}" fi + if [[ -n "${OPENSHELL_SANDBOX_HTTPS_PROXY:-}" ]]; then + printf 'https_proxy = "%s"\n' "${OPENSHELL_SANDBOX_HTTPS_PROXY}" >>"${CONFIG_PATH}" + fi + if [[ -n "${OPENSHELL_SANDBOX_HTTP_PROXY:-}" ]]; then + printf 'http_proxy = "%s"\n' "${OPENSHELL_SANDBOX_HTTP_PROXY}" >>"${CONFIG_PATH}" + fi + if [[ -n "${OPENSHELL_SANDBOX_NO_PROXY:-}" ]]; then + printf 'no_proxy = "%s"\n' "${OPENSHELL_SANDBOX_NO_PROXY}" >>"${CONFIG_PATH}" + fi ;; esac From 42291f2a1b3b1660581dd4fef2f79e3d17e5b7cf Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Thu, 16 Jul 2026 12:32:46 +0200 Subject: [PATCH 2/7] fix(sandbox,podman): make corporate proxy routing operator-owned The operator-configured corporate egress proxy was injected under the conventional HTTPS_PROXY/HTTP_PROXY/NO_PROXY names as defaults beneath sandbox spec/template environment, so a sandbox creator could redirect egress at an arbitrary proxy or disable proxying with NO_PROXY=*. Route the boundary through reserved, supervisor-only variables (OPENSHELL_UPSTREAM_HTTPS_PROXY/HTTP_PROXY/NO_PROXY) written in the Podman driver's required-variable tier. Any sandbox-supplied value under a reserved name is stripped before the operator value is applied, so the supervisor never observes a reserved proxy variable the operator did not set. The supervisor now reads only the reserved names and ignores the conventional proxy variables the sandbox controls. Add the reserved proxy variables to the supervisor-only child-environment denylist so the corporate proxy URL and any embedded credentials are not inherited by the sandbox workload, which reaches egress through the local policy proxy and never needs them. Signed-off-by: Philippe Martin --- architecture/sandbox.md | 23 +-- crates/openshell-core/src/sandbox_env.rs | 24 ++++ crates/openshell-driver-podman/src/config.rs | 21 +-- .../openshell-driver-podman/src/container.rs | 134 ++++++++++++------ .../openshell-supervisor-network/src/proxy.rs | 13 +- .../src/upstream_proxy.rs | 119 +++++++++------- .../src/process.rs | 20 +++ docs/reference/gateway-config.mdx | 5 +- 8 files changed, 235 insertions(+), 124 deletions(-) diff --git a/architecture/sandbox.md b/architecture/sandbox.md index 1a68a9fe4c..edda42e07d 100644 --- a/architecture/sandbox.md +++ b/architecture/sandbox.md @@ -78,15 +78,20 @@ handled by the inference interception path: External inference endpoints that do not use `inference.local` are treated like ordinary network traffic and must be allowed by policy. -In proxy-required networks, the supervisor honors `HTTPS_PROXY`/`HTTP_PROXY`/ -`NO_PROXY` from its own environment: after policy and SSRF checks pass, it -chains the upstream dial through the corporate forward proxy with HTTP CONNECT -instead of connecting directly. `NO_PROXY` destinations, loopback, and -host-gateway aliases always dial directly. Only `http://` proxy URLs are -supported. Local DNS resolution and SSRF validation still run before the -proxied dial; the CONNECT target sent to the corporate proxy is the requested -hostname. The workload child's proxy variables are unaffected — they are -always rewritten to point at the local policy proxy. +In proxy-required networks, the supervisor chains the upstream dial through a +corporate forward proxy with HTTP CONNECT instead of connecting directly, once +policy and SSRF checks pass. The proxy configuration is an operator-owned +boundary read from reserved `OPENSHELL_UPSTREAM_HTTPS_PROXY` / +`OPENSHELL_UPSTREAM_HTTP_PROXY` / `OPENSHELL_UPSTREAM_NO_PROXY` variables that +compute drivers write in their required-variable tier; sandbox and template +environment cannot override them. The conventional `HTTPS_PROXY`/`HTTP_PROXY`/ +`NO_PROXY` variables a sandbox controls are ignored on this path. Reserved +`NO_PROXY` destinations, loopback, and host-gateway aliases always dial +directly. Only `http://` proxy URLs are supported. Local DNS resolution and +SSRF validation still run before the proxied dial; the CONNECT target sent to +the corporate proxy is the requested hostname. The workload child's proxy +variables are unaffected — they are always rewritten to point at the local +policy proxy. ## Credentials diff --git a/crates/openshell-core/src/sandbox_env.rs b/crates/openshell-core/src/sandbox_env.rs index f066580636..e86e50380d 100644 --- a/crates/openshell-core/src/sandbox_env.rs +++ b/crates/openshell-core/src/sandbox_env.rs @@ -114,3 +114,27 @@ pub const SANDBOX_UID: &str = "OPENSHELL_SANDBOX_UID"; /// Used alongside UID for PVC init container `chown` operations and when the /// supervisor drops privileges to a group other than the UID's primary group. pub const SANDBOX_GID: &str = "OPENSHELL_SANDBOX_GID"; + +/// Corporate forward-proxy URL the supervisor chains TLS (CONNECT) egress +/// through, e.g. `http://proxy.corp.com:8080`. +/// +/// This is an operator-owned egress boundary. Compute drivers set it from +/// gateway configuration in the supervisor's required-variable tier so that +/// sandbox spec/template environment cannot override it. The supervisor reads +/// *only* this reserved name — never the conventional `HTTPS_PROXY` / +/// `ALL_PROXY` variables, which the sandbox creator controls and which are +/// reserved for pointing the workload child at the local policy proxy. +pub const UPSTREAM_HTTPS_PROXY: &str = "OPENSHELL_UPSTREAM_HTTPS_PROXY"; + +/// Corporate forward-proxy URL the supervisor uses for plain-HTTP egress. +/// +/// Operator-owned counterpart to [`UPSTREAM_HTTPS_PROXY`]; see that constant +/// for the trust-boundary rationale. +pub const UPSTREAM_HTTP_PROXY: &str = "OPENSHELL_UPSTREAM_HTTP_PROXY"; + +/// Comma-separated `NO_PROXY`-style list of destinations the supervisor dials +/// directly instead of chaining through the corporate proxy. +/// +/// Operator-owned counterpart to [`UPSTREAM_HTTPS_PROXY`]; see that constant +/// for the trust-boundary rationale. +pub const UPSTREAM_NO_PROXY: &str = "OPENSHELL_UPSTREAM_NO_PROXY"; diff --git a/crates/openshell-driver-podman/src/config.rs b/crates/openshell-driver-podman/src/config.rs index 95970b0791..c03dc2707e 100644 --- a/crates/openshell-driver-podman/src/config.rs +++ b/crates/openshell-driver-podman/src/config.rs @@ -134,20 +134,25 @@ pub struct PodmanComputeConfig { /// Set to `0` to disable health checks entirely. /// Defaults to [`DEFAULT_HEALTH_CHECK_INTERVAL_SECS`] (10 seconds). pub health_check_interval_secs: u64, - /// Corporate forward proxy URL injected into sandbox containers as - /// `HTTPS_PROXY`/`https_proxy` (e.g. `http://proxy.corp.com:8080`). + /// Corporate forward proxy URL injected into sandbox containers as the + /// reserved `OPENSHELL_UPSTREAM_HTTPS_PROXY` supervisor variable + /// (e.g. `http://proxy.corp.com:8080`). /// /// The in-container supervisor chains policy-approved TLS tunnels /// through this proxy with HTTP CONNECT instead of dialing upstream /// destinations directly. Only `http://` proxy URLs are supported. - /// Per-sandbox environment values take precedence. + /// This is an operator-owned egress boundary: it is written in the + /// required-variable tier so sandbox/template environment cannot override + /// it, and the conventional `HTTPS_PROXY` variables are not used. pub https_proxy: Option, - /// Corporate forward proxy URL injected as `HTTP_PROXY`/`http_proxy`, - /// used for plain HTTP requests. See `https_proxy`. + /// Corporate forward proxy URL injected as the reserved + /// `OPENSHELL_UPSTREAM_HTTP_PROXY` variable, used for plain HTTP requests. + /// See `https_proxy`. pub http_proxy: Option, - /// Comma-separated `NO_PROXY` list injected alongside the proxy URLs - /// (e.g. `*.svc.cluster.local,10.0.0.0/8`). Destinations matching an - /// entry are dialed directly instead of through the corporate proxy. + /// Comma-separated `NO_PROXY` list injected as the reserved + /// `OPENSHELL_UPSTREAM_NO_PROXY` variable (e.g. + /// `*.svc.cluster.local,10.0.0.0/8`). Destinations matching an entry are + /// dialed directly instead of through the corporate proxy. pub no_proxy: Option, } diff --git a/crates/openshell-driver-podman/src/container.rs b/crates/openshell-driver-podman/src/container.rs index 18f6080741..fb5edb20f6 100644 --- a/crates/openshell-driver-podman/src/container.rs +++ b/crates/openshell-driver-podman/src/container.rs @@ -385,26 +385,40 @@ fn build_env( env.insert(openshell_core::sandbox_env::USER_ENVIRONMENT.into(), json); } - // 1.5. Operator-configured corporate egress proxy. The in-container - // supervisor reads these from its own environment and chains approved - // upstream dials through the corporate proxy; the workload child's proxy - // variables are rewritten separately to point at the local policy proxy. - // Inserted as defaults so per-sandbox user env wins. Both cases are set - // because proxy-aware tooling disagrees on which one it reads. + // 2. Required driver vars (highest priority -- always overwrite). + + // Operator-configured corporate egress proxy. This is a security boundary, + // so it is written in the required-variable tier under reserved + // supervisor-only names. The conventional `HTTPS_PROXY`/`NO_PROXY` variants + // are deliberately NOT used here: those belong to the sandbox creator and + // are separately rewritten to point the workload child at the local policy + // proxy, so letting them steer the supervisor would let a sandbox pick an + // arbitrary proxy or disable proxying with `NO_PROXY=*`. + // + // Any sandbox/template-supplied value under a reserved name is first + // stripped, then replaced with the operator value (if configured), so the + // supervisor can never observe a reserved proxy variable the operator did + // not set. for (name, value) in [ - ("HTTPS_PROXY", &config.https_proxy), - ("https_proxy", &config.https_proxy), - ("HTTP_PROXY", &config.http_proxy), - ("http_proxy", &config.http_proxy), - ("NO_PROXY", &config.no_proxy), - ("no_proxy", &config.no_proxy), + ( + openshell_core::sandbox_env::UPSTREAM_HTTPS_PROXY, + &config.https_proxy, + ), + ( + openshell_core::sandbox_env::UPSTREAM_HTTP_PROXY, + &config.http_proxy, + ), + ( + openshell_core::sandbox_env::UPSTREAM_NO_PROXY, + &config.no_proxy, + ), ] { + env.remove(name); if let Some(value) = value { - env.entry(name.into()).or_insert_with(|| value.clone()); + env.insert(name.into(), value.clone()); } } - // 2. Required driver vars (highest priority -- always overwrite). env.insert( openshell_core::sandbox_env::SANDBOX.into(), sandbox.name.clone(), @@ -1696,6 +1710,10 @@ mod tests { #[test] fn container_spec_injects_operator_proxy_env() { + use openshell_core::sandbox_env::{ + UPSTREAM_HTTP_PROXY, UPSTREAM_HTTPS_PROXY, UPSTREAM_NO_PROXY, + }; + let sandbox = test_sandbox("test-id", "test-name"); let mut config = test_config(); config.https_proxy = Some("http://proxy.corp.com:8080".to_string()); @@ -1705,36 +1723,43 @@ mod tests { let spec = build_container_spec(&sandbox, &config); let env_map = spec["env"].as_object().expect("env should be an object"); - for key in ["HTTPS_PROXY", "https_proxy"] { - assert_eq!( - env_map.get(key).and_then(|v| v.as_str()), - Some("http://proxy.corp.com:8080"), - "{key} should carry the operator proxy URL" - ); - } - for key in ["HTTP_PROXY", "http_proxy"] { - assert_eq!( - env_map.get(key).and_then(|v| v.as_str()), - Some("http://proxy.corp.com:3128"), - "{key} should carry the operator proxy URL" - ); - } - for key in ["NO_PROXY", "no_proxy"] { - assert_eq!( - env_map.get(key).and_then(|v| v.as_str()), - Some("*.svc.cluster.local,10.0.0.0/8"), - "{key} should carry the operator NO_PROXY list" + assert_eq!( + env_map.get(UPSTREAM_HTTPS_PROXY).and_then(|v| v.as_str()), + Some("http://proxy.corp.com:8080"), + "reserved upstream HTTPS var should carry the operator proxy URL" + ); + assert_eq!( + env_map.get(UPSTREAM_HTTP_PROXY).and_then(|v| v.as_str()), + Some("http://proxy.corp.com:3128"), + "reserved upstream HTTP var should carry the operator proxy URL" + ); + assert_eq!( + env_map.get(UPSTREAM_NO_PROXY).and_then(|v| v.as_str()), + Some("*.svc.cluster.local,10.0.0.0/8"), + "reserved upstream NO_PROXY var should carry the operator list" + ); + + // The conventional proxy variables must NOT be set from operator + // config: they belong to the sandbox creator, not the supervisor. + for key in ["HTTPS_PROXY", "https_proxy", "HTTP_PROXY", "http_proxy"] { + assert!( + !env_map.contains_key(key), + "{key} must not be populated from operator proxy config" ); } } #[test] fn container_spec_omits_proxy_env_when_unconfigured() { + use openshell_core::sandbox_env::{ + UPSTREAM_HTTP_PROXY, UPSTREAM_HTTPS_PROXY, UPSTREAM_NO_PROXY, + }; + let sandbox = test_sandbox("test-id", "test-name"); let spec = build_container_spec(&sandbox, &test_config()); let env_map = spec["env"].as_object().expect("env should be an object"); - for key in ["HTTPS_PROXY", "HTTP_PROXY", "NO_PROXY"] { + for key in [UPSTREAM_HTTPS_PROXY, UPSTREAM_HTTP_PROXY, UPSTREAM_NO_PROXY] { assert!( !env_map.contains_key(key), "{key} should be absent without operator proxy config" @@ -1743,16 +1768,32 @@ mod tests { } #[test] - fn container_spec_user_env_overrides_operator_proxy() { + fn container_spec_user_env_cannot_override_operator_proxy() { use openshell_core::proto::compute::v1::{DriverSandboxSpec, DriverSandboxTemplate}; + use openshell_core::sandbox_env::{UPSTREAM_HTTPS_PROXY, UPSTREAM_NO_PROXY}; + // A sandbox creator tries to redirect egress at an attacker proxy and + // disable proxying with `NO_PROXY=*` through both spec and template env. let mut sandbox = test_sandbox("test-id", "test-name"); sandbox.spec = Some(DriverSandboxSpec { - environment: std::collections::HashMap::from([( - "HTTPS_PROXY".to_string(), - "http://sandbox-specific:9999".to_string(), - )]), - template: Some(DriverSandboxTemplate::default()), + environment: std::collections::HashMap::from([ + ( + "HTTPS_PROXY".to_string(), + "http://attacker:9999".to_string(), + ), + ( + UPSTREAM_HTTPS_PROXY.to_string(), + "http://attacker:9999".to_string(), + ), + (UPSTREAM_NO_PROXY.to_string(), "*".to_string()), + ]), + template: Some(DriverSandboxTemplate { + environment: std::collections::HashMap::from([( + UPSTREAM_NO_PROXY.to_string(), + "*".to_string(), + )]), + ..Default::default() + }), ..Default::default() }); let mut config = test_config(); @@ -1762,14 +1803,13 @@ mod tests { let env_map = spec["env"].as_object().expect("env should be an object"); assert_eq!( - env_map.get("HTTPS_PROXY").and_then(|v| v.as_str()), - Some("http://sandbox-specific:9999"), - "per-sandbox HTTPS_PROXY should win over operator config" - ); - assert_eq!( - env_map.get("https_proxy").and_then(|v| v.as_str()), + env_map.get(UPSTREAM_HTTPS_PROXY).and_then(|v| v.as_str()), Some("http://proxy.corp.com:8080"), - "unset lowercase variant still falls back to operator config" + "operator proxy must win over any sandbox-supplied reserved value" + ); + assert!( + !env_map.contains_key(UPSTREAM_NO_PROXY), + "sandbox must not be able to inject a reserved NO_PROXY bypass" ); } diff --git a/crates/openshell-supervisor-network/src/proxy.rs b/crates/openshell-supervisor-network/src/proxy.rs index ed71c4477c..7f28774c18 100644 --- a/crates/openshell-supervisor-network/src/proxy.rs +++ b/crates/openshell-supervisor-network/src/proxy.rs @@ -242,8 +242,10 @@ impl ProxyHandle { } // Corporate egress proxy configured on the supervisor's own - // environment (HTTPS_PROXY/HTTP_PROXY/NO_PROXY). Read once at startup; - // the workload cannot influence the supervisor's environment. + // environment via the operator-owned reserved OPENSHELL_UPSTREAM_* + // variables. Read once at startup; the workload cannot influence the + // supervisor's environment, and the conventional HTTPS_PROXY/NO_PROXY + // variables it does control are ignored on this path. let upstream_proxy: Arc> = Arc::new(UpstreamProxyConfig::from_env()); if let Some(cfg) = upstream_proxy.as_ref() { @@ -2801,9 +2803,10 @@ fn validate_declared_endpoint_resolved_addrs( /// /// Connects directly to the SSRF-checked resolved addresses, or chains /// through the corporate proxy (HTTP CONNECT) when one is configured for -/// this destination via the supervisor's `HTTPS_PROXY`/`HTTP_PROXY` and not -/// excluded by `NO_PROXY`. Policy evaluation and SSRF validation must have -/// already succeeded; only the final TCP dial changes. +/// this destination via the supervisor's reserved upstream proxy variables +/// and not excluded by the reserved `NO_PROXY` list. Policy evaluation and +/// SSRF validation must have already succeeded; only the final TCP dial +/// changes. /// /// The CONNECT target sent to the corporate proxy is the client-requested /// hostname, so hostname-filtering proxies and split-horizon DNS at the diff --git a/crates/openshell-supervisor-network/src/upstream_proxy.rs b/crates/openshell-supervisor-network/src/upstream_proxy.rs index e6023c7d84..1f0ffb50d8 100644 --- a/crates/openshell-supervisor-network/src/upstream_proxy.rs +++ b/crates/openshell-supervisor-network/src/upstream_proxy.rs @@ -5,20 +5,28 @@ //! //! In proxy-required enterprise networks (issue #1792) the supervisor cannot //! dial policy-approved destinations directly: all outbound traffic must go -//! through a corporate forward proxy. This module reads the standard -//! `HTTPS_PROXY` / `HTTP_PROXY` / `ALL_PROXY` / `NO_PROXY` variables from the -//! supervisor's **own** environment (the workload child's proxy variables are -//! rewritten separately to point at the local policy proxy) and chains -//! approved connections through the corporate proxy with HTTP CONNECT. +//! through a corporate forward proxy. This module reads the operator-owned +//! reserved `OPENSHELL_UPSTREAM_HTTPS_PROXY` / `OPENSHELL_UPSTREAM_HTTP_PROXY` +//! / `OPENSHELL_UPSTREAM_NO_PROXY` variables from the supervisor's **own** +//! environment and chains approved connections through the corporate proxy +//! with HTTP CONNECT. +//! +//! The conventional `HTTPS_PROXY` / `HTTP_PROXY` / `ALL_PROXY` / `NO_PROXY` +//! variables are intentionally ignored: those are controlled by the sandbox +//! creator and are rewritten separately to point the workload child at the +//! local policy proxy, so honoring them would let a sandbox pick an arbitrary +//! upstream proxy or disable proxying with `NO_PROXY=*`. The compute driver +//! writes the reserved names in its required-variable tier, which sandbox and +//! template environment cannot override. //! //! Scope and invariants: //! - Only `http://` proxy URLs are supported. `https://` and SOCKS proxies //! are ignored with a warning. //! - Policy evaluation, DNS resolution, and SSRF checks run exactly as in the //! direct-dial path; the corporate proxy only replaces the final TCP dial. -//! - `NO_PROXY` decides which destinations bypass the corporate proxy and -//! keep dialing directly (cluster-internal services, host gateway, etc.). -//! Loopback destinations always bypass the proxy. +//! - The reserved `NO_PROXY` list decides which destinations bypass the +//! corporate proxy and keep dialing directly (cluster-internal services, +//! host gateway, etc.). Loopback destinations always bypass the proxy. use std::io::{Error as IoError, ErrorKind}; use std::net::IpAddr; @@ -164,31 +172,38 @@ pub struct UpstreamProxyConfig { } impl UpstreamProxyConfig { - /// Read `HTTPS_PROXY` / `HTTP_PROXY` / `ALL_PROXY` / `NO_PROXY` (lowercase - /// variants take precedence) from the process environment. Returns `None` - /// when no usable proxy is configured. + /// Read the operator-owned corporate proxy configuration from the + /// supervisor's reserved environment variables + /// ([`UPSTREAM_HTTPS_PROXY`](openshell_core::sandbox_env::UPSTREAM_HTTPS_PROXY), + /// [`UPSTREAM_HTTP_PROXY`](openshell_core::sandbox_env::UPSTREAM_HTTP_PROXY), + /// [`UPSTREAM_NO_PROXY`](openshell_core::sandbox_env::UPSTREAM_NO_PROXY)). + /// Returns `None` when no usable proxy is configured. + /// + /// The conventional `HTTPS_PROXY` / `HTTP_PROXY` / `ALL_PROXY` / `NO_PROXY` + /// variables are intentionally ignored here: they are set by the sandbox + /// creator (and rewritten to point workload children at the local policy + /// proxy), so honoring them would let a sandbox choose an arbitrary + /// upstream proxy or disable proxying entirely. The compute driver writes + /// the reserved names in its required-variable tier, where sandbox and + /// template environment cannot override them. #[must_use] pub fn from_env() -> Option { Self::from_lookup(|name| std::env::var(name).ok()) } fn from_lookup(lookup: impl Fn(&str) -> Option) -> Option { - let var = |upper: &str, lower: &str| { - lookup(lower) - .or_else(|| lookup(upper)) - .filter(|v| !v.trim().is_empty()) + use openshell_core::sandbox_env::{ + UPSTREAM_HTTP_PROXY, UPSTREAM_HTTPS_PROXY, UPSTREAM_NO_PROXY, }; - let all = var("ALL_PROXY", "all_proxy"); - let https = var("HTTPS_PROXY", "https_proxy") - .or_else(|| all.clone()) - .and_then(|url| parse_proxy_url(&url, "HTTPS_PROXY")); - let http = var("HTTP_PROXY", "http_proxy") - .or(all) - .and_then(|url| parse_proxy_url(&url, "HTTP_PROXY")); + let var = |name: &str| lookup(name).filter(|v| !v.trim().is_empty()); + let https = + var(UPSTREAM_HTTPS_PROXY).and_then(|url| parse_proxy_url(&url, UPSTREAM_HTTPS_PROXY)); + let http = + var(UPSTREAM_HTTP_PROXY).and_then(|url| parse_proxy_url(&url, UPSTREAM_HTTP_PROXY)); if https.is_none() && http.is_none() { return None; } - let no_proxy = NoProxy::parse(&var("NO_PROXY", "no_proxy").unwrap_or_default()); + let no_proxy = NoProxy::parse(&var(UPSTREAM_NO_PROXY).unwrap_or_default()); Some(Self { https, http, @@ -428,6 +443,10 @@ async fn connect_via_inner( #[cfg(test)] mod tests { use super::*; + use openshell_core::sandbox_env::{ + UPSTREAM_HTTP_PROXY as HTTP_PROXY, UPSTREAM_HTTPS_PROXY as HTTPS_PROXY, + UPSTREAM_NO_PROXY as NO_PROXY, + }; fn config_from(pairs: &[(&str, &str)]) -> Option { UpstreamProxyConfig::from_lookup(|name| { @@ -443,14 +462,29 @@ mod tests { assert!(config_from(&[]).is_none()); } + #[test] + fn conventional_proxy_vars_are_ignored() { + // The sandbox creator controls these names; they must not steer the + // supervisor's operator-owned egress boundary. + assert!( + config_from(&[ + ("HTTPS_PROXY", "http://attacker:9999"), + ("HTTP_PROXY", "http://attacker:9999"), + ("ALL_PROXY", "http://attacker:9999"), + ("https_proxy", "http://attacker:9999"), + ]) + .is_none() + ); + } + #[test] fn empty_values_yield_none() { - assert!(config_from(&[("HTTPS_PROXY", " "), ("HTTP_PROXY", "")]).is_none()); + assert!(config_from(&[(HTTPS_PROXY, " "), (HTTP_PROXY, "")]).is_none()); } #[test] fn https_proxy_parsed_with_port() { - let cfg = config_from(&[("HTTPS_PROXY", "http://proxy.corp.com:8080")]).unwrap(); + let cfg = config_from(&[(HTTPS_PROXY, "http://proxy.corp.com:8080")]).unwrap(); let ep = cfg .proxy_for(UpstreamScheme::Https, "api.stripe.com") .unwrap(); @@ -462,43 +496,22 @@ mod tests { ); } - #[test] - fn lowercase_takes_precedence() { - let cfg = config_from(&[ - ("HTTPS_PROXY", "http://upper:1111"), - ("https_proxy", "http://lower:2222"), - ]) - .unwrap(); - let ep = cfg.proxy_for(UpstreamScheme::Https, "example.com").unwrap(); - assert_eq!(ep.display_addr(), "lower:2222"); - } - - #[test] - fn all_proxy_fills_both_schemes() { - let cfg = config_from(&[("ALL_PROXY", "http://proxy:3128")]).unwrap(); - assert!( - cfg.proxy_for(UpstreamScheme::Https, "example.com") - .is_some() - ); - assert!(cfg.proxy_for(UpstreamScheme::Http, "example.com").is_some()); - } - #[test] fn scheme_defaults_to_http_and_port_defaults_to_80() { - let cfg = config_from(&[("HTTP_PROXY", "proxy.corp.com")]).unwrap(); + let cfg = config_from(&[(HTTP_PROXY, "proxy.corp.com")]).unwrap(); let ep = cfg.proxy_for(UpstreamScheme::Http, "example.com").unwrap(); assert_eq!(ep.display_addr(), "proxy.corp.com:80"); } #[test] fn tls_and_socks_proxies_rejected() { - assert!(config_from(&[("HTTPS_PROXY", "https://proxy:443")]).is_none()); - assert!(config_from(&[("HTTPS_PROXY", "socks5://proxy:1080")]).is_none()); + assert!(config_from(&[(HTTPS_PROXY, "https://proxy:443")]).is_none()); + assert!(config_from(&[(HTTPS_PROXY, "socks5://proxy:1080")]).is_none()); } #[test] fn userinfo_becomes_basic_auth() { - let cfg = config_from(&[("HTTPS_PROXY", "http://user:p%40ss@proxy:8080")]).unwrap(); + let cfg = config_from(&[(HTTPS_PROXY, "http://user:p%40ss@proxy:8080")]).unwrap(); let ep = cfg.proxy_for(UpstreamScheme::Https, "example.com").unwrap(); let auth = ep.proxy_authorization.as_deref().unwrap(); let expected = base64::engine::general_purpose::STANDARD.encode("user:p@ss"); @@ -507,7 +520,7 @@ mod tests { #[test] fn debug_output_hides_credentials() { - let cfg = config_from(&[("HTTPS_PROXY", "http://user:secret@proxy:8080")]).unwrap(); + let cfg = config_from(&[(HTTPS_PROXY, "http://user:secret@proxy:8080")]).unwrap(); let debug = format!("{cfg:?}"); assert!(!debug.contains("secret")); assert!(!cfg.summary().contains("secret")); @@ -515,7 +528,7 @@ mod tests { #[test] fn ipv6_proxy_address_parses() { - let cfg = config_from(&[("HTTPS_PROXY", "http://[fd00::1]:8080")]).unwrap(); + let cfg = config_from(&[(HTTPS_PROXY, "http://[fd00::1]:8080")]).unwrap(); let ep = cfg.proxy_for(UpstreamScheme::Https, "example.com").unwrap(); assert_eq!(ep.display_addr(), "fd00::1:8080"); } @@ -523,7 +536,7 @@ mod tests { // -- NO_PROXY matching -- fn no_proxy_cfg(no_proxy: &str) -> UpstreamProxyConfig { - config_from(&[("HTTPS_PROXY", "http://proxy:8080"), ("NO_PROXY", no_proxy)]).unwrap() + config_from(&[(HTTPS_PROXY, "http://proxy:8080"), (NO_PROXY, no_proxy)]).unwrap() } fn bypasses(cfg: &UpstreamProxyConfig, host: &str) -> bool { diff --git a/crates/openshell-supervisor-process/src/process.rs b/crates/openshell-supervisor-process/src/process.rs index 72effa98f8..9aa1a8c620 100644 --- a/crates/openshell-supervisor-process/src/process.rs +++ b/crates/openshell-supervisor-process/src/process.rs @@ -78,6 +78,13 @@ const SUPERVISOR_ONLY_ENV_VARS: &[&str] = &[ openshell_core::sandbox_env::TLS_CERT, openshell_core::sandbox_env::TLS_KEY, openshell_core::sandbox_env::PROVIDER_SPIFFE_WORKLOAD_API_SOCKET, + // Corporate proxy routing is a supervisor-only egress boundary and the URLs + // may embed proxy credentials (`http://user:pass@proxy`). The workload + // reaches egress through the local policy proxy, never the corporate proxy + // directly, so these must not be inherited by sandbox child processes. + openshell_core::sandbox_env::UPSTREAM_HTTPS_PROXY, + openshell_core::sandbox_env::UPSTREAM_HTTP_PROXY, + openshell_core::sandbox_env::UPSTREAM_NO_PROXY, ]; pub fn is_supervisor_only_env_var(key: &str) -> bool { @@ -2255,6 +2262,19 @@ mod tests { ); } assert!(stdout.contains("OPENSHELL_ENDPOINT=https://gateway.example.test")); + + // The reserved corporate-proxy variables can carry proxy credentials + // and must be treated as supervisor-only so they are stripped above. + for key in [ + openshell_core::sandbox_env::UPSTREAM_HTTPS_PROXY, + openshell_core::sandbox_env::UPSTREAM_HTTP_PROXY, + openshell_core::sandbox_env::UPSTREAM_NO_PROXY, + ] { + assert!( + is_supervisor_only_env_var(key), + "{key} must be supervisor-only so it is not leaked to the workload" + ); + } } #[test] diff --git a/docs/reference/gateway-config.mdx b/docs/reference/gateway-config.mdx index 90674a3ee4..7ee2545199 100644 --- a/docs/reference/gateway-config.mdx +++ b/docs/reference/gateway-config.mdx @@ -358,8 +358,9 @@ health_check_interval_secs = 10 # supervisor chains policy-approved upstream connections through this proxy # with HTTP CONNECT instead of dialing destinations directly. Only http:// # proxy URLs are supported. NO_PROXY entries (hostnames, domain suffixes, -# IPs, CIDRs) are dialed directly. Per-sandbox HTTPS_PROXY/HTTP_PROXY/NO_PROXY -# environment values take precedence over these defaults. +# IPs, CIDRs) are dialed directly. This is an operator-owned egress boundary: +# sandbox and template environment cannot override it, and the conventional +# HTTPS_PROXY/HTTP_PROXY/NO_PROXY variables a sandbox sets do not affect it. # https_proxy = "http://proxy.corp.com:8080" # http_proxy = "http://proxy.corp.com:8080" # no_proxy = "*.svc.cluster.local,10.0.0.0/8" From d74c780b4388877d50b20689f0ea6c1f1c1c48ae Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Thu, 16 Jul 2026 14:02:53 +0200 Subject: [PATCH 3/7] feat(sandbox,podman): deliver corporate proxy credentials via secret file Proxy credentials were embedded inline in the proxy URL, so they were stored in gateway.toml and exposed in container metadata via 'podman inspect'. Reject inline 'user:pass@' credentials in https_proxy/http_proxy at startup (parsed with the url crate rather than hand-splitting), and add a proxy_auth_file option pointing at a 'user:pass' file. The driver stages that file as a per-sandbox root-only Podman secret, mounts it at a fixed path, and exports only the path in the reserved OPENSHELL_UPSTREAM_PROXY_AUTH_FILE variable, so the credential never appears in config, environment, or container metadata. Reading the file fails closed on a missing, empty, or control-character-bearing value. The supervisor reads the credential from the mounted file and builds the Proxy-Authorization: Basic header, rejecting control characters, and no longer derives credentials from URL userinfo. The auth-file path is added to the child-environment strip list, and the generated gateway.toml is written owner-only (mode 0600). Signed-off-by: Philippe Martin --- Cargo.lock | 1 + architecture/sandbox.md | 9 ++ crates/openshell-core/src/driver_utils.rs | 7 + crates/openshell-core/src/sandbox_env.rs | 9 ++ crates/openshell-driver-podman/Cargo.toml | 1 + crates/openshell-driver-podman/README.md | 21 ++- crates/openshell-driver-podman/src/config.rs | 113 ++++++++++++- .../openshell-driver-podman/src/container.rs | 108 +++++++++++-- crates/openshell-driver-podman/src/driver.rs | 103 +++++++++--- crates/openshell-driver-podman/src/main.rs | 15 +- .../src/upstream_proxy.rs | 153 +++++++++++++----- .../src/process.rs | 2 + docs/reference/gateway-config.mdx | 13 +- tasks/scripts/gateway.sh | 6 + 14 files changed, 468 insertions(+), 93 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d061c74496..289ec359cb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3717,6 +3717,7 @@ dependencies = [ "tonic", "tracing", "tracing-subscriber", + "url", ] [[package]] diff --git a/architecture/sandbox.md b/architecture/sandbox.md index edda42e07d..f17015a090 100644 --- a/architecture/sandbox.md +++ b/architecture/sandbox.md @@ -93,6 +93,15 @@ the corporate proxy is the requested hostname. The workload child's proxy variables are unaffected — they are always rewritten to point at the local policy proxy. +Proxy credentials are never embedded in the URL: an inline `user:pass@` is +rejected at startup because it would be stored in `gateway.toml` and exposed in +container metadata. Operators supply credentials via `proxy_auth_file`; the +driver stages them as a root-only secret mounted at a fixed path and exports +only that path in `OPENSHELL_UPSTREAM_PROXY_AUTH_FILE`. The supervisor reads the +file and builds the `Proxy-Authorization: Basic` header, rejecting credentials +containing control characters. The reserved proxy variables — including the +auth-file path — are stripped from workload child processes. + ## Credentials Provider credentials are stored at the gateway and fetched by the supervisor at diff --git a/crates/openshell-core/src/driver_utils.rs b/crates/openshell-core/src/driver_utils.rs index 9e4411b2a1..f87a23d8ae 100644 --- a/crates/openshell-core/src/driver_utils.rs +++ b/crates/openshell-core/src/driver_utils.rs @@ -72,6 +72,13 @@ pub const TLS_KEY_MOUNT_PATH: &str = "/etc/openshell/tls/client/tls.key"; /// Container-side mount path for the per-sandbox JWT token. pub const SANDBOX_TOKEN_MOUNT_PATH: &str = "/etc/openshell/auth/sandbox.jwt"; +/// Container-side mount path for the corporate upstream-proxy credentials. +/// +/// The file holds the `user:pass` userinfo used to build the +/// `Proxy-Authorization` header. It is delivered through a root-only secret +/// mount so the credential never appears in container environment/metadata. +pub const UPSTREAM_PROXY_AUTH_MOUNT_PATH: &str = "/etc/openshell/auth/upstream-proxy"; + /// Return the XDG state path for a driver's sandbox JWT token file. /// /// The resulting path is `$XDG_STATE_HOME/openshell/[/]//sandbox.jwt`. diff --git a/crates/openshell-core/src/sandbox_env.rs b/crates/openshell-core/src/sandbox_env.rs index e86e50380d..4b696b05fe 100644 --- a/crates/openshell-core/src/sandbox_env.rs +++ b/crates/openshell-core/src/sandbox_env.rs @@ -138,3 +138,12 @@ pub const UPSTREAM_HTTP_PROXY: &str = "OPENSHELL_UPSTREAM_HTTP_PROXY"; /// Operator-owned counterpart to [`UPSTREAM_HTTPS_PROXY`]; see that constant /// for the trust-boundary rationale. pub const UPSTREAM_NO_PROXY: &str = "OPENSHELL_UPSTREAM_NO_PROXY"; + +/// Filesystem path (inside the sandbox) to the corporate proxy credentials. +/// +/// The file holds `user:pass` userinfo the supervisor turns into a +/// `Proxy-Authorization: Basic` header. Credentials are delivered through this +/// root-only file rather than embedded in the proxy URL, so they never appear +/// in container environment or metadata. Compute drivers write this in the +/// required-variable tier; the value is a path, not a secret. +pub const UPSTREAM_PROXY_AUTH_FILE: &str = "OPENSHELL_UPSTREAM_PROXY_AUTH_FILE"; diff --git a/crates/openshell-driver-podman/Cargo.toml b/crates/openshell-driver-podman/Cargo.toml index ed798c0ab2..5f108fd9cc 100644 --- a/crates/openshell-driver-podman/Cargo.toml +++ b/crates/openshell-driver-podman/Cargo.toml @@ -34,6 +34,7 @@ tracing = { workspace = true } tracing-subscriber = { workspace = true } thiserror = { workspace = true } miette = { workspace = true } +url = { workspace = true } [dev-dependencies] prost-types = { workspace = true } diff --git a/crates/openshell-driver-podman/README.md b/crates/openshell-driver-podman/README.md index 91141565ed..ebc79e1ecd 100644 --- a/crates/openshell-driver-podman/README.md +++ b/crates/openshell-driver-podman/README.md @@ -345,14 +345,21 @@ Podman resources after out-of-band container removal or label drift. | `OPENSHELL_PODMAN_TLS_CA` | `--podman-tls-ca` | unset | Host path to the CA certificate mounted for sandbox mTLS. | | `OPENSHELL_PODMAN_TLS_CERT` | `--podman-tls-cert` | unset | Host path to the client certificate mounted for sandbox mTLS. | | `OPENSHELL_PODMAN_TLS_KEY` | `--podman-tls-key` | unset | Host path to the client private key mounted for sandbox mTLS. | -| `OPENSHELL_SANDBOX_HTTPS_PROXY` | `--sandbox-https-proxy` | unset | Corporate forward proxy URL injected into sandbox containers as `HTTPS_PROXY`. The in-container supervisor chains policy-approved upstream dials through it with HTTP CONNECT. Only `http://` proxy URLs are supported. | -| `OPENSHELL_SANDBOX_HTTP_PROXY` | `--sandbox-http-proxy` | unset | Corporate forward proxy URL injected as `HTTP_PROXY` for plain HTTP requests. | +| `OPENSHELL_SANDBOX_HTTPS_PROXY` | `--sandbox-https-proxy` | unset | Corporate forward proxy URL for the supervisor's upstream TLS dials, chained with HTTP CONNECT. Only credential-free `http://` URLs are supported. | +| `OPENSHELL_SANDBOX_HTTP_PROXY` | `--sandbox-http-proxy` | unset | Corporate forward proxy URL for plain HTTP requests. | | `OPENSHELL_SANDBOX_NO_PROXY` | `--sandbox-no-proxy` | unset | Comma-separated `NO_PROXY` list (hostnames, domain suffixes, IPs, CIDRs) dialed directly instead of through the corporate proxy. | - -Through the gateway, the same settings are the `https_proxy`, `http_proxy`, and -`no_proxy` keys under `[openshell.drivers.podman]`; see -`docs/reference/gateway-config.mdx`. Per-sandbox environment values take -precedence over these operator defaults. +| `OPENSHELL_SANDBOX_PROXY_AUTH_FILE` | `--sandbox-proxy-auth-file` | unset | Path to a file containing the proxy credentials as `user:pass`. Staged as a root-only Podman secret so credentials never appear in config or container metadata. | + +Through the gateway, the same settings are the `https_proxy`, `http_proxy`, +`no_proxy`, and `proxy_auth_file` keys under `[openshell.drivers.podman]`; see +`docs/reference/gateway-config.mdx`. + +This is an operator-owned egress boundary: the supervisor reads it from reserved +`OPENSHELL_UPSTREAM_*` variables written at highest priority, so sandbox and +template environment cannot override it, and the conventional +`HTTPS_PROXY`/`HTTP_PROXY`/`NO_PROXY` variables a sandbox controls do not steer +it. Credentials must be supplied through `proxy_auth_file`; an inline +`user:pass@` in the URL is rejected at startup. ## Rootless-Specific Adaptations diff --git a/crates/openshell-driver-podman/src/config.rs b/crates/openshell-driver-podman/src/config.rs index c03dc2707e..2be2a79afd 100644 --- a/crates/openshell-driver-podman/src/config.rs +++ b/crates/openshell-driver-podman/src/config.rs @@ -6,6 +6,19 @@ use std::net::IpAddr; use std::path::PathBuf; use std::str::FromStr; +/// Parse a proxy URL, defaulting a missing scheme to `http://`. +/// +/// A bare `host:port` (no `://`) is normalized to `http://host:port` so it +/// parses the same way the in-sandbox supervisor treats a scheme-less proxy +/// value. Returns the parsed [`url::Url`] for scheme/userinfo/host inspection. +fn parse_proxy_url(raw: &str) -> Result { + if raw.contains("://") { + url::Url::parse(raw) + } else { + url::Url::parse(&format!("http://{raw}")) + } +} + /// Default Podman bridge network name. pub const DEFAULT_NETWORK_NAME: &str = "openshell"; pub const MACOS_PODMAN_MACHINE_HOST_GATEWAY_IP: &str = "192.168.127.254"; @@ -154,6 +167,15 @@ pub struct PodmanComputeConfig { /// `*.svc.cluster.local,10.0.0.0/8`). Destinations matching an entry are /// dialed directly instead of through the corporate proxy. pub no_proxy: Option, + /// Path (on the gateway host) to a file containing the corporate proxy + /// credentials as `user:pass`. + /// + /// Credentials must be supplied through this file, never embedded in the + /// proxy URL: an inline `user:pass@` in `https_proxy`/`http_proxy` is + /// rejected at startup because it would leak into `gateway.toml` and + /// container metadata. The gateway reads this file at sandbox-create time + /// and delivers it to the supervisor through a root-only secret mount. + pub proxy_auth_file: Option, } pub const DEFAULT_HEALTH_CHECK_INTERVAL_SECS: u64 = 10; @@ -211,9 +233,12 @@ impl PodmanComputeConfig { /// Validate optional corporate proxy configuration. /// - /// The in-container supervisor only supports `http://` forward proxies, - /// so reject other schemes at config time instead of silently ignoring - /// them inside every sandbox. + /// The in-container supervisor only supports `http://` forward proxies, so + /// reject other schemes at config time instead of silently ignoring them + /// inside every sandbox. Credentials must be supplied through + /// `proxy_auth_file`; an inline `user:pass@` in the URL is rejected because + /// it would otherwise be stored in `gateway.toml` and exposed in container + /// metadata. pub fn validate_proxy_config(&self) -> Result<(), crate::client::PodmanApiError> { for (field, value) in [ ("https_proxy", &self.https_proxy), @@ -226,15 +251,46 @@ impl PodmanComputeConfig { "{field} must not be empty when set" ))); } - if let Some((scheme, _)) = trimmed.split_once("://") - && !scheme.eq_ignore_ascii_case("http") - { + + let parsed = parse_proxy_url(trimmed).map_err(|e| { + crate::client::PodmanApiError::InvalidInput(format!( + "{field} is not a valid proxy URL: {e}" + )) + })?; + + if !parsed.scheme().eq_ignore_ascii_case("http") { + return Err(crate::client::PodmanApiError::InvalidInput(format!( + "{field} uses unsupported scheme '{}': only http:// forward \ + proxies are supported by the sandbox supervisor", + parsed.scheme() + ))); + } + if !parsed.username().is_empty() || parsed.password().is_some() { + return Err(crate::client::PodmanApiError::InvalidInput(format!( + "{field} must not embed credentials in the URL; supply them via \ + proxy_auth_file so they are not stored in config or container metadata" + ))); + } + if parsed.host_str().is_none_or(str::is_empty) { return Err(crate::client::PodmanApiError::InvalidInput(format!( - "{field} uses unsupported scheme '{scheme}': only http:// forward \ - proxies are supported by the sandbox supervisor" + "{field} is missing a proxy host" ))); } } + + if let Some(path) = self.proxy_auth_file.as_deref() { + if path.trim().is_empty() { + return Err(crate::client::PodmanApiError::InvalidInput( + "proxy_auth_file must not be empty when set".to_string(), + )); + } + if self.https_proxy.is_none() && self.http_proxy.is_none() { + return Err(crate::client::PodmanApiError::InvalidInput( + "proxy_auth_file is set but no https_proxy/http_proxy is configured" + .to_string(), + )); + } + } Ok(()) } @@ -314,6 +370,7 @@ impl Default for PodmanComputeConfig { https_proxy: None, http_proxy: None, no_proxy: None, + proxy_auth_file: None, } } } @@ -344,6 +401,7 @@ impl std::fmt::Debug for PodmanComputeConfig { .field("https_proxy", &self.https_proxy.is_some()) .field("http_proxy", &self.http_proxy.is_some()) .field("no_proxy", &self.no_proxy) + .field("proxy_auth_file", &self.proxy_auth_file.is_some()) .finish() } } @@ -496,6 +554,45 @@ mod tests { assert!(err.to_string().contains("http_proxy"), "{err}"); } + #[test] + fn validate_proxy_config_rejects_inline_credentials() { + for url in [ + "http://user:pass@proxy.corp.com:8080", + "http://user@proxy.corp.com:8080", + "user:pass@proxy.corp.com:8080", + ] { + let cfg = PodmanComputeConfig { + https_proxy: Some(url.to_string()), + ..PodmanComputeConfig::default() + }; + let err = cfg.validate_proxy_config().unwrap_err(); + assert!( + err.to_string().contains("proxy_auth_file"), + "{url} should be rejected and point at proxy_auth_file: {err}" + ); + } + } + + #[test] + fn validate_proxy_config_accepts_auth_file_with_proxy() { + let cfg = PodmanComputeConfig { + https_proxy: Some("http://proxy.corp.com:8080".to_string()), + proxy_auth_file: Some("/etc/openshell/secrets/proxy-auth".to_string()), + ..PodmanComputeConfig::default() + }; + assert!(cfg.validate_proxy_config().is_ok()); + } + + #[test] + fn validate_proxy_config_rejects_auth_file_without_proxy() { + let cfg = PodmanComputeConfig { + proxy_auth_file: Some("/etc/openshell/secrets/proxy-auth".to_string()), + ..PodmanComputeConfig::default() + }; + let err = cfg.validate_proxy_config().unwrap_err(); + assert!(err.to_string().contains("proxy_auth_file"), "{err}"); + } + // ── TLS config validation ───────────────────────────────────────── #[test] diff --git a/crates/openshell-driver-podman/src/container.rs b/crates/openshell-driver-podman/src/container.rs index fb5edb20f6..c793c4f4f8 100644 --- a/crates/openshell-driver-podman/src/container.rs +++ b/crates/openshell-driver-podman/src/container.rs @@ -55,12 +55,15 @@ const VOLUME_PREFIX: &str = "openshell-sandbox-"; /// Secret name prefix for per-sandbox gateway JWTs. const TOKEN_SECRET_PREFIX: &str = "openshell-token-"; +const PROXY_AUTH_SECRET_PREFIX: &str = "openshell-proxy-auth-"; /// Container-side mount paths for client TLS materials and the sandbox token. const TLS_CA_MOUNT_PATH: &str = openshell_core::driver_utils::TLS_CA_MOUNT_PATH; const TLS_CERT_MOUNT_PATH: &str = openshell_core::driver_utils::TLS_CERT_MOUNT_PATH; const TLS_KEY_MOUNT_PATH: &str = openshell_core::driver_utils::TLS_KEY_MOUNT_PATH; const SANDBOX_TOKEN_MOUNT_PATH: &str = openshell_core::driver_utils::SANDBOX_TOKEN_MOUNT_PATH; +const UPSTREAM_PROXY_AUTH_MOUNT_PATH: &str = + openshell_core::driver_utils::UPSTREAM_PROXY_AUTH_MOUNT_PATH; /// Directory inside sandbox containers where the supervisor binary is mounted. const SUPERVISOR_MOUNT_DIR: &str = openshell_core::driver_utils::SUPERVISOR_CONTAINER_DIR; @@ -162,6 +165,12 @@ pub fn token_secret_name(sandbox_id: &str) -> String { format!("{TOKEN_SECRET_PREFIX}{sandbox_id}") } +/// Build the per-sandbox Podman secret name for the corporate proxy credentials. +#[must_use] +pub fn proxy_auth_secret_name(sandbox_id: &str) -> String { + format!("{PROXY_AUTH_SECRET_PREFIX}{sandbox_id}") +} + /// Truncate a container ID to 12 characters (standard short form). #[must_use] pub fn short_id(id: &str) -> String { @@ -399,6 +408,14 @@ fn build_env( // stripped, then replaced with the operator value (if configured), so the // supervisor can never observe a reserved proxy variable the operator did // not set. + // + // Proxy credentials are never passed through the environment: when + // `proxy_auth_file` is configured the supervisor reads them from a + // root-only secret mount, and only the mount *path* is exported. + let proxy_auth_mount = config + .proxy_auth_file + .as_ref() + .map(|_| UPSTREAM_PROXY_AUTH_MOUNT_PATH.to_string()); for (name, value) in [ ( openshell_core::sandbox_env::UPSTREAM_HTTPS_PROXY, @@ -412,6 +429,10 @@ fn build_env( openshell_core::sandbox_env::UPSTREAM_NO_PROXY, &config.no_proxy, ), + ( + openshell_core::sandbox_env::UPSTREAM_PROXY_AUTH_FILE, + &proxy_auth_mount, + ), ] { env.remove(name); if let Some(value) = value { @@ -1026,15 +1047,32 @@ pub fn build_container_spec_with_token_and_gpu_devices( }, resource_limits, secret_env: BTreeMap::new(), - secrets: token_secret_name.map_or_else(Vec::new, |source| { - vec![SecretMount { - source: source.to_string(), - target: SANDBOX_TOKEN_MOUNT_PATH.into(), - uid: 0, - gid: 0, - mode: 0o400, - }] - }), + secrets: { + let mut secrets = Vec::new(); + if let Some(source) = token_secret_name { + secrets.push(SecretMount { + source: source.to_string(), + target: SANDBOX_TOKEN_MOUNT_PATH.into(), + uid: 0, + gid: 0, + mode: 0o400, + }); + } + // Corporate proxy credentials, when configured, are mounted as a + // root-only secret. The driver creates a matching Podman secret + // (see `create_sandbox_proxy_auth_secret`) named deterministically + // from the sandbox id, so no name needs threading through here. + if config.proxy_auth_file.is_some() { + secrets.push(SecretMount { + source: proxy_auth_secret_name(&sandbox.id), + target: UPSTREAM_PROXY_AUTH_MOUNT_PATH.into(), + uid: 0, + gid: 0, + mode: 0o400, + }); + } + secrets + }, stop_timeout: config.stop_timeout_secs, // Inject stable host aliases into /etc/hosts so sandbox containers can // reach services on the host. `host.openshell.internal` is the driver- @@ -2547,6 +2585,58 @@ mod tests { ); } + #[test] + fn container_spec_proxy_auth_file_mounts_secret_and_sets_path_only() { + let sandbox = test_sandbox("proxy-id", "proxy-name"); + let mut config = test_config(); + config.https_proxy = Some("http://proxy.corp.com:8080".to_string()); + config.proxy_auth_file = Some("/etc/openshell/secrets/proxy-auth".to_string()); + + let spec = build_container_spec(&sandbox, &config); + let env_map = spec["env"].as_object().expect("env should be an object"); + + // The supervisor gets only the mount *path*, never the credential. + assert_eq!( + env_map + .get(openshell_core::sandbox_env::UPSTREAM_PROXY_AUTH_FILE) + .and_then(|v| v.as_str()), + Some(UPSTREAM_PROXY_AUTH_MOUNT_PATH) + ); + // The URL carried in the environment must remain credential-free. + assert_eq!( + env_map + .get(openshell_core::sandbox_env::UPSTREAM_HTTPS_PROXY) + .and_then(|v| v.as_str()), + Some("http://proxy.corp.com:8080") + ); + + let secrets = spec["secrets"] + .as_array() + .expect("secrets should be an array"); + assert!( + secrets.iter().any(|secret| { + secret["source"].as_str() == Some(proxy_auth_secret_name(&sandbox.id).as_str()) + && secret["target"].as_str() == Some(UPSTREAM_PROXY_AUTH_MOUNT_PATH) + && secret["mode"].as_u64() == Some(0o400) + }), + "proxy credentials must be delivered through a root-only secret mount" + ); + } + + #[test] + fn container_spec_omits_proxy_auth_mount_when_unconfigured() { + let sandbox = test_sandbox("proxy-id", "proxy-name"); + let mut config = test_config(); + config.https_proxy = Some("http://proxy.corp.com:8080".to_string()); + + let spec = build_container_spec(&sandbox, &config); + let env_map = spec["env"].as_object().expect("env should be an object"); + assert!( + !env_map.contains_key(openshell_core::sandbox_env::UPSTREAM_PROXY_AUTH_FILE), + "auth-file path must be absent when no proxy_auth_file is configured" + ); + } + #[test] fn container_spec_omits_tls_without_config() { let sandbox = test_sandbox("notls-id", "notls-name"); diff --git a/crates/openshell-driver-podman/src/driver.rs b/crates/openshell-driver-podman/src/driver.rs index 4207da08bd..f927f00754 100644 --- a/crates/openshell-driver-podman/src/driver.rs +++ b/crates/openshell-driver-podman/src/driver.rs @@ -114,6 +114,56 @@ async fn cleanup_sandbox_token_secret(client: &PodmanClient, secret_name: &str) } } +/// Read the operator's proxy credentials file and stage it as a per-sandbox +/// Podman secret, so the credentials reach the supervisor through a root-only +/// mount rather than the container environment. +/// +/// Fails closed: when `proxy_auth_file` is configured but cannot be read or is +/// empty/malformed, the sandbox is not created. +async fn create_sandbox_proxy_auth_secret( + client: &PodmanClient, + config: &PodmanComputeConfig, + sandbox: &DriverSandbox, +) -> Result, ComputeDriverError> { + let Some(path) = config.proxy_auth_file.as_deref() else { + return Ok(None); + }; + + let raw = tokio::fs::read_to_string(path).await.map_err(|e| { + ComputeDriverError::Message(format!("failed to read proxy_auth_file '{path}': {e}")) + })?; + let credential = raw.trim(); + if credential.is_empty() { + return Err(ComputeDriverError::InvalidArgument(format!( + "proxy_auth_file '{path}' is empty" + ))); + } + // A credential containing CR/LF/NUL would allow HTTP header injection once + // the supervisor places it in the Proxy-Authorization header. + if credential.contains(['\r', '\n', '\0']) { + return Err(ComputeDriverError::InvalidArgument(format!( + "proxy_auth_file '{path}' must not contain control characters" + ))); + } + + let secret_name = container::proxy_auth_secret_name(&sandbox.id); + client + .create_secret(&secret_name, format!("{credential}\n").as_bytes()) + .await + .map_err(ComputeDriverError::from)?; + Ok(Some(secret_name)) +} + +async fn cleanup_sandbox_proxy_auth_secret(client: &PodmanClient, secret_name: &str) { + if let Err(err) = client.remove_secret(secret_name).await { + warn!( + secret = %secret_name, + error = %err, + "Failed to remove Podman sandbox proxy-auth secret" + ); + } +} + fn local_podman_cdi_gpu_inventory_from(dev_root: &Path) -> CdiGpuInventory { let mut device_ids = std::fs::read_dir(dev_root) .ok() @@ -517,6 +567,29 @@ impl PodmanComputeDriver { return Err(e); } }; + let proxy_auth_secret_name = + match create_sandbox_proxy_auth_secret(&self.client, &self.config, sandbox).await { + Ok(name) => name, + Err(e) => { + let _ = self.client.remove_volume(&vol_name).await; + if let Some(secret) = token_secret_name.as_deref() { + cleanup_sandbox_token_secret(&self.client, secret).await; + } + return Err(e); + } + }; + + // Clean up the volume and both per-sandbox secrets on any failure past + // this point. + let cleanup_created = || async { + let _ = self.client.remove_volume(&vol_name).await; + if let Some(secret) = token_secret_name.as_deref() { + cleanup_sandbox_token_secret(&self.client, secret).await; + } + if let Some(secret) = proxy_auth_secret_name.as_deref() { + cleanup_sandbox_proxy_auth_secret(&self.client, secret).await; + } + }; // 3. Create container. let gpu_devices = match self.resolve_gpu_cdi_devices( @@ -526,10 +599,7 @@ impl PodmanComputeDriver { ) { Ok(devices) => devices, Err(e) => { - let _ = self.client.remove_volume(&vol_name).await; - if let Some(secret) = token_secret_name.as_deref() { - cleanup_sandbox_token_secret(&self.client, secret).await; - } + cleanup_created().await; return Err(e); } }; @@ -541,10 +611,7 @@ impl PodmanComputeDriver { ) { Ok(spec) => spec, Err(e) => { - let _ = self.client.remove_volume(&vol_name).await; - if let Some(secret) = token_secret_name.as_deref() { - cleanup_sandbox_token_secret(&self.client, secret).await; - } + cleanup_created().await; return Err(e); } }; @@ -555,17 +622,11 @@ impl PodmanComputeDriver { // sandbox's ID, not the conflicting container's ID (which // has the same name but a different ID), so it would be // orphaned otherwise. - let _ = self.client.remove_volume(&vol_name).await; - if let Some(secret) = token_secret_name.as_deref() { - cleanup_sandbox_token_secret(&self.client, secret).await; - } + cleanup_created().await; return Err(ComputeDriverError::AlreadyExists); } Err(e) => { - let _ = self.client.remove_volume(&vol_name).await; - if let Some(secret) = token_secret_name.as_deref() { - cleanup_sandbox_token_secret(&self.client, secret).await; - } + cleanup_created().await; return Err(ComputeDriverError::from(e)); } } @@ -578,10 +639,7 @@ impl PodmanComputeDriver { "Failed to start container; cleaning up" ); let _ = self.client.remove_container(&name).await; - let _ = self.client.remove_volume(&vol_name).await; - if let Some(secret) = token_secret_name.as_deref() { - cleanup_sandbox_token_secret(&self.client, secret).await; - } + cleanup_created().await; return Err(ComputeDriverError::from(e)); } @@ -680,6 +738,11 @@ impl PodmanComputeDriver { ); } cleanup_sandbox_token_secret(&self.client, &container::token_secret_name(sandbox_id)).await; + cleanup_sandbox_proxy_auth_secret( + &self.client, + &container::proxy_auth_secret_name(sandbox_id), + ) + .await; Ok(container_existed) } diff --git a/crates/openshell-driver-podman/src/main.rs b/crates/openshell-driver-podman/src/main.rs index 116ad1553c..e7d8f374d4 100644 --- a/crates/openshell-driver-podman/src/main.rs +++ b/crates/openshell-driver-podman/src/main.rs @@ -104,18 +104,26 @@ struct Args { #[arg(long, env = "OPENSHELL_PODMAN_TLS_KEY")] podman_tls_key: Option, - /// Corporate forward proxy URL injected into sandbox containers as - /// `HTTPS_PROXY` for the supervisor's upstream dials (http:// only). + /// Corporate forward proxy URL for the supervisor's upstream TLS dials + /// (http:// only). Credentials must not be embedded in the URL; use + /// `--sandbox-proxy-auth-file` instead. #[arg(long, env = "OPENSHELL_SANDBOX_HTTPS_PROXY")] sandbox_https_proxy: Option, - /// Corporate forward proxy URL injected as `HTTP_PROXY` (http:// only). + /// Corporate forward proxy URL for plain HTTP (http:// only). Credentials + /// must not be embedded in the URL; use `--sandbox-proxy-auth-file`. #[arg(long, env = "OPENSHELL_SANDBOX_HTTP_PROXY")] sandbox_http_proxy: Option, /// Comma-separated `NO_PROXY` list injected alongside the proxy URLs. #[arg(long, env = "OPENSHELL_SANDBOX_NO_PROXY")] sandbox_no_proxy: Option, + + /// Path to a file containing the corporate proxy credentials as + /// `user:pass`. Delivered to the supervisor through a root-only secret + /// mount so the credentials never appear in config or container metadata. + #[arg(long, env = "OPENSHELL_SANDBOX_PROXY_AUTH_FILE")] + sandbox_proxy_auth_file: Option, } #[tokio::main] @@ -153,6 +161,7 @@ async fn main() -> Result<()> { https_proxy: args.sandbox_https_proxy, http_proxy: args.sandbox_http_proxy, no_proxy: args.sandbox_no_proxy, + proxy_auth_file: args.sandbox_proxy_auth_file, ..PodmanComputeConfig::default() }) .await diff --git a/crates/openshell-supervisor-network/src/upstream_proxy.rs b/crates/openshell-supervisor-network/src/upstream_proxy.rs index 1f0ffb50d8..aa841f5f3c 100644 --- a/crates/openshell-supervisor-network/src/upstream_proxy.rs +++ b/crates/openshell-supervisor-network/src/upstream_proxy.rs @@ -58,7 +58,7 @@ pub enum UpstreamScheme { pub struct ProxyEndpoint { host: String, port: u16, - /// Pre-computed `Basic ` header value from URL userinfo. + /// Pre-computed `Basic ` header value from the proxy auth file. /// Never logged. proxy_authorization: Option, } @@ -188,7 +188,26 @@ impl UpstreamProxyConfig { /// template environment cannot override them. #[must_use] pub fn from_env() -> Option { - Self::from_lookup(|name| std::env::var(name).ok()) + let mut config = Self::from_lookup(|name| std::env::var(name).ok())?; + + // Load proxy credentials from the reserved auth file, if configured, + // and apply them to every endpoint. The file is delivered through a + // root-only secret mount so the credentials never appear in the + // environment or container metadata. + if let Some(path) = std::env::var(openshell_core::sandbox_env::UPSTREAM_PROXY_AUTH_FILE) + .ok() + .filter(|p| !p.trim().is_empty()) + { + match std::fs::read_to_string(&path) { + Ok(credential) => config.apply_proxy_auth(basic_auth_header(&credential)), + Err(err) => warn!( + "failed to read upstream proxy auth file '{path}': {err}; \ + proceeding without proxy credentials" + ), + } + } + + Some(config) } fn from_lookup(lookup: impl Fn(&str) -> Option) -> Option { @@ -211,6 +230,19 @@ impl UpstreamProxyConfig { }) } + /// Attach a pre-built `Proxy-Authorization` header value to every + /// configured endpoint. A `None` value clears any existing credentials. + fn apply_proxy_auth(&mut self, proxy_authorization: Option) { + for endpoint in [self.https.as_mut(), self.http.as_mut()] + .into_iter() + .flatten() + { + endpoint + .proxy_authorization + .clone_from(&proxy_authorization); + } + } + /// The corporate proxy to use for `host`, or `None` when the destination /// must be dialed directly. #[must_use] @@ -239,8 +271,13 @@ impl UpstreamProxyConfig { } } -/// Parse an `http://[user:pass@]host[:port]` proxy URL. Unsupported schemes -/// (TLS or SOCKS proxies) are rejected with a warning. +/// Parse an `http://host[:port]` proxy URL. Unsupported schemes (TLS or SOCKS +/// proxies) are rejected with a warning. +/// +/// Credentials are never taken from the URL: they are delivered out of band +/// through [`UPSTREAM_PROXY_AUTH_FILE`](openshell_core::sandbox_env::UPSTREAM_PROXY_AUTH_FILE) +/// so they never appear in config or container metadata. Any `user:pass@` +/// userinfo present in the URL is ignored with a warning. fn parse_proxy_url(raw: &str, var_name: &str) -> Option { let raw = raw.trim(); let rest = match raw.split_once("://") { @@ -258,26 +295,24 @@ fn parse_proxy_url(raw: &str, var_name: &str) -> Option { }; // Drop any path component. let rest = rest.split(['/', '?', '#']).next().unwrap_or(rest); - let (userinfo, authority) = match rest.rsplit_once('@') { - Some((userinfo, authority)) => (Some(userinfo), authority), - None => (None, rest), + let authority = match rest.rsplit_once('@') { + Some((_userinfo, authority)) => { + warn!( + "{var_name} contains inline credentials, which are ignored; \ + supply proxy credentials via the proxy auth file" + ); + authority + } + None => rest, }; let Some((host, port)) = split_host_port(authority) else { warn!("{var_name} has an invalid proxy address '{authority}'; ignoring"); return None; }; - let proxy_authorization = userinfo.map(|userinfo| { - let (user, pass) = userinfo.split_once(':').unwrap_or((userinfo, "")); - let credentials = format!("{}:{}", percent_decode(user), percent_decode(pass)); - format!( - "Basic {}", - base64::engine::general_purpose::STANDARD.encode(credentials) - ) - }); Some(ProxyEndpoint { host, port, - proxy_authorization, + proxy_authorization: None, }) } @@ -305,28 +340,22 @@ fn split_host_port(authority: &str) -> Option<(String, u16)> { } } -/// Minimal percent-decoder for URL userinfo. -fn percent_decode(input: &str) -> String { - let bytes = input.as_bytes(); - let mut out = Vec::with_capacity(bytes.len()); - let mut i = 0; - while i < bytes.len() { - if bytes[i] == b'%' - && i + 2 < bytes.len() - && let (Some(hi), Some(lo)) = ( - (bytes[i + 1] as char).to_digit(16), - (bytes[i + 2] as char).to_digit(16), - ) - { - #[allow(clippy::cast_possible_truncation)] - out.push((hi * 16 + lo) as u8); - i += 3; - } else { - out.push(bytes[i]); - i += 1; - } +/// Build a `Proxy-Authorization: Basic ` header value from a raw +/// `user:pass` credential. +/// +/// Returns `None` for an empty credential or one containing control characters +/// (CR, LF, NUL) that could inject additional HTTP headers. The credential is +/// used verbatim: it is delivered through a trusted operator file, not a URL, +/// so there is no percent-encoding to decode. +fn basic_auth_header(credential: &str) -> Option { + let credential = credential.trim(); + if credential.is_empty() || credential.contains(|c: char| c.is_control()) { + return None; } - String::from_utf8_lossy(&out).into_owned() + Some(format!( + "Basic {}", + base64::engine::general_purpose::STANDARD.encode(credential) + )) } /// Open a tunnel to `host:port` through the corporate proxy with HTTP CONNECT. @@ -510,17 +539,55 @@ mod tests { } #[test] - fn userinfo_becomes_basic_auth() { - let cfg = config_from(&[(HTTPS_PROXY, "http://user:p%40ss@proxy:8080")]).unwrap(); + fn url_userinfo_is_ignored_not_used_as_credentials() { + // Inline credentials in the URL must never become the proxy auth; + // credentials come only from the auth file. + let cfg = config_from(&[(HTTPS_PROXY, "http://user:secret@proxy:8080")]).unwrap(); let ep = cfg.proxy_for(UpstreamScheme::Https, "example.com").unwrap(); - let auth = ep.proxy_authorization.as_deref().unwrap(); - let expected = base64::engine::general_purpose::STANDARD.encode("user:p@ss"); - assert_eq!(auth, format!("Basic {expected}")); + assert_eq!(ep.display_addr(), "proxy:8080"); + assert!( + ep.proxy_authorization.is_none(), + "URL userinfo must not be used as proxy credentials" + ); + } + + #[test] + fn basic_auth_header_encodes_and_rejects_control_chars() { + assert_eq!( + basic_auth_header("user:p@ss").as_deref(), + Some( + format!( + "Basic {}", + base64::engine::general_purpose::STANDARD.encode("user:p@ss") + ) + .as_str() + ) + ); + assert!(basic_auth_header(" ").is_none()); + assert!(basic_auth_header("user:pa\r\nss").is_none()); + assert!(basic_auth_header("user:pa\nInjected: header").is_none()); + } + + #[test] + fn apply_proxy_auth_sets_header_on_all_endpoints() { + let mut cfg = config_from(&[ + (HTTPS_PROXY, "http://proxy:8080"), + (HTTP_PROXY, "http://proxy:3128"), + ]) + .unwrap(); + cfg.apply_proxy_auth(basic_auth_header("user:secret")); + for scheme in [UpstreamScheme::Https, UpstreamScheme::Http] { + let ep = cfg.proxy_for(scheme, "example.com").unwrap(); + let auth = ep.proxy_authorization.as_deref().unwrap(); + let expected = base64::engine::general_purpose::STANDARD.encode("user:secret"); + assert_eq!(auth, format!("Basic {expected}")); + } } #[test] fn debug_output_hides_credentials() { - let cfg = config_from(&[(HTTPS_PROXY, "http://user:secret@proxy:8080")]).unwrap(); + let mut cfg = config_from(&[(HTTPS_PROXY, "http://proxy:8080")]).unwrap(); + cfg.apply_proxy_auth(basic_auth_header("user:secret")); let debug = format!("{cfg:?}"); assert!(!debug.contains("secret")); assert!(!cfg.summary().contains("secret")); diff --git a/crates/openshell-supervisor-process/src/process.rs b/crates/openshell-supervisor-process/src/process.rs index 9aa1a8c620..b5f08f3f1f 100644 --- a/crates/openshell-supervisor-process/src/process.rs +++ b/crates/openshell-supervisor-process/src/process.rs @@ -85,6 +85,7 @@ const SUPERVISOR_ONLY_ENV_VARS: &[&str] = &[ openshell_core::sandbox_env::UPSTREAM_HTTPS_PROXY, openshell_core::sandbox_env::UPSTREAM_HTTP_PROXY, openshell_core::sandbox_env::UPSTREAM_NO_PROXY, + openshell_core::sandbox_env::UPSTREAM_PROXY_AUTH_FILE, ]; pub fn is_supervisor_only_env_var(key: &str) -> bool { @@ -2269,6 +2270,7 @@ mod tests { openshell_core::sandbox_env::UPSTREAM_HTTPS_PROXY, openshell_core::sandbox_env::UPSTREAM_HTTP_PROXY, openshell_core::sandbox_env::UPSTREAM_NO_PROXY, + openshell_core::sandbox_env::UPSTREAM_PROXY_AUTH_FILE, ] { assert!( is_supervisor_only_env_var(key), diff --git a/docs/reference/gateway-config.mdx b/docs/reference/gateway-config.mdx index 7ee2545199..441edb4d5c 100644 --- a/docs/reference/gateway-config.mdx +++ b/docs/reference/gateway-config.mdx @@ -361,9 +361,16 @@ health_check_interval_secs = 10 # IPs, CIDRs) are dialed directly. This is an operator-owned egress boundary: # sandbox and template environment cannot override it, and the conventional # HTTPS_PROXY/HTTP_PROXY/NO_PROXY variables a sandbox sets do not affect it. -# https_proxy = "http://proxy.corp.com:8080" -# http_proxy = "http://proxy.corp.com:8080" -# no_proxy = "*.svc.cluster.local,10.0.0.0/8" +# +# Credentials must NOT be embedded in the URL (an inline user:pass@ is +# rejected at startup, since it would be stored here and exposed in container +# metadata). Instead point proxy_auth_file at a file containing "user:pass"; +# the gateway delivers it to the supervisor through a root-only secret mount. +# Keep gateway.toml and the auth file owner-readable only (mode 0600). +# https_proxy = "http://proxy.corp.com:8080" +# http_proxy = "http://proxy.corp.com:8080" +# no_proxy = "*.svc.cluster.local,10.0.0.0/8" +# proxy_auth_file = "/etc/openshell/secrets/proxy-auth" ``` ### MicroVM diff --git a/tasks/scripts/gateway.sh b/tasks/scripts/gateway.sh index 53edff81d7..92e67bddd9 100644 --- a/tasks/scripts/gateway.sh +++ b/tasks/scripts/gateway.sh @@ -310,6 +310,9 @@ echo "Generating local gateway credentials..." mkdir -p "${STATE_DIR}" CONFIG_PATH="${STATE_DIR}/gateway.toml" +# The config may reference credential-bearing material (e.g. proxy_auth_file); +# keep it owner-only regardless of the ambient umask. +install -m 600 /dev/null "${CONFIG_PATH}" cat >"${CONFIG_PATH}" <>"${CONFIG_PATH}" fi + if [[ -n "${OPENSHELL_SANDBOX_PROXY_AUTH_FILE:-}" ]]; then + printf 'proxy_auth_file = "%s"\n' "${OPENSHELL_SANDBOX_PROXY_AUTH_FILE}" >>"${CONFIG_PATH}" + fi ;; esac From cd9fd6cd84fc5e9f4726542a3770d2508e66f996 Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Thu, 16 Jul 2026 14:57:38 +0200 Subject: [PATCH 4/7] fix(sandbox,podman): fail closed on invalid upstream proxy configuration The reserved OPENSHELL_UPSTREAM_* variables are an operator-owned egress boundary, but the supervisor treated present-but-invalid values as unset: an unsupported or malformed proxy URL was ignored with a warning, an unreadable auth file proceeded without credentials, and a malformed credential silently became unauthenticated. Any of these could quietly downgrade the corporate proxy boundary to direct dialing or unauthenticated proxy access. Make every configured-but-invalid proxy or auth setting fatal to supervisor proxy startup, emit an OCSF ConfigStateChange failure event before refusing, and share URL validation semantics between the Podman driver and the supervisor through a single validator in openshell-core (parse_upstream_proxy_url), so a value accepted at sandbox-create time can never be rejected in-container or vice versa. Inline user:pass@ URL credentials are now fatal in the supervisor too (previously warn-and-strip), matching the driver. Unset or empty variables still mean no proxy; only present-but-invalid values fail. Error paths never include credential content. Addresses the fail-closed review item on #2245. Signed-off-by: Philippe Martin --- Cargo.lock | 1 - architecture/sandbox.md | 22 +- crates/openshell-core/src/driver_utils.rs | 152 ++++++++ crates/openshell-driver-podman/Cargo.toml | 1 - crates/openshell-driver-podman/src/config.rs | 72 ++-- .../openshell-supervisor-network/src/proxy.rs | 21 +- .../src/upstream_proxy.rs | 361 ++++++++++-------- docs/reference/gateway-config.mdx | 5 + 8 files changed, 420 insertions(+), 215 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 289ec359cb..d061c74496 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3717,7 +3717,6 @@ dependencies = [ "tonic", "tracing", "tracing-subscriber", - "url", ] [[package]] diff --git a/architecture/sandbox.md b/architecture/sandbox.md index f17015a090..8a2f00ae4b 100644 --- a/architecture/sandbox.md +++ b/architecture/sandbox.md @@ -93,14 +93,22 @@ the corporate proxy is the requested hostname. The workload child's proxy variables are unaffected — they are always rewritten to point at the local policy proxy. +The configuration is fail-closed: a reserved variable that is present but +invalid — an unsupported or malformed proxy URL, an unreadable auth file, or a +malformed credential — is fatal to supervisor startup instead of being treated +as unset, so a misconfiguration can never silently degrade to direct dialing +or unauthenticated proxy access. The driver validates the same rules at +sandbox-create time through a validator shared with the supervisor +(`openshell_core::driver_utils::parse_upstream_proxy_url`). + Proxy credentials are never embedded in the URL: an inline `user:pass@` is -rejected at startup because it would be stored in `gateway.toml` and exposed in -container metadata. Operators supply credentials via `proxy_auth_file`; the -driver stages them as a root-only secret mounted at a fixed path and exports -only that path in `OPENSHELL_UPSTREAM_PROXY_AUTH_FILE`. The supervisor reads the -file and builds the `Proxy-Authorization: Basic` header, rejecting credentials -containing control characters. The reserved proxy variables — including the -auth-file path — are stripped from workload child processes. +rejected because it would be stored in `gateway.toml` and exposed in container +metadata. Operators supply credentials via `proxy_auth_file`; the driver +stages them as a root-only secret mounted at a fixed path and exports only +that path in `OPENSHELL_UPSTREAM_PROXY_AUTH_FILE`. The supervisor reads the +file and builds the `Proxy-Authorization: Basic` header; an empty credential +or one containing control characters is fatal. The reserved proxy variables — +including the auth-file path — are stripped from workload child processes. ## Credentials diff --git a/crates/openshell-core/src/driver_utils.rs b/crates/openshell-core/src/driver_utils.rs index f87a23d8ae..4a33cae610 100644 --- a/crates/openshell-core/src/driver_utils.rs +++ b/crates/openshell-core/src/driver_utils.rs @@ -79,6 +79,97 @@ pub const SANDBOX_TOKEN_MOUNT_PATH: &str = "/etc/openshell/auth/sandbox.jwt"; /// mount so the credential never appears in container environment/metadata. pub const UPSTREAM_PROXY_AUTH_MOUNT_PATH: &str = "/etc/openshell/auth/upstream-proxy"; +/// A validated corporate upstream-proxy address. +/// +/// Produced by [`parse_upstream_proxy_url`], which is the single source of +/// truth for what counts as a valid upstream proxy URL. Compute drivers use +/// it to reject bad operator config at sandbox-create time, and the +/// in-container supervisor applies the same rules to the reserved +/// `OPENSHELL_UPSTREAM_*` variables so a value one side accepts is never +/// rejected (or silently ignored) by the other. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct UpstreamProxyAddr { + /// Proxy hostname, IPv4, or IPv6 address (IPv6 without brackets). + pub host: String, + /// Proxy TCP port (defaults to 80 when the URL has none). + pub port: u16, +} + +/// Why an upstream proxy URL was rejected by [`parse_upstream_proxy_url`]. +/// +/// Kept as a typed error so each consumer (driver config validation, +/// supervisor startup) can phrase the message for its own surface while +/// enforcing identical semantics. +#[derive(Debug, Clone, PartialEq, Eq, thiserror::Error)] +pub enum UpstreamProxyUrlError { + /// The value is empty or whitespace-only. + #[error("proxy URL is empty")] + Empty, + /// The value does not parse as a URL. + #[error("not a valid proxy URL: {0}")] + Invalid(url::ParseError), + /// The URL uses a scheme other than `http` (TLS and SOCKS proxies are + /// not supported by the sandbox supervisor). + #[error( + "unsupported proxy scheme '{0}': only http:// forward proxies are \ + supported by the sandbox supervisor" + )] + UnsupportedScheme(String), + /// The URL embeds `user:pass@` credentials, which would leak into config + /// and container metadata. Credentials must come from the proxy auth file. + #[error("proxy URL must not embed credentials; supply them via the proxy auth file")] + InlineCredentials, + /// The URL has no host component. + #[error("proxy URL is missing a proxy host")] + MissingHost, +} + +/// Parse and validate a corporate upstream-proxy URL. +/// +/// A bare `host:port` (no `://`) is normalized to `http://host:port`. Only +/// `http://` proxies are accepted, inline userinfo is rejected, and the port +/// defaults to 80. +/// +/// # Errors +/// +/// Returns an [`UpstreamProxyUrlError`] describing the first rule the value +/// violates. +pub fn parse_upstream_proxy_url(raw: &str) -> Result { + let trimmed = raw.trim(); + if trimmed.is_empty() { + return Err(UpstreamProxyUrlError::Empty); + } + let parsed = if trimmed.contains("://") { + url::Url::parse(trimmed) + } else { + url::Url::parse(&format!("http://{trimmed}")) + } + .map_err(UpstreamProxyUrlError::Invalid)?; + + if !parsed.scheme().eq_ignore_ascii_case("http") { + return Err(UpstreamProxyUrlError::UnsupportedScheme( + parsed.scheme().to_string(), + )); + } + if !parsed.username().is_empty() || parsed.password().is_some() { + return Err(UpstreamProxyUrlError::InlineCredentials); + } + let host = match parsed.host() { + // `Host::Ipv6` renders without brackets, which is what socket + // connect APIs expect. + Some(url::Host::Ipv6(ip)) => ip.to_string(), + Some(host) => host.to_string(), + None => return Err(UpstreamProxyUrlError::MissingHost), + }; + if host.is_empty() { + return Err(UpstreamProxyUrlError::MissingHost); + } + Ok(UpstreamProxyAddr { + host, + port: parsed.port().unwrap_or(80), + }) +} + /// Return the XDG state path for a driver's sandbox JWT token file. /// /// The resulting path is `$XDG_STATE_HOME/openshell/[/]//sandbox.jwt`. @@ -167,3 +258,64 @@ pub fn supervisor_image_tag(image: &str) -> Option<&str> { pub fn supervisor_image_should_refresh(image: &str) -> bool { matches!(supervisor_image_tag(image), Some("dev" | "latest")) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn upstream_proxy_url_accepts_http_with_port() { + let addr = parse_upstream_proxy_url("http://proxy.corp.com:8080").unwrap(); + assert_eq!(addr.host, "proxy.corp.com"); + assert_eq!(addr.port, 8080); + } + + #[test] + fn upstream_proxy_url_defaults_scheme_and_port() { + let addr = parse_upstream_proxy_url("proxy.corp.com").unwrap(); + assert_eq!(addr.host, "proxy.corp.com"); + assert_eq!(addr.port, 80); + let addr = parse_upstream_proxy_url("proxy.corp.com:3128").unwrap(); + assert_eq!(addr.port, 3128); + } + + #[test] + fn upstream_proxy_url_ipv6_host_is_bracket_free() { + let addr = parse_upstream_proxy_url("http://[fd00::1]:8080").unwrap(); + assert_eq!(addr.host, "fd00::1"); + assert_eq!(addr.port, 8080); + } + + #[test] + fn upstream_proxy_url_rejects_tls_and_socks_schemes() { + for url in ["https://proxy:443", "socks5://proxy:1080"] { + assert!(matches!( + parse_upstream_proxy_url(url), + Err(UpstreamProxyUrlError::UnsupportedScheme(_)) + )); + } + } + + #[test] + fn upstream_proxy_url_rejects_inline_credentials() { + for url in ["http://user:pass@proxy:8080", "http://user@proxy:8080"] { + assert_eq!( + parse_upstream_proxy_url(url), + Err(UpstreamProxyUrlError::InlineCredentials) + ); + } + } + + #[test] + fn upstream_proxy_url_rejects_empty_and_invalid() { + assert_eq!( + parse_upstream_proxy_url(" "), + Err(UpstreamProxyUrlError::Empty) + ); + assert!(matches!( + parse_upstream_proxy_url("http://proxy:notaport"), + Err(UpstreamProxyUrlError::Invalid(_)) + )); + assert!(parse_upstream_proxy_url("http://").is_err()); + } +} diff --git a/crates/openshell-driver-podman/Cargo.toml b/crates/openshell-driver-podman/Cargo.toml index 5f108fd9cc..ed798c0ab2 100644 --- a/crates/openshell-driver-podman/Cargo.toml +++ b/crates/openshell-driver-podman/Cargo.toml @@ -34,7 +34,6 @@ tracing = { workspace = true } tracing-subscriber = { workspace = true } thiserror = { workspace = true } miette = { workspace = true } -url = { workspace = true } [dev-dependencies] prost-types = { workspace = true } diff --git a/crates/openshell-driver-podman/src/config.rs b/crates/openshell-driver-podman/src/config.rs index 2be2a79afd..9df4f06b76 100644 --- a/crates/openshell-driver-podman/src/config.rs +++ b/crates/openshell-driver-podman/src/config.rs @@ -6,19 +6,6 @@ use std::net::IpAddr; use std::path::PathBuf; use std::str::FromStr; -/// Parse a proxy URL, defaulting a missing scheme to `http://`. -/// -/// A bare `host:port` (no `://`) is normalized to `http://host:port` so it -/// parses the same way the in-sandbox supervisor treats a scheme-less proxy -/// value. Returns the parsed [`url::Url`] for scheme/userinfo/host inspection. -fn parse_proxy_url(raw: &str) -> Result { - if raw.contains("://") { - url::Url::parse(raw) - } else { - url::Url::parse(&format!("http://{raw}")) - } -} - /// Default Podman bridge network name. pub const DEFAULT_NETWORK_NAME: &str = "openshell"; pub const MACOS_PODMAN_MACHINE_HOST_GATEWAY_IP: &str = "192.168.127.254"; @@ -233,49 +220,34 @@ impl PodmanComputeConfig { /// Validate optional corporate proxy configuration. /// - /// The in-container supervisor only supports `http://` forward proxies, so - /// reject other schemes at config time instead of silently ignoring them - /// inside every sandbox. Credentials must be supplied through - /// `proxy_auth_file`; an inline `user:pass@` in the URL is rejected because - /// it would otherwise be stored in `gateway.toml` and exposed in container - /// metadata. + /// Shares validation semantics with the in-container supervisor through + /// [`openshell_core::driver_utils::parse_upstream_proxy_url`], so a value + /// accepted here can never be rejected by the supervisor at sandbox + /// startup (or vice versa). The supervisor only supports `http://` + /// forward proxies, so other schemes are rejected at config time instead + /// of failing inside every sandbox. Credentials must be supplied through + /// `proxy_auth_file`; an inline `user:pass@` in the URL is rejected + /// because it would otherwise be stored in `gateway.toml` and exposed in + /// container metadata. pub fn validate_proxy_config(&self) -> Result<(), crate::client::PodmanApiError> { + use openshell_core::driver_utils::{UpstreamProxyUrlError, parse_upstream_proxy_url}; for (field, value) in [ ("https_proxy", &self.https_proxy), ("http_proxy", &self.http_proxy), ] { let Some(url) = value else { continue }; - let trimmed = url.trim(); - if trimmed.is_empty() { - return Err(crate::client::PodmanApiError::InvalidInput(format!( - "{field} must not be empty when set" - ))); - } - - let parsed = parse_proxy_url(trimmed).map_err(|e| { - crate::client::PodmanApiError::InvalidInput(format!( - "{field} is not a valid proxy URL: {e}" - )) + parse_upstream_proxy_url(url).map_err(|err| { + crate::client::PodmanApiError::InvalidInput(match err { + UpstreamProxyUrlError::Empty => { + format!("{field} must not be empty when set") + } + UpstreamProxyUrlError::InlineCredentials => format!( + "{field} must not embed credentials in the URL; supply them via \ + proxy_auth_file so they are not stored in config or container metadata" + ), + err => format!("{field} {err}"), + }) })?; - - if !parsed.scheme().eq_ignore_ascii_case("http") { - return Err(crate::client::PodmanApiError::InvalidInput(format!( - "{field} uses unsupported scheme '{}': only http:// forward \ - proxies are supported by the sandbox supervisor", - parsed.scheme() - ))); - } - if !parsed.username().is_empty() || parsed.password().is_some() { - return Err(crate::client::PodmanApiError::InvalidInput(format!( - "{field} must not embed credentials in the URL; supply them via \ - proxy_auth_file so they are not stored in config or container metadata" - ))); - } - if parsed.host_str().is_none_or(str::is_empty) { - return Err(crate::client::PodmanApiError::InvalidInput(format!( - "{field} is missing a proxy host" - ))); - } } if let Some(path) = self.proxy_auth_file.as_deref() { @@ -538,7 +510,7 @@ mod tests { }; let err = cfg.validate_proxy_config().unwrap_err(); assert!( - err.to_string().contains("unsupported scheme"), + err.to_string().contains("unsupported proxy scheme"), "{url}: {err}" ); } diff --git a/crates/openshell-supervisor-network/src/proxy.rs b/crates/openshell-supervisor-network/src/proxy.rs index 7f28774c18..6d3d33d10c 100644 --- a/crates/openshell-supervisor-network/src/proxy.rs +++ b/crates/openshell-supervisor-network/src/proxy.rs @@ -246,8 +246,27 @@ impl ProxyHandle { // variables. Read once at startup; the workload cannot influence the // supervisor's environment, and the conventional HTTPS_PROXY/NO_PROXY // variables it does control are ignored on this path. + // + // This is an operator-owned security boundary, so a present-but-invalid + // value (bad proxy URL, unreadable auth file, malformed credential) is + // fatal to proxy startup: failing closed prevents a misconfiguration + // from silently degrading to direct dialing or unauthenticated proxy + // access. let upstream_proxy: Arc> = - Arc::new(UpstreamProxyConfig::from_env()); + Arc::new(UpstreamProxyConfig::from_env().map_err(|err| { + let event = + openshell_ocsf::ConfigStateChangeBuilder::new(openshell_ocsf::ctx::ctx()) + .severity(SeverityId::High) + .status(StatusId::Failure) + .state(openshell_ocsf::StateId::Disabled, "invalid") + .message(format!( + "Upstream corporate proxy configuration invalid; \ + refusing to start: {err}" + )) + .build(); + ocsf_emit!(event); + miette::miette!("invalid upstream corporate proxy configuration: {err}") + })?); if let Some(cfg) = upstream_proxy.as_ref() { let event = openshell_ocsf::ConfigStateChangeBuilder::new(openshell_ocsf::ctx::ctx()) .severity(SeverityId::Informational) diff --git a/crates/openshell-supervisor-network/src/upstream_proxy.rs b/crates/openshell-supervisor-network/src/upstream_proxy.rs index aa841f5f3c..aeb92e8f1a 100644 --- a/crates/openshell-supervisor-network/src/upstream_proxy.rs +++ b/crates/openshell-supervisor-network/src/upstream_proxy.rs @@ -20,8 +20,14 @@ //! template environment cannot override. //! //! Scope and invariants: -//! - Only `http://` proxy URLs are supported. `https://` and SOCKS proxies -//! are ignored with a warning. +//! - Only `http://` proxy URLs are supported. Configuration is fail-closed: +//! any present-but-invalid reserved value — an unsupported (`https://`, +//! SOCKS) or malformed proxy URL, an unreadable auth file, or a malformed +//! credential — is a fatal startup error rather than being silently +//! ignored, so a typo can never quietly downgrade the operator's egress +//! boundary to direct dialing or unauthenticated proxy access. Validation +//! semantics are shared with the compute driver via +//! [`openshell_core::driver_utils::parse_upstream_proxy_url`]. //! - Policy evaluation, DNS resolution, and SSRF checks run exactly as in the //! direct-dial path; the corporate proxy only replaces the final TCP dial. //! - The reserved `NO_PROXY` list decides which destinations bypass the @@ -35,7 +41,7 @@ use std::time::Duration; use base64::Engine as _; use tokio::io::{AsyncReadExt, AsyncWriteExt}; use tokio::net::TcpStream; -use tracing::{debug, warn}; +use tracing::debug; /// Upper bound on the corporate proxy's CONNECT response header block. const MAX_CONNECT_RESPONSE_BYTES: usize = 8 * 1024; @@ -176,8 +182,10 @@ impl UpstreamProxyConfig { /// supervisor's reserved environment variables /// ([`UPSTREAM_HTTPS_PROXY`](openshell_core::sandbox_env::UPSTREAM_HTTPS_PROXY), /// [`UPSTREAM_HTTP_PROXY`](openshell_core::sandbox_env::UPSTREAM_HTTP_PROXY), - /// [`UPSTREAM_NO_PROXY`](openshell_core::sandbox_env::UPSTREAM_NO_PROXY)). - /// Returns `None` when no usable proxy is configured. + /// [`UPSTREAM_NO_PROXY`](openshell_core::sandbox_env::UPSTREAM_NO_PROXY), + /// [`UPSTREAM_PROXY_AUTH_FILE`](openshell_core::sandbox_env::UPSTREAM_PROXY_AUTH_FILE)). + /// Returns `Ok(None)` when no proxy is configured (unset or empty + /// variables). /// /// The conventional `HTTPS_PROXY` / `HTTP_PROXY` / `ALL_PROXY` / `NO_PROXY` /// variables are intentionally ignored here: they are set by the sandbox @@ -186,60 +194,72 @@ impl UpstreamProxyConfig { /// upstream proxy or disable proxying entirely. The compute driver writes /// the reserved names in its required-variable tier, where sandbox and /// template environment cannot override them. - #[must_use] - pub fn from_env() -> Option { - let mut config = Self::from_lookup(|name| std::env::var(name).ok())?; - - // Load proxy credentials from the reserved auth file, if configured, - // and apply them to every endpoint. The file is delivered through a - // root-only secret mount so the credentials never appear in the - // environment or container metadata. - if let Some(path) = std::env::var(openshell_core::sandbox_env::UPSTREAM_PROXY_AUTH_FILE) - .ok() - .filter(|p| !p.trim().is_empty()) - { - match std::fs::read_to_string(&path) { - Ok(credential) => config.apply_proxy_auth(basic_auth_header(&credential)), - Err(err) => warn!( - "failed to read upstream proxy auth file '{path}': {err}; \ - proceeding without proxy credentials" - ), - } - } - - Some(config) - } - - fn from_lookup(lookup: impl Fn(&str) -> Option) -> Option { + /// + /// # Errors + /// + /// These reserved variables are an operator-owned security boundary, so + /// any present-but-invalid value is fatal instead of being treated as + /// unset: an invalid or unsupported proxy URL, an auth file that is set + /// but unreadable or holds a malformed credential, or an auth file with + /// no proxy configured. Failing closed here prevents a misconfiguration + /// from silently degrading to direct dialing or unauthenticated proxy + /// access. + pub fn from_env() -> Result, String> { + Self::from_lookup(|name| std::env::var(name).ok()) + } + + fn from_lookup(lookup: impl Fn(&str) -> Option) -> Result, String> { use openshell_core::sandbox_env::{ - UPSTREAM_HTTP_PROXY, UPSTREAM_HTTPS_PROXY, UPSTREAM_NO_PROXY, + UPSTREAM_HTTP_PROXY, UPSTREAM_HTTPS_PROXY, UPSTREAM_NO_PROXY, UPSTREAM_PROXY_AUTH_FILE, }; let var = |name: &str| lookup(name).filter(|v| !v.trim().is_empty()); - let https = - var(UPSTREAM_HTTPS_PROXY).and_then(|url| parse_proxy_url(&url, UPSTREAM_HTTPS_PROXY)); - let http = - var(UPSTREAM_HTTP_PROXY).and_then(|url| parse_proxy_url(&url, UPSTREAM_HTTP_PROXY)); + let https = var(UPSTREAM_HTTPS_PROXY) + .map(|url| parse_proxy_url(&url, UPSTREAM_HTTPS_PROXY)) + .transpose()?; + let http = var(UPSTREAM_HTTP_PROXY) + .map(|url| parse_proxy_url(&url, UPSTREAM_HTTP_PROXY)) + .transpose()?; + let auth_file = var(UPSTREAM_PROXY_AUTH_FILE); if https.is_none() && http.is_none() { - return None; + if auth_file.is_some() { + return Err(format!( + "{UPSTREAM_PROXY_AUTH_FILE} is set but no upstream proxy is configured" + )); + } + return Ok(None); } let no_proxy = NoProxy::parse(&var(UPSTREAM_NO_PROXY).unwrap_or_default()); - Some(Self { + let mut config = Self { https, http, no_proxy, - }) + }; + + // Load proxy credentials from the reserved auth file, if configured, + // and apply them to every endpoint. The file is delivered through a + // root-only secret mount so the credentials never appear in the + // environment or container metadata. + if let Some(path) = auth_file { + let credential = std::fs::read_to_string(&path).map_err(|err| { + format!("failed to read upstream proxy auth file '{path}': {err}") + })?; + let header = basic_auth_header(&credential).map_err(|err| { + format!("invalid credential in upstream proxy auth file '{path}': {err}") + })?; + config.apply_proxy_auth(header); + } + + Ok(Some(config)) } /// Attach a pre-built `Proxy-Authorization` header value to every - /// configured endpoint. A `None` value clears any existing credentials. - fn apply_proxy_auth(&mut self, proxy_authorization: Option) { + /// configured endpoint. + fn apply_proxy_auth(&mut self, proxy_authorization: String) { for endpoint in [self.https.as_mut(), self.http.as_mut()] .into_iter() .flatten() { - endpoint - .proxy_authorization - .clone_from(&proxy_authorization); + endpoint.proxy_authorization = Some(proxy_authorization.clone()); } } @@ -271,88 +291,49 @@ impl UpstreamProxyConfig { } } -/// Parse an `http://host[:port]` proxy URL. Unsupported schemes (TLS or SOCKS -/// proxies) are rejected with a warning. +/// Parse an `http://host[:port]` proxy URL with the same validation rules the +/// compute driver applies at sandbox-create time +/// ([`parse_upstream_proxy_url`](openshell_core::driver_utils::parse_upstream_proxy_url)). /// /// Credentials are never taken from the URL: they are delivered out of band /// through [`UPSTREAM_PROXY_AUTH_FILE`](openshell_core::sandbox_env::UPSTREAM_PROXY_AUTH_FILE) -/// so they never appear in config or container metadata. Any `user:pass@` -/// userinfo present in the URL is ignored with a warning. -fn parse_proxy_url(raw: &str, var_name: &str) -> Option { - let raw = raw.trim(); - let rest = match raw.split_once("://") { - Some((scheme, rest)) => { - if !scheme.eq_ignore_ascii_case("http") { - warn!( - "{var_name} uses unsupported proxy scheme '{scheme}' \ - (only http:// proxies are supported); ignoring" - ); - return None; - } - rest - } - None => raw, - }; - // Drop any path component. - let rest = rest.split(['/', '?', '#']).next().unwrap_or(rest); - let authority = match rest.rsplit_once('@') { - Some((_userinfo, authority)) => { - warn!( - "{var_name} contains inline credentials, which are ignored; \ - supply proxy credentials via the proxy auth file" - ); - authority - } - None => rest, - }; - let Some((host, port)) = split_host_port(authority) else { - warn!("{var_name} has an invalid proxy address '{authority}'; ignoring"); - return None; - }; - Some(ProxyEndpoint { - host, - port, +/// so they never appear in config or container metadata. +/// +/// # Errors +/// +/// Rejects unsupported schemes (TLS or SOCKS proxies), inline `user:pass@` +/// credentials, and malformed addresses. The error names `var_name` so the +/// operator can locate the offending setting. +fn parse_proxy_url(raw: &str, var_name: &str) -> Result { + let addr = openshell_core::driver_utils::parse_upstream_proxy_url(raw) + .map_err(|err| format!("{var_name} is invalid: {err}"))?; + Ok(ProxyEndpoint { + host: addr.host, + port: addr.port, proxy_authorization: None, }) } -/// Split `host[:port]` (with optional `[v6]` brackets), defaulting to port 80. -fn split_host_port(authority: &str) -> Option<(String, u16)> { - if authority.is_empty() { - return None; - } - if let Some(v6_end) = authority.find(']') { - if !authority.starts_with('[') { - return None; - } - let host = authority[1..v6_end].to_string(); - let port = match authority[v6_end + 1..].strip_prefix(':') { - Some(port) => port.parse().ok()?, - None if authority[v6_end + 1..].is_empty() => 80, - None => return None, - }; - return Some((host, port)); - } - match authority.rsplit_once(':') { - Some((host, port)) if !host.contains(':') => Some((host.to_string(), port.parse().ok()?)), - Some(_) => None, // bare IPv6 without brackets is ambiguous - None => Some((authority.to_string(), 80)), - } -} - /// Build a `Proxy-Authorization: Basic ` header value from a raw /// `user:pass` credential. /// -/// Returns `None` for an empty credential or one containing control characters -/// (CR, LF, NUL) that could inject additional HTTP headers. The credential is -/// used verbatim: it is delivered through a trusted operator file, not a URL, -/// so there is no percent-encoding to decode. -fn basic_auth_header(credential: &str) -> Option { +/// The credential is used verbatim: it is delivered through a trusted +/// operator file, not a URL, so there is no percent-encoding to decode. +/// +/// # Errors +/// +/// Rejects an empty credential and one containing control characters (CR, LF, +/// NUL) that could inject additional HTTP headers. Error messages never +/// include the credential content. +fn basic_auth_header(credential: &str) -> Result { let credential = credential.trim(); - if credential.is_empty() || credential.contains(|c: char| c.is_control()) { - return None; + if credential.is_empty() { + return Err("credential is empty".to_string()); } - Some(format!( + if credential.contains(|c: char| c.is_control()) { + return Err("credential contains control characters".to_string()); + } + Ok(format!( "Basic {}", base64::engine::general_purpose::STANDARD.encode(credential) )) @@ -474,10 +455,10 @@ mod tests { use super::*; use openshell_core::sandbox_env::{ UPSTREAM_HTTP_PROXY as HTTP_PROXY, UPSTREAM_HTTPS_PROXY as HTTPS_PROXY, - UPSTREAM_NO_PROXY as NO_PROXY, + UPSTREAM_NO_PROXY as NO_PROXY, UPSTREAM_PROXY_AUTH_FILE as PROXY_AUTH_FILE, }; - fn config_from(pairs: &[(&str, &str)]) -> Option { + fn config_from(pairs: &[(&str, &str)]) -> Result, String> { UpstreamProxyConfig::from_lookup(|name| { pairs .iter() @@ -486,9 +467,14 @@ mod tests { }) } + /// Shorthand for tests exercising a configuration that must load. + fn config_ok(pairs: &[(&str, &str)]) -> UpstreamProxyConfig { + config_from(pairs).unwrap().unwrap() + } + #[test] fn no_env_yields_none() { - assert!(config_from(&[]).is_none()); + assert!(config_from(&[]).unwrap().is_none()); } #[test] @@ -502,18 +488,23 @@ mod tests { ("ALL_PROXY", "http://attacker:9999"), ("https_proxy", "http://attacker:9999"), ]) + .unwrap() .is_none() ); } #[test] fn empty_values_yield_none() { - assert!(config_from(&[(HTTPS_PROXY, " "), (HTTP_PROXY, "")]).is_none()); + assert!( + config_from(&[(HTTPS_PROXY, " "), (HTTP_PROXY, "")]) + .unwrap() + .is_none() + ); } #[test] fn https_proxy_parsed_with_port() { - let cfg = config_from(&[(HTTPS_PROXY, "http://proxy.corp.com:8080")]).unwrap(); + let cfg = config_ok(&[(HTTPS_PROXY, "http://proxy.corp.com:8080")]); let ep = cfg .proxy_for(UpstreamScheme::Https, "api.stripe.com") .unwrap(); @@ -527,67 +518,127 @@ mod tests { #[test] fn scheme_defaults_to_http_and_port_defaults_to_80() { - let cfg = config_from(&[(HTTP_PROXY, "proxy.corp.com")]).unwrap(); + let cfg = config_ok(&[(HTTP_PROXY, "proxy.corp.com")]); let ep = cfg.proxy_for(UpstreamScheme::Http, "example.com").unwrap(); assert_eq!(ep.display_addr(), "proxy.corp.com:80"); } + // -- Fail-closed configuration validation -- + // + // Present-but-invalid reserved values must be fatal, never silently + // treated as unset: a typo must not downgrade the operator's egress + // boundary to direct dialing or unauthenticated proxy access. + #[test] - fn tls_and_socks_proxies_rejected() { - assert!(config_from(&[(HTTPS_PROXY, "https://proxy:443")]).is_none()); - assert!(config_from(&[(HTTPS_PROXY, "socks5://proxy:1080")]).is_none()); + fn tls_and_socks_proxies_are_fatal() { + for url in ["https://proxy:443", "socks5://proxy:1080"] { + let err = config_from(&[(HTTPS_PROXY, url)]).unwrap_err(); + assert!(err.contains(HTTPS_PROXY), "{err}"); + assert!(err.contains("scheme"), "{err}"); + } } #[test] - fn url_userinfo_is_ignored_not_used_as_credentials() { + fn url_userinfo_is_fatal_not_used_as_credentials() { // Inline credentials in the URL must never become the proxy auth; - // credentials come only from the auth file. - let cfg = config_from(&[(HTTPS_PROXY, "http://user:secret@proxy:8080")]).unwrap(); - let ep = cfg.proxy_for(UpstreamScheme::Https, "example.com").unwrap(); - assert_eq!(ep.display_addr(), "proxy:8080"); + // credentials come only from the auth file. Matching the compute + // driver, a URL that embeds them is rejected outright. + let err = config_from(&[(HTTPS_PROXY, "http://user:secret@proxy:8080")]).unwrap_err(); + assert!(err.contains(HTTPS_PROXY), "{err}"); assert!( - ep.proxy_authorization.is_none(), - "URL userinfo must not be used as proxy credentials" + !err.contains("secret"), + "error must not leak the credential: {err}" ); } #[test] - fn basic_auth_header_encodes_and_rejects_control_chars() { - assert_eq!( - basic_auth_header("user:p@ss").as_deref(), - Some( - format!( - "Basic {}", - base64::engine::general_purpose::STANDARD.encode("user:p@ss") - ) - .as_str() - ) - ); - assert!(basic_auth_header(" ").is_none()); - assert!(basic_auth_header("user:pa\r\nss").is_none()); - assert!(basic_auth_header("user:pa\nInjected: header").is_none()); + fn malformed_proxy_address_is_fatal() { + let err = config_from(&[(HTTP_PROXY, "http://proxy:notaport")]).unwrap_err(); + assert!(err.contains(HTTP_PROXY), "{err}"); + // One invalid endpoint is fatal even when the other one is valid. + let err = config_from(&[ + (HTTPS_PROXY, "http://proxy.corp.com:8080"), + (HTTP_PROXY, "socks5://proxy:1080"), + ]) + .unwrap_err(); + assert!(err.contains(HTTP_PROXY), "{err}"); + } + + #[test] + fn unreadable_auth_file_is_fatal() { + let err = config_from(&[ + (HTTPS_PROXY, "http://proxy.corp.com:8080"), + (PROXY_AUTH_FILE, "/nonexistent/upstream-proxy-auth"), + ]) + .unwrap_err(); + assert!(err.contains("auth file"), "{err}"); + } + + #[test] + fn malformed_auth_file_credential_is_fatal() { + for credential in [" ", "user:pa\r\nss"] { + let file = tempfile::NamedTempFile::new().unwrap(); + std::fs::write(file.path(), credential).unwrap(); + let path = file.path().to_str().unwrap().to_string(); + let err = config_from(&[ + (HTTPS_PROXY, "http://proxy.corp.com:8080"), + (PROXY_AUTH_FILE, &path), + ]) + .unwrap_err(); + assert!(err.contains("auth file"), "{err}"); + assert!( + !err.contains("pa\r\nss"), + "error must not leak the credential: {err}" + ); + } + } + + #[test] + fn auth_file_without_proxy_is_fatal() { + let err = + config_from(&[(PROXY_AUTH_FILE, "/etc/openshell/auth/upstream-proxy")]).unwrap_err(); + assert!(err.contains(PROXY_AUTH_FILE), "{err}"); } #[test] - fn apply_proxy_auth_sets_header_on_all_endpoints() { - let mut cfg = config_from(&[ + fn auth_file_credentials_are_applied_to_all_endpoints() { + let file = tempfile::NamedTempFile::new().unwrap(); + std::fs::write(file.path(), "user:secret\n").unwrap(); + let path = file.path().to_str().unwrap().to_string(); + let cfg = config_ok(&[ (HTTPS_PROXY, "http://proxy:8080"), (HTTP_PROXY, "http://proxy:3128"), - ]) - .unwrap(); - cfg.apply_proxy_auth(basic_auth_header("user:secret")); + (PROXY_AUTH_FILE, &path), + ]); + let expected = format!( + "Basic {}", + base64::engine::general_purpose::STANDARD.encode("user:secret") + ); for scheme in [UpstreamScheme::Https, UpstreamScheme::Http] { let ep = cfg.proxy_for(scheme, "example.com").unwrap(); - let auth = ep.proxy_authorization.as_deref().unwrap(); - let expected = base64::engine::general_purpose::STANDARD.encode("user:secret"); - assert_eq!(auth, format!("Basic {expected}")); + assert_eq!(ep.proxy_authorization.as_deref(), Some(expected.as_str())); } } + #[test] + fn basic_auth_header_encodes_and_rejects_control_chars() { + assert_eq!( + basic_auth_header("user:p@ss").as_deref(), + Ok(format!( + "Basic {}", + base64::engine::general_purpose::STANDARD.encode("user:p@ss") + ) + .as_str()) + ); + assert!(basic_auth_header(" ").is_err()); + assert!(basic_auth_header("user:pa\r\nss").is_err()); + assert!(basic_auth_header("user:pa\nInjected: header").is_err()); + } + #[test] fn debug_output_hides_credentials() { - let mut cfg = config_from(&[(HTTPS_PROXY, "http://proxy:8080")]).unwrap(); - cfg.apply_proxy_auth(basic_auth_header("user:secret")); + let mut cfg = config_ok(&[(HTTPS_PROXY, "http://proxy:8080")]); + cfg.apply_proxy_auth(basic_auth_header("user:secret").unwrap()); let debug = format!("{cfg:?}"); assert!(!debug.contains("secret")); assert!(!cfg.summary().contains("secret")); @@ -595,7 +646,7 @@ mod tests { #[test] fn ipv6_proxy_address_parses() { - let cfg = config_from(&[(HTTPS_PROXY, "http://[fd00::1]:8080")]).unwrap(); + let cfg = config_ok(&[(HTTPS_PROXY, "http://[fd00::1]:8080")]); let ep = cfg.proxy_for(UpstreamScheme::Https, "example.com").unwrap(); assert_eq!(ep.display_addr(), "fd00::1:8080"); } @@ -603,7 +654,7 @@ mod tests { // -- NO_PROXY matching -- fn no_proxy_cfg(no_proxy: &str) -> UpstreamProxyConfig { - config_from(&[(HTTPS_PROXY, "http://proxy:8080"), (NO_PROXY, no_proxy)]).unwrap() + config_ok(&[(HTTPS_PROXY, "http://proxy:8080"), (NO_PROXY, no_proxy)]) } fn bypasses(cfg: &UpstreamProxyConfig, host: &str) -> bool { diff --git a/docs/reference/gateway-config.mdx b/docs/reference/gateway-config.mdx index 441edb4d5c..f91c1e5f83 100644 --- a/docs/reference/gateway-config.mdx +++ b/docs/reference/gateway-config.mdx @@ -362,6 +362,11 @@ health_check_interval_secs = 10 # sandbox and template environment cannot override it, and the conventional # HTTPS_PROXY/HTTP_PROXY/NO_PROXY variables a sandbox sets do not affect it. # +# Configuration is fail-closed: an invalid proxy URL is rejected at gateway +# startup, and a set-but-invalid value reaching a sandbox (for example an +# unreadable or malformed auth file) is fatal to that sandbox's supervisor +# instead of silently falling back to direct or unauthenticated egress. +# # Credentials must NOT be embedded in the URL (an inline user:pass@ is # rejected at startup, since it would be stored here and exposed in container # metadata). Instead point proxy_auth_file at a file containing "user:pass"; From fad5b4e42fe8cf574cd345fb70d0df6af012f35f Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Thu, 16 Jul 2026 15:40:34 +0200 Subject: [PATCH 5/7] fix(sandbox,podman): finish the fail-closed upstream proxy credential contract The upstream proxy URL already had a single shared validator, but the credential did not: the Podman driver rejected only CR/LF/NUL while the supervisor rejected every control character, so a credential accepted at sandbox-create time (e.g. one containing a tab) could still be rejected in-container. Present-but-whitespace reserved OPENSHELL_UPSTREAM_* values were also silently treated as unset, quietly downgrading the operator's egress boundary to direct dialing. Add parse_upstream_proxy_credential to openshell-core as the single source of truth for the documented user:pass credential form (non-empty user, no control characters, trimmed) and use it in both the Podman driver's secret staging and the supervisor's Proxy-Authorization header construction. Error variants carry no payload so credential content can never leak into messages. Make a present-but-empty reserved variable fatal to supervisor proxy startup instead of meaning "unset"; only fully unset variables disable the proxy. The driver correspondingly rejects an empty no_proxy at config time so it can never inject a value the supervisor refuses. Addresses the remaining fail-closed credential/config review item on Signed-off-by: Philippe Martin #2245. --- architecture/sandbox.md | 19 ++-- crates/openshell-core/src/driver_utils.rs | 100 +++++++++++++++++ crates/openshell-driver-podman/src/config.rs | 23 ++++ crates/openshell-driver-podman/src/driver.rs | 24 ++-- .../src/upstream_proxy.rs | 104 +++++++++++------- docs/reference/gateway-config.mdx | 3 + 6 files changed, 211 insertions(+), 62 deletions(-) diff --git a/architecture/sandbox.md b/architecture/sandbox.md index 8a2f00ae4b..1dbf91149d 100644 --- a/architecture/sandbox.md +++ b/architecture/sandbox.md @@ -94,20 +94,23 @@ variables are unaffected — they are always rewritten to point at the local policy proxy. The configuration is fail-closed: a reserved variable that is present but -invalid — an unsupported or malformed proxy URL, an unreadable auth file, or a -malformed credential — is fatal to supervisor startup instead of being treated -as unset, so a misconfiguration can never silently degrade to direct dialing -or unauthenticated proxy access. The driver validates the same rules at -sandbox-create time through a validator shared with the supervisor -(`openshell_core::driver_utils::parse_upstream_proxy_url`). +invalid — a present-but-empty value, an unsupported or malformed proxy URL, an +unreadable auth file, or a malformed credential — is fatal to supervisor +startup instead of being treated as unset, so a misconfiguration can never +silently degrade to direct dialing or unauthenticated proxy access. Only a +fully unset variable means "no proxy". The driver validates the same rules at +sandbox-create time through validators shared with the supervisor +(`openshell_core::driver_utils::parse_upstream_proxy_url` and +`parse_upstream_proxy_credential`). Proxy credentials are never embedded in the URL: an inline `user:pass@` is rejected because it would be stored in `gateway.toml` and exposed in container metadata. Operators supply credentials via `proxy_auth_file`; the driver stages them as a root-only secret mounted at a fixed path and exports only that path in `OPENSHELL_UPSTREAM_PROXY_AUTH_FILE`. The supervisor reads the -file and builds the `Proxy-Authorization: Basic` header; an empty credential -or one containing control characters is fatal. The reserved proxy variables — +file and builds the `Proxy-Authorization: Basic` header; a credential that is +empty, contains control characters, or is not in `user:pass` form is fatal on +both sides. The reserved proxy variables — including the auth-file path — are stripped from workload child processes. ## Credentials diff --git a/crates/openshell-core/src/driver_utils.rs b/crates/openshell-core/src/driver_utils.rs index 4a33cae610..dd322fb80c 100644 --- a/crates/openshell-core/src/driver_utils.rs +++ b/crates/openshell-core/src/driver_utils.rs @@ -170,6 +170,60 @@ pub fn parse_upstream_proxy_url(raw: &str) -> Result Result<&str, UpstreamProxyCredentialError> { + let credential = raw.trim(); + if credential.is_empty() { + return Err(UpstreamProxyCredentialError::Empty); + } + if credential.contains(|c: char| c.is_control()) { + return Err(UpstreamProxyCredentialError::ControlCharacters); + } + match credential.split_once(':') { + None => Err(UpstreamProxyCredentialError::MissingSeparator), + Some(("", _)) => Err(UpstreamProxyCredentialError::EmptyUser), + Some(_) => Ok(credential), + } +} + /// Return the XDG state path for a driver's sandbox JWT token file. /// /// The resulting path is `$XDG_STATE_HOME/openshell/[/]//sandbox.jwt`. @@ -318,4 +372,50 @@ mod tests { )); assert!(parse_upstream_proxy_url("http://").is_err()); } + + #[test] + fn upstream_proxy_credential_accepts_user_pass_and_trims() { + assert_eq!( + parse_upstream_proxy_credential("user:pass\n"), + Ok("user:pass") + ); + // The password may be empty and may contain further colons. + assert_eq!(parse_upstream_proxy_credential("user:"), Ok("user:")); + assert_eq!( + parse_upstream_proxy_credential("user:p@:ss"), + Ok("user:p@:ss") + ); + } + + #[test] + fn upstream_proxy_credential_rejects_empty() { + for raw in ["", " ", "\n"] { + assert_eq!( + parse_upstream_proxy_credential(raw), + Err(UpstreamProxyCredentialError::Empty) + ); + } + } + + #[test] + fn upstream_proxy_credential_rejects_control_characters() { + for raw in ["user:pa\r\nss", "user:pa\0ss", "user:pa\tss"] { + assert_eq!( + parse_upstream_proxy_credential(raw), + Err(UpstreamProxyCredentialError::ControlCharacters) + ); + } + } + + #[test] + fn upstream_proxy_credential_rejects_malformed_user_pass_form() { + assert_eq!( + parse_upstream_proxy_credential("userpass"), + Err(UpstreamProxyCredentialError::MissingSeparator) + ); + assert_eq!( + parse_upstream_proxy_credential(":pass"), + Err(UpstreamProxyCredentialError::EmptyUser) + ); + } } diff --git a/crates/openshell-driver-podman/src/config.rs b/crates/openshell-driver-podman/src/config.rs index 9df4f06b76..e205815fdf 100644 --- a/crates/openshell-driver-podman/src/config.rs +++ b/crates/openshell-driver-podman/src/config.rs @@ -250,6 +250,18 @@ impl PodmanComputeConfig { })?; } + // The supervisor treats a present-but-empty reserved variable as a + // fatal misconfiguration, so never accept (and later inject) one. + if self + .no_proxy + .as_deref() + .is_some_and(|list| list.trim().is_empty()) + { + return Err(crate::client::PodmanApiError::InvalidInput( + "no_proxy must not be empty when set; omit it instead".to_string(), + )); + } + if let Some(path) = self.proxy_auth_file.as_deref() { if path.trim().is_empty() { return Err(crate::client::PodmanApiError::InvalidInput( @@ -526,6 +538,17 @@ mod tests { assert!(err.to_string().contains("http_proxy"), "{err}"); } + #[test] + fn validate_proxy_config_rejects_empty_no_proxy() { + let cfg = PodmanComputeConfig { + https_proxy: Some("http://proxy.corp.com:8080".to_string()), + no_proxy: Some(" ".to_string()), + ..PodmanComputeConfig::default() + }; + let err = cfg.validate_proxy_config().unwrap_err(); + assert!(err.to_string().contains("no_proxy"), "{err}"); + } + #[test] fn validate_proxy_config_rejects_inline_credentials() { for url in [ diff --git a/crates/openshell-driver-podman/src/driver.rs b/crates/openshell-driver-podman/src/driver.rs index f927f00754..548cd390ba 100644 --- a/crates/openshell-driver-podman/src/driver.rs +++ b/crates/openshell-driver-podman/src/driver.rs @@ -118,8 +118,11 @@ async fn cleanup_sandbox_token_secret(client: &PodmanClient, secret_name: &str) /// Podman secret, so the credentials reach the supervisor through a root-only /// mount rather than the container environment. /// -/// Fails closed: when `proxy_auth_file` is configured but cannot be read or is -/// empty/malformed, the sandbox is not created. +/// Fails closed: when `proxy_auth_file` is configured but cannot be read or +/// does not hold a valid `user:pass` credential, the sandbox is not created. +/// Credential validation is shared with the in-container supervisor through +/// [`openshell_core::driver_utils::parse_upstream_proxy_credential`], so a +/// credential staged here can never be rejected at supervisor startup. async fn create_sandbox_proxy_auth_secret( client: &PodmanClient, config: &PodmanComputeConfig, @@ -132,19 +135,10 @@ async fn create_sandbox_proxy_auth_secret( let raw = tokio::fs::read_to_string(path).await.map_err(|e| { ComputeDriverError::Message(format!("failed to read proxy_auth_file '{path}': {e}")) })?; - let credential = raw.trim(); - if credential.is_empty() { - return Err(ComputeDriverError::InvalidArgument(format!( - "proxy_auth_file '{path}' is empty" - ))); - } - // A credential containing CR/LF/NUL would allow HTTP header injection once - // the supervisor places it in the Proxy-Authorization header. - if credential.contains(['\r', '\n', '\0']) { - return Err(ComputeDriverError::InvalidArgument(format!( - "proxy_auth_file '{path}' must not contain control characters" - ))); - } + let credential = + openshell_core::driver_utils::parse_upstream_proxy_credential(&raw).map_err(|err| { + ComputeDriverError::InvalidArgument(format!("proxy_auth_file '{path}': {err}")) + })?; let secret_name = container::proxy_auth_secret_name(&sandbox.id); client diff --git a/crates/openshell-supervisor-network/src/upstream_proxy.rs b/crates/openshell-supervisor-network/src/upstream_proxy.rs index aeb92e8f1a..2fdfb51150 100644 --- a/crates/openshell-supervisor-network/src/upstream_proxy.rs +++ b/crates/openshell-supervisor-network/src/upstream_proxy.rs @@ -21,13 +21,14 @@ //! //! Scope and invariants: //! - Only `http://` proxy URLs are supported. Configuration is fail-closed: -//! any present-but-invalid reserved value — an unsupported (`https://`, -//! SOCKS) or malformed proxy URL, an unreadable auth file, or a malformed -//! credential — is a fatal startup error rather than being silently -//! ignored, so a typo can never quietly downgrade the operator's egress -//! boundary to direct dialing or unauthenticated proxy access. Validation -//! semantics are shared with the compute driver via -//! [`openshell_core::driver_utils::parse_upstream_proxy_url`]. +//! any present-but-invalid reserved value — a present-but-empty variable, +//! an unsupported (`https://`, SOCKS) or malformed proxy URL, an unreadable +//! auth file, or a malformed credential — is a fatal startup error rather +//! than being silently ignored, so a typo can never quietly downgrade the +//! operator's egress boundary to direct dialing or unauthenticated proxy +//! access. Validation semantics are shared with the compute driver via +//! [`openshell_core::driver_utils::parse_upstream_proxy_url`] and +//! [`openshell_core::driver_utils::parse_upstream_proxy_credential`]. //! - Policy evaluation, DNS resolution, and SSRF checks run exactly as in the //! direct-dial path; the corporate proxy only replaces the final TCP dial. //! - The reserved `NO_PROXY` list decides which destinations bypass the @@ -184,8 +185,7 @@ impl UpstreamProxyConfig { /// [`UPSTREAM_HTTP_PROXY`](openshell_core::sandbox_env::UPSTREAM_HTTP_PROXY), /// [`UPSTREAM_NO_PROXY`](openshell_core::sandbox_env::UPSTREAM_NO_PROXY), /// [`UPSTREAM_PROXY_AUTH_FILE`](openshell_core::sandbox_env::UPSTREAM_PROXY_AUTH_FILE)). - /// Returns `Ok(None)` when no proxy is configured (unset or empty - /// variables). + /// Returns `Ok(None)` when no proxy is configured (unset variables). /// /// The conventional `HTTPS_PROXY` / `HTTP_PROXY` / `ALL_PROXY` / `NO_PROXY` /// variables are intentionally ignored here: they are set by the sandbox @@ -199,11 +199,12 @@ impl UpstreamProxyConfig { /// /// These reserved variables are an operator-owned security boundary, so /// any present-but-invalid value is fatal instead of being treated as - /// unset: an invalid or unsupported proxy URL, an auth file that is set - /// but unreadable or holds a malformed credential, or an auth file with - /// no proxy configured. Failing closed here prevents a misconfiguration + /// unset: a present-but-empty (or whitespace-only) reserved variable, an + /// invalid or unsupported proxy URL, an auth file that is set but + /// unreadable or holds a malformed credential, or an auth file with no + /// proxy configured. Failing closed here prevents a misconfiguration /// from silently degrading to direct dialing or unauthenticated proxy - /// access. + /// access. Only fully unset variables mean "no proxy". pub fn from_env() -> Result, String> { Self::from_lookup(|name| std::env::var(name).ok()) } @@ -212,14 +213,27 @@ impl UpstreamProxyConfig { use openshell_core::sandbox_env::{ UPSTREAM_HTTP_PROXY, UPSTREAM_HTTPS_PROXY, UPSTREAM_NO_PROXY, UPSTREAM_PROXY_AUTH_FILE, }; - let var = |name: &str| lookup(name).filter(|v| !v.trim().is_empty()); - let https = var(UPSTREAM_HTTPS_PROXY) + // Only a fully unset reserved variable means "not configured". A + // present-but-empty value is a misconfiguration (the compute driver + // never writes one), so it is fatal rather than silently downgrading + // the boundary to direct dialing or unauthenticated proxy access. + let var = |name: &str| -> Result, String> { + match lookup(name) { + None => Ok(None), + Some(value) if value.trim().is_empty() => Err(format!( + "{name} is set but empty; unset it to disable the upstream proxy" + )), + Some(value) => Ok(Some(value)), + } + }; + let https = var(UPSTREAM_HTTPS_PROXY)? .map(|url| parse_proxy_url(&url, UPSTREAM_HTTPS_PROXY)) .transpose()?; - let http = var(UPSTREAM_HTTP_PROXY) + let http = var(UPSTREAM_HTTP_PROXY)? .map(|url| parse_proxy_url(&url, UPSTREAM_HTTP_PROXY)) .transpose()?; - let auth_file = var(UPSTREAM_PROXY_AUTH_FILE); + let auth_file = var(UPSTREAM_PROXY_AUTH_FILE)?; + let no_proxy_list = var(UPSTREAM_NO_PROXY)?; if https.is_none() && http.is_none() { if auth_file.is_some() { return Err(format!( @@ -228,7 +242,7 @@ impl UpstreamProxyConfig { } return Ok(None); } - let no_proxy = NoProxy::parse(&var(UPSTREAM_NO_PROXY).unwrap_or_default()); + let no_proxy = NoProxy::parse(&no_proxy_list.unwrap_or_default()); let mut config = Self { https, http, @@ -317,22 +331,21 @@ fn parse_proxy_url(raw: &str, var_name: &str) -> Result { /// Build a `Proxy-Authorization: Basic ` header value from a raw /// `user:pass` credential. /// -/// The credential is used verbatim: it is delivered through a trusted -/// operator file, not a URL, so there is no percent-encoding to decode. +/// The credential is used verbatim after trimming: it is delivered through a +/// trusted operator file, not a URL, so there is no percent-encoding to +/// decode. Validation is shared with the compute driver through +/// [`parse_upstream_proxy_credential`](openshell_core::driver_utils::parse_upstream_proxy_credential), +/// so a credential the driver staged at sandbox-create time is never rejected +/// here. /// /// # Errors /// -/// Rejects an empty credential and one containing control characters (CR, LF, -/// NUL) that could inject additional HTTP headers. Error messages never -/// include the credential content. +/// Rejects an empty credential, one containing control characters that could +/// inject additional HTTP headers, and one not in `user:pass` form. Error +/// messages never include the credential content. fn basic_auth_header(credential: &str) -> Result { - let credential = credential.trim(); - if credential.is_empty() { - return Err("credential is empty".to_string()); - } - if credential.contains(|c: char| c.is_control()) { - return Err("credential contains control characters".to_string()); - } + let credential = openshell_core::driver_utils::parse_upstream_proxy_credential(credential) + .map_err(|err| err.to_string())?; Ok(format!( "Basic {}", base64::engine::general_purpose::STANDARD.encode(credential) @@ -494,12 +507,21 @@ mod tests { } #[test] - fn empty_values_yield_none() { - assert!( - config_from(&[(HTTPS_PROXY, " "), (HTTP_PROXY, "")]) - .unwrap() - .is_none() - ); + fn present_but_empty_values_are_fatal() { + // A reserved variable the operator did not set is absent, never + // empty: the driver only writes configured values. A present-but + // -blank value is therefore a misconfiguration and must not silently + // mean "no proxy". + for (name, value) in [ + (HTTPS_PROXY, ""), + (HTTP_PROXY, " "), + (NO_PROXY, " "), + (PROXY_AUTH_FILE, ""), + ] { + let err = config_from(&[(name, value)]).unwrap_err(); + assert!(err.contains(name), "{err}"); + assert!(err.contains("empty"), "{err}"); + } } #[test] @@ -576,7 +598,9 @@ mod tests { #[test] fn malformed_auth_file_credential_is_fatal() { - for credential in [" ", "user:pa\r\nss"] { + // Empty, header-injecting, and non-`user:pass` credentials are all + // rejected by the parser shared with the compute driver. + for credential in [" ", "user:pa\r\nss", "userpass", ":pass"] { let file = tempfile::NamedTempFile::new().unwrap(); std::fs::write(file.path(), credential).unwrap(); let path = file.path().to_str().unwrap().to_string(); @@ -621,7 +645,7 @@ mod tests { } #[test] - fn basic_auth_header_encodes_and_rejects_control_chars() { + fn basic_auth_header_encodes_and_rejects_malformed_credentials() { assert_eq!( basic_auth_header("user:p@ss").as_deref(), Ok(format!( @@ -633,6 +657,7 @@ mod tests { assert!(basic_auth_header(" ").is_err()); assert!(basic_auth_header("user:pa\r\nss").is_err()); assert!(basic_auth_header("user:pa\nInjected: header").is_err()); + assert!(basic_auth_header("no-separator").is_err()); } #[test] @@ -713,7 +738,8 @@ mod tests { #[test] fn loopback_and_localhost_always_bypass() { - let cfg = no_proxy_cfg(""); + // No NO_PROXY at all: loopback still bypasses unconditionally. + let cfg = config_ok(&[(HTTPS_PROXY, "http://proxy:8080")]); assert!(bypasses(&cfg, "localhost")); assert!(bypasses(&cfg, "127.0.0.1")); assert!(bypasses(&cfg, "::1")); diff --git a/docs/reference/gateway-config.mdx b/docs/reference/gateway-config.mdx index f91c1e5f83..5f3a2bef35 100644 --- a/docs/reference/gateway-config.mdx +++ b/docs/reference/gateway-config.mdx @@ -371,6 +371,9 @@ health_check_interval_secs = 10 # rejected at startup, since it would be stored here and exposed in container # metadata). Instead point proxy_auth_file at a file containing "user:pass"; # the gateway delivers it to the supervisor through a root-only secret mount. +# The credential must use the user:pass form (non-empty user, no control +# characters); the same validation runs at sandbox-create time and in the +# supervisor, so a credential accepted here is never rejected in-container. # Keep gateway.toml and the auth file owner-readable only (mode 0600). # https_proxy = "http://proxy.corp.com:8080" # http_proxy = "http://proxy.corp.com:8080" From d08f42d9e6366200caf2e07996ede8581c316215 Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Thu, 16 Jul 2026 16:19:10 +0200 Subject: [PATCH 6/7] fix(sandbox,podman): close remaining fail-open upstream proxy config paths Two configuration paths could still silently run without the proxy boundary the operator believed was in effect. A no_proxy bypass list configured without any https_proxy/http_proxy was accepted by both the driver and the supervisor and simply meant "dial everything directly". Reject it on both sides, exactly like the existing proxy_auth_file-without-proxy rule: an operator who wrote a bypass list assumed proxying was active, so accepting it hides a fail-open state. The gateway.sh dev script guarded proxy settings with [[ -n "${VAR:-}" ]], which conflates unset with explicitly-empty and dropped the latter before the gateway's validation could see it. Use ${VAR+x} instead so a set-but-empty variable is written into gateway.toml and rejected at startup by validate_proxy_config rather than silently discarded. Addresses the remaining fail-open configuration review item on #2245. Signed-off-by: Philippe Martin --- architecture/sandbox.md | 9 ++--- crates/openshell-driver-podman/src/config.rs | 32 ++++++++++++----- .../src/upstream_proxy.rs | 34 +++++++++++++------ docs/reference/gateway-config.mdx | 8 +++-- tasks/scripts/gateway.sh | 12 ++++--- 5 files changed, 66 insertions(+), 29 deletions(-) diff --git a/architecture/sandbox.md b/architecture/sandbox.md index 1dbf91149d..08425b9a85 100644 --- a/architecture/sandbox.md +++ b/architecture/sandbox.md @@ -95,10 +95,11 @@ policy proxy. The configuration is fail-closed: a reserved variable that is present but invalid — a present-but-empty value, an unsupported or malformed proxy URL, an -unreadable auth file, or a malformed credential — is fatal to supervisor -startup instead of being treated as unset, so a misconfiguration can never -silently degrade to direct dialing or unauthenticated proxy access. Only a -fully unset variable means "no proxy". The driver validates the same rules at +unreadable auth file, a malformed credential, or an auth file or `NO_PROXY` +list set while no proxy URL is configured — is fatal to supervisor startup +instead of being treated as unset, so a misconfiguration can never silently +degrade to direct dialing or unauthenticated proxy access. Only a fully unset +variable means "no proxy". The driver validates the same rules at sandbox-create time through validators shared with the supervisor (`openshell_core::driver_utils::parse_upstream_proxy_url` and `parse_upstream_proxy_credential`). diff --git a/crates/openshell-driver-podman/src/config.rs b/crates/openshell-driver-podman/src/config.rs index e205815fdf..23b905053b 100644 --- a/crates/openshell-driver-podman/src/config.rs +++ b/crates/openshell-driver-podman/src/config.rs @@ -252,14 +252,20 @@ impl PodmanComputeConfig { // The supervisor treats a present-but-empty reserved variable as a // fatal misconfiguration, so never accept (and later inject) one. - if self - .no_proxy - .as_deref() - .is_some_and(|list| list.trim().is_empty()) - { - return Err(crate::client::PodmanApiError::InvalidInput( - "no_proxy must not be empty when set; omit it instead".to_string(), - )); + if let Some(list) = self.no_proxy.as_deref() { + if list.trim().is_empty() { + return Err(crate::client::PodmanApiError::InvalidInput( + "no_proxy must not be empty when set; omit it instead".to_string(), + )); + } + // A bypass list only makes sense relative to a proxy boundary. An + // operator who set one believed proxying was in effect, so accepting + // it while all egress dials directly would hide a fail-open state. + if self.https_proxy.is_none() && self.http_proxy.is_none() { + return Err(crate::client::PodmanApiError::InvalidInput( + "no_proxy is set but no https_proxy/http_proxy is configured".to_string(), + )); + } } if let Some(path) = self.proxy_auth_file.as_deref() { @@ -549,6 +555,16 @@ mod tests { assert!(err.to_string().contains("no_proxy"), "{err}"); } + #[test] + fn validate_proxy_config_rejects_no_proxy_without_proxy() { + let cfg = PodmanComputeConfig { + no_proxy: Some("*.svc.cluster.local".to_string()), + ..PodmanComputeConfig::default() + }; + let err = cfg.validate_proxy_config().unwrap_err(); + assert!(err.to_string().contains("no_proxy"), "{err}"); + } + #[test] fn validate_proxy_config_rejects_inline_credentials() { for url in [ diff --git a/crates/openshell-supervisor-network/src/upstream_proxy.rs b/crates/openshell-supervisor-network/src/upstream_proxy.rs index 2fdfb51150..2b3bd4f01d 100644 --- a/crates/openshell-supervisor-network/src/upstream_proxy.rs +++ b/crates/openshell-supervisor-network/src/upstream_proxy.rs @@ -201,8 +201,9 @@ impl UpstreamProxyConfig { /// any present-but-invalid value is fatal instead of being treated as /// unset: a present-but-empty (or whitespace-only) reserved variable, an /// invalid or unsupported proxy URL, an auth file that is set but - /// unreadable or holds a malformed credential, or an auth file with no - /// proxy configured. Failing closed here prevents a misconfiguration + /// unreadable or holds a malformed credential, or an auth file or + /// `NO_PROXY` list with no proxy configured. Failing closed here + /// prevents a misconfiguration /// from silently degrading to direct dialing or unauthenticated proxy /// access. Only fully unset variables mean "no proxy". pub fn from_env() -> Result, String> { @@ -235,10 +236,16 @@ impl UpstreamProxyConfig { let auth_file = var(UPSTREAM_PROXY_AUTH_FILE)?; let no_proxy_list = var(UPSTREAM_NO_PROXY)?; if https.is_none() && http.is_none() { - if auth_file.is_some() { - return Err(format!( - "{UPSTREAM_PROXY_AUTH_FILE} is set but no upstream proxy is configured" - )); + // Auxiliary proxy settings without a proxy mean the operator + // believed a proxy boundary was in effect; refuse rather than + // silently running with direct egress. + for (name, value) in [ + (UPSTREAM_PROXY_AUTH_FILE, &auth_file), + (UPSTREAM_NO_PROXY, &no_proxy_list), + ] { + if value.is_some() { + return Err(format!("{name} is set but no upstream proxy is configured")); + } } return Ok(None); } @@ -618,10 +625,17 @@ mod tests { } #[test] - fn auth_file_without_proxy_is_fatal() { - let err = - config_from(&[(PROXY_AUTH_FILE, "/etc/openshell/auth/upstream-proxy")]).unwrap_err(); - assert!(err.contains(PROXY_AUTH_FILE), "{err}"); + fn auxiliary_settings_without_proxy_are_fatal() { + // An auth file or NO_PROXY list only makes sense relative to a proxy + // boundary the operator believed was in effect. + for (name, value) in [ + (PROXY_AUTH_FILE, "/etc/openshell/auth/upstream-proxy"), + (NO_PROXY, "*.svc.cluster.local"), + ] { + let err = config_from(&[(name, value)]).unwrap_err(); + assert!(err.contains(name), "{err}"); + assert!(err.contains("no upstream proxy"), "{err}"); + } } #[test] diff --git a/docs/reference/gateway-config.mdx b/docs/reference/gateway-config.mdx index 5f3a2bef35..bceaa2eb68 100644 --- a/docs/reference/gateway-config.mdx +++ b/docs/reference/gateway-config.mdx @@ -363,9 +363,11 @@ health_check_interval_secs = 10 # HTTPS_PROXY/HTTP_PROXY/NO_PROXY variables a sandbox sets do not affect it. # # Configuration is fail-closed: an invalid proxy URL is rejected at gateway -# startup, and a set-but-invalid value reaching a sandbox (for example an -# unreadable or malformed auth file) is fatal to that sandbox's supervisor -# instead of silently falling back to direct or unauthenticated egress. +# startup, setting no_proxy or proxy_auth_file without a proxy URL is +# rejected as well, and a set-but-invalid value reaching a sandbox (for +# example an unreadable or malformed auth file) is fatal to that sandbox's +# supervisor instead of silently falling back to direct or unauthenticated +# egress. # # Credentials must NOT be embedded in the URL (an inline user:pass@ is # rejected at startup, since it would be stored here and exposed in container diff --git a/tasks/scripts/gateway.sh b/tasks/scripts/gateway.sh index 92e67bddd9..2476776a42 100644 --- a/tasks/scripts/gateway.sh +++ b/tasks/scripts/gateway.sh @@ -355,16 +355,20 @@ EOF if [[ -n "${GRPC_ENDPOINT}" ]]; then printf 'grpc_endpoint = "%s"\n' "${GRPC_ENDPOINT}" >>"${CONFIG_PATH}" fi - if [[ -n "${OPENSHELL_SANDBOX_HTTPS_PROXY:-}" ]]; then + # ${VAR+x} distinguishes unset from set-but-empty: an unset variable + # writes nothing, but an explicitly empty one is written through so the + # gateway's fail-closed proxy validation rejects it at startup instead of + # this script silently dropping it. + if [[ -n "${OPENSHELL_SANDBOX_HTTPS_PROXY+x}" ]]; then printf 'https_proxy = "%s"\n' "${OPENSHELL_SANDBOX_HTTPS_PROXY}" >>"${CONFIG_PATH}" fi - if [[ -n "${OPENSHELL_SANDBOX_HTTP_PROXY:-}" ]]; then + if [[ -n "${OPENSHELL_SANDBOX_HTTP_PROXY+x}" ]]; then printf 'http_proxy = "%s"\n' "${OPENSHELL_SANDBOX_HTTP_PROXY}" >>"${CONFIG_PATH}" fi - if [[ -n "${OPENSHELL_SANDBOX_NO_PROXY:-}" ]]; then + if [[ -n "${OPENSHELL_SANDBOX_NO_PROXY+x}" ]]; then printf 'no_proxy = "%s"\n' "${OPENSHELL_SANDBOX_NO_PROXY}" >>"${CONFIG_PATH}" fi - if [[ -n "${OPENSHELL_SANDBOX_PROXY_AUTH_FILE:-}" ]]; then + if [[ -n "${OPENSHELL_SANDBOX_PROXY_AUTH_FILE+x}" ]]; then printf 'proxy_auth_file = "%s"\n' "${OPENSHELL_SANDBOX_PROXY_AUTH_FILE}" >>"${CONFIG_PATH}" fi ;; From 0dc7030305877e1bd9ebe482b0ae5979ace04c85 Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Thu, 16 Jul 2026 18:50:23 +0200 Subject: [PATCH 7/7] fix(sandbox,podman): reject upstream proxy URLs with path, query, or fragment parse_upstream_proxy_url accepted URLs like http://proxy.corp.com:8080/some/path and silently discarded everything after host:port, a lenience inherited from the original supervisor parser. A forward proxy is addressed by host:port only, so extra components indicate a misconfiguration (for example a pasted endpoint URL) and silently truncating them violates the present-but-invalid-is-fatal contract enforced everywhere else in this configuration surface. Reject a path, query, or fragment in the shared validator with a new UnexpectedComponent error. A bare trailing slash remains accepted because the url crate normalizes an absent http path to "/", making the two indistinguishable. Both the Podman driver (gateway startup) and the supervisor (sandbox startup) inherit the rule through the shared parser, keeping their semantics identical by construction. Addresses the proxy URL component review item on #2245. Signed-off-by: Philippe Martin --- architecture/sandbox.md | 3 +- crates/openshell-core/src/driver_utils.rs | 42 ++++++++++++++++++- crates/openshell-driver-podman/src/config.rs | 19 +++++++++ .../src/upstream_proxy.rs | 10 +++++ docs/reference/gateway-config.mdx | 6 ++- 5 files changed, 76 insertions(+), 4 deletions(-) diff --git a/architecture/sandbox.md b/architecture/sandbox.md index 08425b9a85..4036888ce6 100644 --- a/architecture/sandbox.md +++ b/architecture/sandbox.md @@ -87,7 +87,8 @@ compute drivers write in their required-variable tier; sandbox and template environment cannot override them. The conventional `HTTPS_PROXY`/`HTTP_PROXY`/ `NO_PROXY` variables a sandbox controls are ignored on this path. Reserved `NO_PROXY` destinations, loopback, and host-gateway aliases always dial -directly. Only `http://` proxy URLs are supported. Local DNS resolution and +directly. Only `http://` proxy URLs in `scheme://host:port` form are +supported; a path, query, or fragment is rejected. Local DNS resolution and SSRF validation still run before the proxied dial; the CONNECT target sent to the corporate proxy is the requested hostname. The workload child's proxy variables are unaffected — they are always rewritten to point at the local diff --git a/crates/openshell-core/src/driver_utils.rs b/crates/openshell-core/src/driver_utils.rs index dd322fb80c..31f1ccb848 100644 --- a/crates/openshell-core/src/driver_utils.rs +++ b/crates/openshell-core/src/driver_utils.rs @@ -122,13 +122,21 @@ pub enum UpstreamProxyUrlError { /// The URL has no host component. #[error("proxy URL is missing a proxy host")] MissingHost, + /// The URL carries a path, query, or fragment. A forward proxy is + /// addressed by `host:port` only, so extra components indicate a + /// misconfiguration (e.g. a pasted endpoint URL) and are rejected instead + /// of being silently discarded. + #[error("proxy URL must not contain a {0}; use scheme://host:port only")] + UnexpectedComponent(&'static str), } /// Parse and validate a corporate upstream-proxy URL. /// /// A bare `host:port` (no `://`) is normalized to `http://host:port`. Only /// `http://` proxies are accepted, inline userinfo is rejected, and the port -/// defaults to 80. +/// defaults to 80. The URL must address the proxy only: a path (other than +/// a bare trailing `/`), query, or fragment is rejected rather than silently +/// discarded. /// /// # Errors /// @@ -164,6 +172,17 @@ pub fn parse_upstream_proxy_url(raw: &str) -> Result