-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
src: modify SecureContext::SetCACert to not create root certificate store #56301
base: main
Are you sure you want to change the base?
Conversation
Review requested:
|
13674e5
to
f61b98f
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #56301 +/- ##
=======================================
Coverage 88.54% 88.54%
=======================================
Files 657 657
Lines 190290 190323 +33
Branches 36533 36547 +14
=======================================
+ Hits 168488 168525 +37
+ Misses 14984 14981 -3
+ Partials 6818 6817 -1
|
db9d305
to
79172cf
Compare
node.exe D:/1/nodejs/benchmark/compare.js --old T:/node.v23.4.0.exe --new D:/1/nodejs/node.exe --runs 20 --filter create-secure-context tls
const common = require('../common.js');
const fixtures = require('../../test/common/fixtures');
const tls = require('tls');
const bench = common.createBenchmark(main, {
n: [1, 5],
ca: [0, 1],
});
function main(conf) {
const n = conf.n;
const options = {
key: fixtures.readKey('rsa_private.pem'),
cert: fixtures.readKey('rsa_cert.crt'),
ca: conf.ca === 1 ? [fixtures.readKey('rsa_ca.crt')] : undefined,
};
bench.start();
tls.createSecureContext(options);
bench.end(n);
} |
6b87eab
to
15e8061
Compare
15e8061
to
5efa9aa
Compare
It's been two weeks. Could someone please take a little time to review and move this PR forward? @addaleax @jasnell @tniessen @mgochoa Let me explain a little more about what I optimized specifically: When creating SecureContext, the call stack is as follows
function createSecureContext
const c = new SecureContext(secureProtocol, secureOptions, minVersion, maxVersion);
Configure SecureContext in configSecureContext(c.context, ...)
if (ca) // When ca option is passed, according to docs, only need to load provided ca cert, no need to load root certs
Called context.addCACert(cert);
sc->SetCACert(bio);
!! Old Logic !!
X509_STORE* cert_store = GetCertStoreOwnedByThisSecureContext()
X509_STORE* cert_store = SSL_CTX_get_cert_store(ctx_.get());
// Check if current ssl ctx's cert store is global store, if yes, create a new cert store
// This is a typical copy on write scenario
if (cert_store == GetOrCreateRootCertStore()) {
// GetOrCreateRootCertStore() does
// static X509_STORE* store = NewRootCertStore(); // !! Slow operation !!
// return store;
cert_store = NewRootCertStore();
SSL_CTX_set_cert_store(ctx_.get(), cert_store);
}
return own_cert_store_cache_ = cert_store;
X509_STORE_add_cert(own_cert_store_cache_, x509.get())
!! New Logic !!
X509_STORE* cert_store;
// Check if current ssl ctx's cert store is global store, if yes, create a new cert store
if (own_cert_store_cache_)
cert_store = own_cert_store_cache_;
else
SSL_CTX_set_cert_store(
ctx_.get(),
// Same copy on write, but copy directly from ssl ctx (should be empty), don't load root certs
own_cert_store_cache_ = CopyX509Store(
SSL_CTX_get_cert_store(ctx_.get())
)
);
X509_STORE_add_cert(own_cert_store_cache_, x509.get())
else // If not passed, load default root certs
context.addRootCerts(); |
Most people take some time off during the holidays. We'll review it eventually. |
Will all the CAs certificates be loaded in case they are needed? |
This is also stated in the current document. If the user wants both, he can write import { createSecureContext, rootCertificates } from 'tls'
createSecureContext({
ca: [userCertificate, ...rootCertificates]
}) |
There is a ci error that seems unrelated to this commit, and I don't know how to fix it
|
This modification is mainly to optimize the startup performance of http2 and https servers (or may be tls clients's first connection?) When the user specifies a ca, it skips loading more than 100 root certificates built into node.js, and the startup speed is 15 ms faster.
In
SecureContext::SetCACert
funciton we cound get the existing cert store from the SSL context instead ofGetCertStoreOwnedByThisSecureContext()
(in which calls NewRootCertStore()) to avoid creating X509_STORE based on root_certs (more than 130), which is very slow.According to the documentation, when passing in the ca option, you do not add it to the node.js built-in root certificates but replace them, so you do not need to use the built-in root certificate to initialize the x509 store.
node/doc/api/tls.md
Lines 1984 to 2004 in 8253290
The slow loading of root certificates is a known issue of openssl3 (openssl/openssl#16871) and has not been fixed. @mhdawson
There are also related issues in node.js:
before optimization
after optimization
cc @nodejs/crypto @nodejs/tls
If anyone can help me benchmark it, I'd be very grateful.