HTTP Version Selection by ManickaP · Pull Request #39201 · dotnet/runtime

ManickaP · 2020-07-13T16:06:25Z

Behavior:

RequestVersionOrLower -- current behavior.

If HTTP/1 and no TLS, use HTTP/1.
If HTTP/1 and TLS, use HTTP/1.
If HTTP/2 and no TLS, use HTTP/1.
If HTTP/2 and TLS, use ALPN to negotiate between HTTP/1 and HTTP/2.

RequestVersionOrHigher -- start at the user's version.

If HTTP/1 and no TLS, use HTTP/1.
If HTTP/1 and TLS, use ALPN to negotiate between HTTP/1 and HTTP/2.

If HTTP/2 and no TLS, use HTTP/2 cleartext (H2C)

if the server doesn't support H2 (only recognizes H1) we get protocol error (observed with loopback server)

it's expected behavior as-is now; discussion: HttpClient: Improve logging and error messages when mismatch between client and server protocol/TLS #31132 (comment), code:

runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Connection.cs

Lines 199 to 211 in 1e23a06

    
           // Parse the frame header from our read buffer and validate it. 
        
           FrameHeader frameHeader = FrameHeader.ReadFrom(_incomingBuffer.ActiveSpan); 
        
           if (frameHeader.PayloadLength > FrameHeader.MaxPayloadLength) 
        
           { 
        
               if (initialFrame && NetEventSource.Log.IsEnabled()) 
        
               { 
        
                   string response = Encoding.ASCII.GetString(_incomingBuffer.ActiveSpan.Slice(0, Math.Min(20, _incomingBuffer.ActiveLength))); 
        
                   Trace($"HTTP/2 handshake failed. Server returned {response}"); 
        
               } 
        
               _incomingBuffer.Discard(FrameHeader.Size); 
        
               ThrowProtocolError(initialFrame ? Http2ProtocolErrorCode.ProtocolError : Http2ProtocolErrorCode.FrameSizeError); 
        
           }

If HTTP/2 and TLS, use ALPN and fail if server returns error.
- we leave the original exception as-is now, because we don't know exactly what to look for to be able to map it to a better one

RequestVersionExact -- exactly what the user asked for.

If HTTP/1 and no TLS, use HTTP/1.
If HTTP/1 and TLS, use HTTP/1.
If HTTP/2 and no TLS, use HTTP/2 cleartext (H2C)
If HTTP/2 and TLS, use ALPN and fail if server returns error.

Other issues

H3 not tested at all yet
~~Loopback servers cannot handle upgrade/downgrade at all. Not as easy as it seams to change. I will probably create a new issue for it.~~ Fixed.
No H3 remote server, no H2C remote server.

@scalablecory: ready for the first serious review.

Resolves #987

Dotnet-GitSync-Bot · 2020-07-13T16:06:28Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2020-07-13T16:06:30Z

Tagging subscribers to this area: @dotnet/ncl
Notify danmosemsft if you want to be subscribed.

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectHelper.cs

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs

ManickaP · 2020-07-21T18:23:25Z

src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs

@scalablecory could you please check whether the expected versions/exceptions correspond to required behavior?
Especially when H3 is involved. I'm really not sure if, for instance, we should downgrade to H2 or go directly for H1 etc.

src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs

ManickaP · 2020-07-22T07:22:16Z

/azp run runtime-libraries outerloop

azure-pipelines · 2020-07-22T07:22:30Z

Azure Pipelines successfully started running 1 pipeline(s).

scalablecory

Initial review -- looks good. Would like to give it a 2nd more in depth pass when I'm more awake.

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs

src/libraries/System.Net.Http/src/System/Net/Http/HttpRequestMessage.cs

src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs

azure-pipelines · 2020-08-05T12:42:27Z

Azure Pipelines successfully started running 1 pipeline(s).

alnikola

Besides a couple of comment below, LGTM.

src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectHelper.cs

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs

scalablecory

some minor comments, otherwise looks good.

scalablecory · 2020-08-06T11:05:21Z

src/libraries/Common/tests/System/Net/Http/HttpAgnosticLoopbackServer.cs

                }
-                if (sslStream.NegotiatedApplicationProtocol == SslApplicationProtocol.Http11)
+
+                var buffer = new byte[24];


Can we construct this server with a bool unencryptedProtocolDetection = false and throw if !unencryptedProtocolDetection && !_options.UseSsl?

I am worried that we will hide test bugs by accidentally using HTTP/1 when HTTP/2 was wanted, etc. -- this behavior should be off by default.

This is enforced in ClearTextVersion in HttpAgnosticOptions which defaults to HttpVersion.Version11.
So unless you construct the server with ClearTextVersion == HttpVersion.Unknown it won't do any unencrypted protocol detection.

Fixed, I removed the default of HTTP/1.1 and I'm explicitly checking for null ClearTextVersion and throwing if not set.

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ConnectHelper.cs

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs

src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs

stephentoub · 2020-08-10T10:30:26Z

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs

+                    return ValueTask.FromException<(HttpConnectionBase? connection, bool isNewConnection, HttpResponseMessage? failureResponse)>
+                        (new HttpRequestException(SR.Format(SR.net_http_requested_version_not_enabled, request.Version, request.VersionPolicy, 3)));
+                }
+                if (request.Version.Major >= 2 && !_http2Enabled)


What happens if requested version is HTTP/3, _http3Enabled is true, but _http2Enabled is false? This is going to throw; that's desirable?

I see what you're pointing at, I'll fix it.
However, with how we implemented HttpConnectionSettings._maxHttpVersion, it's not possible to have H3 enabled while H2 disabled.

No, you're right as usual 😄. It might happen, if ALPN 'disables' H2 while we get H3 authority via Alt-Svc... I'd fix it anyway though.

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs

stephentoub · 2020-08-10T10:38:02Z

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs

            }

+            // Do not allow upgrades for synchronous requests, that might lead to asynchronous code-paths.
+            request.VersionPolicy = HttpVersionPolicy.RequestVersionOrLower;


Doesn't this mean someone could specify RequestVersionOrHigher yet they may silently get a lower version? That seems wrong. Should we instead just throw for unsupported policies?

Fixed, now we throw when user requests upgrades.

src/libraries/System.Net.Http/src/System/Net/Http/HttpRequestMessage.cs

src/libraries/System.Net.Http/src/System/Net/Http/HttpVersionPolicy.cs

src/libraries/System.Net.Http/src/System/Net/Http/HttpRequestMessage.cs

src/libraries/System.Net.Http/src/System/Net/Http/HttpClient.cs

ManickaP · 2020-08-10T19:42:10Z

/azp run runtime-libraries outerloop

azure-pipelines · 2020-08-10T19:42:25Z

Azure Pipelines successfully started running 1 pipeline(s).

ManickaP · 2020-08-11T06:39:28Z

Failures: #33943 and #40606

Dotnet-GitSync-Bot added area-System.Net.Http new-api-needs-documentation labels Jul 13, 2020

scalablecory reviewed Jul 14, 2020

View reviewed changes

ManickaP force-pushed the mapichov/987_http_version_selection branch 4 times, most recently from 557f51b to eda0fde Compare July 18, 2020 10:20

scalablecory mentioned this pull request Jul 20, 2020

Initial (partially-reviewed API) System.Net.Connections. #39524

Merged

ManickaP force-pushed the mapichov/987_http_version_selection branch from eda0fde to dafe13f Compare July 21, 2020 18:15

ManickaP commented Jul 21, 2020

View reviewed changes

ManickaP marked this pull request as ready for review July 21, 2020 20:55

ManickaP changed the title ~~[WIP] HTTP Version Selection~~ HTTP Version Selection Jul 21, 2020

antonfirsov reviewed Jul 21, 2020

View reviewed changes

src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs Outdated Show resolved Hide resolved

scalablecory reviewed Jul 22, 2020

View reviewed changes

ManickaP added 12 commits July 22, 2020 11:51

VersionPolicy API added.

f31c191

WIP Version Policy.

4abfab1

Cleared up some ToDos.

f3fcff0

Some more version upgrade/downgrade logic.

83b8844

Enforce H2C in tests when ALPN is not available.

04a37a1

Exception messages into resources.

08f085b

Some fixes.

ce5f21e

Loopback tests.

53a08d1

RemoteServer tests and fixes.

959f85f

H3 assumed prenegotiated only when explicitly requested.

c0a731f

Netfx compilaris

e332222

Typo fixes

24df30f

alnikola approved these changes Aug 10, 2020

View reviewed changes

src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs Show resolved Hide resolved

src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientTest.cs Show resolved Hide resolved