Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Android: Fingerprint authentication not implemented yet #116

Open
alexanderjarvis opened this issue Mar 20, 2018 · 29 comments
Open

Android: Fingerprint authentication not implemented yet #116

alexanderjarvis opened this issue Mar 20, 2018 · 29 comments

Comments

@alexanderjarvis
Copy link

It's possible to get if the device supports fingerprint authentication with getSupportedBiometryType() but the keychain item doesn't use the fingerprint authentication (or provide options for doing so).

Looking at

//.setUserAuthenticationRequired(true) // Will throw InvalidAlgorithmParameterException if there is no fingerprint enrolled on the device

The code is commented out. I've tried playing with this feature and uncommenting the code and even passing if the device supports fingerprint auth as an extra argument to encrypt.

However I can't get it working and there's nothing in the logs to suggest what is wrong.

@oblador
Copy link
Owner

oblador commented Mar 24, 2018

@alexanderjarvis It's on the todo, but I'm afraid my Android skills are somewhat lacking and the scope of work is fairly big. Storing it with fingerprint encryption isn't that hard, but unlocking it is a pretty big task as it involves a fair bit of UI and logic on how to handle retries etc which iOS neatly just handles for you. I'd be eternally grateful for help with this and could brief anybody willing to take a stab at it of the complexities involved.

@alexanderjarvis
Copy link
Author

@oblador thanks for your reply. It's a shame that unlocking it is not so straightforward. If I get some time I may look at doing it and create a PR.

@pcoltau
Copy link
Collaborator

pcoltau commented Mar 26, 2018

I would recommend that we don't add any UI to this library, but simply notify (through an event?) the app when authentication is needed and then wait for the app to tell us if that was successful or not and then proceed.

@FlaviooLima
Copy link

Hope to see this implemented soon :)
With this implemented this package can go to the 🌔 🚀 🚀 🚀 🚀

@oblador
Copy link
Owner

oblador commented May 31, 2018

Any takers to help out with this? 🙏

@jacksontbryan
Copy link

React native touch id has done some work around fingerprint.

https://github.com/naoufal/react-native-touch-id/tree/master/android

Could this work be leveraged to accelerate this feature?

@cladjules
Copy link

Let me try to add some native code to Android. See how we could link that up easily and PR.

@LinusU
Copy link

LinusU commented Aug 2, 2018

API Level 28 introduces a complete dialog for this: https://developer.android.com/reference/android/hardware/biometrics/BiometricPrompt

Here is a compat lib: https://github.com/fython/BiometricPromptCompat

I of course found all of this just after implementing it myself 😭 😂

@oblador what do you think about shipping with this dialog? I could possibly whip up a PR, need this in an app I'm working on...

@cladjules
Copy link

cladjules commented Aug 2, 2018

@oblador @LinusU I have started some work around it, using the current API (FingerPrintManager), which doesn't offer a native UI for it, you have to implement your own.

At least the logic is there, the only problem, for now, is, you have to put your fingerprint to Encode as well as to Decode the keychain.
I have seen some scenario, where the encoding doesn't require your fingerprint.
https://github.com/cladjules/react-native-keychain

I believe it's a matter of using a different algorithm.

@LinusU
Copy link

LinusU commented Aug 6, 2018

I have a working implementation of UI + logic here: https://github.com/LinusU/react-native-biopass

It's using RSA so that it doesn't require a fingerprint to encode, only to decode.

I'd be interested in PRing that up to this repo if we want to include the UI part here. That way I would get iOS support without implementing it myself ☺️

@cladjules
Copy link

@LinusU That's really good.

I managed to merge some of your code into my PR.
You can now encrypt without using fingerprint and decrypt using it.
https://github.com/cladjules/react-native-keychain

At least, there is now a separate cipherStorage so it's easier to manage and in a different file.

I will try and add the BiometricPromptCompat so it uses both Compat and BiometricPrompt code.

@LinusU
Copy link

LinusU commented Sep 10, 2018

I will try and add the BiometricPromptCompat so it uses both Compat and BiometricPrompt code.

That's awesome!

Let me know if you need any help ☺️

Jyrno42 added a commit to thorgate/react-native-keychain that referenced this issue Sep 11, 2018
@Jyrno42
Copy link
Contributor

Jyrno42 commented Sep 11, 2018

Since I needed this asap I took the modifications by @cladjules and rebased them to master. Then I did a naive integration with BiometricPromptCompat. Feel free to use it (or not use it).

Note: I also bumped the ENCRYPTION_KEY_SIZE to 512 for CipherStorageKeystoreRSAECB since I need to store more than 53 characters.

E: added note

Jyrno42 added a commit to thorgate/react-native-keychain that referenced this issue Sep 11, 2018
@LinusU
Copy link

LinusU commented Sep 12, 2018

Great to see things happening here! I would love to see this work upstreamed as soon as possible, and would be happy to help with this.

@oblador what are your thoughts on this? How do we best get this merged back?

@cladjules
Copy link

I'll try to do a more official implementation of BiometricPrompt, anything I could re-use @Jyrno42 would be great.

Will try to do this weekend.

@vonovak
Copy link
Collaborator

vonovak commented Sep 12, 2018

@LinusU I will be able to do code review for android

@oblador
Copy link
Owner

oblador commented Sep 12, 2018

@LinusU @Jyrno42: I'm sorry that my time is somewhat lacking, so without researching much or even reading this thread attentively I have some thoughts/pointers:

  • Ideally we would be using the fingerprint to encrypt the secret like iOS does. Nothing is currently preventing anyone from doing regular fingerprint auth and then retrieving the secret in your app with mentioned libraries, but it's less secure. Will this BiometricPrompt enable us to encrypt/decrypt using the fingerprint?
  • Make sure to take localization into account (simplest done by accepting custom strings as input).
  • Super excited to finally get this in! 👏 😄

@LinusU
Copy link

LinusU commented Sep 12, 2018

Will this BiometricPrompt enable us to encrypt/decrypt using the fingerprint?

Yes. My implementation in BioPass only allows reading the value if the fingerprint is successfully scanned, otherwise the Android key system won't hand out the private key.

@oblador
Copy link
Owner

oblador commented Sep 12, 2018

@LinusU That's great, would prefer to embed that functionality into this package rather than requiring users to integrate another. Do you think @Jyrno42's work is on the right track? Would you like to own this? What help aside from reviewing do you need from us?

@LinusU
Copy link

LinusU commented Sep 12, 2018

That's great, would prefer to embed that functionality into this package rather than requiring users to integrate another

Absolutely, that's what I want too 😄

Do you think @Jyrno42's work is on the right track?

Yes! I'm looking thru the changes now, and it looks good.

Would you like to own this?

Would love to 🙌

What help aside from reviewing do you need from us?

Just reviewing ☺️

@LinusU
Copy link

LinusU commented Sep 12, 2018

@Jyrno42 5c00a5d doesn't have anything to do with fingerprint, right? Can you submit that as a separate pull request?

@cladjules
Copy link

cladjules commented Sep 12, 2018

@oblador Ideally we would be using the fingerprint to encrypt the secret like iOS does. Nothing is currently preventing anyone from doing regular fingerprint auth and then retrieving the secret in your app with mentioned libraries, but it's less secure. Will this BiometricPrompt enable us to encrypt/decrypt using the fingerprint?

The implementation of FingerprintManager, does encrypt directly the Keychain entry, so it does hardware encryption.

I suggest we modify BiometricPromptCompat which uses both BiometricPrompt and FingerprintManager and directly encrypt the Cipher.
It doesn't look like the default BiometricPromptCompat allows you to pass a Cipher to it. So that wouldn't really do a hardware encryption.

@oblador
Copy link
Owner

oblador commented Sep 12, 2018

@cladjules Cool, maybe collaborate with @LinusU on his PR?

@LinusU Amazing 💯 😄

@cladjules
Copy link

@LinusU I think we miss an important point and we would need to pass the Cipher to BiometricPrompt and FingerprintManager using the FingerprintCrypto.CryptoObject or BiometricPrompt.CryptoObject when calling authenticate (which is always null with the Compat) library.

That would require using the same Cipher to both encrypt that we can retrieve in the Fingerprint callback.

I will update the PR, but we definitely need to add some bits to the Compat one too.

@Jyrno42
Copy link
Contributor

Jyrno42 commented Sep 12, 2018

@Jyrno42 5c00a5d doesn't have anything to do with fingerprint, right? Can you submit that as a separate pull request?

Will do

E: #150 is out

@Timm0
Copy link

Timm0 commented Oct 15, 2018

Is there any progress on this?

@jordanfloyd
Copy link

Also interested in this. It would be nice to use a single library, but I can use another Fingerprint auth library if needed.

@vonovak
Copy link
Collaborator

vonovak commented Oct 16, 2018

please track progress in #148

@OleksandrKucherenko
Copy link
Contributor

#260 - PR with implemented and tested biometric. Enjoy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.