Skip to content

Commit

Permalink
fix: prefer encryptionKey for encryption when possible
Browse files Browse the repository at this point in the history
  • Loading branch information
@mikesposito
mikesposito committed Nov 23, 2023
1 parent 67ae96c commit 2e64041
Show file tree
Hide file tree
Showing 3 changed files with 153 additions and 103 deletions.
2 changes: 1 addition & 1 deletion jest.config.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ module.exports = {
// An object that configures minimum threshold enforcement for coverage results
coverageThreshold: {
global: {
branches: 79.41,
branches: 79.8,
functions: 93.22,
lines: 91.5,
statements: 91.69,
Expand Down
235 changes: 142 additions & 93 deletions src/KeyringController.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,18 @@ async function initializeKeyringController({
return keyringController;
}

/**
* Delete the encryption key and salt from the `memStore` of the given keyring controller.
*
* @param keyringController - The keyring controller to delete the encryption key and salt from.
*/
function deleteEncryptionKeyAndSalt(keyringController: KeyringController) {
const keyringControllerState = keyringController.memStore.getState();
delete keyringControllerState.encryptionKey;
delete keyringControllerState.encryptionSalt;
keyringController.memStore.updateState(keyringControllerState);
}

describe('KeyringController', () => {
afterEach(() => {
sinon.restore();
Expand Down Expand Up @@ -199,83 +211,86 @@ describe('KeyringController', () => {
});

describe('when `cacheEncryptionKey` is enabled', () => {
it('should save an up to date encryption salt to the `memStore` when `password` is unset and `encryptionKey` is set', async () => {
const keyringController = await initializeKeyringController({
password: PASSWORD,
constructorOptions: {
cacheEncryptionKey: true,
},
describe('when `encryptionKey` is set', () => {
it('should save an up to date encryption salt to the `memStore`', async () => {
const keyringController = await initializeKeyringController({
password: PASSWORD,
constructorOptions: {
cacheEncryptionKey: true,
},
});
const vaultEncryptionKey = '🔑';
const vaultEncryptionSalt = '🧂';
const vault = JSON.stringify({ salt: vaultEncryptionSalt });
keyringController.store.updateState({ vault });

await keyringController.unlockKeyrings(
undefined,
vaultEncryptionKey,
vaultEncryptionSalt,
);

expect(keyringController.memStore.getState().encryptionKey).toBe(
vaultEncryptionKey,
);
expect(keyringController.memStore.getState().encryptionSalt).toBe(
vaultEncryptionSalt,
);

const response = await keyringController.persistAllKeyrings();

expect(response).toBe(true);
expect(keyringController.memStore.getState().encryptionKey).toBe(
vaultEncryptionKey,
);
expect(keyringController.memStore.getState().encryptionSalt).toBe(
vaultEncryptionSalt,
);
});
delete keyringController.password;
const vaultEncryptionKey = '🔑';
const vaultEncryptionSalt = '🧂';
const vault = JSON.stringify({ salt: vaultEncryptionSalt });
keyringController.store.updateState({ vault });

await keyringController.unlockKeyrings(
undefined,
vaultEncryptionKey,
vaultEncryptionSalt,
);

expect(keyringController.memStore.getState().encryptionKey).toBe(
vaultEncryptionKey,
);
expect(keyringController.memStore.getState().encryptionSalt).toBe(
vaultEncryptionSalt,
);

const response = await keyringController.persistAllKeyrings();

expect(response).toBe(true);
expect(keyringController.memStore.getState().encryptionKey).toBe(
vaultEncryptionKey,
);
expect(keyringController.memStore.getState().encryptionSalt).toBe(
vaultEncryptionSalt,
);
});

it('should save an up to date encryption salt to the `memStore` when `password` is set through `createNewVaultAndKeychain`', async () => {
const keyringController = await initializeKeyringController({
password: PASSWORD,
constructorOptions: {
cacheEncryptionKey: true,
},
describe('when `encryptionKey` is not set and `password` is set', () => {
it('should save an up to date encryption salt to the `memStore` when `password` is set through `createNewVaultAndKeychain`', async () => {
const keyringController = await initializeKeyringController({
password: PASSWORD,
constructorOptions: {
cacheEncryptionKey: true,
},
});
await keyringController.createNewVaultAndKeychain(PASSWORD);
deleteEncryptionKeyAndSalt(keyringController);

const response = await keyringController.persistAllKeyrings();

expect(response).toBe(true);
expect(keyringController.memStore.getState().encryptionKey).toBe(
MOCK_HARDCODED_KEY,
);
expect(keyringController.memStore.getState().encryptionSalt).toBe(
MOCK_ENCRYPTION_SALT,
);
});

await keyringController.createNewVaultAndKeychain(PASSWORD);

const response = await keyringController.persistAllKeyrings();

expect(response).toBe(true);
expect(keyringController.memStore.getState().encryptionKey).toBe(
MOCK_HARDCODED_KEY,
);
expect(keyringController.memStore.getState().encryptionSalt).toBe(
MOCK_ENCRYPTION_SALT,
);
});

it('should save an up to date encryption salt to the `memStore` when `password` is set through `submitPassword`', async () => {
const keyringController = await initializeKeyringController({
password: PASSWORD,
constructorOptions: {
cacheEncryptionKey: true,
},
it('should save an up to date encryption salt to the `memStore` when `password` is set through `submitPassword`', async () => {
const keyringController = await initializeKeyringController({
password: PASSWORD,
constructorOptions: {
cacheEncryptionKey: true,
},
});
await keyringController.submitPassword(PASSWORD);
deleteEncryptionKeyAndSalt(keyringController);

const response = await keyringController.persistAllKeyrings();

expect(response).toBe(true);
expect(keyringController.memStore.getState().encryptionKey).toBe(
MOCK_HARDCODED_KEY,
);
expect(keyringController.memStore.getState().encryptionSalt).toBe(
MOCK_ENCRYPTION_SALT,
);
});

await keyringController.submitPassword(PASSWORD);

const response = await keyringController.persistAllKeyrings();

expect(response).toBe(true);
expect(keyringController.memStore.getState().encryptionKey).toBe(
MOCK_HARDCODED_KEY,
);
expect(keyringController.memStore.getState().encryptionSalt).toBe(
MOCK_ENCRYPTION_SALT,
);
});
});
});
Expand Down Expand Up @@ -836,29 +851,63 @@ describe('KeyringController', () => {
});

describe('with old vault format', () => {
[true, false].forEach((cacheEncryptionKey) => {
describe(`with cacheEncryptionKey = ${cacheEncryptionKey}`, () => {
it('should update the vault', async () => {
const mockEncryptor = new MockEncryptor();
const keyringController = await initializeKeyringController({
password: PASSWORD,
constructorOptions: {
cacheEncryptionKey: true,
encryptor: mockEncryptor,
},
});
const initialVault = keyringController.store.getState().vault;
const updatedVaultMock =
'{"vault": "updated_vault_detail", "salt": "salt"}';
sinon.stub(mockEncryptor, 'updateVault').resolves(updatedVaultMock);
sinon.stub(mockEncryptor, 'encrypt').resolves(updatedVaultMock);

await keyringController.unlockKeyrings(PASSWORD);
const updatedVault = keyringController.store.getState().vault;

expect(initialVault).not.toBe(updatedVault);
expect(updatedVault).toBe(updatedVaultMock);
describe(`with cacheEncryptionKey = true and encryptionKey is unset`, () => {
it('should update the vault', async () => {
const mockEncryptor = new MockEncryptor();
const keyringController = await initializeKeyringController({
password: PASSWORD,
constructorOptions: {
cacheEncryptionKey: true,
encryptor: mockEncryptor,
},
});
deleteEncryptionKeyAndSalt(keyringController);
const initialVault = keyringController.store.getState().vault;
const updatedVaultMock =
'{"vault": "updated_vault_detail", "salt": "salt"}';
const mockEncryptionResult = {
data: '0x1234',
iv: 'an iv',
};
sinon.stub(mockEncryptor, 'updateVault').resolves(updatedVaultMock);
sinon
.stub(mockEncryptor, 'encryptWithKey')
.resolves(mockEncryptionResult);

await keyringController.unlockKeyrings(PASSWORD);
const updatedVault = keyringController.store.getState().vault;

expect(initialVault).not.toBe(updatedVault);
expect(updatedVault).toBe(
JSON.stringify({
...mockEncryptionResult,
salt: MOCK_ENCRYPTION_SALT,
}),
);
});
});

describe(`with cacheEncryptionKey = false`, () => {
it('should update the vault', async () => {
const mockEncryptor = new MockEncryptor();
const keyringController = await initializeKeyringController({
password: PASSWORD,
constructorOptions: {
cacheEncryptionKey: false,
encryptor: mockEncryptor,
},
});
const initialVault = keyringController.store.getState().vault;
const updatedVaultMock =
'{"vault": "updated_vault_detail", "salt": "salt"}';
sinon.stub(mockEncryptor, 'updateVault').resolves(updatedVaultMock);
sinon.stub(mockEncryptor, 'encrypt').resolves(updatedVaultMock);

await keyringController.unlockKeyrings(PASSWORD);
const updatedVault = keyringController.store.getState().vault;

expect(initialVault).not.toBe(updatedVault);
expect(updatedVault).toBe(updatedVaultMock);
});
});
});
Expand Down
19 changes: 10 additions & 9 deletions src/KeyringController.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -802,7 +802,15 @@ class KeyringController extends EventEmitter {
if (this.#cacheEncryptionKey) {
assertIsExportableKeyEncryptor(this.#encryptor);

if (this.password) {
if (encryptionKey) {
const key = await this.#encryptor.importKey(encryptionKey);
const vaultJSON = await this.#encryptor.encryptWithKey(
key,
serializedKeyrings,
);
vaultJSON.salt = encryptionSalt;
vault = JSON.stringify(vaultJSON);
} else if (this.password) {
const { vault: newVault, exportedKeyString } =
await this.#encryptor.encryptWithDetail(
this.password,
Expand All @@ -811,14 +819,6 @@ class KeyringController extends EventEmitter {

vault = newVault;
newEncryptionKey = exportedKeyString;
} else if (encryptionKey) {
const key = await this.#encryptor.importKey(encryptionKey);
const vaultJSON = await this.#encryptor.encryptWithKey(
key,
serializedKeyrings,
);
vaultJSON.salt = encryptionSalt;
vault = JSON.stringify(vaultJSON);
}
} else {
if (typeof this.password !== 'string') {
Expand Down Expand Up @@ -930,6 +930,7 @@ class KeyringController extends EventEmitter {

if (
this.password &&
(!this.#cacheEncryptionKey || !encryptionKey) &&
this.#encryptor.updateVault &&
(await this.#encryptor.updateVault(encryptedVault, this.password)) !==
encryptedVault
Expand Down

0 comments on commit 2e64041

Please sign in to comment.