Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tls: add allowPartialTrustChain flag #54790

Closed
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions doc/api/tls.md
Original file line number Diff line number Diff line change
Expand Up @@ -1856,6 +1856,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:
- v22.4.0
- v20.16.0
Expand Down Expand Up @@ -1912,6 +1915,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
Expand Down
5 changes: 5 additions & 0 deletions lib/internal/tls/secure-context.js
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ function configSecureContext(context, options = kEmptyObject, name = 'options')
validateObject(options, name);

const {
allowPartialTrustChain,
ca,
cert,
ciphers = getDefaultCiphers(),
Expand Down Expand Up @@ -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`);
}
Expand Down
49 changes: 31 additions & 18 deletions src/crypto/crypto_context.cc
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,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);
Expand Down Expand Up @@ -753,17 +755,38 @@ void SecureContext::SetCert(const FunctionCallbackInfo<Value>& 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<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()));
}
}
Expand Down Expand Up @@ -793,11 +816,7 @@ Maybe<void> SecureContext::SetCRL(Environment* env, const BIOPointer& bio) {
return Nothing<void>();
}

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,
Expand Down Expand Up @@ -1080,8 +1099,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;
Expand Down Expand Up @@ -1135,11 +1152,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;
Expand Down
6 changes: 6 additions & 0 deletions src/crypto/crypto_context.h
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -90,6 +93,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);
Expand Down Expand Up @@ -142,6 +147,7 @@ class SecureContext final : public BaseObject {
SSLCtxPointer ctx_;
X509Pointer cert_;
X509Pointer issuer_;
X509_STORE* owned_cert_store_cached_ = nullptr;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

non-blocking nit: A short comment here about who owns this pointer and who is responsible for cleaning it up would be good for others coming into the code later.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, done in 509156a 👍

#ifndef OPENSSL_NO_ENGINE
bool client_cert_engine_provided_ = false;
ncrypto::EnginePointer private_key_engine_;
Expand Down
44 changes: 44 additions & 0 deletions test/parallel/test-tls-client-allow-partial-trust-chain.js
addaleax marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
'use strict';
const common = require('../common');

if (!common.hasCrypto)
common.skip('missing crypto');
Copy link
Member

@jasnell jasnell Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

non-blocking nit: Since this is using the node:test syntax, this could likely use the skip: !common.hasCrypto option in the test definition itself and just move the require('tls') into the test body.

Copy link
Member

@RedYetiDev RedYetiDev Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this is okay. common.skip will mark the entire file as skipped, while the test runner's skip will only mark the tests as skip, but the file as passed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 509156a 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, did this before seeing @RedYetiDev's response – no strong feelings from my side.


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; }));
Loading