mirror of
https://github.com/openai/codex.git
synced 2026-11-12 22:42:36 +00:00
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
This commit is contained in:
1 parent
922249310f
commit
8139255b46
4 files changed
+152
-20
No files matched your search
@@ -198,6 +198,16 @@ pub fn maybe_build_rustls_client_config_with_custom_ca()
|
|||||||
maybe_build_rustls_client_config_with_env(&ProcessEnv)
|
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<Arc<ClientConfig>, BuildCustomCaTransportError> {
|
||||||
|
build_rustls_client_config_with_env(&ProcessEnv)
|
||||||
|
}
|
||||||
|
|
||||||
/// Builds a reqwest client for spawned subprocess tests that exercise CA behavior.
|
/// 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/`.
|
/// 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);
|
return Ok(None);
|
||||||
};
|
};
|
||||||
|
|
||||||
|
build_rustls_client_config(Some(&bundle)).map(Some)
|
||||||
|
}
|
||||||
|
|
||||||
|
fn build_rustls_client_config_with_env(
|
||||||
|
env_source: &dyn EnvSource,
|
||||||
|
) -> Result<Arc<ClientConfig>, BuildCustomCaTransportError> {
|
||||||
|
let bundle = env_source.configured_ca_bundle();
|
||||||
|
build_rustls_client_config(bundle.as_ref())
|
||||||
|
}
|
||||||
|
|
||||||
|
fn build_rustls_client_config(
|
||||||
|
bundle: Option<&ConfiguredCaBundle>,
|
||||||
|
) -> Result<Arc<ClientConfig>, BuildCustomCaTransportError> {
|
||||||
ensure_rustls_crypto_provider();
|
ensure_rustls_crypto_provider();
|
||||||
|
|
||||||
// Start from the platform roots so websocket callers keep the same baseline trust behavior
|
// 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 _ = root_store.add_parsable_certificates(certs);
|
||||||
|
|
||||||
let certificates = bundle.load_certificates()?;
|
if let Some(bundle) = bundle {
|
||||||
for (idx, cert) in certificates.into_iter().enumerate() {
|
let certificates = bundle.load_certificates()?;
|
||||||
if let Err(source) = root_store.add(cert) {
|
for (idx, cert) in certificates.into_iter().enumerate() {
|
||||||
warn!(
|
if let Err(source) = root_store.add(cert) {
|
||||||
source_env = bundle.source_env,
|
warn!(
|
||||||
ca_path = %bundle.path.display(),
|
source_env = bundle.source_env,
|
||||||
certificate_index = idx + 1,
|
ca_path = %bundle.path.display(),
|
||||||
error = %source,
|
certificate_index = idx + 1,
|
||||||
"failed to register CA certificate in rustls root store"
|
error = %source,
|
||||||
);
|
"failed to register CA certificate in rustls root store"
|
||||||
return Err(BuildCustomCaTransportError::RegisterRustlsCertificate {
|
);
|
||||||
source_env: bundle.source_env,
|
return Err(BuildCustomCaTransportError::RegisterRustlsCertificate {
|
||||||
path: bundle.path.clone(),
|
source_env: bundle.source_env,
|
||||||
certificate_index: idx + 1,
|
path: bundle.path.clone(),
|
||||||
source,
|
certificate_index: idx + 1,
|
||||||
});
|
source,
|
||||||
|
});
|
||||||
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
Ok(Some(Arc::new(
|
Ok(Arc::new(
|
||||||
ClientConfig::builder()
|
ClientConfig::builder()
|
||||||
.with_root_certificates(root_store)
|
.with_root_certificates(root_store)
|
||||||
.with_no_client_auth(),
|
.with_no_client_auth(),
|
||||||
)))
|
))
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Builds a reqwest client using an injected environment source and reqwest builder.
|
/// Builds a reqwest client using an injected environment source and reqwest builder.
|
||||||
|
|||||||
@@ -18,6 +18,7 @@ pub use crate::custom_ca::BuildCustomCaTransportError;
|
|||||||
#[doc(hidden)]
|
#[doc(hidden)]
|
||||||
pub use crate::custom_ca::build_reqwest_client_for_subprocess_tests;
|
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_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::custom_ca::maybe_build_rustls_client_config_with_custom_ca;
|
||||||
pub use crate::default_client::HttpClient;
|
pub use crate::default_client::HttpClient;
|
||||||
pub use crate::default_client::RequestBuilder;
|
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::ClientRouteClass;
|
||||||
pub use crate::outbound_proxy::HttpClientFactory;
|
pub use crate::outbound_proxy::HttpClientFactory;
|
||||||
pub use crate::outbound_proxy::OutboundProxyPolicy;
|
pub use crate::outbound_proxy::OutboundProxyPolicy;
|
||||||
|
pub use crate::outbound_proxy::OutboundProxyRoute;
|
||||||
pub use crate::outbound_proxy::RouteFailureClass;
|
pub use crate::outbound_proxy::RouteFailureClass;
|
||||||
pub use crate::request::EncodedJsonBody;
|
pub use crate::request::EncodedJsonBody;
|
||||||
pub use crate::request::PreparedRequestBody;
|
pub use crate::request::PreparedRequestBody;
|
||||||
|
|||||||
@@ -4,6 +4,7 @@
|
|||||||
//! proxies are the fallback, and the final fallback is a direct connection.
|
//! proxies are the fallback, and the final fallback is a direct connection.
|
||||||
//! When disabled, callers retain the existing reqwest builder behavior.
|
//! When disabled, callers retain the existing reqwest builder behavior.
|
||||||
|
|
||||||
|
use std::borrow::Cow;
|
||||||
use std::collections::HashMap;
|
use std::collections::HashMap;
|
||||||
use std::fmt;
|
use std::fmt;
|
||||||
use std::io;
|
use std::io;
|
||||||
@@ -96,6 +97,30 @@ pub enum OutboundProxyPolicy {
|
|||||||
RespectSystemProxy,
|
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", &"<redacted>").finish(),
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
/// Builds route-specific HTTP clients using one resolved outbound proxy policy.
|
/// Builds route-specific HTTP clients using one resolved outbound proxy policy.
|
||||||
///
|
///
|
||||||
/// Construct this once from the effective application configuration and carry it with the
|
/// Construct this once from the effective application configuration and carry it with the
|
||||||
@@ -119,6 +144,20 @@ impl HttpClientFactory {
|
|||||||
self.outbound_proxy_policy
|
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.
|
/// Builds a reqwest client for a concrete outbound route.
|
||||||
pub fn build_reqwest_client(
|
pub fn build_reqwest_client(
|
||||||
&self,
|
&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.
|
/// Error while building a resolver-aware reqwest client.
|
||||||
#[derive(Debug, Error)]
|
#[derive(Debug, Error)]
|
||||||
pub enum BuildRouteAwareHttpClientError {
|
pub enum BuildRouteAwareHttpClientError {
|
||||||
@@ -259,8 +329,8 @@ impl RequestOrigin {
|
|||||||
let scheme = uri.scheme_str()?.to_ascii_lowercase();
|
let scheme = uri.scheme_str()?.to_ascii_lowercase();
|
||||||
let host = uri.host()?.trim_matches(['[', ']']).to_ascii_lowercase();
|
let host = uri.host()?.trim_matches(['[', ']']).to_ascii_lowercase();
|
||||||
let port = uri.port_u16().or(match scheme.as_str() {
|
let port = uri.port_u16().or(match scheme.as_str() {
|
||||||
"http" => Some(80),
|
"http" | "ws" => Some(80),
|
||||||
"https" => Some(443),
|
"https" | "wss" => Some(443),
|
||||||
_ => None,
|
_ => None,
|
||||||
})?;
|
})?;
|
||||||
Some(Self { scheme, host, port })
|
Some(Self { scheme, host, port })
|
||||||
|
|||||||
@@ -9,6 +9,41 @@ struct MapEnv {
|
|||||||
values: HashMap<String, String>,
|
values: HashMap<String, String>,
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[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 {
|
impl EnvSource for MapEnv {
|
||||||
fn var(&self, key: &str) -> Option<String> {
|
fn var(&self, key: &str) -> Option<String> {
|
||||||
self.values.get(key).cloned()
|
self.values.get(key).cloned()
|
||||||
|
|||||||
Reference in new issue
Block a user