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

Jump to bottom

x/net/websocket: override the Host header #33307

Open
fuglede opened this issue Jul 26, 2019 · 2 comments
Open

x/net/websocket: override the Host header #33307

fuglede opened this issue Jul 26, 2019 · 2 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unreleased

Comments

@fuglede
Copy link

fuglede commented Jul 26, 2019 •
edited

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

Relevant as of today's master of x/net/websocket (see below).

Does this issue reproduce with the latest release?

Yes.

What did you do?

Tried changing the Host header appearing in the initial WebSocket query by creating a config through websocket.NewConfig and changing its host through Header.Set("Host", "example.com").

What did you expect to see?

That the Host header would be set to example.com.

What did you see instead?

That the Host header was config.Location.Host.

In the current master, this is set on line 411 of hybi.go, in hybiClientHandshake.

Is this the intended API? It means that it becomes impossible to change the Host header without also changing the config.Location.Host, which might be undesirable (in my use case because it has the undesired side effect of changing the TLS SNI field, which is also based on config.Location.Host, and I can't have the SNI host and the WebSocket hosts be synchronized like that).

Given that there's a field called Header, I would have expected that to impact what headers end up being set; I would have been less surprised if hybiClientHandshake had used Header.Get("Host") if that header is set, and then fall back to config.Location.Host if it's not (similar to what you would see with e.g. curl).
@gopherbot gopherbot added this to the Unreleased milestone Jul 26, 2019
@FiloSottile FiloSottile changed the title x/net/websocket: Defining the Host header x/net/websocket: override the Host header Jul 30, 2019
@FiloSottile FiloSottile added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 30, 2019
@FiloSottile
Copy link
Contributor

Header is documented as "Additional header fields", and I think the protocol pretty much maps the location host to the host header. If you want a different TLS SNI, have you considered setting TlsConfig?

@fuglede
Copy link
Author

fuglede commented Jul 31, 2019

@FiloSottile Thanks for the comment.

Hm, any particular part of the protocol you're thinking of? I'm not at all familiar with the specification, and all I can see in the RFC is the following:

The client includes the hostname in the |Host| header field of its handshake as per [RFC2616], so that both the client and the server can verify that they agree on which host is in use.

This to me sounds like it ought to be possible to define the Host header independently of the location host; RFC2616 is HTTP and there you'd expect full freedom in setting Host.

Regarding the documentation, do you mean that the "Additional header fields" in the documentation of Config just refers to "header fields in addition to Host"? If so, that could probably be a bit clearer. And again, if that's the intent, it leaves client code with no good way of modifying that header (which, again, may or may not be allowed according to the protocol).

And yep, thanks for the reference to TlsConfig; I actually came across this particular hack while working on HTTP and in that case, modifying SNI indirectly through the location host, then modifying the HTTP Host accordingly, turned out to be the simplest approach, so I figured the same was probably the case for WebSocket (and honestly I never found a way to manipulate SNI hosts just with tls.Config; would be happy to hear if it's simpler than I think it is though).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
3 participants
@FiloSottile @fuglede @gopherbot