[feature] Add option to support late addition of headers

[mvduin]

This adds an option for WebSocket clients that makes it possible to add request headers late minute, specifically to support the use-case where a header can't be determined until the socket is connected (TLS channel binding), which means the req.end() call needs to be deferred.

I wasn't sure what the most elegant way to support this was so I've tried to keep this simple and minimally invasive, especially since it's for a rather obscure use-case. It just adds a callback hook which, if provided, will be invoked instead of req.end(). It's then up to the provided function call req.end() when it's done customizing the request.

If this interface looks okay I'll amend this pull request to document the option and add a test.

[lpinca]

specifically to support the use-case where a header can't be determined until the socket is connected (TLS channel binding), which means the req.end() call needs to be deferred.

Can you show an example using the Node.js https module?

[mvduin]

Here's a minimalistic toy demo of using the tls-exporter channel binding to securely authenticate a websocket client using a pre-shared key. The authentication is bound to the TLS connection which means that even if an attacker successfully impersonates the server to the client, the credential they receive from the client (the Authorization header) cannot be used by the attacker to authenticate to the real server.

This example of course assumes my patch has been applied to ws.

import { WebSocket, WebSocketServer } from 'ws';
import https from 'https';
import fs from 'fs';
import crypto from 'crypto';
import events from 'events';

// CAUTION: this implementation requires TLSv1.3.  older TLS versions can use tls-exporter
// channel binding but that requires additional checks to be implemented to be secure.
function get_tls_exporter_channel_binding( socket ) {
	return Buffer.concat([
		Buffer.from('tls-exporter:'),
		socket.exportKeyingMaterial( 32, 'EXPORTER-Channel-Binding', Buffer.alloc(0) )
	]);
}

// note: this is just a toy example and not a real-world authentication protocol
function _tls_silly_psk_auth_data( socket, psk ) {
	let hmac = crypto.createHmac( 'sha256', psk );
	hmac.update( get_tls_exporter_channel_binding( socket ) );
	return hmac.digest( 'base64url' );
}
async function tls_silly_psk_auth_apply( req, psk ) {
	let socket = req.socket || ( await events.once( req, 'socket' ) )[0];
	if( socket.secureConnecting )
		await events.once( socket, 'secureConnect' );
	let auth = 'X-Silly-PSK-Auth ' + _tls_silly_psk_auth_data( socket, psk );
	req.setHeader( 'Authorization', auth );
}
function tls_silly_psk_auth_verify( req, psk ) {
	let auth = req.headers.authorization;
	if( ! auth )
		return false;
	// FIXME this should probably parse the header properly in case alternate
	// authorizations are provided alongside the one we care about
	[ , auth ] = /^\s*X-Silly-PSK-Auth\s+([\w-]+)\s*$/i.exec( auth ) || [];
	return auth === _tls_silly_psk_auth_data( req.socket, psk );
}


const shared_secret = Buffer.from( '81f98e0e75dbaefa6e62363b293d9629', 'hex' );

const wss = new WebSocketServer({ noServer: true, path: '/' });

const https_server = https.createServer({
	cert: fs.readFileSync('test/fixtures/certificate.pem'),
	key: fs.readFileSync('test/fixtures/key.pem'),
	minVersion: 'TLSv1.3',  // see caution above get_tls_exporter_channel_binding()
});
https_server.on( 'upgrade', ( req, socket, head ) => {
	if( ! tls_silly_psk_auth_verify( req, shared_secret ) ) {
		socket.write(
			'HTTP/1.1 401 Unauthorized\r\n' +
			'WWW-Authenticate: X-Silly-PSK-Auth\r\n' +
			'Content-Length: 0\r\n' +
			'Connection: close\r\n' +
			'\r\n' );
		socket.destroy();
		return;
	}
	wss.handleUpgrade( req, socket, head, ws => {
		console.log( `client connected` );
		ws.on( 'close', ( code, reason ) => {
			reason = reason.toString();
			console.log( `client disconnected`, { code, reason } );
		});
	});
});
https_server.listen( 8123 );
https_server.unref();
await events.once( https_server, 'listening' );
console.log( 'server listening on', https_server.address() );


let ws = new WebSocket( 'wss://localhost:8123/', {
	async finishRequest( req ) {
		await tls_silly_psk_auth_apply( req, shared_secret );
		req.end();
	},
	minVersion: 'TLSv1.3',  // see caution above get_tls_exporter_channel_binding()
	rejectUnauthorized: false,  // ignore bogus server cert
});
await events.once( ws, 'open' );
console.log( 'test successful!' );
ws.close();

[lpinca]

I meant an example without ws to see how you would normally do it for HTTP (no WebSocket) but the above example also shows that.

It is really an obscure use case and I don't like to expose the http.ClientRequest object to the user but I have no better idea. Currently the http.ClientRequest is only exposed to the listeners of the 'redirect' event.

If this interface looks okay I'll amend this pull request to document the option and add a test.

Go ahead. Thank you.

[lpinca]

Please add a test.

[lpinca]

Please clarify that customization should be limited to inspecting and adding headers. For example, the user should not add data to the request body or call request.destroy(). See the description of the 'redirect' event.

[mvduin]

I was wondering, would this perhaps be a nicer approach:

  if (typeof opts.setLateHeaders === 'function') {
    (async () => {
      await opts.setLateHeaders(req, websocket);
      if (req === null || req[kAborted]) return;
      req.end();
    })();
  } else {
    req.end();
  }

This makes options.setLateHeaders an async function that only sets the headers, the responsibility to end() the request remains with ws. It also makes the name of the option specifically focussed on setting headers.

[lpinca]

I think I prefer the original suggestion for simplicity. If we want to keep the responsibility of calling request.end() inside ws, then for consistency with the verifyClient hook, the setLateHeaders function should take a callback instead of being an async function.

if (typeof opts.setLateHeaders) {
  opts.setLateHeaders(req, function done() {
    req.end();
  });
} else {
  req.end();
}

Anyway, if users have access to the http.ClientRequest object nothing prevents them from doing whatever they want with it.

[mvduin]

Yep, fair enough, I'll keep the API as originally proposed and should have the documentation+test done soon.

[mvduin]

Okay, test has been added and documentation updated based on your feedback.

[lpinca]

Is this argument needed? The user already has access to the WebSocket instance returned by the constructor, no?

[mvduin]

finishRequest is called from the constructor, so the new WebSocket hasn't been returned yet

[lpinca]

Fair point, but is it needed? What would it be used for?

[mvduin]

I have no specific use-case, it just seemed polite to provide it to finishRequest just in case it's needed since it gets invoked before the caller has access to the websocket. It's not critical, but it takes no effort to pass it as argument while it would be annoying if it doesn't get passed and you do end up needing it.

[lpinca]

I think we can add it later if someone needs it.

[lpinca]

On second thought, it might be useful if someone want to call websocket.close() or websocket.terminate() synchronously.

[mvduin]

Or they may just want to keep track of the WebSocket for which they're doing asynchronous work for bookkeeping/debugging purposes. Passing it just seemed like a sensible thing to do.

[lpinca]

On second thought, it might be useful if someone want to call websocket.close() or websocket.terminate() synchronously.

I think it doesn't make much sense 😄 . Why would anyone do it?

[mvduin]

In the end I don't care enough to continue debating it, I'll remove the websocket argument.

[lpinca]

No, keep it. I quoted my comment, not yours.

[mvduin]

I've fixed the lint issues

[lpinca]

Suggested change

	it('supports late headers using finishRequest', (done) => {
	it('honors the `finishRequest` option', (done) => {

[lpinca]

Suggested change

	finishRequest(req, ws_arg) {
	setTimeout(() => {
	assert.strictEqual(ws_arg, ws);
	assert.strictEqual(req, ws._req);
	req.setHeader('Cookie', 'foo=bar');
	req.end();
	}, 150);
	}
	finishRequest(request, websocket) {
	assert.strictEqual(request, ws._req);
	assert.strictEqual(websocket, ws);
	setImmediate(() => {
	req.setHeader('Cookie', 'foo=bar');
	req.end();
	});
	}

[mvduin]

I used setTimeout to verify that it works "later" when everything that can progress without req.end() has had a chance to do so, setImmediate will not be sufficient for that.

[lpinca]

It doesn't matter we can also do it synchronously. We only have to test that a header added here is received. Headers are buffered and not sent (unless request.flushHeaders() is called) until request.end() is called.

[mvduin]

I know that delaying it a bit shouldn't matter, but I'd prefer the test to confirm that this is genuinely the case. I could alternatively make the test reflect actual expected use:

    it('honors the `finishRequest` option', (done) => {
      const server = https.createServer({
        cert: fs.readFileSync('test/fixtures/certificate.pem'),
        key: fs.readFileSync('test/fixtures/key.pem')
      });
      const wss = new WebSocket.Server({ server });

      wss.on('connection', (ws, req) => {
        assert.strictEqual(req.headers.cookie, 'foo=bar');
        server.close(done);
      });

      server.listen(0, () => {
        const ws = new WebSocket(`wss://localhost:${wss.address().port}`, {
          rejectUnauthorized: false,
          finishRequest(req, ws) {
            assert.strictEqual(req, ws._req);
            req.on('socket', socket => {
              socket.on('secureConnect', () => {
                req.setHeader('Cookie', 'foo=bar');
                req.end();
              });
            });
          }
        });

        ws.on('open', () => {
          ws.close();
        });
      });
    });

Also, all other tests abbreviate request to req and websocket to ws, it would be inconsistent to spell them out here.

[lpinca]

It would certainly be better than waiting arbitrarily for 150 ms. I don't think the test needs to use TLS, it can use the 'connect' event of the socket.

Also, all other tests abbreviate request to req and websocket to ws, it would be inconsistent to spell them out here.

My suggestion was because I don't like ws_arg (snake case) or wsArg. ws can't be used, so for consistency with the verbose websocket I also used the verbose request.

[mvduin]

How about

    it('honors the `finishRequest` option', (done) => {
      const wss = new WebSocket.Server({ port: 0 }, () => {
        const ws = new WebSocket(`ws://localhost:${wss.address().port}`, {
          finishRequest(req, ws) {
            assert.strictEqual(req, ws._req);
            req.on('socket', socket => {
              socket.on('connect', () => {
                req.setHeader('Cookie', 'foo=bar');
                req.end();
              });
            });
          }
        });

        ws.on('open', () => {
          ws.close();
        });
      });

      wss.on('connection', (ws, req) => {
        assert.strictEqual(req.headers.cookie, 'foo=bar');
        wss.close(done);
      });
    });

this doesn't explicitly test that the inner ws equals the outer one but if the ws argument isn't the websocket then the assert.strictEqual(req, ws._req); test will catch that anyway.

[lpinca]

I'm not very happy with it because that is an implementation detail. Anyway my bad, I should have fixed the nits myself after merging.

[lpinca]

See 23acf8c. It is similar to your suggestion with a few more assertions to make sure that the http.ClientRequest received by the finishRequest function is the same created by the WebSocket constructor. In this way withassert.strictEqual(req, ws._req) we also make sure that the inner ws is the same as the outer ws as they share the same request.

There are still no checks for reference equality so it is not perfect but I prefer it to using process.nextTick().

[mvduin]

I've updated it based on your feedback.

[lpinca]

Thank you.