-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Fix SocketsHttpHandler.PreAuthenticate behavior #28047
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -2,7 +2,9 @@ | |||
// The .NET Foundation licenses this file to you under the MIT license. | ||||
// See the LICENSE file in the project root for more information. | ||||
|
||||
using System.Diagnostics; | ||||
using System.Net.Http.Headers; | ||||
using System.Text; | ||||
using System.Threading; | ||||
using System.Threading.Tasks; | ||||
|
||||
|
@@ -79,25 +81,19 @@ private static bool TryGetValidAuthenticationChallengeForScheme(string scheme, A | |||
|
||||
private static bool TryGetAuthenticationChallenge(HttpResponseMessage response, bool isProxyAuth, Uri authUri, ICredentials credentials, out AuthenticationChallenge challenge) | ||||
{ | ||||
challenge = default; | ||||
|
||||
if (!IsAuthenticationChallenge(response, isProxyAuth)) | ||||
{ | ||||
challenge = default; | ||||
return false; | ||||
} | ||||
|
||||
HttpHeaderValueCollection<AuthenticationHeaderValue> authenticationHeaderValues = GetResponseAuthenticationHeaderValues(response, isProxyAuth); | ||||
|
||||
// Try to get a valid challenge for the schemes we support, in priority order. | ||||
if (!TryGetValidAuthenticationChallengeForScheme(NegotiateScheme, AuthenticationType.Negotiate, authUri, credentials, authenticationHeaderValues, out challenge) && | ||||
!TryGetValidAuthenticationChallengeForScheme(NtlmScheme, AuthenticationType.Ntlm, authUri, credentials, authenticationHeaderValues, out challenge) && | ||||
!TryGetValidAuthenticationChallengeForScheme(DigestScheme, AuthenticationType.Digest, authUri, credentials, authenticationHeaderValues, out challenge) && | ||||
!TryGetValidAuthenticationChallengeForScheme(BasicScheme, AuthenticationType.Basic, authUri, credentials, authenticationHeaderValues, out challenge)) | ||||
{ | ||||
return false; | ||||
} | ||||
|
||||
return true; | ||||
HttpHeaderValueCollection<AuthenticationHeaderValue> authenticationHeaderValues = GetResponseAuthenticationHeaderValues(response, isProxyAuth); | ||||
return | ||||
TryGetValidAuthenticationChallengeForScheme(NegotiateScheme, AuthenticationType.Negotiate, authUri, credentials, authenticationHeaderValues, out challenge) || | ||||
TryGetValidAuthenticationChallengeForScheme(NtlmScheme, AuthenticationType.Ntlm, authUri, credentials, authenticationHeaderValues, out challenge) || | ||||
TryGetValidAuthenticationChallengeForScheme(DigestScheme, AuthenticationType.Digest, authUri, credentials, authenticationHeaderValues, out challenge) || | ||||
TryGetValidAuthenticationChallengeForScheme(BasicScheme, AuthenticationType.Basic, authUri, credentials, authenticationHeaderValues, out challenge); | ||||
} | ||||
|
||||
private static bool TryGetRepeatedChallenge(HttpResponseMessage response, string scheme, bool isProxyAuth, out string challengeData) | ||||
|
@@ -147,7 +143,13 @@ private static void SetRequestAuthenticationHeaderValue(HttpRequestMessage reque | |||
|
||||
private static void SetBasicAuthToken(HttpRequestMessage request, NetworkCredential credential, bool isProxyAuth) | ||||
{ | ||||
SetRequestAuthenticationHeaderValue(request, new AuthenticationHeaderValue(BasicScheme, GetBasicTokenForCredential(credential)), isProxyAuth); | ||||
string authString = !string.IsNullOrEmpty(credential.Domain) ? | ||||
credential.Domain + "\\" + credential.UserName + ":" + credential.Password : | ||||
credential.UserName + ":" + credential.Password; | ||||
|
||||
string base64AuthString = Convert.ToBase64String(Encoding.UTF8.GetBytes(authString)); | ||||
|
||||
SetRequestAuthenticationHeaderValue(request, new AuthenticationHeaderValue(BasicScheme, base64AuthString), isProxyAuth); | ||||
} | ||||
|
||||
private static async Task<bool> TrySetDigestAuthToken(HttpRequestMessage request, NetworkCredential credential, DigestResponse digestResponse, bool isProxyAuth) | ||||
|
@@ -174,49 +176,92 @@ private static Task<HttpResponseMessage> InnerSendAsync(HttpRequestMessage reque | |||
|
||||
private static async Task<HttpResponseMessage> SendWithAuthAsync(HttpRequestMessage request, Uri authUri, ICredentials credentials, bool preAuthenticate, bool isProxyAuth, bool doRequestAuth, HttpConnectionPool pool, CancellationToken cancellationToken) | ||||
{ | ||||
// If preauth is enabled and this isn't proxy auth, try to get a basic credential from the | ||||
// preauth credentials cache, and if successful, set an auth header for it onto the request. | ||||
// Currently we only support preauth for Basic. | ||||
bool performedBasicPreauth = false; | ||||
if (preAuthenticate) | ||||
{ | ||||
NetworkCredential credential = credentials.GetCredential(authUri, BasicScheme); | ||||
Debug.Assert(pool.PreAuthCredentials != null); | ||||
NetworkCredential credential; | ||||
lock (pool.PreAuthCredentials) // TODO #28045: Get rid of this lock. | ||||
{ | ||||
// Just look for basic credentials. If in the future we support preauth | ||||
// for other schemes, this will need to search in order of precedence. | ||||
Debug.Assert(pool.PreAuthCredentials.GetCredential(authUri, NegotiateScheme) == null); | ||||
Debug.Assert(pool.PreAuthCredentials.GetCredential(authUri, NtlmScheme) == null); | ||||
Debug.Assert(pool.PreAuthCredentials.GetCredential(authUri, DigestScheme) == null); | ||||
credential = pool.PreAuthCredentials.GetCredential(authUri, BasicScheme); | ||||
} | ||||
|
||||
if (credential != null) | ||||
{ | ||||
SetBasicAuthToken(request, credential, isProxyAuth); | ||||
performedBasicPreauth = true; | ||||
} | ||||
} | ||||
|
||||
HttpResponseMessage response = await InnerSendAsync(request, isProxyAuth, doRequestAuth, pool, cancellationToken).ConfigureAwait(false); | ||||
|
||||
if (TryGetAuthenticationChallenge(response, isProxyAuth, authUri, credentials, out AuthenticationChallenge challenge)) | ||||
{ | ||||
if (challenge.AuthenticationType == AuthenticationType.Digest) | ||||
switch (challenge.AuthenticationType) | ||||
{ | ||||
var digestResponse = new DigestResponse(challenge.ChallengeData); | ||||
if (await TrySetDigestAuthToken(request, challenge.Credential, digestResponse, isProxyAuth).ConfigureAwait(false)) | ||||
{ | ||||
response.Dispose(); | ||||
response = await InnerSendAsync(request, isProxyAuth, doRequestAuth, pool, cancellationToken).ConfigureAwait(false); | ||||
|
||||
// Retry in case of nonce timeout in server. | ||||
if (TryGetRepeatedChallenge(response, challenge.SchemeName, isProxyAuth, out string challengeData)) | ||||
case AuthenticationType.Digest: | ||||
var digestResponse = new DigestResponse(challenge.ChallengeData); | ||||
if (await TrySetDigestAuthToken(request, challenge.Credential, digestResponse, isProxyAuth).ConfigureAwait(false)) | ||||
{ | ||||
digestResponse = new DigestResponse(challengeData); | ||||
if (IsServerNonceStale(digestResponse) && | ||||
await TrySetDigestAuthToken(request, challenge.Credential, digestResponse, isProxyAuth).ConfigureAwait(false)) | ||||
response.Dispose(); | ||||
response = await InnerSendAsync(request, isProxyAuth, doRequestAuth, pool, cancellationToken).ConfigureAwait(false); | ||||
|
||||
// Retry in case of nonce timeout in server. | ||||
if (TryGetRepeatedChallenge(response, challenge.SchemeName, isProxyAuth, out string challengeData)) | ||||
{ | ||||
response.Dispose(); | ||||
response = await InnerSendAsync(request, isProxyAuth, doRequestAuth, pool, cancellationToken).ConfigureAwait(false); | ||||
digestResponse = new DigestResponse(challengeData); | ||||
if (IsServerNonceStale(digestResponse) && | ||||
await TrySetDigestAuthToken(request, challenge.Credential, digestResponse, isProxyAuth).ConfigureAwait(false)) | ||||
{ | ||||
response.Dispose(); | ||||
response = await InnerSendAsync(request, isProxyAuth, doRequestAuth, pool, cancellationToken).ConfigureAwait(false); | ||||
} | ||||
} | ||||
} | ||||
} | ||||
} | ||||
else if (challenge.AuthenticationType == AuthenticationType.Basic) | ||||
{ | ||||
if (!preAuthenticate) | ||||
{ | ||||
SetBasicAuthToken(request, challenge.Credential, isProxyAuth); | ||||
break; | ||||
|
||||
case AuthenticationType.Basic: | ||||
if (performedBasicPreauth) | ||||
{ | ||||
break; | ||||
} | ||||
|
||||
response.Dispose(); | ||||
SetBasicAuthToken(request, challenge.Credential, isProxyAuth); | ||||
response = await InnerSendAsync(request, isProxyAuth, doRequestAuth, pool, cancellationToken).ConfigureAwait(false); | ||||
} | ||||
|
||||
if (preAuthenticate) | ||||
{ | ||||
switch (response.StatusCode) | ||||
{ | ||||
case HttpStatusCode.ProxyAuthenticationRequired: | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems too generous -- e.g. do we really want to interpret a 5xx as auth success? My instinct is only 2xx/3xx should be interpreted that way. Not sure what other handlers do here though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I may be misreading things, but this appears to be what WinHttpHandler does: corefx/src/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpAuthHelper.cs Line 162 in d7d4fdc
I'll add a test for a bunch of different status codes and update it if need be. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
As we've found previously any time we look at something like this, there are differences between the various handlers. I tried all of the status codes from 100 to 599.... NetfxHandler treated all of them as success for auth with the exception of
WinHttpHandler treated all of them as success for auth, with the exception of:
CurlHandler treated all of them as success for auth, with the exception of:
Given this, I think more closely matching NetfxHandler and CurlHandler, i.e. what's in this PR, makes sense. If nothing else, it's the easiest to explain. But, I don't have a strong opinion. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, we should match .NET Framework as much as possible. That HTTP stack has been in use for 15+ years and its behaviors are what most devs are expecting when moving to .NET Core. |
||||
case HttpStatusCode.Unauthorized: | ||||
break; | ||||
|
||||
default: | ||||
lock (pool.PreAuthCredentials) // TODO #28045: Get rid of this lock. | ||||
{ | ||||
try | ||||
{ | ||||
pool.PreAuthCredentials.Add(authUri, BasicScheme, challenge.Credential); | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Memory leak, you never remove entries. HWR had a similar bug where it would add an entry for /a, /a/b, /a/b/c, etc.. We changed it to a LRU list with a max of 100 entries: https://referencesource.microsoft.com/#System/net/System/Net/_PrefixLookup.cs,1464f3b5a82af37f There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The other .NET Core handlers all use CredentialCache, so this is no worse than those. As noted in https://github.com/dotnet/corefx/issues/28045, we should ideally have a different type anyway that doesn't require taking a giant lock; we can address any such concerns as part of that. |
||||
} | ||||
catch (ArgumentException) | ||||
{ | ||||
// The credential already existed. | ||||
} | ||||
} | ||||
break; | ||||
} | ||||
} | ||||
break; | ||||
} | ||||
} | ||||
|
||||
|
@@ -225,13 +270,12 @@ await TrySetDigestAuthToken(request, challenge.Credential, digestResponse, isPro | |||
|
||||
public static Task<HttpResponseMessage> SendWithProxyAuthAsync(HttpRequestMessage request, Uri proxyUri, ICredentials proxyCredentials, bool doRequestAuth, HttpConnectionPool pool, CancellationToken cancellationToken) | ||||
{ | ||||
return SendWithAuthAsync(request, proxyUri, proxyCredentials, false, true, doRequestAuth, pool, cancellationToken); | ||||
return SendWithAuthAsync(request, proxyUri, proxyCredentials, preAuthenticate:false, isProxyAuth:true, doRequestAuth, pool, cancellationToken); | ||||
} | ||||
|
||||
public static Task<HttpResponseMessage> SendWithRequestAuthAsync(HttpRequestMessage request, ICredentials credentials, bool preAuthenticate, HttpConnectionPool pool, CancellationToken cancellationToken) | ||||
{ | ||||
return SendWithAuthAsync(request, request.RequestUri, credentials, preAuthenticate, false, true, pool, cancellationToken); | ||||
return SendWithAuthAsync(request, request.RequestUri, credentials, preAuthenticate, isProxyAuth:false, doRequestAuth:true, pool, cancellationToken); | ||||
} | ||||
} | ||||
} | ||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little unclear how actually saving off the ICredential and retrieving it here it useful.
It seems like all we're really doing here is remembering that we did successful Basic auth against this particular Uri previously. The credentials should always be the same as if we just retrieved them again from the original Credential, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not just this particular Uri... the credential cache does prefix matching.
I'll add a test for that as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, but you're adding specifically adding authUri to the credential cache below. So that would only matter if authUri ends in a '/', is that right? I mean, if I successfully authenticate against '/abc/foo.html' and then another request comes for '/abc/bar.html', that won't trigger preauth, right? I'm just trying to understand the behavior here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relevant RFC: https://tools.ietf.org/html/rfc7617#section-2.2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test:
produces:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, there are similar (but different of course) rules for digest: https://tools.ietf.org/html/rfc7616#section-3.6, https://tools.ietf.org/html/rfc7616#section-3.3 (for definition of "protection space").
Of course for digest, we need to remember some state from previous challenges in order to preauthenticate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, those results look wacky... but your theory re '/' at least makes them look somewhat sane.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@davidsh, is this an issue with CredentialCache? It seems like in the line here:
corefx/src/System.Net.Primitives/src/System/Net/CredentialCache.cs
Line 524 in 02c3892
prefixLen
should maybe beprefixLen + 1
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure. We haven't had any bug reports previously about this.
cc: @Tratcher comments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, the somethingelse match looks wrong. That said I haven't seen many users that scope by path, scoping by domain is the primary use case.