Skip to content
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

Jump to bottom

Add SecTrustSettings bindings #82

Merged
merged 6 commits into from Sep 15, 2019
Merged

Add SecTrustSettings bindings #82

merged 6 commits into from Sep 15, 2019

Conversation

ctz
Copy link
Contributor

@ctz ctz commented Sep 13, 2019

This allows a consuming crate to enumerate macos trusted certificates via SecTrustSettingsCopyCertificates, and then learn whether they're suitable for use in TLS via SecTrustSettingsCopyTrustSettings.

I've done some manual testing of this by twiddling keychain trust settings and observing the results.

Sample calling code is here if you're interested: https://github.com/ctz/rustls-native-certs/blob/master/src/macos.rs

kornelski
kornelski requested changes Sep 15, 2019
View reviewed changes
Copy link
Owner

@kornelski kornelski left a comment

Choose a reason for hiding this comment

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

Isn't fail-open behavior of tls_trust_settings_for_certificate concerning? I see it's inherited from macOS, but maybe the function could return None if there are no explicit settings and leave it to the caller to do .unwrap_or(trust)?

security-framework/src/trust_settings.rs Outdated Show resolved Hide resolved
@ctz
Tighten tests by looking for specific cert
e905890 
Also, assert there are numerous system certs.
@ctz
Tidy error handling
faaebc9
@ctz
Remove no-trust-settings handling and test this case
f209072 
As it happens this is documented to return errSecItemNotFound
for unknown certificates.
@ctz
Remove default-trust from tls_trust_settings_for_certificate
b97b24d 
This can be done by the caller and it only makes sense
in some contexts.
@ctz
Copy link
Contributor Author

ctz commented Sep 15, 2019

Thanks for the time you spent reviewing this. I think I've addressed all your comments in some remedial commits; let me know if you prefer these to be squashed.

@kornelski kornelski merged commit 6f6f500 into kornelski:master Sep 15, 2019
@kornelski
Copy link
Owner

Great. Thank you!

@ctz ctz mentioned this pull request Oct 27, 2019
Closed
@dekellum dekellum mentioned this pull request Nov 20, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kornelski kornelski kornelski requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@ctz @kornelski