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

twitharshil · 2022-09-06T07:31:40Z

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

nodejs-github-bot · 2022-09-06T07:31:44Z

Review requested:

@nodejs/crypto

bnoordhuis · 2022-09-06T21:43:51Z

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.

twitharshil · 2022-09-12T11:56:58Z

@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.

RaisinTen · 2022-09-12T12:13:13Z

I believe it will be a build-time flag.

Why not a regular cli option?

twitharshil · 2022-09-15T11:59:12Z

@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.

RaisinTen · 2022-09-19T08:46:44Z

@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.

twitharshil · 2022-10-10T08:34:05Z

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

bnoordhuis

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.

src/crypto/crypto_context.cc

bnoordhuis · 2022-10-10T11:04:30Z

src/crypto/crypto_context.cc

+    }
+
+    base64_string_output = "-----BEGIN CERTIFICATE-----\n" +
+                           base64_string_output + "\n-----END CERTIFICATE-----";


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?

test/parallel/test-cli-node-print-help.js

doc/api/cli.md

src/crypto/crypto_context.cc

twitharshil · 2022-10-19T10:04:27Z

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.

doc/api/cli.md

twitharshil · 2022-11-16T16:57:24Z

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

bnoordhuis

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

bnoordhuis · 2022-11-19T09:50:37Z

src/crypto/crypto_context.cc

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


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?

bnoordhuis · 2022-11-19T09:53:23Z

src/crypto/crypto_context.cc

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


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;

bnoordhuis · 2022-11-19T09:57:13Z

src/crypto/crypto_context.cc

+
+    bio.reset();


Suggested change

bio.reset();

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

bnoordhuis · 2022-11-19T09:58:26Z

src/crypto/crypto_context.cc

-          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(),


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(),

bnoordhuis · 2022-11-19T10:00:52Z

src/crypto/crypto_context.cc

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


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())

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

doc/api/cli.md Outdated Show resolved Hide resolved

jasnell reviewed Oct 16, 2022

View reviewed changes

src/crypto/crypto_context.cc Show resolved Hide resolved

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

doc/api/cli.md Show resolved Hide resolved

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

twitharshil commented Sep 6, 2022 •

edited

Loading

nodejs-github-bot commented Sep 6, 2022

bnoordhuis commented Sep 6, 2022

twitharshil commented Sep 12, 2022

RaisinTen commented Sep 12, 2022

twitharshil commented Sep 15, 2022

RaisinTen commented Sep 19, 2022

twitharshil commented Oct 10, 2022

bnoordhuis left a comment

bnoordhuis Oct 10, 2022

twitharshil commented Oct 19, 2022

twitharshil commented Nov 16, 2022

bnoordhuis left a comment

bnoordhuis Nov 19, 2022

bnoordhuis Nov 19, 2022

bnoordhuis Nov 19, 2022

bnoordhuis Nov 19, 2022

bnoordhuis Nov 19, 2022

	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(),

-             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())

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

Are you sure you want to change the base?

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

Conversation

twitharshil commented Sep 6, 2022 • edited Loading

nodejs-github-bot commented Sep 6, 2022

bnoordhuis commented Sep 6, 2022

twitharshil commented Sep 12, 2022

RaisinTen commented Sep 12, 2022

twitharshil commented Sep 15, 2022

RaisinTen commented Sep 19, 2022

twitharshil commented Oct 10, 2022

bnoordhuis left a comment

Choose a reason for hiding this comment

bnoordhuis Oct 10, 2022

Choose a reason for hiding this comment

twitharshil commented Oct 19, 2022

twitharshil commented Nov 16, 2022

bnoordhuis left a comment

Choose a reason for hiding this comment

bnoordhuis Nov 19, 2022

Choose a reason for hiding this comment

bnoordhuis Nov 19, 2022

Choose a reason for hiding this comment

bnoordhuis Nov 19, 2022

Choose a reason for hiding this comment

bnoordhuis Nov 19, 2022

Choose a reason for hiding this comment

bnoordhuis Nov 19, 2022

Choose a reason for hiding this comment

twitharshil commented Sep 6, 2022 •

edited

Loading