Implement CONNECT support over HTTP/2 #2523

nox · 2021-04-28T09:22:09Z

There are quite a few details to sort out, grep for FIXME(nox).

tests/server.rs

src/proto/h2/mod.rs

src/proto/h2/server.rs

tests/server.rs

src/proto/h2/mod.rs

tests/server.rs

inikulin · 2021-04-30T14:26:53Z

@seanmonstar may I ask you to take a look as well?

seanmonstar

This is fantastic, excellent work! Love the detailed tests too!

src/proto/h2/mod.rs

src/proto/h2/server.rs

src/server/conn.rs

nox

Made an exhaustive review of the additional bounds, they are unfortunately indeed breaking changes.

src/proto/h2/server.rs

src/server/conn.rs

src/server/server.rs

src/server/shutdown.rs

nox · 2021-05-07T08:24:25Z

I have an idea to remove the bound on B::Data.

nox

I have implemented my cursed idea, tell me what you think about it.

src/proto/h2/server.rs

src/proto/h2/mod.rs

nox · 2021-05-19T13:38:19Z

I added preliminary client-side support.

src/proto/h2/client.rs

LPardue · 2021-05-19T14:30:19Z

src/proto/h2/server.rs

+                        } else {
+                            if content_length.map_or(false, |len| len != 0) {
+                                warn!("h2 connect request with non-zero body not supported");
+                                respond.send_reset(h2::Reason::INTERNAL_ERROR);


Since, at this stage it is the client breaking the rules, maybe this should be a PROTOCOL_ERROR,

Does it truly break the rules? I don't remember seeing prose explicitly rejecting this.

So the most current HTTP specs (close to being RFCs) say https://httpwg.org/http-core/draft-ietf-httpbis-semantics-latest.html#rfc.section.9.3.6.p.12

A server MUST NOT send any Transfer-Encoding or Content-Length header fields in a 2xx (Successful) response to CONNECT. A client MUST ignore any Content-Length or Transfer-Encoding header fields received in a successful response to CONNECT.

A CONNECT request message does not have content. The interpretation of and allowability of data sent after the header section of the CONNECT request message is specific to the version of HTTP in use.

And the HTTP/2 spec doesn't seem to clearly state things either way. No point splitting hairs over an error code so lets leave that. In the meantime I opened an issue on the HTTP/2 spec in the hope that RFC 7540 CONNECT definition could get tightened up. See httpwg/http2-spec#849

LPardue · 2021-05-19T14:30:36Z

src/proto/h2/server.rs

+                            }
+                            let (pending, upgrade) = crate::upgrade::pending();
+                            debug_assert!(parts.extensions.get::<OnUpgrade>().is_none());
+                            parts.extensions.insert(upgrade);


I might be missing it, does the server enforce that the authority form is used?

h2 itself will fail if passed a non-authority URI for a CONNECT request, on the Hyper side I just did like what already exists for HTTP/1.

SGTM, thanks for explaining.

nox

Replied to Lucas' comments, I'll make sure we don't reject bodies on non-200 responses.

src/proto/h2/client.rs

nox · 2021-05-20T07:12:39Z

src/proto/h2/server.rs

+                            }
+                            let (pending, upgrade) = crate::upgrade::pending();
+                            debug_assert!(parts.extensions.get::<OnUpgrade>().is_none());
+                            parts.extensions.insert(upgrade);


h2 itself will fail if passed a non-authority URI for a CONNECT request, on the Hyper side I just did like what already exists for HTTP/1.

nox · 2021-05-20T07:12:40Z

src/proto/h2/server.rs

+                        } else {
+                            if content_length.map_or(false, |len| len != 0) {
+                                warn!("h2 connect request with non-zero body not supported");
+                                respond.send_reset(h2::Reason::INTERNAL_ERROR);


Does it truly break the rules? I don't remember seeing prose explicitly rejecting this.

seanmonstar

Stupendous!

…ium#2523) Closes hyperium#2508

@nox

Back in hyperium#2523, @nox introduced the notion of an UpgradedSendStream, to support the CONNECT method of HTTP/2. This used `unsafe {}` to support `http_body::Body`, where `Body::Data` did not implement `Send`, since the `Data` type wouldn't be sent across the stream once upgraded. Unfortunately, according to this [thread], I think this may be undefined behavior, because this relies on us requiring the transmute to execute. This patch fixes the potential UB by adding the unncessary `Send` constraints. It appears that all the internal users of `UpgradeSendStream` already work with `http_body::Body` types that have `Send`-able `Data` constraints. We can add this constraint without breaking any external APIs, which lets us remove the `unsafe {}` blocks. [thread]: https://users.rust-lang.org/t/is-a-reference-to-impossible-value-considered-ub/31383

@nox

Back in hyperium#2523, @nox introduced the notion of an UpgradedSendStream, to support the CONNECT method of HTTP/2. This used `unsafe {}` to support `http_body::Body`, where `Body::Data` did not implement `Send`, since the `Data` type wouldn't be sent across the stream once upgraded. Unfortunately, according to this [thread], I think this may be undefined behavior, because this relies on us requiring the transmute to execute. This patch fixes the potential UB by adding the unncessary `Send` constraints. It appears that all the internal users of `UpgradeSendStream` already work with `http_body::Body` types that have `Send`-able `Data` constraints. We can add this constraint without breaking any external APIs, which lets us remove the `unsafe {}` blocks. [thread]: https://users.rust-lang.org/t/is-a-reference-to-impossible-value-considered-ub/31383

nox force-pushed the h2-upgrades branch 5 times, most recently from 6a6d080 to d6ba136 Compare April 28, 2021 10:03

inikulin reviewed Apr 28, 2021

View reviewed changes

tests/server.rs Outdated Show resolved Hide resolved

nox force-pushed the h2-upgrades branch from d6ba136 to 3d27679 Compare April 28, 2021 10:23

cbranch reviewed Apr 28, 2021

View reviewed changes

src/proto/h2/mod.rs Outdated Show resolved Hide resolved

src/proto/h2/server.rs Outdated Show resolved Hide resolved

nox force-pushed the h2-upgrades branch 6 times, most recently from 14876e5 to 817e111 Compare April 28, 2021 14:54

inikulin suggested changes Apr 29, 2021

View reviewed changes

tests/server.rs Show resolved Hide resolved

tests/server.rs Show resolved Hide resolved

nox force-pushed the h2-upgrades branch 3 times, most recently from 3c7a507 to c04e447 Compare April 29, 2021 08:56

nox commented Apr 29, 2021

View reviewed changes

src/proto/h2/mod.rs Outdated Show resolved Hide resolved

nox force-pushed the h2-upgrades branch 5 times, most recently from b3c9b60 to 82dd5bb Compare April 29, 2021 16:37

inikulin reviewed Apr 29, 2021

View reviewed changes

tests/server.rs Outdated Show resolved Hide resolved

nox force-pushed the h2-upgrades branch 2 times, most recently from a68864d to e015d47 Compare April 30, 2021 08:42

seanmonstar reviewed Apr 30, 2021

View reviewed changes

src/proto/h2/mod.rs Show resolved Hide resolved

src/proto/h2/mod.rs Outdated Show resolved Hide resolved

src/proto/h2/mod.rs Outdated Show resolved Hide resolved

src/proto/h2/server.rs Outdated Show resolved Hide resolved

src/server/conn.rs Outdated Show resolved Hide resolved

nox commented May 6, 2021

View reviewed changes

nox force-pushed the h2-upgrades branch 2 times, most recently from 170a72f to a2385ae Compare May 6, 2021 11:43

nox force-pushed the h2-upgrades branch from a2385ae to 3611d29 Compare May 7, 2021 08:51

nox commented May 7, 2021

View reviewed changes

src/proto/h2/server.rs Show resolved Hide resolved

src/proto/h2/server.rs Show resolved Hide resolved

src/proto/h2/mod.rs Show resolved Hide resolved

chore(h2): bound B::Data by 'static in H2Stream impls

90c14ee

nox force-pushed the h2-upgrades branch 3 times, most recently from 845c9c9 to 3d5caf1 Compare May 19, 2021 13:36

LPardue reviewed May 19, 2021

View reviewed changes

nox commented May 20, 2021

View reviewed changes

nox force-pushed the h2-upgrades branch from 3d5caf1 to dcd4796 Compare May 20, 2021 07:42

feat(h2): implement CONNECT support (fixes hyperium#2508)

0089566

nox force-pushed the h2-upgrades branch from dcd4796 to 0089566 Compare May 20, 2021 07:44

seanmonstar approved these changes May 24, 2021

View reviewed changes

Merge branch 'master' into h2-upgrades

40e8dfa

seanmonstar enabled auto-merge (squash) May 24, 2021 18:11

seanmonstar merged commit 5442b6f into hyperium:master May 24, 2021

BenxiangGe pushed a commit to BenxiangGe/hyper that referenced this pull request Jul 26, 2021

feat(http2): Implement Client-side CONNECT support over HTTP/2 (hyper…

7a46ee4

…ium#2523) Closes hyperium#2508

erickt mentioned this pull request May 10, 2022

Document how hyper is using UpgradedSendStream safely #2829

Open

erickt mentioned this pull request May 10, 2022

Remove unsafe from proto::h2 #2831

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement CONNECT support over HTTP/2 #2523

Implement CONNECT support over HTTP/2 #2523

nox commented Apr 28, 2021

inikulin commented Apr 30, 2021

seanmonstar left a comment

nox left a comment

nox commented May 7, 2021

nox left a comment

nox commented May 19, 2021

LPardue May 19, 2021

nox May 20, 2021

LPardue May 20, 2021

LPardue May 19, 2021

nox May 20, 2021

LPardue May 20, 2021

nox left a comment

nox May 20, 2021

nox May 20, 2021

seanmonstar left a comment

Implement CONNECT support over HTTP/2 #2523

Implement CONNECT support over HTTP/2 #2523

Conversation

nox commented Apr 28, 2021

inikulin commented Apr 30, 2021

seanmonstar left a comment

Choose a reason for hiding this comment

nox left a comment

Choose a reason for hiding this comment

nox commented May 7, 2021

nox left a comment

Choose a reason for hiding this comment

nox commented May 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanmonstar left a comment

Choose a reason for hiding this comment