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.
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
feat: added support for reading certificates from macOS system store #56599
base: main
Are you sure you want to change the base?
feat: added support for reading certificates from macOS system store #56599
Changes from 16 commits
f3c212c
884c300
57579c7
3a18890
00e9da7
1959175
7b0197f
438b6ed
da9b740
8306360
42d41cf
9cb41b0
69d176c
19d30a0
e525465
13e8647
efc303b
d4728a1
4c1a3ef
148187d
5bf95f6
1dc93ca
186378e
aea63c4
9581c7d
3ec1669
a03500d
382c7af
91869ce
7da430c
8669766
9e0041f
80fe9e2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Isn't this going against what the docs says? The certificate is only to be trusted as root certificate if trustSettings is not null but points to an empty array? i.e. this should already be covered in IsTrustSettingsTrustedForPolicy?
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.
No is trust settings won’t find it as there’s no trust settings. The validate is implementing the part of the statement must be validated by another certificate. This is for intermediate certificates that are not sent by the web server but are present in the OS keychain
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.
In that case shouldn't this be removed from the global root certificate array? And verified using
SecPolicyCreateSSL
instead on a per-connection basis? IIUC this is taking all the intermediate certificates, doing a basic check withSecPolicyCreateBasicX509
, and if it looks X509 compliant, add it to the global root certificate array, which seems unsafe..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.
SecTrustEvaluateWithError is validating that this is a trusted certificate.
Potentially, I suspect that’s what Chromium is doing although I didn’t find the code, intermediate certificates do work there
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.
As far as I can tell chromium stores intermediates here:
https://github.com/chromium/chromium/blob/main/net/cert/internal/trust_store_mac.cc#L823
The code is quite hard to follow.
When evaluating client certificates they do use
SecTrustEvaluateWithError
in order to get the full chain:https://github.com/chromium/chromium/blob/98f89988c9774d0e138a0724aa64c46187203a77/net/ssl/client_cert_store_mac.cc#L83-L84
but I can't see the same for regular certificate validation.
I think this is compliant but let me know if you think there's a better way of doing it