GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — feat: added support for reading certificates from windows system store by twitharshil · Pull Request #44532 · nodejs/node

Conversation

@twitharshil

@twitharshil twitharshil commented Sep 6, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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:

  1. These changes will make a huge impact on the applications that use open source projects like electron which uses node in their networking layers.

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
tony-go and NVolcz reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@nodejs-github-bot

nodejs-github-bot commented Sep 6, 2022

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/crypto
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot 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
twitharshil force-pushed the feature/add-support-for-reading-system-certificates-on-windows branch from 285a83d to f35fe3c Compare September 6, 2022 07:34
@bnoordhuis

bnoordhuis commented Sep 6, 2022

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@twitharshil

twitharshil commented Sep 12, 2022

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@RaisinTen

RaisinTen commented Sep 12, 2022

Copy link
Copy Markdown
Member

I believe it will be a build-time flag.

Why not a regular cli option?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@twitharshil

twitharshil commented Sep 15, 2022

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@RaisinTen

RaisinTen commented Sep 19, 2022

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@twitharshil
twitharshil marked this pull request as ready for review October 6, 2022 13:11
@twitharshil
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
@twitharshil

twitharshil commented Oct 10, 2022

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bnoordhuis
bnoordhuis reviewed Oct 10, 2022
View reviewed changes

@bnoordhuis bnoordhuis left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/crypto/crypto_context.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/crypto/crypto_context.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/crypto/crypto_context.cc Outdated

@bnoordhuis bnoordhuis Oct 10, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/parallel/test-cli-node-print-help.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
jasnell
jasnell reviewed Oct 16, 2022
View reviewed changes
Comment thread doc/api/cli.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
jasnell
jasnell reviewed Oct 16, 2022
View reviewed changes
Comment thread src/crypto/crypto_context.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
@twitharshil

twitharshil commented Oct 19, 2022

Copy link
Copy Markdown
Contributor Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@twitharshil
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
jasnell reviewed Oct 19, 2022
View reviewed changes
Comment thread doc/api/cli.md Resolved

Uh oh!

There was an error while loading. Please reload this page.
@twitharshil
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
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
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
@twitharshil
crypto: added support for reading system store certificates in windows
716aecf
@twitharshil
crypto: added documentation for the newer node option
9d628a7
@twitharshil
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
@twitharshil
crypto: addressed PR comments
00f666a
@twitharshil
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
Trott force-pushed the main branch from 2d76238 to ca3ed36 Compare November 12, 2022 01:49
@twitharshil

twitharshil commented Nov 16, 2022

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bnoordhuis
bnoordhuis reviewed Nov 19, 2022
View reviewed changes

@bnoordhuis bnoordhuis left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/crypto/crypto_context.cc
void ReadSystemStoreCertificates(
std::vector<std::string>* system_root_certificates) {
#ifdef _WIN32
const HCERTSTORE hStore = CertOpenSystemStoreW(0, L"ROOT");

@bnoordhuis bnoordhuis Nov 19, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/crypto/crypto_context.cc
Comment on lines +213 to +214
while ((certificate_context_ptr = CertEnumCertificatesInStore(
hStore, certificate_context_ptr)) != nullptr) {

@bnoordhuis bnoordhuis Nov 19, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/crypto/crypto_context.cc
Comment on lines +258 to +259

bio.reset();

@bnoordhuis bnoordhuis Nov 19, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
bio.reset();

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread 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(),

@bnoordhuis bnoordhuis Nov 19, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread 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()))

@bnoordhuis bnoordhuis Nov 19, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bnoordhuis bnoordhuis mentioned this pull request Dec 29, 2022
Allow Node to use certificates from the macOS Keychain when making HTTPS requests #39657
Closed
@joyeecheung

joyeecheung commented Jan 8, 2025

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@timja timja mentioned this pull request Jan 14, 2025
feat: added support for reading certificates from macOS system store #56599
Merged
3 tasks done
@timja

timja commented Jan 28, 2025

Copy link
Copy Markdown
Contributor

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

I'll take it over.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@timja

timja commented Jan 29, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
timja
timja reviewed Jan 29, 2025
View reviewed changes
Comment thread src/crypto/crypto_context.cc
void ReadSystemStoreCertificates(
std::vector<std::string>* system_root_certificates) {
#ifdef _WIN32
const HCERTSTORE hStore = CertOpenSystemStoreW(0, L"ROOT");

@timja timja Jan 29, 2025

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@joyeecheung joyeecheung Jan 29, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
timja reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@joyeecheung joyeecheung mentioned this pull request Jan 30, 2025
crypto: support --use-system-ca on Windows #56833
Merged
@joyeecheung

joyeecheung commented Jan 30, 2025

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@timja

timja commented Feb 27, 2025

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@RaisinTen

RaisinTen commented Mar 13, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@RaisinTen RaisinTen closed this Mar 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuis bnoordhuis bnoordhuis left review comments
@jasnell jasnell jasnell left review comments
@joyeecheung joyeecheung joyeecheung left review comments
+1 more reviewer
@timja timja timja left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@twitharshil @nodejs-github-bot @bnoordhuis @RaisinTen @joyeecheung @timja @jasnell
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.