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

x/net/proxy: add HTTP CONNECT support #19504

Open
menghanl opened this issue Mar 10, 2017 · 9 comments
Open

x/net/proxy: add HTTP CONNECT support #19504

menghanl opened this issue Mar 10, 2017 · 9 comments
Milestone

Comments

@menghanl
Copy link

menghanl commented Mar 10, 2017

This is a proposal to add HTTP CONNECT support to x/net/proxy package.

Implement HTTP CONNECT dialer

As what’s done for socks5, we can add an implementation for HTTP CONNECT dialer, it dials to the proxy, and then does a HTTP CONNECT handshake.

Also, we can register this dialer to proxySchemes with key “http” and key “https” as default dialers. This should be done in an init() function, so that users can overwrite those with their custom dialers if they want.

Add function FromEnvironmentForScheme(scheme)

Like FromEnvironment(), this function returns a dialer.

The existing function FromEnvironment() checks the environment variable “all_proxy”, while in the case of HTTP CONNECT, we would want to use “http_proxy” and “https_proxy”.

The new function FromEnvironmentForScheme() checks https_proxy, http_proxy and all_proxy based on the scheme.

  • If scheme is "https", it checks https_proxy, http_proxy and all_proxy.
  • If scheme is "http", it checks http_proxy and all_proxy.
  • In other cases, it checks all_proxy.

Add DialContext() function

As requested in #17759.
Adding DialContext() to Dialer interface will be a breaking change though. What's proposed in #17759 seems like a good solution.

@bradfitz
Copy link
Contributor

Note that the standard library's Transport already supports CONNECT. And see #13290 which required us adding ProxyConnectHeader in b06c93e to let people set headers in their CONNECT request. Anything you add in x/net/proxy should support the same interface, letting people set additional headers.

@menghanl
Copy link
Author

In that case, the function should take a http.Header as additional headers.
And the signature should be
func DoHTTPConnectHandshake(ctx context.Context, conn net.Conn, addr string, header http.Header) (net.Conn, error)

@bradfitz
Copy link
Contributor

A func (especially one prefixed with "Do") doesn't seem to match the style of the package.

I'd make a new type for it with a Dial method. Then the rarely-used http.Header can just be a field on the type.

You can make it take a Dialer in its constructor for now and later we can do a runtime interface check to see if it's also a ContextDialer once we add that type.

@menghanl
Copy link
Author

If the constructor takes a Dialer, but the Dialer doesn't implement DialContext, we still need to call Dial in another goroutine, and wait on context. And that goroutine may still leak.

If we add another interface ContextDialer, we would need a RegisterContextDialerType() to make sure the registered functions take ContextDialer other than Dialer. If we don't enforce this, and do a runtime type assertion instead, we may still end up with the same situation with leaked goroutines.

Would you consider making an API change for this package, probably in a future release? I would think breaking the compatibility by adding DialContext to the API would not be so unacceptable in this situation.

@gopherbot
Copy link

Change https://golang.org/cl/94535 mentions this issue: x/net/proxy: Add HTTP CONNECT proxy support

@gopherbot
Copy link

Change https://golang.org/cl/111135 mentions this issue: x/net/proxy: add HTTP CONNECT proxy support

@gopherbot
Copy link

Change https://golang.org/cl/111136 mentions this issue: net/http: replace HTTP CONNECT client implementation

@gopherbot
Copy link

Change https://golang.org/cl/134675 mentions this issue: internal/httpconnect, http2: new httpconnect package

@gopherbot
Copy link

Change https://golang.org/cl/134716 mentions this issue: net/http: expose AltProto map

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

No branches or pull requests

3 participants