crypto: add extra CA certs to all secure contexts by ebickle · Pull Request #44529 · nodejs/node · GitHub

Fixes the NODE_EXTRA_CA_CERTS root certificates being missing in a SecureContext when the crl or pfx options are specified in a call to tls.createSecureContext(). This was done by loading the NODE_EXTRA_CA_CERTS into root_certs_vector, allowing them to be added to secure contexts when NewRootCertStore() is called.

As part of this change, specifying NODE_EXTRA_CA_CERTS no longer causes the bundled CA store to be immediately loaded 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. 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:

NewRootStore now takes an Environment* as a parameter. This was done so that ProcessEmitWarning could 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.
The new intent of root_certs_vector is that it should contain all certificates added to root_cert_store, not just the ones loaded from node_root_certs.h.
The existing code contained an incorrect X509_up_ref call that resulted in the X509_STORE's reference count continually increasing. This issue has been resolved.

Fixes: #32010
Refs: #40524, #23354, #20434

Review requested:

@nodejs/crypto

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

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.

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

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.

CI: https://ci.nodejs.org/job/node-test-pull-request/60586/

CI: https://ci.nodejs.org/job/node-test-pull-request/60644/

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

@pimterry Rebased back onto upstream main. Can you give it another try?

CI: https://ci.nodejs.org/job/node-test-pull-request/60724/

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 <pimterry@gmail.com>

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 <pimterry@gmail.com>

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 Sep 6, 2022

ebickle force-pushed the fix/tls-missing-extra-certificates branch from 314bada to 3857c32 Compare September 6, 2022 04:51

bnoordhuis reviewed Sep 6, 2022

View reviewed changes

Comment thread src/crypto/crypto_context.cc Outdated

Comment thread src/crypto/crypto_context.cc

Comment thread src/crypto/crypto_context.cc

Comment thread src/crypto/crypto_context.cc Outdated

This comment was marked as outdated.

Sign in to view

Trott force-pushed the main branch from 2d76238 to ca3ed36 Compare November 12, 2022 01:49

pimterry mentioned this pull request Jul 23, 2024

Documented way to add certificates to existing SecureContext #27079

Open

ebickle closed this Jul 23, 2024

ebickle deleted the fix/tls-missing-extra-certificates branch July 23, 2024 12:42

ebickle restored the fix/tls-missing-extra-certificates branch July 23, 2024 15:39

ebickle reopened this Jul 23, 2024

pimterry approved these changes Jul 24, 2024

View reviewed changes

pimterry added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 24, 2024

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 24, 2024

pimterry added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 26, 2024

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 26, 2024

pimterry approved these changes Jul 26, 2024

View reviewed changes

ebickle force-pushed the fix/tls-missing-extra-certificates branch from 53ae059 to 61b1f13 Compare July 26, 2024 15:07

pimterry added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 29, 2024

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 29, 2024

pimterry approved these changes Jul 30, 2024

View reviewed changes

pimterry closed this Jul 30, 2024

RafaelGSS mentioned this pull request Aug 19, 2024

v22.7.0 proposal #54452

Merged

targos added the dont-land-on-v20.x label Sep 21, 2024

+                // instead of replacing, so connection still succeeds.
+                copts.secureContext.context.addCACert(
+                  fixtures.readKey('ca1-cert.pem')
+                );

crypto: add extra CA certs to all secure contexts by ebickle · Pull Request #44529 · nodejs/node · GitHub

Uh oh!

Uh oh!

crypto: add extra CA certs to all secure contexts - #44529

crypto: add extra CA certs to all secure contexts#44529ebickle wants to merge 1 commit intonodejs:mainnodejs/node:mainfrom ebickle:fix/tls-missing-extra-certificatesebickle/node:fix/tls-missing-extra-certificatesCopy head branch name to clipboard

Conversation

ebickle commented Sep 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 6, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

aduh95 commented Sep 20, 2023

Uh oh!

alex1701c commented Dec 7, 2023

Uh oh!

ebickle commented Jul 23, 2024

Uh oh!

pimterry left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pimterry Jul 24, 2024

Choose a reason for hiding this comment

Uh oh!

ebickle Jul 24, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nodejs-github-bot commented Jul 24, 2024

Uh oh!

nodejs-github-bot commented Jul 26, 2024

Uh oh!

pimterry left a comment

Choose a reason for hiding this comment

Uh oh!

ebickle commented Jul 26, 2024

Uh oh!

nodejs-github-bot commented Jul 29, 2024

Uh oh!

pimterry commented Jul 30, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

crypto: add extra CA certs to all secure contexts#44529
ebickle wants to merge 1 commit into
nodejs:mainfrom
ebickle:fix/tls-missing-extra-certificates

ebickle commented Sep 6, 2022 •
edited
Loading