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 <vieuxtech@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
nodejs · Oct 9, 2018 · 87719d792b855e4278dbd3ca209592d83e80ac37 · 87719d7
1 parent 3e3ce22
commit 87719d792b855e4278dbd3ca209592d83e80ac37
Unified Split

Showing with 96 additions and 21 deletions.

+34 −21 src/node_crypto.cc

+40 −0 test/parallel/test-tls-env-extra-ca-file-load.js

+22 −0 test/parallel/test-tls-env-extra-ca-no-crypto.js
diff --git a/src/node_crypto.cc b/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

diff --git a/test/parallel/test-tls-env-extra-ca-file-load.js b/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);
+  
+    }));
+  
+  });
+  
+}
diff --git a/test/parallel/test-tls-env-extra-ca-no-crypto.js b/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);
+  
+  }));
+  
+}