From 1c567f94a73301139c6306d822f9f09688ebcf56 Mon Sep 17 00:00:00 2001 From: Tony Luo Date: Tue, 7 Jul 2026 14:42:13 +0800 Subject: [PATCH] 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!(