Align the HTTP/2 max header list size with the HTTP/1.1 max header size - #51246
Align the HTTP/2 max header list size with the HTTP/1.1 max header size#51246thejamesgore wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Aligns Quarkus HTTP/2 header-list size limits with the existing HTTP/1.1 max header size to prevent larger-header HTTP/2 requests from being rejected by default, and adds regression tests to keep the defaults aligned and overridable.
Changes:
- Set
quarkus.http.limits.max-header-list-size=65535inapplication.propertiesto matchmax-header-size. - Add tests asserting HTTP/1.1 and HTTP/2 header defaults remain equal.
- Add a test asserting the HTTP/2 header-list limit can be overridden via
QUARKUS_HTTP_LIMITS_MAX_HEADER_LIST_SIZE.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| quarkus/runtime/src/main/resources/application.properties | Sets the HTTP/2 header-list size limit to match the HTTP/1.1 header size limit. |
| quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/configuration/ConfigurationTest.java | Adds tests to lock the aligned defaults and validate env var override behavior. |
|
Thanks for the PR @thejamesgore Let's give the user on the issue a chance to weigh in on if this setting resolves their problem before merging this. |
This test can be removed. Generally we don't need to specifically test what we see as the unsupported path for configuring this value.
That would be great. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/configuration/ConfigurationTest.java:712
- The PR description mentions adding tests to assert the value remains overridable via
QUARKUS_HTTP_LIMITS_MAX_HEADER_LIST_SIZE, but this diff only adds a default-alignment assertion. If the override test isn’t implemented elsewhere, please add a test that setsQUARKUS_HTTP_LIMITS_MAX_HEADER_LIST_SIZEand assertsquarkus.http.limits.max-header-list-sizereflects the override (similar to the existing env override tests in this class).
@Test
public void testHttp2HeaderListSizeMatchesHttp1HeaderSizeDefault() {
ConfigArgsConfigSource.setCliArgs("");
SmallRyeConfig config = createConfig();
assertEquals(config.getConfigValue("quarkus.http.limits.max-header-size").getValue(),
config.getConfigValue("quarkus.http.limits.max-header-list-size").getValue());
assertEquals("65535", config.getConfigValue("quarkus.http.limits.max-header-list-size").getValue());
}
eea6602 to
6167d24
Compare
| @Test | ||
| @Launch({"start-dev"}) | ||
| public void largeHeadersTest() throws Exception { | ||
| String largeValue = "a".repeat(20 * 1024); |
There was a problem hiding this comment.
Is this sufficiently above the quarkus default of 20k - that is if our application.properties weren't present, would the http/1.1 request fail?
There was a problem hiding this comment.
Ahh yes! I bumped it to 32KB.
Keycloak configures quarkus.http.limits.max-header-size=65535 for HTTP/1.1 but never sets max-header-list-size so HTTP/2 requests were held to the much smaller Quarkus default, as noted in the original issue. Any request with headers between the two limits succeeded over HTTP/1.1 but was rejected at the HTTP/2 codec with no HTTP response and nothing logged. This aligns the HTTP/2 limit with HTTP/1.1 and adds an integration test covering large headers over both protocols. Closes keycloak#51182 Signed-off-by: James Gore <83005220+thejamesgore@users.noreply.github.com>
6235a81 to
3980192
Compare
Closes #51182
Keycloak configures
quarkus.http.limits.max-header-size=65535for HTTP/1.1 inapplication.propertiesbut never setsmax-header-list-sizeso HTTP/2 requests were held to the much smaller Quarkus default which was noted by @shawkins in the original issue.Any request with headers between the two limits succeeded over HTTP/1.1 but was rejected at the HTTP/2 codec so the server ends up sending
GOAWAY (PROTOCOL_ERROR)with no HTTP response and logs nothing which a fronting proxy surfaces as a bare 500.This aligns the HTTP/2 limit with the HTTP/1.1 limit and adds two tests. A unit test asserting the two defaults stay equal and a distribution level integration test
HttpDistTest#largeHeadersTestthat is sending a 20KB header over both HTTP/2 and HTTP/1.1 and asserting 200 on both. The HTTP/2 integration test case fails with the previous 8k default also.Verified against a locally built dist with same binary, having limit flipped via env var:
GOAWAY error=1, no response, nothing loggedOne thing of note is requests exceeding the now aligned limit still reset the HTTP/2 stream without a 431 unlike HTTP/1.1.
It seems that behaviour comes from the Vert.x layer upstream of this change.