Android Fingerprint #195

cladjules · 2019-02-14T15:52:42Z

Refactor Android Support Fingerprint to not use AndroidX.
Still need to refactor to not force FragmentActivity to be used.

…ttached

vonovak · 2019-02-14T16:16:54Z

hi! Thanks for the PR! For start, can you please rebase on master so there are no conflicts? Thanks.

MarcoLude · 2019-03-06T15:04:00Z

any updates on this?

aeirola · 2019-04-08T11:04:31Z

It seems as if there are now two open, but stalled, PRs for implementing Android Fingerprint support, this one and #148. I'm working on an application which is currently using a custom branch from the #148 PR. The feature seems to work reasonably well, and since we depend on this library I'd be willing to help out finalising implementation of this feature in the library.

But before making yet another PR, I'd just like to check in with @vonovak @cladjules and @LinusU about the state of the development, and what functionality is still missing until the feature could be merged.

Some open question I've seen in the discussion:

How should we handle the dialog prompt on older versions of Android?
- This PR uses a local fork of ~~AndroidX~~ Googles library refactored to use compat lib
- [WIP] Android fingerprint #148 uses moe.feng.support.biometricprompt:library, which has been deprecated
- AndroiX support seems to be coming in RN 0.60 ([META] AndroidX migration plan facebook/react-native#23112 (comment)). Maybe we don't want to wait for that though?
Should we try to get rid of the need for FragmentActivity? Personally I don't have a problem with it.
Should we try to switch from RSA to AES (as suggested in [WIP] Android fingerprint #148 (comment)) or OAEP (as suggested in [WIP] Android fingerprint #148 (comment))?
Customising the authentication prompt. Currently the displayed texts are hardcoded in both PRs, but the application I work on would really benefit from customised strings. This additional functionality could be added in a separate PR though.

Would be good to have answers to these questions, or at least plans on how to solve them, before continuing work. All input is welcome.

P.S. I chose this PR as the ground for discussion, as opposed to creating a separate issue for the feature, in order to not spread the discussion any further. I can move the discussion to a separate issue if someone feels like this is not the appropriate place for the discussion.

cladjules · 2019-04-08T11:11:52Z

It seems as if there are now two open, but stalled, PRs for implementing Android Fingerprint support, this one and #148. I'm working on an application which is currently using a custom branch from the #148 PR. The feature seems to work reasonably well, and since we depend on this library I'd be willing to help out finalising implementation of this feature in the library.

But before making yet another PR, I'd just like to check in with @vonovak @cladjules and @LinusU about the state of the development, and what functionality is still missing until the feature could be merged.

Some open question I've seen in the discussion:

How should we handle the dialog prompt on older versions of Android?

This PR uses a local fork of AndroidX

[WIP] Android fingerprint #148 uses moe.feng.support.biometricprompt:library, which has been deprecated

AndroiX support seems to be coming in RN 0.60 (facebook/react-native#23112 (comment)). Maybe we don't want to wait for that though?

Should we try to get rid of the need for FragmentActivity? Personally I don't have a problem with it.

Should we try to switch from RSA to AES (as suggested in #148 (comment)) or OAEP (as suggested in #148 (comment))?

Customising the authentication prompt. Currently the displayed texts are hardcoded in both PRs, but the application I work on would really benefit from customised strings. This additional functionality could be added in a separate PR though.

Would be good to have answers to these questions, or at least plans on how to solve them, before continuing work. All input is welcome.

P.S. I chose this PR as the ground for discussion, as opposed to creating a separate issue for the feature, in order to not spread the discussion any further. I can move the discussion to a separate issue if someone feels like this is not the appropriate place for the discussion.

Hi @aeirola sorry for being inactive for so long.
To answer your questions,
this PR doesn't use AndroidX, I have re-used the Google Library and refactored to use compat lib instead.

Forcing all users to switch their own code base to use FragmentActivity could potentially cause some issues with other libraries as well, I am not sure what would be involved in showing it inside a detached dialog or activity, but I could give it a go.

We cannot switch to RSA to AES as we need an algorithm that generate private/public key, that's the reason why the algorithm is different for both file, I don't know about OAEP and how it works, but I think it only generated public key.

Customising the authentication prompt definitely doable and pass it through the options.

aeirola · 2019-04-08T13:01:17Z

Hey @cladjules, thanks for the prompt reply.

Sorry for misinterpreting the source of the biometric prompt fork. Thanks for correcting.

Yeah, I do agree that forcing apps to use the FragmentActvitity wouldn't be optimal, but I could see it at least as a stepping stone towards getting the feature implemented. If you are eager, and have the time, to try out alternative solutions, that would be great! Otherwise I could see it as an option to mere experimental fragment-dependant fingerprint support into the library, and then improve on it later.

Unfortunately I'm not familiar with the cryptosystems available in Android. Could look into them in detail if there is a chance that there would be some potentially undiscovered solutions available.

aeirola · 2019-04-08T13:27:11Z

Quickly browsed through Android keystore documentation, and seems like OAEP is referring to an alternative padding scheme for RSA, which is supported from Android 23+ (https://developer.android.com/training/articles/keystore#SupportedCiphers). Thus it could be used instead of the PKCS1 padding used by the current code. Not an expert on the implications of the different padding methods, but I really don't think there is much difference between them for this use case.

Android 23+ also includes support for elliptic curve keypair generators (https://developer.android.com/training/articles/keystore#SupportedKeyPairGenerators), so if there are some security concerns with RSA, it might be better to switch to it instead.

poocart · 2019-10-04T14:46:48Z

Interesting issue @poocart, thanks for bringing it up. I remember there being some previous discussion around setUserAuthenticationValidityDurationSeconds at #148 (comment). Not sure what the cases are that @Jyrno42 referred to when he mentioned that decryption will fail for values less than 2 seconds. Maybe those issues are somehow fixed elsewhere in the code. The comment was for quite an old PR after all.

I'm sorry, actually you're right, setting it as 0 or -1 (as per method description) solves my issue, but then blocks from regular keychain unlock, however I finally managed to get working approach by disabling the code which tries to decrypt from keystore before showing biometrics prompt – this can be found around line 246 in keychain/cipherStorage/CipherStorageKeystoreRSAECB.java.

vascofg · 2019-10-07T09:49:47Z

Any ETA on this functionality?

OleksandrKucherenko · 2019-10-07T12:14:09Z

I start thinking it's much easier to close PR and start a new one than rebase it...

cladjules · 2019-10-07T12:16:57Z

Hi everyone. I am now back from holidays.

There are indeed still few crashes.

Google has now officially released to androidx library.
I will give it a go to update this PR using the Gradle package.
And also merge the conflicts.

OleksandrKucherenko

@cladjules
If you can fix the conflicts and commit update, then I will be happy to update code by applying android "best practices" on it

OleksandrKucherenko · 2019-10-07T12:52:37Z

android/build.gradle

 }

 dependencies {
-  implementation 'com.facebook.react:react-native:+'
-  implementation 'com.facebook.conceal:conceal:1.1.3@aar'
+    implementation 'com.android.support:support-v4:27.0.2'


we should align to version 28.0.0 - it guaranty that androidx with jetifier tool will be without any problems.

OleksandrKucherenko · 2019-10-07T12:52:52Z

android/gradle/wrapper/gradle-wrapper.properties

 distributionBase=GRADLE_USER_HOME
 distributionPath=wrapper/dists
 zipStoreBase=GRADLE_USER_HOME
 zipStorePath=wrapper/dists
-distributionUrl=https\://services.gradle.org/distributions/gradle-2.8-all.zip
+distributionUrl=https\://services.gradle.org/distributions/gradle-4.6-all.zip


5.6.2 is the latest

OleksandrKucherenko · 2019-10-07T12:55:45Z

android/src/main/java/com/oblador/keychain/cipherStorage/CipherStorage.java

    EncryptionResult encrypt(@NonNull String service, @NonNull String username, @NonNull String password, SecurityLevel level) throws CryptoFailedException;

-    DecryptionResult decrypt(@NonNull String service, @NonNull byte[] username, @NonNull byte[] password) throws CryptoFailedException;
+    void decrypt(@NonNull DecryptionResultHandler decryptionResultHandler, @NonNull String service, @NonNull byte[] username, @NonNull byte[] password) throws CryptoFailedException, KeyPermanentlyInvalidatedException;


Its a bad idea to change existing API. we should extend Public interface with new versions of API methods instead.

Correct approach:

mark old api deprecated

introduce new api with additional parameters/changes

provide a migration code for users

after 2-3 releases remove deprecated API methods

OleksandrKucherenko · 2019-10-07T12:56:51Z

android/src/main/java/com/oblador/keychain/cipherStorage/CipherStorage.java

    int getMinSupportedApiLevel();

+    void setCurrentActivity(Activity activity);


keeping a reference on activity is always not a very good idea. It can cause memory leak.

OleksandrKucherenko · 2019-10-07T13:08:33Z

Sorry for stupid questions, but I have to ask to make it clear:

PR contains 31 commits, and I have a feeling that PR should be rebased on top of the current MASTER. it looks a little strange to me that we instead of rebasing, use merge of original/master into PR branch.

Does it mean that this PR will be a standalone branch in the future? or it was just a tricky way of applying the changes on code?

Thanks for answers

cladjules · 2019-10-07T13:15:48Z

Sorry for stupid questions, but I have to ask to make it clear:

PR contains 31 commits, and I have a feeling that PR should be rebased on top of the current MASTER. it looks a little strange to me that we instead of rebasing, use merge of original/master into PR branch.

Does it mean that this PR will be a standalone branch in the future? or it was just a tricky way of applying the changes on code?

Thanks for answers

I suggest once we are happy with the state of the PR, we do a rebase?
All the commits are within my fork anyway.

Thanks.

OleksandrKucherenko · 2019-10-07T13:43:45Z

Sorry for stupid questions, but I have to ask to make it clear:

PR contains 31 commits, and I have a feeling that PR should be rebased on top of the current MASTER. it looks a little strange to me that we instead of rebasing, use merge of original/master into PR branch.

Does it mean that this PR will be a standalone branch in the future? or it was just a tricky way of applying the changes on code?
Thanks for answers

I suggest once we are happy with the state of the PR, we do a rebase?
All the commits are within my fork anyway.

Thanks.

I try to rebase it today... and it's somehow become a "falling snowball" that on each applied commit, produces so many changes that it becomes harder and harder to resolve the conflicts.

OleksandrKucherenko · 2019-10-07T14:36:43Z

android {
  compileOptions {
    sourceCompatibility JavaVersion.VERSION_1_8
    targetCompatibility JavaVersion.VERSION_1_8
  }
}

add this into build.gradle file for enabling lambdas and simplified syntax.

cladjules · 2019-10-19T12:28:10Z

Hi all,
I have refactored the library.
It's now using androidX Compat Biometric,
I have merged the current master to it.
And applied some of the comments.

I didn't test it with an RN App as mine is not 0.60 yet.

I will give it a go but I am not sure it will be a simple upgrade.

OleksandrKucherenko · 2019-10-21T12:55:49Z

I create alternative implementation, based on AndroidX biometric library - #260

used the same code architecture/design concept as in this PR

aeirola · 2019-10-30T13:26:14Z

Nice to see that @OleksandrKucherenko is also eager about biometric authentication for android in the library. Even though there is now a third PR about the same subject, I would suggest to finalize and merge this PR, and then continue the work Oleksandr has done in separate PRs. This includes some refactoring, Java unit tests etc.

There still seems to be some steps left before this PR could be merged. The most immediate that come to mind are:

Fixing the merge conflict markers in gradle/wrapper/gradle-wrapper.properties
Removing the now unused string resource files
Clean up commented code and old comments
Remove editor files like react-native-keychain.iml
Check the remaining review comments

Is this something that you @cladjules would have time to finalize in the near future?

cladjules · 2019-10-31T09:11:54Z

Hi, @aeirola more than happy to do that.
But is there really a point as @OleksandrKucherenko has already re-used all my code in a separate PR.
My PR now only uses AndroidX as well,
so I guess it would make sense to choose what PR we should merge and work on?

I am migrating to RN 0.6X soon, so happy to do some testing with either PR.

Thanks.

aeirola · 2019-10-31T10:24:26Z

I was thinking that the #260 PR is quite large as it is, and would maybe be easier to progress by first introducing initial fingerprint functionality through this PR, and then separately bring in the additional changes by @OleksandrKucherenko in later PRs. This would make it easier and faster to review the changes. I would imagine all the changes to be released under the same version number though.

OleksandrKucherenko · 2019-10-31T20:34:44Z

@aeirola this PR contains files that belong to androidx biometric library. I recommend excluding them from PR and instead use reverse-jetified version of biometric AAR as project dependency.

That will reduce the PR size greatly.

cladjules · 2020-01-11T15:48:59Z

Hi guys,
it has been a while.
I have finally upgraded my App to RN 0.6x and fully use AndroidX.

I have done the changes on that lib and only uses AndroidX BiometricPrompt, seems to be working OK on both Android Pie and pre-Pie.

I will look at merging the current master branch and resolve conflict.

Would be good if you can review on your side.

Thanks :)

…hain

oblador · 2020-03-04T10:47:33Z

Superseded by #260

oblador · 2020-03-04T10:54:06Z

@cladjules Are there are pieces in this PR not currently present in master that you want in?

cladjules · 2020-04-15T08:26:57Z

@oblador Sorry, I haven't been very active those last few months.
@OleksandrKucherenko Well done for getting that merged and working.

In my PR, I had done an implementation of canImplyAuthentication for Android, wondering if it may be worth doing a separate PR to add that?

Thanks :)

oblador · 2020-04-15T09:39:18Z

Absolutely!

cladjules and others added 22 commits July 17, 2018 21:59

Added fingerprintManager for Android biometry support

e988f29

added algo iv for decrypt

46ce672

Cleanup

bac96c7

added separate CipherStorageKeystoreRSAECB for biometry only

c67692f

cancel fingerprintCancellation when auth fails

27a463e

rn keychain compat bio

2e530de

refactor androidx biometric into app compat v4

14f9139

fixed issue and import

3052876

added check for fingerprint and set proper permissiosn

fa8b7ad

cleanup with commits from PR

2c86afe

typo fix

a11691c

edit with pr

795a487

cleanup and added commits from the PR

8d5db02

detect biometry cipher storage when removing old data

315881d

set proper ios method definition

fa2a294

check if cipherStorage has changed and update ios project

3c44296

cleanup import and set setText in a UI Handler

b1c97fa

set exceptino to InvalidKeyException

fa57fc0

fixed issue with android < 5.0

943333d

Added check for android < M and getting biometry cipher

e18fe20

Removed exception thrown when authFailed and check fragment already a…

8886c35

…ttached

added nullPointerException

632dd85

cladjules mentioned this pull request Feb 14, 2019

[WIP] Android fingerprint #148

Closed

MarcoLude mentioned this pull request Mar 6, 2019

Question Android: how should the library work on Android? #194

Closed

OleksandrKucherenko reviewed Oct 7, 2019

View reviewed changes

aeirola mentioned this pull request Oct 15, 2019

Move to AndroidX prompt cladjules/react-native-keychain#2

Closed

cladjules added 4 commits October 19, 2019 13:13

Refactor using androidX Library

b77c278

cleanup imports

5752483

cleanup code and variables

3e985e2

Merge https://github.com/oblador/react-native-keychain

a88b962

OleksandrKucherenko mentioned this pull request Oct 21, 2019

AndroidX biometric manager #260

Merged

add activity to cipherStorage

dce043c

cladjules added 2 commits January 11, 2020 15:55

Merge branch 'master' of https://github.com/oblador/react-native-keyc…

1507676

…hain

merge with master and added methods

4beddc6

oblador closed this Mar 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Android Fingerprint #195

Android Fingerprint #195

cladjules commented Feb 14, 2019

vonovak commented Feb 14, 2019

MarcoLude commented Mar 6, 2019

aeirola commented Apr 8, 2019 •

edited

Loading

cladjules commented Apr 8, 2019

aeirola commented Apr 8, 2019

aeirola commented Apr 8, 2019

poocart commented Oct 4, 2019 •

edited

Loading

vascofg commented Oct 7, 2019

OleksandrKucherenko commented Oct 7, 2019

cladjules commented Oct 7, 2019

OleksandrKucherenko left a comment •

edited

Loading

OleksandrKucherenko Oct 7, 2019

OleksandrKucherenko Oct 7, 2019

OleksandrKucherenko Oct 7, 2019

OleksandrKucherenko Oct 7, 2019

OleksandrKucherenko commented Oct 7, 2019

cladjules commented Oct 7, 2019

OleksandrKucherenko commented Oct 7, 2019

OleksandrKucherenko commented Oct 7, 2019

cladjules commented Oct 19, 2019

OleksandrKucherenko commented Oct 21, 2019 •

edited

Loading

aeirola commented Oct 30, 2019

cladjules commented Oct 31, 2019

aeirola commented Oct 31, 2019

OleksandrKucherenko commented Oct 31, 2019 •

edited

Loading

cladjules commented Jan 11, 2020

oblador commented Mar 4, 2020

oblador commented Mar 4, 2020

cladjules commented Apr 15, 2020

oblador commented Apr 15, 2020

		int getMinSupportedApiLevel();

		void setCurrentActivity(Activity activity);

Android Fingerprint #195

Android Fingerprint #195

Conversation

cladjules commented Feb 14, 2019

vonovak commented Feb 14, 2019

MarcoLude commented Mar 6, 2019

aeirola commented Apr 8, 2019 • edited Loading

cladjules commented Apr 8, 2019

aeirola commented Apr 8, 2019

aeirola commented Apr 8, 2019

poocart commented Oct 4, 2019 • edited Loading

vascofg commented Oct 7, 2019

OleksandrKucherenko commented Oct 7, 2019

cladjules commented Oct 7, 2019

OleksandrKucherenko left a comment • edited Loading

Choose a reason for hiding this comment

OleksandrKucherenko Oct 7, 2019

Choose a reason for hiding this comment

OleksandrKucherenko Oct 7, 2019

Choose a reason for hiding this comment

OleksandrKucherenko Oct 7, 2019

Choose a reason for hiding this comment

OleksandrKucherenko Oct 7, 2019

Choose a reason for hiding this comment

OleksandrKucherenko commented Oct 7, 2019

cladjules commented Oct 7, 2019

OleksandrKucherenko commented Oct 7, 2019

OleksandrKucherenko commented Oct 7, 2019

cladjules commented Oct 19, 2019

OleksandrKucherenko commented Oct 21, 2019 • edited Loading

aeirola commented Oct 30, 2019

cladjules commented Oct 31, 2019

aeirola commented Oct 31, 2019

OleksandrKucherenko commented Oct 31, 2019 • edited Loading

cladjules commented Jan 11, 2020

oblador commented Mar 4, 2020

oblador commented Mar 4, 2020

cladjules commented Apr 15, 2020

oblador commented Apr 15, 2020

aeirola commented Apr 8, 2019 •

edited

Loading

poocart commented Oct 4, 2019 •

edited

Loading

OleksandrKucherenko left a comment •

edited

Loading

OleksandrKucherenko commented Oct 21, 2019 •

edited

Loading

OleksandrKucherenko commented Oct 31, 2019 •

edited

Loading