src: fix off-thread cert loading in bundled cert mode

https://redirect.github.com/nodejs/node/pull/59856 had an typo/mistake
in the skip conditions so that it is skipping when --use-openssl-ca
or --openssl-system-ca-path (configure time) are NOT used, even
though they should be skipped only when those ARE used (which is
not the default for default builds). This change fixes that so
that the perf numbers in that PR is true for the default build.

PR-URL: #60764
Reviewed-By: Aditi Singh <aditisingh1400@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

Author: joyeecheung

src/crypto/crypto_context.cc

@@ -901,7 +901,7 @@ void StartLoadingCertificatesOffThread(
   // loading.
   {
     Mutex::ScopedLock cli_lock(node::per_process::cli_options_mutex);
-    if (!per_process::cli_options->ssl_openssl_cert_store) {
+    if (per_process::cli_options->ssl_openssl_cert_store) {
       return;
     }
   }

test/common/tls.js

@@ -194,6 +194,7 @@ function extractMetadata(cert) {
     subject: x509.subject,
   };
 }
+exports.extractMetadata = extractMetadata;
 
 // To compare two certificates, we can just compare serialNumber, issuer,
 // and subject like X509_comp(). We can't just compare two strings because
@@ -219,3 +220,12 @@ exports.includesCert = function includesCert(certs, cert) {
 };
 
 exports.TestTLSSocket = TestTLSSocket;
+
+// Dumps certs into a file to pass safely into test/fixtures/list-certs.js
+exports.writeCerts = function writeCerts(certs, filename) {
+  const fs = require('fs');
+  for (const cert of certs) {
+    const x509 = new crypto.X509Certificate(cert);
+    fs.appendFileSync(filename, x509.toString());
+  }
+};

test/fixtures/list-certs.js

@@ -0,0 +1,19 @@
+const assert = require('assert');
+const EXPECTED_CERTS_PATH = process.env.EXPECTED_CERTS_PATH;
+let expectedCerts = [];
+if (EXPECTED_CERTS_PATH) {
+  const fs = require('fs');
+  const file = fs.readFileSync(EXPECTED_CERTS_PATH, 'utf-8');
+  const expectedCerts = file.split('-----END CERTIFICATE-----\n')
+    .filter(line => line.trim() !== '')
+    .map(line => line + '-----END CERTIFICATE-----\n');
+}
+
+const tls = require('tls');
+const { includesCert, extractMetadata } = require('../common/tls');
+
+const CERTS_TYPE = process.env.CERTS_TYPE || 'default';
+const actualCerts = tls.getCACertificates(CERTS_TYPE);
+for (const cert of expectedCerts) {
+  assert(includesCert(actualCerts, cert), 'Expected certificate not found: ' + JSON.stringify(extractMetadata(cert)));
+}

test/parallel/test-tls-off-thread-cert-loading-disabled.js

@@ -0,0 +1,40 @@
+'use strict';
+// This tests that when --use-openssl-ca is specified, no off-thread cert loading happens.
+
+const common = require('../common');
+if (!common.hasCrypto) {
+  common.skip('missing crypto');
+}
+const { spawnSyncAndAssert } = require('../common/child_process');
+const fixtures = require('../common/fixtures');
+const assert = require('assert');
+
+spawnSyncAndAssert(
+  process.execPath,
+  [ '--use-openssl-ca', fixtures.path('list-certs.js') ],
+  {
+    env: {
+      ...process.env,
+      NODE_DEBUG_NATIVE: 'crypto',
+      NODE_EXTRA_CA_CERTS: fixtures.path('keys', 'fake-startcom-root-cert.pem'),
+      CERTS_TYPE: 'default',
+    }
+  },
+  {
+    stderr(output) {
+      assert.doesNotMatch(
+        output,
+        /Started loading bundled root certificates off-thread/
+      );
+      assert.doesNotMatch(
+        output,
+        /Started loading extra root certificates off-thread/
+      );
+      assert.doesNotMatch(
+        output,
+        /Started loading system root certificates off-thread/
+      );
+      return true;
+    }
+  }
+);

test/parallel/test-tls-off-thread-cert-loading-system.js

@@ -0,0 +1,56 @@
+'use strict';
+
+// This test verifies that system root certificates loading is loaded off-thread if
+// --use-system-ca is used.
+
+const common = require('../common');
+if (!common.hasCrypto) {
+  common.skip('missing crypto');
+}
+const tmpdir = require('../common/tmpdir');
+const { spawnSyncAndAssert } = require('../common/child_process');
+const fixtures = require('../common/fixtures');
+const assert = require('assert');
+const { writeCerts } = require('../common/tls');
+const tls = require('tls');
+
+tmpdir.refresh();
+writeCerts([
+  // Check that the extra cert is loaded.
+  fixtures.readKey('fake-startcom-root-cert.pem'),
+  // Check that the system certs are loaded.
+  ...tls.getCACertificates('system'),
+  // Check that the bundled certs are loaded.
+  ...tls.getCACertificates('bundled'),
+], tmpdir.resolve('check-cert.pem'));
+
+spawnSyncAndAssert(
+  process.execPath,
+  [ '--use-system-ca', '--use-bundled-ca', fixtures.path('list-certs.js') ],
+  {
+    env: {
+      ...process.env,
+      NODE_DEBUG_NATIVE: 'crypto',
+      NODE_EXTRA_CA_CERTS: fixtures.path('keys', 'fake-startcom-root-cert.pem'),
+      EXPECTED_CERTS_PATH: tmpdir.resolve('check-cert.pem'),
+      CERTS_TYPE: 'default',
+    }
+  },
+  {
+    stderr(output) {
+      assert.match(
+        output,
+        /Started loading bundled root certificates off-thread/
+      );
+      assert.match(
+        output,
+        /Started loading extra root certificates off-thread/
+      );
+      assert.match(
+        output,
+        /Started loading system root certificates off-thread/
+      );
+      return true;
+    }
+  }
+);

test/parallel/test-tls-off-thread-cert-loading.js

@@ -0,0 +1,54 @@
+'use strict';
+
+// This test verifies that when --use-bundled-ca is used (default to true in default builds),
+// the loading of extra and bundled root certificates happens off the main thread.
+
+const common = require('../common');
+if (!common.hasCrypto) {
+  common.skip('missing crypto');
+}
+const tmpdir = require('../common/tmpdir');
+const { spawnSyncAndAssert } = require('../common/child_process');
+const fixtures = require('../common/fixtures');
+const assert = require('assert');
+const { writeCerts } = require('../common/tls');
+const tls = require('tls');
+
+tmpdir.refresh();
+writeCerts([
+  // Check that the extra cert is loaded.
+  fixtures.readKey('fake-startcom-root-cert.pem'),
+  // Check that the bundled certs are loaded.
+  ...tls.getCACertificates('bundled'),
+], tmpdir.resolve('check-cert.pem'));
+
+spawnSyncAndAssert(
+  process.execPath,
+  [ '--no-use-system-ca', '--use-bundled-ca', fixtures.path('list-certs.js') ],
+  {
+    env: {
+      ...process.env,
+      NODE_DEBUG_NATIVE: 'crypto',
+      NODE_EXTRA_CA_CERTS: fixtures.path('keys', 'fake-startcom-root-cert.pem'),
+      EXPECTED_CERTS_PATH: tmpdir.resolve('check-cert.pem'),
+      CERTS_TYPE: 'default',
+    }
+  },
+  {
+    stderr(output) {
+      assert.match(
+        output,
+        /Started loading bundled root certificates off-thread/
+      );
+      assert.match(
+        output,
+        /Started loading extra root certificates off-thread/
+      );
+      assert.doesNotMatch(
+        output,
+        /Started loading system root certificates off-thread/
+      );
+      return true;
+    }
+  }
+);