feat(KeyringController): add `exportEncryptionKey` method to export vault key #5984

matthiasgeihs · 2025-06-14T05:27:52Z

Explanation

Keyring Controller:

Add method controller.exportEncryptionKey to export vault key.
Change method controller.submitEncryptionKey to have an optional salt.
- If the salt is provided, the controller will check that it is consistent with the locally stored salt.
- If the salt is not provided, this check is omitted.
- Before, the salt was mandatory, but it might not be required in the case of unlocking the vault during vault recovery.

This feature is relevant for resolving an audit finding with the seedless onboarding controller.

References

Previously, seedless onboarding was backing up the keyring password to allow for vault recovery after a password change. Now we backup the keyring encryption key.

See the ADR for more details.
This is part of the implementation of option 6.

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

ccharly · 2025-06-20T10:19:37Z

packages/keyring-controller/src/KeyringController.ts

+      // There is a case where the controller is unlocked but the encryption key
+      // is not set, even when #cacheEncryptionKey is true. This happens when
+      // calling changePassword with the existing password. In this case, the
+      // encryption key is deleted, but the state is not recreated, because the
+      // session state does not change in this case, and #updateVault is not
+      // called in #persistOrRollback.
+      if (!this.state.encryptionKey) {
+        return await this.#withVaultLock(async () => {
+          assertIsExportableKeyEncryptor(this.#encryptor);
+          assertIsValidPassword(this.#password);
+          const result = await this.#encryptor.decryptWithDetail(
+            this.#password,
+            // Ignoring undefined. Assuming vault is set when unlocked.
+            this.state.vault as string,
+          );
+          if (this.#cacheEncryptionKey) {
+            this.update((state) => {
+              state.encryptionKey = result.exportedKeyString;
+              state.encryptionSalt = result.salt;
+            });
+          }
+          return result.exportedKeyString;
+        });
+      }


Can't we just make changePassword a noop if we re-use the same password in that case?

Thus, the encryption key won't be re-generated and state would still have a valid encryptionKey too?

Also, another option would be to put the encryption key as part of the session state I guess? 🤔 This way, we would detect it has changed and we would re-trigger updateVault

Any thoughts on that @mikesposito?

I think that making changePassword a noop if the password is the same is a good idea. I prefer it as a solution over putting the encryption key in the session state, as it would keep the password as single source of truth (e.g. is this encryption key that I'm using to encrypt the vault still valid and derived from the current password?).

Actually, another reason for not putting the encryption key in the session state is that it is already part of the committed state, while the rest of the session would be internal instance variables

On that last comment, I am not sure what the relevance is of the encryption key being in the controller state. Why should that be a reason to not include it as session state?

not sure if i'm following all the comments here.

to clarify, this edge case only came up because of testing with changePassword without changing password in the seedless onboarding flow.

after the changePassword without actually changing the password the controller is left in an unlocked state with encryptionKey removed from state.

obviously, the controller must still be able to export the encryption key in this state.

i see the following ways of dealing with this:

we update the changePassword flow to rehydrate the encryption key in all cases. (might be a bit messy because we might have to touch #persistOrRollback which many functions rely on.)

we keep changePassword functionality, but we derive the encryption key within exportEncryptionKey if it is not present. (in this case we can either choose to persist this key in the state (2a), or leave the state untouched (2b).)

we update changePassword to be a no-op in case the password is not changed.

I chose to implement 2, because I didn't want to mess with #persistOrRollback, and 2a) specifically because i thought it is in line with how unlocking works and is more efficient when it comes to multiple calls to exportEncryptionKey. i did not think of option 3 before.

with regards to 3) i see the following edge case where the controller is unlocked using encryption key and #password is not set. in this case changePassword would trigger re-encryption even though the password technically hasn't changed. in any case, it would still be fine here because it would trigger the vault to be updated (because undefined != newPassword) and thus rehydrate the state.encryptionKey as needed.

what do you think? i'm fine with doing 3) if this is the preferred solution.

On that last comment, I am not sure what the relevance is of the encryption key being in the controller state. Why should that be a reason to not include it as session state?

The way #getSessionState() is used is to get the current values of instance variables to decide whether the controller state needs to be updated or not. The variables included in the session are also the ones that can be rolled back in case of an error - so I feel that placing state.encryptionKey along with the session would mix up things a little. Though this issue goes away with #5963 as we are replacing #password with #encryptionKey

Though it is also true that state.encryptionKey has the lifetime of a single session as it is not persisted. So I don't have a super strong opinion, it's just that it would be slightly different compared to how these variables are separated now

changed to no-op in fee5652

mikesposito · 2025-06-20T11:06:26Z

packages/keyring-controller/src/KeyringController.ts

+      // There is a case where the controller is unlocked but the encryption key
+      // is not set, even when #cacheEncryptionKey is true. This happens when
+      // calling changePassword with the existing password. In this case, the
+      // encryption key is deleted, but the state is not recreated, because the
+      // session state does not change in this case, and #updateVault is not
+      // called in #persistOrRollback.
+      if (!this.state.encryptionKey) {
+        return await this.#withVaultLock(async () => {
+          assertIsExportableKeyEncryptor(this.#encryptor);
+          assertIsValidPassword(this.#password);
+          const result = await this.#encryptor.decryptWithDetail(
+            this.#password,
+            // Ignoring undefined. Assuming vault is set when unlocked.
+            this.state.vault as string,
+          );
+          if (this.#cacheEncryptionKey) {
+            this.update((state) => {
+              state.encryptionKey = result.exportedKeyString;
+              state.encryptionSalt = result.salt;
+            });
+          }
+          return result.exportedKeyString;
+        });
+      }


I think that making changePassword a noop if the password is the same is a good idea. I prefer it as a solution over putting the encryption key in the session state, as it would keep the password as single source of truth (e.g. is this encryption key that I'm using to encrypt the vault still valid and derived from the current password?).

mikesposito · 2025-06-20T11:14:54Z

packages/keyring-controller/src/KeyringController.ts

+          const result = await this.#encryptor.decryptWithDetail(
+            this.#password,
+            // Ignoring undefined. Assuming vault is set when unlocked.
+            this.state.vault as string,
+          );


Not sure how often this will be called, but this call takes Key Derivation time + Decryption time, and the Decryption time is all thrown away - which is not ideal. We could use #encryptor.keyFromPassword instead, although we'd need to:

Add keyFromPassword to ExportableKeyEncryptor

Take care of using the same keyMetadata and salt from the vault

I think that adding keyFromPassword and using the salt from the vault is trivial, but dealing with keyMetadata may be trickier or even unwanted (because it heavily depends on the encryptor injected by the client). Perhaps the ideal solution would be to have an additional method on the encryptor interface to derive a key from a password, using salt and keyMetadata from an existing vault (without decrypting it).

This would mean having to add a method in @metamask/browser-passworder and one in the mobile encryptor, and it may require more time to ship this feature. If we are ok with the performance hit for now, we can do it later.

i would expect the decryption time to be negligible. (pure AES is super fast.) hence, i assumed it's not a problem.

one thing i did want here is for that key derivation to function exactly as it would in the #unlockKeyrings method. (to avoid any potential inconsistencies between the derived keys.)

however, this issue would also be solved if changePassword becomes a no-op when password isn't changed. (because then we don't need to do the derivation here.)

(PS: ideally, i'd like to avoid having to touch the encryptor interface)

changed in fee5652

mikesposito · 2025-06-20T11:16:09Z

packages/keyring-controller/src/KeyringController.ts

+          if (this.#cacheEncryptionKey) {
+            this.update((state) => {
+              state.encryptionKey = result.exportedKeyString;
+              state.encryptionSalt = result.salt;
+            });
+          }


Curious: why are we updating this state here if we already used the salt from the vault, and the key is presumably the same?

not sure if i understand the question.

we do this only if state.encryptionKey is undefined.
and in this case i do the update so that we don't have to go through key derivation again if we call exportEncryptionKey multiple times.

changed in fee5652

Gudahtt · 2025-06-20T12:37:58Z

packages/keyring-controller/src/KeyringController.ts

+      // There is a case where the controller is unlocked but the encryption key
+      // is not set, even when #cacheEncryptionKey is true. This happens when
+      // calling changePassword with the existing password. In this case, the
+      // encryption key is deleted, but the state is not recreated, because the


The scenario described in this comment seems like a bug. Perhaps we should fix that bug rather than work around it. Especially given the complexity of this workaround.

discussed in the thread above #5984 (comment)

making changePassword a no-op in case of same passwords would "fix" this

Yes, I think so.

To be specific about why this is a bug: I would expect the encryptionKey state to always represent the key used to encrypt the current vault string that we have. The client in the past did rely on this expectation. In the scenario described, that expectation would not be met.

Making changePassword a no-op when given the same password hopefully should ensure encryptionKey remains present, and still matching the vault.

changed in fee5652

checking for edge case where encryption key is not set after calling `changePassword` with same password

ccharly

LGTM. Would be great to wait for approval from @Gudahtt or @mikesposito also.

Gudahtt

LGTM!

Gudahtt · 2025-06-20T15:41:12Z

packages/keyring-controller/src/KeyringController.ts

   * @returns Promise resolving when the operation completes.
   */
  async submitEncryptionKey(
    encryptionKey: string,
-    encryptionSalt: string,
+    encryptionSalt?: string,


Nit: It would be nice to add a test case to ensure this works if the salt is omitted

matthiasgeihs force-pushed the mg/keyring/export-key branch from ab31883 to 721ff95 Compare June 17, 2025 15:23

matthiasgeihs changed the title ~~Keyring Controller: Add method to export encrypted vault key~~ Keyring Controller: Add method to export vault key Jun 17, 2025

matthiasgeihs force-pushed the mg/keyring/export-key branch from 408fe42 to 97f3959 Compare June 17, 2025 15:38

chaitanyapotti added the team-web3auth label Jun 19, 2025

matthiasgeihs marked this pull request as ready for review June 20, 2025 08:18

matthiasgeihs requested review from a team as code owners June 20, 2025 08:19

matthiasgeihs force-pushed the mg/keyring/export-key branch from 369af94 to f2ef812 Compare June 20, 2025 08:21

ccharly reviewed Jun 20, 2025

View reviewed changes

mikesposito reviewed Jun 20, 2025

View reviewed changes

Gudahtt reviewed Jun 20, 2025

View reviewed changes

matthiasgeihs added 8 commits June 20, 2025 15:33

submitEncryptionKey: make salt optional

089f6d7

add method to export vault encryption key

2135b89

add tests

a8879b5

update CHANGELOG

65ce363

add test cases

2efbe35

checking for edge case where encryption key is not set after calling `changePassword` with same password

fix export key

c533609

add locks

2775713

make changePassword a no-op for same password

9688e86

matthiasgeihs force-pushed the mg/keyring/export-key branch from fee5652 to 9688e86 Compare June 20, 2025 13:33

ccharly changed the title ~~Keyring Controller: Add method to export vault key~~ feat(KeyringController): add exportEncryptionKey method to export vault key Jun 20, 2025

ccharly approved these changes Jun 20, 2025

View reviewed changes

Gudahtt approved these changes Jun 20, 2025

View reviewed changes

Gudahtt reviewed Jun 20, 2025

View reviewed changes

Uh oh!

feat(KeyringController): add exportEncryptionKey method to export vault key #5984

Are you sure you want to change the base?

feat(KeyringController): add exportEncryptionKey method to export vault key #5984

Uh oh!

Conversation

matthiasgeihs commented Jun 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Explanation

References

Changelog

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthiasgeihs Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikesposito Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikesposito Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthiasgeihs Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthiasgeihs Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gudahtt Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ccharly left a comment

Choose a reason for hiding this comment

Uh oh!

Gudahtt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

feat(KeyringController): add `exportEncryptionKey` method to export vault key #5984

feat(KeyringController): add `exportEncryptionKey` method to export vault key #5984

matthiasgeihs commented Jun 14, 2025 •

edited

Loading

matthiasgeihs Jun 20, 2025 •

edited

Loading

mikesposito Jun 20, 2025 •

edited

Loading

mikesposito Jun 20, 2025 •

edited

Loading

matthiasgeihs Jun 20, 2025 •

edited

Loading

matthiasgeihs Jun 20, 2025 •

edited

Loading

Gudahtt Jun 20, 2025 •

edited

Loading