From bb69f1408b13bec3960d9ba8e496c3d61416a19e Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 5 Sep 2024 16:31:24 +0200 Subject: [PATCH 1/6] 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 --- doc/api/tls.md | 5 +++ lib/internal/tls/secure-context.js | 5 +++ src/crypto/crypto_context.cc | 45 +++++++++++-------- src/crypto/crypto_context.h | 5 +++ ...st-tls-client-allow-partial-trust-chain.js | 44 ++++++++++++++++++ 5 files changed, 86 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 5d8fe04515486e..5f0ea0157b14cb 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 b0f971e4eef273..84e74105fdbba9 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 48fecc82c159d8..93908e1e7677a2 100644 --- a/src/crypto/crypto_context.cc +++ b/src/crypto/crypto_context.cc @@ -314,6 +314,7 @@ 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); @@ -753,17 +754,35 @@ void SecureContext::SetCert(const FunctionCallbackInfo& args) { USE(sc->AddCert(env, std::move(bio))); } +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 (owned_cert_store_cached_ != nullptr) return owned_cert_store_cached_; + + 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 owned_cert_store_cached_ = 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 +812,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 +1095,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 +1148,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 00d4c3738cb905..3bee32a484d912 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); + X509_STORE* GetCertStoreOwnedByThisSecureContext(); + // TODO(joyeecheung): track the memory used by OpenSSL types SET_NO_MEMORY_INFO() SET_MEMORY_INFO_NAME(SecureContext) @@ -90,6 +93,7 @@ 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 +146,7 @@ class SecureContext final : public BaseObject { SSLCtxPointer ctx_; X509Pointer cert_; X509Pointer issuer_; + X509_STORE* owned_cert_store_cached_ = 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 00000000000000..e34d2152654878 --- /dev/null +++ b/test/parallel/test-tls-client-allow-partial-trust-chain.js @@ -0,0 +1,44 @@ +'use strict'; +const common = require('../common'); + +if (!common.hasCrypto) + common.skip('missing crypto'); + +const assert = require('assert'); +const { once } = require('events'); +const tls = require('tls'); +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) but + +async function test() { + const 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'); + + const opts = { + port: server.address().port, + ca: fixtures.readKey('ca3-cert.pem'), + checkServerIdentity() {} + }; + + // Connecting succeeds with allowPartialTrustChain: true + const client = tls.connect({ ...opts, allowPartialTrustChain: true }); + await once(client, 'secureConnect'); + client.destroy(); + + // 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' }); + + server.close(); +} + +test().catch((err) => process.nextTick(() => { throw err; })); From d4883402588ea3db29efecfc9b5b0b52619fe866 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 5 Sep 2024 17:01:18 +0200 Subject: [PATCH 2/6] fixup! tls: add `allowPartialTrustChain` flag --- src/crypto/crypto_context.cc | 10 +++++++--- src/crypto/crypto_context.h | 3 ++- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/crypto/crypto_context.cc b/src/crypto/crypto_context.cc index 93908e1e7677a2..a61333183ee6cf 100644 --- a/src/crypto/crypto_context.cc +++ b/src/crypto/crypto_context.cc @@ -314,7 +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, "setAllowPartialTrustChain", SetAllowPartialTrustChain); SetProtoMethod(isolate, tmpl, "addCRL", AddCRL); SetProtoMethod(isolate, tmpl, "addRootCerts", AddRootCerts); SetProtoMethod(isolate, tmpl, "setCipherSuites", SetCipherSuites); @@ -771,7 +772,8 @@ X509_STORE* SecureContext::GetCertStoreOwnedByThisSecureContext() { return owned_cert_store_cached_ = cert_store; } -void SecureContext::SetAllowPartialTrustChain(const FunctionCallbackInfo& args) { +void SecureContext::SetAllowPartialTrustChain( + const FunctionCallbackInfo& args) { SecureContext* sc; ASSIGN_OR_RETURN_UNWRAP(&sc, args.This()); sc->SetX509StoreFlag(X509_V_FLAG_PARTIAL_CHAIN); @@ -782,7 +784,9 @@ void SecureContext::SetCACert(const BIOPointer& bio) { if (!bio) return; while (X509Pointer x509 = X509Pointer(PEM_read_bio_X509_AUX( bio.get(), nullptr, NoPasswordCallback, nullptr))) { - CHECK_EQ(1, X509_STORE_add_cert(GetCertStoreOwnedByThisSecureContext(), x509.get())); + CHECK_EQ(1, + X509_STORE_add_cert(GetCertStoreOwnedByThisSecureContext(), + x509.get())); CHECK_EQ(1, SSL_CTX_add_client_CA(ctx_.get(), x509.get())); } } diff --git a/src/crypto/crypto_context.h b/src/crypto/crypto_context.h index 3bee32a484d912..f2a6befd44144e 100644 --- a/src/crypto/crypto_context.h +++ b/src/crypto/crypto_context.h @@ -93,7 +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 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); From 5f501b457a587e65bbaf90d5c539449d5ea5c260 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 5 Sep 2024 17:56:02 +0200 Subject: [PATCH 3/6] fixup! fixup! tls: add `allowPartialTrustChain` flag --- ...st-tls-client-allow-partial-trust-chain.js | 75 +++++++++++-------- 1 file changed, 43 insertions(+), 32 deletions(-) diff --git a/test/parallel/test-tls-client-allow-partial-trust-chain.js b/test/parallel/test-tls-client-allow-partial-trust-chain.js index e34d2152654878..2296088b19b253 100644 --- a/test/parallel/test-tls-client-allow-partial-trust-chain.js +++ b/test/parallel/test-tls-client-allow-partial-trust-chain.js @@ -10,35 +10,46 @@ const tls = require('tls'); 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) but - -async function test() { - const 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'); - - const opts = { - port: server.address().port, - ca: fixtures.readKey('ca3-cert.pem'), - checkServerIdentity() {} - }; - - // Connecting succeeds with allowPartialTrustChain: true - const client = tls.connect({ ...opts, allowPartialTrustChain: true }); - await once(client, 'secureConnect'); - client.destroy(); - - // 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' }); - - server.close(); -} - -test().catch((err) => process.nextTick(() => { throw err; })); +// The server has a cert chain of agent6->ca3->ca1(root). + +const { it, beforeEach, afterEach, describe } = require('node:test'); + +describe('allowPartialTrustChain', function() { + 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' }); + }); +}); From 509156ac75aed153b3deab72b33eec6c3e6efe17 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 5 Sep 2024 19:09:29 +0200 Subject: [PATCH 4/6] fixup! tls: add `allowPartialTrustChain` flag --- src/crypto/crypto_context.cc | 4 ++-- src/crypto/crypto_context.h | 3 ++- .../parallel/test-tls-client-allow-partial-trust-chain.js | 8 +++----- 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/src/crypto/crypto_context.cc b/src/crypto/crypto_context.cc index a61333183ee6cf..0277f9202d64c3 100644 --- a/src/crypto/crypto_context.cc +++ b/src/crypto/crypto_context.cc @@ -761,7 +761,7 @@ void SecureContext::SetX509StoreFlag(unsigned long flags) { } X509_STORE* SecureContext::GetCertStoreOwnedByThisSecureContext() { - if (owned_cert_store_cached_ != nullptr) return owned_cert_store_cached_; + 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()) { @@ -769,7 +769,7 @@ X509_STORE* SecureContext::GetCertStoreOwnedByThisSecureContext() { SSL_CTX_set_cert_store(ctx_.get(), cert_store); } - return owned_cert_store_cached_ = cert_store; + return own_cert_store_cache_ = cert_store; } void SecureContext::SetAllowPartialTrustChain( diff --git a/src/crypto/crypto_context.h b/src/crypto/crypto_context.h index f2a6befd44144e..c399fd15340598 100644 --- a/src/crypto/crypto_context.h +++ b/src/crypto/crypto_context.h @@ -147,7 +147,8 @@ class SecureContext final : public BaseObject { SSLCtxPointer ctx_; X509Pointer cert_; X509Pointer issuer_; - X509_STORE* owned_cert_store_cached_ = nullptr; + // 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 index 2296088b19b253..ffa6b2b1677c93 100644 --- a/test/parallel/test-tls-client-allow-partial-trust-chain.js +++ b/test/parallel/test-tls-client-allow-partial-trust-chain.js @@ -1,12 +1,9 @@ 'use strict'; const common = require('../common'); - -if (!common.hasCrypto) - common.skip('missing crypto'); +if (!common.hasCrypto) { common.skip('missing crypto'); }; const assert = require('assert'); const { once } = require('events'); -const tls = require('tls'); const fixtures = require('../common/fixtures'); // agent6-cert.pem is signed by intermediate cert of ca3. @@ -14,7 +11,8 @@ const fixtures = require('../common/fixtures'); const { it, beforeEach, afterEach, describe } = require('node:test'); -describe('allowPartialTrustChain', function() { +describe('allowPartialTrustChain', { skip: !common.hasCrypto }, function() { + const tls = require('tls'); let server; let client; let opts; From 433f6b36c7878b47688ba521df2479e7d8fa6736 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 5 Sep 2024 19:44:03 +0200 Subject: [PATCH 5/6] fixup! fixup! tls: add `allowPartialTrustChain` flag --- src/crypto/crypto_context.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/crypto/crypto_context.cc b/src/crypto/crypto_context.cc index 0277f9202d64c3..3af24c2d89bb0b 100644 --- a/src/crypto/crypto_context.cc +++ b/src/crypto/crypto_context.cc @@ -392,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); From 3ae1ea04e561a95fbd96d6f1e721a9191417e985 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 5 Sep 2024 19:47:26 +0200 Subject: [PATCH 6/6] fixup! fixup! fixup! tls: add `allowPartialTrustChain` flag --- src/crypto/crypto_context.cc | 1 + src/crypto/crypto_context.h | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/crypto/crypto_context.cc b/src/crypto/crypto_context.cc index 3af24c2d89bb0b..ad981617e41b68 100644 --- a/src/crypto/crypto_context.cc +++ b/src/crypto/crypto_context.cc @@ -756,6 +756,7 @@ 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)); diff --git a/src/crypto/crypto_context.h b/src/crypto/crypto_context.h index c399fd15340598..c6c10b1426ac6b 100644 --- a/src/crypto/crypto_context.h +++ b/src/crypto/crypto_context.h @@ -64,7 +64,7 @@ class SecureContext final : public BaseObject { void SetCACert(const BIOPointer& bio); void SetRootCerts(); - void SetX509StoreFlag(unsigned long flags); + void SetX509StoreFlag(unsigned long flags); // NOLINT(runtime/int) X509_STORE* GetCertStoreOwnedByThisSecureContext(); // TODO(joyeecheung): track the memory used by OpenSSL types