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

Handle web socket downgrade when HTTP/2 not enabled #73843

Merged

Conversation

greenEkatherine
Copy link
Contributor

During manual testing of #73762 @BrennanConroy and me detected that in the case when HTTP/2 connection is not supported downgrade for web sockets is not working as expected.

In that case ClientWebSocket didn't get exception data indicating that downgrade is possible, so I added this data and tests for it.

However, it doesn't handle plain text scenario because HttpClient in that case silently create Http2Connection with no chance to detect the real version before we try to establish extended connect. If downgrade allowed by policy HttpClient force 1.1 without giving 2.0 a try -- maybe ClientWebSocket should do the same.

@ghost
Copy link

ghost commented Aug 12, 2022

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

Issue Details

During manual testing of #73762 @BrennanConroy and me detected that in the case when HTTP/2 connection is not supported downgrade for web sockets is not working as expected.

In that case ClientWebSocket didn't get exception data indicating that downgrade is possible, so I added this data and tests for it.

However, it doesn't handle plain text scenario because HttpClient in that case silently create Http2Connection with no chance to detect the real version before we try to establish extended connect. If downgrade allowed by policy HttpClient force 1.1 without giving 2.0 a try -- maybe ClientWebSocket should do the same.

Author: greenEkatherine
Assignees: -
Labels:

area-System.Net.Http

Milestone: -

@greenEkatherine greenEkatherine changed the title Handle downgrade when HTTP/2 not enabled Handle web socket downgrade when HTTP/2 not enabled Aug 12, 2022
Katya Sokolova and others added 2 commits August 12, 2022 16:44
…andler/HttpConnectionPool.cs

Co-authored-by: Natalia Kondratyeva <[email protected]>
Copy link
Member

@CarnaViire CarnaViire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@Tratcher
Copy link
Member

If downgrade allowed by policy HttpClient force 1.1 without giving 2.0 a try -- maybe ClientWebSocket should do the same.

Yes, ClientWebSocket should follow the same rules as HttpClient here.

@MihaZupan
Copy link
Member

I agree we should match SocketsHttpHandler

if (_http2Enabled &&
(request.Version.Major >= 2 || (request.VersionPolicy == HttpVersionPolicy.RequestVersionOrHigher && IsSecure)) &&
(request.VersionPolicy != HttpVersionPolicy.RequestVersionOrLower || IsSecure)) // prefer HTTP/1.1 if connection is not secured and downgrade is possible

If the scheme is not secure and the policy allows 1.1, we should not attempt 2.0.

@karelz karelz added this to the 7.0.0 milestone Aug 12, 2022
@greenEkatherine
Copy link
Contributor Author

I added condition to force to 1.1 non-secure web socket requests in ClientWebSocket @MihaZupan @Tratcher

@MihaZupan
Copy link
Member

Does HTTP/2 over cleartext work with WebSockets right now?

SocketsHttpHandler requires that you use set both Version=2.0 and Policy=Exact for that.
From what I can see, we set only the version in WebSockets:

request = new HttpRequestMessage(HttpMethod.Connect, uri) { Version = HttpVersion.Version20 };

@greenEkatherine
Copy link
Contributor Author

Does HTTP/2 over cleartext work with WebSockets right now?

SocketsHttpHandler requires that you use set both Version=2.0 and Policy=Exact for that. From what I can see, we set only the version in WebSockets:

request = new HttpRequestMessage(HttpMethod.Connect, uri) { Version = HttpVersion.Version20 };

VersionPolicy is set up later in the AddWebSocketHeaders

request.VersionPolicy = HttpVersionPolicy.RequestVersionExact;

@dotnet dotnet deleted a comment from azure-pipelines bot Aug 12, 2022
@dotnet dotnet deleted a comment from azure-pipelines bot Aug 12, 2022
@greenEkatherine
Copy link
Contributor Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines

This comment was marked as outdated.

@dotnet dotnet deleted a comment from azure-pipelines bot Aug 12, 2022
@greenEkatherine greenEkatherine force-pushed the fix-downgrade-http2-not-enabled branch from b8d1c66 to 630bd7a Compare August 12, 2022 18:12
@greenEkatherine
Copy link
Contributor Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@greenEkatherine
Copy link
Contributor Author

Failures in the outerloop builds are unrelated

@greenEkatherine greenEkatherine merged commit 3824cb2 into dotnet:main Aug 12, 2022
@greenEkatherine greenEkatherine deleted the fix-downgrade-http2-not-enabled branch August 12, 2022 21:04
@ghost ghost locked as resolved and limited conversation to collaborators Sep 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants