-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Regression in SocketsHttpHandler when establishing connection with Negotiate #58179
Comments
Tagging subscribers to this area: @dotnet/ncl Issue DetailsDescriptionThis is a continuation of #56159 (comment), where we reported that we were observing We observed the issue when using a .NET Core-based client against a custom web server implementation supporting Kerberos authentication. Depending on the presence of two headers in the initial
Neither of these are problematic with HTTP Server code for reproducer using System;
using System.Net;
using System.Net.Sockets;
using System.Text;
namespace BadHttpServer
{
public static class Program
{
public static void Main(string[] _)
{
const string crLf = "\r\n";
const int port = 4444;
var listener = new TcpListener(IPAddress.Parse("127.0.0.1"), port);
listener.Start();
Console.WriteLine($"Listening on {port}...");
while (true)
{
var client = listener.AcceptTcpClient();
var buffer = new byte[10240];
var stream = client.GetStream();
var length = stream.Read(buffer, 0, buffer.Length);
var receivedMessage = Encoding.UTF8.GetString(buffer, 0, length);
Console.WriteLine($"Received: {receivedMessage}");
var responseMessage =
receivedMessage.Contains("Authorization: Negotiate", StringComparison.OrdinalIgnoreCase)
? "HTTP/1.1 200 OK" + crLf
+ "Content-Type: text/plain" + crLf
+ crLf
+ "Successful"
: "HTTP/1.1 401 Unauthorized" + crLf
+ "Content-Type: text/plain" + crLf
+ "WWW-Authenticate: Negotiate" + crLf
//+ "Content-Length: 12" + crLf
//+ "Connection: close" + crLf
+ crLf
+ "Unauthorized" + crLf;
stream.Write(Encoding.UTF8.GetBytes(responseMessage));
client.Close();
Console.WriteLine($"Responded: {responseMessage}");
}
}
}
} HTTP Client code for reproducer using System;
using System.IO;
using System.Net.Http;
using System.Threading.Tasks;
namespace SadHttpClient
{
public static class Program
{
public static async Task Main(string[] _)
{
const string badServer = "http://localhost:4444";
var handler = new HttpClientHandler {UseDefaultCredentials = true};
var httpClient = new HttpClient(handler);
Console.WriteLine($"Runtime Path: {Path.GetDirectoryName(typeof(object).Assembly.Location)}");
Console.Write($"Press q to exit, or any other key to poke {badServer}");
while (Console.ReadKey(true).KeyChar != 'q')
{
try
{
Console.WriteLine();
Console.WriteLine(await httpClient.GetStringAsync(badServer));
}
catch (Exception ex)
{
Console.WriteLine(ex);
}
Console.Write($"\n\nPress q to exit, or any other key to poke {badServer}");
}
}
}
} Configuration
Regression?Yes. This is not a problem with Other informationI've included outputs / stack traces below from a few applicable scenarios (collapsed for brevity). Curl.exe output with both Content-Length and Connection: close missing.
Output from .NET Framework / WinHttpHandler client with both Content-Length and Connection: close missing.
Stack Trace with both Content-Length and Connection: close missing when using SocketsHttpHandler.
Stack Trace with Content-Length set, but Connection: close missing when using SocketsHttpHandler.
|
It sounds like this is not a regression from previous SocketsHttpHandler behavior (3.1/5.0)... is that correct?
Yeah, this is invalid server behavior, but we should not throw ODE here. Do you know if this happens even without auth?
We need to investigate. We should be draining the response body in this case. @wfurt any thoughts? |
I I'm planning to take a look @geoffkizer and at least reproduce the problem. The ODE is haunting us for long time and this should be easy to turn into enterprise-auth test. |
Correct: it's a regression in
I haven't been able to repro this without authentication, no. The same web server which led to us discovering this issue has an unauthenticated endpoint, and that works just fine. (That doesn't mean that I've been exhaustive in my checks though, and I may very well have lacked creativity in triggering the ODE some other way). Happy to provide more context / test results here BTW - this is an important issue that's blocking our ability to adopt .NET Core / PowerShell 7, and although I haven't been successful so far in my ability to pinpoint the bug in the runtime, I'm eager to help. |
Thanks, the data and code you provided is very useful. If we need more info we will let you know. |
Re this:
It looks like you're sending more data than you specified in the Content-Length. You specify 12 bytes, but then send |
Re this:
The core response handling logic treats this as if Connection: close was specified, which is the desired behavior -- it's technically not valid per RFC, but most clients are lenient here and just assume it means Connection: close. But the authentication logic is specifically checking for the Connection: close header, and when it doesn't find it, it assumes the connection won't be closed. So the core response logic ends up closing the connection, which the auth logic isn't expecting, and it tries to use the connection again, leading to ODE. We should probably change this so that the auth logic handles this the same way as the core request logic -- that is, assume that if Content-Length and Transfer-Encoding: chunked are both missing, then this implicitly means Connection: close. |
Ah sorry about that - I was working with repro's for web servers on two platforms, and that discrepancy snuck into the .NET variant. You're right that correcting the FWIW, I'm still observing a discrepancy between |
One thing to check for is if WinHttphandler reused the same connection and used session based NTLM as authentication. |
Triage: This is misbehaving server. We could treat such servers slightly better as @geoffkizer suggested above: #58179 (comment) @kedia990 do you know why the servers are misbehaving? Which server is that? |
Actually, to clarify, the server isn't actually misbehaving here. It's weird to omit the Connection: close and most servers will send it, but it's not strictly invalid. The authentication logic should handle this. |
To expand on what we should do here: Currently, when we receive a challenge, we first check for Connection: close. If we find it, we immediately create a new connection to use for the next request. We don't actually close the original connection yet though, because there's a chance that the challenge is invalid or cannot be satisfied (i.e. authContext.GetOutgoingBlob() returns null), in which case we want to simply return the 401 to the user so they can see it. This in itself is a little weird -- we really shouldn't create the new connection until we know we can satisfy the challenge. Once we have called authContext.GetOutgoingBlob() and gotten the data to send in the next request to respond to the current challege, we then will drain the current response if we didn't create a new connection above. We need to do this before sending the next request, and we now know it's safe to do so because actually want to send another request (as opposed to returning the 401 to the user as described above). Now, draining and trying to reuse the connection can fail for multiple reasons. One is that the connection can't be reused, as in the case above without Content-Length. Another is that the connection becomes invalid because of some protocol violation by the server, like the extra bytes in the case above with Content-Length. We definitely should handle the former, and it seems reasonable to handle the latter too if possible (it seems like this is what WinHttp is doing). So instead of checking up from for Connection: close, we should skip this entirely and just try to drain/reuse the connection (after getting the challenge response from authContext.GetOutgoingBlob()). If this succeeds, we can reuse that connection for the next request. If it fails for whatever reason, we should create a new connection and send the new request on that connection. I believe this would address both the original issue above as well as the weirdness I mentioned with creating the new connection before we know we can satisfy the challenge, and probably simplify the code a bit too. |
would this logic work also for #31133 ? |
I think so, but I'm not 100% sure. |
Sorry about the delayed response, I'd gotten a bit occupied with some personal commitments. I'm not intricately familiar with the codebase obviously, but I generally agree with @geoffkizer's observations. If there's anything we can do to make Also, I see that this issue has been tagged with the 7.0.0 milestone - would the fix also be backported to .NET 6 when it arrives? Aside from the fact that .NET 7 GA would be a year away, we'd like to be on a LTS release for production.
@karelz The other server that we're dealing with is a uWSGI-based server with a custom middleware for Kerberos authentication (running on Linux). Here's a simple repro for the uWSGI approach if you want to give it a shot: # Put this in a myserver.py
def application(env, start_response):
headers = [('Content-Type', 'text/plain')]
# Initial request: return 401 Unauthorized.
if not env.get('HTTP_AUTHORIZATION'):
body = 'Unauthorized'
headers.append(('WWW-Authenticate', 'Negotiate'))
# Neither of the two headers that follow appear to be strictly necessary
# for WinHttpHandler to work, but SocketsHttpHandler will blow up if
# at least Connection: close isn't present.
# If absent, SocketsHttpHandler fails with ODE from NetworkStream/SslStream.
headers.append(('Content-Length', str(len(body))))
# If absent, SocketsHttpHandler fails with "The response ended prematurely."
# If this alone is present, it's sufficient for SocketsHttpHandler to behave.
headers.append(('Connection', 'close'))
start_response('401 Unauthorized', headers)
return [body]
#headers.append(('Content-Length', '11'))
start_response('200 OK', headers)
return [b"Hello World"] And run uwsgi as follows:
Then, point the client (code above) at this server and fire away. |
Triage: Only 1 customer report over last year. Moving to Future as we didn't have enough time to consider addressing it in 7.0. |
Also running into this with a console app using |
Do you have runable repro @brendonparker? I would expect ADFS uses HTTP/1.1 with persistent connection so come of the discussion above would not be relevant.... |
Sure. Totally possible I am doing something wrong... Here is the code snippet: var client = new HttpClient(new HttpClientHandler
{
UseDefaultCredentials = true
});
// WIA rules setup for this user agent
client.DefaultRequestHeaders.Add("User-Agent", "Windows Rights Management Client");
var res = await client.GetAsync("https://adfs.domain.com/adfs/ls/IdpInitiatedSignOn.aspx?loginToRp=urn:amazon:webservices");
Console.WriteLine($"StatusCode: {(int)res.StatusCode} ({res.StatusCode})");
Console.WriteLine("Headers:");
foreach (var header in res.Headers)
{
Console.WriteLine($" - {header.Key}: {string.Join(", ", header.Value)}");
} Here is the output for 4.7.2:
Here is the output for dotnet6
|
It is interesting that the server offers |
BTW If you need to peek inside of the SSL https://www.telerik.com/fiddler is great tool to do that. |
I tried to use fiddler, but I think the proxy it injects to decrypt the https breaks the flow. I then start getting 401s in full framework. |
It may be possible. description in the middle can break |
Description
This is a continuation of #56159 (comment), where we reported that we were observing
ObjectDisposedException
when usingSocketsHttpHandler
for authenticated connections. We've understood the circumstances leading to the issue a little better now, and have a simple reproducer as well.We observed the issue when using a .NET Core-based client against a custom web server implementation supporting Kerberos authentication. Depending on the presence of two headers in the initial
401 Unauthorized
response -Connection: close
andContent-Length
- we were observing different errors withSocketsHttpHandler
:Connection: close
is present, then everything is OK withSocketsHttpHandler
(regardless ofContent-Length
).Connection: close
is absent, andContent-Length
is also absent, thenSocketsHttpHandler
throws anObjectDisposedException
. (I realize that a correct web server implementation should return aContent-Length
orTransfer-Encoding: chunked
for HTTP methods which have payloads defined, but it doesn't seem correct that the runtime would surface an ODE under these circumstances.)Content-Length
is present, then we getHttpRequestException: Authentication failed because the connection could not be reused.
.Neither of these are problematic with
WinHttpHandler
- either with .NET Framework 4.8, or .NET Core 3.1 (SocketsHttpHandler
disabled via environment variable).HTTP Server code for reproducer
HTTP Client code for reproducer
Configuration
net5.0
(although we've managed to repro the issue with other non-.NET web server implementations).net48
,netcoreapp3.1
(with/withoutSocketsHttpHandler
),net5.0
).Regression?
Yes. This is not a problem with
WinHttpHandler
(either in .NET Framework 4.8, or in .NET Core 3.1 with the environment variable set to disableSocketsHttpHandler
).Other information
I've included outputs / stack traces below from a few applicable scenarios (collapsed for brevity).
Curl.exe output with both Content-Length and Connection: close missing.
Output from .NET Framework / WinHttpHandler client with both Content-Length and Connection: close missing.
Stack Trace with both Content-Length and Connection: close missing when using SocketsHttpHandler.
Stack Trace with Content-Length set, but Connection: close missing when using SocketsHttpHandler.
The text was updated successfully, but these errors were encountered: