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

HpackEncoder limits header list size to 8K by default #7825

Closed
ejona86 opened this issue Mar 30, 2018 · 2 comments
Closed

HpackEncoder limits header list size to 8K by default #7825

ejona86 opened this issue Mar 30, 2018 · 2 comments
Assignees
Labels
Milestone

Comments

@ejona86
Copy link
Member

ejona86 commented Mar 30, 2018

Expected behavior

Sending a header list of arbitrary size would succeed, except when the receiver prohibits header list larger than a certain size.

RFC7540 defines SETTINGS_MAX_HEADER_LIST_SIZE's initial value to be unlimited.

Actual behavior

If the receiver does not specify a maximum header list size in SETTINGS, Netty disallows header list sizes greater than 8K.

The problem seems to be in HpackEncoder. While defaulting to 8K makes sense for HpackDecoder in order for it to protect itself against the remote, there's no need for such behavior in HpackEncoder. In fact, since SETTINGS_MAX_HEADER_LIST_SIZE is advisory and the content is locally generated, it could ignore the setting completely and only risk wasting bandwidth.

This was reported against gRPC at grpc/grpc-java#4284. CC @JLofgren

Netty version

4.1.17.Final

I'd be happy to send a PR changing the initialization to use MAX_HEADER_LIST_SIZE. @Scottmitch, @normanmaurer, concerns?

@ejona86
Copy link
Member Author

ejona86 commented Mar 30, 2018

Looks like I can call defaultHttp2HeadersEncoder.maxHeaderListSize(MAX_HEADER_LIST_SIZE) to workaround this. If done immediately on creation, it looks like it won't collide with any received SETTINGS requesting a lower value.

@JLofgren
Copy link
Contributor

JLofgren commented Apr 6, 2018

I've created a demo of this bug here: https://github.com/JLofgren/demo-grpc-java-bug-4284

Locally I have built netty initializing the max header size in HpackEncoder to MAX_HEADER_LIST_SIZE and shown that this solves the issue. I am working on a PR, but am having some issues dotting all the i's of your PR process.

JLofgren added a commit to JLofgren/netty that referenced this issue Apr 6, 2018
Motivation:

When connecting to an HTTP/2 server that did not set any value for the
SETTINGS_MAX_HEADER_LIST_SIZE in the settings frame, the netty client was
imposing an arbitrary maximum header list size of 8kB. There should be no need
for the client to enforce such a limit if the server has not specified any
limit. This caused an issue for a grpc-java client that needed to send a large
header to a server via an Envoy proxy server. The error condition is
demonstrated here: https://github.com/JLofgren/demo-grpc-java-bug-4284

Fixes grpc-java issue netty#4284 - grpc/grpc-java#4284
and netty issue netty#7825 - netty#7825

Modifications:

In HpackEncoder use MAX_HEADER_LIST_SIZE as default maxHeader list size.

Result:

HpackEncoder will only enforce a max header list size if the server has
specified a limit in its settings frame.
Scottmitch pushed a commit that referenced this issue Apr 16, 2018
Motivation:

When connecting to an HTTP/2 server that did not set any value for the
SETTINGS_MAX_HEADER_LIST_SIZE in the settings frame, the netty client was
imposing an arbitrary maximum header list size of 8kB. There should be no need
for the client to enforce such a limit if the server has not specified any
limit. This caused an issue for a grpc-java client that needed to send a large
header to a server via an Envoy proxy server. The error condition is
demonstrated here: https://github.com/JLofgren/demo-grpc-java-bug-4284

Fixes grpc-java issue #4284 - grpc/grpc-java#4284
and netty issue #7825 - #7825

Modifications:

In HpackEncoder use MAX_HEADER_LIST_SIZE as default maxHeader list size.

Result:

HpackEncoder will only enforce a max header list size if the server has
specified a limit in its settings frame.
@normanmaurer normanmaurer self-assigned this Apr 17, 2018
@normanmaurer normanmaurer added this to the 4.1.24.Final milestone Apr 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants