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

🦺[Tech Debt] Refactor Crashlytics: BRKeyStore._getData Exceptions at Runtime #288

Open
4 tasks
kcw-grunt opened this issue Dec 4, 2024 · 0 comments
Open
4 tasks
Assignees
Labels
🦺 tech debt Actions that help maintain the repo and application
Milestone

Comments

@kcw-grunt
Copy link
Contributor

kcw-grunt commented Dec 4, 2024

Description

This is a follow on for the issue:🦟[Bug Report] Crashlytics: BRKeyStore._getData Exceptions at Runtime

We have closed the bug and previously refactored the architecture to simplify the method. We are now seeing more meaningful logs.

Definition of Done

  • Resolve the business logic so the user never sees a critical failure (though we still see the crash logs)
  • The failures should be rare to non-existent
  • A more elegant solution for the method
  • A brief post-mortem description of why the code failed so often

Log Snapshot

Recent Crashes of BRKeyStore._getData on v2.12.4
Screenshot 2024-12-04 at 7 51 09 AM

OLD BRKeyStore._getData Synopsis

Developer Intent

Originally, the Breadwallet developers wanted to create a encrypted keystore when the user created a 'Breadwallet' then they wanted the user to sync the changes to the database and update the keystore. The getData is the attempt to synchronize that keystore with the user so if they lose the keystore, they can pay Breadwallet to restore the information.

So in the method private synchronized static byte[] _getData the are checking to see if that is all working. They are throwing an exception on this method (UserNotAuthorized) by treating it as a malicious attack.

Problems with BRKeyStore._getData

Litewallet and the original fork (called Loafwallet) never had a cloud architecture. This is because, our mission is to not collect any PII (Personal Identifiable Information) for Litewallet / Litecoin users. So, there was never a syncing server...and there will not be.

But we still need to call this method to unlock the users Keystore. The crashes happen because somehow that the KeyStore fails

The try/catch chain is horrific. There is no graceful design ...IMHO.
If you look you can see one of the exceptions (all of them?):
- InvalidKeyException
- UserNotAuthenticatedException
- IOException
- CertificateException
- KeyStoreException
- RuntimeException
- UnrecoverableKeyException
- NoSuchAlgorithmException
- NoSuchPaddingException
- InvalidAlgorithmParameterException

Naturally, these exceptions happen often. I feel there needs to be a bulletproof way to handle this. Even if it fails, there should be something meaningful for the user.


Any calls to a remote user keystore will never be part of the business logic of Litewallet

We need to remove any errant calls to this phantom keystore and refactor (REMOVE) this code and squash the bug

Stack trace (via Crashlytics)

https://console.firebase.google.com/u/0/project/litewallet-beta/crashlytics/app/android:com.loafwallet/issues/c4650474272086b7269389b8d857d62e?time=last-seven-days&sessionEventKey=63D7BB41008000015764C66E3776A37B_1772870423120895334

@kcw-grunt kcw-grunt added the 🦺 tech debt Actions that help maintain the repo and application label Dec 4, 2024
@kcw-grunt kcw-grunt added this to the v2.13.X milestone Dec 4, 2024
@kcw-grunt kcw-grunt changed the title GetData 🦺[Tech Debt] Refactor Crashlytics: BRKeyStore._getData Exceptions at Runtime Dec 4, 2024
kcw-grunt pushed a commit that referenced this issue Dec 19, 2024
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🦺 tech debt Actions that help maintain the repo and application
Projects
None yet
Development

No branches or pull requests

2 participants