agens-no / EllipticCurveKeyPair

Sign, verify, encrypt and decrypt using the Secure Enclave
Other
708 stars 114 forks source link

Latest build does not compile #8

Closed wuf810 closed 6 years ago

wuf810 commented 6 years ago

Compilation error.

I guess you mean to be returning "x9_62HeaderECHeader"

screen shot 2017-11-10 at 09 05 26
hfossli commented 6 years ago

Yep. Sorry about that. Fixed now.

hfossli commented 6 years ago

That was the fastest bug report ever :D Literally minutes after I pushed. Thanks for reporting this issue. 🎉

wuf810 commented 6 years ago

No problem. Great code btw. Been working with ECC keys the last few weeks and your project has been very helpful!

However I'm still seeing issues with using LAContext and setting touchIDAuthenticationAllowableReuseDuration not working the way I expect it to. Have you done anything with this?

hfossli commented 6 years ago

Hey. Thanks for the feedback. That's very valuable to me.

I haven't tried using touchIDAuthenticationAllowableReuseDuration with this library. What's the current behavior and what are you expecting?

hfossli commented 6 years ago

(latest changes is published as 1.0.1 btw)

wuf810 commented 6 years ago

Basically I want to be able to create a LAContext, with touchIDAuthenticationAllowableReuseDuration set to LATouchIDAuthenticationMaximumAllowableReuseDuration that I can pass into any queries (kSecUseAuthenticationContext) where I have locked down the keys with .userPresence.

This means the user can authenticate just once for multiple queries during this period.

But what I am seeing is that the context never "expires" as such. So despite for example setting the LATouchIDAuthenticationMaximumAllowableReuseDuration to 30 seconds, after that time the TouchID authentication is not displayed.

I guess I was expecting it fail, at which point I could re-initialise the context for another period.

To be honest I'm not 100% clear how the context is supposed to work. Whether the context is set to nil after the expired period. There seems no way to get back whether it is still valid or not (although there is a way to invalid a context).

Maybe LAContext is not supposed to be used this way with .UserPresence. Not sure. If you have any thoughts/idea I would be love to hear them.

wuf810 commented 6 years ago

BTW My report may be have been the fastest ever but so was your fix :-)

wuf810 commented 6 years ago

Just done some more tests. Seems regardless of what I set the touchIDAuthenticationAllowableReuseDuration to, it only "times" out after 10 minutes! Once authenticated again, a period of 10 minutes becomes active again.

If at some point you have a chance to test this yourself, it would to know if you are seeing this too.

hfossli commented 6 years ago

Thanks for waiting. I have been testing quite extensively myself. I haven't checked after 10 minutes, but I'm glad to hear there's a limit.

I guess I was expecting it fail, at which point I could re-initialise the context for another period.

That would be my assumption as well. Local Authentication + Security framework seems really buggy...

Also, when reading the documentation on touchIDAuthenticationAllowableReuseDuration it states The default value is 0, meaning that no previous TouchID unlock can be reused., but I am experiencing that it can in fact be reused as many times I want.

This is superweird. I have no explanation. If you file a bugreport to Apple, please dupe in on openradar and paste a link here.

I have fixed some bug I introduced after refactoring. I don't know if you encountered more problems, but master and 1.0.2 should be fine now. Sorry for any inconvenience.

hfossli commented 6 years ago

Also, I recommend opening a TSI to Apple. They might provide some valuable insight.

If you need to fix this now I would recommend to just code your timeout logic yourself as it is better to have some measures than no measures.

wuf810 commented 6 years ago

I’m still doing some testing (which isn’t easy when having to wait 10 minutes for the session to time out).

I’ll raise a bug report once I’m sure of exactly what’s happening. But I think you’re right I that super buggy is the best description.

wuf810 commented 6 years ago

FYI. I finally got round to filing that bug report.

http://www.openradar.me/radar?id=6060524114542592

hfossli commented 6 years ago

Awesome. I like that very much.

I found something in the documentation. It states If the specified context has been previously authenticated, the operation will succeed without asking user for authentication.

    @constant kSecUseAuthenticationContext Specifies a dictionary key whose value
        is LAContext to be used for keychain item authentication.
        * If the item requires authentication and this key is omitted, a new context
          will be created just for the purpose of the single call.
        * If the specified context has been previously authenticated, the operation
          will succeed without asking user for authentication.
        * If the specified context has not been previously authenticated, the new
          authentication will be started on this context, allowing caller to
          eventually reuse the sucessfully authenticated context in subsequent
          keychain operations.

That constant is the one I use when I pass the LAContext.

wuf810 commented 6 years ago

I agree but the context is an LAContext and elsewhere in the documentation it implies that if the touchIDAuthenticationAllowableReuseDuration is set then the context should respect it and require a dialog after the duration has "expired".

I'm guessing that in general use LAContext does work like this but when used with the SecItemCopyMatching query it is just applying the rules as documented above i.e. if authenticated then remain authenticated. This is the behaviour that would have been implemented prior to iOS 9 and prior to touchIDAuthenticationAllowableReuseDuration being added.

I guess the best we can hope is that either it is an oversight/bug and they fix it or they at least update the documentation to be clear.

hfossli commented 6 years ago

Yep