diff --git a/eslint-warning-thresholds.json b/eslint-warning-thresholds.json index cca3ed4748e..fed971175a3 100644 --- a/eslint-warning-thresholds.json +++ b/eslint-warning-thresholds.json @@ -215,10 +215,6 @@ "import-x/namespace": 14, "jest/no-conditional-in-test": 8 }, - "packages/keyring-controller/src/KeyringController.ts": { - "@typescript-eslint/no-unsafe-enum-comparison": 4, - "@typescript-eslint/no-unused-vars": 1 - }, "packages/keyring-controller/tests/mocks/mockKeyring.ts": { "@typescript-eslint/prefer-readonly": 1 }, diff --git a/packages/keyring-controller/CHANGELOG.md b/packages/keyring-controller/CHANGELOG.md index 38f4b66ca6b..d2766b4472e 100644 --- a/packages/keyring-controller/CHANGELOG.md +++ b/packages/keyring-controller/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Add support for envelope encryption ([#5940](https://github.com/MetaMask/core/pull/5940)) + ## [22.0.2] ### Fixed diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index 08e726c34fb..6461153f6d5 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -37,7 +37,9 @@ import { keyringBuilderFactory, } from './KeyringController'; import MockEncryptor, { + DECRYPTION_ERROR, MOCK_ENCRYPTION_KEY, + SALT, } from '../tests/mocks/mockEncryptor'; import { MockErc4337Keyring } from '../tests/mocks/mockErc4337Keyring'; import { MockKeyring } from '../tests/mocks/mockKeyring'; @@ -67,6 +69,13 @@ const uint8ArraySeed = new Uint8Array( const privateKey = '1e4e6a4c0c077f4ae8ddfbf372918e61dd0fb4a4cfa592cb16e7546d505e68fc'; const password = 'password123'; +const password2 = 'password456'; +const mockEncryptionKey2 = JSON.stringify({ + password: 'password2', + salt: 'salt2', +}); +const freshVault = + '{"data":"{\\"tag\\":{\\"key\\":{\\"password\\":\\"password123\\",\\"salt\\":\\"salt\\"},\\"iv\\":\\"iv\\"},\\"value\\":[{\\"type\\":\\"HD Key Tree\\",\\"data\\":{\\"mnemonic\\":[119,97,114,114,105,111,114,32,108,97,110,103,117,97,103,101,32,106,111,107,101,32,98,111,110,117,115,32,117,110,102,97,105,114,32,97,114,116,105,115,116,32,107,97,110,103,97,114,111,111,32,99,105,114,99,108,101,32,101,120,112,97,110,100,32,104,111,112,101,32,109,105,100,100,108,101,32,103,97,117,103,101],\\"numberOfAccounts\\":1,\\"hdPath\\":\\"m/44\'/60\'/0\'/0\\"},\\"metadata\\":{\\"id\\":\\"01JXEFM7DAX2VJ0YFR4ESNY3GQ\\",\\"name\\":\\"\\"}}]}","iv":"iv","salt":"salt"}'; const commonConfig = { chain: Chain.Goerli, hardfork: Hardfork.Berlin }; @@ -553,7 +562,6 @@ describe('KeyringController', () => { await withController( { cacheEncryptionKey }, async ({ controller, initialState }) => { - const initialVault = controller.state.vault; const initialKeyrings = controller.state.keyrings; await controller.createNewVaultAndRestore( password, @@ -561,7 +569,6 @@ describe('KeyringController', () => { ); expect(controller.state).not.toBe(initialState); expect(controller.state.vault).toBeDefined(); - expect(controller.state.vault).toStrictEqual(initialVault); expect(controller.state.keyrings).toHaveLength( initialKeyrings.length, ); @@ -577,7 +584,7 @@ describe('KeyringController', () => { await withController( { cacheEncryptionKey }, async ({ controller, encryptor }) => { - const encryptSpy = jest.spyOn(encryptor, 'encrypt'); + const encryptSpy = jest.spyOn(encryptor, 'encryptWithKey'); const serializedKeyring = await controller.withKeyring( { type: 'HD Key Tree' }, async ({ keyring }) => keyring.serialize(), @@ -590,7 +597,8 @@ describe('KeyringController', () => { currentSeedWord, ); - expect(encryptSpy).toHaveBeenCalledWith(password, [ + const key = JSON.parse(MOCK_ENCRYPTION_KEY); + expect(encryptSpy).toHaveBeenCalledWith(key, [ { data: serializedKeyring, type: 'HD Key Tree', @@ -797,6 +805,147 @@ describe('KeyringController', () => { ); }); + describe('envelope encryption', () => { + it('should create new vault with encryption key', async () => { + await withController( + { cacheEncryptionKey: true, envelopeEncryption: true }, + async ({ controller }) => { + expect(controller.state.encryptionKey).toBeDefined(); + expect(controller.state.encryptedEncryptionKey).toBeDefined(); + }, + ); + }); + + it('should create new vault with encryption key derived from key seed', async () => { + await withController( + { cacheEncryptionKey: true, skipVaultCreation: true }, + async ({ controller, encryptor }) => { + const key = await encryptor.keyFromPassword(password, SALT); + const keyString = await encryptor.exportKey(key); + await controller.createNewVaultAndKeychain(password, keyString); + expect(controller.state.encryptionKey).toBeDefined(); + expect(controller.state.encryptedEncryptionKey).toBeDefined(); + }, + ); + }); + + it('should throw error if creating new vault with encryption key and cacheEncryptionKey is false', async () => { + await withController( + { cacheEncryptionKey: false, skipVaultCreation: true }, + async ({ controller }) => { + await expect( + controller.createNewVaultAndKeychain(password, MOCK_ENCRYPTION_KEY), + ).rejects.toThrow(KeyringControllerError.CacheEncryptionKeyDisabled); + }, + ); + }); + + it('should unlock with password', async () => { + await withController( + { cacheEncryptionKey: true, envelopeEncryption: true }, + async ({ controller }) => { + expect(controller.isUnlocked()).toBe(true); + expect(controller.state.isUnlocked).toBe(true); + + await controller.setLocked(); + + expect(controller.isUnlocked()).toBe(false); + expect(controller.state.isUnlocked).toBe(false); + + await controller.submitPassword(password); + + expect(controller.isUnlocked()).toBe(true); + expect(controller.state.isUnlocked).toBe(true); + }, + ); + }); + + it('should lock and unlock after change password', async () => { + await withController( + { cacheEncryptionKey: true, envelopeEncryption: true }, + async ({ controller }) => { + await controller.changePassword(password2); + + expect(controller.isUnlocked()).toBe(true); + expect(controller.state.isUnlocked).toBe(true); + + await controller.setLocked(); + + expect(controller.isUnlocked()).toBe(false); + expect(controller.state.isUnlocked).toBe(false); + + await controller.submitPassword(password2); + + expect(controller.isUnlocked()).toBe(true); + expect(controller.state.isUnlocked).toBe(true); + }, + ); + }); + + it('should lock and unlock after change password and key', async () => { + await withController( + { cacheEncryptionKey: true, envelopeEncryption: true }, + async ({ controller }) => { + await controller.changePasswordAndEncryptionKey( + password2, + mockEncryptionKey2, + ); + + expect(controller.isUnlocked()).toBe(true); + expect(controller.state.isUnlocked).toBe(true); + + await controller.setLocked(); + + expect(controller.isUnlocked()).toBe(false); + expect(controller.state.isUnlocked).toBe(false); + + await controller.submitPassword(password2); + + expect(controller.isUnlocked()).toBe(true); + expect(controller.state.isUnlocked).toBe(true); + }, + ); + }); + + it('should verify password', async () => { + await withController( + { cacheEncryptionKey: true, envelopeEncryption: true }, + async ({ controller }) => { + expect(await controller.verifyPassword(password)).toBeUndefined(); + }, + ); + }); + + it('should throw error on verify wrong password', async () => { + await withController( + { cacheEncryptionKey: true, envelopeEncryption: true }, + async ({ controller }) => { + await expect(controller.verifyPassword(password2)).rejects.toThrow( + DECRYPTION_ERROR, + ); + }, + ); + }); + + it('should unlock with old key after change password failed', async () => { + await withController( + { cacheEncryptionKey: true, envelopeEncryption: true }, + async ({ controller, encryptor }) => { + // Make change password fail. + jest.spyOn(encryptor, 'encryptWithKey').mockImplementationOnce(() => { + throw new Error('Not implemented'); + }); + await expect(controller.changePassword(password2)).rejects.toThrow( + 'Not implemented', + ); + + // Check that old password still works. + expect(await controller.submitPassword(password)).toBeUndefined(); + }, + ); + }); + }); + describe('setLocked', () => { it('should set locked correctly', async () => { await withController(async ({ controller }) => { @@ -1302,7 +1451,15 @@ describe('KeyringController', () => { }, ], }; - expect(controller.state).toStrictEqual(modifiedState); + const modifiedStateWithoutVault = { + ...modifiedState, + vault: undefined, + }; + const stateWithoutVault = { + ...controller.state, + vault: undefined, + }; + expect(stateWithoutVault).toStrictEqual(modifiedStateWithoutVault); expect(importedAccountAddress).toBe(address); }); }); @@ -1381,7 +1538,15 @@ describe('KeyringController', () => { }, ], }; - expect(controller.state).toStrictEqual(modifiedState); + const modifiedStateWithoutVault = { + ...modifiedState, + vault: undefined, + }; + const stateWithoutVault = { + ...controller.state, + vault: undefined, + }; + expect(stateWithoutVault).toStrictEqual(modifiedStateWithoutVault); expect(importedAccountAddress).toBe(address); }); }); @@ -2678,10 +2843,10 @@ describe('KeyringController', () => { { cacheEncryptionKey, skipVaultCreation: true, - state: { vault: 'my vault' }, + state: { vault: freshVault }, }, async ({ controller, encryptor }) => { - jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ + jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ { type: 'UnsupportedKeyring', data: '0x1234', @@ -2700,10 +2865,10 @@ describe('KeyringController', () => { { cacheEncryptionKey, skipVaultCreation: true, - state: { vault: 'my vault' }, + state: { vault: freshVault }, }, async ({ controller, encryptor }) => { - jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ + jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ { foo: 'bar', }, @@ -2733,11 +2898,11 @@ describe('KeyringController', () => { await withController( { cacheEncryptionKey, - state: { vault: 'my vault' }, + state: { vault: freshVault }, skipVaultCreation: true, }, async ({ controller, encryptor }) => { - jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ + jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ { type: KeyringTypes.hd, data: { @@ -2776,7 +2941,7 @@ describe('KeyringController', () => { { cacheEncryptionKey: true, state: { - vault: 'my vault', + vault: freshVault, }, skipVaultCreation: true, }, @@ -2787,9 +2952,8 @@ describe('KeyringController', () => { ); jest .spyOn(encryptor, 'importKey') - // @ts-expect-error we are assigning a mock value .mockResolvedValue('imported key'); - jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ + jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ { type: KeyringTypes.hd, data: { @@ -2840,7 +3004,7 @@ describe('KeyringController', () => { { cacheEncryptionKey: false, state: { - vault: 'my vault', + vault: freshVault, }, skipVaultCreation: true, }, @@ -2895,14 +3059,14 @@ describe('KeyringController', () => { { skipVaultCreation: true, cacheEncryptionKey, - state: { vault: 'my vault' }, + state: { vault: freshVault }, keyringBuilders: [keyringBuilderFactory(MockKeyring)], }, async ({ controller, encryptor, messenger }) => { const unlockListener = jest.fn(); messenger.subscribe('KeyringController:unlock', unlockListener); jest.spyOn(encryptor, 'isVaultUpdated').mockReturnValue(false); - jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ + jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ { type: KeyringTypes.hd, data: {}, @@ -2926,12 +3090,12 @@ describe('KeyringController', () => { { skipVaultCreation: true, cacheEncryptionKey, - state: { vault: 'my vault' }, + state: { vault: freshVault }, }, async ({ controller, encryptor }) => { jest.spyOn(encryptor, 'isVaultUpdated').mockReturnValue(false); jest.spyOn(encryptor, 'encrypt').mockRejectedValue(new Error()); - jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ + jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ { type: KeyringTypes.hd, data: { @@ -2955,13 +3119,13 @@ describe('KeyringController', () => { { skipVaultCreation: true, cacheEncryptionKey, - state: { vault: 'my vault' }, + state: { vault: freshVault }, keyringBuilders: [keyringBuilderFactory(MockKeyring)], }, async ({ controller, encryptor, messenger }) => { const unlockListener = jest.fn(); messenger.subscribe('KeyringController:unlock', unlockListener); - jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ + jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ { type: KeyringTypes.hd, data: {}, @@ -2986,12 +3150,12 @@ describe('KeyringController', () => { { skipVaultCreation: true, cacheEncryptionKey, - state: { vault: 'my vault' }, + state: { vault: freshVault }, }, async ({ controller, encryptor }) => { jest.spyOn(encryptor, 'isVaultUpdated').mockReturnValue(false); - const encryptSpy = jest.spyOn(encryptor, 'encrypt'); - jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ + const encryptSpy = jest.spyOn(encryptor, 'encryptWithKey'); + jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ { type: KeyringTypes.hd, data: { @@ -3013,12 +3177,12 @@ describe('KeyringController', () => { { skipVaultCreation: true, cacheEncryptionKey, - state: { vault: 'my vault' }, + state: { vault: freshVault }, }, async ({ controller, encryptor }) => { jest.spyOn(encryptor, 'isVaultUpdated').mockReturnValue(true); const encryptSpy = jest.spyOn(encryptor, 'encrypt'); - jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ + jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ { type: KeyringTypes.hd, data: { @@ -3027,6 +3191,10 @@ describe('KeyringController', () => { }, ]); + // TODO actually this does trigger re-encryption. The catch is + // that this test is run with cacheEncryptionKey enabled, so + // `encryptWithKey` is being used instead of `encrypt`. Hence, + // the spy on `encrypt` doesn't trigger. await controller.submitPassword(password); expect(encryptSpy).not.toHaveBeenCalled(); @@ -3040,7 +3208,7 @@ describe('KeyringController', () => { { skipVaultCreation: true, cacheEncryptionKey, - state: { vault: 'my vault' }, + state: { vault: freshVault }, }, async ({ controller, encryptor }) => { jest.spyOn(encryptor, 'isVaultUpdated').mockReturnValue(false); @@ -3066,7 +3234,7 @@ describe('KeyringController', () => { { skipVaultCreation: true, cacheEncryptionKey, - state: { vault: 'my vault' }, + state: { vault: freshVault }, }, async ({ controller, encryptor }) => { jest.spyOn(encryptor, 'isVaultUpdated').mockReturnValue(true); @@ -3119,14 +3287,19 @@ describe('KeyringController', () => { it('should throw error when using the wrong password', async () => { await withController( { - skipVaultCreation: true, cacheEncryptionKey, - state: { vault: 'my vault' }, + skipVaultCreation: true, + state: { + vault: freshVault, + // @ts-expect-error we want to force the controller to have an + // encryption salt equal to the one in the vault + encryptionSalt: SALT, + }, }, async ({ controller }) => { await expect( controller.submitPassword('wrong password'), - ).rejects.toThrow('Incorrect password.'); + ).rejects.toThrow(DECRYPTION_ERROR); }, ); }); @@ -3154,14 +3327,14 @@ describe('KeyringController', () => { cacheEncryptionKey: true, skipVaultCreation: true, state: { - vault: JSON.stringify({ data: '0x123', salt: 'my salt' }), + vault: freshVault, // @ts-expect-error we want to force the controller to have an // encryption salt equal to the one in the vault - encryptionSalt: 'my salt', + encryptionSalt: SALT, }, }, async ({ controller, initialState, encryptor }) => { - jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ + jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ { type: 'UnsupportedKeyring', data: '0x1234', @@ -3188,19 +3361,15 @@ describe('KeyringController', () => { cacheEncryptionKey: true, skipVaultCreation: true, state: { - vault: JSON.stringify({ data: '0x123', salt: 'my salt' }), + vault: freshVault, // @ts-expect-error we want to force the controller to have an // encryption salt equal to the one in the vault - encryptionSalt: 'my salt', + encryptionSalt: SALT, }, }, async ({ controller, initialState, encryptor }) => { const encryptWithKeySpy = jest.spyOn(encryptor, 'encryptWithKey'); - jest - .spyOn(encryptor, 'importKey') - // @ts-expect-error we are assigning a mock value - .mockResolvedValue('imported key'); - jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ + jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ { type: KeyringTypes.hd, data: '0x123', @@ -3213,18 +3382,21 @@ describe('KeyringController', () => { ); expect(controller.state.isUnlocked).toBe(true); - expect(encryptWithKeySpy).toHaveBeenCalledWith('imported key', [ - { - type: KeyringTypes.hd, - data: { - accounts: ['0x123'], - }, - metadata: { - id: expect.any(String), - name: '', + expect(encryptWithKeySpy).toHaveBeenCalledWith( + JSON.parse(MOCK_ENCRYPTION_KEY), + [ + { + type: KeyringTypes.hd, + data: { + accounts: ['0x123'], + }, + metadata: { + id: expect.any(String), + name: '', + }, }, - }, - ]); + ], + ); }, ); }); @@ -3233,7 +3405,7 @@ describe('KeyringController', () => { await withController( { cacheEncryptionKey: true }, async ({ controller, initialState, encryptor }) => { - jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ + jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ { foo: 'bar', }, @@ -3242,7 +3414,7 @@ describe('KeyringController', () => { await expect( controller.submitEncryptionKey( MOCK_ENCRYPTION_KEY, - initialState.encryptionSalt as string, + initialState.encryptionSalt as string, // TODO Why need the salt here?? ), ).rejects.toThrow(KeyringControllerError.VaultDataError); }, @@ -4875,6 +5047,7 @@ type WithControllerCallback = ({ type WithControllerOptions = Partial & { skipVaultCreation?: boolean; + envelopeEncryption?: boolean; }; type WithControllerArgs = @@ -4946,7 +5119,10 @@ async function withController( ...rest, }); if (!rest.skipVaultCreation) { - await controller.createNewVaultAndKeychain(password); + const encryptionKey = rest.envelopeEncryption + ? MOCK_ENCRYPTION_KEY + : undefined; + await controller.createNewVaultAndKeychain(password, encryptionKey); } return await fn({ controller, diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 65b3954f873..d78c59120a0 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -100,7 +100,11 @@ export type KeyringControllerState = { /** * The salt used to derive the encryption key from the password. */ - encryptionSalt?: string; + encryptionSalt?: string; // TODO why is this stored here? + /** + * The encrypted encryption key. If set, envelope encryption is used. + */ + encryptedEncryptionKey?: string; }; export type KeyringControllerMemState = Omit< @@ -327,6 +331,7 @@ export type SerializedKeyring = { type SessionState = { keyrings: SerializedKeyring[]; password?: string; + encryptionKey?: string; }; /** @@ -543,6 +548,20 @@ function assertIsValidPassword(password: unknown): asserts password is string { } } +/** + * Assert that the provided cacheEncryptionKey is true. + * + * @param cacheEncryptionKey - The cacheEncryptionKey to check. + * @throws If the cacheEncryptionKey is not true. + */ +function assertIsCacheEncryptionKeyTrue( + cacheEncryptionKey: boolean, +): asserts cacheEncryptionKey is true { + if (!cacheEncryptionKey) { + throw new Error(KeyringControllerError.CacheEncryptionKeyDisabled); + } +} + /** * Checks if the provided value is a serialized keyrings array. * @@ -649,6 +668,8 @@ export class KeyringController extends BaseController< #password?: string; + #encryptionKey?: string; + #qrKeyringStateListener?: ( state: ReturnType, ) => void; @@ -679,6 +700,7 @@ export class KeyringController extends BaseController< keyrings: { persist: false, anonymous: false }, encryptionKey: { persist: false, anonymous: false }, encryptionSalt: { persist: false, anonymous: false }, + encryptedEncryptionKey: { persist: true, anonymous: false }, }, messenger, state: { @@ -794,41 +816,55 @@ export class KeyringController extends BaseController< * @param password - Password to unlock keychain. * @param seed - A BIP39-compliant seed phrase as Uint8Array, * either as a string or an array of UTF-8 bytes that represent the string. + * @param encryptionKey - Optional encryption key to encrypt the new vault. If + * set, envelope encryption will be used. * @returns Promise resolving when the operation ends successfully. */ async createNewVaultAndRestore( password: string, seed: Uint8Array, + encryptionKey?: string, ): Promise { return this.#persistOrRollback(async () => { assertIsValidPassword(password); - await this.#createNewVaultWithKeyring(password, { - type: KeyringTypes.hd, - opts: { - mnemonic: seed, - numberOfAccounts: 1, + await this.#createNewVaultWithKeyring( + password, + { + type: KeyringTypes.hd, + opts: { + mnemonic: seed, + numberOfAccounts: 1, + }, }, - }); + encryptionKey, + ); }); } /** * Create a new vault and primary keyring. * - * This only works if keyrings are empty. If there is a pre-existing unlocked vault, calling this will have no effect. - * If there is a pre-existing locked vault, it will be replaced. + * This only works if keyrings are empty. If there is a pre-existing unlocked + * vault, calling this will have no effect. If there is a pre-existing locked + * vault, it will be replaced. * * @param password - Password to unlock the new vault. + * @param encryptionKey - Optional encryption key to encrypt the new vault. If + * set, envelope encryption will be used. * @returns Promise resolving when the operation ends successfully. */ - async createNewVaultAndKeychain(password: string) { + async createNewVaultAndKeychain(password: string, encryptionKey?: string) { return this.#persistOrRollback(async () => { const accounts = await this.#getAccountsFromKeyrings(); if (!accounts.length) { - await this.#createNewVaultWithKeyring(password, { - type: KeyringTypes.hd, - }); + await this.#createNewVaultWithKeyring( + password, + { + type: KeyringTypes.hd, + }, + encryptionKey, + ); } }); } @@ -847,7 +883,7 @@ export class KeyringController extends BaseController< ): Promise { this.#assertIsUnlocked(); - if (type === KeyringTypes.qr) { + if (type === (KeyringTypes.qr as string)) { return this.#getKeyringMetadata(await this.getOrAddQRKeyring()); } @@ -866,7 +902,25 @@ export class KeyringController extends BaseController< if (!this.state.vault) { throw new Error(KeyringControllerError.VaultError); } - await this.#encryptor.decrypt(password, this.state.vault); + + if (this.state.encryptedEncryptionKey) { + // Envelope encryption mode. + + assertIsExportableKeyEncryptor(this.#encryptor); + + const decryptedEncryptionKey = (await this.#encryptor.decrypt( + password, + this.state.encryptedEncryptionKey, + )) as string; + + const importedKey = await this.#encryptor.importKey( + decryptedEncryptionKey, + ); + + await this.#encryptor.decryptWithKey(importedKey, this.state.vault); + } else { + await this.#encryptor.decrypt(password, this.state.vault); + } } /** @@ -1095,7 +1149,7 @@ export class KeyringController extends BaseController< const [input, password] = args; try { wallet = importers.fromEtherWallet(input, password); - } catch (e) { + } catch { wallet = wallet || (await Wallet.fromV3(input, password, true)); } privateKey = bytesToHex(wallet.getPrivateKey()); @@ -1168,6 +1222,7 @@ export class KeyringController extends BaseController< this.#unsubscribeFromQRKeyringsEvents(); this.#password = undefined; + this.#encryptionKey = undefined; await this.#clearKeyrings(); this.update((state) => { @@ -1415,12 +1470,32 @@ export class KeyringController extends BaseController< * @returns Promise resolving when the operation completes. */ changePassword(password: string): Promise { + return this.changePasswordAndEncryptionKey(password); + } + + /** + * Changes the password and encryption key used to encrypt the vault. + * + * @param password - The new password. + * @param encryptionKey - The new encryption key. If omitted, the encryption + * key will not be changed. + * @returns Promise resolving when the operation completes. + */ + changePasswordAndEncryptionKey( + password: string, + encryptionKey?: string, + ): Promise { this.#assertIsUnlocked(); return this.#persistOrRollback(async () => { assertIsValidPassword(password); + // Update password. this.#password = password; + + // Update encryption key. + this.#updateCachedEncryptionKey(encryptionKey); + // We need to clear encryption key and salt from state // to force the controller to re-encrypt the vault using // the new password. @@ -2071,6 +2146,7 @@ export class KeyringController extends BaseController< * @param keyring - A object containing the params to instantiate a new keyring. * @param keyring.type - The keyring type. * @param keyring.opts - Optional parameters required to instantiate the keyring. + * @param encryptionKey - Optional encryption key to encrypt the vault. * @returns A promise that resolves to the state. */ async #createNewVaultWithKeyring( @@ -2079,6 +2155,7 @@ export class KeyringController extends BaseController< type: string; opts?: unknown; }, + encryptionKey?: string, ): Promise { this.#assertControllerMutexIsLocked(); @@ -2093,11 +2170,30 @@ export class KeyringController extends BaseController< this.#password = password; + this.#updateCachedEncryptionKey(encryptionKey); + await this.#clearKeyrings(); await this.#createKeyringWithFirstAccount(keyring.type, keyring.opts); this.#setUnlocked(); } + /** + * Updates the cached vault encryption key. This key will be persisted in + * the state at the next update if envelope encryption is enabled. + * + * @param encryptionKey - Optional new vault encryption key. + */ + #updateCachedEncryptionKey(encryptionKey?: string) { + if (!encryptionKey && this.state.encryptedEncryptionKey) { + // If no encryption key is provided and we are in envelope encryption + // mode, use the cached encryption key. This case occurs when we call + // change password without providing a new encryption key. + this.#encryptionKey = this.state.encryptionKey; + } else { + this.#encryptionKey = encryptionKey; + } + } + /** * Internal non-exclusive method to verify the seed phrase. * @@ -2204,6 +2300,7 @@ export class KeyringController extends BaseController< return { keyrings: await this.#getSerializedKeyrings(), password: this.#password, + encryptionKey: this.#encryptionKey, }; } @@ -2255,7 +2352,7 @@ export class KeyringController extends BaseController< newMetadata: boolean; }> { return this.#withVaultLock(async () => { - const encryptedVault = this.state.vault; + const { vault: encryptedVault, encryptedEncryptionKey } = this.state; if (!encryptedVault) { throw new Error(KeyringControllerError.VaultError); } @@ -2267,15 +2364,33 @@ export class KeyringController extends BaseController< assertIsExportableKeyEncryptor(this.#encryptor); if (password) { - const result = await this.#encryptor.decryptWithDetail( - password, - encryptedVault, - ); - vault = result.vault; - this.#password = password; + if (encryptedEncryptionKey) { + const decryptedEncryptionKey = (await this.#encryptor.decrypt( + password, + encryptedEncryptionKey, + )) as string; + + const importedKey = await this.#encryptor.importKey( + decryptedEncryptionKey, + ); - updatedState.encryptionKey = result.exportedKeyString; - updatedState.encryptionSalt = result.salt; + vault = await this.#encryptor.decryptWithKey( + importedKey, + encryptedVault, + ); + this.#password = password; + updatedState.encryptionKey = decryptedEncryptionKey; + } else { + const result = await this.#encryptor.decryptWithDetail( + password, + encryptedVault, + ); + vault = result.vault; + this.#password = password; + + updatedState.encryptionKey = result.exportedKeyString; + updatedState.encryptionSalt = result.salt; + } } else { const parsedEncryptedVault = JSON.parse(encryptedVault); @@ -2287,9 +2402,9 @@ export class KeyringController extends BaseController< throw new TypeError(KeyringControllerError.WrongPasswordType); } - const key = await this.#encryptor.importKey(encryptionKey); + const importedKey = await this.#encryptor.importKey(encryptionKey); vault = await this.#encryptor.decryptWithKey( - key, + importedKey, parsedEncryptedVault, ); @@ -2342,6 +2457,7 @@ export class KeyringController extends BaseController< await this.#assertNoDuplicateAccounts(); const { encryptionKey, encryptionSalt, vault } = this.state; + // READ THIS CAREFULLY: // We do check if the vault is still considered up-to-date, if not, we would not re-use the // cached key and we will re-generate a new one (based on the password). @@ -2358,7 +2474,9 @@ export class KeyringController extends BaseController< const serializedKeyrings = await this.#getSerializedKeyrings(); if ( - !serializedKeyrings.some((keyring) => keyring.type === KeyringTypes.hd) + !serializedKeyrings.some( + (keyring) => keyring.type === (KeyringTypes.hd as string), + ) ) { throw new Error(KeyringControllerError.NoHdKeyring); } @@ -2377,14 +2495,38 @@ export class KeyringController extends BaseController< vaultJSON.salt = encryptionSalt; updatedState.vault = JSON.stringify(vaultJSON); } else if (this.#password) { - const { vault: newVault, exportedKeyString } = - await this.#encryptor.encryptWithDetail( + if (this.#encryptionKey) { + // Update cached key. + updatedState.encryptionKey = this.#encryptionKey; + + // Encrypt key and update encrypted key. + const encryptedEncryptionKey = await this.#encryptor.encrypt( this.#password, - serializedKeyrings, + this.#encryptionKey, ); + updatedState.encryptedEncryptionKey = encryptedEncryptionKey; - updatedState.vault = newVault; - updatedState.encryptionKey = exportedKeyString; + // Encrypt and update vault. + const importedKey = await this.#encryptor.importKey( + this.#encryptionKey, + ); + const vaultJSON = await this.#encryptor.encryptWithKey( + importedKey, + serializedKeyrings, + ); + // Note that we don't need to explicitly append the salt to the + // vault here as it's already stored as part of the encrypted + // encryption key. + updatedState.vault = JSON.stringify(vaultJSON); + } else { + const { vault: newVault, exportedKeyString } = + await this.#encryptor.encryptWithDetail( + this.#password, + serializedKeyrings, + ); + updatedState.vault = newVault; + updatedState.encryptionKey = exportedKeyString; + } } } else { assertIsValidPassword(this.#password); @@ -2394,6 +2536,10 @@ export class KeyringController extends BaseController< ); } + if (this.#isEnvelopeEncryptionEnabled()) { + assertIsCacheEncryptionKeyTrue(this.#cacheEncryptionKey); + } + if (!updatedState.vault) { throw new Error(KeyringControllerError.MissingVaultData); } @@ -2407,6 +2553,9 @@ export class KeyringController extends BaseController< state.encryptionKey = updatedState.encryptionKey; state.encryptionSalt = JSON.parse(updatedState.vault as string).salt; } + if (updatedState.encryptedEncryptionKey) { + state.encryptedEncryptionKey = updatedState.encryptedEncryptionKey; + } }); return true; @@ -2428,6 +2577,15 @@ export class KeyringController extends BaseController< return !this.#encryptor.isVaultUpdated(vault); } + /** + * Checks if envelope encryption is enabled. + * + * @returns A boolean indicating whether envelope encryption is enabled. + */ + #isEnvelopeEncryptionEnabled() { + return this.state.encryptedEncryptionKey || this.#encryptionKey; + } + /** * Retrieves all the accounts from keyrings instances * that are currently in memory. @@ -2533,7 +2691,10 @@ export class KeyringController extends BaseController< await keyring.init(); } - if (type === KeyringTypes.hd && (!isObject(data) || !data.mnemonic)) { + if ( + type === (KeyringTypes.hd as string) && + (!isObject(data) || !data.mnemonic) + ) { if (!keyring.generateRandomMnemonic) { throw new Error( KeyringControllerError.UnsupportedGenerateRandomMnemonic, @@ -2547,7 +2708,7 @@ export class KeyringController extends BaseController< await keyring.addAccounts(1); } - if (type === KeyringTypes.qr) { + if (type === (KeyringTypes.qr as string)) { // In case of a QR keyring type, we need to subscribe // to its events after creating it this.#subscribeToQRKeyringEvents(keyring as unknown as QRKeyring); @@ -2694,11 +2855,16 @@ export class KeyringController extends BaseController< } /** - * Execute the given function after acquiring the controller lock - * and save the vault to state after it (only if needed), or rollback to their - * previous state in case of error. + * Execute the given function after acquiring the controller lock and save the + * vault to state after it (only if needed), or rollback to their previous + * state in case of error. * - * @param callback - The function to execute. + * ATTENTION: The callback must **not** alter `controller.state`. Any state + * change performed by the callback will not be rolled back on error. + * + * @param callback - The function to execute. This callback must **not** alter + * `controller.state`. Any state change performed by the callback will not be + * rolled back on error. * @returns The result of the function. */ async #persistOrRollback( @@ -2731,12 +2897,13 @@ export class KeyringController extends BaseController< return this.#withControllerLock(async ({ releaseLock }) => { const currentSerializedKeyrings = await this.#getSerializedKeyrings(); const currentPassword = this.#password; - + const currentEncryptionKey = this.#encryptionKey; try { return await callback({ releaseLock }); } catch (e) { - // Keyrings and password are restored to their previous state + // Previous state is restored. this.#password = currentPassword; + this.#encryptionKey = currentEncryptionKey; await this.#restoreSerializedKeyrings(currentSerializedKeyrings); throw e; diff --git a/packages/keyring-controller/src/constants.ts b/packages/keyring-controller/src/constants.ts index d914a3d6f74..c82805c0983 100644 --- a/packages/keyring-controller/src/constants.ts +++ b/packages/keyring-controller/src/constants.ts @@ -4,14 +4,16 @@ export enum KeyringControllerError { UnsafeDirectKeyringAccess = 'KeyringController - Returning keyring instances is unsafe', WrongPasswordType = 'KeyringController - Password must be of type string.', InvalidEmptyPassword = 'KeyringController - Password cannot be empty.', + CacheEncryptionKeyDisabled = 'KeyringController - Cache encryption key is disabled.', 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.', VaultDataError = 'KeyringController - The decrypted vault has an unexpected shape.', + StateChangedWhileExecutingCallback = 'KeyringController - State changed while executing callback', UnsupportedEncryptionKeyExport = 'KeyringController - The encryptor does not support encryption key export.', UnsupportedGenerateRandomMnemonic = 'KeyringController - The current keyring does not support the method generateRandomMnemonic.', - UnsupportedExportAccount = '`KeyringController - The keyring for the current address does not support the method exportAccount', - UnsupportedRemoveAccount = '`KeyringController - The keyring for the current address does not support the method removeAccount', + UnsupportedExportAccount = 'KeyringController - The keyring for the current address does not support the method exportAccount', + UnsupportedRemoveAccount = 'KeyringController - The keyring for the current address does not support the method removeAccount', UnsupportedSignTransaction = 'KeyringController - The keyring for the current address does not support the method signTransaction.', UnsupportedSignMessage = 'KeyringController - The keyring for the current address does not support the method signMessage.', UnsupportedSignPersonalMessage = 'KeyringController - The keyring for the current address does not support the method signPersonalMessage.', diff --git a/packages/keyring-controller/tests/mocks/mockEncryptor.ts b/packages/keyring-controller/tests/mocks/mockEncryptor.ts index 034d9e32d1d..47372cb3aaf 100644 --- a/packages/keyring-controller/tests/mocks/mockEncryptor.ts +++ b/packages/keyring-controller/tests/mocks/mockEncryptor.ts @@ -1,84 +1,117 @@ +// Omitting jsdoc because mock is only internal and simple enough. +/* eslint-disable jsdoc/require-jsdoc */ + +import type { + DetailedDecryptResult, + DetailedEncryptionResult, + EncryptionResult, +} from '@metamask/browser-passworder'; +import type { Json } from '@metamask/utils'; +import { isEqual } from 'lodash'; + import type { ExportableKeyEncryptor } from '../../src/KeyringController'; export const PASSWORD = 'password123'; +export const SALT = 'salt'; export const MOCK_ENCRYPTION_KEY = JSON.stringify({ - alg: 'A256GCM', - ext: true, - k: 'wYmxkxOOFBDP6F6VuuYFcRt_Po-tSLFHCWVolsHs4VI', - // eslint-disable-next-line @typescript-eslint/naming-convention - key_ops: ['encrypt', 'decrypt'], - kty: 'oct', + password: PASSWORD, + salt: SALT, }); -export const MOCK_ENCRYPTION_SALT = - 'HQ5sfhsb8XAQRJtD+UqcImT7Ve4n3YMagrh05YTOsjk='; -export const MOCK_HARDCODED_KEY = 'key'; -export const MOCK_HEX = '0xabcdef0123456789'; -// eslint-disable-next-line no-restricted-globals -const MOCK_KEY = Buffer.alloc(32); -const INVALID_PASSWORD_ERROR = 'Incorrect password.'; -export default class MockEncryptor implements ExportableKeyEncryptor { - cacheVal?: string; +export const DECRYPTION_ERROR = 'Decryption failed.'; + +function deriveKey(password: string, salt: string) { + return { + password, + salt, + }; +} - // TODO: Replace `any` with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - async encrypt(password: string, dataObj: any) { +export default class MockEncryptor implements ExportableKeyEncryptor { + async encrypt(password: string, dataObj: Json): Promise { + const salt = generateSalt(); + const key = deriveKey(password, salt); + const result = await this.encryptWithKey(key, dataObj); return JSON.stringify({ - ...(await this.encryptWithKey(password, dataObj)), - salt: this.generateSalt(), + ...result, + salt, }); } - async decrypt(_password: string, _text: string) { - if (_password && _password !== PASSWORD) { - throw new Error(INVALID_PASSWORD_ERROR); - } - - return JSON.parse(this.cacheVal || '') ?? {}; + async decrypt(password: string, text: string): Promise { + const { salt } = JSON.parse(text); + const key = deriveKey(password, salt); + return await this.decryptWithKey(key, text); } - // TODO: Replace `any` with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - async encryptWithKey(_key: unknown, dataObj: any) { - this.cacheVal = JSON.stringify(dataObj); + async encryptWithDetail( + password: string, + dataObj: Json, + salt?: string, + ): Promise { + const _salt = salt ?? generateSalt(); + const key = deriveKey(password, _salt); + const result = await this.encryptWithKey(key, dataObj); return { - data: MOCK_HEX, - iv: 'anIv', + vault: JSON.stringify({ + ...result, + salt: _salt, + }), + exportedKeyString: JSON.stringify(key), }; } - // TODO: Replace `any` with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - async encryptWithDetail(key: string, dataObj: any) { + async decryptWithDetail( + password: string, + text: string, + ): Promise { + const { salt } = JSON.parse(text); + const key = deriveKey(password, salt); return { - vault: await this.encrypt(key, dataObj), - exportedKeyString: MOCK_ENCRYPTION_KEY, + vault: await this.decryptWithKey(key, text), + salt, + exportedKeyString: JSON.stringify(key), }; } - async decryptWithDetail(key: string, text: string) { + async encryptWithKey(key: unknown, dataObj: Json): Promise { + const iv = generateIV(); return { - vault: await this.decrypt(key, text), - salt: MOCK_ENCRYPTION_SALT, - exportedKeyString: MOCK_ENCRYPTION_KEY, + data: JSON.stringify({ + tag: { key, iv }, + value: dataObj, + }), + iv, }; } - async decryptWithKey(key: unknown, text: string) { - return this.decrypt(key as string, text); + async decryptWithKey(key: unknown, ciphertext: string): Promise { + // This conditional assignment is required because sometimes the keyring + // controller passes in the parsed object instead of the string. + const ciphertextObj = + typeof ciphertext === 'string' ? JSON.parse(ciphertext) : ciphertext; + const data = JSON.parse(ciphertextObj.data); + if (!isEqual(data.tag, { key, iv: ciphertextObj.iv })) { + throw new Error(DECRYPTION_ERROR); + } + return data.value; + } + + async importKey(key: string) { + return JSON.parse(key); } - async keyFromPassword(_password: string) { - return MOCK_KEY; + async keyFromPassword( + password: string, + salt: string, + _exportable?: boolean, + _keyDerivationOptions?: unknown, + ) { + return deriveKey(password, salt); } - async importKey(key: string) { - if (key === '{}') { - throw new TypeError( - `Failed to execute 'importKey' on 'SubtleCrypto': The provided value is not of type '(ArrayBuffer or ArrayBufferView or JsonWebKey)'.`, - ); - } - return null; + async exportKey(key: unknown) { + return JSON.stringify(key); } async updateVault(_vault: string, _password: string) { @@ -88,8 +121,18 @@ export default class MockEncryptor implements ExportableKeyEncryptor { isVaultUpdated(_vault: string) { return true; } +} - generateSalt() { - return MOCK_ENCRYPTION_SALT; - } +function generateSalt() { + // Generate random salt. + + // return crypto.randomUUID(); + return SALT; // TODO some tests rely on fixed salt, but wouldn't it be better to generate random value here? +} + +function generateIV() { + // Generate random salt. + + // return crypto.randomUUID(); + return 'iv'; // TODO some tests rely on fixed iv, but wouldn't it be better to generate random value here? }