tls: fix SNICallback certificate selection

src/crypto/crypto_tls.cc

@@ -1612,17 +1612,19 @@ void TLSWrap::CertCbDone(const FunctionCallbackInfo<Value>& args) {
     // Store the SNI context for later use.
     w->sni_context_ = BaseObjectPtr<SecureContext>(sc);
 
-    if (w->ssl_.setSniContext(w->sni_context_->ctx()) && !w->SetCACerts(sc)) {
+    // Replace the complete SSL certificate configuration. Copying only the
+    // certificate and private key leaves credentials for other key types from
+    // the default context in place, allowing OpenSSL to select one of them.
+    if (SSL_set_SSL_CTX(w->ssl_.get(), sc->ctx().get()) == nullptr ||
+        !w->SetCACerts(sc)) {
       // Not clear why sometimes we throw error, and sometimes we call
       // onerror(). Both cause .destroy(), but onerror does a bit more.
       unsigned long err = ERR_get_error();  // NOLINT(runtime/int)
       return ThrowCryptoError(env, err, "CertCbDone");
     }
-    // setSniContext copies the cert via SSL_use_certificate which does not
-    // carry over pre-compressed certificate data (comp_cert). If the new
-    // context has certificate compression configured, set the compression
-    // preferences on this connection and apply the cached compressed cert
-    // data so the server can send CompressedCertificate messages.
+    // Certificate compression preferences are stored on the SSL connection,
+    // rather than the SSL context. Apply the selected context's preferences
+    // after replacing the SSL certificate configuration.
 #ifdef NODE_OPENSSL_HAS_CERT_COMP
     if (sc->HasCertCompression()) {
       SSL_set1_cert_comp_preference(

test/parallel/test-https-snicallback-override.js

@@ -0,0 +1,71 @@
+'use strict';
+
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+
+const assert = require('assert');
+const { X509Certificate } = require('crypto');
+const https = require('https');
+const tls = require('tls');
+const fixtures = require('../common/fixtures');
+
+const defaultCredentials = {
+  cert: fixtures.readKey('ca5-cert.pem'),
+  key: fixtures.readKey('ca5-key.pem'),
+};
+const sniCredentials = {
+  cert: fixtures.readKey('agent1-cert.pem'),
+  key: fixtures.readKey('agent1-key.pem'),
+};
+
+const defaultCertificate = new X509Certificate(defaultCredentials.cert);
+const sniCertificate = new X509Certificate(sniCredentials.cert);
+const sniContext = tls.createSecureContext(sniCredentials);
+
+function request(port, servername, expectedCertificate) {
+  return new Promise((resolve, reject) => {
+    const req = https.get({
+      host: '127.0.0.1',
+      port,
+      servername,
+      rejectUnauthorized: false,
+      agent: false,
+    }, common.mustCall((response) => {
+      try {
+        const certificate = response.socket.getPeerX509Certificate();
+        assert.strictEqual(certificate.fingerprint256,
+                           expectedCertificate.fingerprint256);
+      } catch (err) {
+        reject(err);
+        return;
+      }
+
+      response.resume();
+      response.once('end', resolve);
+      response.once('error', reject);
+    }));
+    req.once('error', reject);
+  });
+}
+
+const server = https.createServer({
+  cert: defaultCredentials.cert,
+  key: defaultCredentials.key,
+  SNICallback: common.mustCall((servername, callback) => {
+    assert.strictEqual(servername, 'agent1.com');
+    callback(null, sniContext);
+  }, 1),
+}, (_request, response) => {
+  response.end('ok');
+});
+
+server.listen(0, common.mustCall(async () => {
+  try {
+    const { port } = server.address();
+    await request(port, undefined, defaultCertificate);
+    await request(port, 'agent1.com', sniCertificate);
+  } finally {
+    server.close(common.mustCall());
+  }
+}));