reverse_proxy: h2 websocket stuck if client needs to send the first message and encode is enabled

[WeidiDeng]

With this merged, I noticed some of my websockets failed to connect while others succeed.

I disabled encode and found out now these requests connect successfully.

The problem is that client requests contain the header accept-encoding: gzip, deflate, br, zstd, which is wrapped

caddy/modules/caddyhttp/encode/encode.go

Lines 155 to 160 in 9c0c71e

	for _, encName := range AcceptedEncodings(r, enc.Prefer) {
	if _, ok := enc.writerPools[encName]; !ok {
	continue // encoding not offered
	}
	w = enc.openResponseWriter(encName, w)
	defer w.(*responseWriter).Close()

This response writer will write informational header immediately

caddy/modules/caddyhttp/encode/encode.go

Lines 250 to 252 in 9c0c71e

	if 100 <= status && status <= 199 {
	rw.ResponseWriter.WriteHeader(status)
	}

During websocket upgrade, for http1, the upgrade will write 101 status,

caddy/modules/caddyhttp/reverseproxy/streaming.go

Line 122 in 9c0c71e

rw.WriteHeader(res.StatusCode)

which will be flushed to the client immediately.

However, for h2, although FlushError is called

caddy/modules/caddyhttp/reverseproxy/streaming.go

Line 111 in 9c0c71e

flushErr := http.NewResponseController(rw).Flush()

response code is 200 same as any other status that indicates success. Because we want to determine if the response can be encoded, this code is stored and flushed later if there is any data from the upstream and we can be sure whether the response can be encoded

caddy/modules/caddyhttp/encode/encode.go

Lines 262 to 269 in 9c0c71e

	func (rw *responseWriter) FlushError() error {
	if !rw.wroteHeader {
	// flushing the underlying ResponseWriter will write header and status code,
	// but we need to delay that until we can determine if we must encode and
	// therefore add the Content-Encoding header; this happens in the first call
	// to rw.Write (see bug in #4314)
	return nil
	}

If the websocket transport a protocol that server sends response first, it is OK as that will cause the status and data to be sent. But if the client sends data first, it won't happen because the response will never be sent, and client thinks the handshake didn't finish.

For now, I created a matcher to ignore encoding for h2 websocket upgrade

@not_h2_ws not {
    header :protocol *
    method CONNECT
    protocol http/2
}
encode @not_h2_ws zstd gzip

But I think an elegant solution should be implemented.

[WeidiDeng]

I suppose I can manually unwrap the response writer until no more unwrap can be done. And use this unwrapped response writer instead. What do you think @mholt ?

[WeidiDeng]

h1 rfc, h2 rfc and h3 rfc all states that CONNECT requests are used to establish a tunnel to the remote hosts and 200 means the connections is successful. The server should then forward the packets blindly between the client and the remote host.

In websockets over h2 scenario, this is also true. As in all cases where websockets are involved.

[bt90]

There seems to be some functionality in place to avoid encoding WebSocket requests:

caddy/modules/caddyhttp/encode/encode.go

Lines 400 to 407 in 57ae9c3

	// AcceptedEncodings returns the list of encodings that the
	// client supports, in descending order of preference.
	// The client preference via q-factor and the server
	// preference via Prefer setting are taken into account. If
	// the Sec-WebSocket-Key header is present then non-identity
	// encodings are not considered. See
	// http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html.
	func AcceptedEncodings(r *http.Request, preferredOrder []string) []string {

I think we should give all CONNECT requests the identity treatment here.

[bt90]

The h2 WebSocket requests aren't using the Sec-WebSocket-Key header which is probably the culprit here. see https://datatracker.ietf.org/doc/html/rfc8441#section-5

Implementations using this extended CONNECT to bootstrap WebSockets do not do the processing of the Sec-WebSocket-Key and Sec-WebSocket-Accept header fields of [RFC6455] as that functionality has been superseded by the :protocol pseudo-header field.

[bt90]

Speaking of Sec-WebSocket-Key and Sec-WebSocket-Accept, I think we're missing the :protocol <-> header conversion for the backend?

[WeidiDeng]

:protocol is converted to Upgrade header.

I think for a non successful upgrade, encode is still useful, instead of bypassing it completely. Users may choose to display custom page.

[bt90]

:protocol is converted to Upgrade header.

The problem is that the backend is still using HTTP/1.1 and therefore expects RFC 6455 compliance: https://datatracker.ietf.org/doc/html/rfc6455#section-11.3.1

Since RFC 8441 has dropped those headers, we would have to handle them for the backend connection. That means we need to create a random Sec-WebSocket-Key for the initial handshake request and strip the obsolete Sec-WebSocket-Accept header from the handshake response.

[bt90]

Apaches mod_http2 does the same:

https://github.com/icing/mod_h2/blob/9f19b6e268198c5e8145bb36e196909c4c9a85df/mod_http2/h2_ws.c#L118-L151

[WeidiDeng]

That header is created. Why do you say it's not?

caddy/modules/caddyhttp/reverseproxy/reverseproxy.go

Line 414 in 57ae9c3

clonedReq.Header["Sec-WebSocket-Key"] = []string{base64.StdEncoding.EncodeToString(key)}

Anyway, that's not relevant in this issue.

[bt90]

My bad, missed it.

[bt90]

I think for a non successful upgrade, encode is still useful, instead of bypassing it completely. Users may choose to display custom page.

Sounds reasonable 👍