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

Kestrel enforces non-default HTTP/2 limits before the Settings ack #17842

Open
Tratcher opened this issue Dec 12, 2019 · 3 comments
Open

Kestrel enforces non-default HTTP/2 limits before the Settings ack #17842

Tratcher opened this issue Dec 12, 2019 · 3 comments
Labels
affected-very-few This issue impacts very few customers area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-kestrel HTTP2 severity-nice-to-have This label is used by an internal tool
Milestone

Comments

@Tratcher
Copy link
Member

At the start of a h2 connection the client and server advertise their non-default settings to each other. However, the client can start sending requests before it receives the settings. The server is supposed to assume default settings until the client ack's the settings (with a timeout), but Kestrel does not, it assumes its own initial settings.

Kestrel has two settings it sets to lower than the default, SETTINGS_MAX_HEADER_LIST_SIZE and SETTINGS_MAX_CONCURRENT_STREAMS. The trouble with both of these settings is that the spec default is infinite, allowing infinite for either would be untenable, even for a short period of time.

The good thing about SETTINGS_MAX_CONCURRENT_STREAMS is that they're rejected with REFUSE_STREAM, meaning they're explicitly re-tryable, and HttpClient does retry them. The resets don't arrive until after the settings frame, so the retry will always happen with knowledge of the expected settings. The stream limit issue also contributes to #17484.

SETTINGS_MAX_HEADER_LIST_SIZE is less of a concern because the request is going to fail whether the client knows about the limit or not, there's no automated way to make the request smaller.

Having now written this up, I don't think kestrel has any alternatives in these scenarios. Temporary infinite limits are untenable and a client could run into the same issues even if we used more forgiving temporary non-infinite limits.

@Tratcher
Copy link
Member Author

Tratcher commented Dec 13, 2019

Followup thought: This would also be a problem for all of the other settings if someone changed kestrel's settings to be lower than the spec defaults. If we run into anyone wanting to do that we would need to use spec defaults until the client acked the settings. In that case the spec default should be fine, none of the others are infinite or even very large.

Note SETTINGS_MAX_FRAME_SIZE is not allowed to be reduced below its default value per spec.
Kestrel does not allow setting SETTINGS_INITIAL_WINDOW_SIZE below the spec default.
That only leaves SETTINGS_HEADER_TABLE_SIZE as a potential concern.

Recommend backlog for this specific scenario.

@analogrelay analogrelay added the enhancement This issue represents an ask for new feature or an enhancement to an existing one label Dec 18, 2019
@analogrelay analogrelay added this to the Backlog milestone Dec 18, 2019
@jkotalik jkotalik added the good first issue Good for newcomers. label Jan 24, 2020
@halter73 halter73 removed the good first issue Good for newcomers. label Mar 18, 2020
@halter73
Copy link
Member

Having now written this up, I don't think kestrel has any alternatives in these scenarios. Temporary infinite limits are untenable and a client could run into the same issues even if we used more forgiving temporary non-infinite limits.

@Tratcher Is there anything actionable in this issue? Can we close it?

@Tratcher
Copy link
Member Author

SETTINGS_HEADER_TABLE_SIZE is one of the few we'd be able to take action on. If a web app tried to use a lower setting than the default and the client tried to use dynamic compression before receiving the settings it could cause a protocol error. We would need to use the default until the setting was ack'd, with a timeout on the ack.

@jkotalik jkotalik added affected-very-few This issue impacts very few customers severity-nice-to-have This label is used by an internal tool labels Nov 13, 2020 — with ASP.NET Core Issue Ranking
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affected-very-few This issue impacts very few customers area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-kestrel HTTP2 severity-nice-to-have This label is used by an internal tool
Projects
None yet
Development

No branches or pull requests

5 participants