search

hyperium / h2

HTTP 2.0 client & server implementation for Rust.
MIT License
1.35k stars 270 forks source link

Implement the extended CONNECT protocol from RFC 8441 #565

Closed nox closed 2 years ago

nox commented 2 years ago

@seanmonstar I wrote "Reject :protocol in responses" but I can't find explicit prose about this, and I'm not sure it's worth doing at all. If we do it, should the server also preemptively reject responses returned by the user, with an INTERNAL_ERROR on the wire and a user error?

nox commented 2 years ago

For "Reject :protocol in responses", we would need to make server::Peer::convert_send_message fallible, there is currently no precedent for making the server reject the response provided by the user's code.

nox commented 2 years ago

There is also no precedent for rejecting on the client side any malformed response, such as responses with :authority etc, so I'm just not going to reject :protocol there either.

seanmonstar commented 2 years ago

Ah, we don't already do that? I guess practically that's fine... though it does say "Clients MUST NOT accept a malformed response." 🤷

nox commented 2 years ago

I have a problem, which is that there is no way through neither Connection nor SendRequest to tell h2 "ok, wait for the handshake initial settings to be acknowledged, so it's really hard to check whether the server told us "yeah, you can make an extended connect request".

seanmonstar commented 2 years ago

wait for the handshake initial settings to be acknowledged

We should (perhaps in separate PR) add a builder option that when enabled, sends the SETTINGS immediately without waiting for a request, and potentially poll_ready doesn't return ready until the SETTINGS have been acked.

nox commented 2 years ago

Reject SETTINGS_ENABLE_CONNECT_PROTOCOL from client.

Spec says “Receipt of this parameter by a server does not have any impact.” so shrug.

nox commented 2 years ago

There is now a client test that checks that we properly send :protocol.

nox commented 2 years ago

I renamed the extension type to just Protocol.

nox commented 2 years ago

I added a test that a client rejects the setting being changed from 1 to 0, but h2 still acknowledges the new settings just before rejecting them.

nox commented 2 years ago

I could make Protocol own a HeaderValue instead of a BytesStr, but I don't really see the point of doing that given all other pseudo-headers are represented with a BytesStr, and making this change would also require a set_pseudo! refactor.

nox commented 2 years ago

I've decided to not implement new user error cases on the client side given there are none for normal connect requests either anyway.

seanmonstar commented 2 years ago

I'll leave merging up to you if there's anything you want to finish.