tls: load NODE_EXTRA_CA_CERTS at startup

`NODE_EXTRA_CA_CERTS` is not intended to be used to set the paths
of extra certificates and this approach to setting is not reliable.
This commit makes node load extra certificates at startup instead
of first use.

Fixes: nodejs#20434
Refs: nodejs#20432

lib/internal/bootstrap/node.js

@@ -23,7 +23,8 @@
                                 _memoryUsage, _rawDebug,
                                 _umask, _initgroups, _setegid, _seteuid,
                                 _setgid, _setuid, _setgroups,
-                                _shouldAbortOnUncaughtToggle },
+                                _shouldAbortOnUncaughtToggle,
+                                _loadExtraRootCertsFile },
                               { internalBinding, NativeModule }) {
   const exceptionHandlerState = { captureFn: null };
   const isMainThread = internalBinding('worker').threadId === 0;
@@ -231,6 +232,11 @@
 
     setupAllowedFlags();
 
+    // `_loadExtraRootCertsFile` is undefined when there is no crypto.
+    if (typeof _loadExtraRootCertsFile === 'function') {
+      _loadExtraRootCertsFile();
+    }
+
     // There are various modes that Node can run in. The most common two
     // are running from a script and running the REPL - but there are a few
     // others like the debugger or running --eval arguments. Here we decide

src/bootstrapper.cc

@@ -3,6 +3,10 @@
 #include "node_internals.h"
 #include "v8.h"
 
+#if HAVE_OPENSSL
+#include "node_crypto.h"
+#endif
+
 #include <atomic>
 
 namespace node {
@@ -153,6 +157,10 @@ void SetupBootstrapObject(Environment* env,
   BOOTSTRAP_METHOD(_rawDebug, RawDebug);
   BOOTSTRAP_METHOD(_umask, Umask);
 
+#if HAVE_OPENSSL
+  BOOTSTRAP_METHOD(_loadExtraRootCertsFile, crypto::LoadExtraRootCertsFile);
+#endif
+
 #if defined(__POSIX__) && !defined(__ANDROID__) && !defined(__CloudABI__)
   if (env->is_main_thread()) {
     BOOTSTRAP_METHOD(_initgroups, InitGroups);

src/node_crypto.cc

@@ -114,10 +114,15 @@ static const char* const root_certs[] = {
 
 static const char system_cert_path[] = NODE_OPENSSL_SYSTEM_CERT_PATH;
 
-static std::string extra_root_certs_file;  // NOLINT(runtime/string)
-
 static X509_STORE* root_cert_store;
 
+struct ExtraRootCertsInfo {
+  std::string file;  // NOLINT(runtime/string)
+  bool is_loaded = false;
+};
+
+static ExtraRootCertsInfo extra_root_certs_info;
+
 // Just to generate static methods
 template void SSLWrap<TLSWrap>::AddMethods(Environment* env,
                                            Local<FunctionTemplate> t);
@@ -831,7 +836,7 @@ void SecureContext::AddCRL(const FunctionCallbackInfo<Value>& args) {
 
 
 void UseExtraCaCerts(const std::string& file) {
-  extra_root_certs_file = file;
+  extra_root_certs_info.file = file;
 }
 
 
@@ -861,29 +866,20 @@ static unsigned long AddCertsFromFile(  // NOLINT(runtime/int)
   return err;
 }
 
+
+static void IsExtraRootCertsFileLoaded(
+    const FunctionCallbackInfo<Value>& args) {
+  return args.GetReturnValue().Set(extra_root_certs_info.is_loaded);
+}
+
+
 void SecureContext::AddRootCerts(const FunctionCallbackInfo<Value>& args) {
   SecureContext* sc;
   ASSIGN_OR_RETURN_UNWRAP(&sc, args.Holder());
   ClearErrorOnReturn clear_error_on_return;
 
-  if (!root_cert_store) {
+  if (root_cert_store == nullptr) {
     root_cert_store = NewRootCertStore();
-
-    if (!extra_root_certs_file.empty()) {
-      unsigned long err = AddCertsFromFile(  // NOLINT(runtime/int)
-                                           root_cert_store,
-                                           extra_root_certs_file.c_str());
-      if (err) {
-        // We do not call back into JS after this line anyway, so ignoring
-        // the return value of ProcessEmitWarning does not affect how a
-        // possible exception would be propagated.
-        ProcessEmitWarning(sc->env(),
-                           "Ignoring extra certs from `%s`, "
-                           "load failed: %s\n",
-                           extra_root_certs_file.c_str(),
-                           ERR_error_string(err, nullptr));
-      }
-    }
   }
 
   // Increment reference count so global store is not deleted along with CTX.
@@ -5667,6 +5663,35 @@ void SetFipsCrypto(const FunctionCallbackInfo<Value>& args) {
 }
 #endif /* NODE_FIPS_MODE */
 
+
+void LoadExtraRootCertsFile(const FunctionCallbackInfo<Value>& args) {
+  ClearErrorOnReturn clear_error_on_return;
+  Environment* env = Environment::GetCurrent(args);
+
+  if (root_cert_store == nullptr) {
+    root_cert_store = NewRootCertStore();
+
+    if (!extra_root_certs_info.file.empty()) {
+      unsigned long err = AddCertsFromFile(  // NOLINT(runtime/int)
+                                           root_cert_store,
+                                           extra_root_certs_info.file.c_str());
+      if (err) {
+        // We do not call back into JS after this line anyway, so ignoring
+        // the return value of ProcessEmitWarning does not affect how a
+        // possible exception would be propagated.
+        ProcessEmitWarning(env,
+                           "Ignoring extra certs from `%s`, "
+                           "load failed: %s\n",
+                           extra_root_certs_info.file.c_str(),
+                           ERR_error_string(err, nullptr));
+      } else {
+        extra_root_certs_info.is_loaded = true;
+      }
+    }
+  }
+}
+
+
 void Initialize(Local<Object> target,
                 Local<Value> unused,
                 Local<Context> context,
@@ -5687,6 +5712,9 @@ void Initialize(Local<Object> target,
   env->SetMethodNoSideEffect(target, "certVerifySpkac", VerifySpkac);
   env->SetMethodNoSideEffect(target, "certExportPublicKey", ExportPublicKey);
   env->SetMethodNoSideEffect(target, "certExportChallenge", ExportChallenge);
+  // Exposed for testing purposes only.
+  env->SetMethodNoSideEffect(target, "isExtraRootCertsFileLoaded",
+                             IsExtraRootCertsFileLoaded);
 
   env->SetMethodNoSideEffect(target, "ECDHConvertKey", ConvertKey);
 #ifndef OPENSSL_NO_ENGINE

src/node_crypto.h

@@ -680,6 +680,7 @@ class ECDH : public BaseObject {
 };
 
 bool EntropySource(unsigned char* buffer, size_t length);
+void LoadExtraRootCertsFile(const v8::FunctionCallbackInfo<v8::Value>& args);
 #ifndef OPENSSL_NO_ENGINE
 void SetEngine(const v8::FunctionCallbackInfo<v8::Value>& args);
 #endif  // !OPENSSL_NO_ENGINE

test/parallel/test-tls-env-extra-ca-file-load.js

@@ -0,0 +1,39 @@
+'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') {
+  if (process.env.CHILD_USE_EXTRA_CA_CERTS === 'yes') {
+    assert.strictEqual(binding.isExtraRootCertsFileLoaded(), true);
+  } else if (process.env.CHILD_USE_EXTRA_CA_CERTS === 'no') {
+    assert.strictEqual(binding.isExtraRootCertsFileLoaded(), false);
+    tls.createServer({});
+    assert.strictEqual(binding.isExtraRootCertsFileLoaded(), false);
+  }
+} else {
+  const NODE_EXTRA_CA_CERTS = fixtures.path('keys', 'ca1-cert.pem');
+
+  [
+    { CHILD_USE_EXTRA_CA_CERTS: 'yes', NODE_EXTRA_CA_CERTS },
+    { 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);
+    }));
+  });
+}

test/parallel/test-tls-env-extra-ca-no-crypto.js

@@ -0,0 +1,22 @@
+'use strict';
+const common = require('../common');
+const fixtures = require('../common/fixtures');
+const assert = require('assert');
+const { fork } = require('child_process');
+
+// This test ensures that trying to load extra certs won't throw even when
+// there is no crypto support, i.e., built with "./configure --without-ssl".
+if (process.argv[2] === 'child') {
+  // exit
+} else {
+  const NODE_EXTRA_CA_CERTS = fixtures.path('keys', 'ca1-cert.pem');
+
+  fork(
+    __filename,
+    ['child'],
+    { env: { NODE_EXTRA_CA_CERTS } },
+  ).on('exit', common.mustCall(function(status) {
+    // client did not succeed in connecting
+    assert.strictEqual(status, 0);
+  }));
+}