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

FIX [VALSinglePromptSecureEnclaveValet containsObjectForKey] on TouchID fingerprints changes #116

Conversation

juantrias
Copy link
Contributor

[VALSinglePromptSecureEnclaveValet containsObjectForKey] is returning YES after the key has been removed from the Keychain due to TouchID fingerprints changes, after adding or removing a fingerprint. The reason is that the LAContext instance kept by VALSinglePromptSecureEnclaveValet has the key cached. So, for containsObjectForKey, we need to query the Keychain without using this instance, as in regular VALSecureEnclaveValet.

Additionally, although this does not affect the functionality, we only need to pass the LAContext instance when reading keys from the Keychain, not when adding or removing keys

…ning YES after the key has been removed from the Keychain due to TouchIDCurrentSet changed

- Do not query the Keychain using cached LAContext. Do not override containsObjectForKey from VALSinglePromptSecureEnclaveValet
…om the Keychain, not when adding or removing keys
@dfed
Copy link
Collaborator

dfed commented Sep 27, 2017

after the key has been removed from the Keychain due to TouchID fingerprints changes

Great find! We'll need to make sure to adopt similar changes in #80.

Additionally, although this does not affect the functionality, we only need to pass the LAContext instance when reading keys from the Keychain, not when adding or removing keys.

Fascinating! Do you have documentation to support this?

I'm going to run this change though its paces in our TouchIDTest app. If it looks good, I'll merge and release an update :)

@dfed
Copy link
Collaborator

dfed commented Sep 27, 2017

This looks good! Will merge when CI goes green. Updating #80 now. Thanks again for this fix!!

@dfed
Copy link
Collaborator

dfed commented Sep 27, 2017

Just updated #80 with 8f83921. The next release of Valet won't regress this :)

@dfed dfed merged commit 91e8479 into square:master Sep 27, 2017
@dfed
Copy link
Collaborator

dfed commented Sep 27, 2017

2.4.2 has been published with this change. Thanks again!

@juantrias
Copy link
Contributor Author

Thank you for the quick reply!

Additionally, although this does not affect the functionality, we only need to pass the LAContext instance when reading keys from the Keychain, not when adding or removing keys.

I haven't found documentation to support this, just my testing and this transcript from a WWDC Session http://asciiwwdc.com/2014/sessions/711

Authentication is the new component, and to specify authentication you have to provide policy.

And policy defines what authentication methods have to be satisfied or done before the Keychain item is decrypted and returned back to your application

Official docs are ambiguous, i.e. for kSecAccessControlTouchIDCurrentSet they say "Constraint to access an item with Touch ID for currently enrolled fingers"

@juantrias juantrias deleted the fix-VALSinglePromtSecureEnclaveValet-containsObject branch September 28, 2017 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants