crypto: include NODE_EXTRA_CA_CERTS in all secure contexts by default

ebickle · targos · commit d52f515bab68 · 2024-08-14T08:14:45.000+02:00
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>
diff --git 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<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
diff --git 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<v8::Value>& args);
 
-void IsExtraRootCertsFileLoaded(
-    const v8::FunctionCallbackInfo<v8::Value>& 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
diff --git a/test/parallel/test-tls-env-extra-ca-with-options.js 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);
+      }));
+    }));
+  }
+}
