crypto: add tls.useSystemCA()

[joyeecheung]

This API allows enabling system CA certificates to be used by the Node.js TLS clients by default. This is equivalent to enabling the --use-system-ca flag, but can be done programmatically at runtime.

Once called, the system CA certificates will be included in the default CA certificate list returned by tls.getCACertificates() and used by TLS connections that don't specify their own CA certificates.

Subsequent calls to this function are no-ops. The system CA certificates are loaded and cached on the first call.

This function only affects the current Node.js thread to avoid races.

Background

This API serves the use case where another party (e.g. corporate infra packages) would need to allow system certificates to be enabled when they are only to be loaded by application developers (who can start tls connections themselves, or use a third-party library that does it) and have no control over the command line.

Note that the functionality provided by this API already is possible via monkey patching tls or the global HTTPS agent, this just provides a more secure way to enable system certificates in the process. User land has already been doing it with a less safe approach, for example, see syswide-cas, win-ca, ssl-root-cas. I am fairly certain when none of the existing options work there are applications/packages that would just go a nuclear route and use rejectUnauthorized: false in the monkey-patched option bag to avoid whatever woes they have, considering how often it shows up on the Internet and on even public GitHub. I think providing an option to allow using system certificates dynamically is at least a lot safer than what people are already doing, since the system certificates tend to be managed in a much more secure fashion.

[nodejs-github-bot]

Review requested:

• @nodejs/crypto
• @nodejs/net

[github-actions]

The notable-change PRs with changes that should be highlighted in changelogs. label has been added by @joyeecheung.

Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section.

[nodejs-github-bot]

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

[codecov]

Codecov Report

Attention: Patch coverage is 94.11765% with 2 lines in your changes missing coverage. Please review.

Project coverage is 90.09%. Comparing base (5584cc5) to head (51ddd68).
Report is 41 commits behind head on main.

Files with missing lines	Patch %	Lines
src/crypto/crypto_context.cc	87.50%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #58822      +/-   ##
==========================================
- Coverage   90.13%   90.09%   -0.05%     
==========================================
  Files         639      640       +1     
  Lines      188192   188394     +202     
  Branches    36916    36940      +24     
==========================================
+ Hits       169633   169728      +95     
- Misses      11324    11366      +42     
- Partials     7235     7300      +65     

Files with missing lines	Coverage Δ
lib/tls.js	96.10% <100.00%> (+0.16%)	⬆️
src/crypto/crypto_context.cc	69.77% <87.50%> (+0.51%)	⬆️

... and 69 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[jasnell]

I think I'd much prefer this to be set as an option on an individual connection rather than programmatically impacting all connections. A cli flag is one thing because it's set by the individual running the app. This API could be set by dependencies impacting global state without the application being aware of it. Setting it per connection seems the safest.

[pimterry]

Agreed, I'm not sure introducing more mutable global state here is worthwhile I'm afraid.

I'd much prefer this to be set as an option on an individual connection rather than programmatically impacting all connections.

I think that's already possible with the other recent improvements around this:

{
  ca: tls.getCACertificates('system')
}

AFAICT, adding that to TLS options anywhere should be equivalent to this change, but scoped to the single context.

I think with the current global CLI flag plus easy per-connection configuration, that should cover most use cases.

[joyeecheung]

As explained, the per-connection option already exists.

This API serves a different use case, where another party (e.g. corporate infra packages) would need to allow system certificates to be enabled when they are only to be loaded by application developers (who can start tls connections themselves, or use a third-party library that does it) and have no control over the command line.

This API could be set by dependencies impacting global state without the application being aware of it.

This is already possible by monkey patching tls or the global HTTPS agent - for example, see https://www.npmjs.com/package/syswide-cas. If both the ca option and NODE_EXTRA_CA_CERTS exist but a package like this still gets 14K+ weekly downloads, that means the existing API surfaces are not enough to cover them. I am fairly certain there are applications/packages that would just go a nuclear route and use rejectUnauthorized: false in the monkey-patched option bag when the woes they are having can't be fixed with existing options, considering how often it shows up on the Internet and on even public GitHub. Providing an option to allow using system certificates dynamically is at least a lot safer than what people are already doing, since the system certificates tend to be managed in a much more secure fashion. I can put that into the PR description, if it's not clear.

[joyeecheung]

Actually there are more packages monkey patching tls/https agent to do this..(sigh):

https://www.npmjs.com/package/win-ca
https://www.npmjs.com/package/ssl-root-cas

[jasnell]

I won't block but I'm still unconvinced this is a good thing to add.

[pimterry]

This API serves a different use case

@joyeecheung That corporate infra use case makes sense. I don't think the library argument is that strong (they all predate the other new improvements you'd made here to cover this, and could easily be a symptom of lack of awareness of new APIs and/or inertia) but it makes sense to include this (runtime global CA configuration) if somebody concrete is asking for it despite the other new APIs.

This API feels like a strange state change though: it's a one-way toggle, that can easily be switched invisibly & unexpectedly in dependencies, which only supports one type of runtime change, and which doesn't act like any other APIs we have (AFAIK). It's not normal global mutable state - it's a runtime-activated CLI flag.

What do you think about an alternative more along the lines of http.globalAgent? A writable property to globally control the TLS defaults.

The easiest direct solution in that vein would be to make tls.rootCertificates directly configurable. I think ESM means it's awkward to make that writable directly, but an API like tls.setDefaultCACertificates(tls.getCACertificates('system')) would be workable. That seems more idiomatic & well-behaved to me, achieves the same result, and is more flexible to allow for globally configuring CAs in other ways too (e.g. adding a single extra CA at runtime - which likely has plenty of overlap in corporate use cases). Is there a reason that's not possible?

We could even do that more generally for all TLS defaults... A tls.setDefaultOptions() method to configure a default used by all secure contexts has other interesting use cases (globally set minimum TLS version, OpenSSL options, TLS timeouts, etc) and would cover this neatly too. Or we could stick with just root certs separately if that's too bold/complicated for now.

[joyeecheung]

The easiest direct solution in that vein would be to make tls.rootCertificates directly configurable. I think ESM means it's awkward to make that writable directly, but an API like tls.setDefaultCACertificates(tls.getCACertificates('system'))

That is a good idea, I think I'll need to look into it a bit further to confirm this is implementable & safe in async operations, but it sounds better than the current API to me.

[joyeecheung]

Or we could stick with just root certs separately if that's too bold/complicated for now.

Personally I'd be more comfortable to just stick to root certs considering the option bags are cherry-picked and copied into various instances, with everything being monkey-patchable and visible to user land, so it seems tricky to ensure consistency. The root cert store lives completely in C++ land so is safer from user tampering.

As indicated in comments, I generally don't think this is a good thing to add but don't feel strongly enough about it to block.