Various bug fixes and new features for Android and iOS

[luckyrat]

I've been working with a fork of this repo for some time now while developing my password manager, Kee Vault. The Android version is already in production and I'm working on the iOS version now so have a good understanding of what's required for both platforms. I've not looked at macOS or any other platforms and don't plan to do so in the short-term.

I think a lot, maybe all, of the changes I've developed will be useful for all package consumers so would like to help incorporate the work into this repo. I've created this PR with the full set of changes but appreciate there are a lot of separate concerns in there at the moment so am happy if you either cherry-pick specific commits or discuss the proposed changes in more detail here before we work on a number of more focussed PRs.

The PR contains the following changes, broken up into separate commits:

Optional override of prompt info per request

An updated approach to that previously proposed in #13 so that a custom prompt can optionally be supplied to each action without having to re-initialise the storage every time.

setRequestStrongBoxBacked (Android P+)

This improves security for Android P users and higher. I don't support any Android versions lower than that due to them being out of support with Google but previous simulator tests haven't revealed any issues with 8.0 and 8.1.

Enable forceInit feature

I think this fixes a bug where asking to force the initialisation of the storage threw an exception rather than initialising it as requested.

[ios] Remove "Checking auth support" Cancel button

The check to see if iOS can evaluate device owner authentication was setting the cancel button of the authentication context to always be "Checking auth support". This change just lets iOS use it's default ("Cancel" - that may be localised too but I've not checked).

Fix ios authenticationValidityDurationSeconds

touchIDAuthenticationAllowableReuseDuration only applies to the biometric authentication which occurs when the user unlocks their device. At least that is the case with iOS 12 - I can't test on any other versions of macOS but the Apple documentation doesn't suggest we would expect a difference. https://developer.apple.com/documentation/localauthentication/lacontext/1622329-touchidauthenticationallowablere http://www.openradar.me/radar?id=6060524114542592

Thus we need to keep track of the time of last authentication ourselves and throw away the old LAContext instance when too much time has elapsed.

Other commits

Remove old code, Clarify comment, Upgrade build tools and Kotlin and the version number increment should all be self-explanatory and hopefully not contentious but feel free to apply your own variation of these at a different time if that helps with your release process.

I'm looking forward to receiving your feedback regarding these changes @hpoul. If you or anyone else wants to try out the whole lot of them at once, https://github.com/kee-org/biometric_storage/tree/keevault is what I'm using for my app (kee-org@f94a779 at the time of writing).

[hpoul]

Hello,
thanks for the PR.. although it would be a bit better to have it split up into smaller once, as there are a lot of topics mangled into one

setRequestStrongBoxBacked

Sounds reasonably, but I think it's in the wrong location, the masterKey of BiometricStorageFile is only a fallback, most of the time the CryptographyManager would be used.. I guess I should cleanup and hopefully nobody is still using the old masterKey.. (the old version consistently produced errors #12 and it doesn't seem like anyone is going to fix that..

forceInit

this pretty much works as intended.. although forceInit is probably a bad name, maybe requireInit would have been better.. but I don't want to change that now.. The documentation says:

if forceInit is true, will throw an exception if the store was already created in this runtime

I don't see how your change fixes anything, it just invalidates the documentation..

Remove "Checking auth support" Cancel button

sounds okay.. i guess we should introduce an additional prompt info config.. but anyway, I don't think anyone is relying on that :-)

Fix ios authenticationValidityDurationSeconds

Do I understand the linked bug report correctly: if touchIDAuthenticationAllowableReuseDuration is >0 the value is ignored, and it's always 600 seconds?

So the code in the plugin is correct, only iOS is broken?

Optional override of prompt info per request

Still not a big fan of that one.. do you have a good use case?

[luckyrat]

Yeah, I understand and appreciate your quick response despite the quantity of bundled issues. I just wanted to get some initial feedback rather than splitting and managing the potentially long-running integration of 9 different PRs without prior discussion :-)

setRequestStrongBoxBacked

You're right. I think I'd implemented this change before you re-wrote the Android encryption code (which was much appreciated because I'd never got the old Androidx implementation to work reliably). Since this only impacts legacy storage, I can't see any benefit to merging this commit now. We can discuss in a followup issue whether there is an equivalent setting we can/should enable for the CryptographyManager implementation.

forceInit

I found that it was not possible to init the store more than once and thus saw this parameter and expected it to resolve that problem. When it didn't I changed the code so that the store can be initialised more than once and used the forceInit parameter since it looked as though that was the intended purpose for the parameter. It's possible the documentation wasn't surfaced by my IDE at the time I needed because I have no recollection of reading about the exception in the docs but this was a very long time ago so my memory is a little hazy.

The current initialisation implementation is incompatible with my password manager architecture (which needs to be able to re-initialise the storage) but I understand the reluctance to change the meaning of a parameter that you think is working as intended so lets defer this commit until we can think of an alternative approach.

Remove "Checking auth support" Cancel button

Yeah we could introduce a new config option there in a future commit but I don't personally need it at the moment.

Fix ios authenticationValidityDurationSeconds

As I understand it, that unacknowledged bug from 6 versions of iOS ago is essentially confirmation from Apple that the behaviour described in the bug is what iOS is designed to do. Their documentation doesn't clearly explain the expected behaviour but when also read in conjunction with "As a result, any Touch ID authentication that keychain services requires is satisfied by the most recent device unlock event, if it happened within the given number of seconds (up to five minutes)." which is buried in https://developer.apple.com/documentation/localauthentication/accessing_keychain_items_with_face_id_or_touch_id , I think it is safe to assume that touchIDAuthenticationAllowableReuseDuration does not work in the way that most people expect it to.

So the value is not actually ignored, it's just not being applied in the way that we expected. If you try to authenticate a write within 10 seconds of unlocking the phone (using the example app) no biometric authentication is required for that first write request (might be easier to reproduce by increasing the limit to more than 10 seconds and there must be no current stored data).

Then, for the next 10 minutes, that LAContext which was authenticated using the credentials supplied by the user during the phone unlock will remain authenticated and thus all ongoing read and write operations succeed. The 10 minute limit is unrelated to the 5 minute maximum value for touchIDAuthenticationAllowableReuseDuration, does not appear to be documented anywhere and presumably is subject to change at Apple's discretion.

It's hard to argue that iOS is not broken but perhaps that's just a perpetual truth! In any case, it appears unlikely that this particular behaviour is going to change any time soon so I think the changes in my proposed commit are what can make the code in the plugin correct as far as iOS is concerned :-)

Not only do we still get the neat automatic authentication if the user has just-that-second authenticated with the device itself but we also get the expected ongoing authentication checks after the configured authenticationValidityDurationSeconds have expired. Plus that aligns with how the setting works on Android too.

Optional override of prompt info per request

I use it to allow the same storage instance to display different titles and descriptions to the user when reading vs writing on Android. I'm happy to work on alternative changes to bring the iOS and Android configuration objects more into alignment if you prefer but I figured this would be the least disruptive way to enable support for my use-case (being as how it is backwards compatible) and perhaps it could be useful in future cases too. Maybe some context around why the user is being asked again like "you changed a biometric setting" or "it's been too long since your last authentication", as examples off the top of my head which might be relevant to multiple platforms.

Let me know if there are any of the commits apart from the setRequestStrongBoxBacked and forceInit ones that you'd like to defer for longer and I can then create a new branch with just the ones you're happy with at this point and therefore allow us to narrow our focus to those that remain.

[hpoul]

thanks for the details :-)

forceInit

So you need a way to change the init options of a storage which was already initialized? I don't think this makes much sense, because the InitOptions only take effect at the time the key is created.. So if you've used the storage before, initializing it again with different options won't change anything..
(ie. it would be practically the same as just having forceInit = false - which simply ignores the init call)

authenticationValidityDurationSeconds

I'm still not sure I fully understand the current behavior.. and it will probably require more testing.. I also can't see how #68 relates to this, as the link you provided specifically only mentions Touch ID, and should't apply to Face ID?

But that documentation was a nice find, I'm not sure i've seen it before 😂 Especially the last sentence sounds pretty strange to me

Note that this grace period applies specifically to device unlock with Touch ID, not keychain retrieval authentications.

So this means, no matter the value of touchIDAuthenticationAllowableReuseDuration, it will only effect when the user uses touch id for device unlock within the last X seconds.. If the user authenticates for our keychain retrieval, it will not be reused anyway? 🤔

And I also don't understand how this now relates to keeping LAContext around. It seems to have changed something.. So when setting touchIDAuthenticationAllowableReuseDuration to 30 seconds, it only takes effect when the LAContext is initialized before the user unlocks the device? Or does the LAContext have to be used before the user unlocks the device?

I think we have to fully understand the actual and intended behavior before adding a lot more workarounds to the code 😂 I also wouldn't mind it too much if android and iOS behavior differ, as long as we can document it, and it aligns to the behavior of the platform.. 🤔

prompt info per request

Oh, I think i understand the problem now after all.. the AndroidPromptInfo only has one set of text, while the iOSPromptInfo has separate strings for save and access 🤨
This is pretty strange.. but I guess you are right, the least disruptive change is to allow overriding it.. but I would prefer just calling it promptInfo in the read and write methods..

All in all, if you rename the parameter to promptInfo and revert the iOS LAContext as well as the forceInit changes, I think we could merge this PR :-)

[luckyrat]

Sounds good. I've created PR #71 with just the changes you're happy with (since this branch is what my app depends upon at the moment). I dropped the version number change too since I figured you might as well pick the one you want along with updating the changelog.

That just leaves two issues which I've continued discussing below - I'm happy for you to branch them off into separate issue numbers or just keep discussing them here until something gets decided one way or the other. In any case, if we want to merge some variation of the two remaining commits I can create individual PRs for them on new branches at that time.

forceInit

I cache the current storage instance:

  Future<BiometricStorageFile> _storageFile() => _storageFileCached ??= BiometricStorage().getStorage(storageFileName,
      forceInit: true, options: StorageFileInitOptions(authenticationValidityDurationSeconds: authGracePeriod));

Then clear that cache when calling (await _storageFile()).delete() (e.g. when the user changes the authenticationValidityDurationSeconds setting).

Without my changes to the forceInit parameter behaviour, the singleton behaviour of BiometricStorage() necessitated a full app kill and restart before the desired setting change could take effect. I.e., even after deleting the key, it was not possible to create a new storage with different StorageFileInitOptions.

That's all from my Android experience but, although pending the conclusion of our ongoing investigation, it looks as though the authenticationValidityDurationSeconds setting has effect on iOS when either reading or writing the key, thus further increasing the benefit of being able to make this change without an app restart being required.

authenticationValidityDurationSeconds

I also can't see how #68 relates to this, as the link you provided specifically only mentions Touch ID, and should't apply to Face ID?

I have found a lot of contradictory information in recent days which makes me think that Apple's TouchId documentation and APIs also affect FaceId, at least in some situations. Without a FaceId phone I can't verify any of this myself but for example:

square/Valet#167 (comment)

Note that this grace period applies specifically to device unlock with Touch ID, not keychain retrieval authentications.

I've rewritten that last sentence in another way which I think is still true to its original form but perhaps is more helpful for understanding how this all fits together:

"Note that this grace period starts only when unlocking with Touch ID (and maybe FaceID); a successful keychain retrieval authentication will not restart the grace period."

And I also don't understand how this now relates to keeping LAContext around. It seems to have changed something..

By keeping the LAContext around now (compared to the code from a few weeks ago) we also apply the touchIDAuthenticationAllowableReuseDuration setting to read operations instead of just write operations.

Once the LAContext has been authenticated once, it remains authenticated "indefinitely" (actually that's the undocumented 10 minutes I've previously mentioned).

So, with the current code on main the touchIDAuthenticationAllowableReuseDuration only has an effect if the first authentication attempt is within that number of seconds of the user unlocking their phone; all subsequent authentication requests will then automatically succeed (for 10 minutes).

If the user takes longer than touchIDAuthenticationAllowableReuseDuration seconds before the first authentication request is made (or if they have configured the package to ask every time), they will be prompted to authenticate at that point, and then all subsequent authentication requests will automatically succeed.

[hpoul]

forceInit

okay, I think I get it now.. You are letting your users choose the options.. I didn't really anticipated those things to change, I thought the developer chooses the "right" choices for each storage and they never change. That's why the exception was actually the initial behavior, because I expect it to be a bug when there are multiple calls (with different options) to the initializer.

I think a proper fix would be to have something like a deleteAndDispose which deletes the storage, clears the key and removes the storage file from the dictionary.. and after disposing you have to initialize it again.

authenticationValidityDurationSeconds

I don't think "(and maybe FaceID)" makes anything more clear 🤣.

Let's create a separate issue or something for this.. I wouldn't create a workaround for it just yet..

I imagine that one possible additional confusion is whether this has to be set while writing, or while reading.. According to your linked issue it has to be set already when writing.. not only when reading..

probably it doesn't matter what the value is while reading the value, when it was 0 (or not set) while writing..

[luckyrat]

Thanks for merging the other PR and the work in those subsequent commits.

I've updated this branch with those latest changes incorporated so that I can include them in my project.

Since there are now only the two outstanding issues, I've created #74 and #75 to continue the discussions there and will close this PR since this branch is not ever likely to become the neatest source for any potential future code submissions.