Skip to content

Commit

Permalink
src: Add NODE_EXTRA_CA_CERTS to modified cert stores
Browse files Browse the repository at this point in the history
Store loaded NODE_EXTRA_CA_CERTS into root_certs_vector, allowing
them to be added to secure contexts when NewRootCertStore() is
called.

When NODE_EXTRA_CA_CERTS is specified, the bundled root certificates
will no longer be preloaded at startup. This improves Node.js
startup time and makes the behavior of NODE_EXTRA_CA_CERTS consistent
with the default behavior when NODE_EXTRA_CA_CERTS is ommitted.

Fixes: nodejs#32010
Refs: nodejs#40524
  • Loading branch information
ebickle committed Aug 29, 2022
1 parent bb4dff7 commit ba09b5c
Show file tree
Hide file tree
Showing 4 changed files with 180 additions and 32 deletions.
62 changes: 30 additions & 32 deletions src/crypto/crypto_context.cc
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@ static const char system_cert_path[] = NODE_OPENSSL_SYSTEM_CERT_PATH;

static X509_STORE* root_cert_store;

static std::vector<X509Pointer> root_certs_vector;
static Mutex root_certs_vector_mutex;
static bool root_certs_loaded = false;

static bool extra_root_certs_loaded = false;

// Takes a string or buffer and loads it into a BIO.
Expand Down Expand Up @@ -191,25 +195,25 @@ int SSL_CTX_use_certificate_chain(SSL_CTX* ctx,
} // namespace

X509_STORE* NewRootCertStore() {
static std::vector<X509*> root_certs_vector;
static Mutex root_certs_vector_mutex;
Mutex::ScopedLock lock(root_certs_vector_mutex);

if (root_certs_vector.empty() &&
if (!root_certs_loaded &&
per_process::cli_options->ssl_openssl_cert_store == false) {
for (size_t i = 0; i < arraysize(root_certs); i++) {
X509* x509 =
X509Pointer x509 = X509Pointer(
PEM_read_bio_X509(NodeBIO::NewFixed(root_certs[i],
strlen(root_certs[i])).get(),
nullptr, // no re-use of X509 structure
nullptr, // no re-use of X509 structure
NoPasswordCallback,
nullptr); // no callback data
nullptr)); // no callback data

// Parse errors from the built-in roots are fatal.
CHECK_NOT_NULL(x509);

root_certs_vector.push_back(x509);
root_certs_vector.push_back(std::move(x509));
}

root_certs_loaded = true;
}

X509_STORE* store = X509_STORE_new();
Expand All @@ -222,11 +226,10 @@ X509_STORE* NewRootCertStore() {
Mutex::ScopedLock cli_lock(node::per_process::cli_options_mutex);
if (per_process::cli_options->ssl_openssl_cert_store) {
X509_STORE_set_default_paths(store);
} else {
for (X509* cert : root_certs_vector) {
X509_up_ref(cert);
X509_STORE_add_cert(store, cert);
}
}

for (X509Pointer& cert : root_certs_vector) {
X509_STORE_add_cert(store, cert.get());
}

return store;
Expand Down Expand Up @@ -1299,7 +1302,7 @@ void SecureContext::GetCertificate(const FunctionCallbackInfo<Value>& args) {

namespace {
unsigned long AddCertsFromFile( // NOLINT(runtime/int)
X509_STORE* store,
std::vector<X509Pointer>& certs,
const char* file) {
ERR_clear_error();
MarkPopErrorOnReturn mark_pop_error_on_return;
Expand All @@ -1308,10 +1311,9 @@ unsigned long AddCertsFromFile( // NOLINT(runtime/int)
if (!bio)
return ERR_get_error();

while (X509* x509 =
PEM_read_bio_X509(bio.get(), nullptr, NoPasswordCallback, nullptr)) {
X509_STORE_add_cert(store, x509);
X509_free(x509);
while (X509Pointer x509 = X509Pointer(
PEM_read_bio_X509(bio.get(), nullptr, NoPasswordCallback, nullptr))) {
certs.push_back(std::move(x509));
}

unsigned long err = ERR_peek_error(); // NOLINT(runtime/int)
Expand All @@ -1329,21 +1331,17 @@ unsigned long AddCertsFromFile( // NOLINT(runtime/int)
void UseExtraCaCerts(const std::string& file) {
ClearErrorOnReturn clear_error_on_return;

if (root_cert_store == nullptr) {
root_cert_store = NewRootCertStore();

if (!file.empty()) {
unsigned long err = AddCertsFromFile( // NOLINT(runtime/int)
root_cert_store,
file.c_str());
if (err) {
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;
}
if (!file.empty()) {
unsigned long err = AddCertsFromFile( // NOLINT(runtime/int)
root_certs_vector,
file.c_str());
if (err) {
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;
}
}
}
Expand Down
55 changes: 55 additions & 0 deletions test/parallel/test-tls-env-extra-ca-with-ca.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
'use strict';

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 { fork } = require('child_process');

if (process.env.CHILD) {
const copts = {
port: process.env.PORT,
checkServerIdentity: common.mustCall()
};

// New secure contexts have the well-known root CAs.
copts.secureContext = tls.createSecureContext();

// Explicit calls to addCACert() add to the root certificates,
// instead of replacing, so connection still succeeds.
copts.secureContext.context.addCACert(
fixtures.readKey('ca1-cert.pem')
);

const client = tls.connect(copts, common.mustCall(() => {
client.end('hi');
}));

return;
}

const options = {
key: fixtures.readKey('agent3-key.pem'),
cert: fixtures.readKey('agent3-cert.pem')
};

const server = tls.createServer(options, common.mustCall((socket) => {
socket.end('bye');
server.close();
})).listen(0, common.mustCall(() => {
const env = Object.assign({}, process.env, {
CHILD: 'yes',
PORT: server.address().port,
NODE_EXTRA_CA_CERTS: fixtures.path('keys', 'ca2-cert.pem')
});

fork(__filename, { env }).on('exit', common.mustCall((status) => {
// Client did not succeed in connecting
assert.strictEqual(status, 0);
}));
}));
47 changes: 47 additions & 0 deletions test/parallel/test-tls-env-extra-ca-with-crl.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
'use strict';

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 { fork } = require('child_process');

if (process.env.CHILD) {
const copts = {
port: process.env.PORT,
checkServerIdentity: common.mustCall(),
crl: fixtures.readKey('ca2-crl.pem')
};

const client = tls.connect(copts, common.mustCall(() => {
client.end('hi');
}));

return;
}

const options = {
key: fixtures.readKey('agent3-key.pem'),
cert: fixtures.readKey('agent3-cert.pem')
};

const server = tls.createServer(options, common.mustCall((socket) => {
socket.end('bye');
server.close();
})).listen(0, common.mustCall(() => {
const env = Object.assign({}, process.env, {
CHILD: 'yes',
PORT: server.address().port,
NODE_EXTRA_CA_CERTS: fixtures.path('keys', 'ca2-cert.pem')
});

fork(__filename, { env }).on('exit', common.mustCall((status) => {
// Client did not succeed in connecting
assert.strictEqual(status, 0);
}));
}));
48 changes: 48 additions & 0 deletions test/parallel/test-tls-env-extra-ca-with-pfx.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
'use strict';

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 { fork } = require('child_process');

if (process.env.CHILD) {
const copts = {
port: process.env.PORT,
checkServerIdentity: common.mustCall(),
pfx: fixtures.readKey('agent1.pfx'),
passphrase: 'sample'
};

const client = tls.connect(copts, common.mustCall(() => {
client.end('hi');
}));

return;
}

const options = {
key: fixtures.readKey('agent3-key.pem'),
cert: fixtures.readKey('agent3-cert.pem')
};

const server = tls.createServer(options, common.mustCall((socket) => {
socket.end('bye');
server.close();
})).listen(0, common.mustCall(() => {
const env = Object.assign({}, process.env, {
CHILD: 'yes',
PORT: server.address().port,
NODE_EXTRA_CA_CERTS: fixtures.path('keys', 'ca2-cert.pem')
});

fork(__filename, { env }).on('exit', common.mustCall((status) => {
// Client did not succeed in connecting
assert.strictEqual(status, 0);
}));
}));

0 comments on commit ba09b5c

Please sign in to comment.