diff --git a/crates/openshell-core/src/secrets.rs b/crates/openshell-core/src/secrets.rs index d71a2139c6..e93bdc5390 100644 --- a/crates/openshell-core/src/secrets.rs +++ b/crates/openshell-core/src/secrets.rs @@ -929,6 +929,20 @@ pub fn rewrite_http_header_block( resolver: Option<&SecretResolver>, ) -> Result { let Some(resolver) = resolver else { + // Fail-closed: with no resolver there is nothing that can rewrite a + // credential placeholder, so forwarding the block verbatim would leak + // the reserved token to the upstream. Scan the header region (mirroring + // the resolved-path scan below) and reject if any reserved marker is + // present. Marker-free traffic still passes through unchanged, which is + // the intended behavior when the endpoint injects no credentials. + let scan_end = raw + .windows(4) + .position(|w| w == b"\r\n\r\n") + .map_or(raw.len(), |p| raw.len().min(p + 4 + 256)); + let header_region = String::from_utf8_lossy(&raw[..scan_end]); + if contains_reserved_credential_marker(&header_region) { + return Err(UnresolvedPlaceholderError { location: "header" }); + } return Ok(RewriteResult { rewritten: raw.to_vec(), redacted_target: None, @@ -1889,11 +1903,44 @@ mod tests { } #[test] - fn no_resolver_passes_through_without_scanning() { - // Even if placeholders are present, None resolver means no scanning + fn no_resolver_with_placeholder_in_request_line_fails_closed() { + // Fail-closed: a placeholder in the request line with no resolver must + // never be forwarded verbatim — it would leak the reserved token. let raw = b"GET /api/openshell:resolve:env:KEY HTTP/1.1\r\nHost: x\r\n\r\n"; - let result = rewrite_http_header_block(raw, None).expect("should succeed"); - assert_eq!(raw.as_slice(), result.rewritten.as_slice()); + let err = rewrite_http_header_block(raw, None) + .expect_err("placeholder without resolver must fail closed"); + assert_eq!(err.location, "header"); + } + + #[test] + fn no_resolver_with_placeholder_in_header_value_fails_closed() { + let raw = + b"GET / HTTP/1.1\r\nAuthorization: Bearer openshell:resolve:env:KEY\r\nHost: x\r\n\r\n"; + let err = rewrite_http_header_block(raw, None) + .expect_err("placeholder without resolver must fail closed"); + assert_eq!(err.location, "header"); + } + + #[test] + fn no_resolver_with_percent_encoded_marker_fails_closed() { + // Percent-encoded form of the canonical marker: a client (or an evasive + // sender) can URL-encode the reserved token, and the no-resolver scan + // must still catch it via the percent-decoded pass. + let raw = b"GET /api/openshell%3Aresolve%3Aenv%3AKEY HTTP/1.1\r\nHost: x\r\n\r\n"; + let err = rewrite_http_header_block(raw, None) + .expect_err("percent-encoded placeholder without resolver must fail closed"); + assert_eq!(err.location, "header"); + } + + #[test] + fn no_resolver_with_provider_alias_marker_fails_closed() { + // The provider-shaped alias marker is also a reserved credential token + // and must fail closed when no resolver can rewrite it. + let raw = + b"GET / HTTP/1.1\r\nX-Api-Key: OPENSHELL-RESOLVE-ENV-CUSTOM_TOKEN\r\nHost: x\r\n\r\n"; + let err = rewrite_http_header_block(raw, None) + .expect_err("alias marker without resolver must fail closed"); + assert_eq!(err.location, "header"); } #[test] diff --git a/crates/openshell-supervisor-network/src/l7/rest.rs b/crates/openshell-supervisor-network/src/l7/rest.rs index 0558a67e55..5c62f6f9ee 100644 --- a/crates/openshell-supervisor-network/src/l7/rest.rs +++ b/crates/openshell-supervisor-network/src/l7/rest.rs @@ -4857,12 +4857,15 @@ mod tests { assert!(forwarded.contains("Host: integrate.api.nvidia.com\r\n")); } - /// Verifies that without a `SecretResolver` (i.e. the L4-only raw tunnel - /// path, or no TLS termination), credential placeholders pass through - /// unmodified. This documents the behavior that causes 401 errors when - /// `tls: terminate` is missing from the endpoint config. + /// Verifies that when a request carries a credential placeholder but no + /// `SecretResolver` is available to rewrite it, the relay fails closed + /// instead of forwarding the reserved token verbatim. A `None` resolver + /// here stands in for a degraded internal state (e.g. secrets could not be + /// wired up); forwarding the placeholder would leak it to the upstream and + /// cause 401s. The fail-closed scan in `rewrite_http_header_block` rejects + /// the request before any byte reaches upstream. #[tokio::test] - async fn relay_request_without_resolver_leaks_placeholders() { + async fn relay_request_without_resolver_fails_closed_on_placeholder() { let (child_env, _resolver) = SecretResolver::from_provider_env( [("NVIDIA_API_KEY".to_string(), "nvapi-secret".to_string())] .into_iter() @@ -4887,55 +4890,39 @@ mod tests { body_length: BodyLength::ContentLength(2), }; - let upstream_task = tokio::spawn(async move { - let mut buf = vec![0u8; 4096]; - let mut total = 0; - loop { - let n = upstream_side.read(&mut buf[total..]).await.unwrap(); - if n == 0 { - break; - } - total += n; - if let Some(hdr_end) = buf[..total].windows(4).position(|w| w == b"\r\n\r\n") { - if total >= hdr_end + 4 + 2 { - break; - } - } - } - upstream_side - .write_all(b"HTTP/1.1 200 OK\r\nContent-Length: 2\r\n\r\nok") - .await - .unwrap(); - upstream_side.flush().await.unwrap(); - String::from_utf8_lossy(&buf[..total]).to_string() - }); - - // Pass `None` for the resolver — simulates the L4 path where no - // rewriting occurs. + // Pass `None` for the resolver — simulates a degraded state where no + // rewriting can occur. The relay must reject the request. let relay = tokio::time::timeout( std::time::Duration::from_secs(5), relay_http_request_with_resolver( &req, &mut proxy_to_client, &mut proxy_to_upstream, - None, // <-- No resolver, as in the L4 raw tunnel path + None, ), ) .await .expect("relay must not deadlock"); - relay.expect("relay should succeed"); - - let forwarded = upstream_task.await.expect("upstream task should complete"); - // Without a resolver, the placeholder LEAKS to upstream — this is the - // documented behavior that causes 401s when `tls: terminate` is missing. assert!( - forwarded.contains("openshell:resolve:env:NVIDIA_API_KEY"), - "Expected placeholder to leak without resolver, got: {forwarded}" + relay.is_err(), + "relay must fail closed when a placeholder cannot be resolved" ); + + // The fail-closed scan rejects the request before any byte reaches + // upstream, so nothing at all must have been forwarded — a strictly + // stronger invariant than "the placeholder/secret did not appear". Drop + // the proxy write half so the read observes EOF instead of blocking. + drop(proxy_to_upstream); + let mut forwarded = Vec::new(); + upstream_side + .read_to_end(&mut forwarded) + .await + .expect("upstream read should complete"); assert!( - !forwarded.contains("nvapi-secret"), - "Real secret should NOT appear without resolver, got: {forwarded}" + forwarded.is_empty(), + "no bytes may reach upstream when the placeholder cannot be resolved; got: {}", + String::from_utf8_lossy(&forwarded) ); } diff --git a/crates/openshell-supervisor-network/src/proxy.rs b/crates/openshell-supervisor-network/src/proxy.rs index 0d2c8c0258..d7dfe30e1a 100644 --- a/crates/openshell-supervisor-network/src/proxy.rs +++ b/crates/openshell-supervisor-network/src/proxy.rs @@ -741,6 +741,15 @@ async fn handle_tcp_connection( return Ok(()); } + // Resolve the route's TLS treatment up front. `query_tls_mode` reads only + // the policy decision + host/port (no peeked bytes), so it is valid before + // the `200`. The fail-closed refusal that consumes it runs after the SSRF/ + // allowed_ips validation below — so an internal-address CONNECT still gets + // the SSRF 403 and telemetry in degraded state — but before the upstream + // connect and before `200 Connection Established`. + let effective_tls_skip = + query_tls_mode(&opa_engine, &decision, &host_lc, port) == crate::l7::TlsMode::Skip; + let sandbox_entrypoint_pid = entrypoint_pid.load(Ordering::Acquire); // Query allowed_ips from the matched endpoint config (if any). @@ -762,7 +771,7 @@ async fn handle_tcp_connection( // The "non-empty" branch is the explicit-allowlist path; reading it first // matches the policy decision narrative. #[allow(clippy::if_not_else)] - let mut upstream = if is_host_gateway_alias(&host_lc) + let validated_addrs = if is_host_gateway_alias(&host_lc) && let Some(gw) = *trusted_host_gateway { // Trusted host-gateway path. The compute driver injected this hostname @@ -771,9 +780,7 @@ async fn handle_tcp_connection( // addresses (used by rootless Podman with pasta) are not hard-blocked. // Cloud metadata IPs and control-plane ports are still rejected. match resolve_and_check_trusted_gateway(&host, port, gw, sandbox_entrypoint_pid).await { - Ok(addrs) => TcpStream::connect(addrs.as_slice()) - .await - .into_diagnostic()?, + Ok(addrs) => addrs, Err(reason) => { { let event = NetworkActivityBuilder::new(openshell_ocsf::ctx::ctx()) @@ -827,9 +834,7 @@ async fn handle_tcp_connection( match resolve_and_check_allowed_ips(&host, port, &nets, sandbox_entrypoint_pid) .await { - Ok(addrs) => TcpStream::connect(addrs.as_slice()) - .await - .into_diagnostic()?, + Ok(addrs) => addrs, Err(reason) => { { let event = NetworkActivityBuilder::new(openshell_ocsf::ctx::ctx()) @@ -929,9 +934,7 @@ async fn handle_tcp_connection( // the resolved IP in allowed_ips. Always-blocked addresses and // control-plane ports remain denied. match resolve_and_check_declared_endpoint(&host, port, sandbox_entrypoint_pid).await { - Ok(addrs) => TcpStream::connect(addrs.as_slice()) - .await - .into_diagnostic()?, + Ok(addrs) => addrs, Err(reason) => { { let event = NetworkActivityBuilder::new(openshell_ocsf::ctx::ctx()) @@ -981,9 +984,7 @@ async fn handle_tcp_connection( } else { // Default: reject all internal IPs (loopback, RFC 1918, link-local). match resolve_and_reject_internal(&host, port, sandbox_entrypoint_pid).await { - Ok(addrs) => TcpStream::connect(addrs.as_slice()) - .await - .into_diagnostic()?, + Ok(addrs) => addrs, Err(reason) => { { let event = NetworkActivityBuilder::new(openshell_ocsf::ctx::ctx()) @@ -1031,6 +1032,52 @@ async fn handle_tcp_connection( } }; + // Fail closed AFTER SSRF/allowed_ips validation (an internal-address CONNECT + // has already returned the SSRF 403 above) but BEFORE connecting upstream + // and BEFORE `200 Connection Established`. A terminating route with no TLS + // termination state cannot rewrite credential placeholders; the 503 must be + // the first bytes on the socket rather than a post-200 in-tunnel write the + // client would misread as a TLS error. No "allowed CONNECT" event is emitted + // for this path — that log lives after the `200` below. + if refuse_connect_when_tls_unavailable(&mut client, tls_state.is_some(), effective_tls_skip) + .await? + { + let event = NetworkActivityBuilder::new(openshell_ocsf::ctx::ctx()) + .activity(ActivityId::Open) + .action(ActionId::Denied) + .disposition(DispositionId::Blocked) + .severity(SeverityId::High) + .status(StatusId::Failure) + .dst_endpoint(Endpoint::from_domain(&host_lc, port)) + .src_endpoint_addr(peer_addr.ip(), peer_addr.port()) + .actor_process( + Process::from_bypass(&binary_str, &pid_str, &ancestors_str) + .with_cmd_line(&cmdline_str), + ) + .firewall_rule(policy_str, "tls") + .message(format!( + "CONNECT refused for {host_lc}:{port}: {TLS_TERMINATION_UNAVAILABLE_DETAIL}" + )) + .status_detail(TLS_TERMINATION_UNAVAILABLE_DETAIL) + .build(); + ocsf_emit!(event); + emit_activity_simple(activity_tx.as_ref(), true, "tls_termination_unavailable"); + emit_denial( + &denial_tx, + &host_lc, + port, + &binary_str, + &decision, + TLS_TERMINATION_UNAVAILABLE_DETAIL, + "connect-tls-termination-unavailable", + ); + return Ok(()); + } + + let mut upstream = TcpStream::connect(validated_addrs.as_slice()) + .await + .into_diagnostic()?; + debug!( "handle_tcp_connection dns_resolve_and_tcp_connect: {}ms host={host_lc}", dns_connect_start.elapsed().as_millis() @@ -1070,10 +1117,8 @@ async fn handle_tcp_connection( } emit_connect_activity_if_l4_only(&activity_tx, l7_route.as_ref()); - // Determine effective TLS mode. Check the raw endpoint config for - // `tls: skip` independently of L7 config (which requires `protocol`). - let effective_tls_skip = - query_tls_mode(&opa_engine, &decision, &host_lc, port) == crate::l7::TlsMode::Skip; + // `effective_tls_skip` was resolved before the `200` above (the fail-closed + // gate needs it) and drives the raw-tunnel branch below. // Build L7 eval context (shared by TLS-terminated and plaintext paths). let ctx = crate::l7::relay::L7EvalContext { @@ -1207,21 +1252,48 @@ async fn handle_tcp_connection( } } } else { - { - let event = NetworkActivityBuilder::new(openshell_ocsf::ctx::ctx()) - .activity(ActivityId::Fail) - .severity(SeverityId::Low) - .status(StatusId::Failure) - .dst_endpoint(Endpoint::from_domain(&host_lc, port)) - .message(format!( - "TLS detected but TLS state not configured for {host_lc}:{port}, falling back to raw tunnel" - )) - .build(); - ocsf_emit!(event); - } - let _ = tokio::io::copy_bidirectional(&mut client, &mut upstream) - .await - .into_diagnostic()?; + // Defense in depth; unreachable in normal operation. The pre-200 + // fail-closed gate already refuses terminating routes when no TLS + // termination state exists, and `tls: skip` routes raw-tunnel + // before this peek. Reaching here means a future refactor bypassed + // that gate. The `200 Connection Established` was already sent, so + // the tunnel is live: any HTTP bytes now would be decoded as a TLS + // protocol error, so we fail closed by DROPPING the connection + // rather than raw-tunneling (which would forward the client's TLS + // stream upstream and leak any `openshell:resolve:env:*` + // placeholder verbatim). + const DETAIL: &str = "TLS termination unavailable after tunnel establishment; \ + closing connection — credential rewrite would be bypassed"; + let event = NetworkActivityBuilder::new(openshell_ocsf::ctx::ctx()) + .activity(ActivityId::Open) + .action(ActionId::Denied) + .disposition(DispositionId::Blocked) + .severity(SeverityId::High) + .status(StatusId::Failure) + .dst_endpoint(Endpoint::from_domain(&host_lc, port)) + .src_endpoint_addr(peer_addr.ip(), peer_addr.port()) + .actor_process( + Process::from_bypass(&binary_str, &pid_str, &ancestors_str) + .with_cmd_line(&cmdline_str), + ) + .firewall_rule(policy_str, "tls") + .message(format!("CONNECT refused for {host_lc}:{port}: {DETAIL}")) + .status_detail(DETAIL) + .build(); + ocsf_emit!(event); + emit_activity_simple(activity_tx.as_ref(), true, "tls_termination_unavailable"); + emit_denial( + &denial_tx, + &host_lc, + port, + &binary_str, + &decision, + DETAIL, + "connect-tls-termination-unavailable", + ); + // No HTTP response: the tunnel is already established, so writing + // bytes here would corrupt the client's TLS handshake. Drop instead. + return Ok(()); } } else if tunnel_protocol == TunnelProtocol::Http1 { // Plaintext HTTP detected. @@ -4347,6 +4419,50 @@ fn build_json_error_response(status: u16, status_text: &str, error: &str, detail .into_bytes() } +/// Detail shared by the fail-closed 503 body, the OCSF denial event, and the +/// denial notification when a terminating CONNECT route has no TLS termination +/// state available. +const TLS_TERMINATION_UNAVAILABLE_DETAIL: &str = "TLS termination unavailable (CA initialization failed); \ + refusing to tunnel — credential rewrite would be bypassed"; + +/// Fail-closed gate evaluated BEFORE `200 Connection Established`. +/// +/// A CONNECT route that terminates TLS (`tls: skip` is exempt) relies on the +/// ephemeral CA to rewrite credential placeholders. When no TLS termination +/// state exists — the CA failed to generate or write at startup (`run.rs`); +/// `mode != Proxy` never starts this handler — the proxy cannot rewrite those +/// placeholders, so raw-tunneling would forward the client's TLS stream +/// straight upstream and leak any `openshell:resolve:env:*` placeholder +/// verbatim. +/// +/// The refusal is written here, as the first bytes on the socket, because a +/// CONNECT client only sends its TLS `ClientHello` after reading the `200`. +/// Refusing after the `200` would land the 503 inside the established tunnel, +/// where the client decodes it as a TLS protocol error rather than a readable +/// HTTP status (the flaw this replaces). Returns `true` when the connection was +/// refused (the caller must stop) and `false` when the caller should proceed to +/// establish the tunnel. +async fn refuse_connect_when_tls_unavailable( + client: &mut TcpStream, + tls_state_present: bool, + effective_tls_skip: bool, +) -> Result { + if tls_state_present || effective_tls_skip { + return Ok(false); + } + respond( + client, + &build_json_error_response( + 503, + "Service Unavailable", + "tls_termination_unavailable", + TLS_TERMINATION_UNAVAILABLE_DETAIL, + ), + ) + .await?; + Ok(true) +} + /// Check if a miette error represents a benign connection close. /// /// TLS handshake EOF, missing `close_notify`, connection resets, and broken @@ -7764,6 +7880,367 @@ network_policies: assert_eq!(body["detail"], "connection to api.example.com:443 failed"); } + /// Locks the fail-closed response the CONNECT handler sends when TLS is + /// detected but no termination state exists (ephemeral CA setup failed). + /// The proxy must refuse the connection with a 503 instead of raw-tunneling + /// the TLS stream, which would bypass credential rewrite and leak + /// placeholders verbatim. + #[test] + fn test_json_error_response_503_tls_termination_unavailable() { + let detail = TLS_TERMINATION_UNAVAILABLE_DETAIL; + let resp = build_json_error_response( + 503, + "Service Unavailable", + "tls_termination_unavailable", + detail, + ); + let resp_str = String::from_utf8(resp).unwrap(); + + assert!(resp_str.starts_with("HTTP/1.1 503 Service Unavailable\r\n")); + assert!(resp_str.contains("Connection: close\r\n")); + + let body_start = resp_str.find("\r\n\r\n").unwrap() + 4; + let body: serde_json::Value = serde_json::from_str(&resp_str[body_start..]).unwrap(); + assert_eq!(body["error"], "tls_termination_unavailable"); + assert_eq!(body["detail"], detail); + } + + /// Connection-level regression for the pre-200 fail-closed gate. A real + /// loopback client opens a connection and the proxy-side gate runs with no + /// TLS termination state for a terminating (non-`tls: skip`) route. The + /// FIRST bytes the client reads back must be a real `HTTP/1.1 503`, not the + /// `HTTP/1.1 200 Connection Established` that would establish the tunnel and + /// bury the refusal inside it as a TLS protocol error (PR #2162 gator flaw). + #[tokio::test] + async fn connect_without_tls_termination_refused_with_503_before_200() { + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + let mut client = TcpStream::connect(addr).await.unwrap(); + let (mut server, _) = listener.accept().await.unwrap(); + + // Terminating route (tls: skip = false) with no termination state. + let refused = refuse_connect_when_tls_unavailable(&mut server, false, false) + .await + .expect("gate write should succeed"); + assert!( + refused, + "gate must refuse when TLS termination is unavailable" + ); + + let mut buf = vec![0u8; 512]; + let n = client.read(&mut buf).await.unwrap(); + let response = String::from_utf8_lossy(&buf[..n]); + assert!( + response.starts_with("HTTP/1.1 503 Service Unavailable\r\n"), + "client must read a 503 as the first bytes, not a 200; got: {response}" + ); + assert!( + !response.contains("200 Connection Established"), + "no tunnel must be established before the refusal; got: {response}" + ); + + let body_start = response.find("\r\n\r\n").unwrap() + 4; + let body: serde_json::Value = serde_json::from_str(&response[body_start..]).unwrap(); + assert_eq!(body["error"], "tls_termination_unavailable"); + assert_eq!(body["detail"], TLS_TERMINATION_UNAVAILABLE_DETAIL); + } + + /// The pre-200 gate must NOT touch the socket when the route can proceed: + /// either TLS termination state is present, or the route is `tls: skip` + /// (raw tunnel, no termination or credential injection). In both cases the + /// gate returns `false` and writes nothing, letting the caller send its + /// own `200 Connection Established`. + #[tokio::test] + async fn connect_with_tls_termination_or_skip_proceeds_without_writing() { + for (tls_present, tls_skip) in [(true, false), (false, true), (true, true)] { + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + let mut client = TcpStream::connect(addr).await.unwrap(); + let (mut server, _) = listener.accept().await.unwrap(); + + let refused = refuse_connect_when_tls_unavailable(&mut server, tls_present, tls_skip) + .await + .expect("gate should succeed"); + assert!( + !refused, + "gate must proceed (tls_present={tls_present}, tls_skip={tls_skip})" + ); + + // Nothing was written; after dropping the server the client sees a + // clean EOF (0 bytes) rather than any buffered response. + drop(server); + let mut buf = vec![0u8; 16]; + let n = client.read(&mut buf).await.unwrap(); + assert_eq!( + n, 0, + "gate must not write to the socket when proceeding \ + (tls_present={tls_present}, tls_skip={tls_skip})" + ); + } + } + + /// Drives a real `CONNECT` through the full `handle_tcp_connection` entry + /// point and returns `(completed, client_response_bytes, denial_stages)`. + /// `completed` is `false` when the handler was still running at `budget` + /// (e.g. a `tls: skip` route that proceeded past the fail-closed gate and + /// blocked on the unroutable upstream connect). `denial_stages` collects the + /// `denial_stage` of every `DenialEvent` the handler emitted — empty means + /// the connection was allowed and not blocked/refused at any stage. + /// + /// The client is an in-process `TcpStream` (no child process), so the client + /// socket is owned solely by this test process. Identity resolution finds + /// that owner in the descendant scan (which includes the entrypoint PID + /// itself), binds to `current_exe()`, and never falls through to the + /// whole-`/proc` scan — the environment-sensitive path that made a forked + /// child flaky under a busy CI `/proc`. Callers gate on Linux; + /// `evaluate_opa_tcp` denies unconditionally without `/proc`. + async fn drive_connect_through_handler( + endpoint_yaml: &str, + connect_target: &str, + budget: std::time::Duration, + ) -> (bool, Vec, Vec) { + const POLICY_REGO: &str = include_str!("../data/sandbox-policy.rego"); + + // Allow the current test binary — the in-process client's + // `/proc//exe` — to reach the endpoint. Matching is by path. + let exe = std::env::current_exe().expect("current_exe"); + let data = format!( + r#"network_policies: + test_allow: + name: test_allow + endpoints: +{endpoint_yaml} binaries: + - {{ path: "{exe}" }} +"#, + exe = exe.display(), + ); + let engine = Arc::new(OpaEngine::from_strings(POLICY_REGO, &data).expect("load policy")); + + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let proxy_port = listener.local_addr().unwrap().port(); + + // In-process client: connect, send the CONNECT request, read the reply to + // EOF. The proxy closes the socket after writing a 503/403; a proceeding + // `tls: skip` route stalls on the upstream connect until the handler is + // dropped at `budget`, which closes the socket and ends the read. + let target = connect_target.to_string(); + let client = tokio::spawn(async move { + let mut sock = TcpStream::connect(("127.0.0.1", proxy_port)).await.unwrap(); + let req = format!("CONNECT {target} HTTP/1.1\r\nHost: {target}\r\n\r\n"); + sock.write_all(req.as_bytes()).await.unwrap(); + let mut buf = Vec::new(); + let _ = sock.read_to_end(&mut buf).await; + buf + }); + + let (server, _peer) = listener.accept().await.unwrap(); + let entrypoint_pid = Arc::new(AtomicU32::new(std::process::id())); + let cache = Arc::new(BinaryIdentityCache::new()); + let (denial_tx, mut denial_rx) = mpsc::unbounded_channel(); + + let completed = tokio::time::timeout( + budget, + Box::pin(handle_tcp_connection( + server, + engine, + cache, + entrypoint_pid, + None, // tls_state — ephemeral CA unavailable + None, // inference_ctx + None, // policy_local_ctx + Arc::new(None), // trusted_host_gateway + None, // secret_resolver + None, // dynamic_credentials + Some(denial_tx), // denial_tx — positive allow/deny signal + None, // activity_tx + )), + ) + .await + .is_ok(); + + let stdout = client.await.expect("client task"); + + // The handler future is now finished or dropped, so its sender half is + // gone; drain every denial it emitted. + let mut denial_stages = Vec::new(); + while let Ok(event) = denial_rx.try_recv() { + denial_stages.push(event.denial_stage); + } + (completed, stdout, denial_stages) + } + + /// End-to-end regression for the gator finding on PR #2162: with no TLS + /// termination state, a terminating `CONNECT` must have its 503 written as + /// the FIRST bytes on the socket — never after a `200 Connection + /// Established` that would bury the refusal inside the tunnel as a TLS error. + #[tokio::test] + async fn connect_handler_refuses_terminating_route_with_503_before_200() { + if !cfg!(target_os = "linux") { + eprintln!("skipping: handler identity binding requires /proc (Linux)"); + return; + } + + // Public documentation IP (RFC 5737 TEST-NET-3): passes the SSRF check + // but is never actually connected — the fail-closed gate refuses first. + // The 30s budget is a generous belt against a slow CI runner; the refusal + // returns in milliseconds. + let (completed, stdout, _denials) = drive_connect_through_handler( + " - { host: \"203.0.113.10\", port: 443 }\n", + "203.0.113.10:443", + std::time::Duration::from_secs(30), + ) + .await; + + assert!(completed, "the refusal must return promptly, not hang"); + let resp = String::from_utf8_lossy(&stdout); + assert!( + resp.starts_with("HTTP/1.1 503 Service Unavailable"), + "first bytes must be a 503, not a 200; got: {resp:?}" + ); + assert!( + !resp.contains("200 Connection Established"), + "no tunnel must be established before the refusal; got: {resp:?}" + ); + assert!( + resp.contains("tls_termination_unavailable"), + "must be the TLS-termination-unavailable refusal; got: {resp:?}" + ); + } + + /// Ordering regression (gator re-check item 1): during CA-init failure, an + /// internal-address `CONNECT` must still receive the SSRF `403`, not the + /// TLS-unavailable `503` — SSRF validation runs before the fail-closed gate. + #[tokio::test] + async fn connect_handler_returns_ssrf_403_for_internal_address_not_503() { + if !cfg!(target_os = "linux") { + eprintln!("skipping: handler identity binding requires /proc (Linux)"); + return; + } + + let (completed, stdout, _denials) = drive_connect_through_handler( + " - { host: \"127.0.0.1\", port: 443 }\n", + "127.0.0.1:443", + std::time::Duration::from_secs(30), + ) + .await; + + assert!(completed, "the SSRF denial must return promptly, not hang"); + let resp = String::from_utf8_lossy(&stdout); + assert!( + resp.starts_with("HTTP/1.1 403 Forbidden"), + "internal address must get the SSRF 403; got: {resp:?}" + ); + assert!( + resp.contains("ssrf_denied"), + "must be an SSRF denial; got: {resp:?}" + ); + assert!( + !resp.contains("503"), + "SSRF runs before the fail-closed gate, so the 503 must not appear; got: {resp:?}" + ); + } + + /// A real `tls: skip` policy path through the handler is exempt from the + /// fail-closed gate even with no TLS termination state: the handler proceeds + /// past the refusal to the raw-tunnel upstream connect (which stalls on the + /// unroutable target), emitting no 503 and no client response before it. + #[tokio::test] + async fn connect_handler_tls_skip_route_is_not_refused() { + if !cfg!(target_os = "linux") { + eprintln!("skipping: handler identity binding requires /proc (Linux)"); + return; + } + + // `_completed` is intentionally ignored: whether the unroutable connect + // stalls (times out) or is rejected fast by a CI egress firewall, the + // invariant is the same — a `tls: skip` route is exempt from the + // fail-closed gate, so it proceeds to the tunnel connect and writes no + // 503/denial to the client beforehand. + let (_completed, stdout, denial_stages) = drive_connect_through_handler( + " - { host: \"203.0.113.10\", port: 443, tls: skip }\n", + "203.0.113.10:443", + std::time::Duration::from_millis(800), + ) + .await; + + let resp = String::from_utf8_lossy(&stdout); + assert!( + stdout.is_empty(), + "tls: skip must emit no refusal/denial before the tunnel connect; got: {resp:?}" + ); + // Positive allow evidence, so this test cannot pass vacuously on a + // policy/identity deny (which would emit a DenialEvent): the handler + // must have emitted no denial at any stage. + assert!( + denial_stages.is_empty(), + "tls: skip must be allowed end-to-end, not denied at any stage; got: {denial_stages:?}" + ); + } + + /// Verifies the policy half of the Linux handler tests on every platform + /// (no `/proc` needed): the temp-dir binary glob yields an Allow for a + /// literal-IP endpoint, `tls: skip` reads back as `TlsMode::Skip`, and a + /// terminating endpoint reads back as `TlsMode::Auto`. Locks the OPA + /// preconditions so a policy-format regression is caught even where the + /// process-identity binding can't run. + #[test] + fn handler_test_policy_allows_glob_binary_and_reads_tls_mode() { + const POLICY_REGO: &str = include_str!("../data/sandbox-policy.rego"); + + let eval = |tls_line: &str| { + let data = format!( + r#"network_policies: + test_allow: + name: test_allow + endpoints: + - {{ host: "203.0.113.10", port: 443{tls_line} }} + binaries: + - {{ path: "/tmp/openshell-connect-test/*" }} +"# + ); + let engine = OpaEngine::from_strings(POLICY_REGO, &data).expect("load policy"); + let input = crate::opa::NetworkInput { + host: "203.0.113.10".to_string(), + port: 443, + binary_path: PathBuf::from("/tmp/openshell-connect-test/connect-bash"), + binary_sha256: "unused".to_string(), + ancestors: vec![], + cmdline_paths: vec![], + }; + let (action, generation) = engine + .evaluate_network_action_with_generation(&input) + .expect("evaluate"); + match &action { + NetworkAction::Allow { matched_policy } => { + assert!(matched_policy.is_some(), "allow must carry the policy name"); + } + NetworkAction::Deny { reason } => { + panic!("glob binary must be allowed, got deny: {reason}") + } + } + let decision = ConnectDecision { + action, + generation, + binary: Some(input.binary_path), + binary_pid: Some(1), + ancestors: vec![], + cmdline_paths: vec![], + }; + query_tls_mode(&engine, &decision, "203.0.113.10", 443) + }; + + assert_eq!( + eval(", tls: skip"), + crate::l7::TlsMode::Skip, + "tls: skip endpoint must resolve to TlsMode::Skip" + ); + assert_eq!( + eval(""), + crate::l7::TlsMode::Auto, + "terminating endpoint must resolve to TlsMode::Auto" + ); + } + #[test] fn test_json_error_response_content_length_matches() { let resp = build_json_error_response(403, "Forbidden", "test", "detail"); diff --git a/crates/openshell-supervisor-network/src/run.rs b/crates/openshell-supervisor-network/src/run.rs index 9553e06736..99bd0eb1d2 100644 --- a/crates/openshell-supervisor-network/src/run.rs +++ b/crates/openshell-supervisor-network/src/run.rs @@ -223,9 +223,13 @@ pub async fn run_networking( (Some(state), Some(paths)) } Err(e) => { + // High severity: with TLS termination disabled the proxy + // cannot rewrite credentials, so it fails closed on + // TLS-bearing connections (see proxy.rs) rather than + // leaking placeholders through a raw tunnel. ocsf_emit!( ConfigStateChangeBuilder::new(ocsf_ctx()) - .severity(SeverityId::Medium) + .severity(SeverityId::High) .status(StatusId::Failure) .state(StateId::Disabled, "disabled") .message(format!( @@ -238,9 +242,13 @@ pub async fn run_networking( } } Err(e) => { + // High severity: with TLS termination disabled the proxy cannot + // rewrite credentials, so it fails closed on TLS-bearing + // connections (see proxy.rs) rather than leaking placeholders + // through a raw tunnel. ocsf_emit!( ConfigStateChangeBuilder::new(ocsf_ctx()) - .severity(SeverityId::Medium) + .severity(SeverityId::High) .status(StatusId::Failure) .state(StateId::Disabled, "disabled") .message(format!(