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

Gorouter: Websockets over HTTP/2 - invalid pseudo-header ":protocol" #230

Open
thomas-kaltenbach opened this issue Sep 30, 2021 · 7 comments

Comments

@thomas-kaltenbach
Copy link

thomas-kaltenbach commented Sep 30, 2021

Is this a security vulnerability?

no

Issue

Gorouter is not able to handle HTTP/2 Websockets requests (RFC 8441).

Affected Versions

latest version when http2 is enabled

Context

We are testing currently the http/2 feature and we noticed that websockets over http/2 are not working. Gorouter is closing the connection without any http response. After activating golang verbose logging (GODEBUG=http2debug=2) you can see the followed log entries:

2021/09/30 10:23:47 http2: invalid pseudo headers: invalid pseudo-header ":protocol"
2021/09/30 10:23:47 http2: Framer 0xc0003ca0e0: wrote RST_STREAM stream=1 len=4 ErrCode=PROTOCOL_ERROR

Traffic Diagram

           +----+----+            +----------+     +-------+
  \o/      |         |            |          |     |       |
   +  +--->+ HAproxy +--http/2--->+ Gorouter +---->+  App  |
  / \      |         |            |          |     |       |
 client    +---------+            +----------+     +-------+

Steps to Reproduce

I could also reproduce the issue with the followed nodejs client

'use strict';
'use strict';
const WebSocket = require('ws');
const http2 = require('http2-wrapper');

const head = Buffer.from('');

var urlArg = process.argv[2].trim();
console.log("try to connect to url '" + urlArg + "'")

const connect = (url, options) => {
	const ws = new WebSocket(null);
	ws._isServer = false;

	const destroy = async error => {
		ws._readyState = WebSocket.CLOSING;

		await Promise.resolve();
		ws.emit('error', error);
	};

	(async () => {
		try {
			const stream = await http2.globalAgent.request(url, options, {
				...options,
				':method': 'CONNECT',
				':protocol': 'websocket',
				origin: (new URL(url)).origin
			});

			stream.once('error', destroy);

			stream.once('response', _headers => {
				stream.off('error', destroy);

				stream.setNoDelay = () => {};
				ws.setSocket(stream, head, 100 * 1024 * 1024);
			});
		} catch (error) {
			destroy(error);
		}
	})();

	return ws;
};

const ws = connect(urlArg, {
	rejectUnauthorized: false
});

ws.once('open', () => {
	console.log('CONNECTED!');

	ws.send('WebSockets over HTTP/2');
});

ws.once('close', () => {
	console.log('DISCONNECTED!');
});

ws.once('message', message => {
	console.log(message);

	ws.close();
});

ws.once('error', error => {
	console.error(error);
});

Steps to run it:

  • save coding as file
  • npm install ws http2-wrapper
  • node <filename> https://<url_gorouter>

Expected result

One of the following behaviors would be acceptable:

  • Gorouter can handle the connection
  • downgrade in case of a websocket connection to HTTP/1.1
  • http response 400 Bad request

Current result

HAproxy reporting the issue with the termination state SH-- in the accesslog.

nodejs client reports followed error:

Error [ERR_HTTP2_STREAM_ERROR]: Stream closed with error code NGHTTP2_REFUSED_STREAM
    at ClientHttp2Stream._destroy [as __destroy] (internal/http2/core.js:2148:13)
    at ClientHttp2Stream.stream._destroy (/Users/d064325/git/mytools/socket_test/node_modules/http2-wrapper/source/utils/delay-async-destroy.js:12:23)
    at ClientHttp2Stream.destroy (internal/streams/destroy.js:38:8)
    at ClientHttp2Stream.[kMaybeDestroy] (internal/http2/core.js:2164:12)
    at Http2Stream.onStreamClose (internal/http2/core.js:511:26) {
  code: 'ERR_HTTP2_STREAM_ERROR'
}

Possible Fix

Root cause of the problem is the followed check:
https://github.com/golang/go/blob/e180e2c27c3c3f06a4df6352386efedc15a1e38c/src/net/http/h2_bundle.go#L2770

func (mh *http2MetaHeadersFrame) checkPseudos() error {
	var isRequest, isResponse bool
	pf := mh.PseudoFields()
	for i, hf := range pf {
		switch hf.Name {
		case ":method", ":path", ":scheme", ":authority":
			isRequest = true
		case ":status":
			isResponse = true
		default:
			return http2pseudoHeaderError(hf.Name)
		}
		// Check for duplicates.
		// This would be a bad algorithm, but N is 4.
		// And this doesn't allocate.
		for _, hf2 := range pf[:i] {
			if hf.Name == hf2.Name {
				return http2duplicatePseudoHeaderError(hf.Name)
			}
		}
	}
	if isRequest && isResponse {
		return http2errMixPseudoHeaderTypes
	}
	return nil
}

I also found followed issue golang/go#32763

I don't know if gorouter can workaround the problem or at least return a proper http response.

@Gerg
Copy link
Member

Gerg commented Oct 5, 2021

My initial thoughts:

Given the discussion in golang/go#32763, it seems like it will be difficult to get RFC 8441 support for Gorouter any time soon (let me know if I'm missing something that would make it possible 🙂).

Based on that, it seems like the next best option would be to make HAProxy (and other clients) use HTTP/1.X for websockets requests. We already impose some Load Balancer configuration requirements for supporting websockets on CF: https://docs.cloudfoundry.org/adminguide/supporting-websockets.html#config.

Doing a little digging, I found haproxy/haproxy@befeae8, though I'm not sure how it impacts HAProxy's backend connections.

@Gerg
Copy link
Member

Gerg commented Oct 6, 2021

I applied haproxy/haproxy@befeae8 as a patch to the existing HAProxy release and configured h2-workaround-bogus-websocket-clients. It did not seem to have any effect on HAProxy's communication with backends.

@Gerg
Copy link
Member

Gerg commented Oct 6, 2021

I was able to hack together a HAProxy config that appears to work for websockets:

Relevant excerpts:

# HTTPS Frontend {{{
frontend https-in
    mode http
      bind :443  ssl crt /var/vcap/jobs/haproxy/config/ssl   alpn h2,http/1.1

    http-request del-header X-Forwarded-Client-Cert
    http-request del-header X-SSL-Client
    http-request del-header X-SSL-Client-Session-ID
    http-request del-header X-SSL-Client-Verify
    http-request del-header X-SSL-Client-Subject-DN
    http-request del-header X-SSL-Client-Subject-CN
    http-request del-header X-SSL-Client-Issuer-DN
    http-request del-header X-SSL-Client-NotBefore
    http-request del-header X-SSL-Client-NotAfter


    capture request header Host len 256
    default_backend http-routers
    acl xfp_exists hdr_cnt(X-Forwarded-Proto) gt 0
    acl is_websocket hdr(Upgrade) -i WebSocket          # Add is_websocket ACL looking for upgrade header
    acl is_websocket hdr_beg(Host) -i ws                # is_websocket ACL also looks for ws* scheme
    use_backend http-routers-ws if is_websocket         # If request matches ACL, route to http-routers-ws backend instead
    http-request add-header X-Forwarded-Proto "https" if ! xfp_exists
# }}}
# Default Backend {{{
backend http-routers
    mode http
    balance roundrobin
        errorfile 503 /var/vcap/jobs/haproxy/errorfiles/custom503.http

    server node0 10.244.0.34:443 check inter 1000  ssl verify required ca-file /var/vcap/jobs/haproxy/config/backend-ca-certs.pem alpn h2,http/1.1
# }}}
backend http-routers-ws         # New backend, identical to http-routers, except without alpn
    mode http
    balance roundrobin
        errorfile 503 /var/vcap/jobs/haproxy/errorfiles/custom503.http

    server node0 10.244.0.34:443 check inter 1000  ssl verify required ca-file /var/vcap/jobs/haproxy/config/backend-ca-certs.pem

Gerg added a commit to pet-forks/docs-cf-admin that referenced this issue Oct 8, 2021
@ameowlia ameowlia assigned ameowlia and Gerg and unassigned ameowlia Oct 28, 2021
@ameowlia ameowlia moved this from Issue Inbox to Blocked in DEPRECATED App Platform - Diego Oct 28, 2021
@ameowlia ameowlia moved this from Blocked to Reviewer Assigned + Pending Review in DEPRECATED App Platform - Diego Oct 28, 2021
@ameowlia ameowlia moved this from Reviewer Assigned + Pending Review to Issue Reviewer Assigned in DEPRECATED App Platform - Diego Oct 28, 2021
@ameowlia ameowlia removed this from Issue Reviewer Assigned in DEPRECATED App Platform - Diego Oct 28, 2021
@ameowlia ameowlia moved this from Newly opened - needs reviewer assigned to In progress in DEPRECATED App Platform - Networking Oct 28, 2021
@ameowlia ameowlia moved this from Reviewer Assigned to Review in progress in DEPRECATED App Platform - Networking Oct 28, 2021
@Gerg
Copy link
Member

Gerg commented Jan 19, 2022

@thomas-kaltenbach Given the changes introduced in cloudfoundry/haproxy-boshrelease#263, is there any remaining work for this issue?

@plowin
Copy link
Contributor

plowin commented Feb 9, 2022

Hi @Gerg, note that Thomas moved within our Org, taking over.

In cloudfoundry/haproxy-boshrelease#263 we downgrade all websockets to h/1 even though h/2 is configured.
Accordingly, this is still an issue and this workaround should not remain for ever. Seems to depend on golang/go#32763.

@Gerg Gerg added the blocked label Mar 18, 2022
@MarcPaquette
Copy link
Contributor

Hi @plowin ,

Checking in on this issue. Is there any further work at this point or is it safe to close this out?

@MarcPaquette MarcPaquette assigned plowin and unassigned Gerg May 2, 2024
@plowin
Copy link
Contributor

plowin commented May 3, 2024

Hi @MarcPaquette , thx for re-assigning.
I prefer keeping it open in state blocked. At some point in the far future, all traffic might be H/2 and we need support for it in gorouter. Keeping an eye on golang/go#32763

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Waiting for Changes | Open for Contribution
DEPRECATED - WG-Application-Runtime-P...
Old PRs and Issues (pre-project creat...
Development

No branches or pull requests

5 participants