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 79.38144% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.73%. Comparing base (37e4004) to head (0a77533).
⚠️ Report is 96 commits behind head on main.

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #60678      +/-   ##
==========================================
- Coverage   89.78%   89.73%   -0.05%     
==========================================
  Files         673      675       +2     
  Lines      203775   204587     +812     
  Branches    39166    39309     +143     
==========================================
+ Hits       182956   183584     +628     
- Misses      13139    13289     +150     
- Partials     7680     7714      +34     

Files with missing lines	Coverage Δ
src/crypto/crypto_common.cc	77.47% <100.00%> (ø)
src/crypto/crypto_context.h	100.00% <ø> (ø)
src/node.cc	76.12% <ø> (+0.03%)	⬆️
src/node_options.cc	76.47% <100.00%> (-1.43%)	⬇️
src/node_options.h	97.89% <100.00%> (ø)
src/crypto/crypto_context.cc	71.59% <77.27%> (+0.75%)	⬆️

... and 67 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]

Oops sorry, I misread. Yeah I think it's still safer to pass in env to Initialize just in case it gets shuffled. (and move validation_error_ too while you are at it ;))

[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);

[Aditi-1400]

This caused several tests to crash because of the assertion failure in CleanupQueue::Add as we were trying to add a cleanup hook for every GetOrCreateRootCertStore call.
Updated this to only add a cleanup hook once, if it didn't exist already, and also extended it to clean up root_certs_from_users as well.