crypto: improve system certificate enumeration logic on macOS#62576
Draft
deepak1556 wants to merge 4 commits intonodejs:mainfrom
Draft
crypto: improve system certificate enumeration logic on macOS#62576deepak1556 wants to merge 4 commits intonodejs:mainfrom
deepak1556 wants to merge 4 commits intonodejs:mainfrom
Conversation
Collaborator
|
Review requested:
|
nodejs-github-bot
added
c++
When kSecTrustSettingsResult is absent from a trust settings dictionary, Apple specifies kSecTrustSettingsResultTrustRoot as the default value. Previously, the trust result evaluation (deny check, self-issued check, TrustAsRoot check) was inside the block that only executed when kSecTrustSettingsResult was explicitly present. When the key was absent, the function fell through to return UNSPECIFIED, incorrectly rejecting self-signed certificates that should have been trusted via the default. Move the trust result evaluation outside the conditional block so the default value of kSecTrustSettingsResultTrustRoot flows through the same code path as explicit values. This aligns with Chromium's trust_store_mac.cc implementation.
1. Fix CFRelease leak in IsTrustDictionaryTrustedForPolicy: the CFDictionaryRef returned by SecPolicyCopyProperties(policy_ref) was not released when the policy OID matched kSecPolicyAppleSSL. 2. Deduplicate certificates: SecItemCopyMatching can return the same certificate from multiple keychains. 3. Filter expired certificates.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #62576 +/- ##
==========================================
- Coverage 89.71% 89.71% -0.01%
==========================================
Files 695 695
Lines 214154 214441 +287
Branches 41009 41061 +52
==========================================
+ Hits 192132 192383 +251
- Misses 14075 14106 +31
- Partials 7947 7952 +5
🚀 New features to boost your workflow:
|
deepak1556
force-pushed
the
robo/cleanup_macos_system_cert_api
branch
from
3325bb6 to
2871528
Compare
joyeecheung
approved these changes
Apr 3, 2026
Member
There was a problem hiding this comment.
LGTM, thanks. Is it possible to add some tests? We have some integration tests in test/system-ca, see the README there for how to add the certificates and how to clean them up.
(caveat: the tests don't actually run without you doing a local mv test/system-ca/test.cfg.py test/system-ca/testcfg.py, it's currently a weird bug we rely on to not run it in the CI because the CI isn't prepped for this..)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
In VSCode we are looking to adopt the builtin system certificate api over our current implementation https://github.com/microsoft/vscode-proxy-agent/blob/59cc268f93f52fdfd0c7ac1f25aadefae74965fb/src/index.ts#L1189-L1201
As part of the rollout we observed a drop in certificate count (some machines also got 0 certs) from the builtin api and I was validating the cases with additional error telemetry https://gist.github.com/deepak1556/ef6c141e0cd3f5381cbf6f37fc590acf. It confirmed the failures are on the valid path, all the certs had no explicit trust settings and were getting filtered under the fallback
SecTrustEvaluateWithErrorpath.The changes in this PR are some cleanups and some misalign in logic with chromium implementation noticed while working on the error api. Each commit has additional context on the change.