Persist profile storage keys to e2ee backend #5630

mirceanis · 2025-04-11T12:37:44Z

Don't merge until @metamask/[email protected]+ is available in the clients.

see MetaMask/metamask-extension#31943
and MetaMask/metamask-mobile#14653

Explanation

The backup and sync UserStorageController internally uses some storageKeys for end to end encryption (and collision avoidance). For now, these storage keys are derived using data specific to the SRP and profile ID but this blocks any progress on pairing multiple SRPs.

This PR introduces a new way to manage these profile storage keys, by leveraging the new deterministic encryption capabilities in the @metamask/message-signing-snap.
The profile storage keys, once derived, are encrypted for the public key provided by the snap and stored in the user-storage backend under a deterministic, SRP specific, key.

The user storage component will attempt to download and decrypt the profile storage key and only derive it if it cannot find it.

This is not a complete replacement of the way storage keys are managed, as we need to remain compatible with previous deployments, but at least it starts storing the encrypted keys in the right box.

This work is a prerequisite to proper pairing of profiles in the milti-SRP universe, where we need to allow different SRPs to supplement each-other's profile-data so that the user maintains a coherent experience regardless of how or where they import their different SRPs.

Dependencies

@metamask/eth-sig-util
I introduced this "new" dependency to perform the encryption.
I figured it was safe to use it since this is already a core dependency in several other controllers and is also used in the clients.
Ideally, the ERC1024 functionality that is imported from that library would be isolated in a smaller library and reused in the message-signing-snap as well. (the snap uses a different set of smaller libraries to implement the same functionality).
This new dependency may affect the bundle size of the @metamask/profile-sync-controller package, but it should continue to be tree-shakeable.
@metamask/[email protected]
While not a direct dependency of this library, the controllers do rely on this preinstalled snap to perform signing and decryption. The decryption functionality was implemented in v1.1.0 of the snap, so that version needs to exist in the clients before a new version of the @metamask/profile-sync-controller is released and used.

References

Related to IDENTITY-7
BLOCKED by: MetaMask/metamask-extension#31943
BLOCKED by: MetaMask/metamask-mobile#14653

Changelog

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 communicated my changes to consumers by updating changelogs for packages I've changed, highlighting breaking changes as necessary
I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

mathieuartu

Review in progress :) We should add a new entry to the changelog for this PR also.

## **Description** This is a version bump for the preinstalled @metamask/message-signing-snap to v1.1.1 This latest version has a number of improvements including support for multiple entropy sources (following SIP-30) and encryption, as well as results differentiated by origin. This snap is used by the AuthenticationController / UserStorageController and the latest version is necessary to make progress in the controllers. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/31943?quickstart=1) ## **Related issues** relates to: [IDENTITY-7](https://consensyssoftware.atlassian.net/browse/IDENTITY-7) required by: MetaMask/core#5630 relates to: MetaMask/metamask-mobile#14653 ## **Manual testing steps** No user-facing changes ## **Screenshots/Recordings**  ### **Before**  ### **After**  ## **Pre-merge author checklist** - [X] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [X] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/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. [IDENTITY-7]: https://consensyssoftware.atlassian.net/browse/IDENTITY-7?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ Signed-off-by: Mircea Nistor <[email protected]>

mathieuartu

Looks amazing, I know it's WIP so I've added a few comments and will wait for de-draft to approve

mathieuartu · 2025-04-15T06:53:27Z

packages/profile-sync-controller/src/shared/storage-schema.ts

@@ -144,7 +148,7 @@ export function createEntryPath<T extends boolean>(
  options: {
    validateAgainstSchema: T;
  } = { validateAgainstSchema: true as T },
-): string {
+): `${string}/${string}` {


easier inference downstream

mathieuartu · 2025-04-15T08:22:43Z

packages/profile-sync-controller/src/sdk/user-storage.ts

@@ -426,11 +513,11 @@ export class UserStorage {
    try {
      const headers = await this.#getAuthorizationHeader();
      const storageKey = await this.getStorageKey();
-      const encryptedPath = createEntryPath(path, storageKey, {
+      const entryPath = createEntryPath(path, storageKey, {


Nice renaming this here also

mathieuartu · 2025-04-15T08:24:25Z

packages/profile-sync-controller/src/sdk/user-storage.ts

+      profileStorageKeyPath = createEntryPath(
+        `${USER_STORAGE_FEATURE_NAMES.keys}.${PROFILE_STORAGE_KEY}`,
+        srpKey,
+        { validateAgainstSchema: false },
+      );


I feel this could be moved L165 as a const assignment, and we could remove the if (profileStorageKeyPath) L193

Oh well, we could save some compute time & memory by doing this here only if (srpKey). Felt it was a tad complicated when reading, but it makes sense in this regard. Feel free to dismiss the above

I did it like this for the case where the deterministic encryption layer is missing.
Then this all short circuits to the behavior we had before

mathieuartu · 2025-04-15T10:20:09Z

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

@@ -11,6 +11,7 @@ import type {
  StateMetadata,
 } from '@metamask/base-controller';
 import { BaseController } from '@metamask/base-controller';
+import { encrypt } from '@metamask/eth-sig-util';


Nit: WDYT we maybe alias encrypt so it's not confusing where it's coming from when reading L389 (@metamask/eth-sig-util or shared/encryption/encryption.ts).

I wouldn't know what to use though 😛

good point!
I'll rename it

mathieuartu · 2025-04-15T10:22:04Z

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

+  async #snapGetEncryptionPublicKey(): Promise<string> {
+    if (this.#_snapEncryptionKeyCache) {
+      return this.#_snapEncryptionKeyCache;
+    }
+
+    if (!this.#isUnlocked) {
+      throw new Error(
+        '#snapGetEncryptionPublicKey - unable to call snap, wallet is locked',
+      );
+    }
+
+    const result = (await this.messagingSystem.call(
+      'SnapController:handleRequest',
+      createSnapEncryptionPublicKeyRequest(),
+    )) as string;
+
+    this.#_snapEncryptionKeyCache = result;
+
+    return result;
+  }
+
+  #_snapDecryptMessageCache: Record<string, string> = {};
+
+  /**
+   * Calls the snap to attempt to decrypt the message.
+   *
+   * @param ciphertext - the encrypted text to decrypt.
+   * @returns The decrypted message, if decryption was possible.
+   */
+  async #snapDecryptMessage(ciphertext: string): Promise<string> {
+    if (this.#_snapDecryptMessageCache[ciphertext]) {
+      return this.#_snapDecryptMessageCache[ciphertext];
+    }
+
+    if (!this.#isUnlocked) {
+      throw new Error(
+        '#snapDecryptMessage - unable to call snap, wallet is locked',
+      );
+    }
+
+    const result = (await this.messagingSystem.call(
+      'SnapController:handleRequest',
+      createSnapDecryptMessageRequest(ciphertext),
+    )) as string;
+
+    if (result) {
+      this.#_snapDecryptMessageCache[ciphertext] = result;
+    }
+
+    return result;
+  }
+


Nit: I feel there's a lot of overlap between those two methods and #snapSignMessage, should we try to refactor and have one snap interaction method that handles designated caches and params?

I suppose it could provide some marginal readability benefits, but at the cost of increasing the complexity of this PR.
For now, we have 4 methods using this pattern, all in one file, all rather small, so I don't think it's a very big deal.
Let's do this in a separate issue/PR, WDYT?

mathieuartu · 2025-04-15T10:22:53Z

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

@@ -1,5 +1,6 @@
 import type { NotNamespacedBy } from '@metamask/base-controller';
 import { Messenger } from '@metamask/base-controller';
+import { decrypt, type EthEncryptedData } from '@metamask/eth-sig-util';


Nit: Same comment as for UserStorageController.ts encrypt, should we alias this so its clear its coming from a library?

This initial approach uses SRP/identifier specific storage keys as an intermediate layer for storing e2ee profile specific keys

…ofile storage keys

… others

## **Description** This is a version bump for the preinstalled @metamask/message-signing-snap to v1.1.1 This latest version has a number of improvements including support for multiple entropy sources (following SIP-30) and encryption, as well as results differentiated by origin. This snap is used by the AuthenticationController / UserStorageController and the latest version is necessary to make progress in the controllers. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/31943?quickstart=1) ## **Related issues** relates to: [IDENTITY-7](https://consensyssoftware.atlassian.net/browse/IDENTITY-7) required by: MetaMask/core#5630 relates to: MetaMask/metamask-mobile#14653 ## **Manual testing steps** No user-facing changes ## **Screenshots/Recordings**  ### **Before**  ### **After**  ## **Pre-merge author checklist** - [X] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [X] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/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. [IDENTITY-7]: https://consensyssoftware.atlassian.net/browse/IDENTITY-7?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ Signed-off-by: Mircea Nistor <[email protected]>

## **Description** This is a version bump for the preinstalled @metamask/message-signing-snap to v1.1.1 This latest version has a number of improvements including support for multiple entropy sources (following SIP-30) and encryption, as well as results differentiated by origin. This snap is used by the AuthenticationController / UserStorageController and the latest version is necessary to make progress in the controllers. ## **Related issues** relates to: [IDENTITY-7](https://consensyssoftware.atlassian.net/browse/IDENTITY-7) required by: MetaMask/core#5630 relates to: MetaMask/metamask-extension#31943 ## **Manual testing steps** There are no user-facing changes to test ## **Screenshots/Recordings**  ### **Before**  ### **After**  ## **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 - [ ] I’ve included tests if applicable - [ ] 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. [IDENTITY-7]: https://consensyssoftware.atlassian.net/browse/IDENTITY-7?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ

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

mirceanis force-pushed the persist-profile-storage-keys-to-e2ee-backend branch 2 times, most recently from f328904 to d492e67 Compare April 14, 2025 11:58

mirceanis added the no-changelog label Apr 14, 2025

This was referenced Apr 14, 2025

chore(deps): bump @metamask/message-signing-snap to v1.1.1 MetaMask/metamask-mobile#14653

Merged

chore(deps): bump message-signing-snap to 1.1.1 MetaMask/metamask-extension#31943

Merged

mathieuartu reviewed Apr 15, 2025

View reviewed changes

mirceanis added 5 commits April 15, 2025 13:58

feat: persist profile-storage-keys in identifier-storage-space

64bcd59

This initial approach uses SRP/identifier specific storage keys as an intermediate layer for storing e2ee profile specific keys

feat: use deterministic asymmetric encryption to persist encrypted pr…

f8047c3

…ofile storage keys

test: profile storage key retrieval and decryption

e0351bb

style: fix linter warnings

31ab6f0

style: alias ERC1024 encrypt/decrypt imports to distinguish them from…

a6f7ce3

… others

mirceanis force-pushed the persist-profile-storage-keys-to-e2ee-backend branch from d492e67 to a6f7ce3 Compare April 15, 2025 11:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Persist profile storage keys to e2ee backend #5630

Persist profile storage keys to e2ee backend #5630

mirceanis commented Apr 11, 2025 •

edited

Loading

mathieuartu left a comment

mathieuartu left a comment

mathieuartu Apr 15, 2025

mirceanis Apr 15, 2025

mathieuartu Apr 15, 2025

mathieuartu Apr 15, 2025

mathieuartu Apr 15, 2025

mirceanis Apr 15, 2025 •

edited

Loading

mathieuartu Apr 15, 2025

mirceanis Apr 15, 2025

mathieuartu Apr 15, 2025

mirceanis Apr 15, 2025

mathieuartu Apr 15, 2025

Persist profile storage keys to e2ee backend #5630

Are you sure you want to change the base?

Persist profile storage keys to e2ee backend #5630

Conversation

mirceanis commented Apr 11, 2025 • edited Loading

Don't merge until @metamask/[email protected]+ is available in the clients.

Explanation

Dependencies

References

Changelog

Checklist

mathieuartu left a comment

Choose a reason for hiding this comment

mathieuartu 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mirceanis Apr 15, 2025 • edited Loading

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

mirceanis commented Apr 11, 2025 •

edited

Loading

mirceanis Apr 15, 2025 •

edited

Loading