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

net/http: document how to create client CONNECT requests #22554

Closed
Audrey-Randall opened this issue Nov 3, 2017 · 11 comments
Closed

net/http: document how to create client CONNECT requests #22554

Audrey-Randall opened this issue Nov 3, 2017 · 11 comments
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@Audrey-Randall
Copy link

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

Go1.9

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

windows/amd64

What did you do?

This error is reproducible at https://play.golang.org/p/9m7rlnqsTm

I attempted to create a new HTTP request with http.NewRequest() using a URI that starts with a number, with no protocol, that also contains a port:
_, err = http.NewRequest(http.MethodConnect, "0.docs.google.com:443", nil)

What did you expect to see?

The request should be created with no error.

What did you see instead?

I get the error "parse : first path segment in URL cannot contain colon"

@bradfitz
Copy link
Contributor

bradfitz commented Nov 3, 2017

This is actually working as intended. "host:port" is not a valid URL, and http.NewRequest takes a URL, not a host:port.

For a CONNECT request (which is a weirdo type of request), you'll probably just want to construct the *http.Request by hand. It's specially recognized by some but not all of net/http helpers. In particular, http.NewRequest does not go out of its way to support CONNECT weirdness.

@bradfitz bradfitz closed this as completed Nov 3, 2017
@bradfitz
Copy link
Contributor

bradfitz commented Nov 3, 2017

Actually, I suppose we could also make NewRequest special-case CONNECT and also support host:port there.

@tombergan, thoughts?

@bradfitz bradfitz reopened this Nov 3, 2017
@bradfitz bradfitz changed the title net: Incorrect handling of URIs that begin with a number and contain a port net/http: let NewRequest accept a host:port "URL" for CONNECT requests? Nov 3, 2017
@bradfitz bradfitz added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Nov 3, 2017
@bradfitz bradfitz added this to the Go1.11 milestone Nov 3, 2017
@sergeyfrolov
Copy link
Contributor

Thanks for reopening this!

I would like to note that it's not necessarily about CONNECT. If user wants to specify port for other types of requests, say, http.MethodGet, then behavior is the same:

  • "0google.com:443" fails
  • "google.com:443" doesn't fail

@bradfitz
Copy link
Contributor

bradfitz commented Nov 3, 2017

@sergeyfrolov, for GET requests or anything other than CONNECT, we will not change behavior. "foo.com:443" is not a valid URL. You need a scheme (http://, https://, etc).

@tombergan
Copy link
Contributor

I would like to note that it's not necessarily about CONNECT. If user wants to specify port for other types of requests, say, http.MethodGet, then behavior is the same:

"0google.com:443" fails
"google.com:443" doesn't fail

That's because "google.com:443" doesn't parse how you think it does ... it parses "google.com" into the scheme, not the host. If "google.com:443" is working for you, that's accidental rather than intentional.
https://play.golang.org/p/omOEwB02Me

Actually, I suppose we could also make NewRequest special-case CONNECT and also support host:port there.

I don't think this would work, for two reasons. First, what scheme does the following example use when connecting to foo.com?

// Assume that NewRequest special-cases CONNECT to support host:port.
// req.URL = {Scheme: ??? , Host = "google.com:443"}
req, _ := http.NewRequest("CONNECT", "google.com:443", nil)

// Connect over TCP to www.proxy.com, which will forward the TCP connection
// to google.com:443. Then, do we TLS to google.com:443 or use raw TCP?
// The :443 suggests that the caller wants TLS, but in general we cannot know
// since req.URL does not have a scheme.
tr := http.Transport{Proxy: http.ProxyURL("http://www.proxy.com/")}
tr.RoundTrip(req)

Second, the following would be confusing:

// Assume that NewRequest special-cases CONNECT to support host:port.
// Note that r1.Host != r2.Host
// r1.URL = {Scheme: ??? , Host = "google.com:443"}
// r2.URL = {Scheme: "google.com", Opaque: "443", Host = ""}
r1, _ := http.NewRequest("CONNECT", "google.com:443", nil)
r2, _ := http.NewRequest("GET", "google.com:443", nil)

@sergeyfrolov's example is a good one. It is confusing that "0google.com:443" fails but "google.com:443" does not. More precisely:

// This is what happens currently.
// r1.URL = {Scheme: "google.com", Opaque: "443", Host = ""}
// r1.URL = nil
r1, _ := http.NewRequest("CONNECT", "google.com:443", nil)
r2, _ := http.NewRequest("CONNECT", "0google.com:443", nil)

Maybe it would be sufficient to fail both of the above http.NewRequest calls? We could do this by adding a requirement to http.NewRequest: when the method is "CONNECT", the URL must contain a non-empty host. In theory, this could break callers ... I can't imagine there are any callers that legitimately want to create a CONNECT request with an empty host, but the possibility is there.

@tombergan tombergan reopened this Nov 3, 2017
@bradfitz
Copy link
Contributor

bradfitz commented Nov 3, 2017

I don't think this would work, for two reasons. First, what scheme does the following example use when connecting to foo.com?

Oh, right.

Okay, there's nothing we can do here except documentation.

I'll repurpose this as a documentation bug, to make it clear how to construct CONNECT requests.

@bradfitz bradfitz added Documentation NeedsFix The path to resolution is known, but the work has not been done. labels Nov 3, 2017
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Nov 3, 2017
@bradfitz bradfitz modified the milestones: Go1.11, Go1.10 Nov 3, 2017
@bradfitz bradfitz changed the title net/http: let NewRequest accept a host:port "URL" for CONNECT requests? net/http: document how to create client CONNECT requests Nov 3, 2017
@Audrey-Randall
Copy link
Author

@tombergan Thanks for the clarification!

@ewust
Copy link

ewust commented Nov 3, 2017

I don't think this would work, for two reasons. First, what scheme does the following example use when connecting to foo.com?

CONNECT isn't supposed to have a scheme; see RFC 7231, Section 4.3.6: https://tools.ietf.org/html/rfc7231#section-4.3.6

You only send a host:port, and then the proxy you sent the CONNECT method to blindly forwards arbitrary TCP data bidirectionally. It's up to the client that requested the CONNECT method to know what scheme it should talk to the remote host.

For example, if I send
CONNECT google.com:443 HTTP/1.1\r\nHost: google.com:443\r\n\r\n

The next thing I send into that is a raw TLS client hello message. So it doesn't make sense to have the CONNECT method require a scheme. In fact, if you send CONNECT https://google.com:443 HTTP/1.1 You'll get an error from anything that supports CONNECT (e.g. Squid sends a 503 with ERR_DNS_FAIL).

@bradfitz
Copy link
Contributor

bradfitz commented Nov 3, 2017

@ewust, the scheme isn't for the CONNECT argument, but for the HTTP (or HTTPS) server handling the CONNECT request. What we need to clarify in the docs is how users are expected to do CONNECT requests with Go's net/http package.

In general, Go's Transport doesn't permit sending a CONNECT request in any useful way (for HTTP/1 proxies), since you can't hijack the connection from the CONNECT response body. That is #17227.

Currently you can only do useful CONNECTS to http2 servers with Go.

So most users of CONNECT (including Go's build infrastructure in: https://github.com/golang/build/blob/ce623a5/cmd/buildlet/reverse.go#L213) either fmt.Fprintf the CONNECT request themselves, or do *http.Request.Write it to a self-dialed net.Conn, rather than using *http.Transport.

@tombergan
Copy link
Contributor

Thanks Brad. I was drafting a more poorly-written version of that response.

or do *http.Request.Write it to a self-dialed net.Conn, rather than using *http.Transport

Yep, I think this is what most people do.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/86277 mentions this issue: net/http: document CONNECT more

@golang golang locked and limited conversation to collaborators Jan 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

6 participants