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.
| 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 😄
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