From d0bd80ea83014f6fd779b89cb87748db218af371 Mon Sep 17 00:00:00 2001 From: Matthias Geihs Date: Sat, 14 Jun 2025 07:21:42 +0200 Subject: [PATCH 1/8] submitEncryptionKey: make salt optional --- .../src/KeyringController.ts | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 65b3954f873..d8cc54cebfe 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -1434,16 +1434,17 @@ export class KeyringController extends BaseController< } /** - * Attempts to decrypt the current vault and load its keyrings, - * using the given encryption key and salt. + * Attempts to decrypt the current vault and load its keyrings, using the + * given encryption key and salt. The optional salt can be used to check for + * consistency with the vault salt. * * @param encryptionKey - Key to unlock the keychain. - * @param encryptionSalt - Salt to unlock the keychain. + * @param encryptionSalt - Optional salt to unlock the keychain. * @returns Promise resolving when the operation completes. */ async submitEncryptionKey( encryptionKey: string, - encryptionSalt: string, + encryptionSalt?: string, ): Promise { const { newMetadata } = await this.#withRollback(async () => { const result = await this.#unlockKeyrings( @@ -2279,8 +2280,10 @@ export class KeyringController extends BaseController< } else { const parsedEncryptedVault = JSON.parse(encryptedVault); - if (encryptionSalt !== parsedEncryptedVault.salt) { + if (encryptionSalt && encryptionSalt !== parsedEncryptedVault.salt) { throw new Error(KeyringControllerError.ExpiredCredentials); + } else { + encryptionSalt = parsedEncryptedVault.salt as string; } if (typeof encryptionKey !== 'string') { @@ -2296,10 +2299,7 @@ export class KeyringController extends BaseController< // This call is required on the first call because encryptionKey // is not yet inside the memStore updatedState.encryptionKey = encryptionKey; - // we can safely assume that encryptionSalt is defined here - // because we compare it with the salt from the vault - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - updatedState.encryptionSalt = encryptionSalt!; + updatedState.encryptionSalt = encryptionSalt; } } else { if (typeof password !== 'string') { From 74792ffd3655677a36188106f448bbbe0e3e2b81 Mon Sep 17 00:00:00 2001 From: Matthias Geihs Date: Sat, 14 Jun 2025 07:23:32 +0200 Subject: [PATCH 2/8] add method to export vault encryption key --- .../src/KeyringController.ts | 24 +++++++++++++++++++ packages/keyring-controller/src/constants.ts | 1 + 2 files changed, 25 insertions(+) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index d8cc54cebfe..b34e3ecdbbf 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -527,6 +527,20 @@ function assertIsExportableKeyEncryptor( } } +/** + * Assert that the encryption key is set. + * + * @param encryptionKey - The encryption key to check. + * @throws If the encryption key is not set. + */ +function assertIsEncryptionKeySet( + encryptionKey: string | undefined, +): asserts encryptionKey is string { + if (!encryptionKey) { + throw new Error(KeyringControllerError.EncryptionKeyNotSet); + } +} + /** * Assert that the provided password is a valid non-empty string. * @@ -1471,6 +1485,16 @@ export class KeyringController extends BaseController< } } + /** + * Exports the vault encryption key. + * + * @returns The vault encryption key. + */ + async exportEncryptionKey(): Promise { + assertIsEncryptionKeySet(this.state.encryptionKey); + return this.state.encryptionKey; + } + /** * Attempts to decrypt the current vault and load its keyrings, * using the given password. diff --git a/packages/keyring-controller/src/constants.ts b/packages/keyring-controller/src/constants.ts index d914a3d6f74..0fa2c22a05d 100644 --- a/packages/keyring-controller/src/constants.ts +++ b/packages/keyring-controller/src/constants.ts @@ -4,6 +4,7 @@ export enum KeyringControllerError { UnsafeDirectKeyringAccess = 'KeyringController - Returning keyring instances is unsafe', WrongPasswordType = 'KeyringController - Password must be of type string.', InvalidEmptyPassword = 'KeyringController - Password cannot be empty.', + EncryptionKeyNotSet = 'KeyringController - Encryption key not set.', NoFirstAccount = 'KeyringController - First Account not found.', DuplicatedAccount = 'KeyringController - The account you are trying to import is a duplicate', VaultError = 'KeyringController - Cannot unlock without a previous vault.', From c3f8f607cec34e1248e3a49c668f782169028049 Mon Sep 17 00:00:00 2001 From: Matthias Geihs Date: Sun, 15 Jun 2025 11:41:51 +0200 Subject: [PATCH 3/8] add tests --- .../src/KeyringController.test.ts | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index 1ebaf55af28..b0f35662a20 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -3303,6 +3303,36 @@ describe('KeyringController', () => { }); }); + describe('exportEncryptionKey', () => { + it('should export encryption key and unlock', async () => { + await withController( + { cacheEncryptionKey: true }, + async ({ controller }) => { + const encryptionKey = await controller.exportEncryptionKey(); + expect(encryptionKey).toBeDefined(); + + await controller.setLocked(); + + await controller.submitEncryptionKey(encryptionKey); + + expect(controller.isUnlocked()).toBe(true); + }, + ); + }); + + it('should throw error if controller is locked', async () => { + await withController( + { cacheEncryptionKey: true }, + async ({ controller }) => { + await controller.setLocked(); + await expect(controller.exportEncryptionKey()).rejects.toThrow( + KeyringControllerError.EncryptionKeyNotSet, + ); + }, + ); + }); + }); + describe('verifySeedPhrase', () => { it('should return current seedphrase', async () => { await withController(async ({ controller }) => { From 21b380ce54d61112624613ca8d0a141699f88631 Mon Sep 17 00:00:00 2001 From: Matthias Geihs Date: Tue, 17 Jun 2025 17:33:10 +0200 Subject: [PATCH 4/8] update CHANGELOG --- packages/keyring-controller/CHANGELOG.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/packages/keyring-controller/CHANGELOG.md b/packages/keyring-controller/CHANGELOG.md index 38f4b66ca6b..016569c6eb3 100644 --- a/packages/keyring-controller/CHANGELOG.md +++ b/packages/keyring-controller/CHANGELOG.md @@ -7,6 +7,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Add method `exportEncryptionKey` ([#5984](https://github.com/MetaMask/core/pull/5984)) + +### Changed + +- Make salt optional with method `submitEncryptionKey` ([#5984](https://github.com/MetaMask/core/pull/5984)) + ## [22.0.2] ### Fixed From 5baf4d2fb1e001daa7a5b728bf3b24aeaca120b6 Mon Sep 17 00:00:00 2001 From: Matthias Geihs Date: Thu, 19 Jun 2025 12:25:06 +0200 Subject: [PATCH 5/8] add test cases checking for edge case where encryption key is not set after calling `changePassword` with same password --- .../src/KeyringController.test.ts | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index b0f35662a20..5c0accd54e3 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -3331,6 +3331,28 @@ describe('KeyringController', () => { }, ); }); + + it('should export key after password change', async () => { + await withController( + { cacheEncryptionKey: true }, + async ({ controller }) => { + await controller.changePassword('new password'); + const encryptionKey = await controller.exportEncryptionKey(); + expect(encryptionKey).toBeDefined(); + }, + ); + }); + + it('should export key after password change to the same password', async () => { + await withController( + { cacheEncryptionKey: true }, + async ({ controller }) => { + await controller.changePassword(password); + const encryptionKey = await controller.exportEncryptionKey(); + expect(encryptionKey).toBeDefined(); + }, + ); + }); }); describe('verifySeedPhrase', () => { From f1fb287128899a7991f207e5f28fd513a3bc5cf9 Mon Sep 17 00:00:00 2001 From: Matthias Geihs Date: Thu, 19 Jun 2025 11:59:03 +0200 Subject: [PATCH 6/8] fix export key --- .../src/KeyringController.test.ts | 2 +- .../src/KeyringController.ts | 39 ++++++++++++------- packages/keyring-controller/src/constants.ts | 1 - 3 files changed, 25 insertions(+), 17 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index 5c0accd54e3..2ea97c479ae 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -3326,7 +3326,7 @@ describe('KeyringController', () => { async ({ controller }) => { await controller.setLocked(); await expect(controller.exportEncryptionKey()).rejects.toThrow( - KeyringControllerError.EncryptionKeyNotSet, + KeyringControllerError.ControllerLocked, ); }, ); diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index b34e3ecdbbf..cda55232a84 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -527,20 +527,6 @@ function assertIsExportableKeyEncryptor( } } -/** - * Assert that the encryption key is set. - * - * @param encryptionKey - The encryption key to check. - * @throws If the encryption key is not set. - */ -function assertIsEncryptionKeySet( - encryptionKey: string | undefined, -): asserts encryptionKey is string { - if (!encryptionKey) { - throw new Error(KeyringControllerError.EncryptionKeyNotSet); - } -} - /** * Assert that the provided password is a valid non-empty string. * @@ -1491,7 +1477,30 @@ export class KeyringController extends BaseController< * @returns The vault encryption key. */ async exportEncryptionKey(): Promise { - assertIsEncryptionKeySet(this.state.encryptionKey); + this.#assertIsUnlocked(); + // 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) { + 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; + } + return this.state.encryptionKey; } diff --git a/packages/keyring-controller/src/constants.ts b/packages/keyring-controller/src/constants.ts index 0fa2c22a05d..d914a3d6f74 100644 --- a/packages/keyring-controller/src/constants.ts +++ b/packages/keyring-controller/src/constants.ts @@ -4,7 +4,6 @@ export enum KeyringControllerError { UnsafeDirectKeyringAccess = 'KeyringController - Returning keyring instances is unsafe', WrongPasswordType = 'KeyringController - Password must be of type string.', InvalidEmptyPassword = 'KeyringController - Password cannot be empty.', - EncryptionKeyNotSet = 'KeyringController - Encryption key not set.', NoFirstAccount = 'KeyringController - First Account not found.', DuplicatedAccount = 'KeyringController - The account you are trying to import is a duplicate', VaultError = 'KeyringController - Cannot unlock without a previous vault.', From e991903c20ae0a45f81a28856995dfeb471ac550 Mon Sep 17 00:00:00 2001 From: Matthias Geihs Date: Fri, 20 Jun 2025 10:16:00 +0200 Subject: [PATCH 7/8] add locks --- .../src/KeyringController.ts | 47 ++++++++++--------- 1 file changed, 26 insertions(+), 21 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index cda55232a84..54afa0be253 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -1478,30 +1478,35 @@ export class KeyringController extends BaseController< */ async exportEncryptionKey(): Promise { this.#assertIsUnlocked(); - // 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) { - 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 await this.#withControllerLock(async () => { + // 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; }); } - return result.exportedKeyString; - } - return this.state.encryptionKey; + return this.state.encryptionKey; + }); } /** From 3b7ffdd48b96e7a1411d30e92b09b980072018c1 Mon Sep 17 00:00:00 2001 From: Matthias Geihs Date: Fri, 20 Jun 2025 15:30:35 +0200 Subject: [PATCH 8/8] make changePassword a no-op for same password --- .../src/KeyringController.test.ts | 8 ++++ .../src/KeyringController.ts | 47 +++++++++---------- packages/keyring-controller/src/constants.ts | 1 + 3 files changed, 31 insertions(+), 25 deletions(-) diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index 2ea97c479ae..05e7542899e 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -3332,6 +3332,14 @@ describe('KeyringController', () => { ); }); + it('should throw error if encryptionKey is not set', async () => { + await withController(async ({ controller }) => { + await expect(controller.exportEncryptionKey()).rejects.toThrow( + KeyringControllerError.EncryptionKeyNotSet, + ); + }); + }); + it('should export key after password change', async () => { await withController( { cacheEncryptionKey: true }, diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 54afa0be253..7062ced7e9d 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -543,6 +543,20 @@ function assertIsValidPassword(password: unknown): asserts password is string { } } +/** + * Assert that the provided encryption key is a valid non-empty string. + * + * @param encryptionKey - The encryption key to check. + * @throws If the encryption key is not a valid string. + */ +function assertIsEncryptionKeySet( + encryptionKey: string | undefined, +): asserts encryptionKey is string { + if (!encryptionKey) { + throw new Error(KeyringControllerError.EncryptionKeyNotSet); + } +} + /** * Checks if the provided value is a serialized keyrings array. * @@ -1417,6 +1431,11 @@ export class KeyringController extends BaseController< changePassword(password: string): Promise { this.#assertIsUnlocked(); + // If the password is the same, do nothing. + if (this.#password === password) { + return Promise.resolve(); + } + return this.#persistOrRollback(async () => { assertIsValidPassword(password); @@ -1480,32 +1499,10 @@ export class KeyringController extends BaseController< this.#assertIsUnlocked(); return await this.#withControllerLock(async () => { - // 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; - }); - } + const { encryptionKey } = this.state; + assertIsEncryptionKeySet(encryptionKey); - return this.state.encryptionKey; + return encryptionKey; }); } diff --git a/packages/keyring-controller/src/constants.ts b/packages/keyring-controller/src/constants.ts index d914a3d6f74..b3ee59ba03c 100644 --- a/packages/keyring-controller/src/constants.ts +++ b/packages/keyring-controller/src/constants.ts @@ -36,4 +36,5 @@ export enum KeyringControllerError { NoHdKeyring = 'KeyringController - No HD Keyring found', ControllerLockRequired = 'KeyringController - attempt to update vault during a non mutually exclusive operation', LastAccountInPrimaryKeyring = 'KeyringController - Last account in primary keyring cannot be removed', + EncryptionKeyNotSet = 'KeyringController - Encryption key not set', }