[release/2.1] Expect100Continue with Windows auth and request body > 1024 bytes fails

[mconnew]

In this code: https://github.com/dotnet/corefx/blob/7ee84596d92e178bce54c986df31ccc52479e772/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs#L534-L549
If the Expect100Continue header is set, and the response code is >= 300 and there's a request body with known length greater than 1024 bytes, if the request body hasn't begun to be sent yet, it sets _connectionClose to true. When using authentication, the challenge response comes with an HTTP status code of 401. As (401 >= 300) evaluates to true, whenever using authentication with request body size > 1024 bytes, _connectionClose will be set to true. In this code: https://github.com/dotnet/corefx/blob/7ee84596d92e178bce54c986df31ccc52479e772/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/AuthenticationHelper.NtAuth.cs#L68-L75
If there was a valid authentication challenge and a response is available to send, a call is made to HttpConnection.DrainResponseAsync. In that method: https://github.com/dotnet/corefx/blob/7ee84596d92e178bce54c986df31ccc52479e772/src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs#L1528-L1531
If _connectionClose is true, it throws and aborts the request.
This is breaking Windows authentication for WCF with requests which are larger than 1024.

[davidsh]

cc: @stephentoub @geoffkizer

[karelz]

@mconnew mentioned to me offline that it is a regression 2.0->2.1
What is the impact on WCF scenarios/customers? How common are the scenarios?
Are there any workarounds on WCF side or on customer side?
(just trying to understand the scope & impact)

[davidsh]

@mconnew mentioned to me offline that it is a regression 2.0->2.1

It could only be a regression on Linux from using CurlHandler on .NET Core 2.0 (which supported Expect: 100-continue) to using SocketsHttpHandler on .NET Core 2.1.

On Windows, with .NET Core 2.0 (WinHttpHandler), Expect: 100-continue is not supported.

[mconnew]

@davidsh, while Expect: 100-Continue is not supported by WinHTTP in actually implementing the functionality to hold off sending the request body, if you add the Expect header, it will still functionally be able to send the request and get the response when using authentication. Now on Windows, that is no longer the case. So while the purpose of the header couldn't be used, things were still functional if you did use it. So this is a regression on Windows in that you could get a response before, you can't now.

[geoffkizer]

@mconnew, I'm curious: Why are you setting Expect: 100-continue in this scenario?

[mconnew]

Without it, the entire request body will be sent for the initial request and for the challenge response. If the request message is for example 10MB, that's double the data you are then sending and double the amount of time it takes to get a response. Additionally, according to the customer who saw this issue, in their testing setting Connection: Close header without Expect: 100-Continue also has the same problem.

[karelz]

@mconnew can you please answer the questions above for us to prioritize it properly?

• What is the impact on WCF scenarios/customers? How common are the scenarios?
• Are there any workarounds on WCF side or on customer side?

[mconnew]

Impact
Anybody using Windows authentication with requests > 1024 bytes. I don't know how common this is as we don't have any telemetry around which authentication method is used.

Work around
Revert to earlier release of WCF which didn't use the Expect header. In an earlier release we sent a HEAD request before every real request as an attempted mitigation to the problems caused by lack of universal support for the Expect header. So basically roll back. The prior approach with the HEAD request was only best effort and would still sometimes have problems as well as introduced new problems.

[karelz]

I don't know how common this is as we don't have any telemetry around which authentication method is used.

If you have customers complaining, let's use that as indicator. How many impacted so far? Is there public issue where it happens or is it internal via support?
How badly is the specific customer impacted? (RSP/SLA/other metric) Or was it just something that was noticed during testing?

Regarding workaround: Does it mean "use older WCF package"? Is that even supported scenario?

[mconnew]

The issue where this is reported is dotnet/wcf#2923 and there's 3 customers there who have reported it. I would expect any new instances of customers having the problem would see that issue and use the workaround.
Using the older WCF package is completely supported. We aren't a part of .Net Core, we're more like a third party library which also happens to have an implementation in the full framework when your runtime is netfx. That means all earlier WCF packages can run on later versions of .NET Core. In addition to that, as we target .NET standard and not .NET Core and .NET Standard is still at 2.0, as I made sure we didn't add any usage of classes added to .NET Core 2.1 someone can use our latest package on .NET Core 2.0 and they wouldn't get any runtime type errors. They might get functional problems if there's missing capabilities in e.g. HttpClientHandler, but it should generally run fine both ways.

[mconnew]

Btw, 3 reported issues in a short period of time is quite high for WCF.

[danmoseley]

@karelz if this is to go into 2.1.x it will need a mail to corefx shiproom. Do you plan to do that?

[danmoseley]

Removing label until this is ready for big shiproom.

[geoffkizer]

Additionally, according to the customer who saw this issue, in their testing setting Connection: Close header without Expect: 100-Continue also has the same problem.

Do you expect this (i.e. setting Connection: close and doing NT auth) to work?

[geoffkizer]

@mconnew, I'm not entirely sure what you want us to do in this scenario. When we get an early response with Expect: 100-continue, we either need to (a) finish sending the request body, so we can keep the connection alive, or (b) close the connection and stop sending the request body.

[mconnew]

@geoffkizer, the current solution means that you can't use use NTLM authentication with the Expect header. It simply can't work as it abandons the request and throws an HttpRequestException at the caller. I ran some experiments and recorded what HttpWebRequest does on the .Net Framework so I'll give a quick overview here.

1. --> Client sends headers including an Expect: 100-continue header
2. <-- Server responds with 401 status and WWW-Authenticate: NTLM header (no blob)
3. --> Client sends the request body (to keep socket open)
4. --> Client sends request headers including Authorization header with initial NTLM authorization blob, no expect header and Content-Length: 0 header (no expect header because no request body).
5. <-- Server responds with a 401 status and a WWW-Authenticate header with NTLM response blob
6. --> Client sends request headers including Authorization header with final NTLM authorization blob, the correct Content-Length header and an Expect: 100-continue header
7. <-- Server responds with a 100 status
8. --> Client sends request body
9. <-- Server responds with final response status, headers and body

This is the minimum that should be done. I believe that this can be optimized further. I suspect in step 3 the client could abandon the socket as there's no state information about the authentication that has been transmitted yet. This is the general idea that the current code attempts to do. But the current implementation then throws an HttpRequestException at the caller and there's no way to complete the request. Instead it should close the connection, not throw and create a new connection instead. Although the current limit of 1024 is too small as that is less than a single ethernet frame payload. Ideally the RTT would be queried from the OS and a heuristic used to work out which approach would be quicker but the System.Net.Socket abstraction is too far removed from the OS to query this information.

Additionally, when PreAuthenticate has been enabled and a credentials cache is used which only contains NTLM credentials, the first request can probably be skipped and can jump straight to step 4 as the server doesn't provide any challenge data in the initial authentication challenge. That would avoid sending the request body completely until after the authentication is completed. There is a risk that some servers could initialize some state when replying with the empty NTLM challenge, but that's easily tested and could be turned off by not using setting PreAuthenticate to true if some implementation failed with this optimization.

[danmoseley]

@karelz @Caesar1995 should this still be 2.1.x? Is it being actively worked on?

[karelz]

Sorry for delayed updates. It is still on our backlog, we hope to get back to it soon.

[mconnew]

WCF has another customer hitting this issue dotnet/wcf#3185

[mconnew]

@karelz, do you have any idea of timeline to fix this issue? The cause of this bug is well understood, the behavior on .NET Framework is understood as I investigated and documented what that behavior is. This bug is now over 6 months old and is blocking multiple customers and there's been no updates for 3 months.

[caesar-chen]

Hi @mconnew , one thing I don't understand:

1. About why setting Expect: 100-continue in this scenario. You mentioned:

Without it, the entire request body will be sent for the initial request and for the challenge response. ...that's double the data you are then sending and double the amount of time it takes to get a response

But from the experiment with .NET Framework, seems data need to be sent twice on Framework, right?

3. --> Client sends the request body (to keep socket open)
...
8. --> Client sends request body

I agree that the current limit of 1024 is too small - maybe the fix here can be more robust than simply increasing the limit. For example, special case some HTTP status codes.

[mconnew]

That's the purpose of the Expect header but the .NET Framework doesn't achieve this. I believe there are multiple optimizations that can be done to improve on what is done in the .NET Framework. For example, in step 2, the server isn't providing an authentication blob with the NTLM challenge header so there's no persistent state tied to the socket. You could do a variation of the current .NET Core behavior. You could send the body if it's less than 1024 bytes (to save TCP and TLS handshake cost if aborting the socket), or close the socket without sending the body. If you close the socket, don't complete the request back to the client with an Exception (which is what happens today) but instead make a new connection to the server and send the request of step 4 to the server with the authentication blob. This would avoid sending a large request more than once.
My preference would be if pre-authentication is enabled to start at step 4. You save an entire round-trip when the client has specified "I know this endpoint does need authentication and I've provided the credentials to use".

[caesar-chen]

You could send the body if it's less than 1024 bytes

Yes that's our current behavior.

If you close the socket, don't complete the request back to the client with an Exception

I think there is no clean way to do this: looks like we need to special case Authentication case.

Does disable 100-Continue work around this case? (request body sent twice - match .NET Framework behavior and doesn't regress it) Is this still blocking partner? We can do the optimization you mentioned about in 3.0.

[mconnew]

Disabling 100-Continue does avoid the issue but that's not something a developer can do very easily. When 100-Continue support was added to HttpClient, I modified WCF to match the behavior we have on .NET Framework. We don't have any API to change that behavior as on .NET Framework you can do this using ServicePointManager so WCF doesn't have any API's exposed to modify this behavior.
This is still blocking partner as our customers are continuing to have problems with this. Look at the issue in WCF where we are tracking this (dotnet/wcf#2923) to see how many people have problem.
On .NET Framework it does send the request twice on the authentication handshake so that behavior is acceptable to exist for fixing in 2.1+ (as 2.1 is a LTS so needs the functional break fixed there). I see my suggestion of the added optimization to remove the second send as being an improvement beyond what .NET Framework does so that's fine to be in 3.0. I'm not looking to retroactively add features 😄 .

[TPucke]

I'm a customer, new to this issue. I updated my application dependencies (core 2.2 and wcf 4.5.3) today and promptly crashed my application. I'm here to vote for prioritization. Any thoughts on a timeline to fix? Thanks.

[JWH89]

Customer here, I would love to see this issue resolved. When starting a new project we shouldn't have to check if every workaround is still required. This issue is still very much there and open for quite sometime now and needs to be resolved :)

[stofte]

I just found my function app stuffed full of classes like "PatchedHttpBinding". What a mess.

[davidsh]

Fixed in master for .NET Core 3.0 with PR dotnet/corefx#38744

Re-opening for release/2.1 and release/2.2 consideration.

[davidsh]

Fix for release/2.1 checked in with PR dotnet/corefx#39341

[karelz]

The fix will be part of 2.1.13 and 2.2.7 releases.