From 65b82cdf96f9d305494bc8c6c8d2404d37709030 Mon Sep 17 00:00:00 2001 From: viyatb-oai Date: Fri, 9 Oct 2026 17:50:37 +0000 Subject: [PATCH] Prevent brokered credential aliases from bypassing MITM hooks (#52661) ## Why A credential bound to a host with MITM hooks could also be used at an alias without hooks, allowing the proxy to inject the real credential without enforcing hook policy. ## What changed Reject requests to unhooked aliases when their selected brokered credentials are also bound to a hooked host. Check both HTTP forwarding and MITM requests before replacing dummy credentials, returning `403` with `blocked-by-mitm-hook` for denied requests. Use the same destination and credential-selection rules for alias checks and injection, including configured URL prefixes, safe paths, equivalent credentials, merged Basic authentication fields, and environment scope. ## Testing Add regression coverage for built-in and configured provider aliases, IPv6 destinations, credential selection, and unrelated credentials. Add an agent-started proxy test that verifies credential masking and alias denial. GitOrigin-RevId: 12be41654761cedcf87b170f7661d6566f7a864e --- codex-rs/core/tests/suite/network_approval.rs | 136 +++++++++++++++ .../network-proxy/src/credential_broker.rs | 96 +++++++---- .../src/credential_broker/destination.rs | 7 +- .../src/credential_broker/providers.rs | 12 ++ .../src/credential_broker/registry.rs | 41 ++++- .../src/credential_broker_tests.rs | 72 ++++++++ codex-rs/network-proxy/src/http_proxy.rs | 162 +++++++++++++++--- codex-rs/network-proxy/src/mitm.rs | 23 ++- codex-rs/network-proxy/src/mitm_tests.rs | 147 ++++++++++++++++ codex-rs/network-proxy/src/runtime.rs | 25 ++- 10 files changed, 649 insertions(+), 72 deletions(-) diff --git a/codex-rs/core/tests/suite/network_approval.rs b/codex-rs/core/tests/suite/network_approval.rs index f59554f5c3..9bf595e64e 100644 --- a/codex-rs/core/tests/suite/network_approval.rs +++ b/codex-rs/core/tests/suite/network_approval.rs @@ -16,6 +16,14 @@ use codex_exec_server::REMOTE_ENVIRONMENT_ID; use codex_exec_server::RemoveOptions; use codex_features::Feature; use codex_history::RolloutItem; +#[cfg(unix)] +use codex_network_proxy::CredentialProviderConfig; +#[cfg(unix)] +use codex_network_proxy::MitmHookActionsConfig; +#[cfg(unix)] +use codex_network_proxy::MitmHookConfig; +#[cfg(unix)] +use codex_network_proxy::MitmHookMatchConfig; use codex_network_proxy::NetworkProxyConfig; use codex_network_proxy::PROXY_ACTIVE_ENV_KEY; use codex_protocol::approvals::ExecApprovalKind; @@ -3143,6 +3151,134 @@ async fn approved_network_host_for_one_environment_still_prompts_in_another() -> Ok(()) } +#[cfg(unix)] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn agent_started_proxy_denies_brokered_provider_alias() -> Result<()> { + const REAL_CREDENTIAL: &str = "alias_aaaaaaaaaaaaaaaaaaaaaaaa"; + let upstream = tokio::net::TcpListener::bind("127.0.0.1:0").await?; + let port = upstream.local_addr()?.port(); + let mut network = NetworkProxyConfig { + enabled: true, + proxy_url: "http://127.0.0.1:0".to_string(), + socks_url: "http://127.0.0.1:0".to_string(), + // Local requests must stay behind the proxy for their credentials to be masked. + allow_local_binding: Some(false), + allow_upstream_proxy: false, + mitm_hooks: vec![MitmHookConfig { + host: "localhost".to_string(), + matcher: MitmHookMatchConfig { + methods: vec!["GET".to_string()], + path_prefixes: vec!["/v1".to_string()], + ..MitmHookMatchConfig::default() + }, + actions: MitmHookActionsConfig::default(), + }], + ..NetworkProxyConfig::default() + }; + network.set_credential_broker_enabled(/*enabled*/ true); + network.set_allowed_domains(vec!["localhost".to_string(), "127.0.0.1".to_string()]); + network.credential_providers.insert( + "alias_test".to_string(), + CredentialProviderConfig { + env: vec!["ALIAS_TEST_TOKEN".to_string()], + patterns: vec!["alias_[a-z]{24}".to_string()], + url_prefixes: vec![ + format!("http://localhost:{port}/v1"), + format!("http://127.0.0.1:{port}/v2"), + ], + ..CredentialProviderConfig::default() + }, + ); + let permission_profile = PermissionProfile::workspace_write_with( + &[], + NetworkSandboxPolicy::Enabled, + /*exclude_tmpdir_env_var*/ false, + /*exclude_slash_tmp*/ false, + ); + let server = start_mock_server().await; + let test = test_codex() + .with_config(move |config| { + for feature in [ + Feature::UnifiedExec, + Feature::ShellSnapshot, + Feature::NetworkProxy, + ] { + config + .features + .enable(feature) + .expect("enable test feature"); + } + config.permissions.approval_policy = Constrained::allow_any(AskForApproval::Never); + config + .permissions + .set_permission_profile(permission_profile.clone()) + .expect("set permission profile"); + config + .permissions + .shell_environment_policy + .r#set + .insert("ALIAS_TEST_TOKEN".to_string(), REAL_CREDENTIAL.to_string()); + config.permissions.network = Some( + NetworkProxySpec::from_config_and_constraints( + network, + /*requirements*/ None, + &permission_profile, + ) + .expect("build brokered managed network proxy"), + ); + }) + // Credential brokerage currently runs only in the local environment. + .build(&server) + .await?; + let proxy = test + .session_configured + .network_proxy + .as_ref() + .context("agent did not start its managed network proxy")?; + let command = r#"printf 'BROKERED:%s\nACTIVE:%s\n' "$ALIAS_TEST_TOKEN" "$CODEX_NETWORK_PROXY_CREDENTIAL_BROKER_ACTIVE""#; + let call_id = "exec-brokered-provider-alias"; + let mut args = network_exec_args(command); + args["yield_time_ms"] = json!(30_000); + let responses = + mount_exec_network_turn(&server, "resp-brokered-provider-alias", call_id, args).await?; + test.submit_text_turn("read the brokered credential") + .await?; + let output = responses + .function_call_output_text(call_id) + .context("agent did not return command output")?; + assert!(output.contains("ACTIVE:1"), "{output}"); + assert!(!output.contains(REAL_CREDENTIAL)); + let token = output + .lines() + .find_map(|line| line.strip_prefix("BROKERED:")) + .context("agent did not expose its brokered dummy")?; + assert!(token.starts_with("alias_")); + + // Probe after the command exits so the denial cannot interrupt the tool call. + let response = tokio::time::timeout(Duration::from_secs(/*secs*/ 5), async { + let mut connection = tokio::net::TcpStream::connect(&proxy.http_addr).await?; + connection + .write_all( + format!( + "GET http://127.0.0.1:{port}/v2/resource HTTP/1.1\r\n\ + Host: 127.0.0.1:{port}\r\nAuthorization: Bearer {token}\r\n\ + Connection: close\r\n\r\n" + ) + .as_bytes(), + ) + .await?; + let mut response = String::new(); + connection.read_to_string(&mut response).await?; + Ok::<_, std::io::Error>(response) + }) + .await + .context("alias denial did not finish")??; + assert!(response.starts_with("HTTP/1.1 403"), "{response}"); + assert!(response.contains("blocked-by-mitm-hook"), "{response}"); + assert!(!response.contains(REAL_CREDENTIAL)); + Ok(()) +} + pub(super) fn guardian_parent_catalog() -> ModelsResponse { let template = codex_models_manager::bundled_models_response() .expect("bundled model catalog") diff --git a/codex-rs/network-proxy/src/credential_broker.rs b/codex-rs/network-proxy/src/credential_broker.rs index bc3e2ffda5..57805c446d 100644 --- a/codex-rs/network-proxy/src/credential_broker.rs +++ b/codex-rs/network-proxy/src/credential_broker.rs @@ -1280,11 +1280,29 @@ impl CredentialBroker { headers: &mut HeaderMap, environment_id: Option<&str>, ) { + let state = self.read_state(); + if !state.enabled { + return; + } + + let credentials = + select_credentials(headers, destination, &state.credentials, environment_id); + for (credential, header_name, header_value) in credentials { + credential + .provider + .insert_request_header(headers, header_name, header_value); + } + } + + pub(crate) fn request_matches_hooked_host_alias( + &self, + destination: &str, + headers: &HeaderMap, + hooked_hosts: &[String], + environment_id: Option<&str>, + ) -> bool { let request = if destination.contains("://") { - let Ok(request) = Url::parse(destination) else { - return; - }; - Some(request) + Url::parse(destination).ok() } else { None }; @@ -1296,38 +1314,50 @@ impl CredentialBroker { ); let state = self.read_state(); if !state.enabled { - return; + return false; } - let credentials = select_credentials( - headers, - &normalized_host, - request.as_ref(), - &state.credentials, - environment_id, - ); - if credentials.iter().any(|(credential, _, _)| { - matches!( - &credential.provider, - BrokeredCredentialProvider::Configured(_) - ) - }) && let Some(request) = request.as_ref() - { - let Ok(raw_request) = destination.parse::() else { - return; - }; - let raw_path = raw_request.path(); - if raw_path != request.path() - || !crate::authorization_path::is_safe_for_authorization(raw_path) - { - return; - } - } - for (credential, header_name, header_value) in credentials { - credential - .provider - .insert_request_header(headers, header_name, header_value); + let selected_headers = + select_credentials(headers, destination, &state.credentials, environment_id) + .into_iter() + .map(|(_, name, _)| name) + .collect::>(); + if selected_headers.is_empty() { + return false; } + let matching_credentials = state + .credentials + .iter() + .filter(|credential| { + credential.belongs_to_environment(environment_id) + && credential + .host_bindings() + .any(|binding| binding.matches_request(&normalized_host, request.as_ref())) + }) + .collect::>(); + + // Ambiguity removes the whole header, so every translation for a surviving header + // contributed through equivalent deduplication or Basic-field merging. + matching_credentials.iter().any(|credential| { + hooked_hosts.iter().any(|hooked_host| { + hooked_host != &normalized_host + && credential + .host_bindings() + .any(|binding| binding.matches_hostname(hooked_host)) + }) && matching_credentials.iter().any(|candidate| { + candidate.provider.same_provider(&credential.provider) + && candidate.real_value == credential.real_value + && credential + .provider + .translate_request_headers( + headers, + &candidate.dummy_value, + &credential.real_value, + ) + .iter() + .any(|(name, _)| selected_headers.contains(name)) + }) + }) } fn read_state(&self) -> std::sync::RwLockReadGuard<'_, CredentialBrokerState> { diff --git a/codex-rs/network-proxy/src/credential_broker/destination.rs b/codex-rs/network-proxy/src/credential_broker/destination.rs index 116ed8d083..ef1dd4a39b 100644 --- a/codex-rs/network-proxy/src/credential_broker/destination.rs +++ b/codex-rs/network-proxy/src/credential_broker/destination.rs @@ -116,9 +116,10 @@ impl CredentialDestination { } pub(super) fn matches_host(&self, host: &str, port: u16) -> bool { - if self.port != port { - return false; - } + self.port == port && self.matches_hostname(host) + } + + pub(super) fn matches_hostname(&self, host: &str) -> bool { if self.wildcard { host.strip_suffix(&self.host) .is_some_and(|prefix| prefix.ends_with('.')) diff --git a/codex-rs/network-proxy/src/credential_broker/providers.rs b/codex-rs/network-proxy/src/credential_broker/providers.rs index aff11dc1bc..36adc873d1 100644 --- a/codex-rs/network-proxy/src/credential_broker/providers.rs +++ b/codex-rs/network-proxy/src/credential_broker/providers.rs @@ -113,6 +113,18 @@ impl CredentialHostBinding { } } + // Hooks apply to the host regardless of each credential destination's port or path. + pub(super) fn matches_hostname(&self, host: &str) -> bool { + match self { + Self::ConfiguredHosts(destinations) => destinations + .iter() + .any(|destination| destination.matches_hostname(host)), + Self::ExactHost(_) | Self::ExactHosts(_) | Self::HostPattern { .. } => { + self.matches_host(host, /*port*/ 443) + } + } + } + pub(super) fn requires_mitm(&self, host: &str, port: u16) -> bool { match self { Self::ConfiguredHosts(destinations) => destinations diff --git a/codex-rs/network-proxy/src/credential_broker/registry.rs b/codex-rs/network-proxy/src/credential_broker/registry.rs index d9e1555f03..2f740a0a86 100644 --- a/codex-rs/network-proxy/src/credential_broker/registry.rs +++ b/codex-rs/network-proxy/src/credential_broker/registry.rs @@ -10,6 +10,7 @@ use super::matching::is_operational_path_match; use super::providers; use super::source_accepts_credential; use super::source_tracks_credential; +use crate::policy::normalize_host; use base64::Engine; use rama_http::HeaderMap; use rama_http::HeaderName; @@ -189,17 +190,30 @@ pub(super) fn is_builtin_shaped_credential(value: &str) -> bool { pub(super) fn select_credentials<'a>( headers: &HeaderMap, - host: &str, - request: Option<&Url>, + destination: &str, credentials: &'a [CredentialRecord], environment_id: Option<&str>, ) -> Vec<(&'a CredentialRecord, HeaderName, HeaderValue)> { + let request = if destination.contains("://") { + let Ok(request) = Url::parse(destination) else { + return Vec::new(); + }; + Some(request) + } else { + None + }; + let host = normalize_host( + request + .as_ref() + .and_then(Url::host_str) + .unwrap_or(destination), + ); let mut translated_matches = Vec::<(&CredentialRecord, HeaderName, HeaderValue)>::new(); for credential in credentials.iter().filter(|credential| { credential.belongs_to_environment(environment_id) && credential .host_bindings() - .any(|binding| binding.matches_request(host, request)) + .any(|binding| binding.matches_request(&host, request.as_ref())) }) { for candidate in credentials.iter().filter(|candidate| { candidate.belongs_to_environment(environment_id) @@ -207,7 +221,7 @@ pub(super) fn select_credentials<'a>( && candidate.real_value == credential.real_value && candidate .host_bindings() - .any(|binding| binding.matches_request(host, request)) + .any(|binding| binding.matches_request(&host, request.as_ref())) }) { for (name, value) in credential.provider.translate_request_headers( headers, @@ -252,7 +266,7 @@ pub(super) fn select_credentials<'a>( merge_basic_auth_fields(original, &selected[existing].2, &header_value) }) { - // Keep the caller's configured-provider raw-path validation for either field. + // Keep configured-provider raw-path validation for either field. if matches!( credential.provider, BrokeredCredentialProvider::Configured(_) @@ -266,6 +280,23 @@ pub(super) fn select_credentials<'a>( ambiguous_headers.push(header_name); } } + if selected.iter().any(|(credential, _, _)| { + matches!( + &credential.provider, + BrokeredCredentialProvider::Configured(_) + ) + }) && let Some(request) = request.as_ref() + { + let Ok(raw_request) = destination.parse::() else { + return Vec::new(); + }; + let raw_path = raw_request.path(); + if raw_path != request.path() + || !crate::authorization_path::is_safe_for_authorization(raw_path) + { + return Vec::new(); + } + } selected } diff --git a/codex-rs/network-proxy/src/credential_broker_tests.rs b/codex-rs/network-proxy/src/credential_broker_tests.rs index a368d2258e..43b62632fe 100644 --- a/codex-rs/network-proxy/src/credential_broker_tests.rs +++ b/codex-rs/network-proxy/src/credential_broker_tests.rs @@ -1435,6 +1435,15 @@ fn openai_credentials_bind_only_to_default_and_configured_trusted_hosts() { ("attacker.example", dummy.as_str()), ] { let mut headers = headers_with_bearer(dummy); + assert_eq!( + broker.request_matches_hooked_host_alias( + host, + &headers, + &["api.openai.com".to_string()], + /*environment_id*/ None, + ), + host != "api.openai.com" && expected_credential == "sk-real", + ); broker.inject_request_headers(host, &mut headers); let expected = format!("Bearer {expected_credential}"); assert_eq!(authorization(&headers), Some(expected.as_str()), "{host}"); @@ -1457,6 +1466,69 @@ fn openai_credentials_bind_only_to_default_and_configured_trusted_hosts() { ); } +#[test] +fn selected_openai_credential_matches_ipv6_alias_with_plaintext_fallback() { + let broker = CredentialBroker::new(/*enabled*/ true); + let mut env = env_map([ + ("OPENAI_API_KEY", "sk-real"), + ("OPENAI_BASE_URL", "https://[fd00::1]/v1"), + ]); + broker.virtualize_child_env(&mut env); + for destination in ["https://[fd00::1]/v1/models", "fd00::1"] { + assert!(broker.request_matches_hooked_host_alias( + destination, + &headers_with_bearer(&env["OPENAI_API_KEY"]), + &["api.openai.com".to_string()], + /*environment_id*/ None, + )); + } +} + +#[test] +fn selected_alias_checks_equivalent_and_merged_credential_bindings() { + let broker = CredentialBroker::new(/*enabled*/ true); + let register = |env_var: &str, token: &str, host: &str| { + let mut env = env_map([("GH_HOST", host), (env_var, token)]); + broker.virtualize_child_env_for_environment(&mut env, Some("selected")); + env.insert("GH_HOST".to_string(), "alias.example".to_string()); + broker.virtualize_child_env_for_environment(&mut env, Some("selected")); + env[env_var].clone() + }; + let first = register("GH_ENTERPRISE_TOKEN", "ghp-first-real", "alias.example"); + let equivalent = register( + "GITHUB_ENTERPRISE_TOKEN", + "ghp-first-real", + "hooked.example", + ); + let second = register( + "GITHUB_ENTERPRISE_TOKEN", + "ghp-second-real", + "second-hooked.example", + ); + assert_ne!(first, equivalent); + + assert!(broker.request_matches_hooked_host_alias( + "alias.example", + &headers_with_bearer(&equivalent), + &["hooked.example".to_string()], + Some("selected"), + )); + let basic = STANDARD.encode(format!("{first}:{second}")); + let headers = headers_with_authorization(&format!("Basic {basic}")); + assert!(broker.request_matches_hooked_host_alias( + "alias.example", + &headers, + &["second-hooked.example".to_string()], + Some("selected"), + )); + assert!(!broker.request_matches_hooked_host_alias( + "alias.example", + &headers, + &["second-hooked.example".to_string()], + Some("other"), + )); +} + #[test] fn github_cloud_credentials_match_ghe_com_host_hint() { let broker = CredentialBroker::new(/*enabled*/ true); diff --git a/codex-rs/network-proxy/src/http_proxy.rs b/codex-rs/network-proxy/src/http_proxy.rs index b2953d35f1..883b4cef81 100644 --- a/codex-rs/network-proxy/src/http_proxy.rs +++ b/codex-rs/network-proxy/src/http_proxy.rs @@ -3,6 +3,7 @@ use crate::config::NetworkMode; use crate::connect_policy::TargetCheckedTcpConnector; use crate::connection_lifecycle::CancelOnShutdown; use crate::mitm; +use crate::mitm_hook::HookEvaluation; use crate::network_policy::BlockDecisionAuditEventArgs; use crate::network_policy::NetworkDecision; use crate::network_policy::NetworkDecisionSource; @@ -16,6 +17,7 @@ use crate::network_policy::emit_block_decision_audit_event; use crate::network_policy::evaluate_host_policy; use crate::policy::normalize_host; use crate::reasons::REASON_METHOD_NOT_ALLOWED; +use crate::reasons::REASON_MITM_HOOK_DENIED; use crate::reasons::REASON_MITM_REQUIRED; use crate::reasons::REASON_NOT_ALLOWED; use crate::reasons::REASON_PROXY_DISABLED; @@ -908,13 +910,70 @@ async fn http_plain_proxy( )); } - if let Err(err) = - inject_forward_request_credentials(app_state.as_ref(), &request_ctx, &mut req).await + let credential_destinations = + match forward_request_credential_destinations(app_state.as_ref(), &request_ctx, &req).await + { + Ok(destinations) => destinations, + Err(err) => { + return Ok(internal_error( + "failed to read credential injection config", + err, + )); + } + }; + // Evaluate the original Authorization header before replacing brokered dummy credentials. + // Otherwise an unhooked provider alias can receive the real credential without hook policy. + match app_state + .evaluate_mitm_hook_request(&host, &req, &credential_destinations) + .await { - return Ok(internal_error( - "failed to read plaintext credential injection config", - err, - )); + Ok(HookEvaluation::HookedHostNoMatch) => { + emit_http_block_decision_audit_event( + &app_state, + BlockDecisionAuditEventArgs { + source: NetworkDecisionSource::ModeGuard, + reason: REASON_MITM_HOOK_DENIED, + protocol: NetworkProtocol::Http, + server_address: host.as_str(), + server_port: port, + method: Some(req.method().as_str()), + client_addr: client.as_deref(), + }, + ); + let details = PolicyDecisionDetails { + decision: NetworkPolicyDecision::Deny, + reason: REASON_MITM_HOOK_DENIED, + source: NetworkDecisionSource::ModeGuard, + protocol: NetworkProtocol::Http, + host: &host, + port, + }; + let _ = app_state + .record_blocked(BlockedRequest::new(BlockedRequestArgs { + host: host.clone(), + reason: REASON_MITM_HOOK_DENIED.to_string(), + client: client.clone(), + method: Some(req.method().as_str().to_string()), + mode: None, + protocol: "http".to_string(), + decision: Some(details.decision.as_str().to_string()), + source: Some(details.source.as_str().to_string()), + port: Some(port), + })) + .await; + let client = client.as_deref().unwrap_or_default(); + warn!( + "request blocked by MITM hook policy (client={client}, host={host}, method={})", + req.method() + ); + return Ok(json_blocked(&host, REASON_MITM_HOOK_DENIED, Some(&details))); + } + Ok(HookEvaluation::Matched { .. } | HookEvaluation::NoHooksForHost) => {} + Err(err) => return Ok(internal_error("failed to evaluate MITM hook policy", err)), + } + + for destination in credential_destinations { + app_state.inject_request_credentials(&destination, req.headers_mut()); } let client = client.as_deref().unwrap_or_default(); @@ -946,11 +1005,11 @@ async fn http_plain_proxy( } } -async fn inject_forward_request_credentials( +async fn forward_request_credential_destinations( app_state: &NetworkProxyState, context: &RequestContext, - req: &mut Request, -) -> Result<()> { + req: &Request, +) -> Result> { let authority = context.host_with_port(); let scheme = req.uri().scheme_str().unwrap_or("http"); // Server-wide OPTIONS may use root-scoped credentials without changing its wire target. @@ -960,16 +1019,11 @@ async fn inject_forward_request_credentials( .map(rama_http::uri::PathAndQuery::as_str) .filter(|path| *path != "*") .unwrap_or("/"); - let destination = format!("{scheme}://{authority}{request_path}"); - let unrestricted_plaintext = app_state.plaintext_credential_injection_enabled().await?; - app_state.inject_request_credentials(&destination, req.headers_mut()); - if unrestricted_plaintext { - app_state.inject_request_credentials( - &normalize_host(&authority.host.to_string()), - req.headers_mut(), - ); + let mut destinations = vec![format!("{scheme}://{authority}{request_path}")]; + if app_state.plaintext_credential_injection_enabled().await? { + destinations.push(normalize_host(&authority.host.to_string())); } - Ok(()) + Ok(destinations) } async fn proxy_via_unix_socket(req: Request, socket_path: &str) -> Result { @@ -1382,9 +1436,12 @@ mod tests { .unwrap(); let context = RequestContext::try_from(&req).unwrap(); - inject_forward_request_credentials(&state, &context, &mut req) + for destination in forward_request_credential_destinations(&state, &context, &req) .await - .unwrap(); + .unwrap() + { + state.inject_request_credentials(&destination, req.headers_mut()); + } let expected = if enabled { real_token } else { dummy }; assert_eq!( @@ -1482,9 +1539,12 @@ mod tests { .unwrap(); let context = RequestContext::try_from(&req).unwrap(); - inject_forward_request_credentials(&state, &context, &mut req) + for destination in forward_request_credential_destinations(&state, &context, &req) .await - .unwrap(); + .unwrap() + { + state.inject_request_credentials(&destination, req.headers_mut()); + } let expected = if inject { real_token } else { dummy }; assert_eq!( @@ -1497,6 +1557,64 @@ mod tests { } } + #[tokio::test] + async fn http_plain_proxy_blocks_brokered_provider_alias_before_credential_injection() { + let mut network = NetworkProxyConfig { + allow_local_binding: Some(true), + credential_broker: true, + dangerously_allow_plaintext_credential_injection: true, + mitm: true, + mitm_hooks: vec![crate::mitm_hook::MitmHookConfig { + host: "api.github.com".to_string(), + matcher: crate::mitm_hook::MitmHookMatchConfig { + methods: vec!["POST".to_string()], + path_prefixes: vec!["/repos/openai/".to_string()], + ..crate::mitm_hook::MitmHookMatchConfig::default() + }, + actions: crate::mitm_hook::MitmHookActionsConfig::default(), + }], + ..NetworkProxyConfig::default() + }; + network.set_allowed_domains(vec!["api.github.com".to_string(), "github.com".to_string()]); + let state = Arc::new(network_proxy_state_for_policy(network)); + let mut env = HashMap::from([("GH_TOKEN".to_string(), "ghp-real".to_string())]); + state.virtualize_child_credentials(&mut env); + + for scheme in ["http", "https"] { + let mut req = Request::builder() + .method(Method::GET) + .uri(format!("{scheme}://github.com/repos/openai/codex/issues")) + .header(header::HOST, "github.com") + .header(header::AUTHORIZATION, format!("Bearer {}", env["GH_TOKEN"])) + .body(Body::empty()) + .expect("absolute-form provider-alias request should build"); + req.extensions_mut().insert(state.clone()); + + let response = http_plain_proxy( + /*policy_decider*/ None, /*environment_id*/ None, req, + ) + .await + .expect("provider-alias policy should return a response"); + + assert_eq!(response.status(), StatusCode::FORBIDDEN); + assert_eq!( + response.headers().get("x-proxy-error").unwrap(), + "blocked-by-mitm-hook" + ); + } + + let blocked = state.drain_blocked().await.unwrap(); + assert_eq!(blocked.len(), 2); + assert!(blocked.iter().all(|request| { + request.host == "github.com" + && request.reason == REASON_MITM_HOOK_DENIED + && request.protocol == "http" + && request.source.as_deref() == Some("mode_guard") + })); + assert_eq!(blocked[0].port, Some(80)); + assert_eq!(blocked[1].port, Some(443)); + } + #[tokio::test] async fn http_connect_accept_blocks_hooked_host_in_full_mode_without_mitm_state() { let mut policy = NetworkProxyConfig { diff --git a/codex-rs/network-proxy/src/mitm.rs b/codex-rs/network-proxy/src/mitm.rs index 60bcc85e34..021afe667e 100644 --- a/codex-rs/network-proxy/src/mitm.rs +++ b/codex-rs/network-proxy/src/mitm.rs @@ -77,6 +77,16 @@ struct MitmPolicyContext { app_state: Arc, } +impl MitmPolicyContext { + fn credential_destination(&self, req: &Request) -> String { + let authority = authority_header_value(&self.target_host, self.target_port, &self.scheme); + let path = path_and_query(req.uri()); + // Server-wide OPTIONS must not borrow authority from a narrower credential URL prefix. + let credential_path = if path == "*" { "/" } else { &path }; + format!("{}://{authority}{credential_path}", self.scheme) + } +} + #[derive(Clone)] struct MitmRequestContext { policy: MitmPolicyContext, @@ -248,6 +258,7 @@ async fn handle_mitm_request( } async fn forward_request(req: Request, request_ctx: &MitmRequestContext) -> Result { + let credential_destination = request_ctx.policy.credential_destination(&req); let hook_actions = match evaluate_mitm_policy(&req, &request_ctx.policy).await? { MitmPolicyDecision::Allow { hook_actions } => hook_actions, MitmPolicyDecision::Block(response) => return Ok(response), @@ -268,12 +279,6 @@ async fn forward_request(req: Request, request_ctx: &MitmRequestContext) -> Resu .authority(authority.as_str()) .path_and_query(path.as_str()) .build()?; - // Server-wide OPTIONS must not borrow authority from a narrower credential URL prefix. - let credential_path = if path == "*" { "/" } else { &path }; - let credential_destination = format!( - "{}://{authority}{credential_path}", - request_ctx.policy.scheme - ); request_ctx .policy .app_state @@ -424,7 +429,11 @@ async fn evaluate_mitm_policy( let hook_actions = match policy .app_state - .evaluate_mitm_hook_request(&policy.target_host, req) + .evaluate_mitm_hook_request( + &policy.target_host, + req, + &[policy.credential_destination(req)], + ) .await? { HookEvaluation::Matched { actions } => Some(actions), diff --git a/codex-rs/network-proxy/src/mitm_tests.rs b/codex-rs/network-proxy/src/mitm_tests.rs index 84ceb16b2c..89f77af3fc 100644 --- a/codex-rs/network-proxy/src/mitm_tests.rs +++ b/codex-rs/network-proxy/src/mitm_tests.rs @@ -1,10 +1,13 @@ use super::*; +use crate::CredentialProviderConfig; use crate::config::NetworkProxyConfig; use crate::reasons::REASON_METHOD_NOT_ALLOWED; use crate::reasons::REASON_MITM_HOOK_DENIED; use crate::reasons::REASON_NOT_ALLOWED_LOCAL; use crate::runtime::network_proxy_state_for_policy; +use base64::Engine as _; +use base64::engine::general_purpose::STANDARD; use codex_utils_absolute_path::AbsolutePathBuf; use pretty_assertions::assert_eq; use rama_http::Body; @@ -14,6 +17,7 @@ use rama_http::Method; use rama_http::Request; use rama_http::StatusCode; use rama_http::header::HeaderName; +use std::collections::HashMap; use tempfile::NamedTempFile; fn github_write_hook() -> crate::mitm_hook::MitmHookConfig { @@ -229,6 +233,149 @@ async fn mitm_policy_allows_matching_hooked_write_in_full_mode() { assert_eq!(app_state.blocked_snapshot().await.unwrap().len(), 0); } +#[tokio::test] +async fn mitm_policy_enforces_brokered_aliases_without_blocking_unrelated_credentials() { + for hooked_hosts in [ + vec!["api.github.com"], + vec!["api.github.com", "github.com"], + vec![], + ] { + let network = NetworkProxyConfig { + credential_broker: true, + mitm: true, + allow_local_binding: Some(true), + mitm_hooks: hooked_hosts + .iter() + .map(|host| { + let mut hook = github_write_hook(); + hook.host = (*host).to_string(); + hook.matcher.methods = vec!["GET".to_string()]; + hook.actions.inject_request_headers.clear(); + hook + }) + .collect(), + ..NetworkProxyConfig::default() + }; + let app_state = Arc::new(network_proxy_state_for_policy(network)); + let mut env = HashMap::from([ + ("GH_TOKEN".to_string(), "ghp-real".to_string()), + ("OPENAI_API_KEY".to_string(), "sk-real".to_string()), + ("GH_HOST".to_string(), "enterprise.example".to_string()), + ( + "GH_ENTERPRISE_TOKEN".to_string(), + "ghp-enterprise-real".to_string(), + ), + ]); + app_state.virtualize_child_credentials(&mut env); + let github = format!("Bearer {}", env["GH_TOKEN"]); + let token = format!("token {}", env["GH_TOKEN"]); + let basic = format!( + "Basic {}", + STANDARD.encode(format!("x-access-token:{}", env["GH_TOKEN"])) + ); + let openai = format!("Bearer {}", env["OPENAI_API_KEY"]); + let enterprise = format!("Bearer {}", env["GH_ENTERPRISE_TOKEN"]); + for (host, authorization, uses_cloud_credential) in [ + ("github.com", Some(github.as_str()), true), + ("api.astemu.ghe.com", Some(github.as_str()), true), + ("github.com", Some(token.as_str()), true), + ("github.com", Some(basic.as_str()), true), + ("github.com", None, false), + ("github.com", Some("Bearer unrelated-token"), false), + ("github.com", Some(openai.as_str()), false), + ("enterprise.example", Some(enterprise.as_str()), false), + ] { + let ctx = policy_ctx( + app_state.clone(), + NetworkMode::Full, + host, + /*target_port*/ 443, + ); + let mut req = Request::builder() + .method(Method::GET) + .uri("/repos/openai/codex/issues") + .header(HOST, host) + .body(Body::empty()) + .unwrap(); + if let Some(value) = authorization { + req.headers_mut() + .insert("authorization", HeaderValue::from_str(value).unwrap()); + } + let denied = + uses_cloud_credential && !hooked_hosts.is_empty() && !hooked_hosts.contains(&host); + let response = mitm_blocking_response(&req, &ctx).await.unwrap(); + assert_eq!( + response.as_ref().map(Response::status), + denied.then_some(StatusCode::FORBIDDEN), + "host: {host}, hooks: {hooked_hosts:?}" + ); + if let Some(response) = response { + assert_eq!(response.headers()["x-proxy-error"], "blocked-by-mitm-hook"); + } + } + } +} + +#[tokio::test] +async fn mitm_policy_checks_configured_credential_aliases_at_the_request_destination() { + let mut hook = github_write_hook(); + hook.host = "api.hooked.example".to_string(); + hook.actions.inject_request_headers.clear(); + let mut network = NetworkProxyConfig { + credential_broker: true, + mitm: true, + allow_local_binding: Some(true), + mitm_hooks: vec![hook], + ..NetworkProxyConfig::default() + }; + network.credential_providers.insert( + "custom".to_string(), + CredentialProviderConfig { + env: vec!["PROVIDER_TOKEN".to_string()], + patterns: vec!["provider_[a-z]{24}".to_string()], + // The hooked host need not use the alias's port or path prefix. + url_prefixes: vec![ + "https://*.hooked.example/v1".to_string(), + "https://alias.example:8443/v2".to_string(), + ], + ..CredentialProviderConfig::default() + }, + ); + let app_state = Arc::new(network_proxy_state_for_policy(network)); + let mut env = HashMap::from([( + "PROVIDER_TOKEN".to_string(), + "provider_abcdefghijklmnopqrstuvwx".to_string(), + )]); + app_state.virtualize_child_credentials(&mut env); + let ctx = policy_ctx( + app_state, + NetworkMode::Full, + "alias.example", + /*target_port*/ 8443, + ); + for (path, denied) in [ + ("/v2/models", true), + ("/v20/models", false), + ("/v2/../v2/models", false), + ] { + let req = Request::builder() + .uri(path) + .header(HOST, "alias.example:8443") + .header("authorization", format!("Bearer {}", env["PROVIDER_TOKEN"])) + .body(Body::empty()) + .unwrap(); + let response = mitm_blocking_response(&req, &ctx).await.unwrap(); + assert_eq!( + response.as_ref().map(Response::status), + denied.then_some(StatusCode::FORBIDDEN), + "path: {path}" + ); + if let Some(response) = response { + assert_eq!(response.headers()["x-proxy-error"], "blocked-by-mitm-hook"); + } + } +} + #[tokio::test] async fn mitm_policy_blocks_encoded_path_traversal_for_repository_allowlist() { let mut hook = github_write_hook(); diff --git a/codex-rs/network-proxy/src/runtime.rs b/codex-rs/network-proxy/src/runtime.rs index 87457c7464..c816bc358e 100644 --- a/codex-rs/network-proxy/src/runtime.rs +++ b/codex-rs/network-proxy/src/runtime.rs @@ -982,10 +982,31 @@ impl NetworkProxyState { &self, host: &str, req: &rama_http::Request, + credential_destinations: &[String], ) -> Result { self.reload_if_needed().await?; - let guard = self.state.read().await; - Ok(evaluate_mitm_hooks(&guard.mitm_hooks, host, req)) + let hooked_hosts = { + let guard = self.state.read().await; + match evaluate_mitm_hooks(&guard.mitm_hooks, host, req) { + HookEvaluation::NoHooksForHost if !guard.mitm_hooks.is_empty() => { + guard.mitm_hooks.keys().cloned().collect::>() + } + evaluation => return Ok(evaluation), + } + }; + + if credential_destinations.iter().any(|destination| { + self.credential_broker.request_matches_hooked_host_alias( + destination, + req.headers(), + &hooked_hosts, + self.environment_id(), + ) + }) { + Ok(HookEvaluation::HookedHostNoMatch) + } else { + Ok(HookEvaluation::NoHooksForHost) + } } pub(crate) async fn host_mitm_requirement(