-
Notifications
You must be signed in to change notification settings - Fork 30.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
crypto: support --use-system-ca on Windows #56833
base: main
Are you sure you want to change the base?
Conversation
Review requested:
|
The X509 structures are never freed. Use ncrypto::X509Pointer to manage it automatically and move the X509* to PEM conversion into a helper to be reused by integration in other systems.
c007259
to
dc737dd
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This patch adds support for --use-system-ca on Windows, the certificates are collected following Chromium's policy, though the following are left as TODO and out of this patch. - Support for user-added intermediate certificates - Support for distrusted certificates Since those aren't typically supported by other runtimes/tools either, and what's implemented in this patch is sufficient for enough use cases already.
dc737dd
to
faf7592
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #56833 +/- ##
=======================================
Coverage 89.20% 89.21%
=======================================
Files 663 663
Lines 192012 192028 +16
Branches 36929 36931 +2
=======================================
+ Hits 171286 171319 +33
- Misses 13582 13595 +13
+ Partials 7144 7114 -30
|
Nice work! What sort of testing have you done on this? - I see the integration test added I'll look to get someone on windows behind our ZScaler enterprise setup next week to test it. (and potentially with https://github.com/timja/openjdk-intermediate-ca-reproducer both root -> leaf and root -> intermediate -> leaf, unless you've tested something similar) |
So far only the test I modified in test-native-certs.mjs - basically the same as the macOS test, just running it first to see that it fails, then adding that certificate locally to Windows, run the test and see that it works, and removing that certificate. From what I can tell the real-world use cases I have only require the root CA certificates from either local machine or the current users to be trusted. I will try verifying it a bit on my end. |
This patch adds support for --use-system-ca on Windows, the
certificates are collected following Chromium's policy,
though the following are left as TODO and out of this patch.
Since those aren't typically supported by other runtimes/tools
either, and what's implemented in this patch is sufficient for
enough use cases already.
This PR re-implements #44532 but is based on the support added for macOS in #56599 with the following modifications to match Chromium's policy:
The first commit comes from #56832 to fix a leak from the macOS PR.