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

Don't import client certificates into Keychain on macOS. #2085

Closed

Conversation

refnum
Copy link

@refnum refnum commented Nov 15, 2017

SecPKCS12Import is used to import a PKCS12 certificate on Darwin, generating a SecIdentityRef that can pass the certificate into Secure Transport.

On iOS SecPKCS12Import never stores the imported certificate into the Keychain. However on macOS this API will always store the imported certificate into the user's Keychain.

As this does not match iOS, and applications may not want to see their client certificate saved into the user's Keychain, the SecItemImport API can be used to obtain a SecIdentityRef without changing the Keychain.

SecItemImport is only available from 10.7 onwards, however this code was already wrapped in a CURL_BUILD_MAC_10_7 test and on macOS the replacement API is available on the same systems that SecPKCS12Import was.

@bagder bagder added the TLS label Nov 17, 2017
@bagder bagder requested a review from nickzman November 17, 2017 15:11
@bagder
Copy link
Member

bagder commented Nov 17, 2017

It's very strange why there was no travis build for this PR! @refnum, would you mind for example rebasing this commit and force-push it to see if it can trigger a correct travis build then? The travis job builds and tests the PR with many different setups and helps making sure it is good!

@refnum refnum force-pushed the feral/darwin_immutable_keychain branch from d7c0414 to 2296b23 Compare November 17, 2017 19:47
@refnum
Copy link
Author

refnum commented Nov 17, 2017

Done; it seems to be doing an AppVeyor build (7.50.0.6191) rather than Travis?

Copy link
Member

@nickzman nickzman left a comment

Choose a reason for hiding this comment

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

I didn't test this, but the code looks okay to me. (Do we have an automated test for loading client-side security identities stored in P12 files?)

@bagder
Copy link
Member

bagder commented Jan 25, 2018

Thanks!

@bagder bagder closed this in f8475c6 Jan 25, 2018
@lock lock bot locked as resolved and limited conversation to collaborators May 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants