Use RNSecureKeyStore for auth token persistent storage

[2624789]

Related issue: https://github.com/GaloyMoney/galoy-mobile/issues/848

KeyStoreWrapper was extended to store/retrieve/remove secure state data.

• SecurePersistentState is a new subset type of PersistentState created to identify which keys of the persistent state should be stored securely
• When loading/saving persistent state the secure part is handled by KeyStore and non secure part keeps being handled by AsyncStorage
• After upgrading, the token will be loaded from AsyncStorage and immediately after loaded it will be persisted in secure storage and it will continue loading from there. This way users don't need to log back in.
• PersitentState type was not modified so migrations were not required

[nicolasburtey]

I quickly skim through the PR, first comments are:

• there are no tests. we need tests for something that is one of the most critical point in the app: token management.
• I don't fully understand where the upgrade path of occurring: I don't think the existing save token is removed from non secured storage.

[UncleSamtoshi]

General approach looks good! I think the logic is correct, and it will actually delete the token from local storage however the code may require some type changes and name changes for clarity.

In the past the PersistentState is a direct mapping to something that has been stored in local storage and migrateAndGetPersistentState is supposed to take one of these objects and bring it up to the latest version. I think it convolutes the logic to manipulate the data being passed into this function. There is probably a way where the secure storage state and local storage state could be combined more explicitly after migrateAndGetPersistentState occurs. It may require some name changing to get it semantically correct.

[2624789]

Thanks for the feedback @nicolasburtey and @UncleSamtoshi

there are no tests

I'll add the tests once the logic is approved

I don't think the existing save token is removed from non secured storage.

It is removed here:

const savePersistentState = async (state: PersistentState) => {
  const { galoyAuthToken, ...data } = state
  saveJson(PERSISTENT_STATE_KEY, data)
  KeyStoreWrapper.setSecurePersitentState({ galoyAuthToken })
}

The saveJson function will replace whatever state that is previously stored in the non secure storage, and because the token is moved apart by destructuring the state, it will be deleted/not stored in the non secure storage.

In the past the PersistentState is a direct mapping to something that has been stored in local storage and migrateAndGetPersistentState is supposed to take one of these objects and bring it up to the latest version. I think it convolutes the logic to manipulate the data being passed into this function.

I think the PersistentState is still a direct mapping to something that has been stored locally. The difference now is that we are using two storage methods, one secure and one non-secure. As I see, the data passed to migrateAndGetPersistentState is not being manipulated, just gathered from two different sources.

There is probably a way where the secure storage state and local storage state could be combined more explicitly after migrateAndGetPersistentState occurs. It may require some name changing to get it semantically correct.

Combining the secure storage state and local storage state after migrateAndGetPersistentState implies that the data stored securely is something apart from the PersistentState.

Do you think that changing KeyStoreWrapper.getSecurePersistentState() to KeyStoreWrapper.getSecureData() will make it more explicit that what is being retrieved is just raw secure data that then will be converted into PersistentState by migrateAndGetPersistentState?

[UncleSamtoshi]

I think the PersistentState is still a direct mapping to something that has been stored locally. The difference now is that we are using two storage methods, one secure and one non-secure.

Previously the PersistentState was coupled directly to the local storage which is under a version control that is possible to be migrated. The secure storage state isn't under a version control which I think is okay because its so simple. However I think it is valuable to keep the integrity of the version control for local storage and be explicit about what is stored in each place.

This could look like PersistentState staying the same type, but then renaming all the existing PersistentState stuff related to local storage. For example migrateAndGetPersistentState would change to migrateAndGetLocalStorageState and then you would combine the LocalStorageState with the secure storage state to create the PersistentState.

Perhaps this is clunky and there is a refactor that simplifies it all, but I think as long as the version controlled local storage state exists, it makes sense for us to explicitly know what is stored in local storage and what is stored in secure storage.

Any thoughts @nicolasburtey

[nicolasburtey]

I'll add the tests once the logic is approved

that is fine, but tests are often the best way to make explicit what the logic aim to be

The saveJson function will replace whatever state that is previously stored in the non secure storage, and because the token is moved apart by destructuring the state, it will be deleted/not stored in the non secure storage.

yup thanks I see that now. I missed the destructuring

Any thoughts @nicolasburtey

At a minimum, PersistentState_6 is assuming a galoyAuthToken, so this would need to be fixed.

Another thing that come to mind: there has been a frequent user request to be able to have several account on the mobile wallet at once. one for personal for professional for instance. in that case, we would need to store 2 tokens in the mobile app. how does the solution we would use here would make it possible or not an upgrade to a multi account solution? Maybe it's just a side point, not sure it's worth diving too much into it.

[gitguardian[bot]]

⚠️ GitGuardian has uncovered 4 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secrets in your pull request

GitGuardian id	GitGuardian status	Secret	Commit	Filename
-		Generic High Entropy Secret	06c91edcb506e8f217732003483289fa0b40a31c	dev/vendor/galoy-quickstart/dev/core-bundle/integration-env.json	View secret
-		Generic High Entropy Secret	06c91edcb506e8f217732003483289fa0b40a31c	dev/vendor/galoy-quickstart/dev/core-bundle/serve-env.json	View secret
-		Generic High Entropy Secret	06c91edcb506e8f217732003483289fa0b40a31c	dev/vendor/galoy-quickstart/dev/Tiltfile	View secret
-		Generic Private Key	06c91edcb506e8f217732003483289fa0b40a31c	dev/vendor/galoy-quickstart/dev/config/notifications/fake_service_account.json	View secret

🛠 Guidelines to remediate hardcoded secrets

1. Understand the implications of revoking this secret by investigating where it is used in your code.
2. Replace and store your secrets safely. Learn here the best practices.
3. Revoke and rotate these secrets.
4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider

• following these best practices for managing and storing secrets including API keys and other credentials
• install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.

---

<sup>🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Our GitHub checks need improvements? Share your feedbacks!</sup>

[UncleSamtoshi]

Changes look good, thanks for making the state more explicit! Looks like check code and tests are still failing and need to be fixed however. You should be able to run these checks locally and get them working.

[sandipndev]

@2624789 what's the state of this PR? Is there any way we can accelerate this?