Remove connection-specific headeres for HTTP/2 and HTTP/3 #24543

Kahbazi · 2020-08-03T19:58:53Z

Fixes #24502

Pilchie · 2020-08-13T19:49:59Z

I think this conflicts with the changes @JamesNK just made.

Tratcher

Also, please rebase.

src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs

src/Servers/Kestrel/shared/KnownHeaders.cs

src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs

src/Servers/Kestrel/shared/KnownHeaders.cs

BrennanConroy · 2020-10-07T20:26:25Z

@Kahbazi Are you still interested in proceeding with this PR?

Kahbazi · 2020-10-08T12:10:04Z

@BrennanConroy Sorry for the delay. I rebased and updated the PR.

src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs

JamesNK · 2020-10-13T23:31:31Z

Spec - https://quicwg.org/base-drafts/draft-ietf-quic-http.html#name-field-formatting-and-compre

This means that an intermediary transforming an HTTP/1.x message to HTTP/3 will need to remove any fields nominated by the Connection field, along with the Connection field itself. Such intermediaries SHOULD also remove other connection-specific fields, such as Keep-Alive, Proxy-Connection, Transfer-Encoding, and Upgrade, even if they are not nominated by the Connection field.

Kestrel isn't an intermediary and it doesn't transform request protocols. It could be used as part of an intermediary, like YARP, but in that case it is YARPs job to remove these headers when transforming a HTTP/1.1 call to 2 or 3.

I don't see why this code should be part of Kestrel.

Tratcher · 2020-10-13T23:46:09Z

I don't see why this code should be part of Kestrel.

A) It's kestrel's job to enforce protocol correctness, it shouldn't allow outgoing or incoming protocol violations. We have similar checks for incoming requests:

aspnetcore/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs

Lines 1314 to 1317 in be472db

    
           if (IsConnectionSpecificHeaderField(name, value)) 
        
           { 
        
               throw new Http2ConnectionErrorException(CoreStrings.Http2ErrorConnectionSpecificHeaderField, Http2ErrorCode.PROTOCOL_ERROR); 
        
           }

B) For consistency with IIS & Http.Sys which I expect removes these silently from responses.

davidfowl · 2020-10-13T23:58:20Z

B) For consistency with IIS & Http.Sys which I expect removes these silently from responses.

I don't think this is a good enough reason to use all of the time. Kestrel is the only server where we can control this level of behavior and that's an advantage. That said, I think we saw other servers beyond our own NOT have this behavior (go, node) so it's worth considering.

Tratcher · 2020-11-20T01:18:36Z

Rebased. I spoke with @Kahbazi and volunteered to finish this PR for them.

I confirmed the behavior in Http.Sys where these headers are all silently removed. This approach allows the server to maintain both protocol compliance and app compatibility. These headers may be added by a library that is not in the developers control.

For Kestrel I've added a generic log statement that "A response header has been removed because it was invalid for the current protocol." . This is logged at the debug level because the issue is something the app can ignore unless they want to learn more about the protocol details. I've limited it to one log per response to limit the complexity and noise.

Addressed

JamesNK · 2020-11-20T02:06:42Z

Currently the PR's behavior is checking for certain set headers when using HTTP/2+, and then clearing them.

From a performance perspective, wouldn't it be better to make setting these headers noop in HTTP/2? That avoids checking whether these headers are set in every HTTP/2+ request.

Tratcher · 2020-11-20T19:28:08Z

The overhead should be pretty minimal, a few bitwise checks, but you've given me an idea on how to reduce that further. I'll try it and report back.

Tratcher · 2020-11-20T23:00:49Z

From a performance perspective, wouldn't it be better to make setting these headers noop in HTTP/2? That avoids checking whether these headers are set in every HTTP/2+ request.

I've updated this to consolidate the operations for these headers and reduce overhead.

The headers collection is quite performant already, the relevant code is only setting two fields. You wouldn't be able to avoid the normal dictionary-like operations for Headers.Add to find those fields.

I'd rather keep the logic here in CreateResponseHeaders. The headers collections don't currently know about things like request protocols, the protocol specific logic for response headers is all consolidated in CreateResponseHeaders right now. I think that makes it easier to maintain.

@halter73 any thoughts on this?

src/Servers/Kestrel/Core/src/Internal/Infrastructure/KestrelTrace.cs

halter73 · 2020-11-21T00:10:06Z

I do like keeping the main logic in CreateResponseHeaders. I think the current implementations of HasInvalidH2H3Headers and ClearInvalidH2H3Headers() are plenty efficient. You could even argue it creates less branching logic on the set-headers path compared to proactively avoiding setting them. We could microbenchmark, but I'm not too concerned about optimizing the case where invalid headers are being set as long as it isn't terrible.

src/Servers/Kestrel/Core/src/Internal/Infrastructure/KestrelTrace.cs

Co-authored-by: Stephen Halter <[email protected]>

src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2StreamTests.cs

…treamTests.cs

src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3StreamTests.cs

…treamTests.cs

halter73 · 2020-11-25T19:04:18Z

src/Http/Headers/src/HeaderNames.cs

@@ -65,6 +65,7 @@ public static class HeaderNames
        public static readonly string Pragma = "Pragma";
        public static readonly string ProxyAuthenticate = "Proxy-Authenticate";
        public static readonly string ProxyAuthorization = "Proxy-Authorization";
+        public static readonly string ProxyConnection = "Proxy-Connection";


Ping @dotnet/aspnet-api-review. We should probably send an email as a heads up.

ghost · 2020-11-25T19:07:28Z

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

halter73 · 2020-11-25T19:07:45Z

src/Servers/Kestrel/Core/src/Internal/Infrastructure/KestrelTrace.cs

@@ -117,6 +117,10 @@ internal class KestrelTrace : IKestrelTrace
            LoggerMessage.Define<string>(LogLevel.Debug, new EventId(40, nameof(Http2MaxConcurrentStreamsReached)),
                @"Connection id ""{ConnectionId}"" reached the maximum number of concurrent HTTP/2 streams allowed.");

+        private static readonly Action<ILogger, Exception> _invalidResponseHeaderRemoved =
+            LoggerMessage.Define(LogLevel.Information, new EventId(41, nameof(InvalidResponseHeaderRemoved)),


I still would prefer this is a warning at least for the early previews. If we get feedback that this is too noisy or not helpful, we could lower the severity. I'll approve as-is since I don't feel super strongly about it though.

Suggested change

LoggerMessage.Define(LogLevel.Information, new EventId(41, nameof(InvalidResponseHeaderRemoved)),

LoggerMessage.Define(LogLevel.Warning, new EventId(41, nameof(InvalidResponseHeaderRemoved)),

Fair, we'll see what they say.

BrennanConroy · 2020-11-25T19:45:23Z

src/Servers/Kestrel/Core/src/Internal/Infrastructure/KestrelTrace.cs

@@ -117,6 +117,10 @@ internal class KestrelTrace : IKestrelTrace
            LoggerMessage.Define<string>(LogLevel.Debug, new EventId(40, nameof(Http2MaxConcurrentStreamsReached)),
                @"Connection id ""{ConnectionId}"" reached the maximum number of concurrent HTTP/2 streams allowed.");

+        private static readonly Action<ILogger, Exception> _invalidResponseHeaderRemoved =
+            LoggerMessage.Define(LogLevel.Information, new EventId(41, nameof(InvalidResponseHeaderRemoved)),


nit: in the rest of the stack we try to avoid using nameof for the EventId because changing the name is technically a breaking change and it's easier to accidentally change it when renaming a method.

Yeah, not sure why all of the Kestrel ones use nameof. Changing them is out of scope here. I'll file something.
#28159

…ace.cs

ghost · 2020-11-25T19:55:50Z

Hello @Tratcher!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

ghost added the area-servers label Aug 3, 2020

Kahbazi marked this pull request as ready for review August 3, 2020 21:06

Kahbazi requested review from halter73, jkotalik and Tratcher as code owners August 3, 2020 21:06

Tratcher previously requested changes Aug 17, 2020

View reviewed changes

Pilchie assigned Tratcher Aug 24, 2020

Pilchie added the community-contribution Indicates that the PR has been added by a community member label Aug 24, 2020

Kahbazi force-pushed the kahbazi/HTTP2Headers branch from af5bff1 to d6752d6 Compare October 8, 2020 10:22

halter73 reviewed Oct 8, 2020

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs Show resolved Hide resolved

Kahbazi and others added 2 commits November 19, 2020 16:19

Remove connection-specific headeres for HTTP/2 and HTTP/3

c2a38e8

Log

cad4c5f

Tratcher force-pushed the kahbazi/HTTP2Headers branch from d6752d6 to cad4c5f Compare November 20, 2020 01:10

Consolidate check

a106f0d

halter73 reviewed Nov 21, 2020

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/Infrastructure/KestrelTrace.cs Outdated Show resolved Hide resolved

Tratcher reviewed Nov 24, 2020

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/Infrastructure/KestrelTrace.cs Outdated Show resolved Hide resolved

Apply suggestions from code review

a11da61

Co-authored-by: Stephen Halter <[email protected]>

Tratcher reviewed Nov 24, 2020

View reviewed changes

src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2StreamTests.cs Outdated Show resolved Hide resolved

Update src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2S…

ccfe73a

…treamTests.cs

Tratcher reviewed Nov 24, 2020

View reviewed changes

src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3StreamTests.cs Outdated Show resolved Hide resolved

Update src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3S…

fa1f6a9

…treamTests.cs

Tratcher requested a review from halter73 November 24, 2020 23:18

Tratcher added this to the 6.0-preview1 milestone Nov 24, 2020

halter73 reviewed Nov 25, 2020

View reviewed changes

davidfowl added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Nov 25, 2020

halter73 reviewed Nov 25, 2020

View reviewed changes

halter73 approved these changes Nov 25, 2020

View reviewed changes

BrennanConroy reviewed Nov 25, 2020

View reviewed changes

Update src/Servers/Kestrel/Core/src/Internal/Infrastructure/KestrelTr…

97d38f8

…ace.cs

Tratcher added the auto-merge label Nov 25, 2020

ghost merged commit 77fe36f into dotnet:master Nov 25, 2020

Tratcher mentioned this pull request Nov 29, 2020

HTTP/2 connection to YARP with HTTP/1.1 to backend fails dotnet/yarp#583

Closed

MihaZupan mentioned this pull request Jan 12, 2021

Remove connection-specific headeres for HTTP/2 and HTTP/3 dotnet/yarp#659

Merged

pranavkm removed the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Mar 22, 2021

amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023

This pull request was closed.

	LoggerMessage.Define(LogLevel.Information, new EventId(41, nameof(InvalidResponseHeaderRemoved)),
	LoggerMessage.Define(LogLevel.Warning, new EventId(41, nameof(InvalidResponseHeaderRemoved)),

Remove connection-specific headeres for HTTP/2 and HTTP/3 #24543

Remove connection-specific headeres for HTTP/2 and HTTP/3 #24543

Uh oh!

Conversation

Kahbazi commented Aug 3, 2020

Uh oh!

Pilchie commented Aug 13, 2020

Uh oh!

Tratcher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BrennanConroy commented Oct 7, 2020

Uh oh!

Kahbazi commented Oct 8, 2020

Uh oh!

Uh oh!

JamesNK commented Oct 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tratcher commented Oct 13, 2020

Uh oh!

davidfowl commented Oct 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Tratcher commented Nov 20, 2020

Uh oh!

JamesNK commented Nov 20, 2020

Uh oh!

Tratcher commented Nov 20, 2020

Uh oh!

Tratcher commented Nov 20, 2020

Uh oh!

Uh oh!

halter73 commented Nov 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

halter73 Nov 25, 2020

Choose a reason for hiding this comment

Uh oh!

ghost commented Nov 25, 2020

Uh oh!

halter73 Nov 25, 2020

Choose a reason for hiding this comment

Uh oh!

Tratcher Nov 25, 2020

Choose a reason for hiding this comment

Uh oh!

Tratcher Nov 25, 2020

Choose a reason for hiding this comment

Uh oh!

BrennanConroy Nov 25, 2020

Choose a reason for hiding this comment

Uh oh!

Tratcher Nov 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Nov 25, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

Uh oh!

JamesNK commented Oct 13, 2020 •

edited

Loading

davidfowl commented Oct 13, 2020 •

edited

Loading

halter73 commented Nov 21, 2020 •

edited

Loading

Tratcher Nov 25, 2020 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.