Revert "src: fix missing extra ca in tls.rootCertificates"

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>
nodejs · Jun 18, 2020 · 3f1c756 · 3f1c756
1 parent c31d514
commit 3f1c756
Show file tree

Hide file tree

Showing 2 changed files with 43 additions and 89 deletions.
diff --git a/src/node_crypto.cc b/src/node_crypto.cc
@@ -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);
 
@@ -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) {
@@ -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;

diff --git a/test/parallel/test-tls-root-certificates.js b/test/parallel/test-tls-root-certificates.js
@@ -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-----');
+}));