feat(http1): add support for receiving trailer fields #3637
Conversation
There was a problem hiding this comment.
Thanks for the PR! It looks great overall.
If the server does not include a trailers: ... header indicating which trailer headers are being returned, should trailers be parsed and passed along? I think this one is almost certainly "yes".
Yes. I think hyper should pass this along and let users decide what to do with the extra trailers.
If the server includes trailers with invalid header names, such as Content-Length: 5, should that still be parsed and passed along?
I'm not too sure about this one. I see two possible options:
- Pass it along but mark it as unexpected
- Err it
I'm interested to see what the others think about this
src/proto/h1/decode.rs
Outdated
| trace!("found possible trailers"); | ||
| match decode_trailers( | ||
| &mut trailers_buf.take().expect("Trailer is None"), | ||
| // decoder enforces that trailers count will not exceed usize::MAX |
There was a problem hiding this comment.
There was a problem hiding this comment.
i also added a check in 545f8c4
| match byte { | ||
| b'\n' => { | ||
| if *trailers_cnt == usize::MAX as u64 { | ||
| return Poll::Ready(Err(io::Error::new( |
There was a problem hiding this comment.
Can we write some tests covering this as well?
There was a problem hiding this comment.
added in 545f8c4
i also lowered the trailer limit to 1024 as i cannot imagine a use case for sending a million trailers.
There was a problem hiding this comment.
Yea, limits are needed, both on number of field pairs, and bytes itself, or else we expose servers to OOM attacks.
There was a problem hiding this comment.
I added a size limit in f194be1
The trailer limit is now 1024 instead of usize::MAX. There is also a test proving that the trailer limit is respected.
The size of the trailer fields is now limited. The limit accounts for a single, very large trailer field or many trailer fields that exceed the limit in aggregate.
src/proto/h1/decode.rs
Outdated
| /// Maximum number of trailer fields allowed in a chunked body. | ||
| const TRAILERS_FIELD_LIMIT: u64 = 1024; | ||
|
|
||
| /// Maximum number of bytes allowed for all trailer fields. | ||
| const TRAILER_LIMIT: u64 = TRAILERS_FIELD_LIMIT * 64; |
There was a problem hiding this comment.
I wonder if we want to expose an option to the user for this.
There was a problem hiding this comment.
sure. when i think about the use cases for this:
- the client sends
TE: trailersand wants to limit the amount of trailers to read in the server's response - the client sends a chunked body with trailer fields and the server wants to limit the amount of trailers to read in the client's request
Looking at prior art:
- We can include
h1_max_trailer_sizeandh1_max_trailersinclient::conn::http1::Builder - We can include
h1_max_trailer_sizeandh1_max_trailersinserver::conn::http1::Builder
There was a problem hiding this comment.
Let's add this in this PR too, I think it's simple enough to include this
There was a problem hiding this comment.
I've been thinking of adding a max_header_size() for the HTTP/1 builder, to mirror the HTTP/2 side. Does it make sense for that to be used by both headers and trailers? Or do people need to allow different sizes?
There was a problem hiding this comment.
Most uses of trailer fields, that I have observed, add 1-2 headers that are relatively short. I don't think people would want more capacity than what was for max_header_size(). People may want to set a lower capacity for trailers than what max_header_size() allows, but I am unsure how common this would be.
I think we should use max_header_size() to enforce both for now. If there is a real use case that people bring up, we can always add max_trailers() and max_trailers_size() in the future.
If we want to go this route, then I can update the trailer parsing logic to check against max_headers(). In a future PR, I can:
- Add
max_header_size()to the client and server builders. - Update the trailer parsing logic to check against
max_headers_size() - Update the header parsing logic
src/proto/h1/role.rsto enforcemax_headers_size()
There was a problem hiding this comment.
i added 6c01c87 to resolve this
Trailer parsing now honors h1_max_headers option. It also supports a future h1_max_header_size option.
There was a problem hiding this comment.
LGTM, thanks for the PR!
Anything you want to add? @seanmonstar
Closes #2703
In #3375 we were strict about sending trailers per RFC 7230. In this PR, I have been much more accepting of trailers. Is this the behavior we want?
Some questions:
TE: trailersbut the server sends trailer headers, should trailers be parsed and passed along?trailers: ...header indicating which trailer headers are being returned, should trailers be parsed and passed along? I think this one is almost certainly "yes".Content-Length: 5, should that still be parsed and passed along?