From 1c567f94a73301139c6306d822f9f09688ebcf56 Mon Sep 17 00:00:00 2001 From: Tony Luo Date: Tue, 7 Jul 2026 14:42:13 +0800 Subject: [PATCH 1/6] fix(network): fail closed when credential placeholders cannot be rewritten When the credential rewriter degrades internally, the proxy forwarded the literal `openshell:resolve:env:` placeholder (or its provider alias marker) to the upstream instead of the resolved secret, leaking the reserved token on the wire and causing upstream auth failures (#2161). Two fail-open paths are closed: - secrets: `rewrite_http_header_block` returned the header block verbatim when no `SecretResolver` was available, so the fail-closed marker scan (which ran only on the resolved path) never saw the placeholder. It now scans the header region for reserved markers even with no resolver and returns `UnresolvedPlaceholderError` when one is present. Marker-free traffic still passes through unchanged. - proxy: when TLS was detected on a CONNECT but `tls_state` was `None` (ephemeral CA generation or CA file write failed at startup), the handler fell back to a raw `copy_bidirectional` tunnel, bypassing credential rewrite. Inside the proxy handler `tls_state` is `None` only on CA-init failure (`mode != Proxy` never starts the handler, and `tls: skip` is handled earlier), so it now refuses the connection with a 503 and a High-severity denial event instead of tunneling. The two startup CA-failure logs are raised from Medium to High. Tests: resolver=None with a placeholder in the request line, a header value, and the provider-alias form now fail closed; marker-free passthrough is unchanged; the relay integration test asserts the request is rejected before any byte reaches upstream; and the 503 fail-closed response contract is locked. Signed-off-by: Tony Luo --- crates/openshell-core/src/secrets.rs | 44 ++++++++- .../src/l7/rest.rs | 66 ++++++-------- .../openshell-supervisor-network/src/proxy.rs | 89 +++++++++++++++---- .../openshell-supervisor-network/src/run.rs | 12 ++- 4 files changed, 152 insertions(+), 59 deletions(-) diff --git a/crates/openshell-core/src/secrets.rs b/crates/openshell-core/src/secrets.rs index d71a2139c6..27034cef16 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,33 @@ 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_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..3f140aef78 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,42 @@ 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"); + assert!( + relay.is_err(), + "relay must fail closed when a placeholder cannot be resolved" + ); - // Without a resolver, the placeholder LEAKS to upstream — this is the - // documented behavior that causes 401s when `tls: terminate` is missing. + // Nothing must have reached upstream. Drop the proxy write half so the + // read observes EOF instead of blocking, then confirm neither the + // placeholder nor the secret leaked. + drop(proxy_to_upstream); + let mut forwarded = Vec::new(); + upstream_side + .read_to_end(&mut forwarded) + .await + .expect("upstream read should complete"); + let forwarded = String::from_utf8_lossy(&forwarded); assert!( - forwarded.contains("openshell:resolve:env:NVIDIA_API_KEY"), - "Expected placeholder to leak without resolver, got: {forwarded}" + !forwarded.contains("openshell:resolve:env:"), + "placeholder must not leak to upstream: {forwarded}" ); assert!( !forwarded.contains("nvapi-secret"), - "Real secret should NOT appear without resolver, got: {forwarded}" + "secret must not leak to upstream: {forwarded}" ); } diff --git a/crates/openshell-supervisor-network/src/proxy.rs b/crates/openshell-supervisor-network/src/proxy.rs index 0d2c8c0258..1c0598f65a 100644 --- a/crates/openshell-supervisor-network/src/proxy.rs +++ b/crates/openshell-supervisor-network/src/proxy.rs @@ -1207,21 +1207,54 @@ 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()?; + // Fail-closed: TLS was detected but no TLS termination state is + // available. Inside the proxy handler this can only mean the + // ephemeral CA failed to generate or write at startup (run.rs) — + // the `mode != Proxy` case never starts this handler, and + // `tls: skip` is handled earlier. Raw-tunneling here would forward + // the client's TLS stream straight to the upstream, bypassing + // credential rewrite and leaking any `openshell:resolve:env:*` + // placeholder verbatim. Refuse the connection instead. + const DETAIL: &str = "TLS termination unavailable (CA initialization failed); \ + refusing to tunnel — 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", + ); + respond( + &mut client, + &build_json_error_response( + 503, + "Service Unavailable", + "tls_termination_unavailable", + DETAIL, + ), + ) + .await?; + return Ok(()); } } else if tunnel_protocol == TunnelProtocol::Http1 { // Plaintext HTTP detected. @@ -7764,6 +7797,32 @@ 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 (CA initialization failed); \ + refusing to tunnel — credential rewrite would be bypassed"; + 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); + } + #[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!( From e736ee9cc5d0fe7ef60cd6ab6e949594f35c76a4 Mon Sep 17 00:00:00 2001 From: Tony Luo Date: Fri, 10 Jul 2026 11:40:06 +0800 Subject: [PATCH 2/6] fix(network): refuse CONNECT before 200 when TLS termination is unavailable The fail-closed refusal for a terminating CONNECT with no TLS termination state (ephemeral CA init failed) was written after the 200 Connection Established response. Because a CONNECT client only sends its TLS ClientHello after reading the 200, the peek-based TLS detection is inherently post-200, so the 503 landed inside the established tunnel and surfaced to the client as a TLS protocol error rather than a readable status. An 'allowed CONNECT' event was also logged first. Move the decision to a pre-200 gate: query_tls_mode resolves purely from the policy decision + host/port (no peeked bytes), so the route's TLS treatment is known before the tunnel is acknowledged. When TLS state is absent and the route is not tls: skip, write the 503 as the first bytes on the socket, emit the High-severity Denied event, and close. tls: skip routes tunnel raw exactly as before, and no allowed-CONNECT event is emitted on the refusal path. The now-unreachable post-200 branch is kept as defense in depth but no longer writes an in-tunnel 503; it fails closed by dropping the connection instead. Add connection-level regression tests over a real loopback socket: the gate refuses with HTTP/1.1 503 as the first bytes for a terminating route, and writes nothing when TLS termination is present or the route is tls: skip. Signed-off-by: Tony Luo --- .../openshell-supervisor-network/src/proxy.rs | 208 +++++++++++++++--- 1 file changed, 182 insertions(+), 26 deletions(-) diff --git a/crates/openshell-supervisor-network/src/proxy.rs b/crates/openshell-supervisor-network/src/proxy.rs index 1c0598f65a..f78c44c57c 100644 --- a/crates/openshell-supervisor-network/src/proxy.rs +++ b/crates/openshell-supervisor-network/src/proxy.rs @@ -741,6 +741,53 @@ async fn handle_tcp_connection( return Ok(()); } + // Determine the route's TLS treatment before acknowledging the tunnel. + // `query_tls_mode` resolves purely from the policy decision + host/port + // (no peeked bytes), so it is valid here, before the `200`. + let effective_tls_skip = + query_tls_mode(&opa_engine, &decision, &host_lc, port) == crate::l7::TlsMode::Skip; + + // Fail closed 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 (see the helper) 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 sandbox_entrypoint_pid = entrypoint_pid.load(Ordering::Acquire); // Query allowed_ips from the matched endpoint config (if any). @@ -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,16 +1252,18 @@ async fn handle_tcp_connection( } } } else { - // Fail-closed: TLS was detected but no TLS termination state is - // available. Inside the proxy handler this can only mean the - // ephemeral CA failed to generate or write at startup (run.rs) — - // the `mode != Proxy` case never starts this handler, and - // `tls: skip` is handled earlier. Raw-tunneling here would forward - // the client's TLS stream straight to the upstream, bypassing - // credential rewrite and leaking any `openshell:resolve:env:*` - // placeholder verbatim. Refuse the connection instead. - const DETAIL: &str = "TLS termination unavailable (CA initialization failed); \ - refusing to tunnel — credential rewrite would be bypassed"; + // 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) @@ -1244,16 +1291,8 @@ async fn handle_tcp_connection( DETAIL, "connect-tls-termination-unavailable", ); - respond( - &mut client, - &build_json_error_response( - 503, - "Service Unavailable", - "tls_termination_unavailable", - DETAIL, - ), - ) - .await?; + // 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 { @@ -4380,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 @@ -7804,8 +7887,7 @@ network_policies: /// placeholders verbatim. #[test] fn test_json_error_response_503_tls_termination_unavailable() { - let detail = "TLS termination unavailable (CA initialization failed); \ - refusing to tunnel — credential rewrite would be bypassed"; + let detail = TLS_TERMINATION_UNAVAILABLE_DETAIL; let resp = build_json_error_response( 503, "Service Unavailable", @@ -7823,6 +7905,80 @@ network_policies: 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})" + ); + } + } + #[test] fn test_json_error_response_content_length_matches() { let resp = build_json_error_response(403, "Forbidden", "test", "detail"); From 0148b4154178870124ca10dde9245aad777fe0f2 Mon Sep 17 00:00:00 2001 From: Tony Luo Date: Fri, 10 Jul 2026 13:43:46 +0800 Subject: [PATCH 3/6] fix(network): order the CONNECT TLS-unavailable refusal after SSRF Addresses the gator re-check on #2162. Ordering: the pre-200 fail-closed refusal ran before SSRF/allowed_ips validation, so during CA-init failure an internal-address CONNECT got a 503 tls_termination_unavailable instead of the normal 403 ssrf_denied, weakening operator visibility in degraded state. The SSRF branches now return validated addresses; the refusal runs after that validation (an internal address has already been denied with 403) but still before the upstream connect and before 200 Connection Established. effective_tls_skip is still resolved up front since the refusal consumes it. Tests: add connection-level regressions through the real handle_tcp_connection, driving a CONNECT from a child /bin/bash copy so the /proc process-identity binding resolves it against a permissive policy (the hot-swap test's identity pattern). They assert the first bytes are HTTP/1.1 503 for a terminating route with no TLS state, a 403 (not 503) for an internal address, and no refusal for a tls: skip route. These are gated to Linux at runtime (evaluate_opa_tcp needs /proc); a companion test verifies the OPA policy shape (glob allow, tls mode) on every platform so the precondition is locked where /proc is unavailable. rest.rs: tighten the fail-closed relay test to assert the forwarded buffer is_empty() rather than merely lacking the placeholder/secret. Signed-off-by: Tony Luo --- .../src/l7/rest.rs | 17 +- .../openshell-supervisor-network/src/proxy.rs | 380 +++++++++++++++--- 2 files changed, 330 insertions(+), 67 deletions(-) diff --git a/crates/openshell-supervisor-network/src/l7/rest.rs b/crates/openshell-supervisor-network/src/l7/rest.rs index 3f140aef78..5c62f6f9ee 100644 --- a/crates/openshell-supervisor-network/src/l7/rest.rs +++ b/crates/openshell-supervisor-network/src/l7/rest.rs @@ -4909,23 +4909,20 @@ mod tests { "relay must fail closed when a placeholder cannot be resolved" ); - // Nothing must have reached upstream. Drop the proxy write half so the - // read observes EOF instead of blocking, then confirm neither the - // placeholder nor the secret leaked. + // 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"); - let forwarded = String::from_utf8_lossy(&forwarded); assert!( - !forwarded.contains("openshell:resolve:env:"), - "placeholder must not leak to upstream: {forwarded}" - ); - assert!( - !forwarded.contains("nvapi-secret"), - "secret must not leak to upstream: {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 f78c44c57c..fa99243252 100644 --- a/crates/openshell-supervisor-network/src/proxy.rs +++ b/crates/openshell-supervisor-network/src/proxy.rs @@ -741,53 +741,15 @@ async fn handle_tcp_connection( return Ok(()); } - // Determine the route's TLS treatment before acknowledging the tunnel. - // `query_tls_mode` resolves purely from the policy decision + host/port - // (no peeked bytes), so it is valid here, before the `200`. + // 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; - // Fail closed 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 (see the helper) 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 sandbox_entrypoint_pid = entrypoint_pid.load(Ordering::Acquire); // Query allowed_ips from the matched endpoint config (if any). @@ -809,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 @@ -818,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()) @@ -874,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()) @@ -976,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()) @@ -1028,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()) @@ -1078,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() @@ -7979,6 +7979,272 @@ network_policies: } } + /// Drives a real `CONNECT` through the full `handle_tcp_connection` entry + /// point and returns `(completed, client_response_bytes)`. `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). + /// + /// A child copy of `/bin/bash` opens the connection via `/dev/tcp` so the + /// proxy's `/proc`-based process-identity binding resolves it against a + /// permissive policy (glob on the temp dir) — the same identity pattern the + /// hot-swap regression test uses. 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) { + use std::os::unix::fs::PermissionsExt; + use std::process::{Command, Stdio}; + + const POLICY_REGO: &str = include_str!("../data/sandbox-policy.rego"); + + let tmp = tempfile::TempDir::new().unwrap(); + let bash = tmp.path().join("connect-bash"); + std::fs::copy("/bin/bash", &bash).expect("copy /bin/bash"); + std::fs::set_permissions(&bash, std::fs::Permissions::from_mode(0o755)).unwrap(); + + // Permissive policy: allow any binary under the temp dir (the child bash + // copy) to reach the endpoint. The binary glob matches the child's + // `/proc//exe`; matching is by path, not by hash. + let data = format!( + r#"network_policies: + test_allow: + name: test_allow + endpoints: +{endpoint_yaml} binaries: + - {{ path: "{glob}/*" }} +"#, + glob = tmp.path().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(); + + // The child connects, sends the CONNECT request, then reads the proxy's + // reply to EOF. `cat` keeps the socket ESTABLISHED while the proxy + // resolves identity, then surfaces whatever bytes the proxy wrote. + let script = format!( + "exec 3<>/dev/tcp/127.0.0.1/{proxy_port}; \ + printf 'CONNECT {connect_target} HTTP/1.1\\r\\nHost: {connect_target}\\r\\n\\r\\n' >&3; \ + cat <&3", + ); + let mut child = Command::new(&bash) + .arg("-c") + .arg(&script) + .stdin(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::null()) + .spawn() + .expect("spawn connect-bash child"); + + let (server, _peer) = listener.accept().await.unwrap(); + let entrypoint_pid = Arc::new(AtomicU32::new(std::process::id())); + let cache = Arc::new(BinaryIdentityCache::new()); + + 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 + None, // denial_tx + None, // activity_tx + )), + ) + .await + .is_ok(); + + let _ = child.kill(); + let output = child.wait_with_output().expect("collect child output"); + (completed, output.stdout) + } + + /// 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; + } + if !std::path::Path::new("/bin/bash").exists() { + eprintln!("skipping: /bin/bash not available"); + return; + } + + // Public documentation IP (RFC 5737 TEST-NET-3): passes the SSRF check + // but is never actually connected — the fail-closed gate refuses first. + let (completed, stdout) = drive_connect_through_handler( + " - { host: \"203.0.113.10\", port: 443 }\n", + "203.0.113.10:443", + std::time::Duration::from_secs(10), + ) + .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; + } + if !std::path::Path::new("/bin/bash").exists() { + eprintln!("skipping: /bin/bash not available"); + return; + } + + let (completed, stdout) = drive_connect_through_handler( + " - { host: \"127.0.0.1\", port: 443 }\n", + "127.0.0.1:443", + std::time::Duration::from_secs(10), + ) + .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; + } + if !std::path::Path::new("/bin/bash").exists() { + eprintln!("skipping: /bin/bash not available"); + 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. A wrongly-refused route would + // instead surface a 503, making `stdout` non-empty. + let (_completed, stdout) = 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:?}" + ); + } + + /// 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"); From f694fc00b3ed116a9a513242ccf29067275bd827 Mon Sep 17 00:00:00 2001 From: Tony Luo Date: Fri, 10 Jul 2026 14:25:42 +0800 Subject: [PATCH 4/6] test(network): keep the CONNECT handler test client fork-free MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The handler regression tests forked cat to read the proxy reply, so the client socket fd was inherited by a second process with a different binary. The identity resolver correctly denies that as ambiguous shared-socket ownership (the same invariant resolve_process_identity_denies_fork_exec_shared_socket_ambiguity pins), so the tests exercised the deny path instead of the allow path — and on busy CI runners the deny-path /proc fallback scan exceeded the test budget and looked like a hang. The client script now uses only bash builtins (exec, printf, read -d '') so exactly one process owns the socket, and the child is left to exit on EOF instead of being killed mid-read. Signed-off-by: Tony Luo --- .../openshell-supervisor-network/src/proxy.rs | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/crates/openshell-supervisor-network/src/proxy.rs b/crates/openshell-supervisor-network/src/proxy.rs index fa99243252..d96f4c6511 100644 --- a/crates/openshell-supervisor-network/src/proxy.rs +++ b/crates/openshell-supervisor-network/src/proxy.rs @@ -8024,14 +8024,18 @@ network_policies: let proxy_port = listener.local_addr().unwrap().port(); // The child connects, sends the CONNECT request, then reads the proxy's - // reply to EOF. `cat` keeps the socket ESTABLISHED while the proxy - // resolves identity, then surfaces whatever bytes the proxy wrote. + // reply to EOF and prints it. Everything is a bash builtin (exec, + // printf, read) so NO external command is forked — a forked `cat`/`head` + // would inherit the socket fd, making the inode owned by two PIDs with + // different binaries, which the resolver correctly denies as ambiguous + // shared-socket ownership. `read -d ''` reads all bytes up to EOF into + // `resp`; on EOF it returns non-zero but `resp` still holds the reply. let script = format!( "exec 3<>/dev/tcp/127.0.0.1/{proxy_port}; \ printf 'CONNECT {connect_target} HTTP/1.1\\r\\nHost: {connect_target}\\r\\n\\r\\n' >&3; \ - cat <&3", + IFS= read -r -d '' resp <&3; printf '%s' \"$resp\"", ); - let mut child = Command::new(&bash) + let child = Command::new(&bash) .arg("-c") .arg(&script) .stdin(Stdio::null()) @@ -8064,7 +8068,10 @@ network_policies: .await .is_ok(); - let _ = child.kill(); + // Do not kill the child: it exits on its own once the server socket is + // dropped (normal return or timeout), at which point its read sees EOF + // and it prints whatever the proxy wrote. Killing it here would race + // that final read and truncate the captured response. let output = child.wait_with_output().expect("collect child output"); (completed, output.stdout) } From cc8aaf42397adf4e02f9dd6ef6dee74a9f34f02d Mon Sep 17 00:00:00 2001 From: Tony Luo Date: Fri, 10 Jul 2026 14:52:28 +0800 Subject: [PATCH 5/6] test(network): drive the CONNECT handler tests with an in-process client MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The child-process client (even fork-free) made the handler tests environment-sensitive: on CI runners with a busy or restricted /proc, resolving the child's socket ownership degraded into the whole-/proc fallback scan and a deny, which surfaced as a hang. The client is now an in-process TcpStream and the test policy allows current_exe(), so identity resolution binds the socket to the test process itself in the descendant scan — the same in-process pattern the passing resolve_process_identity tests rely on. The tls: skip test additionally asserts that the handler emitted no DenialEvent at any stage, so it can no longer pass vacuously on a policy or identity deny. Refusal budgets widened to 30s as a belt for slow runners; the refusals themselves return in milliseconds. Signed-off-by: Tony Luo --- .../openshell-supervisor-network/src/proxy.rs | 141 ++++++++---------- 1 file changed, 65 insertions(+), 76 deletions(-) diff --git a/crates/openshell-supervisor-network/src/proxy.rs b/crates/openshell-supervisor-network/src/proxy.rs index d96f4c6511..d7dfe30e1a 100644 --- a/crates/openshell-supervisor-network/src/proxy.rs +++ b/crates/openshell-supervisor-network/src/proxy.rs @@ -7980,73 +7980,63 @@ network_policies: } /// Drives a real `CONNECT` through the full `handle_tcp_connection` entry - /// point and returns `(completed, client_response_bytes)`. `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). + /// 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. /// - /// A child copy of `/bin/bash` opens the connection via `/dev/tcp` so the - /// proxy's `/proc`-based process-identity binding resolves it against a - /// permissive policy (glob on the temp dir) — the same identity pattern the - /// hot-swap regression test uses. Callers gate on Linux; `evaluate_opa_tcp` - /// denies unconditionally without `/proc`. + /// 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) { - use std::os::unix::fs::PermissionsExt; - use std::process::{Command, Stdio}; - + ) -> (bool, Vec, Vec) { const POLICY_REGO: &str = include_str!("../data/sandbox-policy.rego"); - let tmp = tempfile::TempDir::new().unwrap(); - let bash = tmp.path().join("connect-bash"); - std::fs::copy("/bin/bash", &bash).expect("copy /bin/bash"); - std::fs::set_permissions(&bash, std::fs::Permissions::from_mode(0o755)).unwrap(); - - // Permissive policy: allow any binary under the temp dir (the child bash - // copy) to reach the endpoint. The binary glob matches the child's - // `/proc//exe`; matching is by path, not by hash. + // 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: "{glob}/*" }} + - {{ path: "{exe}" }} "#, - glob = tmp.path().display(), + 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(); - // The child connects, sends the CONNECT request, then reads the proxy's - // reply to EOF and prints it. Everything is a bash builtin (exec, - // printf, read) so NO external command is forked — a forked `cat`/`head` - // would inherit the socket fd, making the inode owned by two PIDs with - // different binaries, which the resolver correctly denies as ambiguous - // shared-socket ownership. `read -d ''` reads all bytes up to EOF into - // `resp`; on EOF it returns non-zero but `resp` still holds the reply. - let script = format!( - "exec 3<>/dev/tcp/127.0.0.1/{proxy_port}; \ - printf 'CONNECT {connect_target} HTTP/1.1\\r\\nHost: {connect_target}\\r\\n\\r\\n' >&3; \ - IFS= read -r -d '' resp <&3; printf '%s' \"$resp\"", - ); - let child = Command::new(&bash) - .arg("-c") - .arg(&script) - .stdin(Stdio::null()) - .stdout(Stdio::piped()) - .stderr(Stdio::null()) - .spawn() - .expect("spawn connect-bash child"); + // 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, @@ -8055,25 +8045,28 @@ network_policies: 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 - None, // denial_tx - None, // activity_tx + 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(); - // Do not kill the child: it exits on its own once the server socket is - // dropped (normal return or timeout), at which point its read sees EOF - // and it prints whatever the proxy wrote. Killing it here would race - // that final read and truncate the captured response. - let output = child.wait_with_output().expect("collect child output"); - (completed, output.stdout) + 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 @@ -8086,17 +8079,15 @@ network_policies: eprintln!("skipping: handler identity binding requires /proc (Linux)"); return; } - if !std::path::Path::new("/bin/bash").exists() { - eprintln!("skipping: /bin/bash not available"); - return; - } // Public documentation IP (RFC 5737 TEST-NET-3): passes the SSRF check // but is never actually connected — the fail-closed gate refuses first. - let (completed, stdout) = drive_connect_through_handler( + // 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(10), + std::time::Duration::from_secs(30), ) .await; @@ -8125,15 +8116,11 @@ network_policies: eprintln!("skipping: handler identity binding requires /proc (Linux)"); return; } - if !std::path::Path::new("/bin/bash").exists() { - eprintln!("skipping: /bin/bash not available"); - return; - } - let (completed, stdout) = drive_connect_through_handler( + 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(10), + std::time::Duration::from_secs(30), ) .await; @@ -8163,18 +8150,13 @@ network_policies: eprintln!("skipping: handler identity binding requires /proc (Linux)"); return; } - if !std::path::Path::new("/bin/bash").exists() { - eprintln!("skipping: /bin/bash not available"); - 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. A wrongly-refused route would - // instead surface a 503, making `stdout` non-empty. - let (_completed, stdout) = drive_connect_through_handler( + // 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), @@ -8186,6 +8168,13 @@ network_policies: 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 From cb5bc5156bd7518f4930bb5612b1f97075f49a8b Mon Sep 17 00:00:00 2001 From: Tony Luo Date: Fri, 10 Jul 2026 15:10:03 +0800 Subject: [PATCH 6/6] test(core): pin the percent-encoded marker no-resolver fail-closed path The no-resolver scan already catches the percent-encoded canonical marker through its decoded pass; this regression pins it: a request line carrying openshell%3Aresolve%3Aenv%3AKEY with no resolver must fail closed with UnresolvedPlaceholderError { location: header }. Signed-off-by: Tony Luo --- crates/openshell-core/src/secrets.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/crates/openshell-core/src/secrets.rs b/crates/openshell-core/src/secrets.rs index 27034cef16..e93bdc5390 100644 --- a/crates/openshell-core/src/secrets.rs +++ b/crates/openshell-core/src/secrets.rs @@ -1921,6 +1921,17 @@ mod tests { 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