-
Notifications
You must be signed in to change notification settings - Fork 84
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
Validate that connection-specific headers aren't sent. #95
Conversation
Motivation: RFC 7540 forbids the use of connection-specific header fields, except for TE sent with the value "trailers". We should police that. Modifications: - Added extra policing around regular header fields. Result: Better validation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very nice, thanks
What's the disadvantage compared to simply ignoring these headers? Some HTTP1-to-HTTP2 proxies forget to suppress these headers during forwarding (or at least used to, see cloudendpoints/esp#560). |
Because the RFC is pretty clear that these are a MUST refuse. Users can elect to disable the validation in cases where it isn’t warranted. However, the HTTP/2 ecosystem has been immeasurably helped by having both clear MUST/MUST NOT directives in the specification, and implementations that police them. Tolerating these in an implementation may have local benefits for the implementation, but imposes negative externalities on the wider ecosystem, especially if they become widely deployed. NIO will always offer a tool to disable this validation to work around bad implementations. But by default we should police that implementations do the right thing, to aid future implementers. |
@Lukasa Thank you for the elaboration! That makes total sense, and policing "good behavior" across all HTTP2 participants by default is a great idea for the overall health of the ecosystem. I was not aware that there was an option (or plans to offer one) to disable such checks in NIOHTTP2 if need be. |
This is a difficult line to walk, but in general my view is that where it is possible to understand a misbehaving peer implementation, it should be possible to choose to do so. This is particularly important for networking frameworks, as if your networking framework prevents your application communicating then you tend to be completely out of luck! My last bit of validation code to add will be verifying that peers set content-length correctly, and that will also be able to be disabled by use of a flag. Users that are either performance-sensitive and willing to tolerate misbehaviour, or who know that they and their peers are spec conformant, will be encouraged to disable this checking. |
Agreed. This bit me with the old nghttp2-based implementation when Endpoints Service Proxy was still sending these headers; I ended up forking (the old version of) NIOHTTP2 just to disable nghttp2's checks. But I also agree that policing spec-conformance by default is a good approach to avoid having hundreds of somewhat-spec-conformant HTTP2 implementations, as is probably the case with HTTP1. |
Motivation:
RFC 7540 forbids the use of connection-specific header fields, except for TE sent
with the value "trailers". We should police that.
Modifications:
Result:
Better validation