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

WebSockets downgrade to 1.1 on network failure during TLS handshake #75399

Open
MihaZupan opened this issue Sep 10, 2022 · 3 comments
Open

WebSockets downgrade to 1.1 on network failure during TLS handshake #75399

MihaZupan opened this issue Sep 10, 2022 · 3 comments

Comments

@MihaZupan
Copy link
Member

MihaZupan commented Sep 10, 2022

Managed WebSocket will attempt to downgrade to HTTP/1.1 if we failed to establish HTTP/2 and the version policy permits.

The logic currently sets HTTP2_ENABLED exception data if we fail to establish TLS:

// Extended connect request is negotiating strictly for ALPN = "h2" because HttpClient is unaware of a possible downgrade.
// At this point, SSL connection for HTTP / 2 failed, and the exception should indicate the reason for the external client / user.
ex.Data["HTTP2_ENABLED"] = false;

This is weird. That failure could be caused by the network and has nothing to do with whether the server supports HTTP/2.
As a result, we will end up retrying on HTTP/1.1 even though we normally wouldn't (and SocketsHttpHandler wouldn't either).

We should be able to just delete this check entirely - actual ALPN issues will be caught later in HttpConnectionPool when we attempt to retry an HTTP/2-only request on the downgraded 1.1 connection.

The current downgrade tests don't exercise this as they are connecting to an http endpoint so we never even attempt HTTP/2.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Sep 10, 2022
@ghost
Copy link

ghost commented Sep 10, 2022

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

Issue Details

Managed WebSocket will attempt to downgrade to HTTP/1.1 if we failed to establish HTTP/2 and the version policy permits.

The logic currently sets HTTP2_ENABLED exception data if we fail to establish TLS:

// Extended connect request is negotiating strictly for ALPN = "h2" because HttpClient is unaware of a possible downgrade.
// At this point, SSL connection for HTTP / 2 failed, and the exception should indicate the reason for the external client / user.
ex.Data["HTTP2_ENABLED"] = false;

This is wrong. That failure could be caused by the network and has nothing to do with whether the server supports HTTP/2.
We should be able to just delete this check entirely - actual ALPN issues will be caught later in HttpConnectionPool when we attempt to retry an HTTP/2-only request on the downgraded 1.1 connection.

The current downgrade test does not exercise this. It connects to an http endpoint so we never even attempt HTTP/2.
dotnet/aspnetcore#43851 is adding many E2E tests that better cover downgrades.

Author: MihaZupan
Assignees: -
Labels:

bug, area-System.Net

Milestone: -

@MihaZupan MihaZupan removed the bug label Sep 10, 2022
@MihaZupan MihaZupan changed the title WebSockets gives up on HTTP/2 on network failure during handshake WebSockets downgrade to 1.1 on network failure during TLS handshake Sep 10, 2022
@MihaZupan MihaZupan added the bug label Sep 11, 2022
@karelz
Copy link
Member

karelz commented Sep 13, 2022

Triage: This may be by design (due to implementation), and if it is not, then the impact on customers is super-small - we will establish 1.1 connection in more cases than we should (when H/2 connection failed due to network error).
Either way, not critical for 7.0. We should decide in 8.0 if it is by design, or if we need a change.

@karelz karelz added this to the 8.0.0 milestone Sep 13, 2022
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Sep 13, 2022
@karelz karelz modified the milestones: 8.0.0, Future Jun 9, 2023
@karelz karelz modified the milestones: Future, 9.0.0 Jul 18, 2023
@MihaZupan MihaZupan modified the milestones: 9.0.0, Future Jun 4, 2024
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants