vault envelope encryption #5940
Conversation
matthiasgeihs
force-pushed
the
feat/vault-envelope-encryption
branch
from
34b4c4b to
33e3bc6
Compare
matthiasgeihs
force-pushed
the
feat/vault-envelope-encryption
branch
from
33e3bc6 to
29fdd1c
Compare
matthiasgeihs
changed the title
matthiasgeihs
force-pushed
the
feat/keyring/mock-encryptor
branch
from
5b85b24 to
96b2144
Compare
matthiasgeihs
force-pushed
the
feat/vault-envelope-encryption
branch
from
29fdd1c to
4062230
Compare
matthiasgeihs
force-pushed
the
feat/keyring/mock-encryptor
branch
from
2c0dbd5 to
209c58d
Compare
matthiasgeihs
force-pushed
the
feat/vault-envelope-encryption
branch
from
4062230 to
774a3e7
Compare
| this.update((state) => { | ||
| state.encryptedEncryptionKey = currentEncryptedEncryptionKey; | ||
| }); |
There was a problem hiding this comment.
Errors thrown during an operation should only rollback changes to class-level variables outside of the controller state, as the state is always updated along with keyrings and vault at the end of each operation with #updateVault.
Is there any specific operation that you were considering when adding this update?
There was a problem hiding this comment.
Yes, I think this is one more thing that is not obvious and not well documented.
I understand now that it's expected that any state update happens "atomically" at the end of the #updateVault function. This is probably also why #password is cached. Probably have to do the same for #encryptedEncryptionKey.
How it currently works is that the state.encryptedEncryptionKey is updated before #updateVault is called. (Concretely, there are two places where this can happen: #createNewVaultWithKeyring and changePasswordAndEncryptionKey.)
This is why I needed to revert this state change manually. However, I understand now that this is not inline with the state update policy (i.e., that state is only updated altogether at the end of #updateVault).
Let me know if my understanding is correct.
There was a problem hiding this comment.
This is probably also why #password is cached. Probably have to do the same for #encryptedEncryptionKey.
Before cacheEncryptionKey was introduced, the #password variable was the only type of credential the controller used to use to encrypt/decrypt, while the cached encryption key was introduced later on (and only for some instances and types of encryptor).
As now both Mobile and Extension use a somewhat compatible encryptor in terms of types, and they both support key encryption export, we may be able to remove the password usage almost completely by deriving (and extracting from the envelope when enabled) the key once, and then caching it instead of the #password.
This would basically translate to removing this.#password, and having this.#encryptionKey instead: this would allow us to:
- Simplify
#updateVault()and#unlockKeyringsmethods, by removing most of password-related branches - Keep the same pattern we have for the rest of class-variables with the encryption key, by assigning it to the class variable instead of the state directly
Thoughts?
There was a problem hiding this comment.
did update the envelope encryption pattern, please check
| encryptedVault, | ||
| ); | ||
| this.#password = password; | ||
| updatedState.encryptionKey = key; |
There was a problem hiding this comment.
Should we also update the salt here?
There was a problem hiding this comment.
hmm I guess that to decrypt the vault with a password, the salt is necessarily reused from the ciphertext. Though I see that in @metamask/browser-passworder the salt is an optional key
There was a problem hiding this comment.
The handling of the salt in keyring controller is another thing that confused me quite a bit.
Note that the salt is not relevant for the decryption itself, just for the key derivation from the password to the encryption key. So it isn't actually relevant for the functions encryptWithKey and decryptWithKey.
To be honest, I think we should be able to remove the salt entirely from the state. I've checked the usages of this salt and we might actually not need it. Anyhow, this is a different issue.
So for your comment, I guess we can add it, but I'd prefer not to unless there is a good reason for it.
There was a problem hiding this comment.
Ah, I can see why this is confusing as its currently implemented. The salt was initially added for good reason though.
Essentially we use the salt to verify that the cached encryption key that we have was used for a specific encrypted vault. It's in support of this check specifically:
We began caching the encryption key in extension session storage in order to ensure that when the service worker process was restarted, the user wouldn't be forced to log back in. At the time, Google was forcing us to migrate to MV3, and MV3 required that the background service worker process would stay running for a maximum of 5 minutes. This was how we avoided forcing the user to log in at minimum once every 5 minutes.
But there was a race condition where the key would be stored before the vault, or the vault before the key, just before the process terminated, resulting in a mismatch. In either case, it would mean the cached key was not valid, so we'd need to force a real password login. Storing the key was how we detected this edge case so that we could force a login.
We don't use this cached key mode anymore because before the MV3 deadline was reached, Google changed their minds about the 5 minute lifespan thing. They're allowing us to keep the service worker alive for the duration of the browser session now, so this became a non-issue.
There was a problem hiding this comment.
thanks for the background!
There was a problem hiding this comment.
i think the handling of this race condition should be documented in the code.
is it still relevant?
There was a problem hiding this comment.
It's not relevant today.
The 5-minute max lifespan of the service worker was dropped, so we no longer actually backup the ecryption key in session storage. A few of the other mitigations we had for that were also removed. Today we can consider encryptionKey and encryptionSalt as effectively unused by the clients.
But there is a case for using them again in the future, if we want to intentionally let the service worker process die when the extension is idle, for performance reasons. I don't think it would be practical for us to pursue that in the near future, but it's on our list of things to consider.
I'd be happy to see them removed in the meantime - we can always bring them back. But that's why we've been hesitant to remove it thus far. Certainly they should be better documented if they are to remain.
| @@ -2093,6 +2180,8 @@ export class KeyringController extends BaseController< | |||
|
|
|||
| this.#password = password; | |||
|
|
|||
| await this.#updateEncryptedEncryptionKey(password, encryptionKey); | |||
There was a problem hiding this comment.
Possibly related to my comment regarding encryptedEncryptionKey rollback, here we are kinda breaking the pattern of updating relevant state data in a single place (#updateVault till now). In the case of the encrypted envelope, this becomes even more critical as we must ensure that vault and envelope are always updated together, and never out of sync.
Though I also understand why it was done, and I don't have yet a clear suggestion to change this - perhaps change the way we use this.#password since in envelope mode we don't really have a reason to cache it I guess
There was a problem hiding this comment.
as mentioned above.
this pattern was not obvious to me and frankly not documented very well.
i've only became aware of it yesterday evening when i realized that the rollback didn't work as expected.
i'll push an update that changes how the encryptedEncryptionKey is handled (similar to password).
There was a problem hiding this comment.
i think we could add a check in the rollback function that ensures that state is not altered. otherwise issues a warning or something.
There was a problem hiding this comment.
did update the envelope encryption pattern, please check
| ); | ||
| vault = result.vault; | ||
| this.#password = password; | ||
| if (encryptedEncryptionKey) { |
There was a problem hiding this comment.
I feel like we are adding an important implementation detail (or an additional operation mode even) without properly documenting somewhere. Thoughts on adding a method like #isEnvelopeModeEnabled() to encode some documentation on this controller functionality?
There was a problem hiding this comment.
this is already documented at any public facing function, right?
though we can add more documentation, sure.
i like adding an explicit function for that check 👍 will do that
|
We should probably throw an error in the constructor if |
matthiasgeihs
force-pushed
the
feat/vault-envelope-encryption
branch
from
98e1359 to
8d3f210
Compare
|
rebase |
matthiasgeihs
force-pushed
the
feat/vault-envelope-encryption
branch
from
8d3f210 to
a272c5e
Compare
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
| #encryptionKey?: string; | ||
|
|
There was a problem hiding this comment.
This should be added to the variables being rolled back in #withRollback, or we risk encrypting with stale credentials.
There was a problem hiding this comment.
done in 59667b8
| importedKey, | ||
| serializedKeyrings, | ||
| ); | ||
| updatedState.vault = JSON.stringify(vaultJSON); |
There was a problem hiding this comment.
I noticed that this vault seems to be missing a salt. But maybe that's OK, because we only decrypt this with the key-derived encryption key? Rather than with the password-derived key.
This is worth at least calling out in a comment. If we somehow end up trying to decrypt this with a password, it will result in a confusing error.
There was a problem hiding this comment.
yes, i think this was pointed out before by michele.
i prefer not to add a salt unless there is a good reason to.
i couldn't see why it would be needed, hence i didn't add it.
is my understanding correct? or do we need to add it here?
(do note that the encryptedEncryptionKey has its own salt here, but again, I don't know why we would need to store it separately.)
it seems like you were hinting with your other comment that we might no longer need to store the salt altogether.
There was a problem hiding this comment.
Ah sorry, no that's not quite what I meant in the other comment. That was about why we expose the salt in controller state. The encryption key and salt were both exposed in state so that the client could read them and back them up in-memory to use when the process suddenly restarted. Otherwise they would be private internal variables. You can think of controller state as a read-only API for the wallet clients (MetaMask extension/mobile). Private variables should not be included there, just what the clients need to see.
The salt is stored in the vault for a different reason - so that we can derive the same key from the password, ensuring that it can be decrypted.
That's why this case is the first time we don't need a salt in the vault - we aren't deriving the encryption key from the password in this case.
I'm not suggesting we need it here. Just that this is a substantive change to this data structure that may confuse readers. Especially it will be confusing if (due to some error) we incorrectly attempt to use a password-derived key to decrypt this vault. If we could anticipate this error and leave a more helpful error in that case, or if we could leave a helpful comment for readers to draw attention to this difference in expectations here (or both), it might be helpful for us in the future.
There was a problem hiding this comment.
IDK if "versioning" the vault could be an option here too?
- no version means "legacy vault"
- version 1 could be using the envelope mode (I heard that we wanted to fully move to that new mode anyway)
Could help us getting better errors, like we know that version 1 does not need a salt since it's encrypted differently?
Just a random idea, I don't wanna make things more complex 😄
There was a problem hiding this comment.
ah, ok, i think i got it now :D
thank you!
added a comment in 0013a1b
matthiasgeihs
force-pushed
the
feat/keyring/mock-encryptor
branch
from
0025960 to
3f83da4
Compare
matthiasgeihs
force-pushed
the
feat/vault-envelope-encryption
branch
from
9f8b301 to
5f4b620
Compare
|
rebase |
- add back skipVaultCreation where it was used before - revert change to branch coverage threshold, as this went back to before levels after reverting the above
matthiasgeihs
force-pushed
the
feat/keyring/mock-encryptor
branch
from
3f83da4 to
72d3d75
Compare
- add documentation - add check
before, we updated state.encryptedEncryptionKey outside of the updateVault function, which is not allowed.
matthiasgeihs
force-pushed
the
feat/vault-envelope-encryption
branch
from
238dda7 to
319a60e
Compare
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
matthiasgeihs
force-pushed
the
feat/keyring/mock-encryptor
branch
from
72d3d75 to
12f8275
Compare
Explanation
The idea is to add support for envelope encryption to the keyring controller.
That is, we allow to inject an external encryption key that is used for vault encryption.
We then encrypt the vault encryption key under the password and store it in the state.
This solves a problem we had with making the vault recoverable from the SRP backup key.
See https://github.com/MetaMask/decisions/pull/85 for context.
Envelope encryption also has other benefits like more efficient rekeying. (Can change the password without having to re-encrypt the whole vault.)
Things to pay extra attention to:
References
Changelog
Checklist