Fix ios authenticationValidityDurationSeconds

[luckyrat]

This PR retains the LAContext until authenticationValidityDurationSeconds have elapsed, thus forcing the user to reauthenticate when it has been that long since either:

• They authenticated to unlock their phone
• They authenticated via this plugin

I've slightly improved it since the earlier commit that was included in #70, to improve clarity and ensure that when authenticationValidityDurationSeconds <= 0 we always create a new context.

[hpoul]

I did quite a few refactorings.. but I think I now understand how it is working and how it is supposed to work..

1. LAContext objects should be created for every authentication flow, and NOT reused.

   The context handles user interaction, and also interfaces to the Secure Enclave, the underlying hardware element that manages biometric data. You create and configure the context, and ask it to carry out the authentication. You then receive an asynchronous callback, which provides an indication of authentication success or failure, and an error instance that explains the reason for a failure, if any.

   I think it is meant that you create an instance, set all options, evaluate the rules, receive your async callback.. and then throw it away. Any subsequent evaluation will probably just lead to the same result, no matter how much time has passed (just guessing, but anyway)

2. touchIDAuthenticationAllowableReuseDuration:

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

   this has absoultely no effect when you are only reading/writing keychain items.. and not unlocking your device..

I played around with the example/ app where I set the duration to 5s .. and create a new LAContext for every invocation.. When I unlock the device I can access the secure keychain item for 5 seconds, afterwards it asks me to authentication every time. So long until I lock the device again.. and Unlock it.. then I've got another 5 second window where I can access the keychain items without any reauthentication.

I don't think it is valid to reuse the LAContext in the first place.. And my experimenting now matches the iOS documentation.. So I guess that is the way it is supposed to work.

[luckyrat]

1. LAContext objects should be created for every authentication flow, and NOT reused.

I think this depends on what the LAContext is intended to be used for.

1. In a situation where you want to require fresh authentication every time, except for when the end user has just authenticated to unlock their phone, then yes, do not reuse the context.
2. In the case where you want any successful authentication to remain valid for a certain amount of time, you must reuse the context.

It was my understanding that the authenticationValidityDurationSeconds setting was intended to behave in the latter way. Indeed, that is what happens on Android and I'm not sure if there is any library consumer or end-user benefit of behaving differently on iOS?

In my opinion, the end user benefits most from this setting when it behaves in the 2nd way (especially if it's a user-facing setting as mentioned in #75).

If you disagree (or even if you don't!) there could be a benefit to creating a new configuration parameter iosDeviceUnlockGracePeriodDurationSeconds. This could allow library consumers to directly control the touchIDAuthenticationAllowableReuseDuration setting independently of the context lifetime/expiry which could still be configured using authenticationValidityDurationSeconds via an approach similar to that proposed in this PR.

I've prefixed with ios because I don't think it's possible to have separate durations on Android but maybe "apple" would be more appropriate if we find that it behaves like this on macOS too.

I can see you've done a lot of work refactoring this part of the code recently so I'll hold off making any specific code suggestions for now while you're actively working on it, but if you want me to rebase my branch and work on any particular changes just let me know - I should have a bit of time in the next couple of weeks.

[hpoul]

depends on what the LAContext is intended to be used for.

Can you find any source of this in the apple documentation? I don't think this is the indented behavior at all.

My concern is, that this is undocumented behavior and might lead to further bugs or unintended security problems.

authenticationValidityDurationSeconds setting was intended to behave in the latter way.

Well, it was, but I didn't know that this is not actually a feature iOS/MacOS provides. I'm also pretty sure that the last sentence of the touchIDAuthenticationAllowableReuseDuration was added in a later version of the API which clarifies that it's only about unlocking the device, not unlocking/accessing the keychain 😅

I actually think the best way would be ..

1. rename authenticationValidityDurationSeconds to androidAuthenticationDuration and link to the documentation https://developer.android.com/reference/android/security/keystore/KeyGenParameterSpec.Builder#setUserAuthenticationParameters(int,%20int)
2. create a iosTouchIDAuthenticationAllowableReuseDuration and link to it's documentation https://developer.apple.com/documentation/localauthentication/lacontext/1622329-touchidauthenticationallowablere
3. deprecate authenticationValidityDurationSeconds and, if set, use it as default value for the two created options..
4. create a workaround for iOS/MacOS .. like iosAuthenticationTimeoutWorkaround and document that it will keep LAContext for as long as that duration as a workaround to not require further authentication. But it will ignore authenticationValidityDurationSeconds

Do you think that makes sense? I guess it would be the best to understand.. and users have everything under control..
If you think that sounds fine I could implement it as well in the current main branch..

[luckyrat]

Can you find any source of this in the apple documentation? I don't think this is the indented behavior at all.

No. Also no documentation about implementing it in the other way either. I therefore conclude that it is entirely up to us to decide under which circumstances it is appropriate to reuse the context and when it should be destroyed.

I share your concern but see nothing to suggest that there is a specific prescribed way in which the context must be used, so I think the uncertainty is not resolved by simply discarding the context. I'm not sure if @radvansky-tomas has had a chance to review any of this discussion yet but given they recently submitted the PR to re-use the context, it's possible they can point us to further documentation we've missed or at least impart some wisdom regarding this topic.

I'm also pretty sure that the last sentence of the touchIDAuthenticationAllowableReuseDuration was added in a later version of the API which clarifies that it's only about unlocking the device, not unlocking/accessing the keychain sweat_smile

https://stackoverflow.com/questions/38379125/touchid-activatetouchwithresponse-returns-success-without-requesting-fingerprint#comment110453577_40471007 seems to hint at a time when Apple significantly changed the documentation on this subject which probably explains why your earlier research into the topic led to the initial implementation and our ongoing uncertainty about what is supposed to be supported. By the way, although the documentation was supposedly changed in iOS 13, the behaviour I see on iOS 12 matches that updated documentation.

In any case, I think your suggested approach will put sufficient control into the library consumers and therefore end-users so that the question of what Apple actually intended can be deferred. If anyone later finds additional documentation or guidance which would help library consumers select their own suitable configuration values, we can reference that without having to change the behaviour of the new properties.

I have a few questions / comments before you implement it:

1. What about additional platforms? Presumably macOS is pretty-much covered by the ios configuration options (though it would be good to make this clear in the property names I guess); I've never looked into the options available on any other platform but we probably can't assume they'll follow exactly the same approach as either Android or iOS.

Perhaps a similar approach to the PromptInfo class could be used so we can easily add additional platforms in future? Then we can probably do away with some of the prefixes on the property names and also create a neat place to later add support for restricting by authentication type once that idea gets more refined. Something like this?

class StorageFileInitOptions {
  List<AuthenticationConfiguration> platformAuthenticationConfigurations;
  // can remove this: androidBiometricOnly 
  // can remove this in future?: authenticationRequired 
  // can remove this: authenticationValidityDurationSeconds 
}

class AuthenticationConfiguration {
  int deviceUnlockGracePeriodSeconds;
  int authenticationValidityPeriodSeconds;
}

class AndroidAuthenticationConfiguration extends AuthenticationConfiguration {
  bool biometricOnly; // could replace with a class/enum like AllowedMethods and maybe move to the base class
  set validityPeriodSeconds(int value) {
    deviceUnlockGracePeriodSeconds = value;
    authenticationValidityPeriodSeconds = value;
  }
}

class IosAuthenticationConfiguration extends AuthenticationConfiguration {}

class MacOSAuthenticationConfiguration extends IosAuthenticationConfiguration {}

//class WindowsAuthenticationConfiguration ... etc.

Could instead put the AuthenticationConfiguration variables on IosAuthenticationConfiguration but maybe making them read-only on the base class would help reinforce that they can be configured independently on iOS but will be set to the same on Android?

I expect you'll have a good idea of what approach works best with the native code interop layer but I figured at least having a base class would allow the configuration to be an unlimited number of platforms in an array (with those common properties defaulting to some reasonable value if no platform configurations are supplied).

Maybe later we could move authenticationRequired into that class too in case there is a desire to change that behaviour on just some platforms (e.g. web).

2. We could deprecate the old authenticationValidityDurationSeconds property but given how its behaviour has changed over different versions of the library anyway, it might be better to just make that a compile-time error when consumers move to the next major version of this library. In other words, I don't think it will do much harm for affected developers to at least take a cursory glance at the new configuration options to understand that the behaviour might have changed (or that they might now want to change from the previous behaviour).

3. I'm personally not a huge fan of the Workaround nomenclature - I don't think it will mean much to anyone that hasn't read this issue in detail so I still favour naming the properties by what they actually do (or at least what our best-estimate is for what they will do). In a similar vein, iosTouchIDAuthenticationAllowableReuseDuration requires library consumers to do some additional research before understanding what the property would do (which we have already determined is non-trivial 😅 )

What do you think?

By the way, if you prefer to work on a branch for WIP and get a 2nd opinion from me or others before rebasing onto main feel free to ping me.

[radvansky-tomas]

Thank you for mention @luckyrat . I didnt check this plugin recently, as the project I used it for is legacy one and it uses my custom fork.

At the time of creation, reusing of context / not reusing context had massive impact on iOS based features.

As the value touchIDAuthenticationAllowableReuseDuration belongs to context, without it...nothing is passed into.

I cant judge Android, as its segmentation and missing core features and implementing custom way of treating secure features by brands like Samsung/Huawei its insanity of different level :D So I would take android as sort of interface for this.

[hpoul]

but see nothing to suggest that there is a specific prescribed way in which the context must be used, so I think the uncertainty is not resolved by simply discarding the context

well.. When the context is discarded, everything works as documented. (no matter the wording of the actual properties). When the context is preserved, weird things start happening (ie. authentication is preserved, no matter what values are set on the reuse duration). ... I think this is a pretty good indication of not using something as intended, when it is not behaving as documented.

What about additional platforms?

MacOS uses exactly the same implementation (it's just symlinked). And behaves very similar.
I don't think Linux has anything similar setting on how long an authentication stays valid. iirc windows has no additional biometric authentication for credential store.. 🤔
and on web everything is stored plain text anyway..

I'm not sure yet if I'd really want to release a new major version to introduce breaking changes.. it feels like those are all bug fixes.. the major version change would only be because the configuration changes :-)

[brianblanchard-wf]

FWIW, when we updated biometric_storage to 4.1.3 via a normal flutter pub upgrade we saw what we perceived as breaking behavior. We were expecting every time we call BiometricStorage.read() that the user gets prompted for a biometric challenge. Our application's authentication depends on this happening every time.

This could be a potentially critical fix for others that experienced the same issue we did because on 4.1.3 users could log in when we were expecting them to have to pass a biometric challenge again.

[hpoul]

I did a quick rewrite in #77 maybe you can take a look @luckyrat
(I still need to actually test it... quite possible that there are some serious bugs, but I think it makes sense this way)

I've used the wording iosTouchIDAuthenticationForceReuseContextDuration which is exactly what it does.. 🤷‍♂️

[brianblanchard-wf]

@hpoul since the 4.1.3 is the latest version on pub and can easily be upgrade to by running a flutter pub upgrade but has some issues as described above would you be willing to remove the release or fix forward with a new release that does not have the breaking issues with the authenticationValidityDurationSeconds?