Skip to content
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

Open
kedia990 opened this issue Aug 26, 2021 · 23 comments
Open
Labels
area-System.Net.Http enhancement Product code improvement that does NOT require public API changes/additions
Milestone

Comments

@kedia990
Copy link

Description

This is a continuation of #56159 (comment), where we reported that we were observing ObjectDisposedException when using SocketsHttpHandler 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 and Content-Length - we were observing different errors with SocketsHttpHandler:

  • If Connection: close is present, then everything is OK with SocketsHttpHandler (regardless of Content-Length).
  • If Connection: close is absent, and
    • Content-Length is also absent, then SocketsHttpHandler throws an ObjectDisposedException. (I realize that a correct web server implementation should return a Content-Length or Transfer-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 get HttpRequestException: 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

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

  • The code was tested on Windows 10 1909 64-bit.
  • The server for the reproducer targeted net5.0 (although we've managed to repro the issue with other non-.NET web server implementations).
  • The client targeted various .NET runtimes (net48, netcoreapp3.1 (with/without SocketsHttpHandler), 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 disable SocketsHttpHandler).

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.
PS C:\Users\redacted> curl.exe -vvv --negotiate -u : "http://localhost:4444/"
*   Trying ::1...
* TCP_NODELAY set
*   Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 4444 (#0)
> GET / HTTP/1.1
> Host: localhost:4444
> User-Agent: curl/7.55.1
> Accept: */*
>
< HTTP/1.1 401 Unauthorized
< Content-Type: text/plain
< WWW-Authenticate: Negotiate
* no chunk, no close, no size. Assume close to signal end
<
* Closing connection 0
* Issue another request to this URL: 'http://localhost:4444/'
* Hostname localhost was found in DNS cache
*   Trying ::1...
* TCP_NODELAY set
*   Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 4444 (#1)
* Server auth using Negotiate with user ''
> GET / HTTP/1.1
> Host: localhost:4444
> Authorization: Negotiate <SNIP>
> User-Agent: curl/7.55.1
> Accept: */*
>
< HTTP/1.1 200 OK
< Content-Type: text/plain
* no chunk, no close, no size. Assume close to signal end
<
Successful* Closing connection 1
Output from .NET Framework / WinHttpHandler client with both Content-Length and Connection: close missing.
Runtime Path: C:\Windows\Microsoft.NET\Framework64\v4.0.30319
Press q to exit, or any other key to poke http://localhost:4444
Successful
Stack Trace with both Content-Length and Connection: close missing when using SocketsHttpHandler.
Runtime Path: C:\Program Files\dotnet\shared\Microsoft.NETCore.App\5.0.9
Press q to exit, or any other key to poke http://localhost:4444
System.Net.Http.HttpRequestException: An error occurred while sending the request.
 ---> System.ObjectDisposedException: Cannot access a disposed object.
Object name: 'System.Net.Sockets.NetworkStream'.
   at System.Net.Sockets.NetworkStream.<ThrowIfDisposed>g__ThrowObjectDisposedException|63_0()
   at System.Net.Sockets.NetworkStream.WriteAsync(ReadOnlyMemory`1 buffer, CancellationToken cancellationToken)
   at System.Net.Http.HttpConnection.WriteToStreamAsync(ReadOnlyMemory`1 source, Boolean async)
   at System.Net.Http.HttpConnection.FlushAsync(Boolean async)
   at System.Net.Http.HttpConnection.SendAsyncCore(HttpRequestMessage request, Boolean async, CancellationToken cancellationToken)
   --- End of inner exception stack trace ---
   at System.Net.Http.HttpConnection.SendAsyncCore(HttpRequestMessage request, Boolean async, CancellationToken cancellationToken)
   at System.Net.Http.AuthenticationHelper.SendWithNtAuthAsync(HttpRequestMessage request, Uri authUri, Boolean async, ICredentials credentials, Boolean isProxyAuth, HttpConnection connection, HttpConnectionPool connectionPool, CancellationToken cancellationToken)
   at System.Net.Http.HttpConnectionPool.SendWithRetryAsync(HttpRequestMessage request, Boolean async, Boolean doRequestAuth, CancellationToken cancellationToken)
   at System.Net.Http.AuthenticationHelper.SendWithAuthAsync(HttpRequestMessage request, Uri authUri, Boolean async, ICredentials credentials, Boolean preAuthenticate, Boolean isProxyAuth, Boolean doRequestAuth, HttpConnectionPool pool, CancellationToken cancellationToken)
   at System.Net.Http.RedirectHandler.SendAsync(HttpRequestMessage request, Boolean async, CancellationToken cancellationToken)
   at System.Net.Http.HttpClient.SendAsyncCore(HttpRequestMessage request, HttpCompletionOption completionOption, Boolean async, Boolean emitTelemetryStartStop, CancellationToken cancellationToken)
   at System.Net.Http.HttpClient.GetStringAsyncCore(HttpRequestMessage request, CancellationToken cancellationToken)
   at SadHttpClient.Program.Main(String[] _) in C:\local\SadHttpClient\Program.cs:line 24
Stack Trace with Content-Length set, but Connection: close missing when using SocketsHttpHandler.
Runtime Path: C:\Program Files\dotnet\shared\Microsoft.NETCore.App\5.0.9
Press q to exit, or any other key to poke http://localhost:4444
System.Net.Http.HttpRequestException: Authentication failed because the connection could not be reused.
   at System.Net.Http.HttpConnection.DrainResponseAsync(HttpResponseMessage response, CancellationToken cancellationToken)
   at System.Net.Http.AuthenticationHelper.SendWithNtAuthAsync(HttpRequestMessage request, Uri authUri, Boolean async, ICredentials credentials, Boolean isProxyAuth, HttpConnection connection, HttpConnectionPool connectionPool, CancellationToken cancellationToken)
   at System.Net.Http.HttpConnectionPool.SendWithRetryAsync(HttpRequestMessage request, Boolean async, Boolean doRequestAuth, CancellationToken cancellationToken)
   at System.Net.Http.AuthenticationHelper.SendWithAuthAsync(HttpRequestMessage request, Uri authUri, Boolean async, ICredentials credentials, Boolean preAuthenticate, Boolean isProxyAuth, Boolean doRequestAuth, HttpConnectionPool pool, CancellationToken cancellationToken)
   at System.Net.Http.RedirectHandler.SendAsync(HttpRequestMessage request, Boolean async, CancellationToken cancellationToken)
   at System.Net.Http.HttpClient.SendAsyncCore(HttpRequestMessage request, HttpCompletionOption completionOption, Boolean async, Boolean emitTelemetryStartStop, CancellationToken cancellationToken)
   at System.Net.Http.HttpClient.GetStringAsyncCore(HttpRequestMessage request, CancellationToken cancellationToken)
   at SadHttpClient.Program.Main(String[] _) in C:\local\SadHttpClient\Program.cs:line 24
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Net.Http untriaged New issue has not been triaged by the area owner labels Aug 26, 2021
@ghost
Copy link

ghost commented Aug 26, 2021

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

This is a continuation of #56159 (comment), where we reported that we were observing ObjectDisposedException when using SocketsHttpHandler 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 and Content-Length - we were observing different errors with SocketsHttpHandler:

  • If Connection: close is present, then everything is OK with SocketsHttpHandler (regardless of Content-Length).
  • If Connection: close is absent, and
    • Content-Length is also absent, then SocketsHttpHandler throws an ObjectDisposedException. (I realize that a correct web server implementation should return a Content-Length or Transfer-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 get HttpRequestException: 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

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

  • The code was tested on Windows 10 1909 64-bit.
  • The server for the reproducer targeted net5.0 (although we've managed to repro the issue with other non-.NET web server implementations).
  • The client targeted various .NET runtimes (net48, netcoreapp3.1 (with/without SocketsHttpHandler), 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 disable SocketsHttpHandler).

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.
PS C:\Users\redacted> curl.exe -vvv --negotiate -u : "http://localhost:4444/"
*   Trying ::1...
* TCP_NODELAY set
*   Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 4444 (#0)
> GET / HTTP/1.1
> Host: localhost:4444
> User-Agent: curl/7.55.1
> Accept: */*
>
< HTTP/1.1 401 Unauthorized
< Content-Type: text/plain
< WWW-Authenticate: Negotiate
* no chunk, no close, no size. Assume close to signal end
<
* Closing connection 0
* Issue another request to this URL: 'http://localhost:4444/'
* Hostname localhost was found in DNS cache
*   Trying ::1...
* TCP_NODELAY set
*   Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 4444 (#1)
* Server auth using Negotiate with user ''
> GET / HTTP/1.1
> Host: localhost:4444
> Authorization: Negotiate <SNIP>
> User-Agent: curl/7.55.1
> Accept: */*
>
< HTTP/1.1 200 OK
< Content-Type: text/plain
* no chunk, no close, no size. Assume close to signal end
<
Successful* Closing connection 1
Output from .NET Framework / WinHttpHandler client with both Content-Length and Connection: close missing.
Runtime Path: C:\Windows\Microsoft.NET\Framework64\v4.0.30319
Press q to exit, or any other key to poke http://localhost:4444
Successful
Stack Trace with both Content-Length and Connection: close missing when using SocketsHttpHandler.
Runtime Path: C:\Program Files\dotnet\shared\Microsoft.NETCore.App\5.0.9
Press q to exit, or any other key to poke http://localhost:4444
System.Net.Http.HttpRequestException: An error occurred while sending the request.
 ---> System.ObjectDisposedException: Cannot access a disposed object.
Object name: 'System.Net.Sockets.NetworkStream'.
   at System.Net.Sockets.NetworkStream.<ThrowIfDisposed>g__ThrowObjectDisposedException|63_0()
   at System.Net.Sockets.NetworkStream.WriteAsync(ReadOnlyMemory`1 buffer, CancellationToken cancellationToken)
   at System.Net.Http.HttpConnection.WriteToStreamAsync(ReadOnlyMemory`1 source, Boolean async)
   at System.Net.Http.HttpConnection.FlushAsync(Boolean async)
   at System.Net.Http.HttpConnection.SendAsyncCore(HttpRequestMessage request, Boolean async, CancellationToken cancellationToken)
   --- End of inner exception stack trace ---
   at System.Net.Http.HttpConnection.SendAsyncCore(HttpRequestMessage request, Boolean async, CancellationToken cancellationToken)
   at System.Net.Http.AuthenticationHelper.SendWithNtAuthAsync(HttpRequestMessage request, Uri authUri, Boolean async, ICredentials credentials, Boolean isProxyAuth, HttpConnection connection, HttpConnectionPool connectionPool, CancellationToken cancellationToken)
   at System.Net.Http.HttpConnectionPool.SendWithRetryAsync(HttpRequestMessage request, Boolean async, Boolean doRequestAuth, CancellationToken cancellationToken)
   at System.Net.Http.AuthenticationHelper.SendWithAuthAsync(HttpRequestMessage request, Uri authUri, Boolean async, ICredentials credentials, Boolean preAuthenticate, Boolean isProxyAuth, Boolean doRequestAuth, HttpConnectionPool pool, CancellationToken cancellationToken)
   at System.Net.Http.RedirectHandler.SendAsync(HttpRequestMessage request, Boolean async, CancellationToken cancellationToken)
   at System.Net.Http.HttpClient.SendAsyncCore(HttpRequestMessage request, HttpCompletionOption completionOption, Boolean async, Boolean emitTelemetryStartStop, CancellationToken cancellationToken)
   at System.Net.Http.HttpClient.GetStringAsyncCore(HttpRequestMessage request, CancellationToken cancellationToken)
   at SadHttpClient.Program.Main(String[] _) in C:\local\SadHttpClient\Program.cs:line 24
Stack Trace with Content-Length set, but Connection: close missing when using SocketsHttpHandler.
Runtime Path: C:\Program Files\dotnet\shared\Microsoft.NETCore.App\5.0.9
Press q to exit, or any other key to poke http://localhost:4444
System.Net.Http.HttpRequestException: Authentication failed because the connection could not be reused.
   at System.Net.Http.HttpConnection.DrainResponseAsync(HttpResponseMessage response, CancellationToken cancellationToken)
   at System.Net.Http.AuthenticationHelper.SendWithNtAuthAsync(HttpRequestMessage request, Uri authUri, Boolean async, ICredentials credentials, Boolean isProxyAuth, HttpConnection connection, HttpConnectionPool connectionPool, CancellationToken cancellationToken)
   at System.Net.Http.HttpConnectionPool.SendWithRetryAsync(HttpRequestMessage request, Boolean async, Boolean doRequestAuth, CancellationToken cancellationToken)
   at System.Net.Http.AuthenticationHelper.SendWithAuthAsync(HttpRequestMessage request, Uri authUri, Boolean async, ICredentials credentials, Boolean preAuthenticate, Boolean isProxyAuth, Boolean doRequestAuth, HttpConnectionPool pool, CancellationToken cancellationToken)
   at System.Net.Http.RedirectHandler.SendAsync(HttpRequestMessage request, Boolean async, CancellationToken cancellationToken)
   at System.Net.Http.HttpClient.SendAsyncCore(HttpRequestMessage request, HttpCompletionOption completionOption, Boolean async, Boolean emitTelemetryStartStop, CancellationToken cancellationToken)
   at System.Net.Http.HttpClient.GetStringAsyncCore(HttpRequestMessage request, CancellationToken cancellationToken)
   at SadHttpClient.Program.Main(String[] _) in C:\local\SadHttpClient\Program.cs:line 24
Author: kedia990
Assignees: -
Labels:

area-System.Net.Http, untriaged

Milestone: -

@kedia990 kedia990 changed the title Regession in SocketsHttpHandler when establishing connection with Negotiate Regression in SocketsHttpHandler when establishing connection with Negotiate Aug 26, 2021
@geoffkizer
Copy link
Contributor

It sounds like this is not a regression from previous SocketsHttpHandler behavior (3.1/5.0)... is that correct?

  • If Connection: close is absent, and

    • Content-Length is also absent, then SocketsHttpHandler throws an ObjectDisposedException. (I realize that a correct web server implementation should return a Content-Length or Transfer-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.)

Yeah, this is invalid server behavior, but we should not throw ODE here. Do you know if this happens even without auth?

  • Content-Length is present, then we get HttpRequestException: Authentication failed because the connection could not be reused..

We need to investigate. We should be draining the response body in this case. @wfurt any thoughts?

@wfurt
Copy link
Member

wfurt commented Aug 26, 2021

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.
We can perhaps tackle #31133 together.

@kedia990
Copy link
Author

It sounds like this is not a regression from previous SocketsHttpHandler behavior (3.1/5.0)... is that correct?

Correct: it's a regression in SocketsHttpHandler relative to WinHttpHandler, but the SocketsHttpHandler in all .NET Core versions I've tested so far are similarly affected. I haven't tested .NET 6 Preview yet; if someone on your team doesn't beat me to it, I might do that over the next couple of days and get back here with the results.

Yeah, this is invalid server behavior, but we should not throw ODE here. Do you know if this happens even without auth?

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.

@geoffkizer
Copy link
Contributor

Thanks, the data and code you provided is very useful. If we need more info we will let you know.

@geoffkizer
Copy link
Contributor

Re this:

  • Content-Length is present, then we get HttpRequestException: Authentication failed because the connection could not be reused..

It looks like you're sending more data than you specified in the Content-Length. You specify 12 bytes, but then send "Unauthorized" + crLf which is 14 bytes.

@geoffkizer
Copy link
Contributor

Re this:

If Connection: close is absent, and

  • Content-Length is also absent, then SocketsHttpHandler throws an ObjectDisposedException. (I realize that a correct web server implementation should return a Content-Length or Transfer-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.)

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.

@kedia990
Copy link
Author

Re this:

  • Content-Length is present, then we get HttpRequestException: Authentication failed because the connection could not be reused..

It looks like you're sending more data than you specified in the Content-Length. You specify 12 bytes, but then send "Unauthorized" + crLf which is 14 bytes.

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 Content-Length mitigates the HttpRequestException in that scenario, even without Connection: close. Surprising that WinHttpHandler tolerated that.

FWIW, I'm still observing a discrepancy between SocketsHttpHandler and WinHttpHandler if Connection: close or Keep-Alive isn't returned by a non-.NET HTTP server (uWSGI, to be specific), but I haven't fully understood that yet.

@wfurt
Copy link
Member

wfurt commented Aug 27, 2021

One thing to check for is if WinHttphandler reused the same connection and used session based NTLM as authentication.
The problem is that if we cannot properly find boundaries for request/responses I don't see good way how to reuse the connection.

@karelz
Copy link
Member

karelz commented Aug 31, 2021

Triage: This is misbehaving server. We could treat such servers slightly better as @geoffkizer suggested above: #58179 (comment)
Does not seem like priority.

@kedia990 do you know why the servers are misbehaving? Which server is that?

@karelz karelz added this to the 7.0.0 milestone Aug 31, 2021
@karelz karelz added enhancement Product code improvement that does NOT require public API changes/additions and removed untriaged New issue has not been triaged by the area owner labels Aug 31, 2021
@geoffkizer
Copy link
Contributor

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.

@geoffkizer
Copy link
Contributor

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.

@wfurt
Copy link
Member

wfurt commented Sep 1, 2021

would this logic work also for #31133 ?

@geoffkizer
Copy link
Contributor

would this logic work also for #31133 ?

I think so, but I'm not 100% sure.

@kedia990
Copy link
Author

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 SocketsHttpHandler more robust when faced with slightly adverse server responses (which we certainly cannot rule out or control in all circumstances, and which WinHttpHandler is able to deal with reasonably well), then I think that'd be generally beneficial.

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.

Which server is that?

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

$ uwsgi --http :4444 --wsgi-file myserver.py

Then, point the client (code above) at this server and fire away.

@karelz
Copy link
Member

karelz commented Jul 21, 2022

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.

@karelz karelz modified the milestones: 7.0.0, Future Jul 21, 2022
@brendonparker
Copy link

Also running into this with a console app using HttpClient against a ADFS wia endpoint. Works on full framework. Not with dotnet6.

@wfurt
Copy link
Member

wfurt commented Sep 19, 2022

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

@brendonparker
Copy link

Sure. Totally possible I am doing something wrong...
Let me know if there is another piece of data I need to include in the output.

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:

StatusCode: 200 (OK)
Headers:
 - Pragma: no-cache
 - X-Frame-Options: DENY
 - Cache-Control: no-store, no-cache
 - Date: Mon, 19 Sep 2022 18:57:21 GMT
 - P3P: ADFS doesn't have P3P policy, please contact your site's admin for more details
 - Set-Cookie: MSISAuth=REDACTED; path=/adfs; HttpOnly; Secure; SameSite=None
 - Server: Microsoft-HTTPAPI/2.0
 - WWW-Authenticate: REDACTED

Here is the output for dotnet6

StatusCode: 401 (Unauthorized)
Headers:
 - Server: Microsoft-HTTPAPI/2.0
 - WWW-Authenticate: Negotiate, NTLM
 - Date: Mon, 19 Sep 2022 18:56:55 GMT

@wfurt
Copy link
Member

wfurt commented Sep 19, 2022

It is interesting that the server offers WWW-Authenticate: Negotiate, NTLM to 6.0.
However, with 4.7 you get WWW-Authenticate: REDACTED.
I'm wondering if there is some issue with processing the cookies...?

@wfurt
Copy link
Member

wfurt commented Sep 19, 2022

BTW If you need to peek inside of the SSL https://www.telerik.com/fiddler is great tool to do that.

@brendonparker
Copy link

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.

@wfurt
Copy link
Member

wfurt commented Sep 20, 2022

It may be possible. description in the middle can break TokenBinding (if enabled) and NTLM use session based authentication and the proxy can change that as well.
Perhaps traditional tracing? I'm mostly curious if the 4.7 really used Negotiate and if so, what mechanism was selected (and what SPN was used)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Net.Http enhancement Product code improvement that does NOT require public API changes/additions
Projects
None yet
Development

No branches or pull requests

5 participants