tls: load NODE_EXTRA_CA_CERTS at startup

This commit makes node load extra certificates at startup instead
of first use.

PR-URL: #23354
Fixes: #20434
Refs: #20432
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>

Author: oyyd

src/node_crypto.cc

@@ -116,10 +116,10 @@ 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;
 
+static bool extra_root_certs_loaded = false;
+
 // Just to generate static methods
 template void SSLWrap<TLSWrap>::AddMethods(Environment* env,
                                            Local<FunctionTemplate> t);
@@ -832,11 +832,6 @@ void SecureContext::AddCRL(const FunctionCallbackInfo<Value>& args) {
 }
 
 
-void UseExtraCaCerts(const std::string& file) {
-  extra_root_certs_file = file;
-}
-
-
 static unsigned long AddCertsFromFile(  // NOLINT(runtime/int)
     X509_STORE* store,
     const char* file) {
@@ -863,30 +858,44 @@ static unsigned long AddCertsFromFile(  // NOLINT(runtime/int)
   return err;
 }
 
-void SecureContext::AddRootCerts(const FunctionCallbackInfo<Value>& args) {
-  SecureContext* sc;
-  ASSIGN_OR_RETURN_UNWRAP(&sc, args.Holder());
+
+void UseExtraCaCerts(const std::string& file) {
   ClearErrorOnReturn clear_error_on_return;
 
-  if (!root_cert_store) {
+  if (root_cert_store == nullptr) {
     root_cert_store = NewRootCertStore();
 
-    if (!extra_root_certs_file.empty()) {
+    if (!file.empty()) {
       unsigned long err = AddCertsFromFile(  // NOLINT(runtime/int)
                                            root_cert_store,
-                                           extra_root_certs_file.c_str());
+                                           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));
+        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;
       }
     }
   }
+}
+
+
+static void IsExtraRootCertsFileLoaded(
+    const FunctionCallbackInfo<Value>& args) {
+  return args.GetReturnValue().Set(extra_root_certs_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 == nullptr) {
+    root_cert_store = NewRootCertStore();
+  }
 
   // Increment reference count so global store is not deleted along with CTX.
   X509_STORE_up_ref(root_cert_store);
@@ -5624,6 +5633,7 @@ void SetFipsCrypto(const FunctionCallbackInfo<Value>& args) {
 }
 #endif /* NODE_FIPS_MODE */
 
+
 void Initialize(Local<Object> target,
                 Local<Value> unused,
                 Local<Context> context,
@@ -5644,6 +5654,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

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

@@ -0,0 +1,40 @@
+'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');
+  const extendsEnv = (obj) => Object.assign({}, process.env, obj);
+
+  [
+    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);
+    }));
+  });
+}

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: Object.assign({}, process.env, { NODE_EXTRA_CA_CERTS }) },
+  ).on('exit', common.mustCall(function(status) {
+    // client did not succeed in connecting
+    assert.strictEqual(status, 0);
+  }));
+}