Make --use-system-ca per-env rather than per-process

[Aditi-1400]

Makes the --use-system-ca option a per-environment option rather than a per-process option so that workers can enable/disable them individually

[nodejs-github-bot]

Review requested:

• @nodejs/config
• @nodejs/crypto
• @nodejs/startup

[joyeecheung]

Hmm, I think this may need more work than just updating the options - the implication of being per-env is that each worker would then be able to toggle this independently. Say when the main thread does not enable it but a worker does, then the worker will have the system CA certs in their default store but the parent doesn't. Can you add a test for this, and the other way around (parent enables it, worker disables it)? My impression is that the default store initialisation code is not yet ready for this and it's still shared across the process (so if a worker enables it, suddenly the parent get it too, which would be unexpected).

[codecov]

Codecov Report

❌ Patch coverage is 75.00000% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.76%. Comparing base (37e4004) to head (aa9f038).
⚠️ Report is 37 commits behind head on main.

Files with missing lines	Patch %	Lines
src/crypto/crypto_context.cc	71.64%	9 Missing and 10 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #60678      +/-   ##
==========================================
- Coverage   89.78%   89.76%   -0.02%     
==========================================
  Files         673      674       +1     
  Lines      203775   204104     +329     
  Branches    39166    39230      +64     
==========================================
+ Hits       182956   183221     +265     
- Misses      13139    13205      +66     
+ Partials     7680     7678       -2     

Files with missing lines	Coverage Δ
src/crypto/crypto_common.cc	77.47% <100.00%> (ø)
src/crypto/crypto_context.h	100.00% <ø> (ø)
src/node.cc	75.93% <ø> (-0.16%)	⬇️
src/node_options.cc	76.37% <100.00%> (-1.53%)	⬇️
src/node_options.h	97.89% <100.00%> (ø)
src/crypto/crypto_context.cc	71.17% <71.64%> (+0.34%)	⬆️

... and 61 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.

[joyeecheung]

Can you add some tests to test/parallel that checks this affects tls.getCACertificates('default') in a worker if tls.getCACertificates('system') returns non-empty in a parent? (if there are no system CAs in the testing machine, then the test can be skipped - that way it can run even without the mock certificates installed)

[joyeecheung]

I think this initialization is buggy, env_ is declared after ctx_, and member initialization order follows declaration order, not the order in this list, so in Initialize(), env_ is not initialized - this can probably be fixed by just passing the env into Initialize instead (which I think is better because it's more explicit), or just switch the declaration order.

[Aditi-1400]

Hmm, from what I see, the env_ is declared before the ctx_. Although, I guess std::string validation_error_ = ""; should be moved above ctx_. But if you prefer, I can change this to pass env into Initialise instead

https://github.com/nodejs/node/pull/60678/files#diff-60a9b54a19f29bc293435249907efb06e7b900bb4b1036b967634d2fbcf18340R273-R278

[joyeecheung]

I think this now leaks a X509_STORE for every worker upon shutdown. Previously this was okay because there's only ever one store in the process, but now they come and go with workers. Also while Node.js executable doesn't do this usually, embedders (including cctests) reuse a thread for different Environments, so the store may go stale when the thread gets reused by another Environment. This probably fixes it:

Suggested change

	root_cert_store = NewRootCertStore(env);
	root_cert_store = NewRootCertStore(env);
	env->AddCleanupHook([](void* arg) {
	if (root_cert_store != nullptr) {
	X509_STORE_free(root_cert_store);
	root_cert_store = nullptr;
	}
	}, nullptr);