From 8139255b46ca91329bae1f545ece4f2f6265bd92 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Tue, 7 Jul 2026 15:37:50 -0700 Subject: [PATCH] http-client: expose WebSocket proxy prerequisites (#31342) ## Why #31335 lets HTTP callers obtain proxy-aware clients from `HttpClientFactory`, but a non-HTTP transport such as WebSockets also needs two pieces of policy owned by `codex-http-client`: a concrete route decision for its destination and the same custom-CA-aware rustls trust configuration used by HTTPS. Keeping these prerequisites in the shared abstraction means the dependent Responses WebSocket change (#31441) cannot independently reinterpret `features.respect_system_proxy`, PAC results, or enterprise CA settings. ## What changed - Add a redaction-safe `OutboundProxyRoute` with explicit transport-default, direct, and concrete-proxy outcomes. - Add `HttpClientFactory::resolve_proxy_route()` so transports can resolve a destination through the already-selected outbound proxy policy. - Resolve `ws://` and `wss://` URLs through their HTTP equivalents so system and PAC rules apply consistently. - Add an always-returned rustls config builder that starts from native roots and layers in any configured Codex custom CA bundle. The existing optional builder remains available to callers that can delegate the default configuration to their transport. - Continue redacting proxy URLs from `Debug` output because they may contain credentials. ## Review guide 1. `http-client/src/outbound_proxy.rs` defines the transport-neutral route result and WebSocket URL normalization. 2. `http-client/src/custom_ca.rs` factors the native-root/custom-CA construction so callers that perform TLS themselves can always obtain a config. 3. `http-client/src/outbound_proxy_tests.rs` verifies WebSocket normalization and legacy transport-default behavior. ## Test plan - `just test -p codex-http-client outbound_proxy` - `just test -p codex-http-client custom_ca` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/31342). * #31431 * #31363 * #31362 * #31361 * #31442 * #31441 * __->__ #31342 --- codex-rs/http-client/src/custom_ca.rs | 61 ++++++++++----- codex-rs/http-client/src/lib.rs | 2 + codex-rs/http-client/src/outbound_proxy.rs | 74 ++++++++++++++++++- .../http-client/src/outbound_proxy_tests.rs | 35 +++++++++ 4 files changed, 152 insertions(+), 20 deletions(-) diff --git a/codex-rs/http-client/src/custom_ca.rs b/codex-rs/http-client/src/custom_ca.rs index 4cfae8f23d..5a2e51c320 100644 --- a/codex-rs/http-client/src/custom_ca.rs +++ b/codex-rs/http-client/src/custom_ca.rs @@ -198,6 +198,16 @@ pub fn maybe_build_rustls_client_config_with_custom_ca() maybe_build_rustls_client_config_with_env(&ProcessEnv) } +/// Builds a rustls client config using native roots and any configured Codex custom CA bundle. +/// +/// Unlike [`maybe_build_rustls_client_config_with_custom_ca`], this always returns a config. Use +/// this when the caller must perform TLS itself instead of delegating default configuration to a +/// transport library. +pub fn build_rustls_client_config_with_custom_ca() +-> Result, BuildCustomCaTransportError> { + build_rustls_client_config_with_env(&ProcessEnv) +} + /// Builds a reqwest client for spawned subprocess tests that exercise CA behavior. /// /// This is the test-only client-construction path used by the subprocess coverage in `tests/`. @@ -219,6 +229,19 @@ fn maybe_build_rustls_client_config_with_env( return Ok(None); }; + build_rustls_client_config(Some(&bundle)).map(Some) +} + +fn build_rustls_client_config_with_env( + env_source: &dyn EnvSource, +) -> Result, BuildCustomCaTransportError> { + let bundle = env_source.configured_ca_bundle(); + build_rustls_client_config(bundle.as_ref()) +} + +fn build_rustls_client_config( + bundle: Option<&ConfiguredCaBundle>, +) -> Result, BuildCustomCaTransportError> { ensure_rustls_crypto_provider(); // Start from the platform roots so websocket callers keep the same baseline trust behavior @@ -235,30 +258,32 @@ fn maybe_build_rustls_client_config_with_env( } let _ = root_store.add_parsable_certificates(certs); - let certificates = bundle.load_certificates()?; - for (idx, cert) in certificates.into_iter().enumerate() { - if let Err(source) = root_store.add(cert) { - warn!( - source_env = bundle.source_env, - ca_path = %bundle.path.display(), - certificate_index = idx + 1, - error = %source, - "failed to register CA certificate in rustls root store" - ); - return Err(BuildCustomCaTransportError::RegisterRustlsCertificate { - source_env: bundle.source_env, - path: bundle.path.clone(), - certificate_index: idx + 1, - source, - }); + if let Some(bundle) = bundle { + let certificates = bundle.load_certificates()?; + for (idx, cert) in certificates.into_iter().enumerate() { + if let Err(source) = root_store.add(cert) { + warn!( + source_env = bundle.source_env, + ca_path = %bundle.path.display(), + certificate_index = idx + 1, + error = %source, + "failed to register CA certificate in rustls root store" + ); + return Err(BuildCustomCaTransportError::RegisterRustlsCertificate { + source_env: bundle.source_env, + path: bundle.path.clone(), + certificate_index: idx + 1, + source, + }); + } } } - Ok(Some(Arc::new( + Ok(Arc::new( ClientConfig::builder() .with_root_certificates(root_store) .with_no_client_auth(), - ))) + )) } /// Builds a reqwest client using an injected environment source and reqwest builder. diff --git a/codex-rs/http-client/src/lib.rs b/codex-rs/http-client/src/lib.rs index 07e579ecb3..49fc4225c3 100644 --- a/codex-rs/http-client/src/lib.rs +++ b/codex-rs/http-client/src/lib.rs @@ -18,6 +18,7 @@ pub use crate::custom_ca::BuildCustomCaTransportError; #[doc(hidden)] pub use crate::custom_ca::build_reqwest_client_for_subprocess_tests; pub use crate::custom_ca::build_reqwest_client_with_custom_ca; +pub use crate::custom_ca::build_rustls_client_config_with_custom_ca; pub use crate::custom_ca::maybe_build_rustls_client_config_with_custom_ca; pub use crate::default_client::HttpClient; pub use crate::default_client::RequestBuilder; @@ -27,6 +28,7 @@ pub use crate::outbound_proxy::BuildRouteAwareHttpClientError; pub use crate::outbound_proxy::ClientRouteClass; pub use crate::outbound_proxy::HttpClientFactory; pub use crate::outbound_proxy::OutboundProxyPolicy; +pub use crate::outbound_proxy::OutboundProxyRoute; pub use crate::outbound_proxy::RouteFailureClass; pub use crate::request::EncodedJsonBody; pub use crate::request::PreparedRequestBody; diff --git a/codex-rs/http-client/src/outbound_proxy.rs b/codex-rs/http-client/src/outbound_proxy.rs index 5506f3d40b..71793025ed 100644 --- a/codex-rs/http-client/src/outbound_proxy.rs +++ b/codex-rs/http-client/src/outbound_proxy.rs @@ -4,6 +4,7 @@ //! proxies are the fallback, and the final fallback is a direct connection. //! When disabled, callers retain the existing reqwest builder behavior. +use std::borrow::Cow; use std::collections::HashMap; use std::fmt; use std::io; @@ -96,6 +97,30 @@ pub enum OutboundProxyPolicy { RespectSystemProxy, } +/// Resolved proxy route for a concrete outbound destination. +/// +/// `TransportDefault` delegates environment-proxy handling to the underlying transport. Proxy +/// URLs are intentionally redacted from `Debug` output because they may contain credentials. +#[derive(Clone, PartialEq, Eq)] +pub enum OutboundProxyRoute { + /// Preserve the underlying transport's existing proxy behavior. + TransportDefault, + /// Connect directly and bypass transport-level proxy discovery. + Direct, + /// Connect through the selected proxy URL. + Proxy { url: String }, +} + +impl fmt::Debug for OutboundProxyRoute { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::TransportDefault => f.write_str("TransportDefault"), + Self::Direct => f.write_str("Direct"), + Self::Proxy { .. } => f.debug_struct("Proxy").field("url", &"").finish(), + } + } +} + /// Builds route-specific HTTP clients using one resolved outbound proxy policy. /// /// Construct this once from the effective application configuration and carry it with the @@ -119,6 +144,20 @@ impl HttpClientFactory { self.outbound_proxy_policy } + /// Resolves the proxy route for a concrete destination. + /// + /// WebSocket schemes are resolved through their HTTP equivalents so platform PAC and system + /// proxy APIs apply the same policy to `ws`/`wss` and `http`/`https` destinations. When system + /// resolution is unavailable, the transport retains responsibility for environment-proxy + /// fallback. + pub fn resolve_proxy_route(&self, request_url: &str) -> OutboundProxyRoute { + resolve_proxy_route( + request_url, + self.outbound_proxy_policy, + resolve_system_proxy, + ) + } + /// Builds a reqwest client for a concrete outbound route. pub fn build_reqwest_client( &self, @@ -135,6 +174,37 @@ impl HttpClientFactory { } } +fn resolve_proxy_route( + request_url: &str, + outbound_proxy_policy: OutboundProxyPolicy, + resolve_system_proxy: impl FnOnce(&str, &RequestOrigin) -> SystemProxyDecision, +) -> OutboundProxyRoute { + if matches!(outbound_proxy_policy, OutboundProxyPolicy::ReqwestDefault) { + return OutboundProxyRoute::TransportDefault; + } + + let request_url = proxy_resolution_url(request_url); + let Some(origin) = RequestOrigin::parse(&request_url) else { + return OutboundProxyRoute::TransportDefault; + }; + + match resolve_system_proxy(&request_url, &origin) { + SystemProxyDecision::Direct => OutboundProxyRoute::Direct, + SystemProxyDecision::Proxy { url } => OutboundProxyRoute::Proxy { url }, + SystemProxyDecision::Unavailable { .. } => OutboundProxyRoute::TransportDefault, + } +} + +fn proxy_resolution_url(request_url: &str) -> Cow<'_, str> { + if let Some(suffix) = request_url.strip_prefix("wss://") { + Cow::Owned(format!("https://{suffix}")) + } else if let Some(suffix) = request_url.strip_prefix("ws://") { + Cow::Owned(format!("http://{suffix}")) + } else { + Cow::Borrowed(request_url) + } +} + /// Error while building a resolver-aware reqwest client. #[derive(Debug, Error)] pub enum BuildRouteAwareHttpClientError { @@ -259,8 +329,8 @@ impl RequestOrigin { let scheme = uri.scheme_str()?.to_ascii_lowercase(); let host = uri.host()?.trim_matches(['[', ']']).to_ascii_lowercase(); let port = uri.port_u16().or(match scheme.as_str() { - "http" => Some(80), - "https" => Some(443), + "http" | "ws" => Some(80), + "https" | "wss" => Some(443), _ => None, })?; Some(Self { scheme, host, port }) diff --git a/codex-rs/http-client/src/outbound_proxy_tests.rs b/codex-rs/http-client/src/outbound_proxy_tests.rs index 734d5483ec..3f8bd925c9 100644 --- a/codex-rs/http-client/src/outbound_proxy_tests.rs +++ b/codex-rs/http-client/src/outbound_proxy_tests.rs @@ -9,6 +9,41 @@ struct MapEnv { values: HashMap, } +#[test] +fn websocket_route_uses_http_equivalent_for_system_resolution() { + let route = resolve_proxy_route( + "wss://api.openai.com/v1/responses", + OutboundProxyPolicy::RespectSystemProxy, + |request_url, origin| { + assert_eq!(request_url, "https://api.openai.com/v1/responses"); + assert_eq!(origin.scheme, "https"); + assert_eq!(origin.host, "api.openai.com"); + assert_eq!(origin.port, 443); + SystemProxyDecision::Proxy { + url: "http://proxy.example:8080".to_string(), + } + }, + ); + + assert_eq!( + route, + OutboundProxyRoute::Proxy { + url: "http://proxy.example:8080".to_string(), + } + ); +} + +#[test] +fn reqwest_default_route_preserves_transport_proxy_behavior() { + let route = resolve_proxy_route( + "wss://api.openai.com/v1/responses", + OutboundProxyPolicy::ReqwestDefault, + |_, _| panic!("default policy should not resolve system proxy settings"), + ); + + assert_eq!(route, OutboundProxyRoute::TransportDefault); +} + impl EnvSource for MapEnv { fn var(&self, key: &str) -> Option { self.values.get(key).cloned()