diff --git a/packages/keyring-controller/CHANGELOG.md b/packages/keyring-controller/CHANGELOG.md index 62e88f97ee9..936d9548a6b 100644 --- a/packages/keyring-controller/CHANGELOG.md +++ b/packages/keyring-controller/CHANGELOG.md @@ -17,6 +17,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **BREAKING:** The `KeyringController` constructor options now require an encryptor ([#7127](https://github.com/MetaMask/core/pull/7127)) - The `encryptor` constructor option was previously optional and defaulted to an instance of `@metamask/browser-passworder`. - **BREAKING:** The `GenericEncryptor` and `ExportableKeyEncryptor` types have been merged into a single `Encryptor` type ([#7127](https://github.com/MetaMask/core/pull/7127)) +- **BREAKING:** The `Encryptor` type requires `exportKey`, `keyFromPassword` and `generateSalt` methods ([#7128](https://github.com/MetaMask/core/pull/7128)) ### Removed diff --git a/packages/keyring-controller/jest.config.js b/packages/keyring-controller/jest.config.js index 79cab1c92de..972e5a3e382 100644 --- a/packages/keyring-controller/jest.config.js +++ b/packages/keyring-controller/jest.config.js @@ -17,10 +17,10 @@ module.exports = merge(baseConfig, { // An object that configures minimum threshold enforcement for coverage results coverageThreshold: { global: { - branches: 94.03, + branches: 95.2, functions: 100, - lines: 98.31, - statements: 98.33, + lines: 98.8, + statements: 98.81, }, }, diff --git a/packages/keyring-controller/package.json b/packages/keyring-controller/package.json index 95c8e22f6a7..344e0278ec7 100644 --- a/packages/keyring-controller/package.json +++ b/packages/keyring-controller/package.json @@ -50,7 +50,7 @@ "dependencies": { "@ethereumjs/util": "^9.1.0", "@metamask/base-controller": "^9.0.0", - "@metamask/browser-passworder": "^4.3.0", + "@metamask/browser-passworder": "^6.0.0", "@metamask/eth-hd-keyring": "^13.0.0", "@metamask/eth-sig-util": "^8.2.0", "@metamask/eth-simple-keyring": "^11.0.0", diff --git a/packages/keyring-controller/src/KeyringController.test.ts b/packages/keyring-controller/src/KeyringController.test.ts index dcc35b1e5f5..cfd586554aa 100644 --- a/packages/keyring-controller/src/KeyringController.test.ts +++ b/packages/keyring-controller/src/KeyringController.test.ts @@ -614,6 +614,23 @@ describe('KeyringController', () => { }); }); + it('should create new vault with a different password', async () => { + await withController(async ({ controller, initialState }) => { + const initialKeyrings = controller.state.keyrings; + + await controller.createNewVaultAndRestore( + 'new-password', + uint8ArraySeed, + ); + + expect(controller.state).not.toBe(initialState); + expect(controller.state.vault).toBeDefined(); + expect(controller.state.keyrings).toHaveLength(initialKeyrings.length); + // new keyring metadata should be generated + expect(controller.state.keyrings).not.toStrictEqual(initialKeyrings); + }); + }); + it('should throw error if creating new vault and restore without password', async () => { await withController(async ({ controller }) => { await expect( @@ -2555,12 +2572,15 @@ describe('KeyringController', () => { it('should encrypt the vault with the new password', async () => { await withController(async ({ controller, encryptor }) => { const newPassword = 'new-password'; - const spiedEncryptionFn = jest.spyOn(encryptor, 'encryptWithDetail'); + const keyFromPasswordSpy = jest.spyOn(encryptor, 'keyFromPassword'); await controller.changePassword(newPassword); - // we pick the first argument of the first call - expect(spiedEncryptionFn.mock.calls[0][0]).toBe(newPassword); + expect(keyFromPasswordSpy).toHaveBeenCalledWith( + newPassword, + controller.state.encryptionSalt, + true, + ); }); }); @@ -2672,6 +2692,24 @@ describe('KeyringController', () => { ); }); + it('should throw an error if the encryptor returns an undefined encryption key', async () => { + await withController( + { skipVaultCreation: true, state: { vault: createVault() } }, + async ({ controller, encryptor }) => { + jest.spyOn(encryptor, 'decryptWithDetail').mockResolvedValueOnce({ + vault: defaultKeyrings, + // @ts-expect-error we are testing a broken encryptor + exportedKeyString: undefined, + salt: '', + }); + + await expect(controller.submitPassword(password)).rejects.toThrow( + KeyringControllerError.MissingCredentials, + ); + }, + ); + }); + it('should unlock succesfully when the controller is instantiated with an existing `keyringsMetadata`', async () => { stubKeyringClassWithAccount(HdKeyring, '0x123'); await withController( @@ -2710,6 +2748,14 @@ describe('KeyringController', () => { }); it('should generate new metadata when there is no metadata in the vault', async () => { + const vault = createVault([ + { + type: KeyringTypes.hd, + data: { + accounts: ['0x123'], + }, + }, + ]); const hdKeyringSerializeSpy = jest.spyOn( HdKeyring.prototype, 'serialize', @@ -2717,20 +2763,12 @@ describe('KeyringController', () => { await withController( { state: { - vault: createVault([ - { - type: KeyringTypes.hd, - data: { - accounts: ['0x123'], - }, - }, - ]), + vault, }, skipVaultCreation: true, }, async ({ controller, encryptor }) => { const encryptWithKeySpy = jest.spyOn(encryptor, 'encryptWithKey'); - jest.spyOn(encryptor, 'importKey').mockResolvedValue('imported key'); hdKeyringSerializeSpy.mockResolvedValue({ // @ts-expect-error we are assigning a mock value accounts: ['0x123'], @@ -2748,7 +2786,7 @@ describe('KeyringController', () => { }, }, ]); - expect(encryptWithKeySpy).toHaveBeenCalledWith('imported key', [ + expect(encryptWithKeySpy).toHaveBeenCalledWith(defaultCredentials, [ { type: KeyringTypes.hd, data: { @@ -2969,6 +3007,43 @@ describe('KeyringController', () => { }, ); }); + + it('should throw an error if the password is not a string', async () => { + await withController(async ({ controller }) => { + await expect( + // @ts-expect-error we are testing wrong input + controller.submitPassword(123456), + ).rejects.toThrow(KeyringControllerError.WrongPasswordType); + }); + }); + + it('should siletly fail the key derivation params upgrade if it fails', async () => { + await withController( + { + skipVaultCreation: true, + state: { + vault: createVault([ + { + type: KeyringTypes.hd, + data: { + accounts: ['0x123'], + }, + }, + ]), + }, + }, + async ({ controller, encryptor }) => { + jest.spyOn(encryptor, 'isVaultUpdated').mockReturnValue(false); + jest + .spyOn(encryptor, 'exportKey') + .mockRejectedValue(new Error('Error')); + + await controller.submitPassword(password); + + expect(controller.state.isUnlocked).toBe(true); + }, + ); + }); }); describe('submitEncryptionKey', () => { @@ -3057,6 +3132,57 @@ describe('KeyringController', () => { ); }); + it('should suppress errors if new metadata is created while unlocking and the vault update fails', async () => { + jest.spyOn(HdKeyring.prototype, 'serialize').mockResolvedValue({ + // @ts-expect-error we are assigning a mock value + accounts: ['0x123'], + }); + await withController( + { + skipVaultCreation: true, + state: { + vault: createVault([ + { + type: KeyringTypes.hd, + data: '0x123', + }, + ]), + // @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, initialState, encryptor }) => { + const encryptWithKeySpy = jest.spyOn(encryptor, 'encryptWithKey'); + jest + .spyOn(encryptor, 'encryptWithKey') + .mockRejectedValueOnce(new Error('Error')); + + await controller.submitEncryptionKey( + MOCK_ENCRYPTION_KEY, + initialState.encryptionSalt as string, + ); + + expect(controller.state.isUnlocked).toBe(true); + expect(encryptWithKeySpy).toHaveBeenCalledWith( + JSON.parse(MOCK_ENCRYPTION_KEY), + [ + { + type: KeyringTypes.hd, + data: { + accounts: ['0x123'], + }, + metadata: { + id: expect.any(String), + name: '', + }, + }, + ], + ); + }, + ); + }); + it('should throw error if vault unlocked has an unexpected shape', async () => { await withController(async ({ controller, initialState, encryptor }) => { jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ @@ -3083,15 +3209,15 @@ describe('KeyringController', () => { }); it('should throw error if encryptionKey is of an unexpected type', async () => { - await withController(async ({ controller, initialState }) => { + await withController(async ({ controller }) => { await expect( controller.submitEncryptionKey( // @ts-expect-error we are testing the case of a user using // the wrong encryptionKey type 12341234, - initialState.encryptionSalt as string, + SALT, ), - ).rejects.toThrow(KeyringControllerError.WrongPasswordType); + ).rejects.toThrow(KeyringControllerError.WrongEncryptionKeyType); }); }); }); diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts index 2bf36e86dc3..b4dce9f1354 100644 --- a/packages/keyring-controller/src/KeyringController.ts +++ b/packages/keyring-controller/src/KeyringController.ts @@ -32,7 +32,7 @@ import { Mutex } from 'async-mutex'; import type { MutexInterface } from 'async-mutex'; import Wallet, { thirdparty as importers } from 'ethereumjs-wallet'; import type { Patch } from 'immer'; -import { isEqual } from 'lodash'; +import { cloneDeep, isEqual } from 'lodash'; // When generating a ULID within the same millisecond, monotonicFactory provides some guarantees regarding sort order. import { ulid } from 'ulid'; @@ -328,12 +328,26 @@ export type SerializedKeyring = { metadata?: KeyringMetadata; }; +/** + * Cached encryption key used to encrypt/decrypt the vault. + */ +type CachedEncryptionKey = { + /** + * The serialized encryption key. + */ + serialized: string; + /** + * The salt used to derive the encryption key. + */ + salt: string; +}; + /** * State/data that can be updated during a `withKeyring` operation. */ type SessionState = { keyrings: SerializedKeyring[]; - password?: string; + encryptionKey?: CachedEncryptionKey; }; export type EncryptionResultConstraint = { @@ -443,6 +457,32 @@ export type Encryptor< * @returns The encryption key. */ importKey: (key: string) => Promise; + /** + * Exports the encryption key as a string. + * + * @param key - The encryption key to export. + * @returns The serialized key string. + */ + exportKey: (key: EncryptionKey) => Promise; + /** + * Derives an encryption key from a password. + * + * @param password - The password to derive the key from. + * @param salt - The salt to use for key derivation. + * @param exportable - Whether the key should be exportable or not. + * @param options - Optional key derivation options. + * @returns The derived encryption key. + */ + keyFromPassword: ( + password: string, + salt: string, + exportable?: boolean, + keyDerivationOptions?: SupportedKeyDerivationParams, + ) => Promise; + /** + * Generates a random salt for key derivation. + */ + generateSalt: typeof encryptorUtils.generateSalt; }; export type KeyringSelector = @@ -659,7 +699,7 @@ export class KeyringController< #unsupportedKeyrings: SerializedKeyring[]; - #password?: string; + #encryptionKey?: CachedEncryptionKey; /** * Creates a KeyringController instance. @@ -1188,7 +1228,7 @@ export class KeyringController< this.#assertIsUnlocked(); return this.#withRollback(async () => { - this.#password = undefined; + this.#encryptionKey = undefined; await this.#clearKeyrings(); this.update((state) => { @@ -1438,21 +1478,10 @@ export class KeyringController< 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); - - this.#password = password; - // We need to clear encryption key and salt from state - // to force the controller to re-encrypt the vault using - // the new password. - this.update((state) => { - delete state.encryptionKey; - delete state.encryptionSalt; + await this.#deriveAndSetEncryptionKey(password, { + ignoreExistingVault: true, }); }); } @@ -1471,11 +1500,10 @@ export class KeyringController< encryptionSalt?: string, ): Promise { const { newMetadata } = await this.#withRollback(async () => { - const result = await this.#unlockKeyrings( - undefined, + const result = await this.#unlockKeyrings({ encryptionKey, encryptionSalt, - ); + }); this.#setUnlocked(); return result; }); @@ -1504,10 +1532,8 @@ export class KeyringController< this.#assertIsUnlocked(); return await this.#withControllerLock(async () => { - const { encryptionKey } = this.state; - assertIsEncryptionKeySet(encryptionKey); - - return encryptionKey; + assertIsEncryptionKeySet(this.#encryptionKey?.serialized); + return this.#encryptionKey.serialized; }); } @@ -1520,7 +1546,7 @@ export class KeyringController< */ async submitPassword(password: string): Promise { const { newMetadata } = await this.#withRollback(async () => { - const result = await this.#unlockKeyrings(password); + const result = await this.#unlockKeyrings({ password }); this.#setUnlocked(); return result; }); @@ -1531,6 +1557,12 @@ export class KeyringController< // can attempt to upgrade the vault. await this.#withRollback(async () => { if (newMetadata || this.#isNewEncryptionAvailable()) { + await this.#deriveAndSetEncryptionKey(password, { + // If the vault is being upgraded, we want to ignore the metadata + // that is already in the vault, so we can effectively + // re-encrypt the vault with the new encryption config. + ignoreExistingVault: true, + }); await this.#updateVault(); } }); @@ -1874,13 +1906,97 @@ export class KeyringController< delete state.encryptionSalt; }); - this.#password = password; + await this.#deriveAndSetEncryptionKey(password, { + ignoreExistingVault: true, + }); await this.#clearKeyrings(); await this.#createKeyringWithFirstAccount(keyring.type, keyring.opts); this.#setUnlocked(); } + /** + * Derive the vault encryption key from the provided password, and + * assign it to the instance variable for later use with cryptographic + * functions. + * + * When the controller has a vault in its state, the key is derived + * using the salt from the vault. If the vault is empty, a new salt + * is generated and used to derive the key. + * + * If `options.ignoreExistingVault` is set to `true`, the existing + * vault is completely ignored: the new key won't be able to decrypt + * the existing vault, and should be used to re-encrypt it. + * + * @param password - The password to use for decryption or derivation. + * @param options - Options for the key derivation. + * @param options.ignoreExistingVault - Whether to ignore the existing vault salt and key metadata + */ + async #deriveAndSetEncryptionKey( + password: string, + options: { ignoreExistingVault: boolean } = { + ignoreExistingVault: false, + }, + ): Promise { + this.#assertControllerMutexIsLocked(); + const { vault } = this.state; + + if (typeof password !== 'string') { + throw new TypeError(KeyringControllerError.WrongPasswordType); + } + + let serializedEncryptionKey: string, salt: string; + if (vault && !options.ignoreExistingVault) { + // The `decryptWithDetail` method is being used here instead of + // `keyFromPassword` + `exportKey` to let the encryptor handle + // any legacy encryption formats and metadata that might be + // present (or absent) in the vault. + const { exportedKeyString, salt: existingSalt } = + await this.#encryptor.decryptWithDetail(password, vault); + serializedEncryptionKey = exportedKeyString; + salt = existingSalt; + } else { + salt = this.#encryptor.generateSalt(); + serializedEncryptionKey = await this.#encryptor.exportKey( + await this.#encryptor.keyFromPassword(password, salt, true), + ); + } + + this.#encryptionKey = { + salt, + serialized: serializedEncryptionKey, + }; + } + + /** + * Set the the `#encryptionKey` instance variable. + * This method is used when the user provides an encryption key and salt + * to unlock the keychain, instead of using a password. + * + * @param encryptionKey - The encryption key to use. + * @param keyDerivationSalt - The salt to use for the encryption key. + */ + #setEncryptionKey(encryptionKey: string, keyDerivationSalt: string): void { + this.#assertControllerMutexIsLocked(); + + if ( + typeof encryptionKey !== 'string' || + typeof keyDerivationSalt !== 'string' + ) { + throw new TypeError(KeyringControllerError.WrongEncryptionKeyType); + } + + const { vault } = this.state; + if (vault && JSON.parse(vault).salt !== keyDerivationSalt) { + throw new Error(KeyringControllerError.ExpiredCredentials); + } + + this.#encryptionKey = { + salt: keyDerivationSalt, + serialized: encryptionKey, + }; + } + /** * Internal non-exclusive method to verify the seed phrase. * @@ -1977,7 +2093,7 @@ export class KeyringController< } /** - * Get a snapshot of session data held by class variables. + * Get a snapshot of session data held by instance variables. * * @returns An object with serialized keyrings, keyrings metadata, * and the user password. @@ -1985,7 +2101,7 @@ export class KeyringController< async #getSessionState(): Promise { return { keyrings: await this.#getSerializedKeyrings(), - password: this.#password, + encryptionKey: this.#encryptionKey, }; } @@ -2023,60 +2139,48 @@ export class KeyringController< * Unlock Keyrings, decrypting the vault and deserializing all * keyrings contained in it, using a password or an encryption key with salt. * - * @param password - The keyring controller password. - * @param encryptionKey - An exported key string to unlock keyrings with. - * @param encryptionSalt - The salt used to encrypt the vault. + * @param credentials - The credentials to unlock the keyrings. * @returns A promise resolving to the deserialized keyrings array. */ async #unlockKeyrings( - password: string | undefined, - encryptionKey?: string, - encryptionSalt?: string, + credentials: + | { + password: string; + } + | { + encryptionKey: string; + encryptionSalt?: string; + }, ): Promise<{ keyrings: { keyring: EthKeyring; metadata: KeyringMetadata }[]; newMetadata: boolean; }> { return this.#withVaultLock(async () => { - const encryptedVault = this.state.vault; - if (!encryptedVault) { + if (!this.state.vault) { throw new Error(KeyringControllerError.VaultError); } + const parsedEncryptedVault = JSON.parse(this.state.vault); - let vault; - const updatedState: Partial = {}; - - if (password) { - const result = await this.#encryptor.decryptWithDetail( - password, - encryptedVault, - ); - vault = result.vault; - this.#password = password; - - updatedState.encryptionKey = result.exportedKeyString; - updatedState.encryptionSalt = result.salt; + if ('password' in credentials) { + await this.#deriveAndSetEncryptionKey(credentials.password); } else { - const parsedEncryptedVault = JSON.parse(encryptedVault); - - if (encryptionSalt && encryptionSalt !== parsedEncryptedVault.salt) { - throw new Error(KeyringControllerError.ExpiredCredentials); - } else { - encryptionSalt = parsedEncryptedVault.salt as string; - } - - if (typeof encryptionKey !== 'string') { - throw new TypeError(KeyringControllerError.WrongPasswordType); - } - - const key = await this.#encryptor.importKey(encryptionKey); - vault = await this.#encryptor.decryptWithKey(key, parsedEncryptedVault); + this.#setEncryptionKey( + credentials.encryptionKey, + credentials.encryptionSalt || parsedEncryptedVault.salt, + ); + } - // This call is required on the first call because encryptionKey - // is not yet inside the memStore - updatedState.encryptionKey = encryptionKey; - updatedState.encryptionSalt = encryptionSalt; + const encryptionKey = this.#encryptionKey?.serialized; + if (!encryptionKey) { + throw new Error(KeyringControllerError.MissingCredentials); } + const key = await this.#encryptor.importKey(encryptionKey); + const vault = await this.#encryptor.decryptWithKey( + key, + parsedEncryptedVault, + ); + if (!isSerializedKeyringsArray(vault)) { throw new Error(KeyringControllerError.VaultDataError); } @@ -2088,10 +2192,8 @@ export class KeyringController< this.update((state) => { state.keyrings = updatedKeyrings; - if (updatedState.encryptionKey || updatedState.encryptionSalt) { - state.encryptionKey = updatedState.encryptionKey; - state.encryptionSalt = updatedState.encryptionSalt; - } + state.encryptionKey = encryptionKey; + state.encryptionSalt = this.#encryptionKey?.salt; }); return { keyrings, newMetadata }; @@ -2108,62 +2210,44 @@ export class KeyringController< // Ensure no duplicate accounts are persisted. 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). - // - // This helps doing seamless updates of the vault. Useful in case we change some cryptographic - // parameters to the KDF. - const useCachedKey = - encryptionKey && vault && this.#encryptor.isVaultUpdated?.(vault); - - if (!this.#password && !encryptionKey) { + if (!this.#encryptionKey) { throw new Error(KeyringControllerError.MissingCredentials); } 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); } - const updatedState: Partial = {}; - - if (useCachedKey) { - const key = await this.#encryptor.importKey(encryptionKey); - const vaultJSON = await this.#encryptor.encryptWithKey( - key, - serializedKeyrings, - ); - vaultJSON.salt = encryptionSalt; - updatedState.vault = JSON.stringify(vaultJSON); - } else if (this.#password) { - const { vault: newVault, exportedKeyString } = - await this.#encryptor.encryptWithDetail( - this.#password, - serializedKeyrings, - ); - - updatedState.vault = newVault; - updatedState.encryptionKey = exportedKeyString; - } - - if (!updatedState.vault) { - throw new Error(KeyringControllerError.MissingVaultData); - } + const key = await this.#encryptor.importKey( + this.#encryptionKey.serialized, + ); + const encryptedVault = await this.#encryptor.encryptWithKey( + key, + serializedKeyrings, + ); + // We need to include the salt used to derive + // the encryption key, to be able to derive it + // from password again. + encryptedVault.salt = this.#encryptionKey.salt; + const updatedState: Partial = { + vault: JSON.stringify(encryptedVault), + encryptionKey: this.#encryptionKey.serialized, + encryptionSalt: this.#encryptionKey.salt, + }; const updatedKeyrings = await this.#getUpdatedKeyrings(); this.update((state) => { state.vault = updatedState.vault; state.keyrings = updatedKeyrings; - if (updatedState.encryptionKey) { - state.encryptionKey = updatedState.encryptionKey; - state.encryptionSalt = JSON.parse(updatedState.vault as string).salt; - } + state.encryptionKey = updatedState.encryptionKey; + state.encryptionSalt = updatedState.encryptionSalt; }); return true; @@ -2178,7 +2262,7 @@ export class KeyringController< #isNewEncryptionAvailable(): boolean { const { vault } = this.state; - if (!vault || !this.#password || !this.#encryptor.isVaultUpdated) { + if (!vault || !this.#encryptor.isVaultUpdated) { return false; } @@ -2481,13 +2565,13 @@ export class KeyringController< ): Promise { return this.#withControllerLock(async ({ releaseLock }) => { const currentSerializedKeyrings = await this.#getSerializedKeyrings(); - const currentPassword = this.#password; + const currentEncryptionKey = cloneDeep(this.#encryptionKey); try { return await callback({ releaseLock }); } catch (e) { - // Keyrings and password are restored to their previous state - this.#password = currentPassword; + // Keyrings and encryption credentials are restored to their previous state + 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 b3ee59ba03c..588d403d223 100644 --- a/packages/keyring-controller/src/constants.ts +++ b/packages/keyring-controller/src/constants.ts @@ -3,6 +3,7 @@ export enum KeyringControllerError { KeyringNotFound = 'KeyringController - Keyring not found.', UnsafeDirectKeyringAccess = 'KeyringController - Returning keyring instances is unsafe', WrongPasswordType = 'KeyringController - Password must be of type string.', + WrongEncryptionKeyType = 'KeyringController - Encryption key must be of type string.', InvalidEmptyPassword = 'KeyringController - Password cannot be empty.', NoFirstAccount = 'KeyringController - First Account not found.', DuplicatedAccount = 'KeyringController - The account you are trying to import is a duplicate', diff --git a/packages/keyring-controller/tests/mocks/mockEncryptor.ts b/packages/keyring-controller/tests/mocks/mockEncryptor.ts index 86d7c932690..02d167bc081 100644 --- a/packages/keyring-controller/tests/mocks/mockEncryptor.ts +++ b/packages/keyring-controller/tests/mocks/mockEncryptor.ts @@ -17,6 +17,7 @@ export const MOCK_ENCRYPTION_KEY = JSON.stringify({ password: PASSWORD, salt: SALT, }); +export const MOCK_KEY = Buffer.alloc(32); export const DECRYPTION_ERROR = 'Decryption failed.'; @@ -100,10 +101,18 @@ export default class MockEncryptor implements Encryptor { return data.value; } + async keyFromPassword(_password: string, _salt: string) { + return JSON.parse(MOCK_ENCRYPTION_KEY); + } + async importKey(key: string) { return JSON.parse(key); } + async exportKey(key: unknown) { + return JSON.stringify(key); + } + async updateVault(_vault: string, _password: string) { return _vault; } diff --git a/packages/seedless-onboarding-controller/CHANGELOG.md b/packages/seedless-onboarding-controller/CHANGELOG.md index 44d5e0536b3..ca2b5fd222b 100644 --- a/packages/seedless-onboarding-controller/CHANGELOG.md +++ b/packages/seedless-onboarding-controller/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- **BREAKING:** The `encryptor` constructor param requires `exportKey`, `keyFromPassword` and `generateSalt` methods ([#7128](https://github.com/MetaMask/core/pull/7128)) - `SeedlessOnboardingController` now accepts an optional `SupportedKeyDerivationOptions` type parameter ([#7127](https://github.com/MetaMask/core/pull/7127)) - The type parameter can be used to specify which key derivation algorithms are supported by the controller instance. diff --git a/packages/seedless-onboarding-controller/package.json b/packages/seedless-onboarding-controller/package.json index f1bedd42e75..3f3439522df 100644 --- a/packages/seedless-onboarding-controller/package.json +++ b/packages/seedless-onboarding-controller/package.json @@ -50,6 +50,7 @@ "dependencies": { "@metamask/auth-network-utils": "^0.3.0", "@metamask/base-controller": "^9.0.0", + "@metamask/browser-passworder": "^6.0.0", "@metamask/messenger": "^0.3.0", "@metamask/toprf-secure-backup": "^0.10.0", "@metamask/utils": "^11.8.1", @@ -62,7 +63,6 @@ "@lavamoat/allow-scripts": "^3.0.4", "@lavamoat/preinstall-always-fail": "^2.1.0", "@metamask/auto-changelog": "^3.4.4", - "@metamask/browser-passworder": "^4.3.0", "@metamask/keyring-controller": "^24.0.0", "@ts-bridge/cli": "^0.6.4", "@types/elliptic": "^6", diff --git a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts index 885116e6678..12b053a2953 100644 --- a/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts +++ b/packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts @@ -11,6 +11,9 @@ import { encryptWithDetail, decryptWithKey as decryptWithKeyBrowserPassworder, importKey as importKeyBrowserPassworder, + exportKey as exportKeyBrowserPassworder, + generateSalt as generateSaltBrowserPassworder, + keyFromPassword as keyFromPasswordBrowserPassworder, } from '@metamask/browser-passworder'; import { TOPRFError, @@ -163,6 +166,9 @@ function getDefaultSeedlessOnboardingVaultEncryptor() { payload: unknown, ) => Promise, importKey: importKeyBrowserPassworder, + exportKey: exportKeyBrowserPassworder, + generateSalt: generateSaltBrowserPassworder, + keyFromPassword: keyFromPasswordBrowserPassworder, }; } @@ -2451,7 +2457,7 @@ describe('SeedlessOnboardingController', () => { }); it('should throw an error if vault unlocked has invalid authentication data', async () => { - const mockVault = JSON.stringify({ foo: 'bar' }); + const mockVault = JSON.stringify({ foo: 'bar', salt: 'baz' }); await withController( { diff --git a/packages/seedless-onboarding-controller/tests/mocks/vaultEncryptor.ts b/packages/seedless-onboarding-controller/tests/mocks/vaultEncryptor.ts index e97c0b49a0a..b424037cd14 100644 --- a/packages/seedless-onboarding-controller/tests/mocks/vaultEncryptor.ts +++ b/packages/seedless-onboarding-controller/tests/mocks/vaultEncryptor.ts @@ -86,7 +86,7 @@ export default class MockVaultEncryptor async keyFromPassword( password: string, - salt: string = this.DEFAULT_SALT, + salt: string, exportable: boolean = true, opts: KeyDerivationOptions = this.DEFAULT_DERIVATION_PARAMS, ) { @@ -218,4 +218,8 @@ export default class MockVaultEncryptor const result = await this.decryptWithKey(key, payload); return result; } + + generateSalt(): string { + return this.DEFAULT_SALT; + } } diff --git a/yarn.lock b/yarn.lock index c08e1f5ddc0..1582dca40e2 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2903,12 +2903,12 @@ __metadata: languageName: unknown linkType: soft -"@metamask/browser-passworder@npm:^4.3.0": - version: 4.3.0 - resolution: "@metamask/browser-passworder@npm:4.3.0" +"@metamask/browser-passworder@npm:^6.0.0": + version: 6.0.0 + resolution: "@metamask/browser-passworder@npm:6.0.0" dependencies: - "@metamask/utils": "npm:^8.2.0" - checksum: 10/8ba5c50cd6274b0cc0f90a1ee16b960ee150f14c29083f3515f4abe018a28ead32c21f5f4a62a6e27a946b1228adc2ff1f195e71e38782fa39fa8fff116173e6 + "@metamask/utils": "npm:^11.0.1" + checksum: 10/44b31729ccd52e62df27a948fbea2320a11861d6465128b1626a723fc47e25f9e3a8c3bdc09329f3f270b5d9e023107cb439cfb6961a3614c6daa58ea4524f00 languageName: node linkType: hard @@ -3893,7 +3893,7 @@ __metadata: "@lavamoat/preinstall-always-fail": "npm:^2.1.0" "@metamask/auto-changelog": "npm:^3.4.4" "@metamask/base-controller": "npm:^9.0.0" - "@metamask/browser-passworder": "npm:^4.3.0" + "@metamask/browser-passworder": "npm:^6.0.0" "@metamask/eth-hd-keyring": "npm:^13.0.0" "@metamask/eth-sig-util": "npm:^8.2.0" "@metamask/eth-simple-keyring": "npm:^11.0.0" @@ -4720,7 +4720,7 @@ __metadata: "@metamask/auth-network-utils": "npm:^0.3.0" "@metamask/auto-changelog": "npm:^3.4.4" "@metamask/base-controller": "npm:^9.0.0" - "@metamask/browser-passworder": "npm:^4.3.0" + "@metamask/browser-passworder": "npm:^6.0.0" "@metamask/keyring-controller": "npm:^24.0.0" "@metamask/messenger": "npm:^0.3.0" "@metamask/toprf-secure-backup": "npm:^0.10.0" @@ -4999,7 +4999,7 @@ __metadata: languageName: unknown linkType: soft -"@metamask/superstruct@npm:^3.0.0, @metamask/superstruct@npm:^3.1.0, @metamask/superstruct@npm:^3.2.1": +"@metamask/superstruct@npm:^3.1.0, @metamask/superstruct@npm:^3.2.1": version: 3.2.1 resolution: "@metamask/superstruct@npm:3.2.1" checksum: 10/9e29380f2cf8b129283ccb2b568296d92682b705109ba62dbd7739ffd6a1982fe38c7228cdcf3cbee94dbcdd5fcc1c846ab9d1dd3582167154f914422fcff547 @@ -5213,23 +5213,6 @@ __metadata: languageName: node linkType: hard -"@metamask/utils@npm:^8.2.0": - version: 8.5.0 - resolution: "@metamask/utils@npm:8.5.0" - dependencies: - "@ethereumjs/tx": "npm:^4.2.0" - "@metamask/superstruct": "npm:^3.0.0" - "@noble/hashes": "npm:^1.3.1" - "@scure/base": "npm:^1.1.3" - "@types/debug": "npm:^4.1.7" - debug: "npm:^4.3.4" - pony-cause: "npm:^2.1.10" - semver: "npm:^7.5.4" - uuid: "npm:^9.0.1" - checksum: 10/68a42a55f7dc750b75467fb7c05a496c20dac073a2753e0f4d9642c4d8dcb3f9ddf51a09d30337e11637f1777f3dfe22e15b5159dbafb0fdb7bd8c9236056153 - languageName: node - linkType: hard - "@metamask/utils@npm:^9.0.0": version: 9.3.0 resolution: "@metamask/utils@npm:9.3.0"