Introducing a new KeyStoreManager to handle BRKeyStore related #291

andhikayuana · 2024-12-08T09:57:44Z

Overview

To get better understanding of the codebase, we are need to refactor the BRKeyStore into separate class, it can be easier for maintaining the codebase. Now, we are introducing a new KeyStoreManager that aim to replace the BRKeyStore. currently it only called from BRKeyStore._getData.

Key Classes

com.loafwallet.util.cryptography.CipherBox: this will be responsible to encrypt/decrypt the data
com.loafwallet.util.cryptography.CipherStorage: which location the encrypted data can will be stored
com.loafwallet.util.cryptography.KeyStoreKeyGenerator: to generate the secret key
com.loafwallet.util.cryptography.KeyStoreManager: this will be used as a replacement for BRKeyStore later

Next Plan

Add remote config?
Add Unit Test?

andhikayuana · 2024-12-08T09:59:16Z

app/src/main/java/com/breadwallet/BreadApp.java

@@ -45,6 +34,8 @@ public class BreadApp extends Application {
    public static long backgroundedTime;
    private static Activity currentActivity;

+    public static KeyStoreManager keyStoreManager;


for now we just use this, maybe in the future we can use dependency injection like koin

Lets. Feel free to add a spike issue in litewallet-engineering for us to study Koin

sure, will add Koin later

app/src/main/java/com/breadwallet/BreadApp.java

andhikayuana · 2024-12-08T10:00:09Z

app/src/main/java/com/breadwallet/tools/security/BRKeyStore.java

+
+        try {
+            lock.lock();
+            return BreadApp.keyStoreManager.getDataBlocking(new AliasObject(alias, alias_file, alias_iv));


calling new method from KeyStoreManager

app/src/main/java/com/loafwallet/util/cryptography/CipherStorage.kt

andhikayuana · 2024-12-08T11:20:21Z

app/src/main/java/com/breadwallet/tools/security/BRKeyStore.java

+            return BreadApp.keyStoreManager.getDataBlocking(new AliasObject(alias, alias_file, alias_iv));
+        } catch (UserNotAuthenticatedException e) {
+            Timber.e(e, "timber:_getData: showAuthenticationScreen: %s", alias);
+            showAuthenticationScreen(context, request_code, alias);


https://developer.android.com/privacy-and-security/keystore#UserAuthentication

I have questions @kcw-grunt, our Android app will still need user's pin lock screen?

still thinking about this how to migrate without the device lock, since the key (for phrase and canary) that created before need to do device unlock first. related with link above

ref:

litewallet-android/app/src/main/java/com/breadwallet/tools/security/BRKeyStore.java

Line 220 in 7b5a20d

.setUserAuthenticationRequired(auth_required)

litewallet-android/app/src/main/java/com/breadwallet/tools/security/BRKeyStore.java

Line 156 in 7b5a20d

secretKey = createKeys(alias, auth_required);

litewallet-android/app/src/main/java/com/breadwallet/tools/security/BRKeyStore.java

Line 384 in 7b5a20d

return !(strToStore == null || strToStore.length == 0) && _setData(context, strToStore, obj.alias, obj.datafileName, obj.ivFileName, requestCode, true);

litewallet-android/app/src/main/java/com/breadwallet/tools/security/BRKeyStore.java

Line 409 in 7b5a20d

return strBytes.length != 0 && _setData(context, strBytes, obj.alias, obj.datafileName, obj.ivFileName, requestCode, true);

kcw-grunt

LGTM just need rebase / resolve conflicts.
Also, @andhikayuana please confirm this is no-op at this time

app/src/main/java/com/breadwallet/BreadApp.java

kcw-grunt · 2024-12-12T11:43:40Z

app/src/main/java/com/breadwallet/BreadApp.java

@@ -45,6 +34,8 @@ public class BreadApp extends Application {
    public static long backgroundedTime;
    private static Activity currentActivity;

+    public static KeyStoreManager keyStoreManager;


Lets. Feel free to add a spike issue in litewallet-engineering for us to study Koin

kcw-grunt · 2024-12-12T11:44:44Z

app/src/main/java/com/breadwallet/BreadApp.java

@@ -55,16 +46,23 @@ public void onCreate() {
        AnalyticsManager.init(this);
        AnalyticsManager.logCustomEvent(BRConstants._20191105_AL);

-        if(BuildConfig.DEBUG) Timber.plant(new Timber.DebugTree());
+        if (BuildConfig.DEBUG) Timber.plant(new Timber.DebugTree());


Was this the linter or Andhika manual @andhikayuana ?

I do manually before commit

josikie

That's good you want to separate the classes so will be easy to maintain

josikie · 2024-12-13T18:04:39Z

app/src/main/java/com/breadwallet/tools/security/BRKeyStore.java

+            return BreadApp.keyStoreManager.getDataBlocking(new AliasObject(alias, alias_file, alias_iv));
+        } catch (UserNotAuthenticatedException e) {
+            Timber.e(e, "timber:_getData: showAuthenticationScreen: %s", alias);
+            showAuthenticationScreen(context, request_code, alias);


I have questions @kcw-grunt, our Android app will still need user's pin lock screen?

…toreManager to handle keystore related

…RKeyStore

…_pin.xml" This reverts commit 04b02ac.

kcw-grunt

Just holding until pairing

kcw-grunt · 2024-12-19T10:59:31Z

@josikie We ultimately want to completely remove the device PIN...then later let user choose from Biometric or app 6 digit PIN

josikie · 2024-12-19T13:27:34Z

Ok, noted! @kcw-grunt

Does it mean when user choose biometric they should already have biometric lock in their phone?

I think we've tried to implement biometric in the past for our new Android UI. When we wanted to implement biometric on our new UI Android, Android phone need user to set their biometric in their phone first, then they can use it in our new UI Android

kcw-grunt · 2024-12-19T14:55:55Z

Ok, noted! @kcw-grunt

Does it mean when user choose biometric they should already have biometric lock in their phone?

I think we've tried to implement biometric in the past for our new Android UI. When we wanted to implement biometric on our new UI Android, Android phone need user to set their biometric in their phone first, then they can use it in our new UI Android

@andhikayuana matched the logic of iOS 👍

* chore: remove forgot_seed_phrase_or_pin_text text at activity_pin.xml * fix: [#288] catch all exception and return null * chore: wip keystore related * chore: wip keystore related for CipherBox * chore: wip keystore related for CipherBox * feat: add new CipherBox, CipherStorage, KeyStoreKeyGenerator and KeyStoreManager to handle keystore related * feat: add keyStoreManager to BreadApp and change _getData method at BRKeyStore * Revert "chore: remove forgot_seed_phrase_or_pin_text text at activity_pin.xml" This reverts commit 04b02ac. * chore: resolve conflict and implement toggle for new KeyStoreManager

andhikayuana self-assigned this Dec 8, 2024

andhikayuana requested review from kcw-grunt and josikie and removed request for kcw-grunt December 8, 2024 09:58

andhikayuana commented Dec 8, 2024

View reviewed changes

app/src/main/java/com/breadwallet/BreadApp.java Outdated Show resolved Hide resolved

andhikayuana commented Dec 8, 2024

View reviewed changes

app/src/main/java/com/loafwallet/util/cryptography/CipherStorage.kt Show resolved Hide resolved

andhikayuana marked this pull request as ready for review December 8, 2024 10:04

andhikayuana changed the title ~~Introducting KeyStoreManager to handle BRKeyStore related~~ Introducing a new KeyStoreManager to handle BRKeyStore related Dec 8, 2024

andhikayuana commented Dec 8, 2024

View reviewed changes

kcw-grunt suggested changes Dec 12, 2024

View reviewed changes

andhikayuana force-pushed the bugfix/288 branch from 8bad496 to 6770f7c Compare December 13, 2024 10:08

josikie reviewed Dec 13, 2024

View reviewed changes

josikie approved these changes Dec 13, 2024

View reviewed changes

andhikayuana added 9 commits December 19, 2024 17:14

chore: remove forgot_seed_phrase_or_pin_text text at activity_pin.xml

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

bc75134

fix: [#288] catch all exception and return null

ada519b

chore: wip keystore related

c12950a

chore: wip keystore related for CipherBox

2b6b326

chore: wip keystore related for CipherBox

e6a608c

feat: add new CipherBox, CipherStorage, KeyStoreKeyGenerator and KeyS…

64995c6

…toreManager to handle keystore related

feat: add keyStoreManager to BreadApp and change _getData method at B…

70ae2bd

…RKeyStore

Revert "chore: remove forgot_seed_phrase_or_pin_text text at activity…

65ded7c

…_pin.xml" This reverts commit 04b02ac.

chore: resolve conflict and implement toggle for new KeyStoreManager

Loading
Loading status checks…

cf3952a

andhikayuana force-pushed the bugfix/288 branch from 6770f7c to cf3952a Compare December 19, 2024 10:18

andhikayuana requested a review from kcw-grunt December 19, 2024 10:23

kcw-grunt approved these changes Dec 19, 2024

View reviewed changes

kcw-grunt merged commit 59540fb into develop Dec 19, 2024
0 of 2 checks passed

kcw-grunt deleted the bugfix/288 branch December 19, 2024 14:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introducing a new KeyStoreManager to handle BRKeyStore related #291

Introducing a new KeyStoreManager to handle BRKeyStore related #291

andhikayuana commented Dec 8, 2024

andhikayuana Dec 8, 2024

kcw-grunt Dec 12, 2024

andhikayuana Dec 13, 2024

andhikayuana Dec 8, 2024

andhikayuana Dec 8, 2024

josikie Dec 13, 2024

andhikayuana Dec 13, 2024

kcw-grunt left a comment

kcw-grunt Dec 12, 2024

kcw-grunt Dec 12, 2024

andhikayuana Dec 13, 2024

josikie left a comment

josikie Dec 13, 2024

kcw-grunt left a comment

kcw-grunt commented Dec 19, 2024

josikie commented Dec 19, 2024

kcw-grunt commented Dec 19, 2024

Introducing a new KeyStoreManager to handle BRKeyStore related #291

Introducing a new KeyStoreManager to handle BRKeyStore related #291

Conversation

andhikayuana commented Dec 8, 2024

Overview

Key Classes

Next Plan

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kcw-grunt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josikie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kcw-grunt left a comment

Choose a reason for hiding this comment

kcw-grunt commented Dec 19, 2024

josikie commented Dec 19, 2024

kcw-grunt commented Dec 19, 2024