crypto: improve system certificate enumeration logic on macOS#62576
Open
deepak1556 wants to merge 1 commit intonodejs:mainfrom
Open
crypto: improve system certificate enumeration logic on macOS#62576deepak1556 wants to merge 1 commit intonodejs:mainfrom
deepak1556 wants to merge 1 commit intonodejs:mainfrom
Conversation
Collaborator
|
Review requested:
|
nodejs-github-bot
added
c++
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #62576 +/- ##
==========================================
+ Coverage 89.79% 89.80% +0.01%
==========================================
Files 697 699 +2
Lines 215773 216465 +692
Branches 41297 41384 +87
==========================================
+ Hits 193749 194398 +649
- Misses 14117 14155 +38
- Partials 7907 7912 +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..)
joyeecheung
approved these changes
Apr 6, 2026
github-actions
bot
removed
the
request-ci
Collaborator
Contributor
Author
|
Looks like unrelated CI failures. Seems to have been addressed by 449a93a. Rebasing. |
deepak1556
force-pushed
the
robo/cleanup_macos_system_cert_api
branch
from
46d1a2a to
5af88e7
Compare
Member
|
Looks like it got closed by the other vscode PR? |
Contributor
Author
|
Ah missed that, Thanks! |
joyeecheung
approved these changes
Apr 13, 2026
github-actions
bot
removed
the
request-ci
Collaborator
joyeecheung
added
commit-queue
joyeecheung
removed
the
commit-queue-squash
Member
|
hmm, wait, I don't think they are suitable for squashing @deepak1556 do you need them to land as separate commits or can you squash them into one? |
nodejs-github-bot
added
commit-queue-failed
Collaborator
Commit Queue failed- Loading data for nodejs/node/pull/62576 ✔ Done loading data for nodejs/node/pull/62576 ----------------------------------- PR info ------------------------------------ Title crypto: improve system certificate enumeration logic on macOS (#62576) Author Robo <hop2deep@gmail.com> (@deepak1556) Branch deepak1556:robo/cleanup_macos_system_cert_api -> nodejs:main Labels crypto, c++, needs-ci Commits 5 - crypto: fix macOS default for missing kSecTrustSettingsResult - crypto: fix leak and improve cert enumeration - crypto: add comment for SecTrustEvaluateWithError policy - src: fix formatting in crypto_context.cc - test: add macOS certificate filtering tests Committers 1 - deepak1556 <hop2deep@gmail.com> PR-URL: https://github.com/nodejs/node/pull/62576 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/62576 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day> -------------------------------------------------------------------------------- ℹ This PR was created on Fri, 03 Apr 2026 12:29:50 GMT ✔ Approvals: 2 ✔ - Joyee Cheung (@joyeecheung) (TSC): https://github.com/nodejs/node/pull/62576#pullrequestreview-4099386193 ✔ - Gürgün Dayıoğlu (@gurgunday): https://github.com/nodejs/node/pull/62576#pullrequestreview-4061571023 ✘ 1 GitHub CI job(s) failed: ✘ - test-macOS: FAILURE (https://github.com/nodejs/node/actions/runs/24142256015/job/70446614310) ℹ Last Full PR CI on 2026-04-13T14:29:35Z: https://ci.nodejs.org/job/node-test-pull-request/72671/ - Querying data for job/node-test-pull-request/72671/ ✔ Build data downloaded ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/24364295251 |
1) Fixed macOS default for missing kSecTrustSettingsResult 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. 2) Fix CFRelease leak in IsTrustDictionaryTrustedForPolicy: the CFDictionaryRef returned by SecPolicyCopyProperties(policy_ref) was not released when the policy OID matched kSecPolicyAppleSSL. 3) Deduplicate certificates: SecItemCopyMatching can return the same certificate from multiple keychains. 4) Filter expired certificates.
deepak1556
force-pushed
the
robo/cleanup_macos_system_cert_api
branch
from
5af88e7 to
1fe7f74
Compare
Contributor
Author
|
Started separate commits for review. Definitely, not required to land as individual commits. Went ahead and squashed them. Thanks! |
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
4 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.