Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

don't close connection for responses to session based authentication and 100Continue #38744

Merged
merged 4 commits into from
Jul 9, 2019
Merged

don't close connection for responses to session based authentication and 100Continue #38744

merged 4 commits into from
Jul 9, 2019

Conversation

wfurt
Copy link
Member

@wfurt wfurt commented Jun 21, 2019

This is minimal fix. We don't have unit test infrastructure for NTLM/NEGO authentication so I tested this with my real server on Windows.

I talked with @mconnew about possible optimizations but they are somewhat difficult because of layering in SocketHttpHandler. With window for 3.0 closing, I'm proposing this change to make it work, even if we may need to send more data and we can possibly improve it post 3.0.

fixes #30760

@wfurt wfurt requested a review from a team June 21, 2019 01:45
@wfurt wfurt self-assigned this Jun 21, 2019
@davidsh davidsh requested a review from stephentoub June 21, 2019 02:26
@wfurt wfurt requested a review from davidsh July 2, 2019 08:39
@wfurt
Copy link
Member Author

wfurt commented Jul 2, 2019

I pushed another update to process sendRequestContentTask (if needed) after processing response headers.

In normal case, we should either get 100 response or timer will kick in for sending request body. So the slight delay for paring response headers should not matter. That seems better choice than special case 401 handling.

It seems like I missed this originally as our tests cannot distinguish between cases when we send response by timer or because of response logic.

It would be great if @davidsh can help with validation in enterprise auth setup.

@davidsh
Copy link
Contributor

davidsh commented Jul 9, 2019

/azp run

@davidsh davidsh self-assigned this Jul 9, 2019
@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@davidsh
Copy link
Contributor

davidsh commented Jul 9, 2019

It would be great if @davidsh can help with validation in enterprise auth setup.

I am going to run a few more validations of this. I think this is an important fix for .NET Core 3.0.

@davidsh davidsh added this to the 3.0 milestone Jul 9, 2019
@mconnew
Copy link
Member

mconnew commented Jul 9, 2019

As this is such a small fix, could this be back ported to .NET Core 2.1 as it's the current LTS release?

@davidsh
Copy link
Contributor

davidsh commented Jul 9, 2019

As this is such a small fix, could this be back ported to .NET Core 2.1 as it's the current LTS release?

Yes. I think this meets the bar for a .NET Core 2.1/2.2 servicing fix.

@davidsh
Copy link
Contributor

davidsh commented Jul 9, 2019

CI failures appear unrelated.

@davidsh davidsh merged commit 2e73965 into dotnet:master Jul 9, 2019
davidsh added a commit to davidsh/corefx that referenced this pull request Jul 9, 2019
@davidsh
Don't close connection for responses to NT auth using Expect: 100-con…
093d664 
…tinue

Port fix from PR dotnet#38744 (.NET Core 3.0) to release/2.1 LTS branch.

Fixes #30760
davidsh added a commit to davidsh/corefx that referenced this pull request Jul 9, 2019
@davidsh
Don't close connection for responses to NT auth using 100continue
6d31d52 
Port fix from PR dotnet#38744 (.NET Core 3.0) to release/2.1 LTS branch.

Fixes #30760
@davidsh davidsh mentioned this pull request Jul 9, 2019
Merged
davidsh added a commit that referenced this pull request Jul 16, 2019
@davidsh
Don't close connection for responses to NT auth using 100continue (#3…
470d787 
…9341)

Port fix from PR #38744 (.NET Core 3.0) to release/2.1 LTS branch.

Fixes #30760
@wfurt wfurt deleted the ntlm_30760 branch June 15, 2020 18:20
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
@wfurt @davidsh
Don't close connection for responses to session based authentication …
c7c606c 
…and 100-Continue (dotnet/corefx#38744)

* don't close connection for responses to session based authenticatin

* move sendRequestContentTask

* feedback from review


Commit migrated from dotnet/corefx@2e73965
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stephentoub stephentoub stephentoub left review comments

@geoffkizer geoffkizer geoffkizer left review comments

@davidsh davidsh davidsh approved these changes

Assignees

@davidsh davidsh

@wfurt wfurt

Labels
None yet
Projects
None yet
Milestone
3.0
Development

Successfully merging this pull request may close these issues.

[release/2.1] Expect100Continue with Windows auth and request body > 1024 bytes fails
5 participants
@wfurt @davidsh @mconnew @stephentoub @geoffkizer