From a4bbd33b6558485d50eb68d55d0bf95c4f8f75b6 Mon Sep 17 00:00:00 2001 From: Winston Howes Date: Wed, 3 Jun 2026 19:18:11 -0700 Subject: [PATCH] Address child CA review feedback --- codex-rs/network-proxy/src/certs.rs | 2 +- codex-rs/network-proxy/src/child_ca.rs | 20 ++++- codex-rs/network-proxy/src/child_ca_tests.rs | 45 ++++++++++ codex-rs/network-proxy/src/proxy.rs | 92 +++++++++++++++----- 4 files changed, 133 insertions(+), 26 deletions(-) diff --git a/codex-rs/network-proxy/src/certs.rs b/codex-rs/network-proxy/src/certs.rs index fb2f6fd532..7c68e81550 100644 --- a/codex-rs/network-proxy/src/certs.rs +++ b/codex-rs/network-proxy/src/certs.rs @@ -417,7 +417,7 @@ pub(crate) fn append_pem_contents(bundle: &mut String, pem: &str) { } } -fn append_bounded_pem_contents(bundle: &mut String, pem: &str) -> Result<()> { +pub(crate) fn append_bounded_pem_contents(bundle: &mut String, pem: &str) -> Result<()> { let separator_len = usize::from(!bundle.is_empty() && !bundle.ends_with('\n')); let trailing_newline_len = usize::from(!pem.ends_with('\n')); anyhow::ensure!( diff --git a/codex-rs/network-proxy/src/child_ca.rs b/codex-rs/network-proxy/src/child_ca.rs index 66f5c54f3d..c0f4e267c3 100644 --- a/codex-rs/network-proxy/src/child_ca.rs +++ b/codex-rs/network-proxy/src/child_ca.rs @@ -40,8 +40,16 @@ where .unwrap_or_default(); if key == "SSL_CERT_FILE" && let Some(ssl_cert_dir_contents) = ssl_cert_dir_contents.as_deref() + && let Err(err) = crate::certs::append_bounded_pem_contents( + &mut custom_ca_bundle, + ssl_cert_dir_contents, + ) { - crate::certs::append_pem_contents(&mut custom_ca_bundle, ssl_cert_dir_contents); + warn!( + ca_env_key = key, + "failed to combine child MITM CA bundle; leaving current value unchanged: {err}" + ); + continue; } if custom_ca_bundle.is_empty() { continue; @@ -142,7 +150,15 @@ where }) { match crate::certs::read_custom_ca_dir(&ca_dir_path, can_read_path) { Ok(contents) if !contents.is_empty() => { - crate::certs::append_pem_contents(&mut trust_bundle, &contents); + if let Err(err) = + crate::certs::append_bounded_pem_contents(&mut trust_bundle, &contents) + { + warn!( + ca_bundle_path = %ca_dir_path.display(), + "failed to combine child MITM CA directories; ignoring SSL_CERT_DIR override: {err}" + ); + return None; + } } Ok(_) => {} Err(err) => { diff --git a/codex-rs/network-proxy/src/child_ca_tests.rs b/codex-rs/network-proxy/src/child_ca_tests.rs index de1ae98b50..40109efd64 100644 --- a/codex-rs/network-proxy/src/child_ca_tests.rs +++ b/codex-rs/network-proxy/src/child_ca_tests.rs @@ -145,6 +145,51 @@ fn materializes_readable_ssl_cert_dir() { assert_eq!(env.get(crate::certs::SSL_CERT_DIR_ENV_KEY), None); } +#[test] +fn bounds_aggregate_ssl_cert_dir_contents() { + let dir = tempdir().unwrap(); + let ssl_cert_dir_paths = [dir.path().join("certs-a"), dir.path().join("certs-b")]; + for path in &ssl_cert_dir_paths { + fs::create_dir(path).unwrap(); + fs::write(path.join("ordinary-ca.pem"), "a".repeat(2_200_000)).unwrap(); + } + let mitm_ca_trust_bundle_path = dir.path().join("ca-bundle.pem"); + fs::write(&mitm_ca_trust_bundle_path, "managed ca\n").unwrap(); + let ssl_cert_dir = std::env::join_paths(["certs-a", "certs-b"]).unwrap(); + let mut env = HashMap::from([ + ( + "SSL_CERT_FILE".to_string(), + mitm_ca_trust_bundle_path.display().to_string(), + ), + ( + crate::certs::SSL_CERT_DIR_ENV_KEY.to_string(), + ssl_cert_dir.to_string_lossy().into_owned(), + ), + ]); + let mitm_ca_trust_bundle = ManagedMitmCaTrustBundle { + path: mitm_ca_trust_bundle_path, + startup_env_values: HashMap::from([( + crate::certs::SSL_CERT_DIR_ENV_KEY, + ssl_cert_dir.to_string_lossy().into_owned(), + )]), + startup_cwd: dir.path().to_path_buf(), + }; + + prepare_mitm_ca_trust_bundle_env( + &mitm_ca_trust_bundle, + &mut env, + dir.path(), + &[crate::certs::SSL_CERT_DIR_ENV_KEY], + |_| true, + ); + + assert_eq!( + env.get("SSL_CERT_FILE"), + Some(&mitm_ca_trust_bundle.path.display().to_string()) + ); + assert_eq!(env.get(crate::certs::SSL_CERT_DIR_ENV_KEY), None); +} + #[test] fn preserves_unreadable_command_scoped_override() { let dir = tempdir().unwrap(); diff --git a/codex-rs/network-proxy/src/proxy.rs b/codex-rs/network-proxy/src/proxy.rs index ed65571ebb..c8c7fe5194 100644 --- a/codex-rs/network-proxy/src/proxy.rs +++ b/codex-rs/network-proxy/src/proxy.rs @@ -379,6 +379,7 @@ pub const PROXY_URL_ENV_KEYS: &[&str] = &[ pub const ALL_PROXY_ENV_KEYS: &[&str] = &["ALL_PROXY", "all_proxy"]; pub const PROXY_ACTIVE_ENV_KEY: &str = "CODEX_NETWORK_PROXY_ACTIVE"; pub const MITM_CA_ENV_ACTIVE_ENV_KEY: &str = "CODEX_NETWORK_PROXY_MITM_CA_ENV_ACTIVE"; +const STARTUP_CA_ENV_KEYS_PRESENT_ENV_KEY: &str = "CODEX_NETWORK_PROXY_STARTUP_CA_ENV_KEYS_PRESENT"; pub const ALLOW_LOCAL_BINDING_ENV_KEY: &str = "CODEX_NETWORK_ALLOW_LOCAL_BINDING"; const ELECTRON_GET_USE_PROXY_ENV_KEY: &str = "ELECTRON_GET_USE_PROXY"; const NODE_USE_ENV_PROXY_ENV_KEY: &str = "NODE_USE_ENV_PROXY"; @@ -387,6 +388,7 @@ const GIT_SSH_COMMAND_ENV_KEY: &str = "GIT_SSH_COMMAND"; pub const PROXY_ENV_KEYS: &[&str] = &[ PROXY_ACTIVE_ENV_KEY, MITM_CA_ENV_ACTIVE_ENV_KEY, + STARTUP_CA_ENV_KEYS_PRESENT_ENV_KEY, ALLOW_LOCAL_BINDING_ENV_KEY, ELECTRON_GET_USE_PROXY_ENV_KEY, NODE_USE_ENV_PROXY_ENV_KEY, @@ -577,24 +579,53 @@ fn apply_proxy_env_overrides( if let Some(mitm_ca_trust_bundle) = mitm_ca_trust_bundle { env.insert(MITM_CA_ENV_ACTIVE_ENV_KEY.to_string(), "1".to_string()); let managed_path = mitm_ca_trust_bundle.path.to_string_lossy().into_owned(); + let startup_ca_env_keys_present_in_child = ca_env_keys() + .filter(|&key| { + env.get(key) + .filter(|value| !value.is_empty()) + .is_some_and(|value| { + mitm_ca_trust_bundle.startup_env_values.get(key) == Some(value) + || (value == &managed_path && is_tracked_startup_ca_env_key(env, key)) + }) + }) + .collect::>(); + if startup_ca_env_keys_present_in_child.is_empty() { + env.remove(STARTUP_CA_ENV_KEYS_PRESENT_ENV_KEY); + } else { + env.insert( + STARTUP_CA_ENV_KEYS_PRESENT_ENV_KEY.to_string(), + startup_ca_env_keys_present_in_child.join(","), + ); + } for key in crate::certs::CUSTOM_CA_ENV_KEYS { if env .get(key) .filter(|value| !value.is_empty()) - .is_some_and(|value| value != &managed_path) + .is_some_and(|value| { + value != &managed_path && !startup_ca_env_keys_present_in_child.contains(&key) + }) { - // Child-scoped overrides, including inherited startup values, need the - // effective filesystem policy before we can combine them with the managed CA - // bundle, so leave them for prepare_child_env(). continue; } env.insert(key.to_string(), managed_path.clone()); } } else { env.remove(MITM_CA_ENV_ACTIVE_ENV_KEY); + env.remove(STARTUP_CA_ENV_KEYS_PRESENT_ENV_KEY); } } +fn ca_env_keys() -> impl Iterator { + crate::certs::CUSTOM_CA_ENV_KEYS + .into_iter() + .chain(std::iter::once(crate::certs::SSL_CERT_DIR_ENV_KEY)) +} + +fn is_tracked_startup_ca_env_key(env: &HashMap, key: &str) -> bool { + env.get(STARTUP_CA_ENV_KEYS_PRESENT_ENV_KEY) + .is_some_and(|keys| keys.split(',').any(|tracked_key| tracked_key == key)) +} + impl NetworkProxy { pub fn builder() -> NetworkProxyBuilder { NetworkProxyBuilder::default() @@ -646,7 +677,7 @@ impl NetworkProxy { pub fn apply_to_env(&self, env: &mut HashMap) { let runtime_settings = self.runtime_settings(); // Enforce proxying for child processes. Proxy endpoint values are always rewritten; - // managed MITM CA vars preserve child-scoped overrides after proxy startup. + // managed MITM CA vars preserve command-scoped overrides after proxy startup. apply_proxy_env_overrides( env, self.http_addr, @@ -668,18 +699,6 @@ impl NetworkProxy { F: Fn(&Path) -> bool, { let runtime_settings = self.runtime_settings(); - let startup_ca_env_keys_present_in_child = runtime_settings - .mitm_ca_trust_bundle - .as_ref() - .map_or_else(Vec::new, |mitm_ca_trust_bundle| { - mitm_ca_trust_bundle - .startup_env_values - .iter() - .filter_map(|(&key, startup_value)| { - (env.get(key) == Some(startup_value)).then_some(key) - }) - .collect() - }); apply_proxy_env_overrides( env, self.http_addr, @@ -688,6 +707,10 @@ impl NetworkProxy { runtime_settings.allow_local_binding, runtime_settings.mitm_ca_trust_bundle.as_ref(), ); + let startup_ca_env_keys_present_in_child = ca_env_keys() + .filter(|&key| is_tracked_startup_ca_env_key(env, key)) + .collect::>(); + env.remove(STARTUP_CA_ENV_KEYS_PRESENT_ENV_KEY); runtime_settings.mitm_ca_trust_bundle.as_ref().map_or_else( Vec::new, |mitm_ca_trust_bundle| { @@ -1178,7 +1201,7 @@ mod tests { } #[test] - fn apply_proxy_env_overrides_preserves_startup_mitm_ca_override() { + fn apply_proxy_env_overrides_tracks_rewritten_startup_mitm_ca_override() { let startup_ca_bundle_path = "/tmp/startup-ca.pem".to_string(); let mut env = HashMap::from([( "REQUESTS_CA_BUNDLE".to_string(), @@ -1187,10 +1210,7 @@ mod tests { let mitm_ca_trust_bundle_path = Path::new("/tmp/codex-proxy/ca-bundle.pem"); let mitm_ca_trust_bundle = crate::certs::ManagedMitmCaTrustBundle { path: mitm_ca_trust_bundle_path.to_path_buf(), - startup_env_values: HashMap::from([( - "REQUESTS_CA_BUNDLE", - startup_ca_bundle_path.clone(), - )]), + startup_env_values: HashMap::from([("REQUESTS_CA_BUNDLE", startup_ca_bundle_path)]), startup_cwd: Path::new("/tmp").to_path_buf(), }; apply_proxy_env_overrides( @@ -1202,11 +1222,37 @@ mod tests { Some(&mitm_ca_trust_bundle), ); - assert_eq!(env.get("REQUESTS_CA_BUNDLE"), Some(&startup_ca_bundle_path)); + assert_eq!( + env.get("REQUESTS_CA_BUNDLE"), + Some(&mitm_ca_trust_bundle_path.display().to_string()) + ); + assert_eq!( + env.get(STARTUP_CA_ENV_KEYS_PRESENT_ENV_KEY), + Some(&"REQUESTS_CA_BUNDLE".to_string()) + ); assert_eq!( env.get("SSL_CERT_FILE"), Some(&mitm_ca_trust_bundle_path.display().to_string()) ); + + env.insert( + "REQUESTS_CA_BUNDLE".to_string(), + "/tmp/command-ca.pem".to_string(), + ); + apply_proxy_env_overrides( + &mut env, + SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 3128), + SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 8081), + /*socks_enabled*/ true, + /*allow_local_binding*/ false, + Some(&mitm_ca_trust_bundle), + ); + + assert_eq!( + env.get("REQUESTS_CA_BUNDLE"), + Some(&"/tmp/command-ca.pem".to_string()) + ); + assert_eq!(env.get(STARTUP_CA_ENV_KEYS_PRESENT_ENV_KEY), None); } #[test]