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

Respect SETTINGS_MAX_HEADER_LIST_SIZE on HTTP/2 and HTTP/3 #79281

Merged
merged 1 commit into from
Dec 27, 2022

Conversation

MihaZupan
Copy link
Member

@MihaZupan MihaZupan commented Dec 6, 2022

Closes #78193

This PR does approximate header list length calculations for HTTP/2 and HTTP/3 in order to enforce the SETTINGS_MAX_HEADER_LIST_SIZE setting. It is a slight overestimation of the actual size (mainly from counting the overhead of HPACK framing), therefore making the client slightly more strict than the spec describes.
ASP.NET Core on the other hand is slightly less strict than the spec describes dotnet/aspnetcore#44668 (this is good, you don't want the client/server to be the other way around).

Do we care about the calculated length being exactly correct? Given this limit is generally really high and should only be hit in exceptional cases, I don't think we do. Doing precise bookkeeping would mainly represent a maintenance burden for us (and a slight performance overhead). The current approach of taking the encoded length from HPACK works well as long as we don't support the dynamic table/compression for request headers.

cc: @Tratcher

@MihaZupan MihaZupan added this to the 8.0.0 milestone Dec 6, 2022
@MihaZupan MihaZupan requested a review from a team December 6, 2022 16:31
@MihaZupan MihaZupan self-assigned this Dec 6, 2022
@ghost
Copy link

ghost commented Dec 6, 2022

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

Issue Details

Closes #78193

This PR does approximate header list length calculations for HTTP/2 and HTTP/3 in order to enforce the SETTINGS_MAX_HEADER_LIST_SIZE setting. It is a slight overestimation of the actual size (mainly from counting the overhead of HPACK framing), therefore making the client slightly more strict than the spec describes.
ASP.NET Core on the other hand is slightly less strict than the spec describes (dotnet/aspnetcore#44668).

Do we care about the calculated length being exactly correct? Given this limit is generally really high and should only be hit in exceptional cases, I don't think we do. Doing precise bookkeeping would mainly represent a maintenance burden for us (and a slight performance overhead). The current approach of taking the encoded length from HPACK works well as long as we don't support the dynamic table/compression for request headers.

cc: @Tratcher

Author: MihaZupan
Assignees: MihaZupan
Labels:

area-System.Net.Http

Milestone: 8.0.0

Copy link
Member

@CarnaViire CarnaViire left a comment

Choose a reason for hiding this comment

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

LGTM

@MihaZupan
Copy link
Member Author

@Tratcher can you please take a look at this one if this reduced change still makes sense to you?

@MihaZupan MihaZupan requested a review from Tratcher December 13, 2022 12:48
@MihaZupan MihaZupan force-pushed the http2-header-list-size-limit branch from 7efb4f5 to d8d975a Compare December 26, 2022 10:10
@build-analysis build-analysis bot mentioned this pull request Dec 26, 2022
@MihaZupan MihaZupan merged commit c84d95d into dotnet:main Dec 27, 2022
@MihaZupan
Copy link
Member Author

/backport to release/7.0

@github-actions
Copy link
Contributor

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/3787989385

@MihaZupan MihaZupan added the tenet-reliability Reliability/stability related issue (stress, load problems, etc.) label Jan 3, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Feb 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Http tenet-reliability Reliability/stability related issue (stress, load problems, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support SETTINGS_MAX_HEADER_LIST_SIZE on HTTP/2 and HTTP/3
3 participants