diff --git a/codex-rs/http-client/src/custom_ca.rs b/codex-rs/http-client/src/custom_ca.rs index 6ca705d0ab..4b4e3d973b 100644 --- a/codex-rs/http-client/src/custom_ca.rs +++ b/codex-rs/http-client/src/custom_ca.rs @@ -6,6 +6,8 @@ //! `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. +//! Concurrent native-root reads may share one in-flight load; later builds reload trust, and +//! custom CA files are read independently for every build. //! //! The module intentionally has a narrow responsibility: //! @@ -49,7 +51,6 @@ 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; @@ -247,16 +248,7 @@ fn build_rustls_client_config( // 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 mut root_store = (*crate::native_roots::load()).clone(); if let Some(bundle) = bundle { let certificates = bundle.load_certificates()?; diff --git a/codex-rs/http-client/src/custom_ca_tls_tests.rs b/codex-rs/http-client/src/custom_ca_tls_tests.rs index fb3c78fe14..3c7c5a15c7 100644 --- a/codex-rs/http-client/src/custom_ca_tls_tests.rs +++ b/codex-rs/http-client/src/custom_ca_tls_tests.rs @@ -1,4 +1,4 @@ -//! Handshake coverage for the custom trust path retained by Windows realtime connections. +//! Handshake and fresh-file-read coverage for shared custom trust configuration. use super::CODEX_CA_CERT_ENV; use super::ConfiguredCaBundle; @@ -26,11 +26,11 @@ fn custom_intermediate_trust_preserves_hostname_validation() { let temp = tempfile::TempDir::new().unwrap(); let path = temp.path().join("intermediate.pem"); std::fs::write(&path, intermediate.pem()).unwrap(); - let config = build_rustls_client_config(Some(&ConfiguredCaBundle { + let bundle = ConfiguredCaBundle { source_env: CODEX_CA_CERT_ENV, path, - })) - .unwrap(); + }; + let config = build_rustls_client_config(Some(&bundle)).unwrap(); let server = Arc::new( rustls::ServerConfig::builder() .with_no_client_auth() @@ -42,7 +42,7 @@ fn custom_intermediate_trust_preserves_hostname_validation() { handshake(config.clone(), server.clone(), "localhost"), Ok(()) ); - let error = handshake(config, server, "wrong.example").unwrap_err(); + let error = handshake(config.clone(), server.clone(), "wrong.example").unwrap_err(); assert!( matches!( error, @@ -53,6 +53,27 @@ fn custom_intermediate_trust_preserves_hostname_validation() { ), "{error:?}" ); + + std::fs::write(&bundle.path, include_str!("../tests/fixtures/test-ca.pem")).unwrap(); + let replaced = build_rustls_client_config(Some(&bundle)).unwrap(); + assert!(matches!( + handshake(replaced, server.clone(), "localhost"), + Err(rustls::Error::InvalidCertificate( + rustls::CertificateError::UnknownIssuer + )) + )); + assert_eq!(handshake(config, server, "localhost"), Ok(())); + + std::fs::write(&bundle.path, "").unwrap(); + assert!(matches!( + build_rustls_client_config(Some(&bundle)), + Err(super::BuildCustomCaTransportError::InvalidCaFile { .. }) + )); + std::fs::remove_file(&bundle.path).unwrap(); + assert!(matches!( + build_rustls_client_config(Some(&bundle)), + Err(super::BuildCustomCaTransportError::ReadCaFile { .. }) + )); } fn handshake( diff --git a/codex-rs/http-client/src/lib.rs b/codex-rs/http-client/src/lib.rs index 5eb01f021b..ccef4a6113 100644 --- a/codex-rs/http-client/src/lib.rs +++ b/codex-rs/http-client/src/lib.rs @@ -5,6 +5,7 @@ mod client_builder; mod client_tls; mod custom_ca; mod error; +mod native_roots; mod network_policy; mod outbound_proxy; mod request; diff --git a/codex-rs/http-client/src/native_roots.rs b/codex-rs/http-client/src/native_roots.rs new file mode 100644 index 0000000000..4dbc4fa47b --- /dev/null +++ b/codex-rs/http-client/src/native_roots.rs @@ -0,0 +1,68 @@ +//! Coalesces overlapping platform-root reads without retaining completed trust snapshots. +//! Later callers always reload. File/directory overrides bypass coalescing so their contents +//! are reread independently; custom Codex CA bundles are handled by the caller. + +use std::sync::Arc; +use std::sync::Mutex; +use std::sync::OnceLock; +use std::sync::Weak; + +use rustls::RootCertStore; + +#[derive(Default)] +struct NativeRootsLoader { + in_flight: Mutex>>>, +} + +impl NativeRootsLoader { + fn load(&self, load: impl FnOnce() -> RootCertStore) -> Arc { + let pending = { + let mut in_flight = self + .in_flight + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + match in_flight + .upgrade() + .filter(|pending| pending.get().is_none()) + { + Some(pending) => pending, + None => { + let pending = Arc::new(OnceLock::new()); + *in_flight = Arc::downgrade(&pending); + pending + } + } + }; + Arc::clone(pending.get_or_init(|| Arc::new(load()))) + } +} + +pub(crate) fn load() -> Arc { + // rustls-native-certs treats these as overrides of the platform store. + // Preserve fresh file reads even when several callers construct clients together. + if std::env::var_os("SSL_CERT_FILE").is_some() || std::env::var_os("SSL_CERT_DIR").is_some() { + return Arc::new(load_roots()); + } + static LOADER: NativeRootsLoader = NativeRootsLoader { + in_flight: Mutex::new(Weak::new()), + }; + LOADER.load(load_roots) +} + +fn load_roots() -> RootCertStore { + let rustls_native_certs::CertificateResult { certs, errors, .. } = + rustls_native_certs::load_native_certs(); + if !errors.is_empty() { + tracing::warn!( + native_root_error_count = errors.len(), + "encountered errors while loading native root certificates" + ); + } + let mut roots = RootCertStore::empty(); + let _ = roots.add_parsable_certificates(certs); + roots +} + +#[cfg(test)] +#[path = "native_roots_tests.rs"] +mod tests; diff --git a/codex-rs/http-client/src/native_roots_tests.rs b/codex-rs/http-client/src/native_roots_tests.rs new file mode 100644 index 0000000000..613491d823 --- /dev/null +++ b/codex-rs/http-client/src/native_roots_tests.rs @@ -0,0 +1,55 @@ +//! Verifies overlapping root loads, fresh trust generations, and panic recovery. + +use super::*; +use pretty_assertions::assert_eq; +use rustls_pki_types::CertificateDer; +use rustls_pki_types::pem::PemObject; +use std::sync::mpsc; +use std::thread; + +#[test] +fn concurrent_loads_share_roots_but_completed_loads_are_never_reused() { + let loader = NativeRootsLoader::default(); + let (entered, started) = mpsc::channel(); + let (release, released) = mpsc::channel(); + thread::scope(|scope| { + let first_loader = &loader; + let first = scope.spawn(move || { + first_loader.load(|| { + entered.send(()).unwrap(); + released.recv().unwrap(); + RootCertStore::empty() + }) + }); + started.recv().unwrap(); + let pending = loader.in_flight.lock().unwrap().upgrade().unwrap(); + let second = scope.spawn(|| loader.load(|| panic!("overlapping load must be shared"))); + // Wait for the second caller to own this generation before allowing it to finish. + while Arc::strong_count(&pending) < 3 { + thread::yield_now(); + } + drop(loader.in_flight.lock().unwrap()); + release.send(()).unwrap(); + let first = first.join().unwrap(); + let second = second.join().unwrap(); + assert!(Arc::ptr_eq(&first, &second)); + + // Keep the old generation alive: completion, not refcount expiry, must force a reload. + let updated = loader.load(|| { + let mut roots = RootCertStore::empty(); + roots.add_parsable_certificates( + CertificateDer::pem_slice_iter(include_bytes!("../tests/fixtures/test-ca.pem")) + .map(Result::unwrap), + ); + roots + }); + assert_eq!((first.len(), second.len(), updated.len()), (0, 0, 1)); + }); +} + +#[test] +fn panicking_load_does_not_poison_later_trust_reads() { + let loader = NativeRootsLoader::default(); + assert!(std::panic::catch_unwind(|| loader.load(|| panic!("interrupted load"))).is_err()); + assert!(loader.load(RootCertStore::empty).is_empty()); +}