dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.61k stars 10.06k forks source link

Rebalance Kestrel's Known headers collection #31492

Open Tratcher opened 3 years ago

Tratcher commented 3 years ago

Related to (#31374) @halter73 @benaadams @davidfowl

Kestrel has a fancy KnownHeaders collection for efficient handling of common request and response headers. https://github.com/dotnet/aspnetcore/blob/bc1ff6a45949c3debc483aae93abd0dfdf97bec1/src/Servers/Kestrel/shared/KnownHeaders.cs#L16-L31

These lists have grown organically and should be revisited. We don't want to add too many entries since it makes the types consume more memory, but we do want to ensure most common headers are represented so we can avoid the slow path.

I've compared the current lists with headers customers have reported seeing in production. Here are some of the discrepancies:

Seen in the wild: Kestrel: Notes
Allow Defined in commonHeaders, but only applies to responses? https://tools.ietf.org/html/rfc7231#section-7.4.1
Authorization
Baggage
authority
bypass
cacheresponse
Client-IP
clientip
Content-Encoding Defined in commonHeaders, but mainly used on responses
Content-Language Defined in commonHeaders, but mainly used on responses
Content-Location Defined in commonHeaders, but mainly used on responses
Content-MD5 Defined in commonHeaders, but mainly used on responses
Content-Range Defined in commonHeaders, but only applies to responses? https://tools.ietf.org/html/rfc7233#section-4.2
ContentType
E2EActivity
el_auth_param
Expires
Forwarded https://tools.ietf.org/html/rfc7239
forwarded-for
gb-branch
gb-no-cache
guzzle-retry
From Uncommon
Grpc-Accept-Encoding
Grpc-Encoding
Grpc-Timeout
http_accept_language
If-Match
If-Modified-Since
If-None-Match
If-Range
If-Unmodified-Since
Keep-Alive
Last-Modified Defined in commonHeaders, but mainly used on responses
LatencyPerfCounterName
Lcid
newrelic
OData-MaxVersion
OData-Version
okversion
postman-token
Proxy-Authorization Kestrel rarely acts as a forward proxy
Prefer https://tools.ietf.org/html/rfc7240
Proxy-Connection
proxy-tool
Request-Context
Save-Data
sec-ch-ua https://wicg.github.io/ua-client-hints/
Sec-Ch-Ua-Mobile
Sec-Fetch-Dest https://w3c.github.io/webappsec-fetch-metadata/#sec-fetch-mode-header
Sec-Fetch-Mode
Sec-Fetch-Site
sec-fetch-user
sec-gpc https://globalprivacycontrol.github.io/gpc-spec/
Sec-GPC
sentry-trace
Soapaction
ssodisabled
Surrogate-Capability
transaction-id
Trailer Defined in commonHeaders, but mainly used on responses
Translate
Upgrade
unique-id
Warning
X_CHAN
X_GLS
X_grg
X_sn
X_ts
X-ARR-LOG-ID
X-ARR-SSL
x-country-code
x-dt-no-cache
x-finder-tools
X-Forwarded-For
x-im-piez
X-IMForwards
X-IWS-Via
x-lgi-host
x-no-varnish
X-Original-URL
x-originating-ip
x-p2p-peerdist
x-p2p-peerdistex
X-ProxyUser-IP
x-remote-addr
x-remote-ip
X-Requested-With
x-serverselect
X-Trace
Xxpect
blowdart commented 3 years ago

Three different SOAP actions? Thank goodness we all moved to REST 😈

Tratcher commented 3 years ago

Hehe, the initial data was case sensitive. I've removed the duplicates.

benaadams commented 3 years ago

DNT is in the Kestrel headers but a bit pointless now as its obsolete and think only IE sends it?

benaadams commented 3 years ago

Another area is could HTTP/2 and HTTP/1.x headers be split into different objects (don't know if the gprc headers are HTTP/2 only? /cc @JamesNK ) but :method, :path, :scheme and :status are HTTP/2 headers only and some HTTP/1.x headers are disallowed on HTTP/2 so don't need to have reserved spots in its collection.

davidfowl commented 3 years ago

Another area is could HTTP/2 and HTTP/1.x headers be split into different objects (don't know if the gprc headers are HTTP/2 only? /cc @JamesNK ) but :method, :path, :scheme and :status are HTTP/2 headers only and some HTTP/1.x headers are disallowed on HTTP/2 so don't need to have reserved spots in its collection.

This idea I like.

blowdart commented 3 years ago

DNT isn't necessary, nothing cares about it any more.

Tratcher commented 3 years ago

https://github.com/dotnet/aspnetcore/issues/30417 already covers removing the HTTP/2 pseudo headers from the collection. I think we'd want to intercept them in OnHeader before they are even added to the collection and divert them to their own fields.

benaadams commented 3 years ago

DNT isn't necessary, nothing cares about it any more.

PR to remove DNT while deciding on others https://github.com/dotnet/aspnetcore/pull/31493

JamesNK commented 3 years ago

don't know if the gprc headers are HTTP/2 only? /cc @JamesNK

grpc headers could be used in HTTP/1.1 with gRPC-Web, but optimizing gRPC-Web isn't a priority.

benaadams commented 3 years ago

Moved some of the common headers to either Request or Response https://github.com/dotnet/aspnetcore/pull/31495

ghost commented 3 years ago

Thanks for contacting us. We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. Because it's not immediately obvious that this is a bug in our framework, we would like to keep this around to collect more feedback, which can later help us determine the impact of it. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

ghost commented 2 years ago

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.