Support for new SecAccessControlCreateFlags in VALSecureEnclaveValet

[dfed]

I want VALSecureEnclaveValet to support taking arbitrary SecAccessControlCreateFlags in the initializer. Currently only kSecAccessControlUserPresence is supported.

[nicwise]

Is this so you can do the finger-print-only TouchID? I had a PR in to do this, but no response from the Sqare folk, and I didn't do a good job of it (will revisit it)

[nicwise]

I was just about to @ the projects main owner, but its YOU! Are you taking PR's, assuming I do a better job than the last one @dfed ?

[dfed]

Hey @nicwise. Sorry for the delay on the last PR! The email notification came in while I was OOO and I failed to circle back once I made it back to the office.

The point of this issue is to support all SecAccessControlCreateFlags, not solely Touch ID. Taking a look at your PR now

[dfed]

Oh hey, you closed your PR. Interesting re surfacing that the enrolled fingerprint changed. Not sure I agree that that's terribly important to do within Valet since it's easy to detect that case using your own code (you could write a pref when you write the secret. If you can't access the secret under that config but can access the keychain, then it means the fingerprints enrolled have changed).

I'm going to investigate handling the creation of the specific SecAccessControlCreateFlags Valet without using a static. We'll see what I come up with.

[dfed]

I'm working on this on the branch federman/secaccesscontrolflags

[nicwise]

The flag idea is good, but at the moment you can't tell the difference between "Cancel" and "No fingerprints". So if the user has set something up (flag: yes), and changes a finger print (stringForKey returns NO always) we can't tell if they hit cancel or just tried to TID in.

Unless the flag has the same "invalidate if the TID print list changes", but I thought that needed a TID prompt to be used.

BTW: http://fastchicken.co.nz/2016/01/08/valet-keychain-done-right/ - tho we've had to roll it out for TouchID. It's still in for general Keychain access, so the SFHF has largely been replaced, which was the point.

[dfed]

Ahh makes sense! I'll see how I can surface that difference.

[nicwise]

Even just surfacing the last osstatus would do. Might be generally useful.

[dfed]

I'm pretty strongly opposed to surfacing the OSStatus. You should never need to read a system header to understand what Valet is doing. I'll see how I can surface this. Gimme a few.

[dfed]

Wait, can't you just use containsObjectForKey: to see if the key still exists? If containsObjectForKey: is YES and stringForKey: returns nil, then you know your customer cancelled.

[nicwise]

except if it's a TID-protected item. containsObjectForKey will throw up the TID UI :)

I agree, tho, OSStatus should be hidden. But... sometimes it'd be nice to know what happened. Hence why I pulled my PR: I've still not come up with a good solution.

[dfed]

No it won't throw up the TID UI. Check it out ;). The internals of containsObjectForKey: on VALSecureEnclaveValet is pretty hot.

[nicwise]

OK, I'll try it again. Might have been 'cos I was passing in a prompt....

[dfed]

Sounds good. Let me know what you find! If I've got a bug I'd be happy to fix it :)

[nicwise]

Those changes look and work really nicely, @dfed - thanks! Just need to work out the cancel vrs fingerprints-changed issue. Will put thinking hat on while uploading builds (slowly)

[nicwise]

awesome. despite my doubting, we can use [self.secureEnclaveValet containsObjectForKey:self.username] to check if an item is there. If it's not, but we expect it to be: finger prints have changed (assuming you use that option in creation). If it's there bit it's null: cancel was pressed. Most likely

[dfed]

Great! That's what I'd expect. Hoping to get #66 merged this week. Just need another set of eyeballs

[nicwise]

Sounds good. I'm going to try it in our app with just pulling the code in - but not committing it, as I'm happy to wait for the proper master-branch'ed Pod version :)

[dfed]

This is now available in v2.1.0