From 7485ad817a5801c479b26a2deafdfee6ae1cc5b0 Mon Sep 17 00:00:00 2001 From: Eric Bickle Date: Mon, 5 Sep 2022 09:16:50 -0700 Subject: [PATCH] crypto: include NODE_EXTRA_CA_CERTS in all secure contexts by default Store loaded NODE_EXTRA_CA_CERTS into root_certs_vector, allowing them to be added to secure contexts when NewRootCertStore() is called, rather than losing them when unrelated options are provided. When NODE_EXTRA_CA_CERTS is specified, the root certificates (both bundled and extra) 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 omitted. The original reason NODE_EXTRA_CA_CERTS were loaded at startup (issues #20432, #20434) was to prevent the environment variable from being changed at runtime. This change preserves the runtime consistency without actually having to load the certs at startup. Fixes: https://github.com/nodejs/node/issues/32010 Refs: https://github.com/nodejs/node/issues/40524 Refs: https://github.com/nodejs/node/pull/23354 PR-URL: https://github.com/nodejs/node/pull/44529 Reviewed-By: Tim Perry --- src/crypto/crypto_context.cc | 130 ++++++++---------- src/crypto/crypto_context.h | 3 - .../test-tls-env-extra-ca-file-load.js | 43 ------ .../test-tls-env-extra-ca-with-options.js | 82 +++++++++++ 4 files changed, 141 insertions(+), 117 deletions(-) delete mode 100644 test/parallel/test-tls-env-extra-ca-file-load.js create mode 100644 test/parallel/test-tls-env-extra-ca-with-options.js diff --git a/src/crypto/crypto_context.cc b/src/crypto/crypto_context.cc index 22443405aca..7b8fa8a589a 100644 --- a/src/crypto/crypto_context.cc +++ b/src/crypto/crypto_context.cc @@ -52,7 +52,7 @@ static const char* const root_certs[] = { static const char system_cert_path[] = NODE_OPENSSL_SYSTEM_CERT_PATH; -static bool extra_root_certs_loaded = false; +static std::string extra_root_certs_file; // NOLINT(runtime/string) X509_STORE* GetOrCreateRootCertStore() { // Guaranteed thread-safe by standard, just don't use -fno-threadsafe-statics. @@ -197,26 +197,66 @@ int SSL_CTX_use_certificate_chain(SSL_CTX* ctx, issuer); } +unsigned long LoadCertsFromFile( // NOLINT(runtime/int) + std::vector* certs, + const char* file) { + MarkPopErrorOnReturn mark_pop_error_on_return; + + BIOPointer bio(BIO_new_file(file, "r")); + if (!bio) return ERR_get_error(); + + while (X509* x509 = PEM_read_bio_X509( + bio.get(), nullptr, NoPasswordCallback, nullptr)) { + certs->push_back(x509); + } + + unsigned long err = ERR_peek_last_error(); // NOLINT(runtime/int) + // Ignore error if its EOF/no start line found. + if (ERR_GET_LIB(err) == ERR_LIB_PEM && + ERR_GET_REASON(err) == PEM_R_NO_START_LINE) { + return 0; + } else { + return err; + } +} + X509_STORE* NewRootCertStore() { static std::vector root_certs_vector; + static bool root_certs_vector_loaded = false; static Mutex root_certs_vector_mutex; Mutex::ScopedLock lock(root_certs_vector_mutex); - if (root_certs_vector.empty() && - per_process::cli_options->ssl_openssl_cert_store == false) { - for (size_t i = 0; i < arraysize(root_certs); i++) { - X509* x509 = - PEM_read_bio_X509(NodeBIO::NewFixed(root_certs[i], - strlen(root_certs[i])).get(), - nullptr, // no re-use of X509 structure - NoPasswordCallback, - nullptr); // no callback data + if (!root_certs_vector_loaded) { + if (per_process::cli_options->ssl_openssl_cert_store == false) { + for (size_t i = 0; i < arraysize(root_certs); i++) { + X509* x509 = PEM_read_bio_X509( + NodeBIO::NewFixed(root_certs[i], strlen(root_certs[i])).get(), + nullptr, // no re-use of X509 structure + NoPasswordCallback, + nullptr); // no callback data - // Parse errors from the built-in roots are fatal. - CHECK_NOT_NULL(x509); + // Parse errors from the built-in roots are fatal. + CHECK_NOT_NULL(x509); - root_certs_vector.push_back(x509); + root_certs_vector.push_back(x509); + } } + + if (!extra_root_certs_file.empty()) { + unsigned long err = LoadCertsFromFile( // NOLINT(runtime/int) + &root_certs_vector, + extra_root_certs_file.c_str()); + if (err) { + char buf[256]; + ERR_error_string_n(err, buf, sizeof(buf)); + fprintf(stderr, + "Warning: Ignoring extra certs from `%s`, load failed: %s\n", + extra_root_certs_file.c_str(), + buf); + } + } + + root_certs_vector_loaded = true; } X509_STORE* store = X509_STORE_new(); @@ -228,12 +268,11 @@ X509_STORE* NewRootCertStore() { Mutex::ScopedLock cli_lock(node::per_process::cli_options_mutex); 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); - } + CHECK_EQ(1, X509_STORE_set_default_paths(store)); + } + + for (X509* cert : root_certs_vector) { + CHECK_EQ(1, X509_STORE_add_cert(store, cert)); } return store; @@ -339,11 +378,6 @@ void SecureContext::Initialize(Environment* env, Local target) { SetMethodNoSideEffect( context, target, "getRootCertificates", GetRootCertificates); - // Exposed for testing purposes only. - SetMethodNoSideEffect(context, - target, - "isExtraRootCertsFileLoaded", - IsExtraRootCertsFileLoaded); } void SecureContext::RegisterExternalReferences( @@ -383,7 +417,6 @@ void SecureContext::RegisterExternalReferences( registry->Register(CtxGetter); registry->Register(GetRootCertificates); - registry->Register(IsExtraRootCertsFileLoaded); } SecureContext* SecureContext::Create(Environment* env) { @@ -1383,54 +1416,9 @@ void SecureContext::GetCertificate(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(buff); } -namespace { -unsigned long AddCertsFromFile( // NOLINT(runtime/int) - X509_STORE* store, - const char* file) { - ERR_clear_error(); - MarkPopErrorOnReturn mark_pop_error_on_return; - - BIOPointer bio(BIO_new_file(file, "r")); - if (!bio) - return ERR_get_error(); - - while (X509Pointer x509 = X509Pointer(PEM_read_bio_X509( - bio.get(), nullptr, NoPasswordCallback, nullptr))) { - X509_STORE_add_cert(store, x509.get()); - } - - unsigned long err = ERR_peek_error(); // NOLINT(runtime/int) - // Ignore error if its EOF/no start line found. - if (ERR_GET_LIB(err) == ERR_LIB_PEM && - ERR_GET_REASON(err) == PEM_R_NO_START_LINE) { - return 0; - } - - return err; -} -} // namespace - // UseExtraCaCerts is called only once at the start of the Node.js process. void UseExtraCaCerts(const std::string& file) { - if (file.empty()) return; - ClearErrorOnReturn clear_error_on_return; - X509_STORE* store = GetOrCreateRootCertStore(); - if (auto err = AddCertsFromFile(store, file.c_str())) { - char buf[256]; - ERR_error_string_n(err, buf, sizeof(buf)); - fprintf(stderr, - "Warning: Ignoring extra certs from `%s`, load failed: %s\n", - file.c_str(), - buf); - } else { - extra_root_certs_loaded = true; - } -} - -// Exposed to JavaScript strictly for testing purposes. -void IsExtraRootCertsFileLoaded( - const FunctionCallbackInfo& args) { - return args.GetReturnValue().Set(extra_root_certs_loaded); + extra_root_certs_file = file; } } // namespace crypto diff --git a/src/crypto/crypto_context.h b/src/crypto/crypto_context.h index f646433bdd2..00d4c3738cb 100644 --- a/src/crypto/crypto_context.h +++ b/src/crypto/crypto_context.h @@ -19,9 +19,6 @@ constexpr int kMaxSupportedVersion = TLS1_3_VERSION; void GetRootCertificates( const v8::FunctionCallbackInfo& args); -void IsExtraRootCertsFileLoaded( - const v8::FunctionCallbackInfo& args); - X509_STORE* NewRootCertStore(); X509_STORE* GetOrCreateRootCertStore(); diff --git a/test/parallel/test-tls-env-extra-ca-file-load.js b/test/parallel/test-tls-env-extra-ca-file-load.js deleted file mode 100644 index c66499a55d0..00000000000 --- a/test/parallel/test-tls-env-extra-ca-file-load.js +++ /dev/null @@ -1,43 +0,0 @@ -'use strict'; -// Flags: --expose-internals - -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 { internalBinding } = require('internal/test/binding'); -const binding = internalBinding('crypto'); - -const { fork } = require('child_process'); - -// This test ensures that extra certificates are loaded at startup. -if (process.argv[2] !== 'child') { - // Parent - const NODE_EXTRA_CA_CERTS = fixtures.path('keys', 'ca1-cert.pem'); - const extendsEnv = (obj) => ({ ...process.env, ...obj }); - - // Remove any pre-existing extra CA certs. - delete process.env.NODE_EXTRA_CA_CERTS; - [ - extendsEnv({ CHILD_USE_EXTRA_CA_CERTS: 'yes', NODE_EXTRA_CA_CERTS }), - extendsEnv({ CHILD_USE_EXTRA_CA_CERTS: 'no' }), - ].forEach((processEnv) => { - fork(__filename, ['child'], { env: processEnv }) - .on('exit', common.mustCall((status) => { - // Client did not succeed in connecting - assert.strictEqual(status, 0); - })); - }); -} else if (process.env.CHILD_USE_EXTRA_CA_CERTS === 'yes') { - // Child with extra certificates loaded at startup. - assert.strictEqual(binding.isExtraRootCertsFileLoaded(), true); -} else { - // Child without extra certificates. - assert.strictEqual(binding.isExtraRootCertsFileLoaded(), false); - tls.createServer({}); - assert.strictEqual(binding.isExtraRootCertsFileLoaded(), false); -} diff --git a/test/parallel/test-tls-env-extra-ca-with-options.js b/test/parallel/test-tls-env-extra-ca-with-options.js new file mode 100644 index 00000000000..8f04decf670 --- /dev/null +++ b/test/parallel/test-tls-env-extra-ca-with-options.js @@ -0,0 +1,82 @@ +'use strict'; + +const common = require('../common'); + +if (!common.hasCrypto) + common.skip('missing crypto'); + +const assert = require('node:assert'); +const tls = require('node:tls'); +const { fork } = require('node:child_process'); +const fixtures = require('../common/fixtures'); + +const tests = [ + { + get clientOptions() { + const secureContext = tls.createSecureContext(); + secureContext.context.addCACert( + fixtures.readKey('ca1-cert.pem') + ); + + return { + secureContext + }; + } + }, + { + clientOptions: { + crl: fixtures.readKey('ca2-crl.pem') + } + }, + { + clientOptions: { + pfx: fixtures.readKey('agent1.pfx'), + passphrase: 'sample' + } + }, +]; + +if (process.argv[2]) { + const testNumber = parseInt(process.argv[2], 10); + assert(testNumber >= 0 && testNumber < tests.length); + + const test = tests[testNumber]; + + const clientOptions = { + ...test.clientOptions, + port: process.argv[3], + checkServerIdentity: common.mustCall() + }; + + const client = tls.connect(clientOptions, common.mustCall(() => { + client.end('hi'); + })); +} else { + const serverOptions = { + key: fixtures.readKey('agent3-key.pem'), + cert: fixtures.readKey('agent3-cert.pem') + }; + + for (const testNumber in tests) { + const server = tls.createServer(serverOptions, common.mustCall((socket) => { + socket.end('bye'); + server.close(); + })); + + server.listen(0, common.mustCall(() => { + const env = { + ...process.env, + NODE_EXTRA_CA_CERTS: fixtures.path('keys', 'ca2-cert.pem') + }; + + const args = [ + testNumber, + server.address().port, + ]; + + fork(__filename, args, { env }).on('exit', common.mustCall((status) => { + assert.strictEqual(status, 0); + })); + })); + } +}