Skip to content
This repository was archived by the owner on Oct 7, 2024. It is now read-only.

Commit 3896d3e

Browse files
mikespositomikesposito
authored
[15.x] Backport encryption improvements (#312)
* Prefer cached `encryptionKey` for encryption when possible (#307) * fix: prefer encryptionKey for encryption when possible * refactor: add test case * Use encryptor `isVaultUpdated` (#310) * chore: update browser-passworder * refactor: remove `updateVault` from `GenericEncryptor`
1 parent 8bbb7f0 commit 3896d3e

7 files changed

+192
-117
lines changed

‎jest.config.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ module.exports = {
4141
// An object that configures minimum threshold enforcement for coverage results
4242
coverageThreshold: {
4343
global: {
44-
branches: 79.41,
44+
branches: 79.8,
4545
functions: 93.22,
4646
lines: 91.5,
4747
statements: 91.69,

‎package.json

+1-1
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@
4444
},
4545
"dependencies": {
4646
"@ethereumjs/tx": "^4.2.0",
47-
"@metamask/browser-passworder": "^4.2.0",
47+
"@metamask/browser-passworder": "^4.3.0",
4848
"@metamask/eth-hd-keyring": "^7.0.1",
4949
"@metamask/eth-sig-util": "^7.0.0",
5050
"@metamask/eth-simple-keyring": "^6.0.1",

‎src/KeyringController.test.ts

+160-93
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,18 @@ async function initializeKeyringController({
7878
return keyringController;
7979
}
8080

81+
/**
82+
* Delete the encryption key and salt from the `memStore` of the given keyring controller.
83+
*
84+
* @param keyringController - The keyring controller to delete the encryption key and salt from.
85+
*/
86+
function deleteEncryptionKeyAndSalt(keyringController: KeyringController) {
87+
const keyringControllerState = keyringController.memStore.getState();
88+
delete keyringControllerState.encryptionKey;
89+
delete keyringControllerState.encryptionSalt;
90+
keyringController.memStore.updateState(keyringControllerState);
91+
}
92+
8193
describe('KeyringController', () => {
8294
afterEach(() => {
8395
sinon.restore();
@@ -199,83 +211,86 @@ describe('KeyringController', () => {
199211
});
200212

201213
describe('when `cacheEncryptionKey` is enabled', () => {
202-
it('should save an up to date encryption salt to the `memStore` when `password` is unset and `encryptionKey` is set', async () => {
203-
const keyringController = await initializeKeyringController({
204-
password: PASSWORD,
205-
constructorOptions: {
206-
cacheEncryptionKey: true,
207-
},
214+
describe('when `encryptionKey` is set', () => {
215+
it('should save an up to date encryption salt to the `memStore`', async () => {
216+
const keyringController = await initializeKeyringController({
217+
password: PASSWORD,
218+
constructorOptions: {
219+
cacheEncryptionKey: true,
220+
},
221+
});
222+
const vaultEncryptionKey = '🔑';
223+
const vaultEncryptionSalt = '🧂';
224+
const vault = JSON.stringify({ salt: vaultEncryptionSalt });
225+
keyringController.store.updateState({ vault });
226+
227+
await keyringController.unlockKeyrings(
228+
undefined,
229+
vaultEncryptionKey,
230+
vaultEncryptionSalt,
231+
);
232+
233+
expect(keyringController.memStore.getState().encryptionKey).toBe(
234+
vaultEncryptionKey,
235+
);
236+
expect(keyringController.memStore.getState().encryptionSalt).toBe(
237+
vaultEncryptionSalt,
238+
);
239+
240+
const response = await keyringController.persistAllKeyrings();
241+
242+
expect(response).toBe(true);
243+
expect(keyringController.memStore.getState().encryptionKey).toBe(
244+
vaultEncryptionKey,
245+
);
246+
expect(keyringController.memStore.getState().encryptionSalt).toBe(
247+
vaultEncryptionSalt,
248+
);
208249
});
209-
delete keyringController.password;
210-
const vaultEncryptionKey = '🔑';
211-
const vaultEncryptionSalt = '🧂';
212-
const vault = JSON.stringify({ salt: vaultEncryptionSalt });
213-
keyringController.store.updateState({ vault });
214-
215-
await keyringController.unlockKeyrings(
216-
undefined,
217-
vaultEncryptionKey,
218-
vaultEncryptionSalt,
219-
);
220-
221-
expect(keyringController.memStore.getState().encryptionKey).toBe(
222-
vaultEncryptionKey,
223-
);
224-
expect(keyringController.memStore.getState().encryptionSalt).toBe(
225-
vaultEncryptionSalt,
226-
);
227-
228-
const response = await keyringController.persistAllKeyrings();
229-
230-
expect(response).toBe(true);
231-
expect(keyringController.memStore.getState().encryptionKey).toBe(
232-
vaultEncryptionKey,
233-
);
234-
expect(keyringController.memStore.getState().encryptionSalt).toBe(
235-
vaultEncryptionSalt,
236-
);
237250
});
238251

239-
it('should save an up to date encryption salt to the `memStore` when `password` is set through `createNewVaultAndKeychain`', async () => {
240-
const keyringController = await initializeKeyringController({
241-
password: PASSWORD,
242-
constructorOptions: {
243-
cacheEncryptionKey: true,
244-
},
252+
describe('when `encryptionKey` is not set and `password` is set', () => {
253+
it('should save an up to date encryption salt to the `memStore` when `password` is set through `createNewVaultAndKeychain`', async () => {
254+
const keyringController = await initializeKeyringController({
255+
password: PASSWORD,
256+
constructorOptions: {
257+
cacheEncryptionKey: true,
258+
},
259+
});
260+
await keyringController.createNewVaultAndKeychain(PASSWORD);
261+
deleteEncryptionKeyAndSalt(keyringController);
262+
263+
const response = await keyringController.persistAllKeyrings();
264+
265+
expect(response).toBe(true);
266+
expect(keyringController.memStore.getState().encryptionKey).toBe(
267+
MOCK_HARDCODED_KEY,
268+
);
269+
expect(keyringController.memStore.getState().encryptionSalt).toBe(
270+
MOCK_ENCRYPTION_SALT,
271+
);
245272
});
246273

247-
await keyringController.createNewVaultAndKeychain(PASSWORD);
248-
249-
const response = await keyringController.persistAllKeyrings();
250-
251-
expect(response).toBe(true);
252-
expect(keyringController.memStore.getState().encryptionKey).toBe(
253-
MOCK_HARDCODED_KEY,
254-
);
255-
expect(keyringController.memStore.getState().encryptionSalt).toBe(
256-
MOCK_ENCRYPTION_SALT,
257-
);
258-
});
259-
260-
it('should save an up to date encryption salt to the `memStore` when `password` is set through `submitPassword`', async () => {
261-
const keyringController = await initializeKeyringController({
262-
password: PASSWORD,
263-
constructorOptions: {
264-
cacheEncryptionKey: true,
265-
},
274+
it('should save an up to date encryption salt to the `memStore` when `password` is set through `submitPassword`', async () => {
275+
const keyringController = await initializeKeyringController({
276+
password: PASSWORD,
277+
constructorOptions: {
278+
cacheEncryptionKey: true,
279+
},
280+
});
281+
await keyringController.submitPassword(PASSWORD);
282+
deleteEncryptionKeyAndSalt(keyringController);
283+
284+
const response = await keyringController.persistAllKeyrings();
285+
286+
expect(response).toBe(true);
287+
expect(keyringController.memStore.getState().encryptionKey).toBe(
288+
MOCK_HARDCODED_KEY,
289+
);
290+
expect(keyringController.memStore.getState().encryptionSalt).toBe(
291+
MOCK_ENCRYPTION_SALT,
292+
);
266293
});
267-
268-
await keyringController.submitPassword(PASSWORD);
269-
270-
const response = await keyringController.persistAllKeyrings();
271-
272-
expect(response).toBe(true);
273-
expect(keyringController.memStore.getState().encryptionKey).toBe(
274-
MOCK_HARDCODED_KEY,
275-
);
276-
expect(keyringController.memStore.getState().encryptionSalt).toBe(
277-
MOCK_ENCRYPTION_SALT,
278-
);
279294
});
280295
});
281296
});
@@ -836,29 +851,81 @@ describe('KeyringController', () => {
836851
});
837852

838853
describe('with old vault format', () => {
839-
[true, false].forEach((cacheEncryptionKey) => {
840-
describe(`with cacheEncryptionKey = ${cacheEncryptionKey}`, () => {
841-
it('should update the vault', async () => {
842-
const mockEncryptor = new MockEncryptor();
843-
const keyringController = await initializeKeyringController({
844-
password: PASSWORD,
845-
constructorOptions: {
846-
cacheEncryptionKey: true,
847-
encryptor: mockEncryptor,
848-
},
849-
});
850-
const initialVault = keyringController.store.getState().vault;
851-
const updatedVaultMock =
852-
'{"vault": "updated_vault_detail", "salt": "salt"}';
853-
sinon.stub(mockEncryptor, 'updateVault').resolves(updatedVaultMock);
854-
sinon.stub(mockEncryptor, 'encrypt').resolves(updatedVaultMock);
855-
856-
await keyringController.unlockKeyrings(PASSWORD);
857-
const updatedVault = keyringController.store.getState().vault;
858-
859-
expect(initialVault).not.toBe(updatedVault);
860-
expect(updatedVault).toBe(updatedVaultMock);
854+
describe(`with cacheEncryptionKey = true and encryptionKey is unset`, () => {
855+
it('should update the vault', async () => {
856+
const mockEncryptor = new MockEncryptor();
857+
const keyringController = await initializeKeyringController({
858+
password: PASSWORD,
859+
constructorOptions: {
860+
cacheEncryptionKey: true,
861+
encryptor: mockEncryptor,
862+
},
861863
});
864+
deleteEncryptionKeyAndSalt(keyringController);
865+
const initialVault = keyringController.store.getState().vault;
866+
const mockEncryptionResult = {
867+
data: '0x1234',
868+
iv: 'an iv',
869+
};
870+
sinon.stub(mockEncryptor, 'isVaultUpdated').returns(false);
871+
sinon
872+
.stub(mockEncryptor, 'encryptWithKey')
873+
.resolves(mockEncryptionResult);
874+
875+
await keyringController.unlockKeyrings(PASSWORD);
876+
const updatedVault = keyringController.store.getState().vault;
877+
878+
expect(initialVault).not.toBe(updatedVault);
879+
expect(updatedVault).toBe(
880+
JSON.stringify({
881+
...mockEncryptionResult,
882+
salt: MOCK_ENCRYPTION_SALT,
883+
}),
884+
);
885+
});
886+
});
887+
888+
describe(`with cacheEncryptionKey = true and encryptionKey is set`, () => {
889+
it('should not update the vault', async () => {
890+
const mockEncryptor = new MockEncryptor();
891+
const keyringController = await initializeKeyringController({
892+
password: PASSWORD,
893+
constructorOptions: {
894+
cacheEncryptionKey: true,
895+
encryptor: mockEncryptor,
896+
},
897+
});
898+
const initialVault = keyringController.store.getState().vault;
899+
sinon.stub(mockEncryptor, 'isVaultUpdated').returns(false);
900+
901+
await keyringController.unlockKeyrings(PASSWORD);
902+
const updatedVault = keyringController.store.getState().vault;
903+
904+
expect(initialVault).toBe(updatedVault);
905+
});
906+
});
907+
908+
describe(`with cacheEncryptionKey = false`, () => {
909+
it('should update the vault', async () => {
910+
const mockEncryptor = new MockEncryptor();
911+
const keyringController = await initializeKeyringController({
912+
password: PASSWORD,
913+
constructorOptions: {
914+
cacheEncryptionKey: false,
915+
encryptor: mockEncryptor,
916+
},
917+
});
918+
const initialVault = keyringController.store.getState().vault;
919+
const updatedVaultMock =
920+
'{"vault": "updated_vault_detail", "salt": "salt"}';
921+
sinon.stub(mockEncryptor, 'isVaultUpdated').returns(false);
922+
sinon.stub(mockEncryptor, 'encrypt').resolves(updatedVaultMock);
923+
924+
await keyringController.unlockKeyrings(PASSWORD);
925+
const updatedVault = keyringController.store.getState().vault;
926+
927+
expect(initialVault).not.toBe(updatedVault);
928+
expect(updatedVault).toBe(updatedVaultMock);
862929
});
863930
});
864931
});

‎src/KeyringController.ts

+12-12
Original file line numberDiff line numberDiff line change
@@ -802,7 +802,15 @@ class KeyringController extends EventEmitter {
802802
if (this.#cacheEncryptionKey) {
803803
assertIsExportableKeyEncryptor(this.#encryptor);
804804

805-
if (this.password) {
805+
if (encryptionKey) {
806+
const key = await this.#encryptor.importKey(encryptionKey);
807+
const vaultJSON = await this.#encryptor.encryptWithKey(
808+
key,
809+
serializedKeyrings,
810+
);
811+
vaultJSON.salt = encryptionSalt;
812+
vault = JSON.stringify(vaultJSON);
813+
} else if (this.password) {
806814
const { vault: newVault, exportedKeyString } =
807815
await this.#encryptor.encryptWithDetail(
808816
this.password,
@@ -811,14 +819,6 @@ class KeyringController extends EventEmitter {
811819

812820
vault = newVault;
813821
newEncryptionKey = exportedKeyString;
814-
} else if (encryptionKey) {
815-
const key = await this.#encryptor.importKey(encryptionKey);
816-
const vaultJSON = await this.#encryptor.encryptWithKey(
817-
key,
818-
serializedKeyrings,
819-
);
820-
vaultJSON.salt = encryptionSalt;
821-
vault = JSON.stringify(vaultJSON);
822822
}
823823
} else {
824824
if (typeof this.password !== 'string') {
@@ -930,9 +930,9 @@ class KeyringController extends EventEmitter {
930930

931931
if (
932932
this.password &&
933-
this.#encryptor.updateVault &&
934-
(await this.#encryptor.updateVault(encryptedVault, this.password)) !==
935-
encryptedVault
933+
(!this.#cacheEncryptionKey || !encryptionKey) &&
934+
this.#encryptor.isVaultUpdated &&
935+
!this.#encryptor.isVaultUpdated(encryptedVault)
936936
) {
937937
// Re-encrypt the vault with safer method if one is available
938938
await this.persistAllKeyrings();

‎src/test/encryptor.mock.ts

+4
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,10 @@ export class MockEncryptor implements ExportableKeyEncryptor {
8181
return _vault;
8282
}
8383

84+
isVaultUpdated(_vault: string) {
85+
return true;
86+
}
87+
8488
generateSalt() {
8589
return MOCK_ENCRYPTION_SALT;
8690
}

‎src/types.ts

+9-5
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import type {
22
DetailedDecryptResult,
33
DetailedEncryptionResult,
44
EncryptionResult,
5+
KeyDerivationOptions,
56
} from '@metamask/browser-passworder';
67
import type { Json, Keyring } from '@metamask/utils';
78

@@ -63,14 +64,17 @@ export type GenericEncryptor = {
6364
*/
6465
decrypt: (password: string, encryptedString: string) => Promise<unknown>;
6566
/**
66-
* Optional vault migration helper. Updates the provided vault, re-encrypting
67-
* data with a safer algorithm if one is available.
67+
* Optional vault migration helper. Checks if the provided vault is up to date
68+
* with the desired encryption algorithm.
6869
*
69-
* @param vault - The encrypted string to update.
70-
* @param password - The password to decrypt the vault with.
70+
* @param vault - The encrypted string to check.
71+
* @param targetDerivationParams - The desired target derivation params.
7172
* @returns The updated encrypted string.
7273
*/
73-
updateVault?: (vault: string, password: string) => Promise<string>;
74+
isVaultUpdated?: (
75+
vault: string,
76+
targetDerivationParams?: KeyDerivationOptions,
77+
) => boolean;
7478
};
7579

7680
/**

0 commit comments

Comments
 (0)