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

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

Closed
riking opened this issue Aug 26, 2018 · 6 comments
Closed

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

riking opened this issue Aug 26, 2018 · 6 comments
Assignees
Labels
Milestone

Comments

@riking
Copy link

@riking riking commented Aug 26, 2018

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.
@gopherbot gopherbot added this to the Proposal milestone Aug 26, 2018
@gopherbot gopherbot added the Proposal label Aug 26, 2018
@meirf

This comment has been minimized.

Copy link
Contributor

@meirf meirf commented Aug 26, 2018

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

This comment has been minimized.

Copy link
Author

@riking riking commented Aug 26, 2018

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

@riking

This comment has been minimized.

Copy link
Author

@riking riking commented Aug 26, 2018

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

@bradfitz bradfitz self-assigned this Aug 27, 2018
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Sep 19, 2018

Ping @bradfitz

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Oct 3, 2018

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.

@rsc rsc closed this Oct 3, 2018
@mikedanese

This comment has been minimized.

Copy link
Contributor

@mikedanese mikedanese commented May 5, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.