search

golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.15k stars 17.56k forks source link

net/http: add support for SETTINGS_ENABLE_CONNECT_PROTOCOL #53208

Open ethanpailes opened 2 years ago

ethanpailes commented 2 years ago

Overview

I propose extending the x/net/http2 and net/http packages to support the extended CONNECT protocol described in RFC 8441. Support for this will be always-on in the http server, and the http client will automatically detect the presence of support based on the settings flag and adjust the checks it applies to outgoing requests to allow the for the extended CONNECT protocol. http.Request will be extended with a new Protocol member to allow users to set the :protocol psudoheader.

Public Interface Changes

http.Request will gain a new member, Protocol of type string.

All other functionality can be achieved with existing interfaces.

Implementation

An example implementation of SETTINGS_ENABLE_CONNECT_PROTOCOL support for both the client and the server can be found on gerrit. This is just meant to be a proof of concept, not a fully production ready implementation. Rather than using a field on http.Request it uses a magic header called HACK-HTTP2-Protocol, but the intention is that this would be switched out for req.Protocol for real code.

The implementation is pretty non-invasive. It consists of extending the code to allow the use of a new :protocol psudo header, sending the new settings flag during the initial handshake, and loosening some checks when the extended CONNECT protocol is in use.

Probably the most complex thing about the implementation I was able to come up with is the fact that a CONNECT request coming from the client can now trigger a ping HTTP/2 frame if the server has not yet sent headers. This is required for an extended CONNECT to work if is the very first request on a given http2.Transport object.

Related Issues

#49918 contains some discussion about some of my initial implementation work and links to further context.

ianlancetaylor commented 2 years ago

CC @neild @bradfitz

neild commented 2 years ago

How do we handle a request sent on an HTTP/1 connection with a Protocol field set? Return an error? Set Upgrade: $PROTOCOL and Connection: upgrade?

An alternative to adding a new field might be to set a :protocol key in the Request.Header map.

ethanpailes commented 2 years ago

How do we handle a request sent on an HTTP/1 connection with a Protocol field set? Return an error? Set Upgrade: $PROTOCOL and Connection: upgrade?

My inclination is to return an error since my understanding of go's philosophy is that it is a one-way-to-do-things language and you can already set the Upgrade and Connection headers in the Request.Header map. I can definitely sympathize with the alternative view that it is less surprising for Protocol = "websocket" to work regardless of the underlying transport though so I'd be happy to do it either way.

An alternative to adding a new field might be to set a :protocol key in the Request.Header map.

I like this approach because it feels a little weird for Protocol to be a top level field on such a common type when it will be so rarely used. I think I actually tried this when implementing support but ran into an error. I decided to propose a new field because psudoheaders are not really supposed to be set by the user, but I'm not so sure how good a justification this is. On reflection, maybe allowing people to set ":protocol" in the headers map by relaxing some checks that forbid user defined psudoheaders is the best way to go since it doesn't shove the feature in users face quite so much. If you're happy with that approach, I would be as well (really I don't have particularly strong feelings in any direction so I'm happy to do whatever, but this design seems good).

neild commented 2 years ago

I don't have a strong feeling on a Protocol field vs. setting a :protocol header. Perhaps @bradfitz has an opinion.

An argument for a Protocol field might be if allows websockets implementations to be agnostic of HTTP/1 or HTTP/2 with net/http handling translation between the Protocol field and the underlying request (pseudo-)headers. I don't know if that's feasible.

We do have existing cases where you can set either a field in the Request or a header (e.g., Content-Length), so doing that for Upgrade and Connection wouldn't necessarily be horrible.

ethanpailes commented 2 years ago

An argument for a Protocol field might be if allows websockets implementations to be agnostic of HTTP/1 or HTTP/2 with net/http handling translation between the Protocol field and the underlying request (pseudo-)headers. I don't know if that's feasible.

The websocket handshake for HTTP/1.1 and HTTP/2 are sufficiently different that attempting this is probably not a good idea. There is a bunch of Sec-* header stuff in HTTP/1.1 that isn't needed in HTTP/2 because psudoheaders are not user-settable from within a browser. The HTTP/2 handshake doesn't need to muck about with nonces for example. Since they are different enough any websocket library will need to explicitly support both protocols unless we move most of the handshake logic into the http and http2 packages themselves, which doesn't seem ideal.

We do have existing cases where you can set either a field in the Request or a header (e.g., Content-Length), so doing that for Upgrade and Connection wouldn't necessarily be horrible.

Gotcha, that is good context and weakens my one-way-to-do-things argument.

I think I'm still a little inclined towards allowing a :protocol header, but not that strongly.

bradfitz commented 1 year ago

I'd rather put :protocol in Request.Header (akin to http.TrailerPrefix) over adding a new Request.Protocol string field.

neild commented 1 year ago

Proposal SGTM with a :protocol pseudo-header in Request.Header.

ethanpailes commented 1 year ago

Should I clean up the gerrit patch to use a :protocol pseudo-header in Request.Header and request another review, or is there another step required for the proposal to be accepted?

ghost commented 1 year ago

We need this!!

mdwn commented 1 year ago

Just wanted to ping on this. This is something that I'm super interested in.

becheran commented 1 year ago

Also wanted to ping this one. Very relevant for us.

gopherbot commented 1 year ago

Change https://go.dev/cl/508238 mentions this issue: http2: implement support for extended CONNECT

taoso commented 8 months ago

Hi @bradfitz @neild @ethanpailes

What about store the :protocol into the req.Proto field? The current value is HTTP{1.x,2.0,3.0}. However, it does not store any new information, because the version of HTTP protocol has been stored in the req.ProtoMajor and req.ProtoMinor.

The quick-go project currently using the req.Proto to store the :protocol pseudo header.

https://github.com/quic-go/quic-go/blob/a968e254a17fcd48743e54ea536fabd4f30c64d1/http3/headers.go#L131-L159

masx200 commented 7 months ago

any update?

ethanpailes commented 6 months ago

As far as I know I'm still waiting on review by the core go maintainers.

neild commented 6 months ago

This is waiting on proposal acceptance. I'll ping the proposal committee to add this to the set under consideration.

The proposal, as I understand it:

The HTTP/2 CONNECT method is used to convert a connection into a tunnel to a remote host. It takes an :authority header indicting which host to connect to.

Extended CONNECT is an HTTP/2 extension (RFC 8441). When enabled, the the CONNECT can alternatively take a :protocol psuedo-header, indicating an intent to convert the connection to some other protocol. When :protocol is present, :authority is optional. Extended CONNECT is used to initiate HTTP/2 WebSocket connections.

This proposal adds support for extended CONNECT to net/http.

For servers, the proposal is to advertise extended CONNECT support. When a client makes an extended CONNECT request, the value of the :protocol pseudo-header will be placed in the Request.Header map under the :protocol key.

For clients, RoundTrip will support setting a :protocol key in the Request.Header map. When making an HTTP/2 request with this key, the Transport will verify that the server supports extended CONNECT (waiting for the server's SETTINGS frame if necessary) before sending the request. If the server does not support extended CONNECT, it will return an error.

Is that all correct?

If so, I support this proposal, with one caveat that I think can be worked out while this works through proposal committee.

A reason extended CONNECT needs to be negotiated is that it changes the meaning of the :authority pseudo-header in CONNECT requests. A request which contains both :protocol and :authority headers is not necessarily requesting a tunnel to be opened to the :authority address, and a server which naively ignores the :protocol header might mishandle such a request, treating it as a non-extended CONNECT.

This applies to existing http handlers which process CONNECT requests: A handler which is unaware of the :protocol header will mishandle extended CONNECT requests that contain one.

Does extended CONNECT for http.Servers need to be opt-in?

rsc commented 6 months ago

This proposal has been added to the active column of the proposals project and will now be reviewed at the weekly proposal review meetings. — rsc for the proposal review group

rsc commented 6 months ago

Does extended CONNECT for http.Servers need to be opt-in?

I don't think it has to be opt-in but it probably needs a GODEBUG to provide a fallback when hitting endpoints or middleware that is buggy when extended CONNECTs are involved. http2xconnect=0 or something like that.

gatalon commented 6 months ago

Want to know if it can support the latest RFC9220[Bootstrapping WebSockets with HTTP/3] at the same time?

ice-cronus commented 6 months ago

http3 is gonna be supported with http3 module, as I've seen it has support of extended connect (via .Proto) and webtransport is already implemented over it.

But if you're going to add extended connect to http2, it might have sense to add AdditionalSettings (similar to http3's) and expose Stream interface from http2, because also webtransport over http2 is coming and it uses the same extended connect protocol but also requires stream access and extra settings (SETTINGS_WEBTRANSPORT_MAX_SESSIONS p.2 above)

ethanpailes commented 6 months ago

Is that all correct?

Sorry, it's been a minute since I did this work so I had to swap it back in, yeah that summarizes my understanding of the situation pretty well. A GODEBUG flag to disable to feature seems like a good idea to me.

The patches I wrote are still in a proof-of-concept state right now. If this proposal is formally approved should I pick them back up and try to polish them, or will you want to do that final phase yourselves?

rsc commented 6 months ago

Have all remaining concerns about this proposal been addressed?

Proposal details are in https://github.com/golang/go/issues/53208#issuecomment-1981461316, plus we would need to add a GODEBUG.

rsc commented 6 months ago

Based on the discussion above, this proposal seems like a likely accept. — rsc for the proposal review group

Proposal details are in https://github.com/golang/go/issues/53208#issuecomment-1981461316, plus we would need to add a GODEBUG.

rsc commented 5 months ago

No change in consensus, so accepted. 🎉 This issue now tracks the work of implementing the proposal. — rsc for the proposal review group

Proposal details are in https://github.com/golang/go/issues/53208#issuecomment-1981461316, plus we would need to add a GODEBUG.

neild commented 4 months ago

The patches I wrote are still in a proof-of-concept state right now. If this proposal is formally approved should I pick them back up and try to polish them, or will you want to do that final phase yourselves?

Sorry, I missed this question when you made it.

I don't anticipate having the time to work on this myself in the near future, but I'd be happy to review your changes if you'd like to pick them up again.

WeidiDeng commented 2 weeks ago

I made http2 servers advertise ENABLE_CONNECT_PROTOCOL in this patch. I'll finish client side support in a later patch.