-
-
Notifications
You must be signed in to change notification settings - Fork 133
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
Keep User Logged In: Export key for encrypted key login #152
Conversation
189f85b
to
12f4526
Compare
How will this work with mobile? Mobile's encryptor doesn't have any equivalent function to |
index.js
Outdated
|
||
if (this.password) { | ||
const { vault: newVault, exportedKeyString } = | ||
await this.encryptor.encryptWithDetail( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative approach (which should not require API changes to browser-passworder
) would be to generate the key itself here (if it does not exist) and use the existing encryptWithKeyMethod
. That would reduce the code complexity and reduce the number of potential states.
Was this considered, and if so, why is it not preferred?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This package has to work across both mobile and extension, and they use entirely different encryption methods. Mobile uses a native AES API, whereas extension uses the web crypto API. This is managed by keeping all cryptography in the "encryptor" instance, which is injected into this class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you elaborate on how that would reduce the number of potential states? I don't follow that part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Gudahtt I was loosely referring to branching complexity. Inlining the logic in encryptWithDetail
could reduce the call sites into encryptor
, parallel logic, and code surface area. Consider:
___
_______/ \______________
\___/
vs
_____ ________
____/ \__/ \____
\_____/ \________/
Conversely for decrypt.
Does that make sense?
It's not a large number of codes we're talking about here, but when juggling passphrases and exposed private keys around it's prudent to be more picky IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see. I don't think it would reduce branching complexity overall, but it would shift some steps from that package into this one.
Either way, we will have to persist the keyring state with a password and no key, or with a key and no password. And the same goes for unlock: sometimes we'll have a password but no key, and sometimes we'll have a key but no password. That's two paths each, for unlock and persist.
Handling mobile as well is more complex, as their encryptor doesn't yet support returning a key (it has no immediate need to support this). So that's an additional path for unlock and persist (where a password is supplied, but we don't want to ask for a key and cache it).
Even if we inline some of the encryptor changes here, we would still have those 6 branches, the same as we have now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could eliminate two of those branches by updating the mobile encryptor to also support encryptWithDetail
.
But it's unclear to me how that would benefit mobile. I guess it would let them keep the key in-memory and let the garbage collector cleanup the password, which.... seems preferable but it's hard to argue it has any tangible benefit to security beyond allowing us to simplify some of this code. Postponing that change for later makes sense to me, especially given that a lot of things are waiting on this change (a lot of MV3 bugs surrounding the restart of the service worker are linked to this work).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we just wanted to reduce the API surface of the encryptor, that would be a bit easier to accomplish. We could update encrypt
and decrypt
to return an object, and add the details to that rather than adding the new encryptWithDetails
/decryptWithDetails
functions.
That'd make the extension encryptor act differently from the mobile encryptor for those methods, so we'd need to handle that difference or change the mobile one as well. But that wouldn't be too difficult either way. I expect we'll want to do this eventually.
index.js
Outdated
|
||
this.store.updateState({ vault }); | ||
|
||
// The keyring updates need to be announced before updating the exportedKeyStrings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the relationship between these two updates? It's not clear to me why this order is important.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grrrr. It's a weird extension issue. If I call this.memStore.updateState({ encryptionKey: newExportedKeyString });
before _updateMemStoreKeyrings
, the account is created in the keyring but somehow isn't being propagated to the extension before the Preferences controller tries to syncAddresses
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The visual side effect is that the new account wouldn't be selected in the extension -- Account 1 would be selected. When you open up the accounts menu, however, it's there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment could be covering the why insteadof/alongwith the what
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, could a test cover this to guarantee it's not undone?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I call
this.memStore.updateState({ encryptionKey: newExportedKeyString });
before_updateMemStoreKeyrings
, the account is created in the keyring but somehow isn't being propagated to the extension before the Preferences controller tries tosyncAddresses
.
Is this also why the calls to _updateMemStoreKeyrings
were removed in a number of places? I was curious about that as well.
Edit: Oh nevermind, I see now. This line is within persistAllKeyrings
, so it was removed from those other places to not be redundant with this addition. Sorry got mixed up for a moment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh this is pretty tricky. I think I follow though.
Whenever memStore
is updated, it'll trigger some other things to run (including syncAddresses
). By setting the encryption key here before calling _updateMemStoreKeyrings
, these state update get triggered with an outdated set of keyrings and associated accounts.
An update with stale keyrings/accounts is a problem because it has side-effects like choosing a new selected address in some cases. Whereas a stale (i.e. unset) encryption key is not a problem at all. It just gets set in the next update.
The |
index.js
Outdated
@@ -131,7 +141,7 @@ class KeyringController extends EventEmitter { | |||
throw new Error('KeyringController - First Account not found.'); | |||
} | |||
|
|||
await this.persistAllKeyrings(password); | |||
await this.persistAllKeyrings(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this need to be called twice? Looks like the initial call is redundant or there's not enough separation of concerns for addNewKeyring to work regardless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. I suspect that it was originally called here for the side-effect of setting this.password
. Now that we're setting this.password
sooner, this might not be required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
async persistAllKeyrings() { | ||
const { encryptionKey } = this.memStore.getState(); | ||
|
||
if (!this.password && !encryptionKey) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
} else { | ||
vault = await this.encryptor.decrypt(password, encryptedVault); | ||
this.password = password; | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments, but none of them are blockers.
Please consider addressing them before merging, but you have my approval already. This is Fiiiine!
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
}), | ||
|
||
decrypt(_password, _text) { | ||
encryptWithDetail: sinon.stub().callsFake(function (_password, dataObj) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be nice to have test coverage for a mobile encryptor as well. Maybe we can leave this as-is, and move these modifications to a new mock encryptor module? Or set two up here, and return them as named exports rather than as the default export.
Then we can ensure things blow up properly if cacheEncryptionKey
is enabled with an encryptor that doesn't support it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Converting this to a class might make that easier, as was done here: https://github.com/MetaMask/controllers/blob/main/tests/mocks/mockEncryptor.ts
Then we can extend the base encryptor to create the one with the browser-passworder additional methods.
test/index.js
Outdated
|
||
expect(keyringController.password).toBe(password); | ||
expect(keyringController.memStore.getState().encryptionSalt).toBe('SALT'); | ||
// eslint-disable-next-line jest/no-restricted-matchers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: In these cases where w want to ensure we have a non-empty string, we can use expect(...).toStrictEqual(expect.stringMatching('.+'))
. This is a bit more specific than the truthy check in that it ensures it's a string, and it avoids the need for this ignore comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent, I had no idea!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is one more place you could use this (on this line 😅 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Still a lint failure to resolve, but this all looks great and I can re-approve after that's fixed.
it('unlocks the keyrings with valid information', async function () { | ||
keyringController.cacheEncryptionKey = true; | ||
const returnValue = await keyringController.encryptor.decryptWithKey(); | ||
const stub = sinon.stub(keyringController.encryptor, 'decryptWithKey'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: IMO this would be a better test with this removed. The keyrings-based assertion is enough to show this works; this part just makes the test more fragile by tying it to internals.
This is an alternative to #147 , whereby we create an encryption key based off of the derived key of browser-passworder
Depends on: MetaMask/browser-passworder#20