diff --git a/src/node_crypto.cc b/src/node_crypto.cc index e136903989ec51..239907c91bb3de 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -112,6 +112,8 @@ static const char* const root_certs[] = { 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 extra_root_certs_loaded = false; @@ -968,9 +970,7 @@ void SecureContext::SetCert(const FunctionCallbackInfo& args) { } -static X509_STORE* NewRootCertStore() { - static std::vector root_certs_vector; - static Mutex root_certs_vector_mutex; +static void EnsureRootCerts() { Mutex::ScopedLock lock(root_certs_vector_mutex); if (root_certs_vector.empty()) { @@ -988,6 +988,11 @@ static X509_STORE* NewRootCertStore() { root_certs_vector.push_back(x509); } } +} + + +static X509_STORE* NewRootCertStore() { + EnsureRootCerts(); X509_STORE* store = X509_STORE_new(); if (*system_cert_path != '\0') { @@ -996,8 +1001,8 @@ static X509_STORE* NewRootCertStore() { if (per_process::cli_options->ssl_openssl_cert_store) { X509_STORE_set_default_paths(store); } else { + Mutex::ScopedLock lock(root_certs_vector_mutex); for (X509* cert : root_certs_vector) { - X509_up_ref(cert); X509_STORE_add_cert(store, cert); } } @@ -1069,8 +1074,7 @@ void SecureContext::AddCRL(const FunctionCallbackInfo& args) { } -static unsigned long AddCertsFromFile( // NOLINT(runtime/int) - X509_STORE* store, +static unsigned long AddRootCertsFromFile( // NOLINT(runtime/int) const char* file) { ERR_clear_error(); MarkPopErrorOnReturn mark_pop_error_on_return; @@ -1079,10 +1083,14 @@ static 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); + // Scope for root_certs_vector lock + { + Mutex::ScopedLock lock(root_certs_vector_mutex); + while (X509* x509 = + PEM_read_bio_X509(bio.get(), nullptr, NoPasswordCallback, nullptr)) { + X509_STORE_add_cert(root_cert_store, x509); + root_certs_vector.push_back(x509); + } } unsigned long err = ERR_peek_error(); // NOLINT(runtime/int) @@ -1103,8 +1111,7 @@ void UseExtraCaCerts(const std::string& file) { root_cert_store = NewRootCertStore(); if (!file.empty()) { - unsigned long err = AddCertsFromFile( // NOLINT(runtime/int) - root_cert_store, + unsigned long err = AddRootCertsFromFile( // NOLINT(runtime/int) file.c_str()); if (err) { fprintf(stderr, @@ -6629,20 +6636,19 @@ void ExportChallenge(const FunctionCallbackInfo& args) { void GetRootCertificates(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - if (root_cert_store == nullptr) - root_cert_store = NewRootCertStore(); + ClearErrorOnReturn clear_error_on_return; - stack_st_X509_OBJECT* objs = X509_STORE_get0_objects(root_cert_store); - int num_objs = sk_X509_OBJECT_num(objs); + EnsureRootCerts(); std::vector> result; - result.reserve(num_objs); - for (int i = 0; i < num_objs; i++) { - X509_OBJECT* obj = sk_X509_OBJECT_value(objs, i); - if (X509_OBJECT_get_type(obj) == X509_LU_X509) { - X509* cert = X509_OBJECT_get0_X509(obj); + // Scope for root_certs_vector lock + { + Mutex::ScopedLock lock(root_certs_vector_mutex); + + result.reserve(root_certs_vector.size()); + for (X509* cert : root_certs_vector) { Local value; if (!X509ToPEM(env, cert).ToLocal(&value)) return; 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..30f2d447c5c1a0 --- /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); + })); +})); 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..6b8990ee8e097b --- /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); + })); +})); 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..11fdf98d1ccae8 --- /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); + })); +}));