Skip to content
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.

Sign up for GitHub

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

feat: persist UserStorage e2e content keys using an encrypted keyStore #5129

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

feat: persist UserStorage e2e content keys using an encrypted keyStore #5129

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,3 +35,6 @@ scripts/coverage

# typescript
packages/*/*.tsbuildinfo

# jetbrains IDE local files
/.idea
2 changes: 2 additions & 0 deletions packages/profile-sync-controller/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,9 @@
"@metamask/network-controller": "^22.1.1",
"@metamask/snaps-sdk": "^6.7.0",
"@metamask/snaps-utils": "^8.3.0",
"@metamask/utils": "^11.0.1",
"@noble/ciphers": "^0.5.2",
"@noble/curves": "^1.7.0",
"@noble/hashes": "^1.4.0",
"immer": "^9.0.6",
"loglevel": "^1.8.1",
Expand Down
37 changes: 37 additions & 0 deletions packages/profile-sync-controller/src/controllers/authentication/auth-snap-requests.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/* eslint-disable @typescript-eslint/no-explicit-any */
import type { HandleSnapRequest } from '@metamask/snaps-controllers';
import type { SnapId } from '@metamask/snaps-sdk';
import type { Eip1024EncryptedData } from '@metamask/utils';

type SnapRPCRequest = Parameters<HandleSnapRequest['handler']>[0];

Expand All @@ -22,6 +23,22 @@ export function createSnapPublicKeyRequest(): SnapRPCRequest {
};
}

/**
* Constructs Request to Message Signing Snap to get the Encryption Public Key
*
* @returns Snap Encryption Public Key Request
*/
export function createSnapEncryptionPublicKeyRequest(): SnapRPCRequest {
return {
snapId,
origin: '',
handler: 'onRpcRequest' as any,
request: {
method: 'getEncryptionPublicKey',
},
};
}

/**
* Constructs Request to get Message Signing Snap to sign a message.
*
Expand All @@ -41,3 +58,23 @@ export function createSnapSignMessageRequest(
},
};
}

/**
* Constructs Request to get Message Signing Snap to decrypt a message.
*
* @param data - message to decrypt
* @returns Snap Sign Message Request
*/
export function createSnapDecryptMessageRequest(
data: Eip1024EncryptedData,
): SnapRPCRequest {
return {
snapId,
origin: '',
handler: 'onRpcRequest' as any,
request: {
method: 'decryptMessage',
params: { data },
},
};
}
9 changes: 9 additions & 0 deletions packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ describe('user-storage/user-storage-controller - performGetStorage() tests', ()
isAccountSyncingInProgress: false,
hasAccountSyncingSyncedAtLeastOnce: false,
isAccountSyncingReadyToBeDispatched: false,
encryptedContentKeys: {},
},
});

Expand Down Expand Up @@ -191,6 +192,7 @@ describe('user-storage/user-storage-controller - performGetStorageAllFeatureEntr
hasAccountSyncingSyncedAtLeastOnce: false,
isAccountSyncingReadyToBeDispatched: false,
isAccountSyncingInProgress: false,
encryptedContentKeys: {},
},
});

Expand Down Expand Up @@ -273,6 +275,7 @@ describe('user-storage/user-storage-controller - performSetStorage() tests', ()
hasAccountSyncingSyncedAtLeastOnce: false,
isAccountSyncingReadyToBeDispatched: false,
isAccountSyncingInProgress: false,
encryptedContentKeys: {},
},
});

Expand Down Expand Up @@ -379,6 +382,7 @@ describe('user-storage/user-storage-controller - performBatchSetStorage() tests'
hasAccountSyncingSyncedAtLeastOnce: false,
isAccountSyncingReadyToBeDispatched: false,
isAccountSyncingInProgress: false,
encryptedContentKeys: {},
},
});

Expand Down Expand Up @@ -482,6 +486,7 @@ describe('user-storage/user-storage-controller - performBatchDeleteStorage() tes
hasAccountSyncingSyncedAtLeastOnce: false,
isAccountSyncingReadyToBeDispatched: false,
isAccountSyncingInProgress: false,
encryptedContentKeys: {},
},
});

Expand Down Expand Up @@ -586,6 +591,7 @@ describe('user-storage/user-storage-controller - performDeleteStorage() tests',
hasAccountSyncingSyncedAtLeastOnce: false,
isAccountSyncingReadyToBeDispatched: false,
isAccountSyncingInProgress: false,
encryptedContentKeys: {},
},
});

Expand Down Expand Up @@ -687,6 +693,7 @@ describe('user-storage/user-storage-controller - performDeleteStorageAllFeatureE
hasAccountSyncingSyncedAtLeastOnce: false,
isAccountSyncingReadyToBeDispatched: false,
isAccountSyncingInProgress: false,
encryptedContentKeys: {},
},
});

Expand Down Expand Up @@ -780,6 +787,7 @@ describe('user-storage/user-storage-controller - getStorageKey() tests', () => {
hasAccountSyncingSyncedAtLeastOnce: false,
isAccountSyncingReadyToBeDispatched: false,
isAccountSyncingInProgress: false,
encryptedContentKeys: {},
},
});

Expand Down Expand Up @@ -827,6 +835,7 @@ describe('user-storage/user-storage-controller - enableProfileSyncing() tests',
hasAccountSyncingSyncedAtLeastOnce: false,
isAccountSyncingReadyToBeDispatched: false,
isAccountSyncingInProgress: false,
encryptedContentKeys: {},
},
});

Expand Down
113 changes: 107 additions & 6 deletions packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import type {
AccountsControllerAccountAddedEvent,
AccountsControllerAccountRenamedEvent,
AccountsControllerListAccountsAction,
AccountsControllerUpdateAccountMetadataAction,
AccountsControllerAccountRenamedEvent,
AccountsControllerAccountAddedEvent,
} from '@metamask/accounts-controller';
import type {
ControllerGetStateAction,
Expand All @@ -12,10 +12,10 @@ import type {
} from '@metamask/base-controller';
import { BaseController } from '@metamask/base-controller';
import {
type KeyringControllerAddNewAccountAction,
type KeyringControllerGetStateAction,
type KeyringControllerLockEvent,
type KeyringControllerUnlockEvent,
type KeyringControllerAddNewAccountAction,
} from '@metamask/keyring-controller';
import type { InternalAccount } from '@metamask/keyring-internal-api';
import type {
Expand All @@ -26,22 +26,29 @@ import type {
NetworkControllerUpdateNetworkAction,
} from '@metamask/network-controller';
import type { HandleSnapRequest } from '@metamask/snaps-controllers';
import type { Eip1024EncryptedData } from '@metamask/utils';

import { createSHA256Hash } from '../../shared/encryption';
import type { KeyStore } from '../../shared/encryption/key-storage';
import { ERC1024WrappedKeyStore } from '../../shared/encryption/key-storage';
import type { UserStorageFeatureKeys } from '../../shared/storage-schema';
import {
type UserStoragePathWithFeatureAndKey,
type UserStoragePathWithFeatureOnly,
} from '../../shared/storage-schema';
import type { NativeScrypt } from '../../shared/types/encryption';
import { createSnapSignMessageRequest } from '../authentication/auth-snap-requests';
import {
createSnapDecryptMessageRequest,
createSnapEncryptionPublicKeyRequest,
createSnapSignMessageRequest,
} from '../authentication/auth-snap-requests';
import type {
AuthenticationControllerGetBearerToken,
AuthenticationControllerGetSessionProfile,
AuthenticationControllerIsSignedIn,
AuthenticationControllerPerformSignIn,
AuthenticationControllerPerformSignOut,
} from '../authentication/AuthenticationController';
} from '../authentication';
import {
saveInternalAccountToUserStorage,
syncInternalAccountsWithUserStorage,
Expand Down Expand Up @@ -102,6 +109,10 @@ export type UserStorageControllerState = {
* Condition used to ensure that we do not perform any network sync mutations until we have synced at least once
*/
hasNetworkSyncingSyncedAtLeastOnce?: boolean;
/**
* Content keys used to encrypt/decrypt user storage content. These are wrapped while at rest.
*/
encryptedContentKeys: Record<string, string>;
};

export const defaultState: UserStorageControllerState = {
Expand All @@ -110,6 +121,7 @@ export const defaultState: UserStorageControllerState = {
hasAccountSyncingSyncedAtLeastOnce: false,
isAccountSyncingReadyToBeDispatched: false,
isAccountSyncingInProgress: false,
encryptedContentKeys: {},
};

const metadata: StateMetadata<UserStorageControllerState> = {
Expand Down Expand Up @@ -137,6 +149,10 @@ const metadata: StateMetadata<UserStorageControllerState> = {
persist: true,
anonymous: false,
},
encryptedContentKeys: {
persist: true,
anonymous: false,
},
};

type ControllerConfig = {
Expand Down Expand Up @@ -313,6 +329,79 @@ export default class UserStorageController extends BaseController<
isNetworkSyncingEnabled: false,
};

#_snapPublicKeyCache: string | null = null;

#keyWrapping = {
/**
* Returns the snap Encryption public key.
*
* @returns The snap Encryption public key.
*/
snapGetEncryptionPublicKey: async (): Promise<string> => {
if (this.#_snapPublicKeyCache) {
return this.#_snapPublicKeyCache;
}

if (!this.#isUnlocked) {
throw new Error(
'#snapGetEncryptionPublicKey - unable to call snap, wallet is locked',
);
}

const result = (await this.messagingSystem.call(
'SnapController:handleRequest',
createSnapEncryptionPublicKeyRequest(),
)) as string;

this.#_snapPublicKeyCache = result;

return result;
},

/**
* Decrypts a message using the message signing snap.
*
* @param data - Eip1024EncryptedData - The encrypted data.
* @returns The decrypted message, if it was intended for this wallet, null otherwise. TODO: check error scenarios
*/
snapDecryptMessage: async (data: Eip1024EncryptedData): Promise<string> => {
if (!this.#isUnlocked) {
throw new Error(
'#snapDecryptMessage - unable to call snap, wallet is locked',
);
}

const result = (await this.messagingSystem.call(
'SnapController:handleRequest',
createSnapDecryptMessageRequest(data),
)) as string;

return result;
},

loadWrappedKey: async (keyRef: string): Promise<string | null> => {
return this.state.encryptedContentKeys[keyRef] ?? null;
},

storeWrappedKey: (keyRef: string, wrappedKey: string): Promise<void> => {
return new Promise((resolve) => {
this.update((state) => {
state.encryptedContentKeys[keyRef] = wrappedKey;
resolve();
});
});
},
Comment on lines +386 to +393
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(...)
}


getWrappedKeyStore: (): KeyStore => {
return new ERC1024WrappedKeyStore({
Prithpal-Sooriya marked this conversation as resolved.
Show resolved Hide resolved
decryptMessage: this.#keyWrapping.snapDecryptMessage,
getPublicKey: this.#keyWrapping.snapGetEncryptionPublicKey,
getItem: this.#keyWrapping.loadWrappedKey,
setItem: this.#keyWrapping.storeWrappedKey,
});
},
};

#auth = {
getBearerToken: async () => {
return await this.messagingSystem.call(
Expand Down Expand Up @@ -374,7 +463,9 @@ export default class UserStorageController extends BaseController<
},
};

#nativeScryptCrypto: NativeScrypt | undefined = undefined;
#nativeScryptCrypto: NativeScrypt | undefined;

#keyStore: KeyStore | undefined;

getMetaMetricsState: () => boolean;

Expand All @@ -385,6 +476,7 @@ export default class UserStorageController extends BaseController<
config,
getMetaMetricsState,
nativeScryptCrypto,
keyStore,
}: {
messenger: UserStorageControllerMessenger;
state?: UserStorageControllerState;
Expand All @@ -395,6 +487,7 @@ export default class UserStorageController extends BaseController<
};
getMetaMetricsState: () => boolean;
nativeScryptCrypto?: NativeScrypt;
keyStore?: KeyStore;
}) {
super({
messenger,
Expand Down Expand Up @@ -432,6 +525,8 @@ export default class UserStorageController extends BaseController<
!this.state.hasNetworkSyncingSyncedAtLeastOnce,
});
}

this.#keyStore = keyStore ?? this.#keyWrapping.getWrappedKeyStore();
}

/**
Expand Down Expand Up @@ -491,6 +586,7 @@ export default class UserStorageController extends BaseController<
storageKey,
bearerToken,
nativeScryptCrypto: this.#nativeScryptCrypto,
keyStore: this.#keyStore,
};
}

Expand Down Expand Up @@ -581,6 +677,7 @@ export default class UserStorageController extends BaseController<
bearerToken,
storageKey,
nativeScryptCrypto: this.#nativeScryptCrypto,
keyStore: this.#keyStore,
});

return result;
Expand All @@ -606,6 +703,7 @@ export default class UserStorageController extends BaseController<
bearerToken,
storageKey,
nativeScryptCrypto: this.#nativeScryptCrypto,
keyStore: this.#keyStore,
});

return result;
Expand Down Expand Up @@ -633,6 +731,7 @@ export default class UserStorageController extends BaseController<
bearerToken,
storageKey,
nativeScryptCrypto: this.#nativeScryptCrypto,
keyStore: this.#keyStore,
});
}

Expand Down Expand Up @@ -660,6 +759,7 @@ export default class UserStorageController extends BaseController<
bearerToken,
storageKey,
nativeScryptCrypto: this.#nativeScryptCrypto,
keyStore: this.#keyStore,
});
}

Expand Down Expand Up @@ -730,6 +830,7 @@ export default class UserStorageController extends BaseController<
bearerToken,
storageKey,
nativeScryptCrypto: this.#nativeScryptCrypto,
keyStore: this.#keyStore,
});
}

Expand Down
Loading
Loading