Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Fix SocketsHttpHandler.PreAuthenticate behavior #28047

Merged
merged 4 commits into from
Mar 15, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions src/System.Net.Http/src/System.Net.Http.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,6 @@
<!-- SocketsHttpHandler implementation -->
<ItemGroup Condition="'$(TargetGroup)' == 'netcoreapp'">
<Compile Include="System\Net\Http\SocketsHttpHandler\AuthenticationHelper.cs" />
<Compile Include="System\Net\Http\SocketsHttpHandler\AuthenticationHelper.Basic.cs" />
<Compile Include="System\Net\Http\SocketsHttpHandler\AuthenticationHelper.Digest.cs" />
<Compile Include="System\Net\Http\SocketsHttpHandler\AuthenticationHelper.NtAuth.cs" />
<Compile Include="System\Net\Http\SocketsHttpHandler\ChunkedEncodingReadStream.cs" />
Expand Down Expand Up @@ -634,8 +633,6 @@
<Reference Include="System.Security.Cryptography.Encoding" />
<Reference Include="System.Security.Cryptography.Primitives" />
</ItemGroup>
<ItemGroup>
<Folder Include="Common\System\Threading\Tasks\" />
</ItemGroup>
<ItemGroup />
<Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
</Project>

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ private static async Task<HttpResponseMessage> SendWithNtAuthAsync(HttpRequestMe

string spn = "HTTP/" + authUri.IdnHost;
ChannelBinding channelBinding = connection.TransportContext?.GetChannelBinding(ChannelBindingKind.Endpoint);
NTAuthentication authContext = new NTAuthentication(false, challenge.SchemeName, challenge.Credential, spn, ContextFlagsPal.Connection, channelBinding);
NTAuthentication authContext = new NTAuthentication(isServer:false, challenge.SchemeName, challenge.Credential, spn, ContextFlagsPal.Connection, channelBinding);
try
{
while (true)
Expand Down Expand Up @@ -86,12 +86,12 @@ private static async Task<HttpResponseMessage> SendWithNtAuthAsync(HttpRequestMe

public static Task<HttpResponseMessage> SendWithNtProxyAuthAsync(HttpRequestMessage request, Uri proxyUri, ICredentials proxyCredentials, HttpConnection connection, CancellationToken cancellationToken)
{
return SendWithNtAuthAsync(request, proxyUri, proxyCredentials, true, connection, cancellationToken);
return SendWithNtAuthAsync(request, proxyUri, proxyCredentials, isProxyAuth:true, connection, cancellationToken);
}

public static Task<HttpResponseMessage> SendWithNtConnectionAuthAsync(HttpRequestMessage request, ICredentials credentials, HttpConnection connection, CancellationToken cancellationToken)
{
return SendWithNtAuthAsync(request, request.RequestUri, credentials, false, connection, cancellationToken);
return SendWithNtAuthAsync(request, request.RequestUri, credentials, isProxyAuth:false, connection, cancellationToken);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand All @@ -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);

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?

Copy link
Member Author

@stephentoub stephentoub Mar 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

against this particular Uri previously

Not just this particular Uri... the credential cache does prefix matching.

I'll add a test for that as well.

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...

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

@stephentoub stephentoub Mar 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test:

using System;
using System.Net;

class Program
{
    static void Main()
    {
        var cred = new NetworkCredential("hello", "world");
        var cc = new CredentialCache();
        cc.Add(new Uri("http://bing.com/something/hello.html"), "Basic", cred);

        string[] tests =
        {
            "http://microsoft.com/something/hello.html",
            "http://bing.com/something/hello.html",
            "http://bing.com/something/world.html",
            "http://bing.com/something/",
            "http://bing.com/",
            "http://bing.com/hello.html",
            "http://bing.com/world.html",
            "http://bing.com/another/",
            "http://bing.com/another/hello.html",
            "http://bing.com/somethingelse/",
            "http://bing.com/somethingelse/hello.html",
        };

        foreach (string s in tests)
        {
            Console.WriteLine(s.PadRight(50) + ": " + cc.GetCredential(new Uri(s), "Basic"));
        }
    }
}

produces:

http://microsoft.com/something/hello.html         :
http://bing.com/something/hello.html              : System.Net.NetworkCredential
http://bing.com/something/world.html              : System.Net.NetworkCredential
http://bing.com/something/                        : System.Net.NetworkCredential
http://bing.com/                                  :
http://bing.com/hello.html                        :
http://bing.com/world.html                        :
http://bing.com/another/                          :
http://bing.com/another/hello.html                :
http://bing.com/somethingelse/                    : System.Net.NetworkCredential
http://bing.com/somethingelse/hello.html          : System.Net.NetworkCredential

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.

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.

Copy link
Member Author

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:

return string.Compare(uri.AbsolutePath, 0, prefixUri.AbsolutePath, 0, prefixLen, StringComparison.OrdinalIgnoreCase) == 0;

prefixLen should maybe be prefixLen + 1?

Copy link
Contributor

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?

I'm not sure. We haven't had any bug reports previously about this.

cc: @Tratcher comments?

Copy link
Member

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.

}

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:

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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:


I'll add a test for a bunch of different status codes and update it if need be.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add a test for a bunch of different status codes and update it if need be.

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

  • Unauthorized (401), as expected, and
  • Continue (100)

WinHttpHandler treated all of them as success for auth, with the exception of:

  • All codes in the range [100, 199]
  • Moved (301)
  • Redirect (302)
  • RedirectMethod (303)
  • TemporaryRedirect (307)
  • PermanentRedirect (308)
  • Unauthorized (401)
  • ProxyAuthenticationRequired (407)

CurlHandler treated all of them as success for auth, with the exception of:

  • Unauthorized (401)

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think more closely matching NetfxHandler

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);
Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you never remove entries

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;
}
}

Expand All @@ -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);
}
}
}

Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ public HttpAuthenticatedConnectionHandler(HttpConnectionPoolManager poolManager)

protected internal override Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
{
return _poolManager.SendAsync(request, true, cancellationToken);
return _poolManager.SendAsync(request, doRequestAuth:true, cancellationToken);
}

protected override void Dispose(bool disposing)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ public HttpConnectionHandler(HttpConnectionPoolManager poolManager)

protected internal override Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
{
return _poolManager.SendAsync(request, false, cancellationToken);
return _poolManager.SendAsync(request, doRequestAuth:false, cancellationToken);
}

protected override void Dispose(bool disposing)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,12 @@ public HttpConnectionPool(HttpConnectionPoolManager poolManager, HttpConnectionK
_hostHeaderValueBytes = Encoding.ASCII.GetBytes(hostHeader);
Debug.Assert(Encoding.ASCII.GetString(_hostHeaderValueBytes) == hostHeader);
}

// Set up for PreAuthenticate. Access to this cache is guarded by a lock on the cache itself.
if (_poolManager.Settings._preAuthenticate)
{
PreAuthCredentials = new CredentialCache();
}
}

private static SslClientAuthenticationOptions ConstructSslOptions(HttpConnectionPoolManager poolManager, string sslHostName)
Expand All @@ -139,6 +145,7 @@ private static SslClientAuthenticationOptions ConstructSslOptions(HttpConnection
public Uri ProxyUri => _proxyUri;
public ICredentials ProxyCredentials => _poolManager.ProxyCredentials;
public byte[] HostHeaderValueBytes => _hostHeaderValueBytes;
public CredentialCache PreAuthCredentials { get; }

/// <summary>Object used to synchronize access to state in the pool.</summary>
private object SyncObj => _idleConnections;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -200,21 +200,23 @@ public Task<HttpResponseMessage> SendAsyncCore(HttpRequestMessage request, Uri p
}
break;
}

pool.Dispose();
}

return pool.SendAsync(request, doRequestAuth, cancellationToken);
}

public Task<HttpResponseMessage> SendProxyConnectAsync(HttpRequestMessage request, Uri proxyUri, CancellationToken cancellationToken)
{
return SendAsyncCore(request, proxyUri, false, true, cancellationToken);
return SendAsyncCore(request, proxyUri, doRequestAuth:false, isProxyConnect:true, cancellationToken);
}

public Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, bool doRequestAuth, CancellationToken cancellationToken)
{
if (_proxy == null)
{
return SendAsyncCore(request, null, doRequestAuth, false, cancellationToken);
return SendAsyncCore(request, null, doRequestAuth, isProxyConnect:false, cancellationToken);
}

// Do proxy lookup.
Expand All @@ -237,7 +239,7 @@ public Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, bool doRe
throw new NotSupportedException(SR.net_http_invalid_proxy_scheme);
}

return SendAsyncCore(request, proxyUri, doRequestAuth, false, cancellationToken);
return SendAsyncCore(request, proxyUri, doRequestAuth, isProxyConnect:false, cancellationToken);
}

/// <summary>Disposes of the pools, disposing of each individual pool.</summary>
Expand Down
Loading