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

SocketsHttpHandler throws exception when authenticating proxy or server closes first 407 response #26461

Closed
davidsh opened this issue Jun 12, 2018 · 29 comments · Fixed by dotnet/corefx#31527
Assignees
Labels
bug tenet-compatibility Incompatibility with previous versions or .NET Framework
Milestone

Comments

@davidsh
Copy link
Contributor

davidsh commented Jun 12, 2018

Found this bug while investigating #26397.

Many servers and proxy servers that require authentication will send 'Connection: close' (and close the TCP connection) for the first 407 response. This is frequently true for hardware firewalls, etc.

See: https://www.cisco.com/c/en/us/support/docs/security/web-security-appliance/117931-technote-ntml.pdf

8 The proxy FINs this TCP socket. This is correct and normal.

SocketsHttpHandler doesn't have a problem with this for Basic or Digest schemes. But for Windows auth schemes such as Negotiate or NTLM, it will throw an exception:

System.Net.Http.HttpRequestException: Authentication failed because the connection could not be reused at System.Net.Http.HttpConnection.DrainResponseAsync(HttpResponseMessage response) in s:\GitHub\corefx\src\System.Net.Http\src\System\Net\Http\SocketsHttpHandler\HttpConnection.cs

https://github.com/dotnet/corefx/blob/f5c0d3f6e36ffd706b56b67a17699ca4ce0fca00/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs#L1534

It is true that Windows auth schemes require the connection to stay alive during the multi-leg challenge/response between client and server (or proxy). But that is only true once the challenge/response process starts. And that is after the client responds to the first 407 and sends a 'Proxy-Authorization' (or 'Authorization') header with a based64-encoded blob with the Negotiate or NTLM scheme.

Repro code showing an authenticating proxy and resulting in an exception while trying to connect to a destination HTTP server. Note: this doesn't repro if the destination server is HTTPS and thus CONNECT tunneling would be used.

static void Main()
{
    Console.WriteLine($"(Framework: {Path.GetDirectoryName(typeof(object).Assembly.Location)})");
    Socket listener = null;

    // Start a "proxy" server in the background.
    listener = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
    listener.Bind(new IPEndPoint(IPAddress.Loopback, 0));
    listener.Listen(int.MaxValue);
    var ep = (IPEndPoint)listener.LocalEndPoint;
    var proxyUri = new Uri($"http://{ep.Address}:{ep.Port}/");

    Task.Run(async () =>
    {
        while (true)
        {
            Socket s = await listener.AcceptAsync();
            var ignored = Task.Run(() =>
            {
                using (var ns = new NetworkStream(s))
                using (var reader = new StreamReader(ns))
                using (var writer = new StreamWriter(ns) { AutoFlush = true })
                {
                    int request = 1;
                    while (true)
                    {
                        string line = null;
                        while (!string.IsNullOrEmpty(line = reader.ReadLine()))
                        {
                            Console.WriteLine($"    [request:{request}] Server received: {line}");
                        }

                        Console.WriteLine($"    Server sending response\r\n");
                        writer.Write(
                            "HTTP/1.1 407 Proxy Authentication Required\r\n" +
                            "Proxy-Authenticate: NEGOTIATE\r\n" +
                            "Proxy-Authenticate: NTLM\r\n" +
                            "Cache-Control: no-cache\r\n" +
                            "Pragma: no-cache\r\n" +
                            "Proxy-Connection: close\r\n" +
                            "Connection: close\r\n" +
                            "Content-Length: 0\r\n\r\n");
                        request++;
                    }
                }
            });
        }
    });
            
    var serverUri = new Uri("http://corefx-net.cloudapp.net/echo.ashx/");

    var handler = new HttpClientHandler();
    handler.Proxy = new WebProxy(proxyUri);
    handler.Proxy.Credentials = CredentialCache.DefaultCredentials;
    using (var client = new HttpClient(handler))
    {
        Console.WriteLine($"Doing GET for {serverUri}");
        HttpResponseMessage response = client.GetAsync(serverUri).GetAwaiter().GetResult();
    }
}
@davidsh
Copy link
Contributor Author

davidsh commented Jun 12, 2018

Repro code showing an authenticating HTTP server and resulting in an exception:

static void Main()
{
    Console.WriteLine($"(Framework: {Path.GetDirectoryName(typeof(object).Assembly.Location)})");
    Socket listener = null;

    // Start a server in the background.
    listener = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
    listener.Bind(new IPEndPoint(IPAddress.Loopback, 0));
    listener.Listen(int.MaxValue);
    var ep = (IPEndPoint)listener.LocalEndPoint;
    var serverUri = new Uri($"http://{ep.Address}:{ep.Port}/");

    Task.Run(async () =>
    {
        while (true)
        {
            Socket s = await listener.AcceptAsync();
            var ignored = Task.Run(() =>
            {
                using (var ns = new NetworkStream(s))
                using (var reader = new StreamReader(ns))
                using (var writer = new StreamWriter(ns) { AutoFlush = true })
                {
                    int request = 1;
                    while (true)
                    {
                        string line = null;
                        while (!string.IsNullOrEmpty(line = reader.ReadLine()))
                        {
                            Console.WriteLine($"    [request:{request}] Server received: {line}");
                        }

                        Console.WriteLine($"    Server sending response\r\n");
                        writer.Write(
                            "HTTP/1.1 401 Authentication Required\r\n" +
                            "Www-Authenticate: NEGOTIATE\r\n" +
                            "Www-Authenticate: NTLM\r\n" +
                            "Cache-Control: no-cache\r\n" +
                            "Pragma: no-cache\r\n" +
                            "Connection: close\r\n" +
                            "Content-Length: 0\r\n\r\n");
                        request++;
                    }
                }
            });
        }
    });
            
    var handler = new HttpClientHandler();
    handler.Credentials = CredentialCache.DefaultCredentials;
    using (var client = new HttpClient(handler))
    {
        Console.WriteLine($"Doing GET for {serverUri}");
        HttpResponseMessage response = client.GetAsync(serverUri).GetAwaiter().GetResult();
    }
}

@davidsh
Copy link
Contributor Author

davidsh commented Jun 12, 2018

cc: @stephentoub @geoffkizer

@karelz
Copy link
Member

karelz commented Jun 12, 2018

@davidsh should we try to get it into 2.1.x too (once we have a fix)?

@karelz karelz closed this as completed Jun 12, 2018
@karelz karelz reopened this Jun 12, 2018
@davidsh
Copy link
Contributor Author

davidsh commented Jun 12, 2018

@davidsh should we try to get it into 2.1.x too (once we have a fix)?

Yes. And also dotnet/corefx#30330

@davidsh davidsh self-assigned this Jun 15, 2018
@danmoseley
Copy link
Member

@karelz if your team wants to propose a fix in servicing, we need a fix in master, PR prepped, mail to shiproom, etc.

@karelz
Copy link
Member

karelz commented Jul 10, 2018

This one is higher priority, @geoffkizer is working on it. We will let shiproom know once we have a fix. In the worst case 2.1.4+ :(

@karelz karelz assigned geoffkizer and unassigned davidsh Jul 10, 2018
@danmoseley
Copy link
Member

Removing label until we are ready to take to big shiproom.

@giggio
Copy link

giggio commented Jul 31, 2018

Is there a timeframe and target version to release a fix for this problem? It is affecting our upgrade to .NET Core.

davidsh referenced this issue in dotnet/corefx Aug 3, 2018
When we receive an initial NT auth challenge that has Connection: close set on the response, we need to proceed with authentication on a new connection.

Fixes #30327
@davidsh davidsh reopened this Aug 3, 2018
@davidsh
Copy link
Contributor Author

davidsh commented Aug 3, 2018

Is there a timeframe and target version to release a fix for this problem? It is affecting our upgrade to .NET Core.

@giggio We have fixed this bug in the master branch. We are working on a timeline for porting to the release/2.1.x servicing branch.

@davidsh davidsh assigned davidsh and unassigned geoffkizer Aug 3, 2018
@davidsh
Copy link
Contributor Author

davidsh commented Aug 3, 2018

Shiproom template

Description

Many servers and proxy servers that require Windows authentication (Negotiate or NTLM) will send 'Connection: close' (and close the TCP connection) for the first 401/407 response that contains the authorization schemes. This is frequently true for hardware appliances in enterprise environments.

Windows auth does require the connection to stay alive during the multi-leg challenge/response between client and server (or proxy). But that is only true once the challenge/response process starts and Base64 encoded authentication blobs are exchanged. Those exchanges happen after the client responds to the first 401/407 from the server.

SocketsHttpHandler was assuming that the connection would stay alive even after the initial 401/407 response from the proxy/server. When this doesn't happen, SocketsHttpHandler throws an exception and the HTTP request fails.

The fix is to detect when the initial 401/407 from the server or proxy results in a closed connection. If so, we need to proceed with authentication on a new connection. This involves changing some code logic from the HttpConnection object to the HttpConnectionPool object so that a new connection could be made.

Customer Impact

Without this fix, HTTP requests (which use SocketsHttpHandler by default) won’t work with many enterprise authenticating proxies or servers that use Windows auth.

Regression?

Regression from 2.0.

Risk

Low to medium risk due to moving code around from HttpConnection to HttpConnectionPool. These changes were regression tested in CI. But since enterprise scenarios can't be tested right now in CI, we ran many manual tests of these scenarios to validate this change.

Link to release/2.1.x PR

dotnet/corefx#31589

@Petermarcu
Copy link
Member

This is an important one for us to take. I'd like to get it in as soon as possible to get it baking.

@Petermarcu
Copy link
Member

Did you already get this change into Master? Given the level of risk, we need to figure out how to think about it in 2.1.4.

@davidsh
Copy link
Contributor Author

davidsh commented Aug 3, 2018

Did you already get this change into Master?

Yes. Change in master was merged today with PR dotnet/corefx#31527

@Petermarcu
Copy link
Member

Great, lets raise awareness on the next full stack build that has the change in it so we can make sure we get as much usage on it as possible.

@talanc
Copy link

talanc commented Aug 4, 2018

Will PR dotnet/corefx#31527 go into the daily build?

@davidsh
Copy link
Contributor Author

davidsh commented Aug 4, 2018

Will PR dotnet/corefx#31527 go into the daily build?

The PR went into master branch. So, when the branch is built, it will be available in those feeds. I'm not sure that full stack builds are done daily. But the next build will contain this fix.

@vivmishra
Copy link
Contributor

Approved for 2.1.5

@talanc
Copy link

talanc commented Aug 12, 2018

I installed the latest daily sdk (3.0.0-preview1-26710-03) and I'm getting this exception now:
System.Net.Http.HttpRequestException: Authentication failed because the connection could not be reused.

Full details are available here: https://github.com/talanc/dotnetcore-proxytest

@davidsh
Copy link
Contributor Author

davidsh commented Aug 13, 2018

That 3.0.0-preview build probably doesn't have the fix from the master branch. The builds are not really "daily".

@talanc
Copy link

talanc commented Aug 14, 2018

Is there an sdk with the fixes available? I'm keen to try out the fixes.

@davidsh
Copy link
Contributor Author

davidsh commented Aug 14, 2018

I installed the latest daily sdk (3.0.0-preview1-26710-03) and I'm getting this exception now

That build doesn't have the fix.

However, the latest SDK does have the fix:
https://github.com/dotnet/core-sdk/blob/master/README.md#installers-and-binaries

https://dotnetcli.blob.core.windows.net/dotnet/Sdk/master/dotnet-sdk-latest-win-x64.exe

S:\dotnet\proxytest>dotnet --info
.NET Core SDK (reflecting any global.json):
 Version:   3.0.100-alpha1-20180720-2
 Commit:    7f52497ea5

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.17134
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\3.0.100-alpha1-009424\

Host (useful for support):
  Version: 3.0.0-preview1-26813-02
  Commit:  8ab5510c4c

.NET Core SDKs installed:
  3.0.100-alpha1-009424 [C:\Program Files\dotnet\sdk]

.NET Core runtimes installed:
  Microsoft.AspNetCore.All 3.0.0-alpha1-10062 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.App 3.0.0-alpha1-10062 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 3.0.0-preview1-26813-02 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]

@danmoseley
Copy link
Member

Moving label to PR per new process

@talanc
Copy link

talanc commented Aug 20, 2018

@davidsh thanks -- i've tested the latest sdk and it works -- 3.0.0-preview1-26814-05

@talanc
Copy link

talanc commented Aug 25, 2018

I just tried 2.2-preview1 and it's not working there. Will 2.2 get a fix before it's final release?

@karelz
Copy link
Member

karelz commented Aug 27, 2018

@talanc the bug will be fixed even in 2.1.x servicing see PR dotnet/corefx#31589 - it has been already approved for 2.1.5 release.
All 2.1.x changes will flow into release/2.2 automatically, it just didn't happen yet.

@los93sol
Copy link

Any idea when this will be publicly available? We're experiencing the issue with dotnet restore

@karelz
Copy link
Member

karelz commented Aug 29, 2018

We do not have official date for 2.1.5 release. Once it is merged, you can use daily builds from 2.1 branch if you want to get unblocked ASAP.

Servicing releases are officially estimated at 1-2 months cadence: https://github.com/dotnet/core/blob/master/roadmap.md#upcoming-ship-dates
If you look at 2.1.x history, you can guess at rough dates for 2.1.5: https://github.com/dotnet/core/blob/master/release-notes/download-archive.md#net-core-21---lts-release (no promises of course).

@talanc
Copy link

talanc commented Aug 30, 2018

@los93sol I've been able to get around that on my local dev machine by using Fiddler with Automatically Authenticate turned on.

@karelz
Copy link
Member

karelz commented Aug 30, 2018

Fixed in PR dotnet/corefx#31589 in release/2.1 branch, it will be part of 2.1.5 release.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug tenet-compatibility Incompatibility with previous versions or .NET Framework
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants