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

Keep User Logged In: Export key for encrypted key login #152

Merged
merged 57 commits into from
Nov 8, 2022
Merged
Show file tree
Hide file tree
Changes from 50 commits
Commits
Show all changes
57 commits
Select commit Hold shift + click to select a range
c25efe0
Export key for encrypted key login
darkwing Sep 30, 2022
e239774
Progress
darkwing Oct 3, 2022
3c4e88d
Delete encryption data upon lock
darkwing Oct 4, 2022
eb8b357
Progress: Ensure addAccount and removeAccount work by allowing encryp…
darkwing Oct 5, 2022
aed7c3b
Ensure the user is properly authenticated with encryption key. Also,…
darkwing Oct 6, 2022
97ebd51
Progress: Tests
darkwing Oct 6, 2022
4d62780
Progress on tests
darkwing Oct 6, 2022
fb461b5
Don't lock upon password
darkwing Oct 7, 2022
1ff9e9d
Add test to ensure decryptWithEncryptedKeyString
darkwing Oct 7, 2022
b200969
Add test to ensure persistAllKeyrings is being called
darkwing Oct 10, 2022
52605f9
Add encryption key to memStore so it's accessible to extension. Also…
darkwing Oct 12, 2022
645ecd9
Add jsdoc for submitEncryptionKey, code cleanups
darkwing Oct 13, 2022
8e8e771
Avoid using encryptionData when all that matters is salt
darkwing Oct 13, 2022
6e840cc
Fix test
darkwing Oct 14, 2022
76d1adf
Ensure keyring is unlocked when new vault is created
darkwing Oct 14, 2022
ef8dc4f
Call _updateMemStoreKeyrings inside persistAllKeyrings
darkwing Oct 14, 2022
2dafa98
Add, improve tests
darkwing Oct 17, 2022
751fbfe
Remove persistAllKeyrings check which isn't necessary
darkwing Oct 17, 2022
db4dc1d
Remove unnecessary password arguments
darkwing Oct 17, 2022
e141b84
Remove useless return value
darkwing Oct 18, 2022
2a2e53b
Avoid breaking changes to browser-passworder
darkwing Oct 20, 2022
0ace5f5
submitEncryptionKey doesn't need to require encryptedData -- we alrea…
darkwing Oct 26, 2022
a5fe1dc
Remove usage of decryptWithEncryptedKeyString from encryptor
darkwing Oct 26, 2022
e28b476
Rename extractedKeyString to exportedKeyString
darkwing Oct 26, 2022
7cd4d40
Pin version of browser-passworder
darkwing Oct 27, 2022
dc73e95
Update build
darkwing Oct 31, 2022
a321458
Make use of encryption key optional
darkwing Nov 1, 2022
e594170
Remove unnecessary password setting
darkwing Nov 2, 2022
1093c17
Change to importKey
darkwing Nov 3, 2022
76c0123
Fix tests
darkwing Nov 3, 2022
72145ba
Update to latest browser-passworder
darkwing Nov 3, 2022
aedfbe1
Add workable dist for testing extension
darkwing Nov 4, 2022
46f39db
Add comment explaining memory store update
darkwing Nov 7, 2022
76aa90d
Update to v4.0.1 of browser-passworder
darkwing Nov 7, 2022
5ef3bb6
Lint fix
darkwing Nov 7, 2022
3694ce5
Remove unnecessary call to persistAllKeyrings
darkwing Nov 7, 2022
4769f37
Don't setLocked from submitPassword
darkwing Nov 7, 2022
a4e01d0
Revert unnecessary variable name changes
darkwing Nov 7, 2022
37af169
Restore _updateMemStoreKeyrings call to forgetDevice
darkwing Nov 7, 2022
4021e0c
Merge remote-tracking branch 'origin/main' into export-key
Gudahtt Nov 7, 2022
81cd50a
Remove redundant verifyPassword call
darkwing Nov 7, 2022
12d8365
Remove default value for `opts` with addNewKeyring
darkwing Nov 7, 2022
a7a7bc7
Remove unnecessary memstore keyring update
darkwing Nov 7, 2022
2d4b51d
Avoid parsing the encryptedVault twice
darkwing Nov 7, 2022
78412cb
Ensure encryptionKey is set, rename test set to cacheEncryptionKey
darkwing Nov 8, 2022
9a0343e
Add test coverage for attempting to persist without key or password
darkwing Nov 8, 2022
0071c04
Improve 'cleans up login artifacts upon lock' test to ensure encrypti…
darkwing Nov 8, 2022
7907b34
Lock keyringcontroller before submitting encryption key
darkwing Nov 8, 2022
9178496
Move encryptionSalt to memStore
darkwing Nov 8, 2022
88299ef
Rename newExportedKey to newEncryptionKey
darkwing Nov 8, 2022
3848e04
Validate freshness of encryption key by validating salt
darkwing Nov 8, 2022
c137eae
Use string equality matcher for expect
darkwing Nov 8, 2022
83e0973
Ensure the encryption key method is used in tests
darkwing Nov 8, 2022
a5fe207
await setLocked
darkwing Nov 8, 2022
aacd2a1
Fix missing string matcher
darkwing Nov 8, 2022
ad5169d
Fix lint
darkwing Nov 8, 2022
cc7ff86
Merge branch 'main' into export-key
darkwing Nov 8, 2022
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
136 changes: 119 additions & 17 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ const { EventEmitter } = require('events');
const { Buffer } = require('buffer');
const bip39 = require('@metamask/bip39');
const ObservableStore = require('obs-store');
const encryptor = require('browser-passworder');
const encryptor = require('@metamask/browser-passworder');
const { normalize: normalizeAddress } = require('eth-sig-util');

const SimpleKeyring = require('eth-simple-keyring');
Expand All @@ -14,6 +14,7 @@ const KEYRINGS_TYPE_MAP = {
HD_KEYRING: 'HD Key Tree',
SIMPLE_KEYRING: 'Simple Key Pair',
};

/**
* Strip the hex prefix from an address, if present
* @param {string} address - The address that might be hex prefixed.
Expand All @@ -40,12 +41,17 @@ class KeyringController extends EventEmitter {
this.store = new ObservableStore(initState);
this.memStore = new ObservableStore({
isUnlocked: false,
keyringTypes: this.keyringTypes.map((krt) => krt.type),
keyringTypes: this.keyringTypes.map((keyringType) => keyringType.type),
keyrings: [],
encryptionKey: null,
});

this.encryptor = opts.encryptor || encryptor;
this.keyrings = [];

// This option allows the controller to cache an exported key
// for use in decrypting and encrypting data without password
this.cacheEncryptionKey = Boolean(opts.cacheEncryptionKey);
}

/**
Expand Down Expand Up @@ -143,9 +149,15 @@ class KeyringController extends EventEmitter {
* @returns {Promise<Object>} A Promise that resolves to the state.
*/
async setLocked() {
delete this.password;

// set locked
this.password = null;
this.memStore.updateState({ isUnlocked: false });
this.memStore.updateState({
isUnlocked: false,
encryptionKey: null,
encryptionSalt: null,
});

// remove keyrings
this.keyrings = [];
await this._updateMemStoreKeyrings();
Expand All @@ -168,6 +180,24 @@ class KeyringController extends EventEmitter {
*/
async submitPassword(password) {
this.keyrings = await this.unlockKeyrings(password);

this.setUnlocked();
this.fullUpdate();
}

/**
* Submit Encryption Key
*
* Attempts to decrypt the current vault and load its keyrings
* into memory based on the vault and CryptoKey information
*
* @emits KeyringController#unlock
* @param {string} encryptionKey - The encrypted key information used to decrypt the vault
* @param {string} loginData - The vault data to decrypt
* @returns {Promise<Object>} A Promise that resolves to the state.
*/
async submitEncryptionKey(encryptionKey) {
this.keyrings = await this.unlockKeyrings(undefined, encryptionKey);
this.setUnlocked();
this.fullUpdate();
}
Expand Down Expand Up @@ -215,7 +245,6 @@ class KeyringController extends EventEmitter {
this.keyrings.push(keyring);
await this.persistAllKeyrings();

await this._updateMemStoreKeyrings();
this.fullUpdate();

return keyring;
Expand Down Expand Up @@ -297,7 +326,6 @@ class KeyringController extends EventEmitter {
});

await this.persistAllKeyrings();
await this._updateMemStoreKeyrings();
this.fullUpdate();
}

Expand Down Expand Up @@ -347,7 +375,6 @@ class KeyringController extends EventEmitter {
}

await this.persistAllKeyrings();
await this._updateMemStoreKeyrings();
this.fullUpdate();
}

Expand Down Expand Up @@ -513,6 +540,14 @@ class KeyringController extends EventEmitter {
* @returns {Promise<boolean>} Resolves to true once keyrings are persisted.
*/
async persistAllKeyrings() {
Gudahtt marked this conversation as resolved.
Show resolved Hide resolved
const { encryptionKey, encryptionSalt } = this.memStore.getState();

if (!this.password && !encryptionKey) {
Copy link

Choose a reason for hiding this comment

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

What's the difference between holding something in memory store vs class field? Why do we use/need both?

Copy link
Contributor Author

@darkwing darkwing Nov 7, 2022

Choose a reason for hiding this comment

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

Putting the encryptionKey in the memory store bubbles it up to the extension, which allows us to store it in session storage each time it changes: https://github.com/MetaMask/metamask-extension/pull/15558/files#diff-6fbff2cfe97ac01b77296ef2122c7e0a5b3ff6a84b584b4d1a87482f35eea3d6R3941

Copy link
Member

@Gudahtt Gudahtt Nov 7, 2022

Choose a reason for hiding this comment

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

We don't generally have a use for public class fields; the password field should be private. That's a change I was going to make in a separate PR at some point; it would be a breaking change because @metamask/controllers uses this field today, but it does not need to.

throw new Error(
'Cannot persist vault without password and encryption key',
legobeat marked this conversation as resolved.
Show resolved Hide resolved
darkwing marked this conversation as resolved.
Show resolved Hide resolved
);
}

const serializedKeyrings = await Promise.all(
this.keyrings.map(async (keyring) => {
const [type, data] = await Promise.all([
Expand All @@ -522,11 +557,48 @@ class KeyringController extends EventEmitter {
return { type, data };
}),
);
const encryptedString = await this.encryptor.encrypt(
this.password,
serializedKeyrings,
);
this.store.updateState({ vault: encryptedString });

let vault;
let newEncryptionKey;

if (this.cacheEncryptionKey) {
if (this.password) {
const { vault: newVault, exportedKeyString } =
await this.encryptor.encryptWithDetail(
this.password,
serializedKeyrings,
);

vault = newVault;
newEncryptionKey = exportedKeyString;
} else if (encryptionKey) {
darkwing marked this conversation as resolved.
Show resolved Hide resolved
const key = await this.encryptor.importKey(encryptionKey);
const vaultJSON = await this.encryptor.encryptWithKey(
key,
serializedKeyrings,
);
vaultJSON.salt = encryptionSalt;
vault = JSON.stringify(vaultJSON);
}
} else {
vault = await this.encryptor.encrypt(this.password, serializedKeyrings);
}

if (!vault) {
throw new Error('Cannot persist vault without vault information');
}

this.store.updateState({ vault });

// The keyring updates need to be announced before updating the encryptionKey
// so that the updated keyring gets propagated to the extension first.
// Not calling _updateMemStoreKeyrings results in the wrong account being selected
// in the extension.
await this._updateMemStoreKeyrings();
darkwing marked this conversation as resolved.
Show resolved Hide resolved
if (newEncryptionKey) {
this.memStore.updateState({ encryptionKey: newEncryptionKey });
}

return true;
}

Expand All @@ -539,15 +611,46 @@ class KeyringController extends EventEmitter {
* @param {string} password - The keyring controller password.
* @returns {Promise<Array<Keyring>>} The keyrings.
*/
async unlockKeyrings(password) {
async unlockKeyrings(password, encryptionKey) {
const encryptedVault = this.store.getState().vault;
if (!encryptedVault) {
throw new Error('Cannot unlock without a previous vault.');
}

await this.clearKeyrings();
const vault = await this.encryptor.decrypt(password, encryptedVault);
this.password = password;

let vault;

if (this.cacheEncryptionKey) {
if (password) {
const result = await this.encryptor.decryptWithDetail(
password,
encryptedVault,
);
vault = result.vault;
this.password = password;

this.memStore.updateState({
encryptionKey: result.exportedKeyString,
encryptionSalt: result.salt,
});
} else {
const key = await this.encryptor.importKey(encryptionKey);
const parsedEncryptedVault = JSON.parse(encryptedVault);
vault = await this.encryptor.decryptWithKey(key, parsedEncryptedVault);

// This call is required on the first call because encryptionKey
// is not yet inside the memStore
this.memStore.updateState({
encryptionKey,
encryptionSalt: parsedEncryptedVault.salt,
});
}
} else {
vault = await this.encryptor.decrypt(password, encryptedVault);
this.password = password;
}
Copy link

Choose a reason for hiding this comment

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

(nit) This if structure looks similar across both functions. I'd be tempted to create a function that accepts state and descriptively named functions as reactions to certain states.


await Promise.all(vault.map(this._restoreKeyring.bind(this)));
await this._updateMemStoreKeyrings();
return this.keyrings;
Expand Down Expand Up @@ -602,7 +705,7 @@ class KeyringController extends EventEmitter {
* @returns {Keyring|undefined} The class, if it exists.
*/
getKeyringClassForType(type) {
return this.keyringTypes.find((kr) => kr.type === type);
return this.keyringTypes.find((keyring) => keyring.type === type);
}

/**
Expand Down Expand Up @@ -744,7 +847,6 @@ class KeyringController extends EventEmitter {
if (keyring.forgetDevice) {
keyring.forgetDevice();
this.persistAllKeyrings();
this._updateMemStoreKeyrings.bind(this)();
} else {
throw new Error(
`KeyringController - keyring does not have method "forgetDevice", keyring type: ${keyring.type}`,
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@
},
"dependencies": {
"@metamask/bip39": "^4.0.0",
"@metamask/browser-passworder": "^4.0.1",
"@metamask/eth-hd-keyring": "^4.0.2",
"browser-passworder": "^2.0.3",
"eth-sig-util": "^3.0.1",
"eth-simple-keyring": "^4.2.0",
"obs-store": "^4.0.3"
Expand Down
Loading