Skip to content

Have kestrel filter out pseudo headers #36166

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

Merged
merged 1 commit into from
Sep 8, 2021

Conversation

Tratcher
Copy link
Member

@Tratcher Tratcher commented Sep 3, 2021

Fixes #30417
6.0 rc2 candidate: interoperability

HTTP/2 & 3 use pseudo headers for special values like method, path, authority, scheme, etc., but they have lots of constraints about where they can be placed, only using reserved ones, etc.. These fields are also available elsewhere in the request.

https://tools.ietf.org/html/rfc7540#section-8.1.2.1

Pseudo-header fields are not HTTP header fields.

As a result, other http header APIs won't accept them as input. E.g. HttpRequestMessage, WebHeaderCollection, etc.. This means components that copy request headers from Kestrel have to filter these out. E.g. YARP, WCF, gateways, etc..

https://github.com/microsoft/reverse-proxy/blob/a3247ba1168325f46ab0ee4a24d3de1d9acdd853/src/ReverseProxy/Service/Proxy/HttpTransformer.cs#L49-L53

// Filter out HTTP/2 pseudo headers like ":method" and ":path", those go into other fields.
if (headerName.Length > 0 && headerName[0] == ':')
{
    continue;
}

Http.Sys and IIS also filter out pseudo headers.

Side note: I noticed that the new header properties don't support removal. I've updated them.

@Tratcher Tratcher added this to the 7.0-preview1 milestone Sep 3, 2021
@Tratcher Tratcher requested review from halter73 and JamesNK September 3, 2021 23:47
@Tratcher Tratcher self-assigned this Sep 3, 2021
@@ -843,7 +843,14 @@ internal partial class {loop.ClassName} : IHeaderDictionary
}}
set
{{
{header.SetBit()};
if (!StringValues.IsNullOrEmpty(value))
Copy link
Member

Choose a reason for hiding this comment

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

This is an existing bug in .NET 6 being fixed right?

Copy link
Member

Choose a reason for hiding this comment

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

Should this particular change go into 6.0 RC2?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, and yes I plan to backport this entire PR to rc2.

Copy link
Member

@wtgodbe wtgodbe left a comment

Choose a reason for hiding this comment

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

Seems reasonable, are there any interop tests we need to perform/add to make sure this plays nice with YARP or any internal components?

@Tratcher
Copy link
Member Author

Tratcher commented Sep 8, 2021

YARP has 6.0 integration tests that will cover this.

@Tratcher
Copy link
Member Author

Tratcher commented Sep 8, 2021

/backport to release/6.0

@github-actions
Copy link
Contributor

github-actions bot commented Sep 8, 2021

Started backporting to release/6.0: https://github.com/dotnet/aspnetcore/actions/runs/1214277678

@Tratcher Tratcher merged commit 800dff4 into dotnet:main Sep 8, 2021
@Tratcher Tratcher deleted the tratcher/kestrel/pseudo branch September 8, 2021 17:22
@@ -301,6 +303,7 @@ private bool TryValidateMethod()
{
// :method
_methodText = HttpRequestHeaders.HeaderMethod.ToString();
HttpRequestHeaders.HeaderMethod = default; // Suppress pseduo headers from the public headers collection.
Copy link
Member Author

@Tratcher Tratcher Apr 24, 2023

Choose a reason for hiding this comment

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

We identified a bug with this approach in 6.0. On line 221 above, TryValidateMethod is skipped if the method was sent via static hpack index. That means we fail to remove it from the collection.

#38581 fixed this in inadvertently in 7.0 by bulk clearing the set bits in the collection itself rather than clearing the individual properties.

@amcasey 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Have kestrel filter out pseudo headers
5 participants