Fix SocketsHttpHandler.PreAuthenticate behavior #28047
Conversation
stephentoub
force-pushed
the
preauthsocketshandler
branch
from
db2a86e to
9c00556
Compare
| { | ||
| switch (response.StatusCode) | ||
| { | ||
| case HttpStatusCode.ProxyAuthenticationRequired: |
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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); |
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
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.
Relevant RFC: https://tools.ietf.org/html/rfc7617#section-2.2
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
yeah, those results look wacky... but your theory re '/' at least makes them look somewhat sane.
There was a problem hiding this comment.
@davidsh, is this an issue with CredentialCache? It seems like in the line here:
prefixLen should maybe be prefixLen + 1?
There was a problem hiding this comment.
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 two threads are racing to create the first connection for a specific pool, one of them will end up creating a pool object that's then discarded. Make sure we dispose it. Right now it doesn't really matter as there aren't any resources in an empty pool that need to be cleaned up, but it's a good measure and will help in the future if that changes.
Previously SocketsHttpHandler.PreAuthenticate was causing SocketsHttpHandler to always add a basic auth header on every request if a basic credential could be retrieved from the supplied credentials. This is not how PreAuthenticate is supposed to work. Rather, the handler is supposed to track credentials that have previously been used to successfully authenticate, and then on subsequent requests use credentials from that successfully-authenticated cache.
stephentoub
force-pushed
the
preauthsocketshandler
branch
from
9c00556 to
9b7de1b
Compare
| { | ||
| try | ||
| { | ||
| pool.PreAuthCredentials.Add(authUri, BasicScheme, challenge.Credential); |
There was a problem hiding this comment.
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.
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.
|
@dotnet/dnceng, the Linux leg failed here with this error: |
|
And, @dotnet/dnceng, the OSX legs appear to also have failed fairly catastrophically, e.g. |
|
Oh I think I know what this is. @MattGal is currently cycling our secrets and these look like auth errors. If you are pulling the secrets from KeyVault then this issue will resolve itself shortly. If not, you'll have to manually update secret info. |
|
@jonfortescue is correct, I'm chatting with @mmitche now to see if this transient (there's no good time to unplannedly cycle a storage key) or if we have to fix it in Jenkins. |
|
Not transient, I learned that Jenkins stores its own copy of things (joy), updating now. |
|
@stephentoub this should be fixed now, but I'll keep an eye on it. |
|
@dotnet-bot Test Linux x64 Release Build please |
|
Thanks, Matt. |
|
@dotnet-bot test OSX x64 Debug Build please |
|
@MattGal, is there still a problem here? |
|
There's backup right now and I'm urgently trying to find @mmitche for a jenkins expert but otherwise I believe the issues that caused this are addressed. |
|
OSX is working fine now |
|
What about the Windows and Linux failures... is that just due to a backup? |
|
@stephentoub : All Jenkins stuff should be affected equally due to Jenkins keeping its own copy of secrets i was unaware of. The difference is elastic scale; we can make tons of Linux and Windows machines to match any workload, we have a fixed set of macs. I believe everything should be fixed, feel free to queue new builds. @mmitche and I did some cleanup for old OSX jobs that may have cancelled some work items for existing ones but OSX is now healthy as well. If you want me to requeue stuff anywhere I see jobs failing i can (I'm skittish as sometimes the bot requeues things I didn't want it to) |
|
@dotnet-bot test Linux x64 Release Build please |
|
@dotnet-bot test Windows x64 Debug Build please |
1 similar comment
|
@dotnet-bot test Windows x64 Debug Build please |
* Use named arguments for more bool arguments * Dispose of new HttpConnectionPool if we lose race condition If two threads are racing to create the first connection for a specific pool, one of them will end up creating a pool object that's then discarded. Make sure we dispose it. Right now it doesn't really matter as there aren't any resources in an empty pool that need to be cleaned up, but it's a good measure and will help in the future if that changes. * Fix SocketsHttpHandler.PreAuthenticate behavior Previously SocketsHttpHandler.PreAuthenticate was causing SocketsHttpHandler to always add a basic auth header on every request if a basic credential could be retrieved from the supplied credentials. This is not how PreAuthenticate is supposed to work. Rather, the handler is supposed to track credentials that have previously been used to successfully authenticate, and then on subsequent requests use credentials from that successfully-authenticated cache. * Add more tests
* Use named arguments for more bool arguments * Dispose of new HttpConnectionPool if we lose race condition If two threads are racing to create the first connection for a specific pool, one of them will end up creating a pool object that's then discarded. Make sure we dispose it. Right now it doesn't really matter as there aren't any resources in an empty pool that need to be cleaned up, but it's a good measure and will help in the future if that changes. * Fix SocketsHttpHandler.PreAuthenticate behavior Previously SocketsHttpHandler.PreAuthenticate was causing SocketsHttpHandler to always add a basic auth header on every request if a basic credential could be retrieved from the supplied credentials. This is not how PreAuthenticate is supposed to work. Rather, the handler is supposed to track credentials that have previously been used to successfully authenticate, and then on subsequent requests use credentials from that successfully-authenticated cache. * Add more tests Commit migrated from dotnet/corefx@e2ed932
Previously PreAuthenticate was causing SocketsHttpHandler to always add a basic auth header on every request if a basic credential could be retrieved from the supplied credentials. This is not how PreAuthenticate is supposed to work. Rather, the handler is supposed to track credentials that have previously been used to successfully authenticate, and then on subsequent requests use credentials from that successfully-authenticated cache.
Closes https://github.com/dotnet/corefx/issues/27597
cc: @geoffkizer, @davidsh, @mconnew