tls: add allowPartialTrustChain flag

This commit exposes the `X509_V_FLAG_PARTIAL_CHAIN` OpenSSL flag to users. This is behavior that has been requested repeatedly in the Github issues, and allows aligning behavior with other TLS libraries and commonly used applications (e.g. `curl`). As a drive-by, simplify the `SecureContext` source by deduplicating call sites at which a new custom certificate store was created for the `secureContext` in question. Fixes: #36453 PR-URL: #54790 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: James M Snell <jasnell@gmail.com>
nodejs · Oct 2, 2024 · bc020f7 · bc020f7
1 parent 1dfd238
commit bc020f7
Show file tree

Hide file tree

Showing 5 changed files with 103 additions and 18 deletions.
diff --git a/doc/api/tls.md b/doc/api/tls.md
@@ -1813,6 +1813,9 @@ argument.
 <!-- YAML
 added: v0.11.13
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/54790
+    description: The `allowPartialTrustChain` option has been added.
   - version: v20.16.0
     pr-url: https://github.com/nodejs/node/pull/53329
     description: The `clientCertEngine`, `privateKeyEngine` and
@@ -1867,6 +1870,8 @@ changes:
 -->
 
 * `options` {Object}
+  * `allowPartialTrustChain` {boolean} Treat intermediate (non-self-signed)
+    certificates in the trust CA certificate list as trusted.
   * `ca` {string|string\[]|Buffer|Buffer\[]} Optionally override the trusted CA
     certificates. Default is to trust the well-known CAs curated by Mozilla.
     Mozilla's CAs are completely replaced when CAs are explicitly specified

diff --git a/lib/internal/tls/secure-context.js b/lib/internal/tls/secure-context.js
@@ -130,6 +130,7 @@ function configSecureContext(context, options = kEmptyObject, name = 'options')
   validateObject(options, name);
 
   const {
+    allowPartialTrustChain,
     ca,
     cert,
     ciphers = getDefaultCiphers(),
@@ -182,6 +183,10 @@ function configSecureContext(context, options = kEmptyObject, name = 'options')
     context.addRootCerts();
   }
 
+  if (allowPartialTrustChain) {
+    context.setAllowPartialTrustChain();
+  }
+
   if (cert) {
     setCerts(context, ArrayIsArray(cert) ? cert : [cert], `${name}.cert`);
   }

diff --git a/src/crypto/crypto_context.cc b/src/crypto/crypto_context.cc
@@ -273,6 +273,8 @@ Local<FunctionTemplate> SecureContext::GetConstructorTemplate(
     SetProtoMethod(isolate, tmpl, "setKey", SetKey);
     SetProtoMethod(isolate, tmpl, "setCert", SetCert);
     SetProtoMethod(isolate, tmpl, "addCACert", AddCACert);
+    SetProtoMethod(
+        isolate, tmpl, "setAllowPartialTrustChain", SetAllowPartialTrustChain);
     SetProtoMethod(isolate, tmpl, "addCRL", AddCRL);
     SetProtoMethod(isolate, tmpl, "addRootCerts", AddRootCerts);
     SetProtoMethod(isolate, tmpl, "setCipherSuites", SetCipherSuites);
@@ -354,6 +356,7 @@ void SecureContext::RegisterExternalReferences(
   registry->Register(AddCACert);
   registry->Register(AddCRL);
   registry->Register(AddRootCerts);
+  registry->Register(SetAllowPartialTrustChain);
   registry->Register(SetCipherSuites);
   registry->Register(SetCiphers);
   registry->Register(SetSigalgs);
@@ -715,17 +718,39 @@ void SecureContext::SetCert(const FunctionCallbackInfo<Value>& args) {
   USE(sc->AddCert(env, std::move(bio)));
 }
 
+// NOLINTNEXTLINE(runtime/int)
+void SecureContext::SetX509StoreFlag(unsigned long flags) {
+  X509_STORE* cert_store = GetCertStoreOwnedByThisSecureContext();
+  CHECK_EQ(1, X509_STORE_set_flags(cert_store, flags));
+}
+
+X509_STORE* SecureContext::GetCertStoreOwnedByThisSecureContext() {
+  if (own_cert_store_cache_ != nullptr) return own_cert_store_cache_;
+
+  X509_STORE* cert_store = SSL_CTX_get_cert_store(ctx_.get());
+  if (cert_store == GetOrCreateRootCertStore()) {
+    cert_store = NewRootCertStore();
+    SSL_CTX_set_cert_store(ctx_.get(), cert_store);
+  }
+
+  return own_cert_store_cache_ = cert_store;
+}
+
+void SecureContext::SetAllowPartialTrustChain(
+    const FunctionCallbackInfo<Value>& args) {
+  SecureContext* sc;
+  ASSIGN_OR_RETURN_UNWRAP(&sc, args.This());
+  sc->SetX509StoreFlag(X509_V_FLAG_PARTIAL_CHAIN);
+}
+
 void SecureContext::SetCACert(const BIOPointer& bio) {
   ClearErrorOnReturn clear_error_on_return;
   if (!bio) return;
-  X509_STORE* cert_store = SSL_CTX_get_cert_store(ctx_.get());
   while (X509Pointer x509 = X509Pointer(PEM_read_bio_X509_AUX(
              bio.get(), nullptr, NoPasswordCallback, nullptr))) {
-    if (cert_store == GetOrCreateRootCertStore()) {
-      cert_store = NewRootCertStore();
-      SSL_CTX_set_cert_store(ctx_.get(), cert_store);
-    }
-    CHECK_EQ(1, X509_STORE_add_cert(cert_store, x509.get()));
+    CHECK_EQ(1,
+             X509_STORE_add_cert(GetCertStoreOwnedByThisSecureContext(),
+                                 x509.get()));
     CHECK_EQ(1, SSL_CTX_add_client_CA(ctx_.get(), x509.get()));
   }
 }
@@ -754,11 +779,7 @@ Maybe<bool> SecureContext::SetCRL(Environment* env, const BIOPointer& bio) {
     return Nothing<bool>();
   }
 
-  X509_STORE* cert_store = SSL_CTX_get_cert_store(ctx_.get());
-  if (cert_store == GetOrCreateRootCertStore()) {
-    cert_store = NewRootCertStore();
-    SSL_CTX_set_cert_store(ctx_.get(), cert_store);
-  }
+  X509_STORE* cert_store = GetCertStoreOwnedByThisSecureContext();
 
   CHECK_EQ(1, X509_STORE_add_crl(cert_store, crl.get()));
   CHECK_EQ(1,
@@ -1042,8 +1063,6 @@ void SecureContext::LoadPKCS12(const FunctionCallbackInfo<Value>& args) {
   sc->issuer_.reset();
   sc->cert_.reset();
 
-  X509_STORE* cert_store = SSL_CTX_get_cert_store(sc->ctx_.get());
-
   DeleteFnPtr<PKCS12, PKCS12_free> p12;
   EVPKeyPointer pkey;
   X509Pointer cert;
@@ -1097,11 +1116,7 @@ void SecureContext::LoadPKCS12(const FunctionCallbackInfo<Value>& args) {
   for (int i = 0; i < sk_X509_num(extra_certs.get()); i++) {
     X509* ca = sk_X509_value(extra_certs.get(), i);
 
-    if (cert_store == GetOrCreateRootCertStore()) {
-      cert_store = NewRootCertStore();
-      SSL_CTX_set_cert_store(sc->ctx_.get(), cert_store);
-    }
-    X509_STORE_add_cert(cert_store, ca);
+    X509_STORE_add_cert(sc->GetCertStoreOwnedByThisSecureContext(), ca);
     SSL_CTX_add_client_CA(sc->ctx_.get(), ca);
   }
   ret = true;

diff --git a/src/crypto/crypto_context.h b/src/crypto/crypto_context.h
@@ -65,6 +65,9 @@ class SecureContext final : public BaseObject {
   void SetCACert(const BIOPointer& bio);
   void SetRootCerts();
 
+  void SetX509StoreFlag(unsigned long flags);  // NOLINT(runtime/int)
+  X509_STORE* GetCertStoreOwnedByThisSecureContext();
+
   // TODO(joyeecheung): track the memory used by OpenSSL types
   SET_NO_MEMORY_INFO()
   SET_MEMORY_INFO_NAME(SecureContext)
@@ -91,6 +94,8 @@ class SecureContext final : public BaseObject {
 #endif  // !OPENSSL_NO_ENGINE
   static void SetCert(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void AddCACert(const v8::FunctionCallbackInfo<v8::Value>& args);
+  static void SetAllowPartialTrustChain(
+      const v8::FunctionCallbackInfo<v8::Value>& args);
   static void AddCRL(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void AddRootCerts(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void SetCipherSuites(const v8::FunctionCallbackInfo<v8::Value>& args);
@@ -143,6 +148,8 @@ class SecureContext final : public BaseObject {
   SSLCtxPointer ctx_;
   X509Pointer cert_;
   X509Pointer issuer_;
+  // Non-owning cache for SSL_CTX_get_cert_store(ctx_.get())
+  X509_STORE* own_cert_store_cache_ = nullptr;
 #ifndef OPENSSL_NO_ENGINE
   bool client_cert_engine_provided_ = false;
   EnginePointer private_key_engine_;

diff --git a/test/parallel/test-tls-client-allow-partial-trust-chain.js b/test/parallel/test-tls-client-allow-partial-trust-chain.js
@@ -0,0 +1,53 @@
+'use strict';
+const common = require('../common');
+if (!common.hasCrypto) { common.skip('missing crypto'); }
+
+const assert = require('assert');
+const { once } = require('events');
+const fixtures = require('../common/fixtures');
+
+// agent6-cert.pem is signed by intermediate cert of ca3.
+// The server has a cert chain of agent6->ca3->ca1(root).
+
+const { it, beforeEach, afterEach, describe } = require('node:test');
+
+describe('allowPartialTrustChain', { skip: !common.hasCrypto }, function() {
+  const tls = require('tls');
+  let server;
+  let client;
+  let opts;
+
+  beforeEach(async function() {
+    server = tls.createServer({
+      ca: fixtures.readKey('ca3-cert.pem'),
+      key: fixtures.readKey('agent6-key.pem'),
+      cert: fixtures.readKey('agent6-cert.pem'),
+    }, (socket) => socket.resume());
+    server.listen(0);
+    await once(server, 'listening');
+
+    opts = {
+      port: server.address().port,
+      ca: fixtures.readKey('ca3-cert.pem'),
+      checkServerIdentity() {}
+    };
+  });
+
+  afterEach(async function() {
+    client?.destroy();
+    server?.close();
+  });
+
+  it('can connect successfully with allowPartialTrustChain: true', async function() {
+    client = tls.connect({ ...opts, allowPartialTrustChain: true });
+    await once(client, 'secureConnect'); // Should not throw
+  });
+
+  it('fails without with allowPartialTrustChain: true for an intermediate cert in the CA', async function() {
+    // Consistency check: Connecting fails without allowPartialTrustChain: true
+    await assert.rejects(async () => {
+      const client = tls.connect(opts);
+      await once(client, 'secureConnect');
+    }, { code: 'UNABLE_TO_GET_ISSUER_CERT' });
+  });
+});