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.

Sign up for GitHub

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

Jump to bottom

Hyper does not re-join cookie headers split by http/2 #2528

Open
NeoLegends opened this issue Apr 30, 2021 · 4 comments · May be fixed by #2529
Open

Hyper does not re-join cookie headers split by http/2 #2528

NeoLegends opened this issue Apr 30, 2021 · 4 comments · May be fixed by #2529
Labels
A-headers Area: headers. B-rfc Blocked: More comments would be useful in determine next steps.

Comments

@NeoLegends
Copy link

NeoLegends commented Apr 30, 2021 •
edited
Loading

As per https://tools.ietf.org/html/rfc7540#section-8.1.2.5 http/2 allows splitting cookie headers into multiple parts if it improves compression efficiency. The spec mandates that the header needs to be rejoined if the associated request is proxied to a http/1 server, though.

To allow for better compression efficiency, the Cookie header field
MAY be split into separate header fields, each with one or more
cookie-pairs. If there are multiple Cookie header fields after
decompression, these MUST be concatenated into a single octet string
using the two-octet delimiter of 0x3B, 0x20 (the ASCII string "; ")
before being passed into a non-HTTP/2 context, such as an HTTP/1.1
connection, or a generic HTTP server application.

Currently, the Hyper server exposes the split cookie header as separate entries in the HeaderMap, but if the request is then proxied to a h1 server, the Client does not re-join them again (although it probably should).

I do not think it's desirable to always and forcibly re-join cookie headers before sending them out on http/1 connections, given that hyper is a low-level library and users might want to experiment with that. It might also be a breaking change for users of this library.

Therefore, to limit the blast radius of the change, I propose to only re-join headers iff the incoming request was h2 and the outgoing one is h1. Hyper neatly exposes the HTTP version on the request, which should let one do that. I'd be happy to send a PR implementing that.

Eventually, I think, hyper's server should re-join the cookie header transparently, before invoking the actual server-Service. This would then conform with the last sentence of the spec paragraph above.
@nox
Copy link
Contributor

nox commented Apr 30, 2021 •
edited
Loading

I guess it could do it there:

hyper/src/proto/h1/role.rs

Lines 1033 to 1036 in 4e9a006

Version::HTTP_2 => {
debug!("request with HTTP2 version coerced to HTTP/1.1");
extend(dst, b"HTTP/1.1");
}

Edit: fixed the link, which was previously pointing at the server encode function.

@NeoLegends NeoLegends linked a pull request Apr 30, 2021 that will close this issue
Open
@seanmonstar
Copy link
Member

Just curious, is this something that hyper itself should do? Or a responsibility of the proxy using hyper?

@NeoLegends
Copy link
Author

NeoLegends commented Apr 30, 2021 •
edited
Loading

This is a really interesting point. I've had some discussion about this with @nox on Discord, and IMO this mainly depends on whether you would consider the proxy service "generic" (as per the spec).

Since there are http-2 only proxies or services, I am not sure whether hyper should blindly merge Cookie values on http/2 connections before handing the request off to the service.

@nox and me came to the conclusion that it would make sense to implement the minimal approach first, where the hyper client merges cookie headers if it's coercing a h2 request into a h1 request, such that the "bad" requests at least don't leave the server in their broken (from an h1 perspective) state. I filed #2529 to implement this. Would appreciate your review!

In our production service we added a layer that merged the cookies, so working around this w/o hyper support is definitely not hard -- just surprising.

@nox
Copy link
Contributor

nox commented Apr 30, 2021

Just curious, is this something that hyper itself should do? Or a responsibility of the proxy using hyper?

I feel like the only reason something would ever pass a h2 request to a h1 origin is that it is a proxy, and then the less expensive way to merge those headers is in the header encode loop deep in the role.rs file. But I don't have a strong opinion on this.

@davidpdrsn davidpdrsn added A-headers Area: headers. C-performance Category: performance. This is making existing behavior go faster. labels May 7, 2021
@seanmonstar seanmonstar added the B-rfc Blocked: More comments would be useful in determine next steps. label Jan 14, 2022
@seanmonstar seanmonstar removed the C-performance Category: performance. This is making existing behavior go faster. label Dec 8, 2022
@nox nox mentioned this issue Sep 13, 2023
Open
@dekkers dekkers mentioned this issue May 8, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-headers Area: headers. B-rfc Blocked: More comments would be useful in determine next steps.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat: Join headers from http/2 requests on http/1 connections NeoLegends/hyper
4 participants
@seanmonstar @nox @davidpdrsn @NeoLegends