feat: persist UserStorage e2e content keys using an encrypted keyStore #5129

mirceanis · 2025-01-10T13:46:15Z

⚠️ Tentative PR for early feedback gathering

Explanation

The UserStorageController e2e encryption keys are derived from a storageKey that is specific to the user profile. The key derivation function used is scrypt, with parameters recommended for password inputs. This means that it's a very costly operation (on the order of seconds on a 2024 mobile device).
These derived keys are cached in memory for the lifetime of the controller instance, but a better approach would be to use a Key Store, to persist the derived keys in a safe manner. This would avoid the rerun of the costly key derivation operation on every app restart.

This set of changes also comes at a time when we're preparing for a multi-device/multi-srp user profile, so the implementation of the KeyStore relies on the new encryption capabilities of the message-signing-snap. The snap is already used for auth and is a key component of the UserStorageController, but we are introducing some defaults here that create even more coupling between the controller and the snap.
The proposed KeyStore implementation relies on the persistable state properties and the messagingSystem available to MM controllers, so externalizing this would create some overhead, but feedback is very welcome about the approach.

References

fixes #5128

Changelog

`@metamask/profile-sync-controller`

: KeyStore persistence for e2ee keys

Checklist

I've updated the test suite for new or updated code as appropriate
I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
I've highlighted breaking changes using the "BREAKING" category above as appropriate
I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

socket-security · 2025-01-10T13:47:50Z

New dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/@noble/[email protected]	None	`0`	859 kB	paulmillr

View full report↗︎

fixes #5128

Prithpal-Sooriya · 2025-01-15T10:16:46Z

packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts

+    storeWrappedKey: (keyRef: string, wrappedKey: string): Promise<void> => {
+      return new Promise((resolve) => {
+        this.update((state) => {
+          state.encryptedContentKeys[keyRef] = wrappedKey;
+          resolve();
+        });
+      });
+    },


Do these need to be promises?

If we do want to make then pseudo promises, we can make the functions async and avoid return explicit promises.

// By using the `async` function syntax, the return type will be inferred as a promise. storeWrappedKey: async (...): Promise<void> => { this.update(...) }

packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts

Prithpal-Sooriya · 2025-01-15T10:19:47Z

packages/profile-sync-controller/src/shared/encryption/encryption.ts

@@ -54,12 +56,14 @@ class EncryptorDecryptor {
    plaintext: string,
    password: string,
    nativeScryptCrypto?: NativeScrypt,
+    keyStore?: KeyStore,


long term (far future) - I don't really like how many layers we have to drill in this context (both for NativeScrypt, and for this).

We can think of some diff approach later.

Coming back to this, lets see if it is worth exposing an interface to persist keys.
But maybe persisting will not be necessary in this approach that uses ECIES and does not use an expensive KDF.

Prithpal-Sooriya · 2025-01-15T10:29:43Z

packages/profile-sync-controller/src/shared/encryption/encryption.ts

+    const keyRef = `${hashedPassword}${bytesToHex(newSalt)}`;
+    if (keyStore) {
+      try {
+        const storedKey = await keyStore.loadKey(keyRef);


Oooh very cool, so this keyStore will be used to store the encrypted keys to be used by other potential platforms?

As we persist this on the extension, would we need a way for this to be accessible outside of the extension? Reason I'm asking is could we place this keystore into the snap storage? That way sites can access this from the snap rather than the extension?

Prithpal-Sooriya · 2025-01-15T10:32:10Z

packages/profile-sync-controller/src/shared/encryption/key-storage.ts

+          nonce: bytesToBase64(nonce),
+          ephemPublicKey: bytesToBase64(ephemeralPublic),
+          ciphertext: bytesToBase64(encryptedMessage),
+        } as Eip1024EncryptedData;


What fields are missing for this to be be type asserted?

@mathieuartu

…ing the app (#13803) ## **Description** There is serious performance impact of loading notifications on startup. It is roughly a 2 second lag, and the app freezes during this process. This is due to us processing the encryption key to be used for decrypting entries in UserStorage. This persists the a cached encryption key using existing storage mechanisms to securely store this hashed key. **NOTE:** This fix is an interim solution until we add e2e content keys for encryption (CC @mathieuartu @mirceanis) MetaMask/core#5129 ## **Related issues** Fixes: ## **Manual testing steps** 1. Enable Notifications 2. Close the app 3. Open the app and unlock the wallet EXPECTED - you should not see any lag or app freezing. ## **Screenshots/Recordings**  ### **Before** https://www.loom.com/share/8951a714b9b34f79a3e988ed50f9f656?sid=ad2ff732-c893-49d6-bd4b-e7f13a3a08b5  ### **After** https://www.loom.com/share/fe0afadcd49b46bbbafe219f95777fb6?sid=ca0df6aa-b5c5-4ffd-a5ee-bc811f0f1058  ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@mathieuartu

… opening the app (#13803) ## **Description** There is serious performance impact of loading notifications on startup. It is roughly a 2 second lag, and the app freezes during this process. This is due to us processing the encryption key to be used for decrypting entries in UserStorage. This persists the a cached encryption key using existing storage mechanisms to securely store this hashed key. **NOTE:** This fix is an interim solution until we add e2e content keys for encryption (CC @mathieuartu @mirceanis) MetaMask/core#5129 ## **Related issues** Fixes: ## **Manual testing steps** 1. Enable Notifications 2. Close the app 3. Open the app and unlock the wallet EXPECTED - you should not see any lag or app freezing. ## **Screenshots/Recordings**  ### **Before** https://www.loom.com/share/8951a714b9b34f79a3e988ed50f9f656?sid=ad2ff732-c893-49d6-bd4b-e7f13a3a08b5  ### **After** https://www.loom.com/share/fe0afadcd49b46bbbafe219f95777fb6?sid=ca0df6aa-b5c5-4ffd-a5ee-bc811f0f1058  ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@mathieuartu

… opening the app (#13803) ## **Description** There is serious performance impact of loading notifications on startup. It is roughly a 2 second lag, and the app freezes during this process. This is due to us processing the encryption key to be used for decrypting entries in UserStorage. This persists the a cached encryption key using existing storage mechanisms to securely store this hashed key. **NOTE:** This fix is an interim solution until we add e2e content keys for encryption (CC @mathieuartu @mirceanis) MetaMask/core#5129 ## **Related issues** Fixes: ## **Manual testing steps** 1. Enable Notifications 2. Close the app 3. Open the app and unlock the wallet EXPECTED - you should not see any lag or app freezing. ## **Screenshots/Recordings**  ### **Before** https://www.loom.com/share/8951a714b9b34f79a3e988ed50f9f656?sid=ad2ff732-c893-49d6-bd4b-e7f13a3a08b5  ### **After** https://www.loom.com/share/fe0afadcd49b46bbbafe219f95777fb6?sid=ca0df6aa-b5c5-4ffd-a5ee-bc811f0f1058  ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@mathieuartu

…fications loading when opening the app (#13803) ## **Description** There is serious performance impact of loading notifications on startup. It is roughly a 2 second lag, and the app freezes during this process. This is due to us processing the encryption key to be used for decrypting entries in UserStorage. This persists the a cached encryption key using existing storage mechanisms to securely store this hashed key. **NOTE:** This fix is an interim solution until we add e2e content keys for encryption (CC @mathieuartu @mirceanis) MetaMask/core#5129 ## **Related issues** Fixes: ## **Manual testing steps** 1. Enable Notifications 2. Close the app 3. Open the app and unlock the wallet EXPECTED - you should not see any lag or app freezing. ## **Screenshots/Recordings**  ### **Before** https://www.loom.com/share/8951a714b9b34f79a3e988ed50f9f656?sid=ad2ff732-c893-49d6-bd4b-e7f13a3a08b5  ### **After** https://www.loom.com/share/fe0afadcd49b46bbbafe219f95777fb6?sid=ca0df6aa-b5c5-4ffd-a5ee-bc811f0f1058  ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@mathieuartu

…fications loading when opening the app (#13811) - fix: cp-7.42.0 improve performance of notifications loading when opening the app (#13803) ## **Description** There is serious performance impact of loading notifications on startup. It is roughly a 2 second lag, and the app freezes during this process. This is due to us processing the encryption key to be used for decrypting entries in UserStorage. This persists the a cached encryption key using existing storage mechanisms to securely store this hashed key. **NOTE:** This fix is an interim solution until we add e2e content keys for encryption (CC @mathieuartu @mirceanis) MetaMask/core#5129 ## **Related issues** Fixes: ## **Manual testing steps** 1. Enable Notifications 2. Close the app 3. Open the app and unlock the wallet EXPECTED - you should not see any lag or app freezing. ## **Screenshots/Recordings**  ### **Before** https://www.loom.com/share/8951a714b9b34f79a3e988ed50f9f656?sid=ad2ff732-c893-49d6-bd4b-e7f13a3a08b5  ### **After** https://www.loom.com/share/fe0afadcd49b46bbbafe219f95777fb6?sid=ca0df6aa-b5c5-4ffd-a5ee-bc811f0f1058  ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. [18f84aa](18f84aa) Co-authored-by: Prithpal Sooriya <[email protected]>

@mathieuartu

…ing the app (#13803) There is serious performance impact of loading notifications on startup. It is roughly a 2 second lag, and the app freezes during this process. This is due to us processing the encryption key to be used for decrypting entries in UserStorage. This persists the a cached encryption key using existing storage mechanisms to securely store this hashed key. **NOTE:** This fix is an interim solution until we add e2e content keys for encryption (CC @mathieuartu @mirceanis) MetaMask/core#5129 Fixes: 1. Enable Notifications 2. Close the app 3. Open the app and unlock the wallet EXPECTED - you should not see any lag or app freezing.  https://www.loom.com/share/8951a714b9b34f79a3e988ed50f9f656?sid=ad2ff732-c893-49d6-bd4b-e7f13a3a08b5  https://www.loom.com/share/fe0afadcd49b46bbbafe219f95777fb6?sid=ca0df6aa-b5c5-4ffd-a5ee-bc811f0f1058  - [x] I’ve followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. Modify better mocks Adjust jsdoc Adjust codeowner file Fix initModularizedControllers unit tests Remove log Add TransactionControllerInit function Add missing test cases Add missing event to NFT Controller Use template for controllerMessenger in test utils Use buildControllerInitRequestMock in notification controller unit tests Use callbacks as standadolone functions in transaction-controller-init Cleanup unnecessary events from transaction-controller-messenger Add back ControllerMessengerByControllerName Rename getCurrentChainId to getGlobalChainId Rename getUIState Alphabetical order of request Revert back deleted test Fix tests of utils Remove unused import Revert lint changes

mirceanis added the team-identity Identity Team changes. https://github.com/orgs/MetaMask/teams/identity label Jan 10, 2025

feat: persist UserStorage e2e content keys using an encrypted keyStore

62a9880

fixes #5128

mirceanis force-pushed the 5128-persist-user-storage-e2e-keys branch from be2f7ca to 62a9880 Compare January 10, 2025 13:58

Prithpal-Sooriya reviewed Jan 15, 2025

View reviewed changes

packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts Show resolved Hide resolved

Prithpal-Sooriya reviewed Jan 15, 2025

View reviewed changes

Prithpal-Sooriya mentioned this pull request Mar 3, 2025

fix: cp-7.42.0 improve performance of notifications loading when opening the app MetaMask/metamask-mobile#13803

Merged

7 tasks

runway-github bot mentioned this pull request Mar 3, 2025

chore(runway): cherry-pick fix: cp-7.42.0 improve performance of notifications loading when opening the app MetaMask/metamask-mobile#13811

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

feat: persist UserStorage e2e content keys using an encrypted keyStore #5129

feat: persist UserStorage e2e content keys using an encrypted keyStore #5129

Uh oh!

mirceanis commented Jan 10, 2025 •

edited

Loading

Uh oh!

socket-security bot commented Jan 10, 2025 •

edited

Loading

Uh oh!

Prithpal-Sooriya Jan 15, 2025

Uh oh!

Uh oh!

Prithpal-Sooriya Jan 15, 2025

Uh oh!

Prithpal-Sooriya Mar 4, 2025

Uh oh!

Prithpal-Sooriya Jan 15, 2025

Uh oh!

Prithpal-Sooriya Jan 15, 2025

Uh oh!

Uh oh!

Uh oh!

feat: persist UserStorage e2e content keys using an encrypted keyStore #5129

Are you sure you want to change the base?

feat: persist UserStorage e2e content keys using an encrypted keyStore #5129

Uh oh!

Conversation

mirceanis commented Jan 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ Tentative PR for early feedback gathering

Explanation

References

Changelog

@metamask/profile-sync-controller

Checklist

Uh oh!

socket-security bot commented Jan 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Prithpal-Sooriya Jan 15, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Prithpal-Sooriya Jan 15, 2025

Choose a reason for hiding this comment

Uh oh!

Prithpal-Sooriya Mar 4, 2025

Choose a reason for hiding this comment

Uh oh!

Prithpal-Sooriya Jan 15, 2025

Choose a reason for hiding this comment

Uh oh!

Prithpal-Sooriya Jan 15, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mirceanis commented Jan 10, 2025 •

edited

Loading

`@metamask/profile-sync-controller`

socket-security bot commented Jan 10, 2025 •

edited

Loading