Root certificate is prioritized over SNICallback context in HTTPS Server

[Ethan-Arrowood]

Version

22.5.1

Platform

Darwin Ethan 23.4.0 Darwin Kernel Version 23.4.0: Wed Feb 21 21:44:54 PST 2024; root:xnu-10063.101.15~2/RELEASE_ARM64_T6030 arm64

(Reproducible on other Node.js versions and platforms as well)

Subsystem

tls, https

What steps will reproduce the bug?

Edit: I've added a test that reproduces the issue here: #54251

Old Example Repro

Note: The actual common names (CN) have been replaced with fake strings. To run this yourself you will need to provide your own cert/key pairs (1 generated locally using openssl, 1 or 2 generated from Let's Encrypt).

import https from "node:https";
import fs from "node:fs";
import tls from "node:tls";
import events from "node:events";
import dns from "node:dns";

// Generated locally using `openssl` (CN=test.cloud.com)
let test = {
	cert: fs.readFileSync("certs/test.cloud.com.crt"),
	key: fs.readFileSync("certs/test.cloud.com.key"),
};

// Generated by Let's Encrypt (CN=foo.cloud.com)
let foo = {
	cert: fs.readFileSync("certs/foo.cloud.com/cert.pem"),
	key: fs.readFileSync("certs/foo.cloud.com/key.pem"),
};

// Generated by Let's Encrypt (CN=bar.cloud.com)
let bar = {
	cert: fs.readFileSync("certs/bar.cloud.com/cert.pem"),
	key: fs.readFileSync("certs/bar.cloud.com/key.pem"),
};

let contexts = new Map([
	['test.cloud.com', tls.createSecureContext(test)],
	['foo.cloud.com', tls.createSecureContext(foo)],
	['bar.cloud.com', tls.createSecureContext(bar)],
])

let server = https.createServer({
        // Set the HTTPS Server Default Cert and Key
	cert: foo.cert,
	key: foo.key,
        // SNICallback that returns key/cert pair 
	SNICallback: (servername, cb) => {
		cb(null, contexts.get(servername))
	}
}, (req, res) => {
	console.log(`Server processing request from ${req.socket.servername}`);
	res.writeHead(200);
	res.end(`Hello, World!`);
});

// You can also do this using `/etc/hosts` i.e.:
// `127.0.0.1 foo.cloud.com bar.cloud.com test.cloud.com`
https.globalAgent = new https.Agent({
	lookup: (hostname, options, cb) => {
		if (hostname === "foo.cloud.com" || hostname === "bar.cloud.com" || hostname === "test.cloud.com")
			hostname = "localhost";
		return dns.lookup(hostname, options, cb);
	},
});

server.listen(8443);

await events.once(server, 'listening');

console.log('Server is ready');

function makeRequest (hostname) {
	console.log(`\nRequesting https://${hostname}`);

	return new Promise((resolve, reject) => {
		https.get(`https://${hostname}:8443`, { rejectUnauthorized: false }, (res) => {
			let cert = res.socket.getPeerCertificate();
	
			console.log('Certificate subject', cert.subject);
			console.log('Certificate issuer', cert.issuer);
			console.log('Certificate subjectaltname', cert.subjectaltname);
	
			res.on('data', (chunk) => {
				console.log(`Response message: ${chunk.toString('utf-8')}`);
				resolve();
			});
	
			res.on("error", (e) => {
				console.log(`Response Error: ${e.message}`);
				reject(e);
			});
		}).on("error", (e) => {
			console.log(`https.get Error: ${e.message}`);
			reject(e);
		});
	});
}

await makeRequest('test.cloud.com')
await makeRequest('foo.cloud.com')
await makeRequest('bar.cloud.com')
await makeRequest('127.0.0.1') // Also need to support non SNI requests

server.close();

Output:

❯ node repro.mjs
Server is ready

Requesting https://test.cloud.com
Server processing request from test.cloud.com
Certificate subject { CN: 'foo.cloud.com' }  # <- Expect this to be test.cloud.com
Certificate issuer {
  C: 'US',
  O: "(STAGING) Let's Encrypt",
  CN: '(STAGING) Pseudo Plum E5'
}
Certificate subjectaltname DNS:foo.cloud.com # <- Expect this to be test.cloud.com
Response message: Hello, World!

Requesting https://foo.cloud.com
Server processing request from foo.cloud.com
Certificate subject { CN: 'foo.cloud.com' } # <- this is correct
Certificate issuer {
  C: 'US',
  O: "(STAGING) Let's Encrypt",
  CN: '(STAGING) Pseudo Plum E5'
}
Certificate subjectaltname DNS:foo.cloud.com # <- this is correct
Response message: Hello, World!

Requesting https://bar.cloud.com
Server processing request from bar.cloud.com
Certificate subject { CN: 'bar.cloud.com' } # <- this is correct
Certificate issuer {
  C: 'US',
  O: "(STAGING) Let's Encrypt",
  CN: '(STAGING) False Fennel E6'
}
Certificate subjectaltname DNS:bar.cloud.com # <- this is correct
Response message: Hello, World!

Requesting https://127.0.0.1
Server processing request from false # <- no req.socket.servername is expected
Certificate subject { CN: 'foo.cloud.com' } # <- this is correct
Certificate issuer {
  C: 'US',
  O: "(STAGING) Let's Encrypt",
  CN: '(STAGING) Pseudo Plum E5'
}
Certificate subjectaltname DNS:foo.cloud.com # <- this is correct
Response message: Hello, World!

How often does it reproduce? Is there a required condition?

This bug only seems to happen when the root cert/key pair is from Let's Encrypt. We ran into this issue with a Digicert first, but were able to reproduce using a locally generated certificate too.

I've further narrowed down the conditions. It seems that certain certificates (those with better CA or maybe created using an ext) will be prioritized over others without those things. It is not directly related to a certain provider. The reproduction demonstrates that the test/fixtures/keys/ca5-cert.pem is prioritized over test/fixtures/keys/agent1-cert.pem.

What is the expected behavior? Why is that the expected behavior?

We expect the correct certificate to be used every time.

Additional information

Other issues/prs I reviewed:

• https server should allow changing credentials dynamically #4464
• tls: support changing credentials dynamically #23644
• test: fix flaky test #23811
• tls: use the most recently added matching SecureContext in default SN… #36072
• https://stackoverflow.com/questions/29604697/i-cannot-use-snicallback

I also asked Claude AI about this to see if it could help me along and it replied with some interesting points:

1. Certificate Priority:
Some HTTPS implementations give priority to certain types of certificates. Let's Encrypt certificates are widely trusted and might be given precedence over locally generated ones.
2. SNI Implementation:
The SNI (Server Name Indication) callback is typically used to select the appropriate certificate after the initial TLS handshake has begun. If the server has already selected a default certificate (in this case, the Let's Encrypt one), it might not switch to the one returned by the SNI callback.
3. Certificate Chain:
Let's Encrypt certificates come with a full certificate chain that establishes trust up to a root CA. Locally generated certificates might not have this, potentially influencing the server's decision.
4. TLS/SSL Library Behavior:
The underlying TLS/SSL library (like OpenSSL) might have specific behaviors or optimizations that prefer certain certificate types or structures.

Maybe that'll help folks who understand SNI better 🤷‍♂️

[Ethan-Arrowood]

Edit: this issue doesn't seem strictly related to Let's Encrypt. I was able to create a locally generated cert that is prioritized. I think this highlights that this is maybe not a bug but a poorly documented feature of OpenSSL? If anyone knows anything about certificate prioritization, more information would be greatly appreciated. I'm working on a Node.js regression test too that hopefully demonstrates it.

[Ethan-Arrowood]

@nodejs/http @nodejs/net any ideas?

[Ethan-Arrowood]

I did a test using uWebSockets and this doesn't seem reproducible there:

const app = uWS.SSLApp({
  key_file_name: "example.com-key.pem",
  cert_file_name: 'example.com.pem',
}).addServerName('test.cloud.com', {
  key_file_name: 'test.cloud.com.key',
  cert_file_name: 'test.cloud.com.crt'
}).get('/*', (res, req) => {
  res.end('Hello World!');
}).listen(port, (token) => {
  if (token) {
    console.log('Listening to port ' + port);
  } else {
    console.log('Failed to listen to port ' + port);
  }
});

Requests to this app will get the correct certificates back regardless of the certificate content.

I think this helps point the issue back towards Node.js and its usage of OpenSSL.

[Ethan-Arrowood]

I did a debug step-through using the test I wrote in #54251 here is what I found:

Starting off with the SNICallback flow:

• node/lib/_tls_wrap.js

  Lines 214 to 230 in e4f61de

  	owner._SNICallback(servername, (err, context) => {
  	if (once)
  	return owner.destroy(new ERR_MULTIPLE_CALLBACK());
  	once = true;
  	if (err)
  	return owner.destroy(err);
  	if (owner._handle === null)
  	return owner.destroy(new ERR_SOCKET_CLOSED());
  	// TODO(indutny): eventually disallow raw `SecureContext`
  	if (context)
  	owner._handle.sni_context = context.context || context;
  	requestOCSP(owner, info);
  	});

  This is where the SNICallback is actually executed, on line 227, the context returned by the callback is assigned to the sni_context of the socket
• Following that, the requestOCSP() method is called with the updated socket + info as {servername: '[agent1.com](http://agent1.com/)', OCSPRequest: false}
• Stepping into that method we hit the very first conditional (

  node/lib/_tls_wrap.js

  Line 274 in e4f61de

  if (!info.OCSPRequest || !socket.server)

  ) which enters into requestOCSPDone(socket)
• This small function (

  node/lib/_tls_wrap.js

  Line 325 in e4f61de

  socket._handle.certCbDone();

  ) calls into a native method certCbDone() defined here: https://github.com/nodejs/node/blob/main/src/crypto/crypto_tls.cc#L1553

Stepping through TLSWrap::CertCBDone:

• There are two properties on TLSWrap::SecureContext, sni_context_ and sc_ and these are two separate certificates.
• sc_ is the default certificate for that server, meanwhile sni_context_ is in fact the correct certificate for the given servername (see screenshot for proof).

- The next important call is to [UseSNIContext](https://github.com/nodejs/node/blob/e4f61de14f8cfb83f1ce0ad1597b86278cd5f5f1/src/crypto/crypto_common.cc#L118-L131) called from here: https://github.com/nodejs/node/blob/e4f61de14f8cfb83f1ce0ad1597b86278cd5f5f1/src/crypto/crypto_tls.cc#L1573 - That function is what is doing the actual certificate resolution for the request. It makes calls to SSL and as far as I can tell it is up to SSL's resolution strategy for which certificate in a chain gets used. This is the unexpected behavior I'm struggling to understand as either "intended behavior" or a "bug" of `SNICallback`/`addContext`

[Ethan-Arrowood]

I am considering a fix to UseSNIContext. I'm digging into the SSL calls now to understand if maybe theres a better way to call them for this.

[Ethan-Arrowood]

@indutny do you happen to know whats going on here? I'm still trying to determine if there is a better way to implement UseSNIContext or not. This is my first time working with OpenSSL directly.

[Ethan-Arrowood]

I guess a code change may not be necessary here. As far as I can tell this is about the OpenSSL Verification configuration (https://docs.openssl.org/3.0/man1/openssl-verification-options/).

I think I could try using these options to get the certs to play nice.

I'd still appreciate someone with more OpenSSL experience to weigh in here whether this is a bug or not.

Thank you!