Skip to content

Keyring Controller: Add method to export vault key #5984

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions packages/keyring-controller/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
52 changes: 52 additions & 0 deletions packages/keyring-controller/src/KeyringController.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3277,6 +3277,58 @@ 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.ControllerLocked,
);
},
);
});

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', () => {
it('should return current seedphrase', async () => {
await withController(async ({ controller }) => {
Expand Down
56 changes: 47 additions & 9 deletions packages/keyring-controller/src/KeyringController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<void> {
const { newMetadata } = await this.#withRollback(async () => {
const result = await this.#unlockKeyrings(
Expand All @@ -1470,6 +1471,44 @@ export class KeyringController extends BaseController<
}
}

/**
* Exports the vault encryption key.
*
* @returns The vault encryption key.
*/
async exportEncryptionKey(): Promise<string> {
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,
);
Comment on lines +1493 to +1497
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  • Add keyFromPassword to ExportableKeyEncryptor
  • Take care of using the same keyMetadata and salt from the vault

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

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

if (this.#cacheEncryptionKey) {
this.update((state) => {
state.encryptionKey = result.exportedKeyString;
state.encryptionSalt = result.salt;
});
}
Comment on lines +1498 to +1503
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return result.exportedKeyString;
});
}
Comment on lines +1483 to +1506
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

Any thoughts on that @mikesposito?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


return this.state.encryptionKey;
});
}

/**
* Attempts to decrypt the current vault and load its keyrings,
* using the given password.
Expand Down Expand Up @@ -2279,8 +2318,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') {
Expand All @@ -2296,10 +2337,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') {
Expand Down
Loading