Merge branch 'main' into codex/fix-dns-rebinding-vulnerability-in-network-proxy

codex-rs/network-proxy/src/http_proxy.rs

@@ -44,6 +44,7 @@ use rama_http_backend::server::layer::upgrade::Upgraded;
 use rama_net::Protocol;
 use rama_net::address::ProxyAddress;
 use rama_net::client::ConnectorService;
+use rama_net::client::ConnectorTarget;
 use rama_net::client::EstablishedClientConnection;
 use rama_net::http::RequestContext;
 use rama_net::proxy::ProxyRequest;
@@ -158,8 +159,16 @@ async fn http_connect_accept(
         exec_policy_hint: None,
     });
 
-    match evaluate_host_policy(&app_state, policy_decider.as_ref(), &request).await {
+    let outcome = match evaluate_host_policy(&app_state, policy_decider.as_ref(), &request).await {
-        Ok(NetworkDecision::Deny { reason }) => {
+        Ok(outcome) => outcome,
+        Err(err) => {
+            error!("failed to evaluate host for CONNECT {host}: {err}");
+            return Err(text_response(StatusCode::INTERNAL_SERVER_ERROR, "error"));
+        }
+    };
+
+    match outcome.decision {
+        NetworkDecision::Deny { reason } => {
             let _ = app_state
                 .record_blocked(BlockedRequest::new(BlockedRequestArgs {
                     host: host.clone(),
@@ -174,14 +183,14 @@ async fn http_connect_accept(
             warn!("CONNECT blocked (client={client}, host={host}, reason={reason})");
             return Err(blocked_text(&reason));
         }
-        Ok(NetworkDecision::Allow) => {
+        NetworkDecision::Allow => {
             let client = client.as_deref().unwrap_or_default();
             info!("CONNECT allowed (client={client}, host={host})");
         }
-        Err(err) => {
+    }
-            error!("failed to evaluate host for CONNECT {host}: {err}");
+
-            return Err(text_response(StatusCode::INTERNAL_SERVER_ERROR, "error"));
+    if let Some(target) = outcome.connector_target {
-        }
+        req.extensions_mut().insert(ConnectorTarget(target));
     }
 
     let mode = app_state
@@ -298,7 +307,7 @@ async fn forward_connect_tunnel(
 
 async fn http_plain_proxy(
     policy_decider: Option<Arc<dyn NetworkPolicyDecider>>,
-    req: Request,
+    mut req: Request,
 ) -> Result<Response, Infallible> {
     let app_state = match req.extensions().get::<Arc<NetworkProxyState>>().cloned() {
         Some(state) => state,
@@ -437,8 +446,16 @@ async fn http_plain_proxy(
         exec_policy_hint: None,
     });
 
-    match evaluate_host_policy(&app_state, policy_decider.as_ref(), &request).await {
+    let outcome = match evaluate_host_policy(&app_state, policy_decider.as_ref(), &request).await {
-        Ok(NetworkDecision::Deny { reason }) => {
+        Ok(outcome) => outcome,
+        Err(err) => {
+            error!("failed to evaluate host for {host}: {err}");
+            return Ok(text_response(StatusCode::INTERNAL_SERVER_ERROR, "error"));
+        }
+    };
+
+    match outcome.decision {
+        NetworkDecision::Deny { reason } => {
             let _ = app_state
                 .record_blocked(BlockedRequest::new(BlockedRequestArgs {
                     host: host.clone(),
@@ -453,11 +470,7 @@ async fn http_plain_proxy(
             warn!("request blocked (client={client}, host={host}, reason={reason})");
             return Ok(json_blocked(&host, &reason));
         }
-        Ok(NetworkDecision::Allow) => {}
+        NetworkDecision::Allow => {}
-        Err(err) => {
-            error!("failed to evaluate host for {host}: {err}");
-            return Ok(text_response(StatusCode::INTERNAL_SERVER_ERROR, "error"));
-        }
     }
 
     if !method_allowed {
@@ -483,6 +496,10 @@ async fn http_plain_proxy(
     let method = req.method();
     info!("request allowed (client={client}, host={host}, method={method})");
 
+    if let Some(target) = outcome.connector_target {
+        req.extensions_mut().insert(ConnectorTarget(target));
+    }
+
     let allow_upstream_proxy = match app_state
         .allow_upstream_proxy()
         .await

codex-rs/network-proxy/src/network_policy.rs

@@ -1,9 +1,11 @@
 use crate::reasons::REASON_POLICY_DENIED;
 use crate::runtime::HostBlockDecision;
 use crate::runtime::HostBlockReason;
+use crate::runtime::HostPolicyResult;
 use crate::state::NetworkProxyState;
 use anyhow::Result;
 use async_trait::async_trait;
+use rama_net::address::HostWithPort;
 use std::future::Future;
 use std::sync::Arc;
 
@@ -94,6 +96,12 @@ impl<D: NetworkPolicyDecider + ?Sized> NetworkPolicyDecider for Arc<D> {
     }
 }
 
+#[derive(Clone, Debug, PartialEq, Eq)]
+pub struct NetworkPolicyOutcome {
+    pub decision: NetworkDecision,
+    pub connector_target: Option<HostWithPort>,
+}
+
 #[async_trait]
 impl<F, Fut> NetworkPolicyDecider for F
 where
@@ -109,18 +117,30 @@ pub(crate) async fn evaluate_host_policy(
     state: &NetworkProxyState,
     decider: Option<&Arc<dyn NetworkPolicyDecider>>,
     request: &NetworkPolicyRequest,
-) -> Result<NetworkDecision> {
+) -> Result<NetworkPolicyOutcome> {
-    match state.host_blocked(&request.host, request.port).await? {
+    let HostPolicyResult {
-        HostBlockDecision::Allowed => Ok(NetworkDecision::Allow),
+        decision: host_decision,
+        connector_target,
+    } = state.host_blocked(&request.host, request.port).await?;
+    let decision = match host_decision {
+        HostBlockDecision::Allowed => NetworkDecision::Allow,
         HostBlockDecision::Blocked(HostBlockReason::NotAllowed) => {
             if let Some(decider) = decider {
-                Ok(decider.decide(request.clone()).await)
+                decider.decide(request.clone()).await
             } else {
-                Ok(NetworkDecision::deny(HostBlockReason::NotAllowed.as_str()))
+                NetworkDecision::deny(HostBlockReason::NotAllowed.as_str())
             }
         }
-        HostBlockDecision::Blocked(reason) => Ok(NetworkDecision::deny(reason.as_str())),
+        HostBlockDecision::Blocked(reason) => NetworkDecision::deny(reason.as_str()),
-    }
+    };
+    let connector_target = match decision {
+        NetworkDecision::Allow => connector_target,
+        NetworkDecision::Deny { .. } => None,
+    };
+    Ok(NetworkPolicyOutcome {
+        decision,
+        connector_target,
+    })
 }
 
 #[cfg(test)]
@@ -160,10 +180,10 @@ mod tests {
             exec_policy_hint: None,
         });
 
-        let decision = evaluate_host_policy(&state, Some(&decider), &request)
+        let outcome = evaluate_host_policy(&state, Some(&decider), &request)
             .await
             .unwrap();
-        assert_eq!(decision, NetworkDecision::Allow);
+        assert_eq!(outcome.decision, NetworkDecision::Allow);
         assert_eq!(calls.load(Ordering::SeqCst), 1);
     }
 
@@ -193,11 +213,11 @@ mod tests {
             exec_policy_hint: None,
         });
 
-        let decision = evaluate_host_policy(&state, Some(&decider), &request)
+        let outcome = evaluate_host_policy(&state, Some(&decider), &request)
             .await
             .unwrap();
         assert_eq!(
-            decision,
+            outcome.decision,
             NetworkDecision::Deny {
                 reason: REASON_DENIED.to_string()
             }
@@ -231,11 +251,11 @@ mod tests {
             exec_policy_hint: None,
         });
 
-        let decision = evaluate_host_policy(&state, Some(&decider), &request)
+        let outcome = evaluate_host_policy(&state, Some(&decider), &request)
             .await
             .unwrap();
         assert_eq!(
-            decision,
+            outcome.decision,
             NetworkDecision::Deny {
                 reason: REASON_NOT_ALLOWED_LOCAL.to_string()
             }

codex-rs/network-proxy/src/runtime.rs

@@ -14,6 +14,8 @@ use anyhow::Context;
 use anyhow::Result;
 use codex_utils_absolute_path::AbsolutePathBuf;
 use globset::GlobSet;
+use rama_net::address::Host as NetHost;
+use rama_net::address::HostWithPort;
 use serde::Serialize;
 use std::collections::HashSet;
 use std::collections::VecDeque;
@@ -62,6 +64,21 @@ pub enum HostBlockDecision {
     Blocked(HostBlockReason),
 }
 
+#[derive(Clone, Debug, PartialEq, Eq)]
+pub struct HostPolicyResult {
+    pub decision: HostBlockDecision,
+    pub connector_target: Option<HostWithPort>,
+}
+
+impl HostPolicyResult {
+    const fn blocked(reason: HostBlockReason) -> Self {
+        Self {
+            decision: HostBlockDecision::Blocked(reason),
+            connector_target: None,
+        }
+    }
+}
+
 #[derive(Clone, Debug, Serialize)]
 pub struct BlockedRequest {
     pub host: String,
@@ -198,11 +215,11 @@ impl NetworkProxyState {
         }
     }
 
-    pub async fn host_blocked(&self, host: &str, port: u16) -> Result<HostBlockDecision> {
+    pub async fn host_blocked(&self, host: &str, port: u16) -> Result<HostPolicyResult> {
         self.reload_if_needed().await?;
         let host = match Host::parse(host) {
             Ok(host) => host,
-            Err(_) => return Ok(HostBlockDecision::Blocked(HostBlockReason::NotAllowed)),
+            Err(_) => return Ok(HostPolicyResult::blocked(HostBlockReason::NotAllowed)),
         };
         let (deny_set, allow_set, allow_local_binding, allowed_domains_empty, allowed_domains) = {
             let guard = self.state.read().await;
@@ -222,19 +239,20 @@ impl NetworkProxyState {
         //  2) local/private networking is opt-in (defense-in-depth)
         //  3) allowlist is enforced when configured
         if deny_set.is_match(host_str) {
-            return Ok(HostBlockDecision::Blocked(HostBlockReason::Denied));
+            return Ok(HostPolicyResult::blocked(HostBlockReason::Denied));
         }
 
         let is_allowlisted = allow_set.is_match(host_str);
+        let mut connector_target = None;
         if !allow_local_binding {
             // If the intent is "prevent access to local/internal networks", we must not rely solely
             // on string checks like `localhost` / `127.0.0.1`. Attackers can use DNS rebinding or
             // public suffix services that map hostnames onto private IPs.
             //
-            // We therefore do a best-effort DNS + IP classification check before allowing the
+            // We therefore do a DNS + IP classification check before allowing the request and
-            // request. Explicit local/loopback literals are allowed only when explicitly
+            // reuse the resolved IPs for the eventual connect to avoid DNS rebinding gaps.
-            // allowlisted; hostnames that resolve to local/private IPs are blocked even if
+            // Explicit local/loopback literals are allowed only when explicitly allowlisted;
-            // allowlisted.
+            // hostnames that resolve to local/private IPs are blocked even if allowlisted.
             let local_literal = {
                 let host_no_scope = host_str
                     .split_once('%')
@@ -251,18 +269,34 @@ impl NetworkProxyState {
 
             if local_literal {
                 if !is_explicit_local_allowlisted(&allowed_domains, &host) {
-                    return Ok(HostBlockDecision::Blocked(HostBlockReason::NotAllowedLocal));
+                    return Ok(HostPolicyResult::blocked(HostBlockReason::NotAllowedLocal));
                 }
-            } else if host_resolves_to_non_public_ip(host_str, port).await {
+            } else {
-                return Ok(HostBlockDecision::Blocked(HostBlockReason::NotAllowedLocal));
+                let resolved_ips = match resolve_host_ips(host_str, port).await {
+                    Some(resolved_ips) => resolved_ips,
+                    None => {
+                        return Ok(HostPolicyResult::blocked(HostBlockReason::NotAllowedLocal));
+                    }
+                };
+                if resolved_ips.iter().copied().any(is_non_public_ip) {
+                    return Ok(HostPolicyResult::blocked(HostBlockReason::NotAllowedLocal));
+                }
+                connector_target = resolved_ips
+                    .first()
+                    .copied()
+                    .map(|ip| HostWithPort::new(NetHost::Address(ip), port));
             }
         }
 
-        if allowed_domains_empty || !is_allowlisted {
+        let decision = if allowed_domains_empty || !is_allowlisted {
-            Ok(HostBlockDecision::Blocked(HostBlockReason::NotAllowed))
+            HostBlockDecision::Blocked(HostBlockReason::NotAllowed)
         } else {
-            Ok(HostBlockDecision::Allowed)
+            HostBlockDecision::Allowed
-        }
+        };
+        Ok(HostPolicyResult {
+            decision,
+            connector_target,
+        })
     }
 
     pub async fn record_blocked(&self, entry: BlockedRequest) -> Result<()> {
@@ -392,26 +426,20 @@ pub(crate) fn unix_socket_permissions_supported() -> bool {
     cfg!(target_os = "macos")
 }
 
-async fn host_resolves_to_non_public_ip(host: &str, port: u16) -> bool {
+async fn resolve_host_ips(host: &str, port: u16) -> Option<Vec<IpAddr>> {
-    if let Ok(ip) = host.parse::<IpAddr>() {
+    let host_no_scope = host.split_once('%').map(|(ip, _)| ip).unwrap_or(host);
-        return is_non_public_ip(ip);
+    if let Ok(ip) = host_no_scope.parse::<IpAddr>() {
+        return Some(vec![ip]);
     }
 
-    // If DNS lookup fails, default to "not local/private" rather than blocking. In practice, the
+    // If DNS lookup fails, return `None` so callers can decide whether to block. We treat this as
-    // subsequent connect attempt will fail anyway, and blocking on transient resolver issues would
+    // a hard failure when local binding is disabled to avoid DNS rebinding gaps.
-    // make the proxy fragile. The allowlist/denylist remains the primary control plane.
     let addrs = match timeout(DNS_LOOKUP_TIMEOUT, lookup_host((host, port))).await {
         Ok(Ok(addrs)) => addrs,
-        Ok(Err(_)) | Err(_) => return false,
+        Ok(Err(_)) | Err(_) => return None,
     };
-
+    let ips = addrs.map(|addr| addr.ip()).collect::<Vec<_>>();
-    for addr in addrs {
+    if ips.is_empty() { None } else { Some(ips) }
-        if is_non_public_ip(addr.ip()) {
-            return true;
-        }
-    }
-
-    false
 }
 
 fn log_policy_changes(previous: &NetworkProxyConfig, next: &NetworkProxyConfig) {
@@ -535,7 +563,11 @@ mod tests {
         });
 
         assert_eq!(
-            state.host_blocked("example.com", 80).await.unwrap(),
+            state
+                .host_blocked("example.com", 80)
+                .await
+                .unwrap()
+                .decision,
             HostBlockDecision::Blocked(HostBlockReason::Denied)
         );
     }
@@ -543,18 +575,18 @@ mod tests {
     #[tokio::test]
     async fn host_blocked_requires_allowlist_match() {
         let state = network_proxy_state_for_policy(NetworkPolicy {
-            allowed_domains: vec!["example.com".to_string()],
+            allowed_domains: vec!["1.1.1.1".to_string()],
             ..NetworkPolicy::default()
         });
 
         assert_eq!(
-            state.host_blocked("example.com", 80).await.unwrap(),
+            state.host_blocked("1.1.1.1", 80).await.unwrap().decision,
             HostBlockDecision::Allowed
         );
         assert_eq!(
             // Use a public IP literal to avoid relying on ambient DNS behavior (some networks
             // resolve unknown hostnames to private IPs, which would trigger `not_allowed_local`).
-            state.host_blocked("8.8.8.8", 80).await.unwrap(),
+            state.host_blocked("8.8.8.8", 80).await.unwrap().decision,
             HostBlockDecision::Blocked(HostBlockReason::NotAllowed)
         );
     }
@@ -563,15 +595,20 @@ mod tests {
     async fn host_blocked_subdomain_wildcards_exclude_apex() {
         let state = network_proxy_state_for_policy(NetworkPolicy {
             allowed_domains: vec!["*.openai.com".to_string()],
+            allow_local_binding: true,
             ..NetworkPolicy::default()
         });
 
         assert_eq!(
-            state.host_blocked("api.openai.com", 80).await.unwrap(),
+            state
+                .host_blocked("api.openai.com", 80)
+                .await
+                .unwrap()
+                .decision,
             HostBlockDecision::Allowed
         );
         assert_eq!(
-            state.host_blocked("openai.com", 80).await.unwrap(),
+            state.host_blocked("openai.com", 80).await.unwrap().decision,
             HostBlockDecision::Blocked(HostBlockReason::NotAllowed)
         );
     }
@@ -585,11 +622,11 @@ mod tests {
         });
 
         assert_eq!(
-            state.host_blocked("127.0.0.1", 80).await.unwrap(),
+            state.host_blocked("127.0.0.1", 80).await.unwrap().decision,
             HostBlockDecision::Blocked(HostBlockReason::NotAllowedLocal)
         );
         assert_eq!(
-            state.host_blocked("localhost", 80).await.unwrap(),
+            state.host_blocked("localhost", 80).await.unwrap().decision,
             HostBlockDecision::Blocked(HostBlockReason::NotAllowedLocal)
         );
     }
@@ -603,7 +640,7 @@ mod tests {
         });
 
         assert_eq!(
-            state.host_blocked("127.0.0.1", 80).await.unwrap(),
+            state.host_blocked("127.0.0.1", 80).await.unwrap().decision,
             HostBlockDecision::Blocked(HostBlockReason::NotAllowedLocal)
         );
     }
@@ -617,7 +654,7 @@ mod tests {
         });
 
         assert_eq!(
-            state.host_blocked("10.0.0.1", 80).await.unwrap(),
+            state.host_blocked("10.0.0.1", 80).await.unwrap().decision,
             HostBlockDecision::Blocked(HostBlockReason::NotAllowedLocal)
         );
     }
@@ -631,7 +668,7 @@ mod tests {
         });
 
         assert_eq!(
-            state.host_blocked("localhost", 80).await.unwrap(),
+            state.host_blocked("localhost", 80).await.unwrap().decision,
             HostBlockDecision::Allowed
         );
     }
@@ -645,7 +682,7 @@ mod tests {
         });
 
         assert_eq!(
-            state.host_blocked("10.0.0.1", 80).await.unwrap(),
+            state.host_blocked("10.0.0.1", 80).await.unwrap().decision,
             HostBlockDecision::Allowed
         );
     }
@@ -659,7 +696,11 @@ mod tests {
         });
 
         assert_eq!(
-            state.host_blocked("fe80::1%lo0", 80).await.unwrap(),
+            state
+                .host_blocked("fe80::1%lo0", 80)
+                .await
+                .unwrap()
+                .decision,
             HostBlockDecision::Blocked(HostBlockReason::NotAllowedLocal)
         );
     }
@@ -673,7 +714,11 @@ mod tests {
         });
 
         assert_eq!(
-            state.host_blocked("fe80::1%lo0", 80).await.unwrap(),
+            state
+                .host_blocked("fe80::1%lo0", 80)
+                .await
+                .unwrap()
+                .decision,
             HostBlockDecision::Allowed
         );
     }
@@ -687,7 +732,7 @@ mod tests {
         });
 
         assert_eq!(
-            state.host_blocked("10.0.0.1", 80).await.unwrap(),
+            state.host_blocked("10.0.0.1", 80).await.unwrap().decision,
             HostBlockDecision::Blocked(HostBlockReason::NotAllowedLocal)
         );
     }
@@ -701,7 +746,7 @@ mod tests {
         });
 
         assert_eq!(
-            state.host_blocked("127.0.0.1", 80).await.unwrap(),
+            state.host_blocked("127.0.0.1", 80).await.unwrap().decision,
             HostBlockDecision::Blocked(HostBlockReason::NotAllowedLocal)
         );
     }