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
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@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 SSL/TLS label Nov 17, 2017

@bagder bagder requested a review from nickzman Nov 17, 2017

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Nov 17, 2017

Member

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!

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

This comment has been minimized.

Show comment
Hide comment
@refnum

refnum Nov 17, 2017

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

refnum commented Nov 17, 2017

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

@nickzman

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

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Jan 25, 2018

Member

Thanks!

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.