golang/go

proposal: x/net/http2: experiment with Extended CONNECT draft

riking opened this issue · 6 comments

riking commented

What version of Go are you using (go version)?

go1.10 linux/amd64

Summary

Implement the https://tools.ietf.org/html/draft-ietf-httpbis-h2-websockets-07 draft for Websockets over HTTP/2 to discover any implementation/API issues before the document is finalized.

Initial review shows several concerns:

  • A new SETTINGS frame value, SETTINGS_ENABLE_CONNECT_PROTOCOL needs to be sent when configured (or always).
    • How is it configured?
  • A CONNECT request with :protocol gets the normal treatment of the :authority pseudoheader (equivalent to Host: header).
  • Must process the new 'CONNECT with :protocol' requests in some way. There isn't an existing field in Request to handle this.
    • We could mandate that a Hijacker interface call is performed if support for the protocol is registered, but this is error-prone on the programmer side.
meirf commented

I don't know if this is something we'd include in x/net/http2 since it's an extension, but assuming we would include it:

Implement the https://tools.ietf.org/html/draft-ietf-httpbis-h2-websockets-07 draft for Websockets over HTTP/2 to discover any implementation/API issues before the document is finalized.

I'm not sure it's worth implementing before finalization since the spec might change. However, I do think it's worth to keep the draft in mind when implementing other changes now so we don't make it harder to implement once it's finalized. @riking do you see any in progress/open issues that might conflict?

riking commented

Looks like a possible conflict with #26479, because CONNECT already isn't supported.

riking commented

It might be better to experiment with this out-of-tree.

rsc commented

Ping @bradfitz

  • 👍1
rsc commented

It sounds like the proposal is "find out more about this", which doesn't need to be a proposal.
Agree with comment 2 up saying it would be better to experiment out-of-tree.

Historically we've added support for new features like this once multiple browsers are doing it. Please feel free to file a new proposal to add this functionality to the main tree once (1) you know how it should be done, and (2) browsers are supporting it.

mikedanese commented

The draft is now published as https://tools.ietf.org/html/rfc8441. It introduces:

  • A new SETTINGS_ENABLE_CONNECT_PROTOCOL setting set by a server to advertise support for extended connect. x/net/http2 appers to have no support for setting or reading initial settings on an http2 connection.
  • A new :protocol pseudo-header sent by clients to request a desired protocol. x/net/http2 errors on unknown pseudo headers.

Firefox, chrome and envoy now support this feature:

https://bugzilla.mozilla.org/show_bug.cgi?id=1434137
https://bugs.chromium.org/p/chromium/issues/detail?id=801564 (behind a flag)
envoyproxy/envoy#4767

For some motivation: Kubernetes recently had a security issue where we were incorrectly reusing proxied http/1.1 connections on websocket upgrade errors:

https://gravitational.com/blog/kubernetes-websocket-upgrade-security-vulnerability/

Clients of the apiserver proxy that could craft an upgrade request that would fail backend validation could confuse the apiserver into proxying arbitrary requests on the same connection authenticated as the apiserver. We feel that the new spec provides better protection against future bugs like this by encapsulating protocols rather than relinquishing the connection to them.

Agree with comment 2 up saying it would be better to experiment out-of-tree.

Is the recommendation that someone forks x/net/http2 to add these 2 new capabilities? Or am I overlooking some extension point that already exists?

Ref kubernetes/kubernetes#7452

  • 👍3