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: optimize root cert handling during startup, store as DER #45768

Open
bnoordhuis opened this issue Dec 7, 2022 · 2 comments
Open

tls: optimize root cert handling during startup, store as DER #45768

bnoordhuis opened this issue Dec 7, 2022 · 2 comments
Labels
crypto Issues and PRs related to the crypto subsystem. performance Issues and PRs related to the performance of Node.js. tls Issues and PRs related to the tls subsystem.

Comments

@bnoordhuis
Copy link
Member

The root certificates are currently baked into the binary as PEM - basically base64-encoded binary data.

On startup, node dutifully turns each of the ~140 certifcates into a X509 instance with PEM_read_bio_X509(), which decodes the PEM to DER before passing it to d2i_X509().

You can see where this is going: it's a lot more efficient to store the certificates as DER and pass them to d2i_X509() directly.

One caveat: tls.rootCertificates is documented to be an array of PEM strings. Can be fixed by turning the DER objects into PEM in GetRootCertificates() in src/crypto/crypto_context.cc.

@bnoordhuis bnoordhuis added tls Issues and PRs related to the tls subsystem. crypto Issues and PRs related to the crypto subsystem. performance Issues and PRs related to the performance of Node.js. labels Dec 7, 2022
@krk
Copy link
Contributor

krk commented Jan 10, 2023

Performance looks like related to openssl/openssl#19119

@joyeecheung
Copy link
Member

joyeecheung commented Feb 7, 2025

Cross referencing conversations in #56843 - I was looking into it since we are now appending root certificates from system store under --use-system-ca, so it's even weirder that we convert them into PEM to append them, only to deserialize later.

@joyeecheung

By the way I wonder what we think about migrating away from tools/mk-ca-bundle.pl, I am thinking about updating it to output the certificate data in octal literals in #56832 to skip the unnecessary serdes cost, but then if we are changing it substantially, we might as well just rewrite it in JavaScript instead of invoking a Perl script from JavaScript (and the Perl script already has some modifications from our side, like omitting TrustCor CAs)

@richardlau

I think if we're not planning to resync to upstream curl's version of the tool at any point in the future (I think it was tried once and abandoned) then rewriting in something other than Perl would be a plus.

Maybe this discussion should be an issue to itself. FWIW https://blog.mozilla.org/security/2021/05/10/beware-of-applications-misusing-root-stores/ recommends https://www.ccadb.org/resources rather than parsing certdata.txt.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. performance Issues and PRs related to the performance of Node.js. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

No branches or pull requests

3 participants