From 31a6a83e3ba23324ee6e427d30ab78c2531cac6d Mon Sep 17 00:00:00 2001 From: Eric Bickle Date: Thu, 25 Aug 2022 14:20:29 -0700 Subject: [PATCH] src: Add NODE_EXTRA_CA_CERTS to modified cert stores Store loaded NODE_EXTRA_CA_CERTS into root_certs_vector, allowing them to be added to secure contexts when NewRootCertStore() is called. When NODE_EXTRA_CA_CERTS is specified, the bundled root certificates will no longer be preloaded at startup. This improves Node.js startup time and makes the behavior of NODE_EXTRA_CA_CERTS consistent with the default behavior when NODE_EXTRA_CA_CERTS is ommitted. Fixes: https://github.com/nodejs/node/issues/32010 Refs: https://github.com/nodejs/node/issues/40524 --- src/crypto/crypto_context.cc | 59 +++++++++---------- .../parallel/test-tls-env-extra-ca-with-ca.js | 55 +++++++++++++++++ .../test-tls-env-extra-ca-with-crl.js | 47 +++++++++++++++ .../test-tls-env-extra-ca-with-pfx.js | 48 +++++++++++++++ 4 files changed, 178 insertions(+), 31 deletions(-) create mode 100644 test/parallel/test-tls-env-extra-ca-with-ca.js create mode 100644 test/parallel/test-tls-env-extra-ca-with-crl.js create mode 100644 test/parallel/test-tls-env-extra-ca-with-pfx.js diff --git a/src/crypto/crypto_context.cc b/src/crypto/crypto_context.cc index 7eab9de37cb3a1..68beeb4899eb3b 100644 --- a/src/crypto/crypto_context.cc +++ b/src/crypto/crypto_context.cc @@ -49,6 +49,10 @@ static const char system_cert_path[] = NODE_OPENSSL_SYSTEM_CERT_PATH; static X509_STORE* root_cert_store; +static std::vector root_certs_vector; +static Mutex root_certs_vector_mutex; +static bool root_certs_loaded = false; + static bool extra_root_certs_loaded = false; // Takes a string or buffer and loads it into a BIO. @@ -191,25 +195,25 @@ int SSL_CTX_use_certificate_chain(SSL_CTX* ctx, } // namespace X509_STORE* NewRootCertStore() { - static std::vector root_certs_vector; - static Mutex root_certs_vector_mutex; Mutex::ScopedLock lock(root_certs_vector_mutex); - if (root_certs_vector.empty() && + if (!root_certs_loaded && per_process::cli_options->ssl_openssl_cert_store == false) { for (size_t i = 0; i < arraysize(root_certs); i++) { - X509* x509 = + X509Pointer x509 = X509Pointer( PEM_read_bio_X509(NodeBIO::NewFixed(root_certs[i], strlen(root_certs[i])).get(), - nullptr, // no re-use of X509 structure + nullptr, // no re-use of X509 structure NoPasswordCallback, - nullptr); // no callback data + nullptr)); // no callback data // Parse errors from the built-in roots are fatal. CHECK_NOT_NULL(x509); - root_certs_vector.push_back(x509); + root_certs_vector.push_back(std::move(x509)); } + + root_certs_loaded = true; } X509_STORE* store = X509_STORE_new(); @@ -223,10 +227,8 @@ X509_STORE* NewRootCertStore() { if (per_process::cli_options->ssl_openssl_cert_store) { X509_STORE_set_default_paths(store); } else { - for (X509* cert : root_certs_vector) { - X509_up_ref(cert); - X509_STORE_add_cert(store, cert); - } + for (X509Pointer& cert : root_certs_vector) + X509_STORE_add_cert(store, cert.get()); } return store; @@ -1299,7 +1301,7 @@ void SecureContext::GetCertificate(const FunctionCallbackInfo& args) { namespace { unsigned long AddCertsFromFile( // NOLINT(runtime/int) - X509_STORE* store, + std::vector& certs, const char* file) { ERR_clear_error(); MarkPopErrorOnReturn mark_pop_error_on_return; @@ -1308,10 +1310,9 @@ unsigned long AddCertsFromFile( // NOLINT(runtime/int) if (!bio) return ERR_get_error(); - while (X509* x509 = - PEM_read_bio_X509(bio.get(), nullptr, NoPasswordCallback, nullptr)) { - X509_STORE_add_cert(store, x509); - X509_free(x509); + while (X509Pointer x509 = X509Pointer( + PEM_read_bio_X509(bio.get(), nullptr, NoPasswordCallback, nullptr))) { + certs.push_back(std::move(x509)); } unsigned long err = ERR_peek_error(); // NOLINT(runtime/int) @@ -1329,21 +1330,17 @@ unsigned long AddCertsFromFile( // NOLINT(runtime/int) void UseExtraCaCerts(const std::string& file) { ClearErrorOnReturn clear_error_on_return; - if (root_cert_store == nullptr) { - root_cert_store = NewRootCertStore(); - - if (!file.empty()) { - unsigned long err = AddCertsFromFile( // NOLINT(runtime/int) - root_cert_store, - file.c_str()); - if (err) { - fprintf(stderr, - "Warning: Ignoring extra certs from `%s`, load failed: %s\n", - file.c_str(), - ERR_error_string(err, nullptr)); - } else { - extra_root_certs_loaded = true; - } + if (!file.empty()) { + unsigned long err = AddCertsFromFile( // NOLINT(runtime/int) + root_certs_vector, + file.c_str()); + if (err) { + fprintf(stderr, + "Warning: Ignoring extra certs from `%s`, load failed: %s\n", + file.c_str(), + ERR_error_string(err, nullptr)); + } else { + extra_root_certs_loaded = true; } } } diff --git a/test/parallel/test-tls-env-extra-ca-with-ca.js b/test/parallel/test-tls-env-extra-ca-with-ca.js new file mode 100644 index 00000000000000..b1f7b10ff8a3dd --- /dev/null +++ b/test/parallel/test-tls-env-extra-ca-with-ca.js @@ -0,0 +1,55 @@ +'use strict'; + +const common = require('../common'); + +if (!common.hasCrypto) + common.skip('missing crypto'); + +const assert = require('assert'); +const tls = require('tls'); +const fixtures = require('../common/fixtures'); + +const { fork } = require('child_process'); + +if (process.env.CHILD) { + const copts = { + port: process.env.PORT, + checkServerIdentity: common.mustCall() + }; + + // New secure contexts have the well-known root CAs. + copts.secureContext = tls.createSecureContext(); + + // Explicit calls to addCACert() add to the root certificates, + // instead of replacing, so connection still succeeds. + copts.secureContext.context.addCACert( + fixtures.readKey('ca1-cert.pem') + ); + + const client = tls.connect(copts, common.mustCall(() => { + client.end('hi'); + })); + + return; +} + +const options = { + key: fixtures.readKey('agent3-key.pem'), + cert: fixtures.readKey('agent3-cert.pem') +}; + +const server = tls.createServer(options, common.mustCall((socket) => { + socket.end('bye'); + server.close(); +})).listen(0, common.mustCall(() => { + const env = Object.assign({}, process.env, { + CHILD: 'yes', + PORT: server.address().port, + NODE_EXTRA_CA_CERTS: fixtures.path('keys', 'ca2-cert.pem') + }); + + fork(__filename, { env }).on('exit', common.mustCall((status) => { + // Client did not succeed in connecting + assert.strictEqual(status, 0); + })); +})); \ No newline at end of file diff --git a/test/parallel/test-tls-env-extra-ca-with-crl.js b/test/parallel/test-tls-env-extra-ca-with-crl.js new file mode 100644 index 00000000000000..e4d6c7fb3fe385 --- /dev/null +++ b/test/parallel/test-tls-env-extra-ca-with-crl.js @@ -0,0 +1,47 @@ +'use strict'; + +const common = require('../common'); + +if (!common.hasCrypto) + common.skip('missing crypto'); + +const assert = require('assert'); +const tls = require('tls'); +const fixtures = require('../common/fixtures'); + +const { fork } = require('child_process'); + +if (process.env.CHILD) { + const copts = { + port: process.env.PORT, + checkServerIdentity: common.mustCall(), + crl: fixtures.readKey('ca2-crl.pem') + }; + + const client = tls.connect(copts, common.mustCall(() => { + client.end('hi'); + })); + + return; +} + +const options = { + key: fixtures.readKey('agent3-key.pem'), + cert: fixtures.readKey('agent3-cert.pem') +}; + +const server = tls.createServer(options, common.mustCall((socket) => { + socket.end('bye'); + server.close(); +})).listen(0, common.mustCall(() => { + const env = Object.assign({}, process.env, { + CHILD: 'yes', + PORT: server.address().port, + NODE_EXTRA_CA_CERTS: fixtures.path('keys', 'ca2-cert.pem') + }); + + fork(__filename, { env }).on('exit', common.mustCall((status) => { + // Client did not succeed in connecting + assert.strictEqual(status, 0); + })); +})); \ No newline at end of file diff --git a/test/parallel/test-tls-env-extra-ca-with-pfx.js b/test/parallel/test-tls-env-extra-ca-with-pfx.js new file mode 100644 index 00000000000000..12ee21a88878e5 --- /dev/null +++ b/test/parallel/test-tls-env-extra-ca-with-pfx.js @@ -0,0 +1,48 @@ +'use strict'; + +const common = require('../common'); + +if (!common.hasCrypto) + common.skip('missing crypto'); + +const assert = require('assert'); +const tls = require('tls'); +const fixtures = require('../common/fixtures'); + +const { fork } = require('child_process'); + +if (process.env.CHILD) { + const copts = { + port: process.env.PORT, + checkServerIdentity: common.mustCall(), + pfx: fixtures.readKey('agent1.pfx'), + passphrase: 'sample' + }; + + const client = tls.connect(copts, common.mustCall(() => { + client.end('hi'); + })); + + return; +} + +const options = { + key: fixtures.readKey('agent3-key.pem'), + cert: fixtures.readKey('agent3-cert.pem') +}; + +const server = tls.createServer(options, common.mustCall((socket) => { + socket.end('bye'); + server.close(); +})).listen(0, common.mustCall(() => { + const env = Object.assign({}, process.env, { + CHILD: 'yes', + PORT: server.address().port, + NODE_EXTRA_CA_CERTS: fixtures.path('keys', 'ca2-cert.pem') + }); + + fork(__filename, { env }).on('exit', common.mustCall((status) => { + // Client did not succeed in connecting + assert.strictEqual(status, 0); + })); +})); \ No newline at end of file