refactor!: drop uncached encryption support #5963
Conversation
mikesposito
force-pushed
the
mikesposito/refactor/remove-uncached-encryption
branch
from
ba3626f to
cdddc1c
Compare
mikesposito
force-pushed
the
mikesposito/refactor/remove-uncached-encryption
branch
from
cdddc1c to
a11b2a3
Compare
|
@matthiasgeihs this PR doesn't allow encryption key injection, but rather removes |
mikesposito
force-pushed
the
mikesposito/refactor/remove-uncached-encryption
branch
from
a11b2a3 to
cf465ce
Compare
|
@metamaskbot publish-previews |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
mikesposito
force-pushed
the
mikesposito/refactor/remove-uncached-encryption
branch
from
e73e808 to
27657e3
Compare
mikesposito
force-pushed
the
mikesposito/refactor/remove-uncached-encryption
branch
from
6112020 to
86b6397
Compare
|
@metamaskbot publish-previews |
| type CachedEncryptionKey = { | ||
| /** | ||
| * The exported encryption key string. | ||
| */ | ||
| exported: string; | ||
| /** | ||
| * The salt used to derive the encryption key. | ||
| */ | ||
| salt?: string; | ||
| }; |
There was a problem hiding this comment.
We can easily add encryptedEncryptionKey in #5940 as a property of this type, so we can ensure that they are kept in sync with each other and with the values in the state and vault by placing them in the same data structure.
|
The main problem I see here is that it risks delaying the seedless onboarding feature. I think we should apply these optimizations after the changes needed for seedless onboarding. Otherwise, I think these changes are worthwile looking into further. (btw: a refactor is a code change that doesn't change functionality. so technically, this is not a refactor.) |
|
@metamaskbot publish-previews |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
|
This can now be tested on extension using MetaMask/metamask-extension#33613 |
| if (typeof password !== 'string') { | ||
| throw new TypeError(KeyringControllerError.WrongPasswordType); | ||
| } | ||
| const { exportedEncryptionKey } = credentials; |
There was a problem hiding this comment.
Is it ok to assume that it will be defined in the runtime? Maybe we could use something like:
if ('password' in credentials) {
// ...
} else if ('exportedEncryptionKey' in credentials) {
// ...
} else {
// Handle error
}I don't have a strong opinion about this. It's a question, not a suggestion.
There was a problem hiding this comment.
The key derivation / addition could indeed fail (or the passed credentials invalid), but there is this check right after this if/else block:
const encryptionKey = this.#encryptionKey?.exported;
if (!encryptionKey) {
throw new Error(KeyringControllerError.MissingCredentials);
}I tried to keep the if/else block as simple as possible, but let me know if you think we should handle the error differently.
There was a problem hiding this comment.
Note also that if we include the check in the if/else block we'll still need to make TypeScript happy with the additional check I quoted in the previous comment, as this.#encryptionKey may still be undefined
There was a problem hiding this comment.
Though, we could make this prettier by moving the if/else block to another internal method that handles the credentials, moving complexity out of this method
There was a problem hiding this comment.
Indeed, it will be tested in the #useEncryptionKey method:
if (
typeof encryptionKey !== 'string' || // <-- exportedEncryptionKey
typeof encryptionSalt !== 'string'
) {
throw new TypeError(KeyringControllerError.WrongEncryptionKeyType);
}I find this a bit confusing because we validate the password directly, but delegate the validation of the exportedEncryptionKey .
That said, I'm okay with keeping it as is. I’d prefer to validate inputs in the public methods, but that’s probably outside the scope of this PR.
There was a problem hiding this comment.
I find this a bit confusing because we validate the
passworddirectly, but delegate the validation of theexportedEncryptionKey
Where do you see the validation of the password? I believe #unlockKeyrings does not validate the password type, it's delegated to the #deriveEncryptionKey method
There was a problem hiding this comment.
Sorry, I meant the if ('password' in credentials) { check.
There was a problem hiding this comment.
Most often we don't validate things that the type system tells us. The type system is guaranteeing the key is set in this case, due to the parameter types.
We have made exceptions before for APIs that we know are being used from JavaScript though, where the types won't protect us. And we can make exceptions for sensitive libraries as well, in case a TypeScript error manages to impact how this is called (e.g. something might get cast to any in a way that isn't obvious to the reader).
Agreed with doing that in a separate PR if we want to go that route though.
| const controller = new SeedlessOnboardingController< | ||
| EncryptionKey | webcrypto.CryptoKey, | ||
| KeyDerivationOptions | ||
| >({ |
There was a problem hiding this comment.
I'm surprised the type parameters did not get inferred here 🤔 Should be "inferable" from encryptor no?
There was a problem hiding this comment.
It usually works with functions, but TypeScript does not infer class type parameters depending on constructors params unless the generic type is explicitly specified when instantiating the object
So we need to specify the derivation options type in the same way we specify the key type
There was a problem hiding this comment.
Yes, but given that we were not providing the EncryptionKey type parameter explicitly before this PR, I thought it would have worked the same with a 2nd parameter 😅 anyway, I was just curious! Thanks.
There was a problem hiding this comment.
Actually EncryptionKey was already passed in explicitly, I only added the second one:
-export class SeedlessOnboardingController<EncryptionKey> extends BaseController<
+export class SeedlessOnboardingController<
+ EncryptionKey,
+ SupportedKeyDerivationOptions,
+> extends BaseController<There was a problem hiding this comment.
On this note, I'm wondering if we should add the same to KeyringController.. thoughts?
There was a problem hiding this comment.
It was not passed explicitly though? 🤔
- const controller = new SeedlessOnboardingController({Anyway, if that's required, that's ok, I was just curious 😄
if we should add the same to KeyringController.. thoughts?
Good question... I generally like to have type parameters, and right now we're using unknown on our encryptor 😅 So that does sound better to me yes and it would "align" with the SeedlessOnbordingController too, this way we can make sure that once we introduce the other encryption/decryption key, both types are aligned.
| const controller = new SeedlessOnboardingController< | ||
| EncryptionKey | webcrypto.CryptoKey, | ||
| KeyDerivationOptions | ||
| >({ |
| state.encryptionSalt = updatedState.encryptionSalt; | ||
| } | ||
| state.encryptionKey = encryptionKey; | ||
| state.encryptionSalt = parsedEncryptedVault.salt; |
There was a problem hiding this comment.
What about adding a const encryptionSalt = this.#encryptionKey?.salt; above and use it when checking for throw new Error(KeyringControllerError.MissingCredentials); and use it here like:
| state.encryptionSalt = parsedEncryptedVault.salt; | |
| state.encryptionSalt = encryptionSalt; |
I just think that the salt and the encryption key have to be "coupled" together here even though yes, both salts would be the same here.
There was a problem hiding this comment.
We could check if the salt is matching the vault, but it's not strictly necessary: the actual decryption can still happen because we only need the encryption key to decrypt the vault, and if the salt is out of date we can simply update it at the end of the decryption (when we are sure that the encryption key is the correct one) - this way we implicitly keep them in sync, but we don't block decryption if it's not necessary.
There was a problem hiding this comment.
Now that I'm thinking about this, there probably is one counterargument to trying the encryption key directly: comparing the salt is faster than decrypting with the wrong key
There was a problem hiding this comment.
Now that I'm thinking about this, there probably is one counterargument to trying the encryption key directly: comparing the salt is faster than decrypting with the wrong key
There was a problem hiding this comment.
it's not strictly necessary
Yes, that one was of the reason I wanted to use the salt "associated" with the key, cause I think this makes a bit more sense when you read the code. But again, at this point they should both be equal
comparing the salt is faster than decrypting with the wrong key
True, but IDK if comparing salt is a common practice in that scenario. I'd just the code as it is for this, it looks great IMO.
but we don't block decryption if it's not necessary.
I don't see why using the encryptionSalt (from this.#encryptionKey?.salt) would block anything though? 🤔 I mean, as you said, we can just decrypt the vault with just the key, so updating the this state.encryptionSalt using the key's salt here seems a bit more natural to me. But if you prefer to keep it as-is, I'm fine too 😄
There was a problem hiding this comment.
We added the salt and key to controller state to support the caching the key in-memory so that we could keep the wallet unlocked in the event that the service worker process restarted. The salt was used to verify that the key matches the vault.
Comparing salts is better than attempting to decrypt, not just because it saves time. It also provides clarity why it failed. If the salt is mismatched, we know that it was an expired key, so we can ignore it (the scenario is identifiable by a unique error, KeyringControllerError.ExpiredCredentials). There can be other reasons for decryption failing apart from that (e.g. a bug that we introduced, or an incorrect password)
We don't use this "cache encryption key in memory" feature anymore, but if we want to remove it, I'd recommend doing that separately from the change in this PR (dropping uncached encryption key support). And we should remove encryptionKey and encryptionSalt from state if we want to do that as well, since that was the only purpose of it being in state.
| /** | ||
| * The exported encryption key string. | ||
| */ | ||
| exported: string; |
There was a problem hiding this comment.
IMO we could find a more suitable name than exported here. I feel like we key could work no? We could change the doc to match what we expect (even if type-wise we should be ok already)
| /** | |
| * The exported encryption key string. | |
| */ | |
| exported: string; | |
| /** | |
| * The encryption key (exported as a string). | |
| */ | |
| key: string; |
We could also go with keyValue but I find it less elegant.
Also, it's more of a personal opinion, feel free to disagree and resolve this suggestion if you prefer the current naming. 😄
There was a problem hiding this comment.
I wanted to make a clear distinction between an exported key and the actual encryption key, as in the rest of the code and API is very confusing (and I also found confusing to call importKey(encryptionKey.key)
The exported key is a stringified JSON representation of an encryption key, which instead is usually an instance of whatever the cipher supports (CryptoKey, SubtleCryptoKey, etc.).
Though as you said, personal opinion - I'm fine with changing it to key or else if you prefer. 😄
There was a problem hiding this comment.
The exported key is a stringified JSON representation of an encryption key, which instead is usually an instance of whatever the cipher supports (CryptoKey, SubtleCryptoKey, etc.).
Yes I got that, I just thought having key in the field name would have been slightly better.
But I do agree with you that this could also be confusing because of the "wrapping" of #encryptionKey.
So let's keep exported! 😄
There was a problem hiding this comment.
this actually confused me as well.
if you want the names to be different depending on whether the key is serialized or not, you could also named it serializedKey. However, I don't think it's necessary as it will be obvious from the type.
I think exported is not a good name because it suggests it's an exported object, but here it is an internal variable, so that's what's confusing me.
| * @returns Promise resolving when the operation completes. | ||
| */ | ||
| async submitEncryptionKey( | ||
| encryptionKey: string, | ||
| encryptionSalt: string, |
There was a problem hiding this comment.
The salt parameter serves an important role still in identifying whether the key is expired. We should keep it, so long as we support caching the key in this way (see here for details: https://github.com/MetaMask/core/pull/5963/files#r2158856115)
| * | ||
| * @param password - The password to use for decryption or derivation. | ||
| * @param options - Options for the key derivation. | ||
| * @param options.ignoreVaultKeyMetadata - Whether to ignore the vault key metadata |
There was a problem hiding this comment.
Nit: Not important, but, usually it makes code a bit easier to read if we avoid double-negatives. We could make this option useVaultKeyMetadata instead of ignore. Then we're either using it, or not. Rather than now, where we are either not using it, or not not using it.
There was a problem hiding this comment.
Good point. I'll change this as you suggested.
Though, on this note, I think I'll have to revisit how this keyMetadata is managed: I don't think we can reliably manage it at KeyringController level, because it strictly depends on the encryptor used
I think that ideally, encryptors should expose a method to derive a key from a password, but using salt and metadata from an encrypted vault - so that KeyringController doesn't need to know that. It would also mean that I need to come up with another way to update the metadata when needed though
| key, | ||
| serializedKeyrings, | ||
| ); | ||
| if (this.#encryptionKey.salt) { |
There was a problem hiding this comment.
Do we need a condition here - is it possible for this to be unset at this point?
| this.#password = currentPassword; | ||
| // Keyrings and encryption credentials are restored to their previous state | ||
| if (currentEncryptionKey && currentEncryptionSalt) { | ||
| this.#useEncryptionKey(currentEncryptionKey, currentEncryptionSalt); |
There was a problem hiding this comment.
Nit: Perhaps we could store this.#encryptionKey as currentEncryptionKey rather than storing the exported key and salt separately? And we could also update #useEncryptionKey to accept that object directly here, rather than separate positional key and salt parameters.
That might make it more obvious that they're expected to always be in-sync, you know. Reading this if condition, my first thought was "what if currentEncryptionKey was set but currentEncryptionSalt was not", then it wouldn't get reset property here. But using the object instead of the two values separately would eliminate that branch and that possibility.
There was a problem hiding this comment.
i think overall this is a good change.
it streamlines the way of how vault updates work, and puts key derivation in a separate, dedicated place. (before it was part of #updateVault.) (i think the PR could even be named streamline key derivation and drop uncached encryption, to emphasize that his is not just about dropping a feature, but about streamlining the existing design.)
left a couple of notes. not (yet) familiar enough with the design of the keyring controller that i would want to make the ultimate call here.
| branches: 94.01, | ||
| functions: 100, | ||
| lines: 98.79, | ||
| statements: 98.8, | ||
| lines: 98.76, | ||
| statements: 98.77, |
There was a problem hiding this comment.
why is coverage decreasing?
| @@ -64,6 +65,8 @@ const uint8ArraySeed = new Uint8Array( | |||
| seedWords.split(' ').map((word) => wordlist.indexOf(word)), | |||
| ).buffer, | |||
| ); | |||
| const mockVault = | |||
There was a problem hiding this comment.
hmm, looks like this is from my mock encryptor. might be conflicting on rebase.
| }, | ||
| }, | ||
| async ({ controller, encryptor }) => { | ||
| jest.spyOn(encryptor, 'decrypt').mockResolvedValueOnce([ | ||
| jest.spyOn(encryptor, 'decryptWithKey').mockResolvedValueOnce([ |
There was a problem hiding this comment.
unrelated change? revert?
(same comment applies to other instances of this change)
There was a problem hiding this comment.
i think it would be good to rebase on top of new mock encryptor because it might incur some changes
There was a problem hiding this comment.
there is still several reference to cacheEncryptionKey. i suggest searching through the code and making sure that the relevant occurrences are resolved.
| /** | ||
| * State/data that can be updated during a `withKeyring` operation. | ||
| */ | ||
| type SessionState = { | ||
| keyrings: SerializedKeyring[]; | ||
| password?: string; | ||
| encryptionKey?: CachedEncryptionKey; |
There was a problem hiding this comment.
previously, the encryption key was only updated within #updateVault.
while SessionState was used to check if #updateVault should be called.
how does it work now?
| * @param encryptionKey - The encryption key to use. | ||
| * @param encryptionSalt - The salt to use for the encryption key. | ||
| */ | ||
| #useEncryptionKey(encryptionKey: string, encryptionSalt: string): void { |
There was a problem hiding this comment.
| #useEncryptionKey(encryptionKey: string, encryptionSalt: string): void { | |
| #setEncryptionKey(encryptionKey: string, keyDerivationSalt: string): void { |
since we are redesigning the controller, maybe we can redefine the terminology that we are using.
for me encryptionSalt is a misnomer, because the salt has nothing to do with the encryption procedure itself, but with the key derivation procedure. i'd suggest to use keyDerivationSalt or something like that.
also i think setEncryptionKey is clearer than useEncryptionKey. or, if you wanna be more specific, setEncryptionKeyAndSalt.
| * @returns Promise resolving when the operation completes. | ||
| */ | ||
| async submitEncryptionKey( |
There was a problem hiding this comment.
if you drop the salt here, there is no reason to include it in the state.
the only reason, as far as i could see, for it being there was to avoid the race condition that @Gudahtt pointed out.
to be fair, however, i don't see submitEncryptionKey with salt being used anywhere so far?
| password: string; | ||
| } | ||
| | { | ||
| exportedEncryptionKey: string; |
There was a problem hiding this comment.
| exportedEncryptionKey: string; | |
| encryptionKey: string; |
?
| /** | ||
| * The exported encryption key string. | ||
| */ | ||
| exported: string; |
There was a problem hiding this comment.
this actually confused me as well.
if you want the names to be different depending on whether the key is serialized or not, you could also named it serializedKey. However, I don't think it's necessary as it will be obvious from the type.
I think exported is not a good name because it suggests it's an exported object, but here it is an internal variable, so that's what's confusing me.
Explanation
In preparation for #5940, this PR drops support for uncached encryption. Previously,
KeyringControlleraccepted acacheEncryptionKeyoption that allowed the encryption key to be stored in memory and used during encryption/decryption directly as opposed to using a password. ThecacheEncryptionKeyoption is being removed, and the encryption key is now always derived and cached when the password is provided.This change allows to simplify
#unlockKeyringsand#updateVaultmethods, and remove all the logic and tests related tocacheEncryptionKey. This also allows to removethis.#password, that has been replaced bythis.#encryptionKey.The
this.#encryptionKeyassignment logic has been moved to two new internal methods with these specific responsibilities:#deriveEncryptionKey(string): Derives the encryption key from the password, to be used during password login and password change.#useEncryptionKey(string, string): Uses an existing encryption key to be used directly, to be used bysubmitEncryptionKeymainly.With the upcoming changes in #5940, this allows to change the encryption key to use (i.e. by calling the aformentioned new internal methods) without having to deal with logic related to vault unlock/update, and code branches related to password-based encryption and key caching.
This PR can be tested on extension with the following: MetaMask/metamask-extension#33613
References
Changelog
Checklist