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

[bug] Can't establish websocket connection when using NetDialTLSContext and Proxy together #779

Open
sleeyax opened this issue May 4, 2022 · 2 comments · May be fixed by #782
Open

[bug] Can't establish websocket connection when using NetDialTLSContext and Proxy together #779

sleeyax opened this issue May 4, 2022 · 2 comments · May be fixed by #782
Labels

Comments

@sleeyax
Copy link

sleeyax commented May 4, 2022

Describe the bug
I can't connect to a websocket server over a HTTP proxy when I set both NetDialTLSContext and Proxy fields on the websocket.Dialer struct. See the code snippet below for more details, it's best explained by code.

Versions
Go version: go1.18.1 linux/amd64.
Package version: v1.5.0.

Steps to Reproduce

  1. Get a HTTP proxy to connect to. You could install a local HTTP debugging proxy like Burp Suite, Charles Proxy, MITMProxy, ... on your development machine for testing.
  2. Execute the code snippet below and observe that no websocket connection can be established over the proxy you configured.

Expected behavior
The websocket dialer should be able to establish a websocket connection over a HTTP proxy while also applying a custom TLS connection without issues.

Code Snippets
The following code results in an error like websocket: bad handshake or unexpected EOF (depends on the proxy):

package main

import (
	"context"
	"crypto/tls"
	"github.com/gorilla/websocket"
	"log"
	"net"
	"net/http"
	"net/url"
)

const (
	// Local or remote HTTP proxy.
	proxyUrl = "http://127.0.0.1:8888"

	// Websocket endpoint for testing
	websocketUrl = "wss://echo.websocket.events/"
)

func main() {
	p, _ := url.Parse(proxyUrl)

	dialer := websocket.Dialer{
		Proxy:             http.ProxyURL(p),
		EnableCompression: true,
		NetDialTLSContext: func(ctx context.Context, network, addr string) (net.Conn, error) {
			// TCP dial
			netConn, err := net.Dial(network, addr)
			if err != nil {
				return nil, err
			}

			// 'NetDialTLSContext' also gets called during the proxy CONNECT for some reason (at this point 'network' equals "TCP" and 'addr' equals "127.0.0.1:8888")
			// The HTTP proxy doesn't support HTTPS however, so I return the established TCP connection early.
			// If I don't do this check, the connection hangs forever (tested with several proxies).
			// This feels kinda hacky though, not sure if this is the correct approach...
			if p.Host == addr {
				return netConn, err
			}

			// Example TLS handshake
			tlsConn := tls.Client(netConn, &tls.Config{ServerName: "echo.websocket.events", InsecureSkipVerify: true})
			if err = tlsConn.Handshake(); err != nil {
				return nil, err
			}

			return tlsConn, nil
		},
	}

	conn, _, err := dialer.Dial(websocketUrl, nil)
	if err != nil {
		log.Fatalln(err)
	}

	log.Println(conn.LocalAddr())
	// ...
}

When I check the request in burp, it looks like this:

image

As highlighted in red, the host URL became http://echo.websocket.events:443/, which obviously should just be https://echo.websocket.events/. I've been trying to debug why this is happening exactly all day but can't seem to find the culprit so I need help.

If I remove NetDialTLSContext and just specify TLSClientConfig everything works as expected. Same result with the Proxy field removed. The issue only occurs when both of these fields are set.

Am I missing something obvious?

@sleeyax sleeyax added the bug label May 4, 2022
sleeyax added a commit to sleeyax/websocket that referenced this issue May 12, 2022
Removed the call to NetDialTLSContext from the HTTP proxy CONNECT step and replaced it with a regular net.Dial in order to prevent connection issues. Custom TLS connections can now be made via the new optional ProxyTLSConnection method, after the proxy connection has been successfully established.
sleeyax added a commit to sleeyax/websocket that referenced this issue May 12, 2022
Removed the call to NetDialTLSContext from the HTTP proxy CONNECT step and replaced it with a regular net.Dial in order to prevent connection issues. Custom TLS connections can now be made via the new optional ProxyTLSConnection method, after the proxy connection has been successfully established.
@sleeyax
Copy link
Author

sleeyax commented May 12, 2022

I spent hours trying to debug and understand this issue and I think I finally figured it out! Just had to learn more about the HTTP proxy CONNECT sub-protocol to get a better understanding.

In short, what should happen is basically the following:

  1. Establish a TCP connection to the HTTP proxy.
  2. Send a HTTP request using the CONNECT method over the connection, which at least contains the target host to connect to.
  3. Establish a TLS connection to the target host through the established proxy connection (optional).

However, there's currently an issue in the implementation of this flow. See the code snippets below.

Here, conn, err := hpd.forwardDial(network, hostPort) actually calls NetDialTLSContext if it's set. But in this context it is only the first step in the flow, resulting in connection errors because the proxy doesn't understand TLS yet:

websocket/proxy.go

Lines 34 to 39 in 78cf1bc

func (hpd *httpProxyDialer) Dial(network string, addr string) (net.Conn, error) {
hostPort, _ := hostPortNoPort(hpd.proxyURL)
conn, err := hpd.forwardDial(network, hostPort)
if err != nil {
return nil, err
}

If we change this to a regular net.Dial, the first and second step in the flow are fixed. But we're not there yet.

We get the established proxy connection here:

netConn, err := netDial("tcp", hostPort)

But there's currently no API to convert an existing TCP connection net.Conn into a TLS connection tls.Conn . The NetDialTLSContext method requires you to do the dial again, so that's unusable at this point. Therefore, an API change to add such method is required. For example:

// client.go
type Dialer struct {
    // ...
    ProxyTLSConnection func(ctx context.Context, proxyConn net.Conn) (net.Conn, error)
}

Now, further down in the code we can establish the TLS connection to the target host through the proxy connection by calling this new method (if it is defined):

// client.go
if u.Scheme == "https" {
	  if d.ProxyTLSConnection != nil && d.Proxy != nil {
	  		// If we are connected to a proxy, perform the TLS handshake through the existing tunnel
	  		netConn, err = d.ProxyTLSConnection(ctx, netConn)
	  } else if d.NetDialTLSContext == nil {
	  		// ...
	  }
}

Now the third and final step in the flow is fixed!


I hope my detailed explanation is clear and helpful to someone. I'll open a PR soon.

@sleeyax sleeyax linked a pull request May 12, 2022 that will close this issue
@mikaelthd
Copy link

mikaelthd commented Oct 31, 2022

What bothers me about this is the fact that in net/http's Transport type one can specify a Proxy field as well as a DialTLSContext func which is exactly what NetDialTLSContext in gorilla was supposed to mirror. I therefore suspect that it is possible to fix this without requiring an API change.

Before posting this comment, I went ahead and had a quick peek at the source code. Even the golang source code has some weird behavior if DialTLSContext and Proxy are set. Unless I'm misreading this, it turns out that they actually skip the user defined DialTLSContext if the matched proxy URL has an HTTP scheme seen here. If the target's scheme is HTTPS they will upgrade the connection using their own TLS dial function with the user defined TLSClientConfig seen here, completely skipping DialTLSContext. Interestingly, if the proxy scheme is HTTPS, then they will attempt to establish a TLS connection using DialTLSContext directly against the proxy.

In other words, they "fixed" this issue in their code by completely ignoring it.

JeelsBoobz pushed a commit to JeelsBoobz/websocket that referenced this issue Dec 14, 2022
Removed the call to NetDialTLSContext from the HTTP proxy CONNECT step and replaced it with a regular net.Dial in order to prevent connection issues. Custom TLS connections can now be made via the new optional ProxyTLSConnection method, after the proxy connection has been successfully established.
@coreydaley coreydaley moved this to 🏗 In progress in Gorilla Web Toolkit Jul 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: 🏗 In progress
Development

Successfully merging a pull request may close this issue.

2 participants