crypto: add extra CA certs to all secure contexts #44529
Conversation
|
Review requested:
|
nodejs-github-bot
added
c++
ebickle
force-pushed
the
fix/tls-missing-extra-certificates
branch
from
314bada
to
3857c32
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
Ping @nodejs/crypto for reviews |
|
Would this also work with electron? I ask regarding electron/electron#10257 |
|
@nodejs/crypto I've resolved the conflicts in this PR - it's ready for review again ccing @pimterry |
There was a problem hiding this comment.
Some minor nitpicking about test details but nothing critical (up to you if/how you want to do those). The implementation itself looks great to me 👍
Would be nice to have a review from somebody else from @nodejs/crypto who is a bit more familiar with this if we can, since the core TLS trust store setup is quite sensitive code. Let's give it a few days for anybody else to chime in.
| // instead of replacing, so connection still succeeds. | ||
| copts.secureContext.context.addCACert( | ||
| fixtures.readKey('ca1-cert.pem') | ||
| ); |
There was a problem hiding this comment.
It would be better if we did this without directly messing with the context like this, but I understand there's no usable API for additional CAs right now - this is really just a note that we should update this as part of adding that API in #27079
There was a problem hiding this comment.
Agree completely. So long as we land the "additional CA" functionality shortly after and then replace this test, temporarily relying on the 'internal' context behavior makes sense to ensure we have proper coverage in the mean time.
github-actions
bot
removed
the
request-ci
github-actions
bot
removed
the
request-ci
There was a problem hiding this comment.
Test changes look good, thanks 👍
CI is failing because of a conflict with main (https://ci.nodejs.org/job/node-test-commit/72499/console) can you rebase?
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 root certificates (both bundled and extra) 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 omitted. The original reason NODE_EXTRA_CA_CERTS were loaded at startup (issues nodejs#20432, nodejs#20434) was to prevent the environment variable from being changed at runtime. This change preserves the runtime consistency without actually having to load the certs at startup. Fixes: nodejs#32010 Refs: nodejs#40524 Refs: nodejs#23354
ebickle
force-pushed
the
fix/tls-missing-extra-certificates
branch
from
53ae059
to
61b1f13
Compare
|
@pimterry Rebased back onto upstream main. Can you give it another try? |
github-actions
bot
removed
the
request-ci
Store loaded NODE_EXTRA_CA_CERTS into root_certs_vector, allowing them to be added to secure contexts when NewRootCertStore() is called, rather than losing them when unrelated options are provided. When NODE_EXTRA_CA_CERTS is specified, the root certificates (both bundled and extra) 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 omitted. The original reason NODE_EXTRA_CA_CERTS were loaded at startup (issues #20432, #20434) was to prevent the environment variable from being changed at runtime. This change preserves the runtime consistency without actually having to load the certs at startup. Fixes: #32010 Refs: #40524 Refs: #23354 PR-URL: #44529 Reviewed-By: Tim Perry <[email protected]>
|
Landed in 7485ad8 |
Store loaded NODE_EXTRA_CA_CERTS into root_certs_vector, allowing them to be added to secure contexts when NewRootCertStore() is called, rather than losing them when unrelated options are provided. When NODE_EXTRA_CA_CERTS is specified, the root certificates (both bundled and extra) 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 omitted. The original reason NODE_EXTRA_CA_CERTS were loaded at startup (issues #20432, #20434) was to prevent the environment variable from being changed at runtime. This change preserves the runtime consistency without actually having to load the certs at startup. Fixes: #32010 Refs: #40524 Refs: #23354 PR-URL: #44529 Reviewed-By: Tim Perry <[email protected]>
targos
added
the
dont-land-on-v20.x
Fixes the
NODE_EXTRA_CA_CERTSroot certificates being missing in a SecureContext when thecrlorpfxoptions are specified in a call totls.createSecureContext(). This was done by loading theNODE_EXTRA_CA_CERTSintoroot_certs_vector, allowing them to be added to secure contexts whenNewRootCertStore()is called.As part of this change, specifying
NODE_EXTRA_CA_CERTSno longer causes the bundled CA store to be immediately loaded at startup. This improves Node.js startup time and makes the behavior ofNODE_EXTRA_CA_CERTSconsistent with the default behavior whenNODE_EXTRA_CA_CERTSis omitted. Although this change effectively reverts #20434, it does not reintroduce issue #20432 because the environment variable is read at startup; modifying it at runtime has no effect.Notes for code reviewers:
NewRootStorenow takes anEnvironment*as a parameter. This was done so thatProcessEmitWarningcould be called when the extra certificates could not be loaded. As a bonus, the warning can now be programatically read via the process warning event.root_certs_vectoris that it should contain all certificates added toroot_cert_store, not just the ones loaded fromnode_root_certs.h.X509_up_refcall that resulted in theX509_STORE's reference count continually increasing. This issue has been resolved.Fixes: #32010
Refs: #40524, #23354, #20434