From c3a7b29e56a5ada6327ebb622ba746d022685742 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 5 Sep 2024 16:31:24 +0200 Subject: [PATCH] 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: https://github.com/nodejs/node/issues/36453 PR-URL: https://github.com/nodejs/node/pull/54790 Reviewed-By: Yagiz Nizipli Reviewed-By: James M Snell --- doc/api/tls.md | 5 ++ lib/internal/tls/secure-context.js | 5 ++ src/crypto/crypto_context.cc | 51 +++++++++++------- src/crypto/crypto_context.h | 7 +++ ...st-tls-client-allow-partial-trust-chain.js | 53 +++++++++++++++++++ 5 files changed, 103 insertions(+), 18 deletions(-) create mode 100644 test/parallel/test-tls-client-allow-partial-trust-chain.js diff --git a/doc/api/tls.md b/doc/api/tls.md index 5d8fe045154..5f0ea0157b1 100644 --- a/doc/api/tls.md +++ b/doc/api/tls.md @@ -1856,6 +1856,9 @@ argument. * `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 index b0f971e4eef..84e74105fdb 100644 --- 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 index aa5ba34c761..ea7be16fa73 100644 --- a/src/crypto/crypto_context.cc +++ b/src/crypto/crypto_context.cc @@ -314,6 +314,8 @@ Local 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); @@ -390,6 +392,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); @@ -753,17 +756,39 @@ void SecureContext::SetCert(const FunctionCallbackInfo& 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& 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())); } } @@ -793,11 +818,7 @@ Maybe SecureContext::SetCRL(Environment* env, const BIOPointer& bio) { return Nothing(); } - 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, @@ -1080,8 +1101,6 @@ void SecureContext::LoadPKCS12(const FunctionCallbackInfo& args) { sc->issuer_.reset(); sc->cert_.reset(); - X509_STORE* cert_store = SSL_CTX_get_cert_store(sc->ctx_.get()); - DeleteFnPtr p12; EVPKeyPointer pkey; X509Pointer cert; @@ -1135,11 +1154,7 @@ void SecureContext::LoadPKCS12(const FunctionCallbackInfo& 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 index 00d4c3738cb..c6c10b1426a 100644 --- a/src/crypto/crypto_context.h +++ b/src/crypto/crypto_context.h @@ -64,6 +64,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) @@ -90,6 +93,8 @@ class SecureContext final : public BaseObject { #endif // !OPENSSL_NO_ENGINE static void SetCert(const v8::FunctionCallbackInfo& args); static void AddCACert(const v8::FunctionCallbackInfo& args); + static void SetAllowPartialTrustChain( + const v8::FunctionCallbackInfo& args); static void AddCRL(const v8::FunctionCallbackInfo& args); static void AddRootCerts(const v8::FunctionCallbackInfo& args); static void SetCipherSuites(const v8::FunctionCallbackInfo& args); @@ -142,6 +147,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; ncrypto::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 new file mode 100644 index 00000000000..ffa6b2b1677 --- /dev/null +++ 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' }); + }); +});