Support http proxy correctly

[134130]

What type of PR is this? (check all applicable)

• [x] Refactor
• [x] Feature
• [x] Bug Fix
• [ ] Optimization
• [ ] Documentation Update
• [ ] Go Version Update
• [ ] Dependency Update

Description

• Currently, the gorilla/websocket's HTTP Proxy implementation is only supports CONNECT proxy, which can be called as "Tunnel Proxy".
• But there is another HTTP Proxy:
  • At the TCP level, the request is made to the Proxy address.
  • But the Host in the HTTP request is different.
• Typically, the HTTP_PROXY environment variable works as later one, and the HTTPS_PROXY environment variable works as "Tunnel Proxy".
  • Many applications, including curl and go http client, follow this behavior.
  • So there is a reason, why #892 is issued.
• I have referenced the golang's http/transport implementation: https://github.com/golang/go/blob/master/src/net/http/transport.go#L1744

Related Tickets & Documents

• Closes #892

Added/updated tests?

• [x] Yes
• [ ] No, and this is why: please replace this line with details on why tests have not been included
• [ ] I need help with writing tests

Run verifications and test

• [ ] make verify is passing
• [ ] make test is passing

No Makefile I can find. I have tested with go test ./... Also, I have wrote the integration tests like this: https://github.com/134130/websocket/blob/proxy-test/proxy_test.go

[134130]

I want to make another PR which closes https://github.com/gorilla/websocket/issues/739 after this Pull Requests is merged. @gorilla/pull-request-reviewers can you review this PR?

[134130]

ping

[ghost]

This PR changes how the package connects to HTTP proxies. Can this PR break applications that use HTTP proxies, or do all proxies support both modes (CONNECT vs no CONNECT)?

[adrianosela]

Hey folks, can we add to this work to allow providing client certificates and also means to override the proxy server's CA cert pool? I assume plenty of folks out there (me included) use mTLS certificates for HTTPS proxy authentication.

[jaitaiwan]

We need Canelohills question answered before we can decide next steps.

[adrianosela]

Maybe as a quick work-around to unblock those of us waiting for this change, we can just turn the private function proxy_RegisterDialerType function into a public one (RegisterDialerType) and allow folks to register their own custom scheme dialers...

I've done that in a local (vendored) copy of this repo and I am able to register my custom dialer for "https" and got it to work.

For @134130, here's some code that does what my dialer does... if anything for inspiration 😄:

func getTcpConnOverHttpsProxy(
	target string,
	clientCert tls.Certificate,
        proxyCertPool *x509.CertPool,
) (net.Conn, error) {
	host, _, err := net.SplitHostPort(target)
	if err != nil {
		return nil, fmt.Errorf("failed to parse host from target address: %v", err)
	}

	tcpToProxy, err := net.Dial("tcp", target)
	if err != nil {
		return nil, fmt.Errorf("failed to establish tcp connection to proxy: %v", err)
	}

	tlsToProxy := tls.Client(tcpToProxy, &tls.Config{
		ServerName:   host,
		Certificates: []tls.Certificate{clientCertificate},
		RootCAs:      proxyCertPool,
	})

	if err = tlsToProxy.Handshake(); err != nil {
		tlsToProxy.Close()
		return nil, fmt.Errorf("failed to establish tls connection to proxy: %v", err)
	}

	connectReq := fmt.Sprintf("CONNECT %s HTTP/1.1\r\nHost: %s\r\n\r\n", target, host)
	if _, err = tlsToProxy.Write([]byte(connectReq)); err != nil {
		tlsToProxy.Close()
		return nil, fmt.Errorf("failed to write CONNECT http request to proxy: %v", err)
	}

	response, err := http.ReadResponse(bufio.NewReader(tlsToProxy), &http.Request{Method: "CONNECT"})
	if err != nil {
		tlsToProxy.Close()
		return nil, fmt.Errorf("failed to read response for CONNECT http request to proxy: %v", err)
	}

	if response.StatusCode != http.StatusOK {
		tlsToProxy.Close()
		return nil, fmt.Errorf("failed to establish CONNECT tunnel to proxy, got status code: %d", response.StatusCode)
	}

	tcpToTarget := tlsToProxy

	return tcpToTarget, nil
}

[134130]

Hey folks, can we add to this work to allow providing client certificates and also means to override the proxy server's CA cert pool? I assume plenty of folks out there (me included) use mTLS certificates for HTTPS proxy authentication.

I thought the PR is large enough.

As I wrote above, the next PR will support TLS between client and proxy. Thanks!

[ghost]

@adrianosela I removed proxy_RegisterDialerType from this package. The function is replaced by proxy.RegisterDialerType.

[jaitaiwan]

Alright, now I'm lost :sweat_smile: where are we at now?

[134130]

This PR changes how the package connects to HTTP proxies. Can this PR break applications that use HTTP proxies, or do all proxies support both modes (CONNECT vs no CONNECT)?

@canelohill

Sorry for the lack of explanation. The PR makes both of CONNECT and No CONNECT Proxy mode support.

• AS IS: gorilla/websocket always tries "CONNECT Proxy", even if "No CONNECT Proxy" is correct.
• Now: gorilla/websocket correctly choose the Proxy mode.

[134130]

@adrianosela I removed proxy_RegisterDialerType from this package. The function is replaced by proxy.RegisterDialerType.

IMO, gorilla/websocket should support default proxy modes as it's own feature, and should not have behaviour that conflicts with golang's net/http, not something users need to make implement.

[ghost]

@134130 Your comments do not address my question.

Can this PR break an application that is currently using an HTTP proxy? This is a different question from what the package should do.

Did you test the change with popular proxies?

[134130]

-	MITM Proxy	Fiddler	Burp Suite	goproxy	gomitmproxy
Before (HTTP)	✅	✅	✅	✅	✅
Before (HTTPS)	✅	✅	✅	✅	✅
After (HTTP)	✅	✅	❌	✅	✅
After (HTTPS)	✅	✅	✅	✅	✅

Oops, Sorry. Burp Suite will be broken with these changes.

Previously, HTTP_PROXY didn't work correctly on me, and there was another issue that symptom is the same. But I think I've misconfigured the settings at that time.

Because many client libraries are using No-Connect method for non-websocket requests (including SSE), but gorilla/websocket always uses Connect method, and It looks weird to me, and thats why I've thought that these changes are needed.

I think this PR is no need to be merged. I'll make another PR which closes: #739. Sorry for wasting your time.

[ghost]

I suspected that this PR would not work with a proxy server in the wild because the WebSocket protocol uses full-duplex data transmission.

The HTTP protocol (and SSE layered on HTTP) uses half-duplex data transmission. The websocket protocol will not work through a proxy that assumes that all data transmission is half-duplex.

The CONNECT method tells the proxy to get out of the way and just copy the data back and forth.