Control biometric / keychain access control settings #39

mikenachbaur-okta · 2022-04-21T17:31:39Z

No description provided.

…ess control

IldarAbdullin-okta

Very clean and readable 👍
Left several comments to consider

IldarAbdullin-okta · 2022-04-26T20:51:09Z

Sources/AuthFoundation/Security/Internal/Keychain+Extensions.swift

+            }
+        }
+
+        guard let result = ref as? Dictionary<String, Any> else {


Shall we wrap AnyObject into Keychain.Item rather than wrap concrete type so owner can do the upcast on its side(it can be Data for example)? Looks as strict requirement to GET item only as Dictionary<String, Any>

I'm not sure I understand. Are you referring to using AnyObject in the value property, instead of using Data explicitly?

I've done that in the past, where I did auto-encoding of data into the keychain, but encountered a lot of problems with custom coders/decoders, among other issues. So I decided this time around to stick as close to what the Keychain itself provides, and leave it as an exercise to the developer to encode the data they want (or, create a custom extension of Keychain.Item that does the right thing).

If that's not what you meant, my apologies :-D

Ok, I think I missed important setting in query kSecReturnAttributes so you basically asking keychain to return attributes along with data. So downcast to Dictionary<String, Any> a bit confused me as without kSecReturnAttributes setting keychain returns reference to Data or SecKey type

Correct; this current use-case for the Keychain class is limited to the Generic Password type (pretty much the majority use-case, since dealing with certificates isn't as common).

IldarAbdullin-okta · 2022-04-26T20:52:25Z

Sources/AuthFoundation/Security/Internal/Keychain+Extensions.swift

+extension KeychainGettable {
+    var getQuery: [String: Any] {
+        var result = self.query
+        result[kSecMatchLimit as String] = kSecMatchLimitOne


Do we wanna make it configurable?

I wanted to keep it simple, making a get only return a single item ( it include data), while a list could return multiple items without data (meaning a list could safely be used without popping FaceID/TouchID prompts).

I also wanted to make the interface unambiguous without the need for nullability checks. So if an item is returned, it can be accessed simply.

I could theoretically make list configurable though. List is useful because a search result itself has a get method, allowing multiple items to be retrieved.

IldarAbdullin-okta · 2022-04-26T20:59:22Z

Sources/AuthFoundation/Security/Internal/Keychain+Extensions.swift

+        }
+    }
+
+    func performUpdate(_ item: Keychain.Item, authenticationContext: KeychainAuthenticationContext?) throws {


Looks like this function only updates data via remove/insert. Do we want to allow update not only data but some other attributes, let's say accessibility parameters?

This is calling the underlying keychain update API.

IldarAbdullin-okta · 2022-04-26T21:03:50Z

Sources/AuthFoundation/Security/Internal/Keychain+Extensions.swift

+        let updateSearchQuery = self.updateQuery
+
+        var saveQuery = item.query
+        saveQuery.removeValue(forKey: kSecClass as String)


BTW, why we are doing remove if our underlying call is SecItemUpdate? User may loose previous data if SecItemUpdate fails

This is removing the keychain item class, not the actual keychain item itself. This is to just format the query into something acceptable by the keychain API.

Ok, I see. We just normalizing query here

IldarAbdullin-okta · 2022-04-26T21:16:46Z

Sources/AuthFoundation/Security/Keychain.swift


-            Keychain.implementation.deleteItem(cfDictionary)
+            Keychain.implementation.deleteItem(cfDictionary as CFDictionary)


Maybe just create explicit error for duplicate item(errSecDuplicateItem) case instead of physically delete? There is a risk that user may loose the data

My concern around saves is a bit complex.

If we save and get a duplicateItem error, we'll have to delete anyway, and then re-save. That'll result in 3 operations.

If we save and get a duplicateItem, we can call update, but will still need to perform a Get to retrieve the saved item, which risks popping up a FaceID prompt in the process, and still results in 3 operations.

If we perform a delete and it fails for a reason other than duplicateItem, the save may or may not fail as well, which would be moot point.

If the delete succeeds, but the save fails for some reason, there is a chance of data loss, but only if the newly created keychain item is invalid / poorly formed (e.g. conflicting access control policy settings).

I think the risk is low, though I'd like to get your thoughts on the matter.

IldarAbdullin-okta · 2022-04-26T21:25:20Z

Sources/AuthFoundation/Token Management/Internal/KeychainTokenStorage.swift


-        delegate?.token(storage: self, replaced: id, from: oldToken, to: token)
+        try? oldResult.delete()
+        try newItem.save(authenticationContext: security?.context)


Why we don't use update API here?

Update has some limitations, in particular it doesn't let you return a reference of the newly created keychain item. So for cases such as this, a delete/remove is often safer.

I plan to use update in the setMetadata method however.

You basically already did the GET of oldResult so you have all required attributes to copy into the new Item. I still like the GET/UPDATE approach since it is only 2 operations comparing to GET/DELETE/ADD. Data safety aspect is also important

You're right, in this particular use-case you're totally correct, this is a prime candidate for Update. I'll make that change.

IldarAbdullin-okta · 2022-04-26T21:31:01Z

Sources/AuthFoundation/Token Management/Internal/KeychainTokenStorage.swift

            .Item(account: metadata.id,
                  service: KeychainTokenStorage.metadataName,
                  accessibility: .afterFirstUnlock,
                  value: try encoder.encode(metadata))
-            .save()
+
+        try? item.delete()


Risky. Maybe just build the logic around Add/Update APIs rather than Delete/Add? I think Add/Update is not that risky, at least can't cause data loss

I mentioned this a bit up above, both the discussion around how to handle Delete/Add, as well as the use-cases around Update. For Metadata items, I'll change it to use Update, since I don't need to get the resulting item.

IldarAbdullin-okta · 2022-04-27T16:23:44Z

Sources/AuthFoundation/Token Management/Internal/KeychainTokenStorage.swift

-                  accessibility: .unlocked,
-                  value: data)
-            .save()
+        let accessibility = security?.accessibility ?? .afterFirstUnlock


Should we derive this setting from oldResult?

The user could potentially change the accessibility settings, but you're correct in that it should default to the current value.

IldarAbdullin-okta · 2022-04-27T16:24:50Z

Sources/AuthFoundation/Token Management/Internal/KeychainTokenStorage.swift

-                  value: data)
-            .save()
+        let accessibility = security?.accessibility ?? .afterFirstUnlock
+        let accessGroup = security?.accessGroup


Should we derive this setting from oldResult?

mikenachbaur-okta marked this pull request as draft April 21, 2022 17:34

mikenachbaur-okta added 2 commits April 26, 2022 13:03

Ensure Form conforms to Hashable

23ef421

Add support for keychain update, biometry, security settings, and acc…

cea880c

…ess control

mikenachbaur-okta force-pushed the man-OKTA-467632-Biometric branch from a1ea1b7 to cea880c Compare April 26, 2022 20:03

mikenachbaur-okta marked this pull request as ready for review April 26, 2022 20:04

mikenachbaur-okta requested review from stevelind-okta and jaynewstrom-okta April 26, 2022 20:04

IldarAbdullin-okta approved these changes Apr 26, 2022

View reviewed changes

IldarAbdullin-okta reviewed Apr 27, 2022

View reviewed changes

mikenachbaur-okta added 2 commits April 27, 2022 11:06

Ensure keychain results include all attributes

122adfc

Update and preserve previous keychain settings when replacing a token

0c13706

mikenachbaur-okta merged commit 30170d2 into master Apr 27, 2022

mikenachbaur-okta deleted the man-OKTA-467632-Biometric branch April 27, 2022 18:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Control biometric / keychain access control settings #39

Control biometric / keychain access control settings #39

mikenachbaur-okta commented Apr 21, 2022

IldarAbdullin-okta left a comment

IldarAbdullin-okta Apr 26, 2022

mikenachbaur-okta Apr 27, 2022

IldarAbdullin-okta Apr 27, 2022

mikenachbaur-okta Apr 27, 2022

IldarAbdullin-okta Apr 26, 2022

mikenachbaur-okta Apr 27, 2022

IldarAbdullin-okta Apr 26, 2022

mikenachbaur-okta Apr 27, 2022

IldarAbdullin-okta Apr 26, 2022

mikenachbaur-okta Apr 27, 2022

IldarAbdullin-okta Apr 27, 2022

IldarAbdullin-okta Apr 26, 2022 •

edited

Loading

mikenachbaur-okta Apr 27, 2022

IldarAbdullin-okta Apr 26, 2022

mikenachbaur-okta Apr 27, 2022

IldarAbdullin-okta Apr 27, 2022

mikenachbaur-okta Apr 27, 2022

IldarAbdullin-okta Apr 26, 2022

mikenachbaur-okta Apr 27, 2022

IldarAbdullin-okta Apr 27, 2022

mikenachbaur-okta Apr 27, 2022

IldarAbdullin-okta Apr 27, 2022


		Keychain.implementation.deleteItem(cfDictionary)
		Keychain.implementation.deleteItem(cfDictionary as CFDictionary)

Control biometric / keychain access control settings #39

Control biometric / keychain access control settings #39

Conversation

mikenachbaur-okta commented Apr 21, 2022

IldarAbdullin-okta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IldarAbdullin-okta Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IldarAbdullin-okta Apr 26, 2022 •

edited

Loading