crypto: add extra CA certs to all secure contexts

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 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 nodejs#20432, nodejs#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: nodejs#32010
Refs: nodejs#40524
Refs: nodejs#23354

src/crypto/crypto_context.cc

@@ -8,6 +8,7 @@
 #include "node.h"
 #include "node_buffer.h"
 #include "node_options.h"
+#include "node_process-inl.h"
 #include "util.h"
 #include "v8.h"
 
@@ -49,7 +50,7 @@ static const char system_cert_path[] = NODE_OPENSSL_SYSTEM_CERT_PATH;
 
 static X509_STORE* root_cert_store;
 
-static bool extra_root_certs_loaded = false;
+static std::string extra_root_certs_file;  // NOLINT(runtime/string)
 
 // Takes a string or buffer and loads it into a BIO.
 // Caller responsible for BIO_free_all-ing the returned object.
@@ -188,28 +189,69 @@ int SSL_CTX_use_certificate_chain(SSL_CTX* ctx,
                                        issuer);
 }
 
+unsigned long LoadCertsFromFile(  // NOLINT(runtime/int)
+    std::vector<X509*>* certs,
+    const char* file) {
+  ClearErrorOnReturn clear_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_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
 
-X509_STORE* NewRootCertStore() {
+X509_STORE* NewRootCertStore(Environment* env) {
   static std::vector<X509*> root_certs_vector;
-  static Mutex root_certs_vector_mutex;
+  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) {
+        ProcessEmitWarning(
+          env,
+          "Ignoring extra certs from `%s`, load failed: %s\n",
+          extra_root_certs_file.c_str(),
+          ERR_error_string(err, nullptr));
+      }
+    }
+
+    root_certs_vector_loaded = true;
   }
 
   X509_STORE* store = X509_STORE_new();
@@ -222,11 +264,10 @@ 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);
-    }
+  }
+
+  for (X509* cert : root_certs_vector) {
+    X509_STORE_add_cert(store, cert);
   }
 
   return store;
@@ -333,11 +374,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(
@@ -377,7 +413,6 @@ void SecureContext::RegisterExternalReferences(
   registry->Register(CtxGetter);
 
   registry->Register(GetRootCertificates);
-  registry->Register(IsExtraRootCertsFileLoaded);
 }
 
 SecureContext* SecureContext::Create(Environment* env) {
@@ -702,7 +737,7 @@ void SecureContext::AddCACert(const FunctionCallbackInfo<Value>& args) {
   while (X509* x509 = PEM_read_bio_X509_AUX(
       bio.get(), nullptr, NoPasswordCallback, nullptr)) {
     if (cert_store == root_cert_store) {
-      cert_store = NewRootCertStore();
+      cert_store = NewRootCertStore(env);
       SSL_CTX_set_cert_store(sc->ctx_.get(), cert_store);
     }
     X509_STORE_add_cert(cert_store, x509);
@@ -733,7 +768,7 @@ void SecureContext::AddCRL(const FunctionCallbackInfo<Value>& args) {
 
   X509_STORE* cert_store = SSL_CTX_get_cert_store(sc->ctx_.get());
   if (cert_store == root_cert_store) {
-    cert_store = NewRootCertStore();
+    cert_store = NewRootCertStore(env);
     SSL_CTX_set_cert_store(sc->ctx_.get(), cert_store);
   }
 
@@ -748,7 +783,8 @@ void SecureContext::AddRootCerts(const FunctionCallbackInfo<Value>& args) {
   ClearErrorOnReturn clear_error_on_return;
 
   if (root_cert_store == nullptr) {
-    root_cert_store = NewRootCertStore();
+    Environment* env = Environment::GetCurrent(args);    
+    root_cert_store = NewRootCertStore(env);
   }
 
   // Increment reference count so global store is not deleted along with CTX.
@@ -1027,7 +1063,7 @@ void SecureContext::LoadPKCS12(const FunctionCallbackInfo<Value>& args) {
       X509* ca = sk_X509_value(extra_certs.get(), i);
 
       if (cert_store == root_cert_store) {
-        cert_store = NewRootCertStore();
+        cert_store = NewRootCertStore(env);
         SSL_CTX_set_cert_store(sc->ctx_.get(), cert_store);
       }
       X509_STORE_add_cert(cert_store, ca);
@@ -1297,61 +1333,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 (X509* x509 =
-      PEM_read_bio_X509(bio.get(), nullptr, NoPasswordCallback, nullptr)) {
-    X509_STORE_add_cert(store, x509);
-    X509_free(x509);
-  }
-
-  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) {
-  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;
-      }
-    }
-  }
-}
-
-// 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

@@ -18,10 +18,7 @@ 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* NewRootCertStore(Environment* env);
 
 BIOPointer LoadBIO(Environment* env, v8::Local<v8::Value> v);
 

test/cctest/test_node_crypto.cc

@@ -6,16 +6,27 @@
 #include "node_options.h"
 #include "openssl/err.h"
 #include "gtest/gtest.h"
+#include "node_test_fixture.h"
+
+class NodeCryptoTest : public EnvironmentTestFixture {};
 
 /*
  * This test verifies that a call to NewRootCertDir with the build time
  * configuration option --openssl-system-ca-path set to an missing file, will
  * not leave any OpenSSL errors on the OpenSSL error stack.
  * See https://github.com/nodejs/node/issues/35456 for details.
  */
-TEST(NodeCrypto, NewRootCertStore) {
+TEST_F(NodeCryptoTest, NewRootCertStore) {
+  const v8::HandleScope handle_scope(isolate_);
+  Argv argv;
+
+  Env test_env{handle_scope, argv};
+
+  node::Environment* env = *test_env;
+  node::LoadEnvironment(env, "");
+
   node::per_process::cli_options->ssl_openssl_cert_store = true;
-  X509_STORE* store = node::crypto::NewRootCertStore();
+  X509_STORE* store = node::crypto::NewRootCertStore(env);
   ASSERT_TRUE(store);
   ASSERT_EQ(ERR_peek_error(), 0UL) << "NewRootCertStore should not have left "
                                       "any errors on the OpenSSL error stack\n";

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);
+  }));
+}));