feat(ext): support non-canonical HTTP/1 reason phrases #2792
Conversation
So the tricky question is: is this worth a feature flag? Here are the performance numbers for the cargo bench suite (red is with the new feature disabled, green is with it enabled): https://gist.github.com/acfoltzer/c64e92ac95c390cb1c08624ecf61bfdf
It looks to me like roughly a wash, though there's a fair amount of noise in the sample. The problem is that none of the existing benches use the custom reason phrase, so it's hard to see the overall effect that existing users of "full" might get if they're already receiving responses with non-canonical phrases.
From an API cleanliness perspective I would prefer rolling this extension into the existing "http1" flag, but I also don't want to be introducing undue surprises for users of client who are not currently having to bear the cost of either the comparison against the canonical reason phrase or the allocation to store a custom one when sighted.
src/proto/h1/role.rs
Outdated
| #[cfg(feature = "http1_reason_phrase")] | ||
| let custom_reason_phrase = msg.head.extensions.get::<crate::ext::ReasonPhrase>(); | ||
| #[cfg(not(feature = "http1_reason_phrase"))] | ||
| #[allow(non_upper_case_globals)] | ||
| const custom_reason_phrase: Option<()> = None; |
There was a problem hiding this comment.
Here's where it starts getting a bit messy. To reduce the overall cfgness of this code, I'm relying on const evaluation to ensure this does not have an effect on performance when the flag is not enabled.
Add a new extension type `hyper::ext::ReasonPhrase` gated by a new feature flag `http1_reason_phrase`. When enabled, store any non-canonical reason phrases in this extension when parsing responses, and write this reason phrase instead of the canonical reason phrase when emitting responses. Reason phrases are a disused corner of the spec that implementations ought to treat as opaque blobs of bytes. Unfortunately, real-world traffic sometimes does depend on being able to inspect and manipulate them. My main outstanding question is: should this new feature flag be included in `full`? Excluding it means this patch also has to modify the CI configuration in order to run the `client` and `server` tests.
acfoltzer
force-pushed
the
acfoltzer/http1_reason_phrase
branch
from
a99c3a7
to
e7be2e1
Compare
Very good start, I appreciate the thoroughness of the docs and tests
Cargo.toml
Outdated
| @@ -86,6 +86,9 @@ full = [ | |||
| http1 = [] | |||
| http2 = ["h2"] | |||
|
|
|||
| # Support for non-canonical HTTP/1 reason phrases | |||
| http1_reason_phrase = ["http1"] | |||
There was a problem hiding this comment.
Yea, so let's talk about if we need a feature. My initial gut feeling is that it seems like such a small feature that it doesn't pull its weight, just include it. Especially if the benchmarks don't show a noticeable difference.
There was a problem hiding this comment.
I'd be in favor of folding it into http1. I think it'd take some very unusual traffic (like maxing out reason phrase length on every response) to cause a noticeable difference. It also helps that the cost is incurred on responses rather than on requests which are usually more concerning as a DoS vector.
I'll go ahead and fold it into http1 but will keep it in a separate commit while this is under review.
There was a problem hiding this comment.
Made this change in d621c74, which led to a pleasant reduction in the amount of awkwardly cfged code
src/ext/h1_reason_phrase.rs
Outdated
| } | ||
|
|
||
| /// Converts a static byte slice to a reason phrase. | ||
| pub const fn from_static(reason: &'static [u8]) -> Self { |
There was a problem hiding this comment.
Looking at this and the other constructors, I think we probably need to verify there's no illegal bytes in here that would mess up the HTTP/1 message.
There was a problem hiding this comment.
Ack, that's a really good catch. I'll convert some of the existing From impls to TryFrom, and expose an unsafe constructor for unchecked creation.
There was a problem hiding this comment.
Is the unchecked one needed yet? A from_static can likely just panic, similar to HeaderName::from_static. The verification can happen at compile time for most static strings.
There was a problem hiding this comment.
We need to check when constructing at runtime, because the server role will happily splat out whatever bytes are there into the status line. If someone put non-spec characters into the byte string this would lead to Problems.
The unchecked one is there for the client role to save the validation step; it already knows the characters are valid since it made it through parsing.
Most notably, this adds runtime checking for validity of reason phrases, which is super important to prevent invalid and dangerous characters from being emitted when writing responses. An `unsafe` escape hatch is present for hyper itself to create reason phrases that have been parsed (and therefore implicitly validated) by httparse.
Our discussion in the PR thread is leaning toward this option, so let's have it available as a separate commit so we can decide one way or the other.
Add a new extension type
hyper::ext::ReasonPhrasegated by a new feature flaghttp1_reason_phrase. When enabled, store any non-canonical reason phrases in this extension when parsing responses, and write this reason phrase instead of the canonical reason phrase when emitting responses.Reason phrases are a disused corner of the spec that implementations ought to treat as opaque blobs of bytes. Unfortunately, real-world traffic sometimes does depend on being able to inspect and manipulate them.
My main outstanding question is: should this new feature flag be included in
full? Excluding it means this patch also has to modify the CI configuration in order to run theclientandservertests.