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

Implement CONNECT support over HTTP/2 #2523

Merged
merged 3 commits into from
May 24, 2021
Merged

Conversation

nox
Copy link
Contributor

@nox nox commented Apr 28, 2021

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

@nox nox force-pushed the h2-upgrades branch 5 times, most recently from 6a6d080 to d6ba136 Compare April 28, 2021 10:03
tests/server.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
@nox nox force-pushed the h2-upgrades branch 6 times, most recently from 14876e5 to 817e111 Compare April 28, 2021 14:54
tests/server.rs Show resolved Hide resolved
tests/server.rs Show resolved Hide resolved
@nox nox force-pushed the h2-upgrades branch 3 times, most recently from 3c7a507 to c04e447 Compare April 29, 2021 08:56
src/proto/h2/mod.rs Outdated Show resolved Hide resolved
@nox nox force-pushed the h2-upgrades branch 5 times, most recently from b3c9b60 to 82dd5bb Compare April 29, 2021 16:37
tests/server.rs Outdated Show resolved Hide resolved
@nox nox force-pushed the h2-upgrades branch 2 times, most recently from a68864d to e015d47 Compare April 30, 2021 08:42
@inikulin
Copy link
Contributor

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

Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

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

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
Copy link
Contributor Author

@nox nox left a comment

Choose a reason for hiding this comment

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

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

src/proto/h2/server.rs Outdated Show resolved Hide resolved
src/proto/h2/server.rs Outdated Show resolved Hide resolved
src/proto/h2/server.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
src/server/server.rs Outdated Show resolved Hide resolved
src/server/server.rs Outdated Show resolved Hide resolved
src/server/shutdown.rs Outdated Show resolved Hide resolved
src/server/shutdown.rs Outdated Show resolved Hide resolved
src/server/shutdown.rs Outdated Show resolved Hide resolved
@nox nox force-pushed the h2-upgrades branch 2 times, most recently from 170a72f to a2385ae Compare May 6, 2021 11:43
@nox
Copy link
Contributor Author

nox commented May 7, 2021

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

Copy link
Contributor Author

@nox nox left a comment

Choose a reason for hiding this comment

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

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

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
@nox nox force-pushed the h2-upgrades branch 3 times, most recently from 845c9c9 to 3d5caf1 Compare May 19, 2021 13:36
@nox
Copy link
Contributor Author

nox commented May 19, 2021

I added preliminary client-side support.

src/proto/h2/client.rs Show resolved Hide resolved
} 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);
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

SGTM, thanks for explaining.

Copy link
Contributor Author

@nox nox left a comment

Choose a reason for hiding this comment

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

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

src/proto/h2/client.rs Show resolved Hide resolved
}
let (pending, upgrade) = crate::upgrade::pending();
debug_assert!(parts.extensions.get::<OnUpgrade>().is_none());
parts.extensions.insert(upgrade);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

} 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);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

Stupendous!

@seanmonstar seanmonstar enabled auto-merge (squash) May 24, 2021 18:11
@seanmonstar 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
erickt added a commit to erickt/hyper that referenced this pull request May 10, 2022
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
erickt added a commit to erickt/hyper that referenced this pull request May 10, 2022
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants