Conversation
Username in config, password via keyring reference (env://, op://, etc.). Zeroized intermediate credential, scheme validation, clean 502 on error. Signed-off-by: Aleksy Siek <aleksy@alwaysfurther.ai>
PR Review SummarySize
Affected crates
Blast radius — ContainedThis PR touches: source code Updated automatically on each push to this PR. |
There was a problem hiding this comment.
Code Review
This pull request implements basic authentication support for external proxies, allowing the proxy username to be stored in the configuration and the password to be securely retrieved from the keyring. It also integrates this authentication flow into both standard CONNECT requests and TLS-intercepted connections, backed by new unit and integration tests. The review feedback highlights critical improvements, including fixing a race condition in the test EnvGuard by holding the mutex lock for its lifetime, zeroizing intermediate base64-encoded credentials to prevent sensitive data exposure in memory, and ensuring proxy authentication failures are properly logged to the audit trail.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Signed-off-by: Aleksy Siek <aleksy@alwaysfurther.ai>
| let intercept_proxy_auth: Option<Zeroizing<String>> = if let Some( | ||
| ref ext_config, | ||
| ) = | ||
| state.config.external_proxy | ||
| { | ||
| let bypassed = !state.bypass_matcher.is_empty() | ||
| && state.bypass_matcher.matches(&host); | ||
| if bypassed { | ||
| None | ||
| } else { | ||
| match ext_config | ||
| .auth | ||
| .as_ref() | ||
| .map(external::build_basic_proxy_auth_header) | ||
| .transpose() | ||
| { | ||
| Ok(h) => h, | ||
| Err(e) => { | ||
| audit::log_denied( | ||
| Some(&state.audit_log), | ||
| audit::ProxyMode::ConnectIntercept, | ||
| &audit::EventContext { | ||
| route_id, | ||
| auth_mechanism: Some(nono::undo::NetworkAuditAuthMechanism::ProxyAuthorization), | ||
| auth_outcome: Some(nono::undo::NetworkAuditAuthOutcome::Failed), | ||
| denial_category: Some(nono::undo::NetworkAuditDenialCategory::AuthenticationFailed), | ||
| ..audit::EventContext::default() | ||
| }, | ||
| &host, | ||
| port, | ||
| &e.to_string(), | ||
| ); | ||
| external::send_response(&mut stream, 502, "Bad Gateway") | ||
| .await?; | ||
| return Err(e); | ||
| } | ||
| } | ||
| } | ||
| } else { | ||
| None | ||
| }; | ||
| let upstream_proxy = | ||
| if let Some(ref ext_config) = state.config.external_proxy { | ||
| let bypassed = !state.bypass_matcher.is_empty() | ||
| && state.bypass_matcher.matches(&host); | ||
| if bypassed { | ||
| debug!("tls_intercept: bypassing upstream proxy for {}", host); | ||
| None | ||
| } else if ext_config.auth.is_some() { | ||
| // Auth is configured but not yet implemented. | ||
| // Fail loudly rather than silently connecting | ||
| // without auth — the corporate proxy would | ||
| // reject anyway. | ||
| let msg = "external proxy authentication is configured \ | ||
| but not yet implemented; remove the auth \ | ||
| section from the external proxy config or \ | ||
| wait for a future release"; | ||
| audit::log_denied( | ||
| Some(&state.audit_log), | ||
| audit::ProxyMode::ConnectIntercept, | ||
| &audit::EventContext { | ||
| route_id, | ||
| ..audit::EventContext::default() | ||
| }, | ||
| &host, | ||
| port, | ||
| msg, | ||
| ); | ||
| let response = | ||
| "HTTP/1.1 502 Bad Gateway\r\nContent-Length: 0\r\n\r\n"; | ||
| stream.write_all(response.as_bytes()).await?; | ||
| return Err(ProxyError::ExternalProxy(msg.to_string())); | ||
| } else { | ||
| Some(tls_intercept::InterceptUpstreamProxy { | ||
| proxy_addr: &ext_config.address, | ||
| proxy_auth_header: None, | ||
| proxy_auth_header: intercept_proxy_auth | ||
| .as_ref() | ||
| .map(|s| s.as_str()), | ||
| }) | ||
| } |
There was a problem hiding this comment.
You could simplify this, merging both blocks for intercept_proxy_auth and upstream, and computing bypassed once.
| let intercept_proxy_auth: Option<Zeroizing<String>> = if let Some( | |
| ref ext_config, | |
| ) = | |
| state.config.external_proxy | |
| { | |
| let bypassed = !state.bypass_matcher.is_empty() | |
| && state.bypass_matcher.matches(&host); | |
| if bypassed { | |
| None | |
| } else { | |
| match ext_config | |
| .auth | |
| .as_ref() | |
| .map(external::build_basic_proxy_auth_header) | |
| .transpose() | |
| { | |
| Ok(h) => h, | |
| Err(e) => { | |
| audit::log_denied( | |
| Some(&state.audit_log), | |
| audit::ProxyMode::ConnectIntercept, | |
| &audit::EventContext { | |
| route_id, | |
| auth_mechanism: Some(nono::undo::NetworkAuditAuthMechanism::ProxyAuthorization), | |
| auth_outcome: Some(nono::undo::NetworkAuditAuthOutcome::Failed), | |
| denial_category: Some(nono::undo::NetworkAuditDenialCategory::AuthenticationFailed), | |
| ..audit::EventContext::default() | |
| }, | |
| &host, | |
| port, | |
| &e.to_string(), | |
| ); | |
| external::send_response(&mut stream, 502, "Bad Gateway") | |
| .await?; | |
| return Err(e); | |
| } | |
| } | |
| } | |
| } else { | |
| None | |
| }; | |
| let upstream_proxy = | |
| if let Some(ref ext_config) = state.config.external_proxy { | |
| let bypassed = !state.bypass_matcher.is_empty() | |
| && state.bypass_matcher.matches(&host); | |
| if bypassed { | |
| debug!("tls_intercept: bypassing upstream proxy for {}", host); | |
| None | |
| } else if ext_config.auth.is_some() { | |
| // Auth is configured but not yet implemented. | |
| // Fail loudly rather than silently connecting | |
| // without auth — the corporate proxy would | |
| // reject anyway. | |
| let msg = "external proxy authentication is configured \ | |
| but not yet implemented; remove the auth \ | |
| section from the external proxy config or \ | |
| wait for a future release"; | |
| audit::log_denied( | |
| Some(&state.audit_log), | |
| audit::ProxyMode::ConnectIntercept, | |
| &audit::EventContext { | |
| route_id, | |
| ..audit::EventContext::default() | |
| }, | |
| &host, | |
| port, | |
| msg, | |
| ); | |
| let response = | |
| "HTTP/1.1 502 Bad Gateway\r\nContent-Length: 0\r\n\r\n"; | |
| stream.write_all(response.as_bytes()).await?; | |
| return Err(ProxyError::ExternalProxy(msg.to_string())); | |
| } else { | |
| Some(tls_intercept::InterceptUpstreamProxy { | |
| proxy_addr: &ext_config.address, | |
| proxy_auth_header: None, | |
| proxy_auth_header: intercept_proxy_auth | |
| .as_ref() | |
| .map(|s| s.as_str()), | |
| }) | |
| } | |
| let upstream_proxy = | |
| if let Some(ref ext_config) = state.config.external_proxy { | |
| let bypassed = !state.bypass_matcher.is_empty() | |
| && state.bypass_matcher.matches(&host); | |
| if bypassed { | |
| debug!( | |
| "tls_intercept: bypassing upstream proxy for {}", | |
| host | |
| ); | |
| None | |
| } else { | |
| let proxy_auth_header = match ext_config | |
| .auth | |
| .as_ref() | |
| .map(external::build_basic_proxy_auth_header) | |
| .transpose() | |
| { | |
| Ok(h) => h, | |
| Err(e) => { | |
| audit::log_denied( | |
| Some(&state.audit_log), | |
| audit::ProxyMode::ConnectIntercept, | |
| &audit::EventContext { | |
| route_id, | |
| auth_mechanism: Some( | |
| nono::undo::NetworkAuditAuthMechanism::ProxyAuthorization, | |
| ), | |
| auth_outcome: Some( | |
| nono::undo::NetworkAuditAuthOutcome::Failed, | |
| ), | |
| denial_category: Some( | |
| nono::undo::NetworkAuditDenialCategory::AuthenticationFailed, | |
| ), | |
| ..audit::EventContext::default() | |
| }, | |
| &host, | |
| port, | |
| &e.to_string(), | |
| ); | |
| external::send_response( | |
| &mut stream, | |
| 502, | |
| "Bad Gateway", | |
| ) | |
| .await?; | |
| return Err(e); | |
| } | |
| }; | |
| Some(tls_intercept::InterceptUpstreamProxy { | |
| proxy_addr: &ext_config.address, | |
| proxy_auth_header: proxy_auth_header | |
| .as_ref() | |
| .map(|s| s.as_str()), | |
| }) | |
| } | |
| } else { | |
| None | |
| }; | |
Linked Issue
Closes #1184
Summary
Username in config, password via keyring reference (env://, op://, etc.). Zeroized intermediate credential, scheme validation, clean 502 on error.
cc @caiocdcs this is basic auth but a good starter for adding other methods like NTML etc :)
feel free to expand the scheme field
Test Plan
Checklist
CHANGELOG.mdif needed