Skip to content

Commit

Permalink
tls: load NODE_EXTRA_CA_CERTS at startup
Browse files Browse the repository at this point in the history
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]>
  • Loading branch information
oyyd authored and refack committed Oct 20, 2018
1 parent 3e3ce22 commit 87719d7
Show file tree
Hide file tree
Showing 3 changed files with 96 additions and 21 deletions.
55 changes: 34 additions & 21 deletions src/node_crypto.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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) {
Expand All @@ -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);
Expand Down Expand Up @@ -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,
Expand All @@ -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
Expand Down
40 changes: 40 additions & 0 deletions test/parallel/test-tls-env-extra-ca-file-load.js
Original file line number Diff line number Diff line change
@@ -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);
}));
});
}
22 changes: 22 additions & 0 deletions test/parallel/test-tls-env-extra-ca-no-crypto.js
Original file line number Diff line number Diff line change
@@ -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);
}));
}

0 comments on commit 87719d7

Please sign in to comment.