From 87719d792b855e4278dbd3ca209592d83e80ac37 Mon Sep 17 00:00:00 2001 From: Ouyang Yadong Date: Tue, 9 Oct 2018 08:51:15 -0400 Subject: [PATCH] tls: load NODE_EXTRA_CA_CERTS at startup This commit makes node load extra certificates at startup instead of first use. PR-URL: https://github.com/nodejs/node/pull/23354 Fixes: https://github.com/nodejs/node/issues/20434 Refs: https://github.com/nodejs/node/issues/20432 Reviewed-By: Sam Roberts Reviewed-By: Sakthipriyan Vairamani Reviewed-By: Refael Ackermann Reviewed-By: Anna Henningsen --- src/node_crypto.cc | 55 ++++++++++++------- .../test-tls-env-extra-ca-file-load.js | 40 ++++++++++++++ .../test-tls-env-extra-ca-no-crypto.js | 22 ++++++++ 3 files changed, 96 insertions(+), 21 deletions(-) create mode 100644 test/parallel/test-tls-env-extra-ca-file-load.js create mode 100644 test/parallel/test-tls-env-extra-ca-no-crypto.js diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 421db555d21973..bd8d9e0325546b 100644 --- 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::AddMethods(Environment* env, Local t); @@ -832,11 +832,6 @@ void SecureContext::AddCRL(const FunctionCallbackInfo& 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& 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& args) { + return args.GetReturnValue().Set(extra_root_certs_loaded); +} + + +void SecureContext::AddRootCerts(const FunctionCallbackInfo& 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& args) { } #endif /* NODE_FIPS_MODE */ + void Initialize(Local target, Local unused, Local context, @@ -5644,6 +5654,9 @@ void Initialize(Local 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 new file mode 100644 index 00000000000000..fa97d7c0c6103f --- /dev/null +++ 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 new file mode 100644 index 00000000000000..06399c5d239fc6 --- /dev/null +++ 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); + })); +}