crypto: add NODE_EXTRA_CA_CERTS to modified cert stores #44448
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
|
Review requested:
|
nodejs-github-bot
added
c++
ebickle
commented
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)); |
There was a problem hiding this comment.
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
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes the
NODE_EXTRA_CA_CERTSroot certificates being missing in a SecureContext when thecrlorpfxoptions are specified in a call totls.createSecureContext().As part of this change, specifying
NODE_EXTRA_CA_CERTSno longer causes the bundled CA store to be immediately loaded at startup. Instead, the bundled CAs will be loaded on the first call totls.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_CERTSare now loaded into theX509_STOREbefore 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