src: fix CAs missing from secure contexts

[ebickle]

1. Adds CAs from NODE_EXTRA_CA_CERTS to root_certs_vector in node_crypto.cc so that the extra certificates are always added to SecureContext instances. The extra certificates were omitted if crl or pfx were specified as options to createSecureContext.

2. tls.rootCertificates restored to previous behavior of returning built-in Node.js certificates (+NODE_EXTRA_CA_CERTS) when --openssl-use-def-ca-store CLI option or NODE_OPENSSL_SYSTEM_CERT_PATH compiler define are set.

Fixes: #32229
Fixes: #32010
Refs: #32075

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

Notes for reviewers

• The call to X509_up_ref when building the root_certs_vector in the old NewRootCertStore() function was a double reference counting bug. X509_STORE_add_cert increments the reference count.
• Scopes were used to keep the root_certs_vector_mutex lock to an absolute minimum and avoid any unnecessary function calls within the lock.
• AddCertsFromFile was changed to AddRootCertsFromFile to make it possible to directly modify root_certs_vector. The function is not used anywhere else.
• tls.rootCertificates returning the built-in node.js root CAs when --openssl-use-def-ca-store is set was the behavior v13.11.0 and has been restored in this PR. Returning the OpenSSL certificates from the file system caused syncronous IO and leaving the behavior of v13.11.0 of returning a blank array created a high risk of a breaking change and was non-deterministic when a file system certificate was cached.
• Please carefully review changes for thread-safety. Fairly certain I got it right, but there may be some nuances to node.js multithreading I'm unaware of.

[MylesBorins]

/cc @nodejs/crypto PTAL

[MylesBorins]

@ebickle we've had some changes to CI (specifically removing the ASAN job that failed) so I rebased your PR. Just a heads up in case you go to make changes

[nodejs-github-bot]

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

[ebickle]

@ebickle we've had some changes to CI (specifically removing the ASAN job that failed) so I rebased your PR. Just a heads up in case you go to make changes

Thanks, appreciate it!

[addaleax]

ping @nodejs/crypto

[bnoordhuis]

I kind of regret addding tls.rootCertificates and signing off on that other PR if it results in complexity like this...

I have some misgivings about this PR:

1. Ad hoc locking. I don't want to have to re-review whether the locking is sound every time nearby code is changed. (The current code is pretty ad hoc too, I'll give you that.)

2. Performance. Having to lock every time the cert store is queried is a serialization bottleneck, certainly when the critical section contains things like (possibly many) calls to X509ToPEM().

I have some ideas on how to fix that but I'm going to take the Socratic approach first.

[ebickle]

1. Ad hoc locking. I don't want to have to re-review whether the locking is sound every time nearby code is changed. (The current code is pretty ad hoc too, I'll give you that.)

I agree. The issue I ran into was that all code paths involving root certificates were readonly other than UseExtraCaCerts. This ended up resulting in edge case defects with root certificate handling - e.g. #32010 and #32074.

Solving #32010 requires adding the loaded X509* objects from UseExtraCaCerts/AddRootCertsFromFile to the X509_STORE* returned by NewRootCertStore() (or similar). I could think of two options:

1. The approach from the previous PR I cancelled - Clone root_cert_store (X509_STORE*) into a secure context instead of calling NewRootCertStore(). This removes the need to have root_certs_vector and its syncronization altogether. The problem is that it's not a coding pattern OpenSSL ever intended - they expect most apps to have one and only one X509 store. There's no built-in functionality to clone a store and no way to enumerate the X509_LOOKUPs (e.g. lookups added by X509_STORE_load_locations and X509_STORE_set_default_paths).

2. Have Node.js manage a mutable list of loaded X509_STORE*. This keeps most of Node's logic in place at the cost of ending up with more manual syncronization wherever root_cert_store is touched.

Both are imperfect solutions.

Performance. Having to lock every time the cert store is queried is a serialization bottleneck, certainly when the critical section contains things like (possibly many) calls to X509ToPEM().

This was a memory/perf/code simplicity tradeoff. One alternative would be to copy the X509 pointers into a temporary local list with a tighter critical section. The loop with the X509ToPEM() calls would no longer be in the critical section.

I kind of regret adding tls.rootCertificates and signing off on that other PR if it results in complexity like this...

I'm open to reverting/undoing the other PR if it's the best option. We'd still need to look at solving #32010 separately, so quite a bit of the complexity would still remain.

tls.rootCertificates will never be able to fulfill its specification of returning the root certificates "used for verifying peer certificates" because of how OpenSSL X509_LOOKUPs are loaded on demand and cached.

The code could be reverted and documentation changed to indicate it only returns the default root certificates instead of the actual certificates used for verifying peer certificates. It could even be deprecated at that point.

Mind you, the reason tls.rootCertificates was requested in the first place was a workaround - someone wanted to add intermediate certificates at runtime without overwriting the current/default certificates. I was intending to submit a PR after this that would make tls.rootCertificates settable. This would be one option for solving the core problem (e.g. needing to add enterprise certificates at runtime) without drastically increasing node.js' API surface area.

Take a look at ebickle@b9e0b7a if you have a bit of time. The end result of that commit would be:

1. tls.rootCertificates becomes settable, allowing node's root certificates to be manipulated at runtime without performance impacts.
2. tls.rootCertificates returns the root certificates currently being used for certificate validation. If a system/disk CA store is used, tls.rootCertificates defaults to returning node's hard-coded certificates.
3. Extra CA certificates missing when certain TLS options specified #32010 is fixed and createSecureContext now properly includes any extra/changed root certificates unless overridden by an option.

[ebickle]

@bnoordhuis If you have a moment, I'm still curious what your ideas on this are. I'm willing to pull this PR or heavily modify it in favor of a different approach if it's a better for for the Node codebase :)

[ebickle]

Closing this PR since #33313 supersedes it. #32010 is still an open issue.