From cae0df46e84532a51cc949e2c7da277ac981840a Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Tue, 18 Nov 2025 13:11:51 +0900 Subject: [PATCH] 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. --- src/crypto/crypto_context.cc | 2 +- test/common/tls.js | 10 ++++ test/fixtures/list-certs.js | 19 +++++++ ...st-tls-off-thread-cert-loading-disabled.js | 40 +++++++++++++ ...test-tls-off-thread-cert-loading-system.js | 56 +++++++++++++++++++ .../test-tls-off-thread-cert-loading.js | 54 ++++++++++++++++++ 6 files changed, 180 insertions(+), 1 deletion(-) create mode 100644 test/fixtures/list-certs.js create mode 100644 test/parallel/test-tls-off-thread-cert-loading-disabled.js create mode 100644 test/parallel/test-tls-off-thread-cert-loading-system.js create mode 100644 test/parallel/test-tls-off-thread-cert-loading.js diff --git a/src/crypto/crypto_context.cc b/src/crypto/crypto_context.cc index 0e0546c313fe94..17d4e988adbdb5 100644 --- a/src/crypto/crypto_context.cc +++ b/src/crypto/crypto_context.cc @@ -878,7 +878,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; } } diff --git a/test/common/tls.js b/test/common/tls.js index e59255191b4290..8568659d13cc33 100644 --- a/test/common/tls.js +++ b/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()); + } +}; diff --git a/test/fixtures/list-certs.js b/test/fixtures/list-certs.js new file mode 100644 index 00000000000000..7c9538df242115 --- /dev/null +++ b/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))); +} diff --git a/test/parallel/test-tls-off-thread-cert-loading-disabled.js b/test/parallel/test-tls-off-thread-cert-loading-disabled.js new file mode 100644 index 00000000000000..c2e466fcae1db5 --- /dev/null +++ b/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; + } + } +); diff --git a/test/parallel/test-tls-off-thread-cert-loading-system.js b/test/parallel/test-tls-off-thread-cert-loading-system.js new file mode 100644 index 00000000000000..b2893a3a501276 --- /dev/null +++ b/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; + } + } +); diff --git a/test/parallel/test-tls-off-thread-cert-loading.js b/test/parallel/test-tls-off-thread-cert-loading.js new file mode 100644 index 00000000000000..ce414ddec2e0bd --- /dev/null +++ b/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; + } + } +);