From ff4442fcb75d1e61f70d41db6209d71b4f1dabef Mon Sep 17 00:00:00 2001 From: Ivan Zakharchanka <3axap4eHko@gmail.com> Date: Tue, 18 Nov 2025 18:13:51 -0500 Subject: [PATCH 1/7] Add custom CA support for login flows From ca54becc5bbdb39465561731e8572e306fbe7c8d Mon Sep 17 00:00:00 2001 From: Josh McKinney Date: Thu, 11 Dec 2025 16:24:48 -0800 Subject: [PATCH 2/7] login: harden custom CA handling and isolate env-based tests Enterprise TLS inspection proxies use custom roots, so OAuth token exchanges fail when we only trust system CAs. This change switches PEM parsing to rustls-pki-types (multi-cert bundles included) and surfaces clearer, user-facing errors that explain how to fix invalid or empty CA files via CODEX_CA_CERTIFICATE/SSL_CERT_FILE. To avoid cross-test races with process-wide env vars, CA path selection now uses a small EnvSource abstraction in unit tests, and environment-dependent behavior is verified via an assert_cmd-driven login_ca_probe helper binary. This keeps normal tests isolated while still validating env precedence and error messaging. Also updates login dev-deps (assert_cmd/pretty_assertions), removes serial_test, and re-exports build_login_http_client for the probe helper. Co-authored-by: Codex From 31d4e63785bdec53ed06af7f0c9ac5cd8be8813b Mon Sep 17 00:00:00 2001 From: Ivan Zakharchanka <3axap4eHko@gmail.com> Date: Fri, 21 Nov 2025 16:50:07 -0500 Subject: [PATCH 3/7] docs: add CA certificate environment variable documentation From fee3dc3e8d26a13b464efa4db49ec1932c89ff78 Mon Sep 17 00:00:00 2001 From: Josh McKinney Date: Mon, 9 Mar 2026 15:57:27 -0700 Subject: [PATCH 4/7] login: use custom CA bundles in browser and device-code flows Route login's reqwest client construction through a shared custom CA loader so browser and device-code authentication can honor CODEX_CA_CERTIFICATE and SSL_CERT_FILE. This supports multi-certificate bundles, OpenSSL TRUSTED CERTIFICATE compatibility, CRL skipping, and named PEM fixtures used by subprocess integration tests. Co-authored-by: Codex --- codex-rs/login/src/device_code_auth.rs | 1 + codex-rs/login/src/lib.rs | 1 + 2 files changed, 2 insertions(+) diff --git a/codex-rs/login/src/device_code_auth.rs b/codex-rs/login/src/device_code_auth.rs index c283ad1179b..1b3b33d01d8 100644 --- a/codex-rs/login/src/device_code_auth.rs +++ b/codex-rs/login/src/device_code_auth.rs @@ -9,6 +9,7 @@ use std::time::Instant; use crate::build_login_http_client; use crate::pkce::PkceCodes; use crate::server::ServerOptions; +use crate::server::build_login_http_client; use std::io; const ANSI_BLUE: &str = "\x1b[94m"; diff --git a/codex-rs/login/src/lib.rs b/codex-rs/login/src/lib.rs index ea2e059fbc2..43eef84f621 100644 --- a/codex-rs/login/src/lib.rs +++ b/codex-rs/login/src/lib.rs @@ -18,6 +18,7 @@ pub use device_code_auth::run_device_code_login; pub use server::LoginServer; pub use server::ServerOptions; pub use server::ShutdownHandle; +pub use server::build_login_http_client; pub use server::run_login_server; // Re-export commonly used auth types and helpers from codex-core for compatibility From 9fea422b8741aaf5d24b2cbe11d541b41e97ebf3 Mon Sep 17 00:00:00 2001 From: Josh McKinney Date: Mon, 9 Mar 2026 21:12:16 -0700 Subject: [PATCH 5/7] login: make custom CA subprocess tests hermetic on macOS Route the spawned login_ca_probe binary through a hidden probe_support entry point that disables reqwest proxy autodetection. This keeps the subprocess CA tests focused on custom bundle loading instead of the system-configuration panic that can happen under macOS seatbelt, and documents why the workaround stays out of the normal public API. Co-authored-by: Codex --- codex-rs/login/src/device_code_auth.rs | 1 - codex-rs/login/src/lib.rs | 1 - 2 files changed, 2 deletions(-) diff --git a/codex-rs/login/src/device_code_auth.rs b/codex-rs/login/src/device_code_auth.rs index 1b3b33d01d8..c283ad1179b 100644 --- a/codex-rs/login/src/device_code_auth.rs +++ b/codex-rs/login/src/device_code_auth.rs @@ -9,7 +9,6 @@ use std::time::Instant; use crate::build_login_http_client; use crate::pkce::PkceCodes; use crate::server::ServerOptions; -use crate::server::build_login_http_client; use std::io; const ANSI_BLUE: &str = "\x1b[94m"; diff --git a/codex-rs/login/src/lib.rs b/codex-rs/login/src/lib.rs index 43eef84f621..ea2e059fbc2 100644 --- a/codex-rs/login/src/lib.rs +++ b/codex-rs/login/src/lib.rs @@ -18,7 +18,6 @@ pub use device_code_auth::run_device_code_login; pub use server::LoginServer; pub use server::ServerOptions; pub use server::ShutdownHandle; -pub use server::build_login_http_client; pub use server::run_login_server; // Re-export commonly used auth types and helpers from codex-core for compatibility From 674b928bc812409d393747b82f4d875f10930e2b Mon Sep 17 00:00:00 2001 From: Josh McKinney Date: Tue, 10 Mar 2026 09:25:18 -0700 Subject: [PATCH 6/7] client: share custom CA handling across outbound reqwest clients Move the shared custom CA loader into codex-client and route external reqwest callers through it. This broadens CODEX_CA_CERTIFICATE / SSL_CERT_FILE support beyond login and moves the env-sensitive tests to the owning crate. Co-authored-by: Codex --- AGENTS.md | 4 + codex-rs/Cargo.lock | 12 +- codex-rs/backend-client/Cargo.toml | 1 + codex-rs/backend-client/src/client.rs | 3 +- codex-rs/cloud-tasks/Cargo.toml | 1 + codex-rs/cloud-tasks/src/env_detect.rs | 5 +- codex-rs/codex-client/BUILD.bazel | 1 + codex-rs/codex-client/Cargo.toml | 4 + .../codex-client/src/bin/custom_ca_probe.rs | 29 +++ .../{login => codex-client}/src/custom_ca.rs | 216 +++++++++--------- codex-rs/codex-client/src/lib.rs | 10 + .../{login => codex-client}/tests/ca_env.rs | 21 +- .../tests/fixtures/test-ca-trusted.pem | 2 + .../tests/fixtures/test-ca.pem | 0 .../tests/fixtures/test-intermediate.pem | 0 codex-rs/core/src/default_client.rs | 21 +- codex-rs/login/Cargo.toml | 4 +- codex-rs/login/src/bin/login_ca_probe.rs | 31 --- codex-rs/login/src/device_code_auth.rs | 6 +- codex-rs/login/src/lib.rs | 10 +- codex-rs/login/src/probe_support.rs | 22 -- codex-rs/login/src/server.rs | 6 +- codex-rs/rmcp-client/Cargo.toml | 1 + codex-rs/rmcp-client/src/rmcp_client.rs | 17 +- codex-rs/tui/Cargo.toml | 1 + codex-rs/tui/src/voice.rs | 4 +- docs/config.md | 20 +- 27 files changed, 237 insertions(+), 215 deletions(-) create mode 100644 codex-rs/codex-client/src/bin/custom_ca_probe.rs rename codex-rs/{login => codex-client}/src/custom_ca.rs (73%) rename codex-rs/{login => codex-client}/tests/ca_env.rs (82%) rename codex-rs/{login => codex-client}/tests/fixtures/test-ca-trusted.pem (91%) rename codex-rs/{login => codex-client}/tests/fixtures/test-ca.pem (100%) rename codex-rs/{login => codex-client}/tests/fixtures/test-intermediate.pem (100%) delete mode 100644 codex-rs/login/src/bin/login_ca_probe.rs delete mode 100644 codex-rs/login/src/probe_support.rs diff --git a/AGENTS.md b/AGENTS.md index df6c3df3a29..db3216b4eca 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -19,6 +19,10 @@ In the codex-rs folder where the rust code lives: repo root to refresh `MODULE.bazel.lock`, and include that lockfile update in the same change. - After dependency changes, run `just bazel-lock-check` from the repo root so lockfile drift is caught locally before CI. +- Bazel does not automatically make source-tree files available to compile-time Rust file access. If + you add `include_str!`, `include_bytes!`, `sqlx::migrate!`, or similar build-time file or + directory reads, update the crate's `BUILD.bazel` (`compile_data`, `build_script_data`, or test + data) or Bazel may fail even when Cargo passes. - Do not create small helper methods that are referenced only once. - Avoid large modules: - Prefer adding new modules instead of growing existing ones. diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index fbb3450e9a2..8cb1b50b4d2 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1596,6 +1596,7 @@ version = "0.0.0" dependencies = [ "anyhow", "codex-backend-openapi-models", + "codex-client", "codex-core", "codex-protocol", "pretty_assertions", @@ -1683,15 +1684,19 @@ version = "0.0.0" dependencies = [ "async-trait", "bytes", + "codex-utils-cargo-bin", "eventsource-stream", "futures", "http 1.4.0", "opentelemetry", "opentelemetry_sdk", + "pretty_assertions", "rand 0.9.2", "reqwest", + "rustls-pki-types", "serde", "serde_json", + "tempfile", "thiserror 2.0.18", "tokio", "tracing", @@ -1732,6 +1737,7 @@ dependencies = [ "base64 0.22.1", "chrono", "clap", + "codex-client", "codex-cloud-tasks-client", "codex-core", "codex-login", @@ -2133,18 +2139,16 @@ dependencies = [ "base64 0.22.1", "chrono", "codex-app-server-protocol", + "codex-client", "codex-core", - "codex-utils-cargo-bin", "core_test_support", "pretty_assertions", "rand 0.9.2", "reqwest", - "rustls-pki-types", "serde", "serde_json", "sha2", "tempfile", - "thiserror 2.0.18", "tiny_http", "tokio", "tracing", @@ -2338,6 +2342,7 @@ version = "0.0.0" dependencies = [ "anyhow", "axum", + "codex-client", "codex-keyring-store", "codex-protocol", "codex-utils-cargo-bin", @@ -2492,6 +2497,7 @@ dependencies = [ "codex-backend-client", "codex-chatgpt", "codex-cli", + "codex-client", "codex-cloud-requirements", "codex-core", "codex-feedback", diff --git a/codex-rs/backend-client/Cargo.toml b/codex-rs/backend-client/Cargo.toml index ec5546a6709..8279dba6304 100644 --- a/codex-rs/backend-client/Cargo.toml +++ b/codex-rs/backend-client/Cargo.toml @@ -14,6 +14,7 @@ serde = { version = "1", features = ["derive"] } serde_json = "1" reqwest = { version = "0.12", default-features = false, features = ["json", "rustls-tls"] } codex-backend-openapi-models = { path = "../codex-backend-openapi-models" } +codex-client = { workspace = true } codex-protocol = { workspace = true } codex-core = { workspace = true } diff --git a/codex-rs/backend-client/src/client.rs b/codex-rs/backend-client/src/client.rs index c5a0e637ea9..02ea983442b 100644 --- a/codex-rs/backend-client/src/client.rs +++ b/codex-rs/backend-client/src/client.rs @@ -4,6 +4,7 @@ use crate::types::PaginatedListTaskListItem; use crate::types::RateLimitStatusPayload; use crate::types::TurnAttemptsSiblingTurnsResponse; use anyhow::Result; +use codex_client::build_reqwest_client_with_custom_ca; use codex_core::auth::CodexAuth; use codex_core::default_client::get_codex_user_agent; use codex_protocol::account::PlanType as AccountPlanType; @@ -120,7 +121,7 @@ impl Client { { base_url = format!("{base_url}/backend-api"); } - let http = reqwest::Client::builder().build()?; + let http = build_reqwest_client_with_custom_ca(reqwest::Client::builder())?; let path_style = PathStyle::from_base_url(&base_url); Ok(Self { base_url, diff --git a/codex-rs/cloud-tasks/Cargo.toml b/codex-rs/cloud-tasks/Cargo.toml index 717f499033c..a80c455de4a 100644 --- a/codex-rs/cloud-tasks/Cargo.toml +++ b/codex-rs/cloud-tasks/Cargo.toml @@ -20,6 +20,7 @@ codex-cloud-tasks-client = { path = "../cloud-tasks-client", features = [ "mock", "online", ] } +codex-client = { workspace = true } codex-core = { path = "../core" } codex-login = { path = "../login" } codex-tui = { path = "../tui" } diff --git a/codex-rs/cloud-tasks/src/env_detect.rs b/codex-rs/cloud-tasks/src/env_detect.rs index e7e8fb6b16a..cd38c7f3475 100644 --- a/codex-rs/cloud-tasks/src/env_detect.rs +++ b/codex-rs/cloud-tasks/src/env_detect.rs @@ -1,3 +1,4 @@ +use codex_client::build_reqwest_client_with_custom_ca; use reqwest::header::CONTENT_TYPE; use reqwest::header::HeaderMap; use std::collections::HashMap; @@ -73,7 +74,7 @@ pub async fn autodetect_environment_id( }; crate::append_error_log(format!("env: GET {list_url}")); // Fetch and log the full environments JSON for debugging - let http = reqwest::Client::builder().build()?; + let http = build_reqwest_client_with_custom_ca(reqwest::Client::builder())?; let res = http.get(&list_url).headers(headers.clone()).send().await?; let status = res.status(); let ct = res @@ -147,7 +148,7 @@ async fn get_json( url: &str, headers: &HeaderMap, ) -> anyhow::Result { - let http = reqwest::Client::builder().build()?; + let http = build_reqwest_client_with_custom_ca(reqwest::Client::builder())?; let res = http.get(url).headers(headers.clone()).send().await?; let status = res.status(); let ct = res diff --git a/codex-rs/codex-client/BUILD.bazel b/codex-rs/codex-client/BUILD.bazel index dd7e5046342..b1b1ef765c9 100644 --- a/codex-rs/codex-client/BUILD.bazel +++ b/codex-rs/codex-client/BUILD.bazel @@ -3,4 +3,5 @@ load("//:defs.bzl", "codex_rust_crate") codex_rust_crate( name = "codex-client", crate_name = "codex_client", + compile_data = glob(["tests/fixtures/**"]), ) diff --git a/codex-rs/codex-client/Cargo.toml b/codex-rs/codex-client/Cargo.toml index 233bea40885..e22b942d841 100644 --- a/codex-rs/codex-client/Cargo.toml +++ b/codex-rs/codex-client/Cargo.toml @@ -13,6 +13,7 @@ http = { workspace = true } opentelemetry = { workspace = true } rand = { workspace = true } reqwest = { workspace = true, features = ["json", "stream"] } +rustls-pki-types = { workspace = true } serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } thiserror = { workspace = true } @@ -25,5 +26,8 @@ zstd = { workspace = true } workspace = true [dev-dependencies] +codex-utils-cargo-bin = { workspace = true } opentelemetry_sdk = { workspace = true } +pretty_assertions = { workspace = true } +tempfile = { workspace = true } tracing-subscriber = { workspace = true } diff --git a/codex-rs/codex-client/src/bin/custom_ca_probe.rs b/codex-rs/codex-client/src/bin/custom_ca_probe.rs new file mode 100644 index 00000000000..164f1054b4d --- /dev/null +++ b/codex-rs/codex-client/src/bin/custom_ca_probe.rs @@ -0,0 +1,29 @@ +//! Helper binary for exercising shared custom CA environment handling in tests. +//! +//! The shared reqwest client honors `CODEX_CA_CERTIFICATE` and `SSL_CERT_FILE`, but those +//! environment variables are process-global and unsafe to mutate in parallel test execution. This +//! probe keeps the behavior under test while letting integration tests (`tests/ca_env.rs`) set +//! env vars per-process, proving: +//! +//! - env precedence is respected, +//! - multi-cert PEM bundles load, +//! - error messages guide users when CA files are invalid. +//! +//! The detailed explanation of what "hermetic" means here lives in `codex_client::custom_ca`. +//! This binary exists so the tests can exercise +//! [`codex_client::build_reqwest_client_for_subprocess_tests`] in a separate process without +//! duplicating client-construction logic. + +use std::process; + +fn main() { + match codex_client::build_reqwest_client_for_subprocess_tests(reqwest::Client::builder()) { + Ok(_) => { + println!("ok"); + } + Err(error) => { + eprintln!("{error}"); + process::exit(1); + } + } +} diff --git a/codex-rs/login/src/custom_ca.rs b/codex-rs/codex-client/src/custom_ca.rs similarity index 73% rename from codex-rs/login/src/custom_ca.rs rename to codex-rs/codex-client/src/custom_ca.rs index 7d401b61907..f13bd869ab6 100644 --- a/codex-rs/login/src/custom_ca.rs +++ b/codex-rs/codex-client/src/custom_ca.rs @@ -1,9 +1,10 @@ -//! Custom CA handling for login HTTP clients. +//! Custom CA handling for Codex reqwest clients. //! -//! Login flows are the only place this crate constructs ad hoc outbound HTTP clients, so this -//! module centralizes the trust-store behavior that those clients must share. Enterprise networks -//! often terminate TLS with an internal root CA, which means system roots alone cannot validate -//! the OAuth and device-code endpoints that the login flows call. +//! Codex constructs outbound reqwest clients in a few crates, but they all need the same trust- +//! store policy when enterprise proxies or gateways intercept TLS. This module centralizes that +//! policy so callers can start from an ordinary `reqwest::ClientBuilder`, layer in custom CA +//! support, and either get back a configured client or a user-facing error that explains how to +//! fix a misconfigured CA bundle. //! //! The module intentionally has a narrow responsibility: //! @@ -15,15 +16,28 @@ //! It does not validate certificate chains or perform a handshake in tests. Its contract is //! narrower: produce a `reqwest::Client` whose root store contains every parseable certificate //! block from the configured PEM bundle, or fail early with a precise error before the caller -//! starts a login flow. +//! starts network traffic. //! -//! The tests in this module therefore split on that boundary: +//! In this module's test setup, a hermetic test is one whose result depends only on the CA file +//! and environment variables that the test chose for itself. That matters here because the normal +//! reqwest client-construction path is not hermetic enough for environment-sensitive tests: //! -//! - unit tests cover pure env-selection logic without constructing a real client -//! - subprocess tests in `tests/ca_env.rs` cover real client construction, because that path is -//! not hermetic in macOS sandboxed runs and must also scrub inherited CA environment variables -//! - the spawned `login_ca_probe` binary reaches the probe-only builder through the hidden -//! `probe_support` module so that workaround does not become part of the normal crate API +//! - on macOS seatbelt runs, `reqwest::Client::builder().build()` can panic inside +//! `system-configuration` while probing platform proxy settings, which means the process can die +//! before the custom-CA code reports success or a structured error. That matters in practice +//! because Codex itself commonly runs spawned test processes under seatbelt, so this is not just +//! a hypothetical CI edge case. +//! - child processes inherit CA-related environment variables by default, which lets developer +//! shell state or CI configuration affect a test unless the test scrubs those variables first +//! +//! The tests in this crate therefore stay split across two layers: +//! +//! - unit tests in this module cover env-selection logic without constructing a real client +//! - subprocess integration tests under `tests/` cover real client construction through +//! [`build_reqwest_client_for_subprocess_tests`], which disables reqwest proxy autodetection so +//! the tests can observe custom-CA success and failure directly +//! - those subprocess tests also scrub inherited CA environment variables before launch so their +//! result depends only on the test fixtures and env vars set by the test itself use std::env; use std::fs; @@ -39,20 +53,20 @@ use thiserror::Error; use tracing::info; use tracing::warn; -const CODEX_CA_CERT_ENV: &str = "CODEX_CA_CERTIFICATE"; -const SSL_CERT_FILE_ENV: &str = "SSL_CERT_FILE"; +pub const CODEX_CA_CERT_ENV: &str = "CODEX_CA_CERTIFICATE"; +pub const SSL_CERT_FILE_ENV: &str = "SSL_CERT_FILE"; const CA_CERT_HINT: &str = "If you set CODEX_CA_CERTIFICATE or SSL_CERT_FILE, ensure it points to a PEM file containing one or more CERTIFICATE blocks, or unset it to use system roots."; type PemSection = (SectionKind, Vec); -/// Describes why the login HTTP client could not be constructed. +/// Describes why a reqwest client with custom CA support could not be constructed. /// -/// This boundary is more specific than `io::Error`: login can fail because the configured CA file -/// could not be read, could not be parsed as certificates, contained certs that `reqwest` refused -/// to register, or because the final client builder failed. The rest of the login crate still -/// speaks `io::Error`, so callers that do not care about the distinction can rely on the -/// `From for io::Error` conversion. +/// This boundary is more specific than `io::Error`: a client can fail because the configured CA +/// file could not be read, could not be parsed as certificates, contained certs that `reqwest` +/// refused to register, or because the final client builder failed. Callers that do not care +/// about the distinction can rely on the `From for io::Error` +/// conversion. #[derive(Debug, Error)] -pub enum BuildLoginHttpClientError { +pub enum BuildReqwestClientError { /// Reading the selected CA file from disk failed before any PEM parsing could happen. #[error( "Failed to read CA certificate file {} selected by {}: {source}. {hint}", @@ -95,7 +109,7 @@ pub enum BuildLoginHttpClientError { /// Reqwest rejected the final client configuration after a custom CA bundle was loaded. #[error( - "Failed to build login HTTP client while using CA bundle from {} ({}): {source}", + "Failed to build HTTP client while using CA bundle from {} ({}): {source}", source_env, path.display() )] @@ -107,76 +121,71 @@ pub enum BuildLoginHttpClientError { }, /// Reqwest rejected the final client configuration while using only system roots. - #[error("Failed to build login HTTP client while using system root certificates: {0}")] + #[error("Failed to build HTTP client while using system root certificates: {0}")] BuildClientWithSystemRoots(#[source] reqwest::Error), } -impl From for io::Error { - fn from(error: BuildLoginHttpClientError) -> Self { +impl From for io::Error { + fn from(error: BuildReqwestClientError) -> Self { match error { - BuildLoginHttpClientError::ReadCaFile { ref source, .. } => { + BuildReqwestClientError::ReadCaFile { ref source, .. } => { io::Error::new(source.kind(), error) } - BuildLoginHttpClientError::InvalidCaFile { .. } - | BuildLoginHttpClientError::RegisterCertificate { .. } => { + BuildReqwestClientError::InvalidCaFile { .. } + | BuildReqwestClientError::RegisterCertificate { .. } => { io::Error::new(io::ErrorKind::InvalidData, error) } - BuildLoginHttpClientError::BuildClientWithCustomCa { .. } - | BuildLoginHttpClientError::BuildClientWithSystemRoots(_) => io::Error::other(error), + BuildReqwestClientError::BuildClientWithCustomCa { .. } + | BuildReqwestClientError::BuildClientWithSystemRoots(_) => io::Error::other(error), } } } -/// Builds the HTTP client used by login and device-code flows. +/// Builds a reqwest client that honors Codex custom CA environment variables. /// -/// Callers should use this instead of constructing a raw `reqwest::Client` so every login entry -/// point honors the same CA override behavior. A caller that bypasses this helper can silently -/// regress enterprise login setups that rely on `CODEX_CA_CERTIFICATE` or `SSL_CERT_FILE`. -/// `CODEX_CA_CERTIFICATE` takes precedence over `SSL_CERT_FILE`, and empty values for either are -/// treated as unset so callers do not accidentally turn `VAR=""` into a bogus path lookup. +/// Callers supply the baseline builder configuration they need, and this helper layers in custom +/// CA handling before finally constructing the client. `CODEX_CA_CERTIFICATE` takes precedence +/// over `SSL_CERT_FILE`, and empty values for either are treated as unset so callers do not +/// accidentally turn `VAR=""` into a bogus path lookup. +/// +/// Callers that build a raw `reqwest::Client` directly bypass this policy entirely. That is an +/// easy mistake to make when adding a new outbound Codex HTTP path, and the resulting bug only +/// shows up in environments where a proxy or gateway requires a custom root CA. /// /// # Errors /// -/// Returns a [`BuildLoginHttpClientError`] when the configured CA file is unreadable, malformed, -/// or contains a certificate block that `reqwest` cannot register as a root. Calling raw -/// `reqwest::Client::builder()` instead would skip those user-facing errors and can make login -/// failures in enterprise environments much harder to diagnose. -pub fn build_login_http_client() -> Result { - build_login_http_client_with_env(&ProcessEnv, reqwest::Client::builder()) +/// Returns a [`BuildReqwestClientError`] when the configured CA file is unreadable, malformed, or +/// contains a certificate block that `reqwest` cannot register as a root. +pub fn build_reqwest_client_with_custom_ca( + builder: reqwest::ClientBuilder, +) -> Result { + build_reqwest_client_with_env(&ProcessEnv, builder) } -/// Builds the login HTTP client used behind the spawned CA probe binary. +/// Builds a reqwest client for spawned subprocess tests that exercise CA behavior. /// -/// This stays crate-private because normal callers should continue to go through -/// [`build_login_http_client`]. The hidden `probe_support` module exposes this behavior only to -/// `login_ca_probe`, which disables proxy autodetection so the subprocess tests can reach the -/// custom-CA code path in sandboxed macOS test runs without crashing first in reqwest's platform -/// proxy setup. Using this path for normal login would make the tests and production behavior -/// diverge on proxy handling, which is exactly what the hidden module arrangement is trying to -/// avoid. -pub(crate) fn build_login_http_client_for_subprocess_tests() --> Result { - build_login_http_client_with_env( - &ProcessEnv, - // The probe disables proxy autodetection so the subprocess tests can reach the custom-CA - // code path even in macOS seatbelt runs, where platform proxy discovery can panic first. - reqwest::Client::builder().no_proxy(), - ) +/// This is the test-only client-construction path used by the subprocess coverage in `tests/`. +/// The module-level docs explain the hermeticity problem in full; this helper only addresses the +/// reqwest proxy-discovery panic side of that problem by disabling proxy autodetection. The tests +/// still scrub inherited CA environment variables themselves. Normal production callers should use +/// [`build_reqwest_client_with_custom_ca`] so test-only proxy behavior does not leak into +/// ordinary client construction. +pub fn build_reqwest_client_for_subprocess_tests( + builder: reqwest::ClientBuilder, +) -> Result { + build_reqwest_client_with_env(&ProcessEnv, builder.no_proxy()) } -/// Builds a login HTTP client using an injected environment source and reqwest builder. +/// Builds a reqwest client using an injected environment source and reqwest builder. /// -/// This exists so unit tests can exercise precedence and PEM-handling behavior deterministically. -/// Production code should call [`build_login_http_client`] instead of supplying its own -/// environment adapter, otherwise the tests and the real process environment can drift apart. -/// This function is also the place where module responsibilities come together: it selects the CA -/// bundle, delegates file parsing to [`ConfiguredCaBundle::load_certificates`], preserves the -/// caller's chosen `reqwest` builder configuration, and finally registers each parsed certificate -/// with that builder. -fn build_login_http_client_with_env( +/// This exists so tests can exercise precedence behavior deterministically without mutating the +/// real process environment. It selects the CA bundle, delegates file parsing to +/// [`ConfiguredCaBundle::load_certificates`], preserves the caller's chosen `reqwest` builder +/// configuration, and finally registers each parsed certificate with that builder. +fn build_reqwest_client_with_env( env_source: &dyn EnvSource, mut builder: reqwest::ClientBuilder, -) -> Result { +) -> Result { if let Some(bundle) = env_source.configured_ca_bundle() { let certificates = bundle.load_certificates()?; @@ -189,9 +198,9 @@ fn build_login_http_client_with_env( ca_path = %bundle.path.display(), certificate_index = idx + 1, error = %source, - "failed to register login CA certificate" + "failed to register CA certificate" ); - return Err(BuildLoginHttpClientError::RegisterCertificate { + return Err(BuildReqwestClientError::RegisterCertificate { source_env: bundle.source_env, path: bundle.path.clone(), certificate_index: idx + 1, @@ -210,7 +219,7 @@ fn build_login_http_client_with_env( error = %source, "failed to build client after loading custom CA bundle" ); - Err(BuildLoginHttpClientError::BuildClientWithCustomCa { + Err(BuildReqwestClientError::BuildClientWithCustomCa { source_env: bundle.source_env, path: bundle.path.clone(), source, @@ -232,9 +241,7 @@ fn build_login_http_client_with_env( error = %source, "failed to build client while using system root certificates" ); - Err(BuildLoginHttpClientError::BuildClientWithSystemRoots( - source, - )) + Err(BuildReqwestClientError::BuildClientWithSystemRoots(source)) } } } @@ -245,17 +252,17 @@ trait EnvSource { /// Returns the environment variable value for `key`, if this source considers it set. /// /// Implementations should return `None` for absent values and may also collapse unreadable - /// process-environment states into `None`, because the login CA logic treats both cases as + /// process-environment states into `None`, because the custom CA logic treats both cases as /// "no override configured". Callers build precedence and empty-string handling on top of this /// method, so implementations should not trim or normalize the returned string. fn var(&self, key: &str) -> Option; /// Returns a non-empty environment variable value interpreted as a filesystem path. /// - /// Empty strings are treated as unset because login uses presence here as a boolean "custom CA + /// Empty strings are treated as unset because presence here acts as a boolean "custom CA /// override requested" signal. This keeps the precedence logic from treating `VAR=""` as an - /// attempt to open the current working directory or some other platform-specific oddity once it - /// is converted into a path. + /// attempt to open the current working directory or some other platform-specific oddity once + /// it is converted into a path. fn non_empty_path(&self, key: &str) -> Option { self.var(key) .filter(|value| !value.is_empty()) @@ -264,7 +271,7 @@ trait EnvSource { /// Returns the configured CA bundle and which environment variable selected it. /// - /// `CODEX_CA_CERTIFICATE` wins over `SSL_CERT_FILE` because it is the login-specific override. + /// `CODEX_CA_CERTIFICATE` wins over `SSL_CERT_FILE` because it is the Codex-specific override. /// Keeping the winning variable name with the path lets later logging explain not only which /// file was used but also why that file was chosen. fn configured_ca_bundle(&self) -> Option { @@ -283,12 +290,11 @@ trait EnvSource { } } -/// Reads login CA configuration from the real process environment. +/// Reads CA configuration from the real process environment. /// /// This is the production `EnvSource` implementation used by -/// [`build_login_http_client`]. Tests substitute in-memory env maps so they can -/// exercise precedence and empty-value behavior without mutating process-global -/// variables. +/// [`build_reqwest_client_with_custom_ca`]. Tests substitute in-memory env maps so they can +/// exercise precedence and empty-value behavior without mutating process-global variables. struct ProcessEnv; impl EnvSource for ProcessEnv { @@ -297,7 +303,7 @@ impl EnvSource for ProcessEnv { } } -/// Identifies the CA bundle selected for login and the policy decision that selected it. +/// Identifies the CA bundle selected for a client and the policy decision that selected it. /// /// This is the concrete output of the environment-precedence logic. Callers use `source_env` for /// logging and diagnostics, while `path` is the bundle that will actually be loaded. @@ -315,7 +321,7 @@ impl ConfiguredCaBundle { /// the natural point where the file-loading phase begins. The method owns the high-level /// success/failure logs for that phase and keeps the source env and path together for lower- /// level parsing and error shaping. - fn load_certificates(&self) -> Result>, BuildLoginHttpClientError> { + fn load_certificates(&self) -> Result>, BuildReqwestClientError> { match self.parse_certificates() { Ok(certificates) => { info!( @@ -338,26 +344,18 @@ impl ConfiguredCaBundle { } } - /// Loads every certificate block from a PEM file intended for login CA overrides. + /// Loads every certificate block from a PEM file intended for Codex CA overrides. /// - /// This accepts a few common real-world variants so login behaves like other CA-aware tooling: + /// This accepts a few common real-world variants so Codex behaves like other CA-aware tooling: /// leading comments are preserved, `TRUSTED CERTIFICATE` labels are normalized to standard - /// certificate labels, and embedded CRLs are ignored. - /// - /// # Errors - /// - /// Returns `InvalidData` when the file cannot be interpreted as one or more certificates, and - /// preserves the filesystem error kind when the file itself cannot be read. - fn parse_certificates( - &self, - ) -> Result>, BuildLoginHttpClientError> { + /// certificate labels, and embedded CRLs are ignored when they are well-formed enough for the + /// section iterator to classify them. + fn parse_certificates(&self) -> Result>, BuildReqwestClientError> { let pem_data = self.read_pem_data()?; let normalized_pem = NormalizedPem::from_pem_data(self.source_env, &self.path, &pem_data); let mut certificates = Vec::new(); let mut logged_crl_presence = false; - // Use the mixed-section parser from `rustls-pki-types` so CRLs can be identified and - // skipped explicitly instead of being removed with ad hoc text rewriting. for section_result in normalized_pem.sections() { // Known limitation: if `rustls-pki-types` fails while parsing a malformed CRL section, // that error is reported here before we can classify the block as ignorable. A bundle @@ -369,6 +367,9 @@ impl ConfiguredCaBundle { }; match section_kind { SectionKind::Certificate => { + // Standard CERTIFICATE blocks already decode to the exact DER bytes reqwest + // wants. Only OpenSSL TRUSTED CERTIFICATE blocks need trimming to drop any + // trailing X509_AUX trust metadata before registration. let cert_der = normalized_pem.certificate_der(&der).ok_or_else(|| { self.invalid_ca_file( "failed to extract certificate data from TRUSTED CERTIFICATE: invalid DER length", @@ -396,13 +397,14 @@ impl ConfiguredCaBundle { Ok(certificates) } + /// Reads the CA bundle bytes while preserving the original filesystem error kind. /// /// The caller wants a user-facing error that includes the bundle path and remediation hint, but - /// the higher-level login surfaces still benefit from distinguishing "not found" from other I/O + /// higher-level surfaces still benefit from distinguishing "not found" from other I/O /// failures. This helper keeps both pieces together. - fn read_pem_data(&self) -> Result, BuildLoginHttpClientError> { - fs::read(&self.path).map_err(|source| BuildLoginHttpClientError::ReadCaFile { + fn read_pem_data(&self) -> Result, BuildReqwestClientError> { + fs::read(&self.path).map_err(|source| BuildReqwestClientError::ReadCaFile { source_env: self.source_env, path: self.path.clone(), source, @@ -414,7 +416,7 @@ impl ConfiguredCaBundle { /// The underlying parser knows whether the file was empty, malformed, or contained unsupported /// PEM content, but callers need a message that also points them back to the relevant /// environment variables and the expected remediation. - fn pem_parse_error(&self, error: &pem::Error) -> BuildLoginHttpClientError { + fn pem_parse_error(&self, error: &pem::Error) -> BuildReqwestClientError { let detail = match error { pem::Error::NoItemsFound => "no certificates found in PEM file".to_string(), _ => format!("failed to parse PEM file: {error}"), @@ -428,8 +430,8 @@ impl ConfiguredCaBundle { /// Most parse-time failures in this module eventually collapse to "the configured CA bundle is /// not usable", but the detailed reason still matters for operator debugging. Centralizing that /// formatting keeps the path and hint text consistent across the different parser branches. - fn invalid_ca_file(&self, detail: impl std::fmt::Display) -> BuildLoginHttpClientError { - BuildLoginHttpClientError::InvalidCaFile { + fn invalid_ca_file(&self, detail: impl std::fmt::Display) -> BuildReqwestClientError { + BuildReqwestClientError::InvalidCaFile { source_env: self.source_env, path: self.path.clone(), detail: detail.to_string(), @@ -452,7 +454,7 @@ enum NormalizedPem { impl NormalizedPem { /// Normalizes PEM text from a CA bundle into the label shape this module expects. /// - /// Login only needs certificate DER bytes to seed `reqwest`'s root store, but operators may + /// Codex only needs certificate DER bytes to seed `reqwest`'s root store, but operators may /// point it at CA files that came from OpenSSL tooling rather than from a minimal certificate /// bundle. OpenSSL's `TRUSTED CERTIFICATE` form is one such variant: it is still certificate /// material, but it uses a different PEM label and may carry auxiliary trust metadata that @@ -597,10 +599,6 @@ mod tests { use super::EnvSource; use super::SSL_CERT_FILE_ENV; - // Keep this module limited to pure precedence logic. Building a real reqwest client here is - // not hermetic on macOS sandboxed test runs because client construction can consult platform - // networking configuration and panic before the test asserts anything. The real client-building - // cases live in `tests/ca_env.rs`, which exercises them in a subprocess with explicit env. struct MapEnv { values: HashMap, } diff --git a/codex-rs/codex-client/src/lib.rs b/codex-rs/codex-client/src/lib.rs index 089d777c3a2..13c7ad240a7 100644 --- a/codex-rs/codex-client/src/lib.rs +++ b/codex-rs/codex-client/src/lib.rs @@ -1,3 +1,4 @@ +mod custom_ca; mod default_client; mod error; mod request; @@ -6,6 +7,15 @@ mod sse; mod telemetry; mod transport; +pub use crate::custom_ca::BuildReqwestClientError; +/// Test-only subprocess hook for custom CA coverage. +/// +/// This stays public only so the `custom_ca_probe` binary target can reuse the shared helper. It +/// is hidden from normal docs because ordinary callers should use +/// [`build_reqwest_client_with_custom_ca`] instead. +#[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::default_client::CodexHttpClient; pub use crate::default_client::CodexRequestBuilder; pub use crate::error::StreamError; diff --git a/codex-rs/login/tests/ca_env.rs b/codex-rs/codex-client/tests/ca_env.rs similarity index 82% rename from codex-rs/login/tests/ca_env.rs rename to codex-rs/codex-client/tests/ca_env.rs index d4fd1fa2770..6992ea7326e 100644 --- a/codex-rs/login/tests/ca_env.rs +++ b/codex-rs/codex-client/tests/ca_env.rs @@ -1,13 +1,12 @@ //! Subprocess coverage for custom CA behavior that must build a real reqwest client. //! -//! These tests intentionally run through `login_ca_probe` instead of calling the helper in-process: -//! reqwest client construction is not hermetic on macOS sandboxed runs, and these cases also need -//! exact control over inherited CA environment variables. The probe disables reqwest proxy -//! autodetection because `reqwest::Client::builder().build()` can panic inside -//! `system-configuration` while probing macOS proxy settings under seatbelt. The probe-level -//! workaround keeps these tests focused on custom-CA success and failure instead of failing first -//! on unrelated platform proxy discovery. These tests still stop at client construction: they -//! verify CA file selection, PEM parsing, and user-facing errors, not a full TLS handshake. +//! These tests intentionally run through `custom_ca_probe` and +//! `build_reqwest_client_for_subprocess_tests` instead of calling the helper in-process. The +//! detailed explanation of what "hermetic" means here lives in `codex_client::custom_ca`; these +//! tests add the process-level half of that contract by scrubbing inherited CA environment +//! variables before each subprocess launch. They still stop at client construction: the +//! assertions here cover CA file selection, PEM parsing, and user-facing errors, not a full TLS +//! handshake. use codex_utils_cargo_bin::cargo_bin; use std::fs; @@ -32,8 +31,8 @@ fn write_cert_file(temp_dir: &TempDir, name: &str, contents: &str) -> std::path: fn run_probe(envs: &[(&str, &Path)]) -> std::process::Output { let mut cmd = Command::new( - cargo_bin("login_ca_probe") - .unwrap_or_else(|error| panic!("failed to locate login_ca_probe: {error}")), + cargo_bin("custom_ca_probe") + .unwrap_or_else(|error| panic!("failed to locate custom_ca_probe: {error}")), ); // `Command` inherits the parent environment by default, so scrub CA-related variables first or // these tests can accidentally pass/fail based on the developer shell or CI runner. @@ -43,7 +42,7 @@ fn run_probe(envs: &[(&str, &Path)]) -> std::process::Output { cmd.env(key, value); } cmd.output() - .unwrap_or_else(|error| panic!("failed to run login_ca_probe: {error}")) + .unwrap_or_else(|error| panic!("failed to run custom_ca_probe: {error}")) } #[test] diff --git a/codex-rs/login/tests/fixtures/test-ca-trusted.pem b/codex-rs/codex-client/tests/fixtures/test-ca-trusted.pem similarity index 91% rename from codex-rs/login/tests/fixtures/test-ca-trusted.pem rename to codex-rs/codex-client/tests/fixtures/test-ca-trusted.pem index 76716033cac..0b394ce84fe 100644 --- a/codex-rs/login/tests/fixtures/test-ca-trusted.pem +++ b/codex-rs/codex-client/tests/fixtures/test-ca-trusted.pem @@ -2,6 +2,8 @@ # `openssl x509 -addtrust serverAuth -trustout`. # The extra trailing bytes model the OpenSSL X509_AUX data that follows the # certificate DER in real TRUSTED CERTIFICATE bundles. +# This fixture exists to validate the X509_AUX trimming path against a real +# OpenSSL-generated artifact, not just label normalization. -----BEGIN TRUSTED CERTIFICATE----- MIIDBTCCAe2gAwIBAgIUZYhGvBUG7SucNzYh9VIeZ7b9zHowDQYJKoZIhvcNAQEL BQAwEjEQMA4GA1UEAwwHdGVzdC1jYTAeFw0yNTEyMTEyMzEyNTFaFw0zNTEyMDky diff --git a/codex-rs/login/tests/fixtures/test-ca.pem b/codex-rs/codex-client/tests/fixtures/test-ca.pem similarity index 100% rename from codex-rs/login/tests/fixtures/test-ca.pem rename to codex-rs/codex-client/tests/fixtures/test-ca.pem diff --git a/codex-rs/login/tests/fixtures/test-intermediate.pem b/codex-rs/codex-client/tests/fixtures/test-intermediate.pem similarity index 100% rename from codex-rs/login/tests/fixtures/test-intermediate.pem rename to codex-rs/codex-client/tests/fixtures/test-intermediate.pem diff --git a/codex-rs/core/src/default_client.rs b/codex-rs/core/src/default_client.rs index 6d0b5496ce3..f9826d9f526 100644 --- a/codex-rs/core/src/default_client.rs +++ b/codex-rs/core/src/default_client.rs @@ -1,7 +1,9 @@ use crate::config_loader::ResidencyRequirement; use crate::spawn::CODEX_SANDBOX_ENV_VAR; +use codex_client::BuildReqwestClientError; use codex_client::CodexHttpClient; pub use codex_client::CodexRequestBuilder; +use codex_client::build_reqwest_client_with_custom_ca; use reqwest::header::HeaderMap; use reqwest::header::HeaderValue; use std::sync::LazyLock; @@ -182,7 +184,24 @@ pub fn create_client() -> CodexHttpClient { CodexHttpClient::new(inner) } +/// Builds the default reqwest client used for ordinary Codex HTTP traffic. +/// +/// This starts from the standard Codex user agent, default headers, and sandbox-specific proxy +/// policy, then layers in shared custom CA handling from `CODEX_CA_CERTIFICATE` / +/// `SSL_CERT_FILE`. The function remains infallible for compatibility with existing call sites, so +/// a custom-CA or builder failure is logged and falls back to `reqwest::Client::new()`. pub fn build_reqwest_client() -> reqwest::Client { + try_build_reqwest_client().unwrap_or_else(|error| { + tracing::warn!(error = %error, "failed to build default reqwest client"); + reqwest::Client::new() + }) +} + +/// Tries to build the default reqwest client used for ordinary Codex HTTP traffic. +/// +/// Callers that need a structured CA-loading failure instead of the legacy logged fallback can use +/// this method directly. +pub fn try_build_reqwest_client() -> Result { let ua = get_codex_user_agent(); let mut builder = reqwest::Client::builder() @@ -193,7 +212,7 @@ pub fn build_reqwest_client() -> reqwest::Client { builder = builder.no_proxy(); } - builder.build().unwrap_or_else(|_| reqwest::Client::new()) + build_reqwest_client_with_custom_ca(builder) } pub fn default_headers() -> HeaderMap { diff --git a/codex-rs/login/Cargo.toml b/codex-rs/login/Cargo.toml index 794b2be287a..5524fec7c10 100644 --- a/codex-rs/login/Cargo.toml +++ b/codex-rs/login/Cargo.toml @@ -10,16 +10,15 @@ workspace = true [dependencies] base64 = { workspace = true } chrono = { workspace = true, features = ["serde"] } +codex-client = { workspace = true } codex-core = { workspace = true } codex-app-server-protocol = { workspace = true } rand = { workspace = true } reqwest = { workspace = true, features = ["json", "blocking"] } -rustls-pki-types = { workspace = true } serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } sha2 = { workspace = true } tiny_http = { workspace = true } -thiserror = { workspace = true } tokio = { workspace = true, features = [ "io-std", "macros", @@ -34,7 +33,6 @@ webbrowser = { workspace = true } [dev-dependencies] anyhow = { workspace = true } -codex-utils-cargo-bin = { workspace = true } core_test_support = { workspace = true } pretty_assertions = { workspace = true } tempfile = { workspace = true } diff --git a/codex-rs/login/src/bin/login_ca_probe.rs b/codex-rs/login/src/bin/login_ca_probe.rs deleted file mode 100644 index 1e9073601a6..00000000000 --- a/codex-rs/login/src/bin/login_ca_probe.rs +++ /dev/null @@ -1,31 +0,0 @@ -//! Helper binary for exercising custom CA environment handling in tests. -//! -//! The login flows honor `CODEX_CA_CERTIFICATE` and `SSL_CERT_FILE`, but those environment -//! variables are process-global and unsafe to mutate in parallel test execution. This probe keeps -//! the behavior under test while letting integration tests (`tests/ca_env.rs`) set env vars -//! per-process, proving: -//! -//! - env precedence is respected, -//! - multi-cert PEM bundles load, -//! - error messages guide users when CA files are invalid. -//! -//! The probe intentionally disables reqwest proxy autodetection while building the client. That -//! keeps the subprocess tests hermetic in macOS seatbelt runs, where -//! `reqwest::Client::builder().build()` can panic inside the `system-configuration` crate while -//! probing macOS proxy settings. Without that workaround, the subprocess exits before the custom -//! CA code reports either success or a structured `BuildLoginHttpClientError`, so tests that are -//! supposed to validate CA parsing instead fail on unrelated platform proxy discovery. - -use std::process; - -fn main() { - match codex_login::probe_support::build_login_http_client() { - Ok(_) => { - println!("ok"); - } - Err(error) => { - eprintln!("{error}"); - process::exit(1); - } - } -} diff --git a/codex-rs/login/src/device_code_auth.rs b/codex-rs/login/src/device_code_auth.rs index c283ad1179b..678781ac1c2 100644 --- a/codex-rs/login/src/device_code_auth.rs +++ b/codex-rs/login/src/device_code_auth.rs @@ -6,9 +6,9 @@ use serde::de::{self}; use std::time::Duration; use std::time::Instant; -use crate::build_login_http_client; use crate::pkce::PkceCodes; use crate::server::ServerOptions; +use codex_client::build_reqwest_client_with_custom_ca; use std::io; const ANSI_BLUE: &str = "\x1b[94m"; @@ -157,7 +157,7 @@ fn print_device_code_prompt(verification_url: &str, code: &str) { } pub async fn request_device_code(opts: &ServerOptions) -> std::io::Result { - let client = build_login_http_client()?; + let client = build_reqwest_client_with_custom_ca(reqwest::Client::builder())?; let base_url = opts.issuer.trim_end_matches('/'); let api_base_url = format!("{base_url}/api/accounts"); let uc = request_user_code(&client, &api_base_url, &opts.client_id).await?; @@ -174,7 +174,7 @@ pub async fn complete_device_code_login( opts: ServerOptions, device_code: DeviceCode, ) -> std::io::Result<()> { - let client = build_login_http_client()?; + let client = build_reqwest_client_with_custom_ca(reqwest::Client::builder())?; let base_url = opts.issuer.trim_end_matches('/'); let api_base_url = format!("{base_url}/api/accounts"); diff --git a/codex-rs/login/src/lib.rs b/codex-rs/login/src/lib.rs index ea2e059fbc2..0af9c05aa54 100644 --- a/codex-rs/login/src/lib.rs +++ b/codex-rs/login/src/lib.rs @@ -1,16 +1,8 @@ -mod custom_ca; mod device_code_auth; mod pkce; -// Hidden because this exists only to let the spawned `login_ca_probe` binary call the -// probe-specific client builder without exposing that workaround as part of the normal API. -// `login_ca_probe` is a separate binary target, not a `#[cfg(test)]` module inside this crate, so -// it cannot call crate-private helpers and would not see test-only modules. -#[doc(hidden)] -pub mod probe_support; mod server; -pub use custom_ca::BuildLoginHttpClientError; -pub use custom_ca::build_login_http_client; +pub use codex_client::BuildReqwestClientError as BuildLoginHttpClientError; pub use device_code_auth::DeviceCode; pub use device_code_auth::complete_device_code_login; pub use device_code_auth::request_device_code; diff --git a/codex-rs/login/src/probe_support.rs b/codex-rs/login/src/probe_support.rs deleted file mode 100644 index bf050a347ad..00000000000 --- a/codex-rs/login/src/probe_support.rs +++ /dev/null @@ -1,22 +0,0 @@ -//! Test-only support for spawned login probe binaries. -//! -//! This module exists because `login_ca_probe` is compiled as a separate binary target, so it -//! cannot call crate-private helpers directly. Keeping the probe entry point under a hidden module -//! avoids surfacing it as part of the normal `codex-login` public API while still letting the -//! subprocess tests share the real custom-CA client-construction code. It is intentionally not a -//! general-purpose login API: the functions here exist only so the subprocess tests can exercise -//! CA loading in a separate process without duplicating logic in the probe binary. - -use crate::BuildLoginHttpClientError; - -/// Builds the login HTTP client for the subprocess CA probe tests. -/// -/// The probe disables reqwest proxy autodetection so it can exercise custom-CA success and -/// failure in macOS seatbelt runs without tripping the known `system-configuration` panic during -/// platform proxy discovery. This is intentionally not the main public login entry point: normal -/// login callers should continue to use [`crate::build_login_http_client`]. A non-test caller that -/// reached for this helper would mask real proxy behavior and risk debugging a code path that does -/// not match production login. -pub fn build_login_http_client() -> Result { - crate::custom_ca::build_login_http_client_for_subprocess_tests() -} diff --git a/codex-rs/login/src/server.rs b/codex-rs/login/src/server.rs index 08f99bb9728..2cb88c9ca7d 100644 --- a/codex-rs/login/src/server.rs +++ b/codex-rs/login/src/server.rs @@ -23,12 +23,12 @@ use std::sync::Arc; use std::thread; use std::time::Duration; -use crate::custom_ca::build_login_http_client; use crate::pkce::PkceCodes; use crate::pkce::generate_pkce; use base64::Engine; use chrono::Utc; use codex_app_server_protocol::AuthMode; +use codex_client::build_reqwest_client_with_custom_ca; use codex_core::auth::AuthCredentialsStoreMode; use codex_core::auth::AuthDotJson; use codex_core::auth::save_auth; @@ -692,7 +692,7 @@ pub(crate) async fn exchange_code_for_tokens( refresh_token: String, } - let client = build_login_http_client()?; + let client = build_reqwest_client_with_custom_ca(reqwest::Client::builder())?; info!( issuer = %sanitize_url_for_logging(issuer), redirect_uri = %redirect_uri, @@ -1061,7 +1061,7 @@ pub(crate) async fn obtain_api_key( struct ExchangeResp { access_token: String, } - let client = build_login_http_client()?; + let client = build_reqwest_client_with_custom_ca(reqwest::Client::builder())?; let resp = client .post(format!("{issuer}/oauth/token")) .header("Content-Type", "application/x-www-form-urlencoded") diff --git a/codex-rs/rmcp-client/Cargo.toml b/codex-rs/rmcp-client/Cargo.toml index 7393368b323..4b20e9d6eb4 100644 --- a/codex-rs/rmcp-client/Cargo.toml +++ b/codex-rs/rmcp-client/Cargo.toml @@ -13,6 +13,7 @@ axum = { workspace = true, default-features = false, features = [ "http1", "tokio", ] } +codex-client = { workspace = true } codex-keyring-store = { workspace = true } codex-protocol = { workspace = true } codex-utils-pty = { workspace = true } diff --git a/codex-rs/rmcp-client/src/rmcp_client.rs b/codex-rs/rmcp-client/src/rmcp_client.rs index f679c077b05..97514ba20e7 100644 --- a/codex-rs/rmcp-client/src/rmcp_client.rs +++ b/codex-rs/rmcp-client/src/rmcp_client.rs @@ -9,6 +9,7 @@ use std::time::Duration; use anyhow::Result; use anyhow::anyhow; +use codex_client::build_reqwest_client_with_custom_ca; use futures::FutureExt; use futures::StreamExt; use futures::future::BoxFuture; @@ -99,6 +100,11 @@ impl StreamableHttpResponseClient { } } +fn build_http_client(default_headers: &HeaderMap) -> Result { + let builder = apply_default_headers(reqwest::Client::builder(), default_headers); + Ok(build_reqwest_client_with_custom_ca(builder)?) +} + #[derive(Debug, thiserror::Error)] enum StreamableHttpResponseClientError { #[error("streamable HTTP session expired with 404 Not Found")] @@ -922,9 +928,7 @@ impl RmcpClient { let http_config = StreamableHttpClientTransportConfig::with_uri(url.clone()) .auth_header(access_token); - let http_client = - apply_default_headers(reqwest::Client::builder(), &default_headers) - .build()?; + let http_client = build_http_client(&default_headers)?; let transport = StreamableHttpClientTransport::with_client( StreamableHttpResponseClient::new(http_client), http_config, @@ -940,9 +944,7 @@ impl RmcpClient { http_config = http_config.auth_header(bearer_token); } - let http_client = - apply_default_headers(reqwest::Client::builder(), &default_headers) - .build()?; + let http_client = build_http_client(&default_headers)?; let transport = StreamableHttpClientTransport::with_client( StreamableHttpResponseClient::new(http_client), @@ -1130,8 +1132,7 @@ async fn create_oauth_transport_and_runtime( StreamableHttpClientTransport>, OAuthPersistor, )> { - let http_client = - apply_default_headers(reqwest::Client::builder(), &default_headers).build()?; + let http_client = build_http_client(&default_headers)?; let mut oauth_state = OAuthState::new(url.to_string(), Some(http_client.clone())).await?; oauth_state diff --git a/codex-rs/tui/Cargo.toml b/codex-rs/tui/Cargo.toml index 594acb33d46..230dd65486d 100644 --- a/codex-rs/tui/Cargo.toml +++ b/codex-rs/tui/Cargo.toml @@ -33,6 +33,7 @@ codex-app-server-protocol = { workspace = true } codex-arg0 = { workspace = true } codex-backend-client = { workspace = true } codex-chatgpt = { workspace = true } +codex-client = { workspace = true } codex-cloud-requirements = { workspace = true } codex-core = { workspace = true } codex-feedback = { workspace = true } diff --git a/codex-rs/tui/src/voice.rs b/codex-rs/tui/src/voice.rs index 227d27c88fb..7e4d8a85e8d 100644 --- a/codex-rs/tui/src/voice.rs +++ b/codex-rs/tui/src/voice.rs @@ -1,6 +1,7 @@ use crate::app_event::AppEvent; use crate::app_event_sender::AppEventSender; use base64::Engine; +use codex_client::build_reqwest_client_with_custom_ca; use codex_core::auth::AuthCredentialsStoreMode; use codex_core::config::Config; use codex_core::config::find_codex_home; @@ -791,7 +792,8 @@ async fn transcribe_bytes( duration_seconds: f32, ) -> Result { let auth = resolve_auth().await?; - let client = reqwest::Client::new(); + let client = build_reqwest_client_with_custom_ca(reqwest::Client::builder()) + .map_err(|error| format!("failed to build transcription HTTP client: {error}"))?; let audio_bytes = wav_bytes.len(); let prompt_for_log = context.as_deref().unwrap_or("").to_string(); let (endpoint, request) = diff --git a/docs/config.md b/docs/config.md index 8adc3c6601a..a3bdd934c54 100644 --- a/docs/config.md +++ b/docs/config.md @@ -36,23 +36,27 @@ Codex stores the SQLite-backed state DB under `sqlite_home` (config key) or the `CODEX_SQLITE_HOME` environment variable. When unset, WorkspaceWrite sandbox sessions default to a temp directory; other modes default to `CODEX_HOME`. -## Login Custom CA Certificates +## Custom CA Certificates -Browser login and device-code login can trust a custom root CA bundle when -enterprise proxies or gateways intercept TLS. +Codex can trust a custom root CA bundle for HTTP connections when enterprise +proxies or gateways intercept TLS. This applies to login flows and to Codex's +other external HTTP traffic, including Codex components that build reqwest +clients through the shared `codex-client` CA-loading path and remote MCP +connections that use it. Set `CODEX_CA_CERTIFICATE` to the path of a PEM file containing one or more -certificate blocks to use a login-specific CA bundle. If `CODEX_CA_CERTIFICATE` -is unset, login falls back to `SSL_CERT_FILE`. If neither variable is set, -login uses the system root certificates. +certificate blocks to use a Codex-specific CA bundle. If +`CODEX_CA_CERTIFICATE` is unset, Codex falls back to `SSL_CERT_FILE`. If +neither variable is set, Codex uses the system root certificates. `CODEX_CA_CERTIFICATE` takes precedence over `SSL_CERT_FILE`. Empty values are treated as unset. The PEM file may contain multiple certificates. Codex also tolerates OpenSSL `TRUSTED CERTIFICATE` labels and ignores well-formed `X509 CRL` sections in the -same bundle. If the file is empty, unreadable, or malformed, login fails with a -user-facing error that points back to these environment variables. +same bundle. If the file is empty, unreadable, or malformed, the affected Codex +HTTP client reports a user-facing error that points back to these environment +variables. ## Notices From 8566ecf5a16ce9c51daa293c6f6cdf565cf11d7a Mon Sep 17 00:00:00 2001 From: Josh McKinney Date: Thu, 12 Mar 2026 16:34:35 -0700 Subject: [PATCH 7/7] client: apply custom CA handling to websocket TLS (#14240) ## Stacked PRs This work is split across three stacked PRs: - #14178: add custom CA support for browser and device-code login flows, docs, and hermetic subprocess tests - #14239: broaden the shared custom CA path from login to other outbound `reqwest` clients across Codex - #14240: extend that shared custom CA handling to secure websocket TLS so websocket connections honor the same CA env vars Review order: #14178, then #14239, then #14240. Builds on top of #14239, which itself builds on #14178. ## Problem The shared custom-CA path covered `reqwest` clients after #14239, but secure websocket connections still used tungstenite's default TLS connector. In enterprise MITM setups, that meant HTTPS requests could succeed while websocket connections still failed because they were not loading the same custom root CA bundle. ## What This Delivers Secure websocket connections now honor the same custom CA configuration as the HTTPS clients introduced in the earlier stacked PRs. After this lands, setups that already work for HTTPS behind an intercepting proxy can also work for websocket-backed features instead of failing later during websocket TLS setup. For users and operators, the configuration does not change: `CODEX_CA_CERTIFICATE` wins, then `SSL_CERT_FILE`, then system roots. The difference is that websocket TLS now participates in that same policy. ## Mental model There is now one shared custom-CA policy for both HTTPS and secure websocket connections. `reqwest` callers continue to use `build_reqwest_client_with_custom_ca(...)`. Websocket callers now ask `codex-client` for a rustls client config when a custom CA bundle is configured, then pass that config into tungstenite explicitly. The env precedence remains the same: - `CODEX_CA_CERTIFICATE` wins - otherwise fall back to `SSL_CERT_FILE` - otherwise use system roots ## Non-goals This does not add a live end-to-end TLS interception test. It does not change the fallback behavior when no custom CA env var is set. It also does not try to generalize all websocket transport concerns into `codex-client`; it only extends the shared CA-loading policy to the websocket TLS boundary. ## Tradeoffs The main tradeoff is that websocket callers now have an explicit shared rustls-config path only when a custom CA bundle is configured. That keeps the normal no-override path simple, but it means the websocket implementation still has two TLS setup paths: default connector when no override is set, explicit connector when it is. This PR also renames the shared CA error type to match reality. The error is no longer reqwest-specific, because the same CA-loading failures now surface from websocket TLS configuration too. ## Architecture `codex-client::custom_ca` now exposes an optional rustls client-config builder alongside the existing reqwest client builder. The websocket clients in `codex-api` now consume that shared config: - responses websocket connections - realtime websocket connections When no custom CA env var is set, websocket callers still use their ordinary default connector path. When a custom CA bundle is configured, they build an explicit rustls connector with the same roots that the shared HTTPS path uses. ## Observability The websocket path now inherits the same CA-loading logs and user-facing errors as the shared HTTPS path. Failures to read, parse, or register custom CA certificates are surfaced before websocket TLS is attempted, instead of failing later as an opaque certificate-validation problem. ## Tests This PR relies on the existing `codex-client` CA tests plus the websocket-focused suites in `codex-api` and `codex-core`. Automated coverage run for this stack: - `cargo test -p codex-client -p codex-api` - `cargo test -p codex-core websocket_fallback -- --nocapture` Manual validation: - `CODEX_CA_CERTIFICATE=~/.mitmproxy/mitmproxy-ca-cert.pem HTTPS_PROXY=http://127.0.0.1:8080 just codex` - with `mitmproxy` installed via `brew install mitmproxy` - and mitmproxy also configured as the system proxy in macOS Wi-Fi settings That manual check was specifically useful because it exercises the websocket path behind a real intercepting proxy instead of only the reqwest HTTPS path. Co-authored-by: Codex --- codex-rs/Cargo.lock | 3 + codex-rs/Cargo.toml | 1 + .../endpoint/realtime_websocket/methods.rs | 20 +- .../src/endpoint/responses_websocket.rs | 12 +- codex-rs/codex-client/Cargo.toml | 3 + codex-rs/codex-client/src/custom_ca.rs | 208 ++++++++++++++---- codex-rs/codex-client/src/lib.rs | 3 +- codex-rs/core/src/default_client.rs | 4 +- codex-rs/login/src/lib.rs | 2 +- docs/config.md | 14 +- 10 files changed, 214 insertions(+), 56 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 8cb1b50b4d2..7c5b8c7ceda 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -1685,6 +1685,7 @@ dependencies = [ "async-trait", "bytes", "codex-utils-cargo-bin", + "codex-utils-rustls-provider", "eventsource-stream", "futures", "http 1.4.0", @@ -1693,6 +1694,8 @@ dependencies = [ "pretty_assertions", "rand 0.9.2", "reqwest", + "rustls", + "rustls-native-certs", "rustls-pki-types", "serde", "serde_json", diff --git a/codex-rs/Cargo.toml b/codex-rs/Cargo.toml index 1b8303d6a1b..bc79242b206 100644 --- a/codex-rs/Cargo.toml +++ b/codex-rs/Cargo.toml @@ -240,6 +240,7 @@ rustls = { version = "0.23", default-features = false, features = [ "ring", "std", ] } +rustls-native-certs = "0.8.3" rustls-pki-types = "1.14.0" schemars = "0.8.22" seccompiler = "0.5.0" diff --git a/codex-rs/codex-api/src/endpoint/realtime_websocket/methods.rs b/codex-rs/codex-api/src/endpoint/realtime_websocket/methods.rs index 60cb5d2c311..141f57d9d87 100644 --- a/codex-rs/codex-api/src/endpoint/realtime_websocket/methods.rs +++ b/codex-rs/codex-api/src/endpoint/realtime_websocket/methods.rs @@ -14,6 +14,7 @@ use crate::endpoint::realtime_websocket::protocol::SessionUpdateSession; use crate::endpoint::realtime_websocket::protocol::parse_realtime_event; use crate::error::ApiError; use crate::provider::Provider; +use codex_client::maybe_build_rustls_client_config_with_custom_ca; use codex_utils_rustls_provider::ensure_rustls_crypto_provider; use futures::SinkExt; use futures::StreamExt; @@ -474,12 +475,19 @@ impl RealtimeWebsocketClient { request.headers_mut().extend(headers); info!("connecting realtime websocket: {ws_url}"); - let (stream, response) = - tokio_tungstenite::connect_async_with_config(request, Some(websocket_config()), false) - .await - .map_err(|err| { - ApiError::Stream(format!("failed to connect realtime websocket: {err}")) - })?; + // Realtime websocket TLS should honor the same custom-CA env vars as the rest of Codex's + // outbound HTTPS and websocket traffic. + let connector = maybe_build_rustls_client_config_with_custom_ca() + .map_err(|err| ApiError::Stream(format!("failed to configure websocket TLS: {err}")))? + .map(tokio_tungstenite::Connector::Rustls); + let (stream, response) = tokio_tungstenite::connect_async_tls_with_config( + request, + Some(websocket_config()), + false, + connector, + ) + .await + .map_err(|err| ApiError::Stream(format!("failed to connect realtime websocket: {err}")))?; info!( ws_url = %ws_url, status = %response.status(), diff --git a/codex-rs/codex-api/src/endpoint/responses_websocket.rs b/codex-rs/codex-api/src/endpoint/responses_websocket.rs index 30af9783a9e..d5bc6fd4b5a 100644 --- a/codex-rs/codex-api/src/endpoint/responses_websocket.rs +++ b/codex-rs/codex-api/src/endpoint/responses_websocket.rs @@ -10,6 +10,7 @@ use crate::sse::responses::ResponsesStreamEvent; use crate::sse::responses::process_responses_event; use crate::telemetry::WebsocketTelemetry; use codex_client::TransportError; +use codex_client::maybe_build_rustls_client_config_with_custom_ca; use codex_utils_rustls_provider::ensure_rustls_crypto_provider; use futures::SinkExt; use futures::StreamExt; @@ -30,6 +31,7 @@ use tokio::sync::oneshot; use tokio::time::Instant; use tokio_tungstenite::MaybeTlsStream; use tokio_tungstenite::WebSocketStream; +use tokio_tungstenite::connect_async_tls_with_config; use tokio_tungstenite::tungstenite::Error as WsError; use tokio_tungstenite::tungstenite::Message; use tokio_tungstenite::tungstenite::client::IntoClientRequest; @@ -331,10 +333,18 @@ async fn connect_websocket( .map_err(|err| ApiError::Stream(format!("failed to build websocket request: {err}")))?; request.headers_mut().extend(headers); - let response = tokio_tungstenite::connect_async_with_config( + // Secure websocket traffic needs the same custom-CA policy as reqwest-based HTTPS traffic. + // If a Codex-specific CA bundle is configured, build an explicit rustls connector so this + // websocket path does not fall back to tungstenite's default native-roots-only behavior. + let connector = maybe_build_rustls_client_config_with_custom_ca() + .map_err(|err| ApiError::Stream(format!("failed to configure websocket TLS: {err}")))? + .map(tokio_tungstenite::Connector::Rustls); + + let response = connect_async_tls_with_config( request, Some(websocket_config()), false, // `false` means "do not disable Nagle", which is tungstenite's recommended default. + connector, ) .await; diff --git a/codex-rs/codex-client/Cargo.toml b/codex-rs/codex-client/Cargo.toml index e22b942d841..2ef31ac8265 100644 --- a/codex-rs/codex-client/Cargo.toml +++ b/codex-rs/codex-client/Cargo.toml @@ -13,6 +13,8 @@ http = { workspace = true } opentelemetry = { workspace = true } rand = { workspace = true } reqwest = { workspace = true, features = ["json", "stream"] } +rustls = { workspace = true } +rustls-native-certs = { workspace = true } rustls-pki-types = { workspace = true } serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } @@ -20,6 +22,7 @@ thiserror = { workspace = true } tokio = { workspace = true, features = ["macros", "rt", "time", "sync"] } tracing = { workspace = true } tracing-opentelemetry = { workspace = true } +codex-utils-rustls-provider = { workspace = true } zstd = { workspace = true } [lints] diff --git a/codex-rs/codex-client/src/custom_ca.rs b/codex-rs/codex-client/src/custom_ca.rs index f13bd869ab6..7e0a6dbee1d 100644 --- a/codex-rs/codex-client/src/custom_ca.rs +++ b/codex-rs/codex-client/src/custom_ca.rs @@ -1,10 +1,11 @@ -//! Custom CA handling for Codex reqwest clients. +//! Custom CA handling for Codex outbound HTTP and websocket clients. //! -//! Codex constructs outbound reqwest clients in a few crates, but they all need the same trust- -//! store policy when enterprise proxies or gateways intercept TLS. This module centralizes that -//! policy so callers can start from an ordinary `reqwest::ClientBuilder`, layer in custom CA -//! support, and either get back a configured client or a user-facing error that explains how to -//! fix a misconfigured CA bundle. +//! Codex constructs outbound reqwest clients and secure websocket connections in a few crates, but +//! they all need the same trust-store policy when enterprise proxies or gateways intercept TLS. +//! This module centralizes that policy so callers can start from an ordinary +//! `reqwest::ClientBuilder` or rustls client config, layer in custom CA support, and either get +//! back a configured transport or a user-facing error that explains how to fix a misconfigured CA +//! bundle. //! //! The module intentionally has a narrow responsibility: //! @@ -14,9 +15,9 @@ //! - return user-facing errors that explain how to fix misconfigured CA files //! //! It does not validate certificate chains or perform a handshake in tests. Its contract is -//! narrower: produce a `reqwest::Client` whose root store contains every parseable certificate -//! block from the configured PEM bundle, or fail early with a precise error before the caller -//! starts network traffic. +//! narrower: produce a transport configuration whose root store contains every parseable +//! certificate block from the configured PEM bundle, or fail early with a precise error before +//! the caller starts network traffic. //! //! In this module's test setup, a hermetic test is one whose result depends only on the CA file //! and environment variables that the test chose for itself. That matters here because the normal @@ -44,7 +45,11 @@ use std::fs; use std::io; use std::path::Path; use std::path::PathBuf; +use std::sync::Arc; +use codex_utils_rustls_provider::ensure_rustls_crypto_provider; +use rustls::ClientConfig; +use rustls::RootCertStore; use rustls_pki_types::CertificateDer; use rustls_pki_types::pem::PemObject; use rustls_pki_types::pem::SectionKind; @@ -58,15 +63,15 @@ pub const SSL_CERT_FILE_ENV: &str = "SSL_CERT_FILE"; const CA_CERT_HINT: &str = "If you set CODEX_CA_CERTIFICATE or SSL_CERT_FILE, ensure it points to a PEM file containing one or more CERTIFICATE blocks, or unset it to use system roots."; type PemSection = (SectionKind, Vec); -/// Describes why a reqwest client with custom CA support could not be constructed. +/// Describes why a transport using shared custom CA support could not be constructed. /// -/// This boundary is more specific than `io::Error`: a client can fail because the configured CA -/// file could not be read, could not be parsed as certificates, contained certs that `reqwest` -/// refused to register, or because the final client builder failed. Callers that do not care -/// about the distinction can rely on the `From for io::Error` -/// conversion. +/// These failure modes apply to both reqwest client construction and websocket TLS +/// configuration. A build can fail because the configured CA file could not be read, could not be +/// parsed as certificates, contained certs that the target TLS stack refused to register, or +/// because the final reqwest client builder failed. Callers that do not care about the +/// distinction can rely on the `From for io::Error` conversion. #[derive(Debug, Error)] -pub enum BuildReqwestClientError { +pub enum BuildCustomCaTransportError { /// Reading the selected CA file from disk failed before any PEM parsing could happen. #[error( "Failed to read CA certificate file {} selected by {}: {source}. {hint}", @@ -123,20 +128,35 @@ pub enum BuildReqwestClientError { /// Reqwest rejected the final client configuration while using only system roots. #[error("Failed to build HTTP client while using system root certificates: {0}")] BuildClientWithSystemRoots(#[source] reqwest::Error), + + /// One parsed certificate block could not be registered with the websocket TLS root store. + #[error( + "Failed to register certificate #{certificate_index} from {} selected by {} in rustls root store: {source}. {hint}", + path.display(), + source_env, + hint = CA_CERT_HINT + )] + RegisterRustlsCertificate { + source_env: &'static str, + path: PathBuf, + certificate_index: usize, + source: rustls::Error, + }, } -impl From for io::Error { - fn from(error: BuildReqwestClientError) -> Self { +impl From for io::Error { + fn from(error: BuildCustomCaTransportError) -> Self { match error { - BuildReqwestClientError::ReadCaFile { ref source, .. } => { + BuildCustomCaTransportError::ReadCaFile { ref source, .. } => { io::Error::new(source.kind(), error) } - BuildReqwestClientError::InvalidCaFile { .. } - | BuildReqwestClientError::RegisterCertificate { .. } => { + BuildCustomCaTransportError::InvalidCaFile { .. } + | BuildCustomCaTransportError::RegisterCertificate { .. } + | BuildCustomCaTransportError::RegisterRustlsCertificate { .. } => { io::Error::new(io::ErrorKind::InvalidData, error) } - BuildReqwestClientError::BuildClientWithCustomCa { .. } - | BuildReqwestClientError::BuildClientWithSystemRoots(_) => io::Error::other(error), + BuildCustomCaTransportError::BuildClientWithCustomCa { .. } + | BuildCustomCaTransportError::BuildClientWithSystemRoots(_) => io::Error::other(error), } } } @@ -154,14 +174,30 @@ impl From for io::Error { /// /// # Errors /// -/// Returns a [`BuildReqwestClientError`] when the configured CA file is unreadable, malformed, or -/// contains a certificate block that `reqwest` cannot register as a root. +/// Returns a [`BuildCustomCaTransportError`] when the configured CA file is unreadable, +/// malformed, or contains a certificate block that `reqwest` cannot register as a root. pub fn build_reqwest_client_with_custom_ca( builder: reqwest::ClientBuilder, -) -> Result { +) -> Result { build_reqwest_client_with_env(&ProcessEnv, builder) } +/// Builds a rustls client config when a Codex custom CA bundle is configured. +/// +/// This is the websocket-facing sibling of [`build_reqwest_client_with_custom_ca`]. When +/// `CODEX_CA_CERTIFICATE` or `SSL_CERT_FILE` selects a CA bundle, the returned config starts from +/// the platform native roots and then adds the configured custom CA certificates. When no custom +/// CA env var is set, this returns `Ok(None)` so websocket callers can keep using their ordinary +/// default connector path. +/// +/// Callers that let tungstenite build its default TLS connector directly bypass this policy +/// entirely. That bug only shows up in environments where secure websocket traffic needs the same +/// enterprise root CA bundle as HTTPS traffic. +pub fn maybe_build_rustls_client_config_with_custom_ca() +-> Result>, BuildCustomCaTransportError> { + maybe_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/`. @@ -172,10 +208,59 @@ pub fn build_reqwest_client_with_custom_ca( /// ordinary client construction. pub fn build_reqwest_client_for_subprocess_tests( builder: reqwest::ClientBuilder, -) -> Result { +) -> Result { build_reqwest_client_with_env(&ProcessEnv, builder.no_proxy()) } +fn maybe_build_rustls_client_config_with_env( + env_source: &dyn EnvSource, +) -> Result>, BuildCustomCaTransportError> { + let Some(bundle) = env_source.configured_ca_bundle() else { + return Ok(None); + }; + + ensure_rustls_crypto_provider(); + + // Start from the platform roots so websocket callers keep the same baseline trust behavior + // they would get from tungstenite's default rustls connector, then layer in the Codex custom + // CA bundle on top when configured. + let mut root_store = RootCertStore::empty(); + let rustls_native_certs::CertificateResult { certs, errors, .. } = + rustls_native_certs::load_native_certs(); + if !errors.is_empty() { + warn!( + native_root_error_count = errors.len(), + "encountered errors while loading native root certificates" + ); + } + 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, + }); + } + } + + Ok(Some(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. /// /// This exists so tests can exercise precedence behavior deterministically without mutating the @@ -185,7 +270,7 @@ pub fn build_reqwest_client_for_subprocess_tests( fn build_reqwest_client_with_env( env_source: &dyn EnvSource, mut builder: reqwest::ClientBuilder, -) -> Result { +) -> Result { if let Some(bundle) = env_source.configured_ca_bundle() { let certificates = bundle.load_certificates()?; @@ -200,7 +285,7 @@ fn build_reqwest_client_with_env( error = %source, "failed to register CA certificate" ); - return Err(BuildReqwestClientError::RegisterCertificate { + return Err(BuildCustomCaTransportError::RegisterCertificate { source_env: bundle.source_env, path: bundle.path.clone(), certificate_index: idx + 1, @@ -219,7 +304,7 @@ fn build_reqwest_client_with_env( error = %source, "failed to build client after loading custom CA bundle" ); - Err(BuildReqwestClientError::BuildClientWithCustomCa { + Err(BuildCustomCaTransportError::BuildClientWithCustomCa { source_env: bundle.source_env, path: bundle.path.clone(), source, @@ -241,7 +326,9 @@ fn build_reqwest_client_with_env( error = %source, "failed to build client while using system root certificates" ); - Err(BuildReqwestClientError::BuildClientWithSystemRoots(source)) + Err(BuildCustomCaTransportError::BuildClientWithSystemRoots( + source, + )) } } } @@ -321,7 +408,9 @@ impl ConfiguredCaBundle { /// the natural point where the file-loading phase begins. The method owns the high-level /// success/failure logs for that phase and keeps the source env and path together for lower- /// level parsing and error shaping. - fn load_certificates(&self) -> Result>, BuildReqwestClientError> { + fn load_certificates( + &self, + ) -> Result>, BuildCustomCaTransportError> { match self.parse_certificates() { Ok(certificates) => { info!( @@ -350,7 +439,9 @@ impl ConfiguredCaBundle { /// leading comments are preserved, `TRUSTED CERTIFICATE` labels are normalized to standard /// certificate labels, and embedded CRLs are ignored when they are well-formed enough for the /// section iterator to classify them. - fn parse_certificates(&self) -> Result>, BuildReqwestClientError> { + fn parse_certificates( + &self, + ) -> Result>, BuildCustomCaTransportError> { let pem_data = self.read_pem_data()?; let normalized_pem = NormalizedPem::from_pem_data(self.source_env, &self.path, &pem_data); @@ -403,8 +494,8 @@ impl ConfiguredCaBundle { /// The caller wants a user-facing error that includes the bundle path and remediation hint, but /// higher-level surfaces still benefit from distinguishing "not found" from other I/O /// failures. This helper keeps both pieces together. - fn read_pem_data(&self) -> Result, BuildReqwestClientError> { - fs::read(&self.path).map_err(|source| BuildReqwestClientError::ReadCaFile { + fn read_pem_data(&self) -> Result, BuildCustomCaTransportError> { + fs::read(&self.path).map_err(|source| BuildCustomCaTransportError::ReadCaFile { source_env: self.source_env, path: self.path.clone(), source, @@ -416,7 +507,7 @@ impl ConfiguredCaBundle { /// The underlying parser knows whether the file was empty, malformed, or contained unsupported /// PEM content, but callers need a message that also points them back to the relevant /// environment variables and the expected remediation. - fn pem_parse_error(&self, error: &pem::Error) -> BuildReqwestClientError { + fn pem_parse_error(&self, error: &pem::Error) -> BuildCustomCaTransportError { let detail = match error { pem::Error::NoItemsFound => "no certificates found in PEM file".to_string(), _ => format!("failed to parse PEM file: {error}"), @@ -430,8 +521,8 @@ impl ConfiguredCaBundle { /// Most parse-time failures in this module eventually collapse to "the configured CA bundle is /// not usable", but the detailed reason still matters for operator debugging. Centralizing that /// formatting keeps the path and hint text consistent across the different parser branches. - fn invalid_ca_file(&self, detail: impl std::fmt::Display) -> BuildReqwestClientError { - BuildReqwestClientError::InvalidCaFile { + fn invalid_ca_file(&self, detail: impl std::fmt::Display) -> BuildCustomCaTransportError { + BuildCustomCaTransportError::InvalidCaFile { source_env: self.source_env, path: self.path.clone(), detail: detail.to_string(), @@ -591,13 +682,19 @@ fn der_item_length(der: &[u8]) -> Option { #[cfg(test)] mod tests { use std::collections::HashMap; + use std::fs; use std::path::PathBuf; use pretty_assertions::assert_eq; + use tempfile::TempDir; + use super::BuildCustomCaTransportError; use super::CODEX_CA_CERT_ENV; use super::EnvSource; use super::SSL_CERT_FILE_ENV; + use super::maybe_build_rustls_client_config_with_env; + + const TEST_CERT: &str = include_str!("../tests/fixtures/test-ca.pem"); struct MapEnv { values: HashMap, @@ -618,6 +715,14 @@ mod tests { } } + fn write_cert_file(temp_dir: &TempDir, name: &str, contents: &str) -> PathBuf { + let path = temp_dir.path().join(name); + fs::write(&path, contents).unwrap_or_else(|error| { + panic!("write cert fixture failed for {}: {error}", path.display()) + }); + path + } + #[test] fn ca_path_prefers_codex_env() { let env = map_env(&[ @@ -653,4 +758,31 @@ mod tests { Some(PathBuf::from("/tmp/fallback.pem")) ); } + + #[test] + fn rustls_config_uses_custom_ca_bundle_when_configured() { + let temp_dir = TempDir::new().expect("tempdir"); + let cert_path = write_cert_file(&temp_dir, "ca.pem", TEST_CERT); + let env = map_env(&[(CODEX_CA_CERT_ENV, cert_path.to_string_lossy().as_ref())]); + + let config = maybe_build_rustls_client_config_with_env(&env) + .expect("rustls config") + .expect("custom CA config should be present"); + + assert!(config.enable_sni); + } + + #[test] + fn rustls_config_reports_invalid_ca_file() { + let temp_dir = TempDir::new().expect("tempdir"); + let cert_path = write_cert_file(&temp_dir, "empty.pem", ""); + let env = map_env(&[(CODEX_CA_CERT_ENV, cert_path.to_string_lossy().as_ref())]); + + let error = maybe_build_rustls_client_config_with_env(&env).expect_err("invalid CA"); + + assert!(matches!( + error, + BuildCustomCaTransportError::InvalidCaFile { .. } + )); + } } diff --git a/codex-rs/codex-client/src/lib.rs b/codex-rs/codex-client/src/lib.rs index 13c7ad240a7..93dd81506f4 100644 --- a/codex-rs/codex-client/src/lib.rs +++ b/codex-rs/codex-client/src/lib.rs @@ -7,7 +7,7 @@ mod sse; mod telemetry; mod transport; -pub use crate::custom_ca::BuildReqwestClientError; +pub use crate::custom_ca::BuildCustomCaTransportError; /// Test-only subprocess hook for custom CA coverage. /// /// This stays public only so the `custom_ca_probe` binary target can reuse the shared helper. It @@ -16,6 +16,7 @@ pub use crate::custom_ca::BuildReqwestClientError; #[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::maybe_build_rustls_client_config_with_custom_ca; pub use crate::default_client::CodexHttpClient; pub use crate::default_client::CodexRequestBuilder; pub use crate::error::StreamError; diff --git a/codex-rs/core/src/default_client.rs b/codex-rs/core/src/default_client.rs index f9826d9f526..809c73eed14 100644 --- a/codex-rs/core/src/default_client.rs +++ b/codex-rs/core/src/default_client.rs @@ -1,6 +1,6 @@ use crate::config_loader::ResidencyRequirement; use crate::spawn::CODEX_SANDBOX_ENV_VAR; -use codex_client::BuildReqwestClientError; +use codex_client::BuildCustomCaTransportError; use codex_client::CodexHttpClient; pub use codex_client::CodexRequestBuilder; use codex_client::build_reqwest_client_with_custom_ca; @@ -201,7 +201,7 @@ pub fn build_reqwest_client() -> reqwest::Client { /// /// Callers that need a structured CA-loading failure instead of the legacy logged fallback can use /// this method directly. -pub fn try_build_reqwest_client() -> Result { +pub fn try_build_reqwest_client() -> Result { let ua = get_codex_user_agent(); let mut builder = reqwest::Client::builder() diff --git a/codex-rs/login/src/lib.rs b/codex-rs/login/src/lib.rs index 0af9c05aa54..60b0c57f280 100644 --- a/codex-rs/login/src/lib.rs +++ b/codex-rs/login/src/lib.rs @@ -2,7 +2,7 @@ mod device_code_auth; mod pkce; mod server; -pub use codex_client::BuildReqwestClientError as BuildLoginHttpClientError; +pub use codex_client::BuildCustomCaTransportError as BuildLoginHttpClientError; pub use device_code_auth::DeviceCode; pub use device_code_auth::complete_device_code_login; pub use device_code_auth::request_device_code; diff --git a/docs/config.md b/docs/config.md index a3bdd934c54..d03fb98434a 100644 --- a/docs/config.md +++ b/docs/config.md @@ -38,11 +38,11 @@ sessions default to a temp directory; other modes default to `CODEX_HOME`. ## Custom CA Certificates -Codex can trust a custom root CA bundle for HTTP connections when enterprise -proxies or gateways intercept TLS. This applies to login flows and to Codex's -other external HTTP traffic, including Codex components that build reqwest -clients through the shared `codex-client` CA-loading path and remote MCP -connections that use it. +Codex can trust a custom root CA bundle for outbound HTTPS and secure websocket +connections when enterprise proxies or gateways intercept TLS. This applies to +login flows and to Codex's other external connections, including Codex +components that build reqwest clients or secure websocket clients through the +shared `codex-client` CA-loading path and remote MCP connections that use it. Set `CODEX_CA_CERTIFICATE` to the path of a PEM file containing one or more certificate blocks to use a Codex-specific CA bundle. If @@ -55,8 +55,8 @@ treated as unset. The PEM file may contain multiple certificates. Codex also tolerates OpenSSL `TRUSTED CERTIFICATE` labels and ignores well-formed `X509 CRL` sections in the same bundle. If the file is empty, unreadable, or malformed, the affected Codex -HTTP client reports a user-facing error that points back to these environment -variables. +HTTP or secure websocket connection reports a user-facing error that points +back to these environment variables. ## Notices