Fix authenticated proxy for dotnet restore #123328
Conversation
Many proxies require the Proxy-Authorization header on the first request rather than sending a 407 Proxy Authentication Required response. This is especially common for: 1. Proxies that drop/ignore unauthenticated connections 2. HTTPS CONNECT tunnels where connection reuse isn't possible 3. Environment variable proxies (HTTP_PROXY/HTTPS_PROXY) with embedded credentials (http://user:password@proxy:port) This change modifies SendWithProxyAuthAsync to proactively send Basic authentication when credentials are available from the ICredentials provider, rather than waiting for a 407 challenge that may never come. The reactive authentication flow is still preserved for cases where the proxy does send a 407 response with different auth scheme requirements (Digest, NTLM, Negotiate).
dotnet-policy-service
bot
added
the
community-contribution
|
Tagging subscribers to this area: @karelz, @dotnet/ncl |
Add functional tests to verify that Basic proxy authentication is sent proactively when credentials are provided, without waiting for a 407 challenge. This tests the fix for proxies that don't send 407 responses. Tests cover: - Proactive auth on first HTTP request - Proactive auth for HTTPS CONNECT tunnels - DefaultNetworkCredentials are NOT sent proactively (NTLM/Negotiate only)
|
@dotnet-policy-service agree |
|
@ptarjan could you please start with filing an issue to kick off a discussion? There are security implications of pro-actively sending credentials, especially with basic auth (equivalent of plain text) on non-secured connections. So this is not so straightforward and we cannot just accept a PR without any considerations. |
|
cc @wfurt |
|
This seems like something that could be driven by the existing |
This is problematic IMHO and we will have difficulty with security group IMHO as it especially with basic AUTH leaks the credentials. The "Many proxies" claim seems doubtful as it was generally ok for everyone and the 407 is described in RFC. I also do not understand the case 2 & 3. I'm not sure how for example environment configuration is related to lack of 407. having said that I would probably be OK with that if that has some opt-in method. Cutting the rounds can be viewed as general optimization. (as for example noted in #33964) |
Thanks for the detailed feedback. Let me address each point: On "many proxies" and RFC compliance: You're right that RFC 7235 specifies the 407 challenge-response flow. However, the real-world issue is that some proxies (particularly in containerized/cloud environments) don't follow the RFC:
The specific environment where this was reproduced (http://claude.ai/code) uses an egress proxy that returns On the opt-in approach: I don't really understand the credential leak, but I'm eager to learn. If the proxy would really like to see your credentials it could return an HTTP 407 and then you will happily send them along, right? Or is this less about an attacker and instead trying to limit logging? If so, then why is the user even using credentials in their proxy url. I agree an opt-in is more conservative but it does provide user friction. |
Address PR feedback by making proactive proxy authentication opt-in rather than enabled by default. This preserves RFC 7235 compliant behavior (waiting for 407 challenge) as the default. Users can enable proactive auth via: - AppContext switch: System.Net.Http.EnableProactiveProxyAuth - Environment variable: DOTNET_SYSTEM_NET_HTTP_ENABLEPROACTIVEPROXYAUTH=1 This is useful for proxies that don't send 407 challenges but instead drop or reject unauthenticated connections (especially HTTPS CONNECT tunnel proxies). Updated tests to use RemoteExecutor with the environment variable to verify both opt-in and default behavior.
|
any concern here @dotnet/ncl ? Since this would be opt-in I feel it is ..... acceptable. And it would fix #100515 as well. For the 2 cases we have seen so far the AppContext seems sufficient e.g. adding new explicit API would be overkill and it would still depend on 3rd party tools to use it. |
|
Given that there are design decisions concerns, I would suggest to close this PR or switch it to Draft, until we have consensus. |
|
No need to change into draft, we will take this change. Me and @wfurt will review this. |
| private const string EnableProactiveProxyAuthCtxSwitch = "System.Net.Http.EnableProactiveProxyAuth"; | ||
| private const string EnableProactiveProxyAuthEnvironmentVariable = "DOTNET_SYSTEM_NET_HTTP_ENABLEPROACTIVEPROXYAUTH"; | ||
|
|
||
| private static volatile int s_enableProactiveProxyAuth = -1; | ||
|
|
||
| private static bool EnableProactiveProxyAuth | ||
| { | ||
| get | ||
| { | ||
| int enableProactiveProxyAuth = s_enableProactiveProxyAuth; | ||
| if (enableProactiveProxyAuth != -1) | ||
| { | ||
| return enableProactiveProxyAuth != 0; | ||
| } | ||
|
|
||
| // First check for the AppContext switch, giving it priority over the environment variable. | ||
| if (AppContext.TryGetSwitch(EnableProactiveProxyAuthCtxSwitch, out bool value)) | ||
| { | ||
| s_enableProactiveProxyAuth = value ? 1 : 0; | ||
| } | ||
| else | ||
| { | ||
| // AppContext switch wasn't used. Check the environment variable. | ||
| s_enableProactiveProxyAuth = | ||
| Environment.GetEnvironmentVariable(EnableProactiveProxyAuthEnvironmentVariable) is string envVar && | ||
| (envVar == "1" || envVar.Equals("true", StringComparison.OrdinalIgnoreCase)) ? 1 : 0; | ||
| } | ||
|
|
||
| return s_enableProactiveProxyAuth != 0; | ||
| } | ||
| } |
There was a problem hiding this comment.
Please move this to
and use the existing parsing helpers| private const string NtlmScheme = "NTLM"; | ||
| private const string NegotiateScheme = "Negotiate"; | ||
|
|
||
| private const string EnableProactiveProxyAuthCtxSwitch = "System.Net.Http.EnableProactiveProxyAuth"; |
There was a problem hiding this comment.
Nit: I'd call it System.Net.Http.SocketsHttpHandler.ProxyPreAuthenticate
| if (EnableProactiveProxyAuth) | ||
| { | ||
| NetworkCredential? credential = proxyCredentials.GetCredential(proxyUri, BasicScheme); | ||
| if (credential != null && credential != CredentialCache.DefaultNetworkCredentials) | ||
| { | ||
| SetBasicAuthToken(request, credential, isProxyAuth: true); | ||
| } | ||
| } |
There was a problem hiding this comment.
Could this call SendWithAuthAsync(preAuthenticate: EnableProactiveProxyAuth) instead and have it handle setting the headers since it already contains that logic?
| private const string NtlmScheme = "NTLM"; | ||
| private const string NegotiateScheme = "Negotiate"; | ||
|
|
||
| private const string EnableProactiveProxyAuthCtxSwitch = "System.Net.Http.EnableProactiveProxyAuth"; |
There was a problem hiding this comment.
There's a helper RuntimeSettingParser exactly for parsing AppContext and Env Var switches. Can it be used here?
| // drop or reject unauthenticated connections (e.g., some HTTPS CONNECT tunnel proxies). | ||
| if (EnableProactiveProxyAuth) | ||
| { | ||
| NetworkCredential? credential = proxyCredentials.GetCredential(proxyUri, BasicScheme); |
There was a problem hiding this comment.
@wfurt, do we want this in general for proxies or only for CONNECT when establishing the tunnel?
Is this the right place to do it? Where would you actually put this code if we were to introduce it as a property like ProxyPreAuthenticate? Shouldn't this be rather in the method above?
| await LoopbackServer.CreateServerAsync(async (proxyServer, proxyUri) => | ||
| { | ||
| var psi = new ProcessStartInfo(); | ||
| psi.Environment.Add("http_proxy", $"http://{proxyUri.Host}:{proxyUri.Port}"); |
There was a problem hiding this comment.
There should also be test with the creds in the env var. Similar to the original issue.
|
|
||
| namespace System.Net.Http.Functional.Tests | ||
| { | ||
| public abstract class ProactiveProxyAuthTest : HttpClientHandlerTestBase |
There was a problem hiding this comment.
I'd like to see coverage for both HTTP and HTTPS proxy as well as for HTTP and HTTPS requests (proxy request vs proxy tunnel), i.e. 4 cases.
7 participants
Fixes: #123363
Many proxies require the Proxy-Authorization header on the first request rather than sending a 407 Proxy Authentication Required response. This is especially common for:
This change modifies SendWithProxyAuthAsync to proactively send Basic authentication when credentials are available from the ICredentials provider, rather than waiting for a 407 challenge that may never come.
The reactive authentication flow is still preserved for cases where the proxy does send a 407 response with different auth scheme requirements (Digest, NTLM, Negotiate).