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: #32010 Refs: #40524 Refs: #23354 PR-URL: #44529 Reviewed-By: Tim Perry <pimterry@gmail.com>

Author: ebickle

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<X509*>* 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<X509*> 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<Object> 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<Value>& 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<Value>& args) {
- return args.GetReturnValue().Set(extra_root_certs_loaded);
+ extra_root_certs_file = file;
 }
 
 } // namespace crypto

src/crypto/crypto_context.h

@@ -19,9 +19,6 @@ constexpr int kMaxSupportedVersion = TLS1_3_VERSION;
 void GetRootCertificates(
  const v8::FunctionCallbackInfo<v8::Value>& args);
 
-void IsExtraRootCertsFileLoaded(
- const v8::FunctionCallbackInfo<v8::Value>& args);
-
 X509_STORE* NewRootCertStore();
 
 X509_STORE* GetOrCreateRootCertStore();

test/parallel/test-tls-env-extra-ca-file-load.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);
+ }));
+ }));
+ }
+}