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

[Feature] Support WebSocket upgrade protocol in reverse proxy. #25

Closed
naturalethic opened this issue Apr 10, 2023 · 9 comments
Closed

Comments

@naturalethic
Copy link

I'm trying to install a service (minio) as proxied by mox, and some features do not work because a web socket connection cannot be established.

@mjl-
Copy link
Owner

mjl- commented Apr 11, 2023

Sounds reasonable to want. It looks like websockets shouldn't be too hard to implement, though will have to keep an eye on potential security issues. I presume in this case that minio is offering a ws:// url? Or is it wss://? Let me know if you have know about things the implementation would need take special care for.

I don't know when I'll have time to work on this. I'm short on time at the moment.

@naturalethic
Copy link
Author

The client is using wss:// as the site is served over https. The service however is open and the ssl termination should occur on the proxy, same as http/s. I have switched to using mox to do all my reverse proxying, because it eases cert management between emails and http service.

I asked GPT-4 how it would modify your handler to support this. I'm not a Go programmer, so I cannot evaluate the utility of the following code, but perhaps it is a jumping off point.

GPT-4 response follows.


To support HTTP Upgrade for WebSockets, you can modify the HandleForward function by including the following changes:

  1. Add "github.com/gorilla/websocket" to your imports.

  2. Define a custom director for the reverse proxy that handles WebSocket upgrade requests separately.

  3. Add a custom connection handling function for WebSocket connections.

Here's the modified HandleForward function:

import (
	// Other imports...
	"github.com/gorilla/websocket"
)

// HandleForward handles a request by forwarding it to another webserver and
// passing the response on. I.e. a reverse proxy.
func HandleForward(h *config.WebForward, w http.ResponseWriter, r *http.Request, path string) (handled bool) {
	// Existing function contents...

	// Define a custom director for the reverse proxy
	proxy.Director = func(req *http.Request) {
		// Update the request header as before
		req.Header = hdr
		req.URL.Scheme = h.TargetURL.Scheme
		req.URL.Host = h.TargetURL.Host

		if websocket.IsWebSocketUpgrade(req) {
			// Handle WebSocket connections
			handleWebSocketUpgrade(req, w, h.TargetURL)
			return
		}
	}

	whdr := w.Header()
	for k, v := range h.ResponseHeaders {
		whdr.Add(k, v)
	}
	proxy.ServeHTTP(w, r)
	return true
}

// handleWebSocketUpgrade handles WebSocket upgrade requests by forwarding them to the target server.
func handleWebSocketUpgrade(r *http.Request, w http.ResponseWriter, targetURL *url.URL) {
	dialer := websocket.DefaultDialer
	target := targetURL.String()

	// Replace the scheme for WebSocket connections
	target = strings.Replace(target, "http://", "ws://", 1)
	target = strings.Replace(target, "https://", "wss://", 1)

	connHeader := http.Header{}
	for k, v := range r.Header {
		connHeader[k] = v
	}

	// Dial the target WebSocket server
	targetConn, _, err := dialer.Dial(target, connHeader)
	if err != nil {
		http.Error(w, "Error connecting to WebSocket backend", http.StatusBadGateway)
		return
	}
	defer targetConn.Close()

	// Upgrade the client connection to a WebSocket
	upgrader := websocket.Upgrader{}
	clientConn, err := upgrader.Upgrade(w, r, nil)
	if err != nil {
		http.Error(w, "Error upgrading client connection to WebSocket", http.StatusBadRequest)
		return
	}
	defer clientConn.Close()

	// Start the proxying of WebSocket messages
	errCh := make(chan error, 2)
	go proxyWS(targetConn, clientConn, errCh)
	go proxyWS(clientConn, targetConn, errCh)

	// Wait for an error or the connections to close
	<-errCh
}

// proxyWS forwards WebSocket messages between two connections.
func proxyWS(srcConn, dstConn *websocket.Conn, errCh chan error) {
	for {
		msgType, msg, err := srcConn.ReadMessage()
		if err != nil {
			errCh <- err
			break
		}
		err = dstConn.WriteMessage(msgType, msg)
		if err != nil {
			errCh <- err
			break
		}
	}
}

This modification adds WebSocket support to your reverse proxy by defining a custom director for the reverse proxy, which checks for WebSocket upgrade requests and forwards them to the handleWebSocketUpgrade function. The handleWebSocketUpgrade function establishes WebSocket connections to both the client and the target server, and then starts proxying WebSocket messages between the two connections.

@mjholub
Copy link

mjholub commented May 24, 2023

@naturalethic I've made a PR with a solution, please try running my fork and and let me know if it solves your issue.

@naturalethic
Copy link
Author

@154pinkchairs thanks for this work.

I only have a production setup so I'd prefer to get @mjl- stamp of approval before I deploy there -- I don't have the expertise in Go to evaluate your PR myself, nor the time at the moment to properly set up a test environment.

@mjl-
Copy link
Owner

mjl- commented May 25, 2023

i'll try to look at the PR tomorrow, thanks for sending it in!

@mjl-
Copy link
Owner

mjl- commented May 26, 2023

i had a look, and i'm afraid we'll need a different approach.
see golang/go#18152 for comments on the golang.org/x/net/websocket package. summary: it is providing the wrong api (without knowing the details, it looks it isn't preserving the payload type).
the suggested implementation with gorilla also isn't great: gorilla is deprecated. besides that, i don't want to introduce external dependencies this easily.
and i think we don't have to. i think we can get away with only monitoring the websocket "handshake", passing it along to the backend, verify that the backend understands it, and forwards its response. and then just shuffle around the bytes both sides send each other. that does mean the backend is exposed to potentially invalid websocket frames. we could in theory monitor the frames too, but i don't think it's worth the trouble. the whole point of websocket is to communicate mostly raw bytes between two sides (http just happens to be in the way).
i'll try to get this concept working in a basic form and will report back.

mjl- added a commit that referenced this issue May 30, 2023
if we recognize that a request for a WebForward is trying to turn the
connection into a websocket, we forward it to the backend and check if the
backend understands the websocket request. if so, we pass back the upgrade
response and get out of the way, copying bytes between the two. we do log the
total amount of bytes read from the client and written to the client. if the
backend doesn't respond with a websocke response, or an invalid one, we respond
with a regular non-websocket response. and we log details about the failed
connection, should help with debugging and any bug reports.

we don't try to parse the websocket framing, that's between the client and the
backend.  we could try to parse it, in part to protect the backend from bad
frames, but it would be a lot of work and could be brittle in the face of
extensions.

this doesn't yet handle websocket connections when a http proxy is configured.
we'll implement it when someone needs it. we do recognize it and fail the
connection.

for issue #25
@mjl-
Copy link
Owner

mjl- commented May 30, 2023

this latest commits made my basic websocket application work (built with golang.org/x/net/websocket) when forwarded through mox.

@naturalethic when you are ready for it, you could give it a try.

@154pinkchairs with this approach, only the http request/response part of websockets are verified/interpreted by mox. if that looks good, meaning the backend has the right headers indicating support, the connection is upgraded and mox just copies data between the client and the backend. golang.org/x/net/websocket is indeed useful for testing! but in the PR you made, i think we ended up parsing the websocket framing, and packing it again when copying. i think we would lose information when interpreting the frames. could you have a look at the last commit? i don't normally use websockets, so i may not be up to date on all the intricacies and have missed some handling.

@naturalethic
Copy link
Author

@mjl- Looks good. I've got minio running behind it. Thanks! And thanks @154pinkchairs for your contribution.

@mjl-
Copy link
Owner

mjl- commented Jun 1, 2023

great to hear, thanks for testing, and thanks @154pinkchairs for the PR!

@mjl- mjl- closed this as completed Jun 1, 2023
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

Successfully merging a pull request may close this issue.

3 participants