feat: added support for reading certificates from windows system store by twitharshil · Pull Request #44532 · nodejs/node · GitHub

Description of Change

MITM-based proxy environments like ZScaler intercepts the requests sent to a certain endpoint from the user machine and serve a redirect to their own servers ( for eg: https://gateway.zscloud.net/ ) before finally redirecting back to the original endpoint. We have observed root certificates of these proxy vendors present on the user system store. These certificates are needed for the request's SSL certificate verification.

Chrome reads certificates from the system store hence the certificate verification step passes when a request is sent out of the user machine through chrome as a client.
Node uses a statically compiled, hardcoded list of certificate authorities, rather than relying on the system's trust store, hence request going out from node as a client fails at the SSL verification step behind such environments.

This PR targets to read the certificates from the user system store and embed them with the existing list of root certificates with the node.

Note:

These changes will make a huge impact on the applications that use open source projects like electron which uses node in their networking layers.
In my current implementation, I've added logic to read the certificates from the system store and embed it with the existing list together in a single file. I am open to suggestions if we want the certificate read logic to be written somewhere else.
This PR only targets the windows platform for now. All the users from which we collected feedback were windows users only. Changes made in the PR are feature flagged behind a runtime CLI option named --node-use-system-ca

Review requested:

@nodejs/crypto

Related: #39657 (comment)

Having such functionality in node isn't completely out of the question but the default behavior should be consistent across platforms, meaning it should almost certainly be opt-in.

@bnoordhuis Thanks for taking a look at the draft PR. I've taken a quick look at the thread mentioned in your comment. It seems like for windows we can go ahead with the current approach of reading system store and implement the solution behind an opt-in flag. I believe it will be a build-time flag.

I believe it will be a build-time flag.

Why not a regular cli option?

@RaisinTen I choose a build time flag rather than a regular cli option more from my understanding so far. I see a similar use case with node which reads the default OpenSSL cert store for certificates when built with openssl-use-def-ca-store.

@twitharshil that one is actually a runtime flag accompanied by a build time flag. --use-bundled-ca and --use-openssl-ca are the runtime flags. --openssl-use-def-ca-store is the build time flag that is used to select which one of those 2 runtime flags would be enabled by default.

Hey @bnoordhuis @RaisinTen
I've opened the PR for review, PTAL. Thanks :)

Left some comments. I've commented on some of the technical aspects but I'm still on the fence as to whether it's a desirable feature.

Speaking for myself, the fact that so far it's Windows-only counts against it.

This manual formatting of a certificate is kind of questionable. If you have the certificate in DER format, you can pass it to openssl directly:

X509Pointer cert(d2i_X509(nullptr, buf, len));
if (cert) {
  // ...
}

openssl has utility functions like X509_print_ex() for formatting it as PEM, which I guess you're doing so they show up in tls.rootCertificates?

Left some comments. I've commented on some of the technical aspects, but I'm still unsure whether it's a desirable feature.

Speaking for myself, the fact that so far it's Windows-only counts against it.

@bnoordhuis I think this feature has been one of the most demanding features for quite some time now. In recent times I worked closely to support these MITM-based proxies environment only for the windows platform so I thought of upstreaming this feature to node.

Over the years no one picked up this work, so I believe supporting the windows platform at least is better than nothing? We can iteratively get this done for other platforms as well in separate PRs. I am open if someone else wants to pick this work for macOS & Linux otherwise I will pick it up once I find more time.

Hey, @bnoordhuis @jasnell I've addressed the PR comments, can I get another round of review on my PR?

Generally LGTM but this should probably be reviewed by someone more familiar with the Windows crypto API than I am.

Suggested change

      
              const HCERTSTORE hStore = CertOpenSystemStoreW(0, L"ROOT");
          
              const HCERTSTORE hStore = CertOpenSystemStoreW(nullptr, L"ROOT");

Because the first arg is a pointer, right? How likely is it for this method to fail?

Prefer something like this for readability reasons:

Suggested change

      
              while ((certificate_context_ptr = CertEnumCertificatesInStore(
          
                          hStore, certificate_context_ptr)) != nullptr) {
          
              for (;;) {
          
                certificate_context_ptr =
          
                    CertEnumCertificatesInStore(hStore, certificate_context_ptr);
          
                if (certificate_context_ptr == nullptr) break;

Suggested change


	bio.reset();

Not necessary to reset manually, the destructor does that automatically when it goes out of scope.

Suggested change

      
                      PEM_read_bio_X509(NodeBIO::NewFixed(combined_root_certs[i].c_str(),
          
                      PEM_read_bio_X509(NodeBIO::NewFixed(combined_root_certs[i].data(),

Minor optimization:

Suggested change

      
                         reinterpret_cast<const uint8_t*>(combined_root_certs[i].c_str()))
          
                         reinterpret_cast<const uint8_t*>(combined_root_certs[i].data()),
          
                         v8::NewStringType::kNormal,
          
                         combined_root_certs[i].size())

It seems this PR has stalled. @twitharshil Do you plan to finish it?

It seems this PR has stalled. @twitharshil Do you plan to finish it?

I'll take it over.

It seems this PR has stalled. @twitharshil Do you plan to finish it?

@joyeecheung in case you want to pick this up I've pushed a branch here with conflicts resolved:
https://github.com/nodejs/node/compare/main...timja:node:feature/add-support-for-reading-system-certificates-on-windows?expand=1

let me know either way

Worth taking a look at this change to openjdk and thinking about is the system store enough:
openjdk/jdk@5e5500c

https://bugs.openjdk.org/browse/JDK-6782021

There's also the Jetbrains implementation that accesses a number of stores and aggregates them:
https://github.com/JetBrains/jvm-native-trusted-roots/blob/trunk/src/main/java/org/jetbrains/nativecerts/win32/Crypt32ExtUtil.java#L30-L36

I've had confirmation that the jetbrains one doesn't work with intermediate certificates, awaiting confirmation with the OpenJDK one.

To make matters a bit more complex - Windows has this feature of revoking certificates through the "disallowed" store, that Chromium implements, for example https://github.com/chromium/chromium/blob/a5cf86ae718b86764946713e7abae12f1fa42d08/net/cert/internal/trust_store_win.cc#L334

Though I feel that for an initial implementation, not supporting that is fine because apparently many runtimes do not support it either (also we do not respect that when using the bundled certificates, anyway)

@timja I opened #56833 which rewrites the code a bit to make it easier to add more certificates other than the root CA of the current user (what this PR allows). There is a pretty extensive list of stores that Chromium considers that is documented here https://chromium.googlesource.com/chromium/src/+/main/net/data/ssl/chrome_root_store/faq.md#does-the-chrome-certificate-verifier-consider-local-trust-decisions - maybe it's not necessary to implement them all in the first iteration, but it would be nice to follow them all eventually. And to match what Chromium does we should also check that the certificates are configured for TLS use, which I left a TODO there as well, will look into that tomorrow before I open the PR for review.

@joyeecheung / someone mind closing this? It was resolved as part of #56833

#56833 has already landed and released, so I'll close this. Thanks for initiating the work Harshil! :)

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

twitharshil force-pushed the feature/add-support-for-reading-system-certificates-on-windows branch from 285a83d to f35fe3c Compare September 6, 2022 07:34

twitharshil marked this pull request as ready for review October 6, 2022 13:11

twitharshil force-pushed the feature/add-support-for-reading-system-certificates-on-windows branch 6 times, most recently from 193496f to 87a7def Compare October 10, 2022 05:21

bnoordhuis reviewed Oct 10, 2022

View reviewed changes

jasnell reviewed Oct 16, 2022

View reviewed changes

Comment thread doc/api/cli.md Outdated

jasnell reviewed Oct 16, 2022

View reviewed changes

Comment thread src/crypto/crypto_context.cc Outdated

twitharshil force-pushed the feature/add-support-for-reading-system-certificates-on-windows branch 4 times, most recently from 8fce134 to f61d113 Compare October 19, 2022 17:53

jasnell reviewed Oct 19, 2022

View reviewed changes

Comment thread doc/api/cli.md Outdated

twitharshil force-pushed the feature/add-support-for-reading-system-certificates-on-windows branch 3 times, most recently from fed9bf6 to 012f11e Compare October 20, 2022 05:43

twitharshil force-pushed the feature/add-support-for-reading-system-certificates-on-windows branch 3 times, most recently from 52fbf17 to 5082ab3 Compare October 21, 2022 08:27

twitharshil force-pushed the feature/add-support-for-reading-system-certificates-on-windows branch from 5082ab3 to 1b0265f Compare November 2, 2022 07:52

twitharshil added 2 commits November 2, 2022 13:25

crypto: added support for reading system store certificates in windows

716aecf

crypto: added documentation for the newer node option

9d628a7

twitharshil force-pushed the feature/add-support-for-reading-system-certificates-on-windows branch 2 times, most recently from 8493223 to f17bec7 Compare November 2, 2022 10:28

crypto: addressed PR comments

00f666a

twitharshil force-pushed the feature/add-support-for-reading-system-certificates-on-windows branch from 69d7cff to 00f666a Compare November 3, 2022 07:52

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

bnoordhuis reviewed Nov 19, 2022

View reviewed changes

bnoordhuis mentioned this pull request Dec 29, 2022

Allow Node to use certificates from the macOS Keychain when making HTTPS requests #39657

Closed

timja mentioned this pull request Jan 14, 2025

feat: added support for reading certificates from macOS system store #56599

Merged

3 tasks

timja reviewed Jan 29, 2025

View reviewed changes

joyeecheung mentioned this pull request Jan 30, 2025

crypto: support --use-system-ca on Windows #56833

Merged

RaisinTen closed this Mar 13, 2025

+              void ReadSystemStoreCertificates(
+                  std::vector<std::string>* system_root_certificates) {
+              #ifdef _WIN32
+                const HCERTSTORE hStore = CertOpenSystemStoreW(0, L"ROOT");

-                        PEM_read_bio_X509(NodeBIO::NewFixed(root_certs[i],
-                                                            strlen(root_certs[i])).get(),
-                                          nullptr,   // no re-use of X509 structure
+                        PEM_read_bio_X509(NodeBIO::NewFixed(combined_root_certs[i].c_str(),

-                          reinterpret_cast<const uint8_t*>(root_certs[i]))
-                          .ToLocal(&result[i])) {
+                           env->isolate(),
+                           reinterpret_cast<const uint8_t*>(combined_root_certs[i].c_str()))

+              void ReadSystemStoreCertificates(
+                  std::vector<std::string>* system_root_certificates) {
+              #ifdef _WIN32
+                const HCERTSTORE hStore = CertOpenSystemStoreW(0, L"ROOT");

	const HCERTSTORE hStore = CertOpenSystemStoreW(0, L"ROOT");
	const HCERTSTORE hStore = CertOpenSystemStoreW(nullptr, L"ROOT");

		while ((certificate_context_ptr = CertEnumCertificatesInStore(
		hStore, certificate_context_ptr)) != nullptr) {

	PEM_read_bio_X509(NodeBIO::NewFixed(combined_root_certs[i].c_str(),
	PEM_read_bio_X509(NodeBIO::NewFixed(combined_root_certs[i].data(),

feat: added support for reading certificates from windows system store by twitharshil · Pull Request #44532 · nodejs/node · GitHub

Uh oh!

Uh oh!

feat: added support for reading certificates from windows system store - #44532

Conversation

twitharshil 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!

bnoordhuis commented Sep 6, 2022

Uh oh!

twitharshil commented Sep 12, 2022

Uh oh!

RaisinTen commented Sep 12, 2022

Uh oh!

twitharshil commented Sep 15, 2022

Uh oh!

RaisinTen commented Sep 19, 2022

Uh oh!

twitharshil commented Oct 10, 2022

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bnoordhuis Oct 10, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

twitharshil commented Oct 19, 2022

Uh oh!

Uh oh!

twitharshil commented Nov 16, 2022

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Nov 19, 2022

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Nov 19, 2022

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Nov 19, 2022

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Nov 19, 2022

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Nov 19, 2022

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Jan 8, 2025

Uh oh!

timja commented Jan 28, 2025

Uh oh!

timja commented Jan 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

timja Jan 29, 2025

Choose a reason for hiding this comment

Uh oh!

joyeecheung Jan 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Jan 30, 2025

Uh oh!

timja commented Feb 27, 2025

Uh oh!

RaisinTen commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

twitharshil commented Sep 6, 2022 •
edited
Loading

timja commented Jan 29, 2025 •
edited
Loading

joyeecheung Jan 29, 2025 •
edited
Loading

RaisinTen commented Mar 13, 2025 •
edited
Loading