feat: persist UserStorage e2e content keys using an encrypted keyStore #5129
Conversation
mirceanis
added
the
team-identity
|
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
mirceanis
force-pushed
the
5128-persist-user-storage-e2e-keys
branch
from
be2f7ca to
62a9880
Compare
| storeWrappedKey: (keyRef: string, wrappedKey: string): Promise<void> => { | ||
| return new Promise((resolve) => { | ||
| this.update((state) => { | ||
| state.encryptedContentKeys[keyRef] = wrappedKey; | ||
| resolve(); | ||
| }); | ||
| }); | ||
| }, |
There was a problem hiding this comment.
Do these need to be promises?
If we do want to make then pseudo promises, we can make the functions async and avoid return explicit promises.
// By using the `async` function syntax, the return type will be inferred as a promise.
storeWrappedKey: async (...): Promise<void> => {
this.update(...)
}
packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts
Show resolved
Hide resolved
| @@ -54,12 +56,14 @@ class EncryptorDecryptor { | |||
| plaintext: string, | |||
| password: string, | |||
| nativeScryptCrypto?: NativeScrypt, | |||
| keyStore?: KeyStore, | |||
There was a problem hiding this comment.
long term (far future) - I don't really like how many layers we have to drill in this context (both for NativeScrypt, and for this).
We can think of some diff approach later.
| const keyRef = `${hashedPassword}${bytesToHex(newSalt)}`; | ||
| if (keyStore) { | ||
| try { | ||
| const storedKey = await keyStore.loadKey(keyRef); |
There was a problem hiding this comment.
Oooh very cool, so this keyStore will be used to store the encrypted keys to be used by other potential platforms?
As we persist this on the extension, would we need a way for this to be accessible outside of the extension? Reason I'm asking is could we place this keystore into the snap storage? That way sites can access this from the snap rather than the extension?
| nonce: bytesToBase64(nonce), | ||
| ephemPublicKey: bytesToBase64(ephemeralPublic), | ||
| ciphertext: bytesToBase64(encryptedMessage), | ||
| } as Eip1024EncryptedData; |
There was a problem hiding this comment.
What fields are missing for this to be be type asserted?
Explanation
The
UserStorageControllere2e encryption keys are derived from astorageKeythat is specific to the user profile. The key derivation function used isscrypt, with parameters recommended for password inputs. This means that it's a very costly operation (on the order of seconds on a 2024 mobile device).These derived keys are cached in memory for the lifetime of the controller instance, but a better approach would be to use a Key Store, to persist the derived keys in a safe manner. This would avoid the rerun of the costly key derivation operation on every app restart.
This set of changes also comes at a time when we're preparing for a multi-device/multi-srp user profile, so the implementation of the KeyStore relies on the new encryption capabilities of the message-signing-snap. The snap is already used for auth and is a key component of the UserStorageController, but we are introducing some defaults here that create even more coupling between the controller and the snap.
The proposed KeyStore implementation relies on the persistable state properties and the messagingSystem available to MM controllers, so externalizing this would create some overhead, but feedback is very welcome about the approach.
References
fixes #5128
Changelog
@metamask/profile-sync-controllerChecklist