Root certificate is prioritized over SNICallback context in HTTPS Server

Ethan-Arrowood commented 2 months ago

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: https://github.com/nodejs/node/pull/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). ```js 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:

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 commented 2 months ago

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 commented 2 months ago

@nodejs/http @nodejs/net any ideas?

Ethan-Arrowood commented 2 months ago

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 commented 2 months ago

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

Starting off with the SNICallback flow:

https://github.com/nodejs/node/blob/e4f61de14f8cfb83f1ce0ad1597b86278cd5f5f1/lib/_tls_wrap.js#L214-L230 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 (https://github.com/nodejs/node/blob/e4f61de14f8cfb83f1ce0ad1597b86278cd5f5f1/lib/_tls_wrap.js#L274) which enters into requestOCSPDone(socket)
This small function (https://github.com/nodejs/node/blob/e4f61de14f8cfb83f1ce0ad1597b86278cd5f5f1/lib/_tls_wrap.js#L325) 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 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 commented 2 months ago

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 commented 2 months ago

@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 commented 2 months ago

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!

nodejs / node