Skip to content

Commit

Permalink
Revert "src: fix missing extra ca in tls.rootCertificates"
Browse files Browse the repository at this point in the history
A fix to tls.rootCertificates to have it correctly return the
process' current root certificates resulted in non-deterministic
behavior when Node.js is configured to use an OpenSSL system or
file-based certificate store.

The safest action is to revert the change and change the specification
for tls.rootCertificates to state that it only returns the bundled
certificates instead of the current ones.

Fixes: #32229
Refs: #32074

PR-URL: #33313
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
ebickle authored and codebytere committed Jun 18, 2020
1 parent c31d514 commit 3f1c756
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 89 deletions.
63 changes: 18 additions & 45 deletions src/node_crypto.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1008,6 +1008,24 @@ static X509_STORE* NewRootCertStore() {
}


void GetRootCertificates(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
Local<Value> result[arraysize(root_certs)];

for (size_t i = 0; i < arraysize(root_certs); i++) {
if (!String::NewFromOneByte(
env->isolate(),
reinterpret_cast<const uint8_t*>(root_certs[i]),
NewStringType::kNormal).ToLocal(&result[i])) {
return;
}
}

args.GetReturnValue().Set(
Array::New(env->isolate(), result, arraysize(root_certs)));
}


void SecureContext::AddCACert(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);

Expand Down Expand Up @@ -2684,21 +2702,6 @@ static inline Local<Value> BIOToStringOrBuffer(Environment* env,
}
}

static MaybeLocal<Value> X509ToPEM(Environment* env, X509* cert) {
BIOPointer bio(BIO_new(BIO_s_mem()));
if (!bio) {
ThrowCryptoError(env, ERR_get_error(), "BIO_new");
return MaybeLocal<Value>();
}

if (PEM_write_bio_X509(bio.get(), cert) == 0) {
ThrowCryptoError(env, ERR_get_error(), "PEM_write_bio_X509");
return MaybeLocal<Value>();
}

return BIOToStringOrBuffer(env, bio.get(), kKeyFormatPEM);
}

static bool WritePublicKeyInner(EVP_PKEY* pkey,
const BIOPointer& bio,
const PublicKeyEncodingConfig& config) {
Expand Down Expand Up @@ -6648,36 +6651,6 @@ void ExportChallenge(const FunctionCallbackInfo<Value>& args) {
}


void GetRootCertificates(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);

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

stack_st_X509_OBJECT* objs = X509_STORE_get0_objects(root_cert_store);
int num_objs = sk_X509_OBJECT_num(objs);

std::vector<Local<Value>> result;
result.reserve(num_objs);

for (int i = 0; i < num_objs; i++) {
X509_OBJECT* obj = sk_X509_OBJECT_value(objs, i);
if (X509_OBJECT_get_type(obj) == X509_LU_X509) {
X509* cert = X509_OBJECT_get0_X509(obj);

Local<Value> value;
if (!X509ToPEM(env, cert).ToLocal(&value))
return;

result.push_back(value);
}
}

args.GetReturnValue().Set(
Array::New(env->isolate(), result.data(), result.size()));
}


// Convert the input public key to compressed, uncompressed, or hybrid formats.
void ConvertKey(const FunctionCallbackInfo<Value>& args) {
MarkPopErrorOnReturn mark_pop_error_on_return;
Expand Down
69 changes: 25 additions & 44 deletions test/parallel/test-tls-root-certificates.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,49 +2,30 @@
const common = require('../common');
if (!common.hasCrypto) common.skip('missing crypto');

const fixtures = require('../common/fixtures');
const assert = require('assert');
const tls = require('tls');
const { fork } = require('child_process');

if (process.argv[2] !== 'child') {
// Parent
const NODE_EXTRA_CA_CERTS = fixtures.path('keys', 'ca1-cert.pem');

fork(
__filename,
['child'],
{ env: { ...process.env, NODE_EXTRA_CA_CERTS } }
).on('exit', common.mustCall(function(status) {
assert.strictEqual(status, 0);
}));
} else {
// Child
assert(Array.isArray(tls.rootCertificates));
assert(tls.rootCertificates.length > 0);

// Getter should return the same object.
assert.strictEqual(tls.rootCertificates, tls.rootCertificates);

// Array is immutable...
assert.throws(() => tls.rootCertificates[0] = 0, /TypeError/);
assert.throws(() => tls.rootCertificates.sort(), /TypeError/);

// ...and so is the property.
assert.throws(() => tls.rootCertificates = 0, /TypeError/);

// Does not contain duplicates.
assert.strictEqual(tls.rootCertificates.length,
new Set(tls.rootCertificates).size);

assert(tls.rootCertificates.every((s) => {
return s.startsWith('-----BEGIN CERTIFICATE-----\n');
}));

assert(tls.rootCertificates.every((s) => {
return s.endsWith('\n-----END CERTIFICATE-----\n');
}));

const extraCert = fixtures.readKey('ca1-cert.pem', 'utf8');
assert(tls.rootCertificates.includes(extraCert));
}

assert(Array.isArray(tls.rootCertificates));
assert(tls.rootCertificates.length > 0);

// Getter should return the same object.
assert.strictEqual(tls.rootCertificates, tls.rootCertificates);

// Array is immutable...
assert.throws(() => tls.rootCertificates[0] = 0, /TypeError/);
assert.throws(() => tls.rootCertificates.sort(), /TypeError/);

// ...and so is the property.
assert.throws(() => tls.rootCertificates = 0, /TypeError/);

// Does not contain duplicates.
assert.strictEqual(tls.rootCertificates.length,
new Set(tls.rootCertificates).size);

assert(tls.rootCertificates.every((s) => {
return s.startsWith('-----BEGIN CERTIFICATE-----\n');
}));

assert(tls.rootCertificates.every((s) => {
return s.endsWith('\n-----END CERTIFICATE-----');
}));

0 comments on commit 3f1c756

Please sign in to comment.