Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Prefer cached encryptionKey for encryption when possible #307

Merged
merged 2 commits into from
Nov 23, 2023
Merged

Prefer cached encryptionKey for encryption when possible #307

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2e64041
fix: prefer encryptionKey for encryption when possible
mikesposito Nov 23, 2023
fe45b61
refactor: add test case
mikesposito Nov 23, 2023
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
2 changes: 1 addition & 1 deletion jest.config.js
View file
Open in desktop
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
264 changes: 171 additions & 93 deletions src/KeyringController.test.ts
View file
Open in desktop
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);
Copy link
Member

@Gudahtt Gudahtt Nov 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Ah I see; this method is needed because this kinda isn't a real use-case. Or it is, but only as part of the initial unlock/vault creation, it's never a use case when calling persistAllKeyrings directly.

Makes sense. Seems like a bit of a code smell that this condition is possible only when this external method is called in a certain way internally, but that problem will go away soon hopefully when we merge this into core.

Copy link
Member

Choose a reason for hiding this comment

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

A nicer way to test this would be to test it as part of createNewVaultAndKeychain (and the other methods that trigger this condition), but given that this is to be deleted soon I don't think it'd be worth the effort 😅


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,92 @@ 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`, () => {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Is there a test case for when cacheEncryptionKey = true and encryptionKey is set?

Copy link
Member Author

@mikesposito mikesposito Nov 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Added in fe45b61!

The test checks that the vault is not being updated

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 = true and encryptionKey is set`, () => {
it('should not 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"}';
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);
});
});

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
Open in desktop
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) &&
Copy link
Member Author

Choose a reason for hiding this comment

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

This ensures that we don't do the whole persistAllKeyrings operation for nothing, as the vault will not get updated if we reuse the cached encryption key

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