feat: reduce KDF computing cost on clients

packages/profile-sync-controller/CHANGELOG.md

@@ -7,6 +7,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Changed
+
+- Reduce the `N` `scrypt` parameter used to compute the encryption key in order to significantly increase performance on clients.
+  - The input to the KDF is already a long, high entropy non-user-generated string so it does not make sense to use a KDF with high compute cost.
+  - This decreases the impact of the KDF on all clients by around 99%
+  - Add backwards compatible migration logic
+  - Add encryption callbacks so we can measure the impact of future migrations through analytics
+
 ## [21.0.0]
 
 ### Added

packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.test.ts

@@ -17,6 +17,13 @@ import * as AccountSyncControllerIntegrationModule from './account-syncing/contr
 import { BACKUPANDSYNC_FEATURES } from './constants';
 import { MOCK_STORAGE_DATA, MOCK_STORAGE_KEY } from './mocks/mockStorage';
 import UserStorageController, { defaultState } from './UserStorageController';
+import {
+  ALGORITHM_KEY_SIZE,
+  SCRYPT_N_V2,
+  SCRYPT_p,
+  SCRYPT_r,
+  SCRYPT_SALT_SIZE,
+} from '../../shared/encryption/constants';
 import { USER_STORAGE_FEATURE_NAMES } from '../../shared/storage-schema';
 
 describe('user-storage/user-storage-controller - constructor() tests', () => {
@@ -936,3 +943,59 @@ describe('user-storage/user-storage-controller - snap handling', () => {
     expect(await controller.getStorageKey()).toBe(MOCK_STORAGE_KEY);
   });
 });
+
+describe('user-storage/user-storage-controller - encryption callbacks', () => {
+  const arrangeMocks = async () => {
+    const messengerMocks = mockUserStorageMessenger();
+    const mockOnDecrypt = jest.fn();
+    const mockOnEncrypt = jest.fn();
+    const controller = new UserStorageController({
+      messenger: messengerMocks.messenger,
+      config: {
+        encryption: {
+          onDecrypt: mockOnDecrypt,
+          onEncrypt: mockOnEncrypt,
+        },
+      },
+    });
+
+    await Promise.all([
+      mockEndpointGetUserStorage(
+        `${USER_STORAGE_FEATURE_NAMES.notifications}.notification_settings`,
+      ),
+      mockEndpointUpsertUserStorage(
+        `${USER_STORAGE_FEATURE_NAMES.notifications}.notification_settings`,
+      ),
+    ]);
+
+    return { mockOnDecrypt, mockOnEncrypt, controller };
+  };
+
+  it('calls encryption callbacks if they are set', async () => {
+    const { mockOnEncrypt, mockOnDecrypt, controller } = await arrangeMocks();
+
+    await controller.performSetStorage(
+      `${USER_STORAGE_FEATURE_NAMES.notifications}.notification_settings`,
+      'new data',
+    );
+
+    await controller.performGetStorage(
+      `${USER_STORAGE_FEATURE_NAMES.notifications}.notification_settings`,
+    );
+
+    const scryptParams = {
+      o: {
+        N: SCRYPT_N_V2,
+        dkLen: ALGORITHM_KEY_SIZE,
+        p: SCRYPT_p,
+        r: SCRYPT_r,
+      },
+      saltLen: SCRYPT_SALT_SIZE,
+      t: 'scrypt',
+      v: '1',
+    };
+
+    expect(mockOnEncrypt).toHaveBeenCalledWith(scryptParams);
+    expect(mockOnDecrypt).toHaveBeenCalledWith(scryptParams);
+  });
+});

packages/profile-sync-controller/src/controllers/user-storage/UserStorageController.ts

@@ -44,6 +44,7 @@ import { BACKUPANDSYNC_FEATURES } from './constants';
 import { syncContactsWithUserStorage } from './contact-syncing/controller-integration';
 import { setupContactSyncingSubscriptions } from './contact-syncing/setup-subscriptions';
 import type {
+  EncryptedPayload,
   UserStorageGenericFeatureKey,
   UserStorageGenericPathWithFeatureAndKey,
   UserStorageGenericPathWithFeatureOnly,
@@ -147,6 +148,10 @@ const metadata: StateMetadata<UserStorageControllerState> = {
 
 type ControllerConfig = {
   env: Env;
+  encryption?: {
+    onEncrypt?: (encryptedData: Omit<EncryptedPayload, 'd'>) => Promise<void>;
+    onDecrypt?: (encryptedData: Omit<EncryptedPayload, 'd'>) => Promise<void>;
+  };
   accountSyncing?: {
     maxNumberOfAccountsToAdd?: number;
     /**
@@ -490,6 +495,8 @@ export default class UserStorageController extends BaseController<
     return await this.#userStorage.getItem(path, {
       nativeScryptCrypto: this.#nativeScryptCrypto,
       entropySourceId,
+      onDecrypt: this.#config.encryption?.onDecrypt,
+      onEncrypt: this.#config.encryption?.onEncrypt,
     });
   }
 
@@ -508,6 +515,8 @@ export default class UserStorageController extends BaseController<
     return await this.#userStorage.getAllFeatureItems(path, {
       nativeScryptCrypto: this.#nativeScryptCrypto,
       entropySourceId,
+      onDecrypt: this.#config.encryption?.onDecrypt,
+      onEncrypt: this.#config.encryption?.onEncrypt,
     });
   }
 
@@ -528,6 +537,8 @@ export default class UserStorageController extends BaseController<
     return await this.#userStorage.setItem(path, value, {
       nativeScryptCrypto: this.#nativeScryptCrypto,
       entropySourceId,
+      onDecrypt: this.#config.encryption?.onDecrypt,
+      onEncrypt: this.#config.encryption?.onEncrypt,
     });
   }
 
@@ -548,6 +559,8 @@ export default class UserStorageController extends BaseController<
     return await this.#userStorage.batchSetItems(path, values, {
       nativeScryptCrypto: this.#nativeScryptCrypto,
       entropySourceId,
+      onDecrypt: this.#config.encryption?.onDecrypt,
+      onEncrypt: this.#config.encryption?.onEncrypt,
     });
   }
 

packages/profile-sync-controller/src/sdk/user-storage.test.ts

@@ -1,5 +1,3 @@
-import type { UserStorageGenericFeatureKey } from 'src/shared/storage-schema';
-
 import { arrangeAuthAPIs } from './__fixtures__/auth';
 import { arrangeAuth, typedMockFn } from './__fixtures__/test-utils';
 import {
@@ -20,8 +18,9 @@ import {
 import type { StorageOptions } from './user-storage';
 import { STORAGE_URL, UserStorage } from './user-storage';
 import encryption, { createSHA256Hash } from '../shared/encryption';
-import { SHARED_SALT } from '../shared/encryption/constants';
+import { SCRYPT_N_V2, SHARED_SALT_V2 } from '../shared/encryption/constants';
 import { Env } from '../shared/env';
+import type { UserStorageGenericFeatureKey } from '../shared/storage-schema';
 import { USER_STORAGE_FEATURE_NAMES } from '../shared/storage-schema';
 
 const MOCK_SRP = '0x6265617665726275696c642e6f7267';
@@ -115,11 +114,52 @@ describe('User Storage', () => {
           return;
         }
 
-        const isEncryptedUsingSharedSalt =
+        const isEncryptedUsingSharedSaltV2 =
           encryption.getSalt(requestBody.data).toString() ===
-          SHARED_SALT.toString();
+          SHARED_SALT_V2.toString();
+
+        const isEncryptedUsingNewScryptN =
+          JSON.parse(requestBody.data).o.N === SCRYPT_N_V2;
+
+        expect(isEncryptedUsingSharedSaltV2).toBe(true);
+        expect(isEncryptedUsingNewScryptN).toBe(true);
+      },
+    );
+
+    await userStorage.getItem(
+      `${USER_STORAGE_FEATURE_NAMES.notifications}.notification_settings`,
+    );
+    expect(mockGet.isDone()).toBe(true);
+    expect(mockPut.isDone()).toBe(true);
+  });
+
+  it('re-encrypts data if received entry was encrypted with the old scrypt N param, and saves it back to user storage', async () => {
+    const { auth } = arrangeAuth('SRP', MOCK_SRP);
+    const { userStorage } = arrangeUserStorage(auth);
+
+    // This corresponds to 'data1'
+    // Encrypted with the old scrypt N param
+    const mockResponse = {
+      HashedKey: 'entry1',
+      Data: '{"v":"1","t":"scrypt","d":"HIu+WgFBCtKo6rEGy0R8h8t/JgXhzC2a3AF6epahGY2h6GibXDKxSBf6ppxM099Gmg==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}',
+    };
 
-        expect(isEncryptedUsingSharedSalt).toBe(true);
+    const mockGet = await handleMockUserStorageGet({
+      status: 200,
+      body: JSON.stringify(mockResponse),
+    });
+    const mockPut = handleMockUserStoragePut(
+      undefined,
+      async (_, requestBody) => {
+        // eslint-disable-next-line jest/no-conditional-in-test
+        if (typeof requestBody === 'string') {
+          return;
+        }
+
+        const isEncryptedUsingNewScryptN =
+          JSON.parse(requestBody.data).o.N === SCRYPT_N_V2;
+
+        expect(isEncryptedUsingNewScryptN).toBe(true);
       },
     );
 
@@ -184,20 +224,19 @@ describe('User Storage', () => {
           return;
         }
 
-        const doEntriesHaveDifferentSalts =
-          encryption.getIfEntriesHaveDifferentSalts(
-            Object.entries(requestBody.data).map((entry) => entry[1] as string),
-          );
-
-        expect(doEntriesHaveDifferentSalts).toBe(false);
-
-        const doEntriesUseSharedSalt = Object.entries(requestBody.data).every(
+        const doEntriesUseSharedSaltV2 = Object.entries(requestBody.data).every(
           ([_entryKey, entryValue]) =>
             encryption.getSalt(entryValue as string).toString() ===
-            SHARED_SALT.toString(),
+            SHARED_SALT_V2.toString(),
+        );
+
+        const doEntriesUseNewScryptN = Object.entries(requestBody.data).every(
+          ([_entryKey, entryValue]) =>
+            JSON.parse(entryValue as string).o.N === SCRYPT_N_V2,
         );
 
-        expect(doEntriesUseSharedSalt).toBe(true);
+        expect(doEntriesUseSharedSaltV2).toBe(true);
+        expect(doEntriesUseNewScryptN).toBe(true);
 
         const wereOnlyNonEmptySaltEntriesUploaded =
           Object.entries(requestBody.data).length === 2;
@@ -213,6 +252,54 @@ describe('User Storage', () => {
     expect(mockPut.isDone()).toBe(true);
   });
 
+  it('re-encrypts data if received entries were encrypted with the old scrypt N param, and saves it back to user storage', async () => {
+    // This corresponds to [['entry1', 'data1'], ['entry2', 'data2'], ['HASHED_KEY', '{ "hello": "world" }']]
+    // Each entry has been encrypted with a random salt, except for the last entry
+    // The last entry is used to test if the function can handle payloads that contain both random salts and the shared salt
+    const mockResponse = [
+      {
+        HashedKey: 'entry1',
+        Data: '{"v":"1","t":"scrypt","d":"HIu+WgFBCtKo6rEGy0R8h8t/JgXhzC2a3AF6epahGY2h6GibXDKxSBf6ppxM099Gmg==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}',
+      },
+      {
+        HashedKey: 'entry2',
+        Data: '{"v":"1","t":"scrypt","d":"3ioo9bxhjDjTmJWIGQMnOlnfa4ysuUNeLYTTmJ+qrq7gwI6hURH3ooUcBldJkHtvuQ==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}',
+      },
+      await MOCK_STORAGE_RESPONSE('data3'),
+    ];
+
+    const { auth } = arrangeAuth('SRP', MOCK_SRP);
+    const { userStorage } = arrangeUserStorage(auth);
+
+    const mockGetAll = await handleMockUserStorageGetAllFeatureEntries({
+      status: 200,
+      body: mockResponse,
+    });
+
+    const mockPut = handleMockUserStoragePut(
+      undefined,
+      async (_, requestBody) => {
+        // eslint-disable-next-line jest/no-conditional-in-test
+        if (typeof requestBody === 'string') {
+          return;
+        }
+
+        const doEntriesUseNewScryptN = Object.entries(requestBody.data).every(
+          ([_entryKey, entryValue]) =>
+            JSON.parse(entryValue as string).o.N === SCRYPT_N_V2,
+        );
+
+        expect(doEntriesUseNewScryptN).toBe(true);
+      },
+    );
+
+    await userStorage.getAllFeatureItems(
+      USER_STORAGE_FEATURE_NAMES.notifications,
+    );
+    expect(mockGetAll.isDone()).toBe(true);
+    expect(mockPut.isDone()).toBe(true);
+  });
+
   it('batch set items', async () => {
     const dataToStore: [UserStorageGenericFeatureKey, string][] = [
       ['0x123', JSON.stringify(MOCK_NOTIFICATIONS_DATA)],