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

Validate that connection-specific headers aren't sent. #95

Merged
merged 1 commit into from
Apr 12, 2019

Conversation

Lukasa
Copy link
Contributor

@Lukasa Lukasa commented Apr 12, 2019

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

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
@Lukasa Lukasa added the 🔨 semver/patch No public API change. label Apr 12, 2019
@Lukasa Lukasa added this to the 1.0.2 milestone Apr 12, 2019
@Lukasa Lukasa requested a review from weissi April 12, 2019 14:46
Copy link
Member

@weissi weissi left a comment

Choose a reason for hiding this comment

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

very nice, thanks

@Lukasa Lukasa merged commit ae57c2a into apple:master Apr 12, 2019
@Lukasa Lukasa deleted the cb-no-connection-headers branch April 12, 2019 15:01
@Lukasa Lukasa added 🆕 semver/minor Adds new public API. and removed 🔨 semver/patch No public API change. labels Apr 12, 2019
@MrMage
Copy link
Contributor

MrMage commented Apr 12, 2019

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).

@Lukasa
Copy link
Contributor Author

Lukasa commented Apr 12, 2019

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.

@MrMage
Copy link
Contributor

MrMage commented Apr 15, 2019

@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.

@Lukasa
Copy link
Contributor Author

Lukasa commented Apr 15, 2019

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.

@MrMage
Copy link
Contributor

MrMage commented Apr 15, 2019

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!

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🆕 semver/minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants