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

crypto: add NODE_EXTRA_CA_CERTS to modified cert stores #44448

Closed

Conversation

ebickle
Copy link
Contributor

@ebickle ebickle commented Aug 30, 2022

Fixes the NODE_EXTRA_CA_CERTS root certificates being missing in a SecureContext when the crl or pfx options are specified in a call totls.createSecureContext().

As part of this change, specifying NODE_EXTRA_CA_CERTS no longer causes the bundled CA store to be immediately loaded at startup. Instead, the bundled CAs will be loaded on the first call to tls.createSecureContext(), the same as how Node.js works by default. This improves startup performance and partially mitigates issue #40524.

Due to the deferred bundled CA loading described above, the NODE_EXTRA_CA_CERTS are now loaded into the X509_STORE before the bundled certificates instead of after. Please let me know if this creates a risk of a breaking change; I opted for simpler logic for the initial PR.

Fixes: #32010
Refs: #40524

Store loaded NODE_EXTRA_CA_CERTS into root_certs_vector, allowing
them to be added to secure contexts when NewRootCertStore() is
called.

When NODE_EXTRA_CA_CERTS is specified, the bundled root certificates
will no longer be preloaded at startup. This improves Node.js
startup time and makes the behavior of NODE_EXTRA_CA_CERTS consistent
with the default behavior when NODE_EXTRA_CA_CERTS is ommitted.

Fixes: nodejs#32010
Refs: nodejs#40524
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels Aug 30, 2022

// Parse errors from the built-in roots are fatal.
CHECK_NOT_NULL(x509);

root_certs_vector.push_back(x509);
root_certs_vector.push_back(std::move(x509));
Copy link
Contributor Author

@ebickle ebickle Aug 30, 2022

Choose a reason for hiding this comment

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

If NODE_EXTRA_CA_CERTS was specified at startup, the roots_certs_vector will not be empty at this point. This change causes the "extra CAs" to be loaded before the "bundled CAs" - the opposite of today.

My expectation is that this won't cause a problem since we wouldn't expect an extra CA to "spoof" a real one, but please review carefully in case I'm wrong. 😊

@ebickle ebickle changed the title src: add NODE_EXTRA_CA_CERTS to modified cert stores crypto: add NODE_EXTRA_CA_CERTS to modified cert stores Aug 30, 2022
@ebickle ebickle closed this Aug 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extra CA certificates missing when certain TLS options specified
2 participants