Skip to content

Commit 947355c

Browse files
authored
feat(user-storage-controller): use deferred promises cache for KDF operations (#6736)
## Explanation ```md - Use deferred promises for encryption/decryption KDF operations ([#6736](#6736)) - That will prevent duplicate KDF operations from being computed if one with the same options is already in progress. - For operations that already completed, we use the already existing cache. ``` This improves startup performance on mobile by ~70%; going from an average of ~6s to an average of ~2s for the time it takes to derive the key. ## References Fixes: https://consensyssoftware.atlassian.net/browse/MUL-1100 ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've communicated my changes to consumers by [updating changelogs for packages I've changed](https://github.com/MetaMask/core/tree/main/docs/contributing.md#updating-changelogs), highlighting breaking changes as necessary - [ ] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Adds a deferred-promise cache for scrypt KDF to eliminate duplicate work during concurrent encrypt/decrypt, with new concurrency tests and changelog update. > > - **Encryption (`src/shared/encryption/encryption.ts`)**: > - Add deferred promise cache for ongoing scrypt KDF operations with a bounded size to dedupe concurrent requests. > - Update `#getOrGenerateScryptKey` to reuse in-progress/completed KDF results; add `#createKdfCacheKey` and `#performKdfOperation` helpers. > - **Tests (`src/shared/encryption/encryption.test.ts`)**: > - Add concurrency tests for parallel encryptions, mixed encrypt/decrypt, different passwords, and load. > - **Docs**: > - Update `CHANGELOG.md` to note deferred KDF promise caching. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 309edae. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
1 parent c8633d9 commit 947355c

File tree

4 files changed

+233
-4
lines changed

4 files changed

+233
-4
lines changed

packages/profile-sync-controller/CHANGELOG.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
99

1010
### Changed
1111

12+
- Use deferred promises for encryption/decryption KDF operations ([#6736](https://github.com/MetaMask/core/pull/6736))
13+
- That will prevent duplicate KDF operations from being computed if one with the same options is already in progress.
14+
- For operations that already completed, we use the already existing cache.
1215
- Bump `@metamask/utils` from `^11.8.0` to `^11.8.1` ([#6708](https://github.com/MetaMask/core/pull/6708))
1316
- Bump `@metamask/keyring-api` from `^20.1.0` to `^21.0.0` ([#6560](https://github.com/MetaMask/core/pull/6560))
1417
- Bump `@metamask/keyring-internal-api` from `^8.1.0` to `^9.0.0` ([#6560](https://github.com/MetaMask/core/pull/6560))

packages/profile-sync-controller/src/shared/encryption/constants.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,3 +12,5 @@ export const SCRYPT_p = 1; // Parallelization parameter
1212
export const SHARED_SALT = new Uint8Array([
1313
0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15,
1414
]);
15+
16+
export const MAX_KDF_PROMISE_CACHE_SIZE = 20;

packages/profile-sync-controller/src/shared/encryption/encryption.test.ts

Lines changed: 154 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { MAX_KDF_PROMISE_CACHE_SIZE } from './constants';
12
import encryption, { createSHA256Hash } from './encryption';
23

34
describe('encryption tests', () => {
@@ -112,4 +113,157 @@ describe('encryption tests', () => {
112113
expect(result).toBe(false);
113114
});
114115
});
116+
117+
describe('Deferred Promise KDF Functionality', () => {
118+
it('should handle concurrent encryption operations with same password', async () => {
119+
const password = 'test-password-concurrent';
120+
const plaintext = 'test-data';
121+
122+
// Start multiple concurrent encryption operations
123+
const promises = Array(3)
124+
.fill(0)
125+
.map(async (_, i) => {
126+
return encryption.encryptString(`${plaintext}-${i}`, password);
127+
});
128+
129+
const results = await Promise.all(promises);
130+
expect(results).toHaveLength(3);
131+
132+
// Verify all results can be decrypted
133+
for (let i = 0; i < results.length; i++) {
134+
const decrypted = await encryption.decryptString(results[i], password);
135+
expect(decrypted).toBe(`${plaintext}-${i}`);
136+
}
137+
});
138+
139+
it('should handle concurrent encrypt/decrypt operations', async () => {
140+
const password = 'test-concurrent-mixed';
141+
const testData = 'concurrent-test-data';
142+
143+
// First encrypt some data
144+
const encryptedData = await encryption.encryptString(testData, password);
145+
146+
// Start concurrent operations
147+
const decryptPromises = Array(2)
148+
.fill(0)
149+
.map(() => {
150+
return encryption.decryptString(encryptedData, password);
151+
});
152+
153+
const encryptPromises = Array(2)
154+
.fill(0)
155+
.map((_, i) => {
156+
return encryption.encryptString(`new-data-${i}`, password);
157+
});
158+
159+
const allResults = await Promise.all([
160+
...decryptPromises,
161+
...encryptPromises,
162+
]);
163+
164+
// Verify decrypt results
165+
expect(allResults[0]).toBe(testData);
166+
expect(allResults[1]).toBe(testData);
167+
168+
// Verify encrypt results can be decrypted
169+
const newDecrypted0 = await encryption.decryptString(
170+
allResults[2],
171+
password,
172+
);
173+
const newDecrypted1 = await encryption.decryptString(
174+
allResults[3],
175+
password,
176+
);
177+
expect(newDecrypted0).toBe('new-data-0');
178+
expect(newDecrypted1).toBe('new-data-1');
179+
});
180+
181+
it('should handle different passwords concurrently', async () => {
182+
const password1 = 'password-one';
183+
const password2 = 'password-two';
184+
const testData = 'multi-password-test';
185+
186+
// Start concurrent operations with different passwords
187+
const promises = [
188+
encryption.encryptString(testData, password1),
189+
encryption.encryptString(testData, password2),
190+
];
191+
192+
const results = await Promise.all(promises);
193+
expect(results).toHaveLength(2);
194+
195+
// Verify decryption with correct passwords
196+
const decrypted1 = await encryption.decryptString(results[0], password1);
197+
const decrypted2 = await encryption.decryptString(results[1], password2);
198+
199+
expect(decrypted1).toBe(testData);
200+
expect(decrypted2).toBe(testData);
201+
202+
// Cross-password decryption should fail
203+
await expect(
204+
encryption.decryptString(results[0], password2),
205+
).rejects.toThrow(
206+
'Unable to decrypt string - aes/gcm: invalid ghash tag',
207+
);
208+
await expect(
209+
encryption.decryptString(results[1], password1),
210+
).rejects.toThrow(
211+
'Unable to decrypt string - aes/gcm: invalid ghash tag',
212+
);
213+
});
214+
215+
it('should work correctly under concurrent load', async () => {
216+
const password = 'load-test-password';
217+
const baseData = 'load-test-data';
218+
219+
// Create a larger number of concurrent operations
220+
const encryptPromises = Array(10)
221+
.fill(0)
222+
.map((_, i) => encryption.encryptString(`${baseData}-${i}`, password));
223+
224+
const results = await Promise.all(encryptPromises);
225+
expect(results).toHaveLength(10);
226+
227+
// Verify all can be decrypted
228+
const decryptPromises = results.map((encrypted, i) =>
229+
encryption.decryptString(encrypted, password).then((decrypted) => {
230+
expect(decrypted).toBe(`${baseData}-${i}`);
231+
return decrypted;
232+
}),
233+
);
234+
235+
await Promise.all(decryptPromises);
236+
});
237+
238+
it('should limit KDF promise cache size and remove oldest entries when limit is reached', async () => {
239+
// Create enough operations to exceed the actual cache limit
240+
const numOperations = MAX_KDF_PROMISE_CACHE_SIZE + 5; // 25 operations to exceed the limit
241+
242+
const promises: Promise<string>[] = [];
243+
for (let i = 0; i < numOperations; i++) {
244+
// Use different passwords to create unique cache keys
245+
const uniquePassword = `cache-test-${i}`;
246+
promises.push(encryption.encryptString('test-data', uniquePassword));
247+
}
248+
249+
// All operations should complete successfully despite cache limit
250+
const results = await Promise.all(promises);
251+
expect(results).toHaveLength(numOperations);
252+
253+
// Verify a sampling of results can be decrypted (testing all 25 would be slow)
254+
const sampleIndices = [
255+
0,
256+
Math.floor(MAX_KDF_PROMISE_CACHE_SIZE / 2),
257+
numOperations - 1,
258+
]; // Test first, middle, and last
259+
for (const i of sampleIndices) {
260+
const uniquePassword = `cache-test-${i}`;
261+
const decrypted = await encryption.decryptString(
262+
results[i],
263+
uniquePassword,
264+
);
265+
expect(decrypted).toBe('test-data');
266+
}
267+
}, 30000);
268+
});
115269
});

packages/profile-sync-controller/src/shared/encryption/encryption.ts

Lines changed: 74 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import {
1212
import {
1313
ALGORITHM_KEY_SIZE,
1414
ALGORITHM_NONCE_SIZE,
15+
MAX_KDF_PROMISE_CACHE_SIZE,
1516
SCRYPT_N,
1617
SCRYPT_p,
1718
SCRYPT_r,
@@ -49,6 +50,12 @@ export type EncryptedPayload = {
4950
};
5051

5152
class EncryptorDecryptor {
53+
// Promise cache for ongoing KDF operations to prevent duplicate work
54+
readonly #kdfPromiseCache = new Map<
55+
string,
56+
Promise<{ key: Uint8Array; salt: Uint8Array }>
57+
>();
58+
5259
async encryptString(
5360
plaintext: string,
5461
password: string,
@@ -239,6 +246,8 @@ class EncryptorDecryptor {
239246
nativeScryptCrypto?: NativeScrypt,
240247
) {
241248
const hashedPassword = createSHA256Hash(password);
249+
250+
// Check if we already have the key cached
242251
const cachedKey = salt
243252
? getCachedKeyBySalt(hashedPassword, salt)
244253
: getCachedKeyGeneratedWithSharedSalt(hashedPassword);
@@ -250,33 +259,94 @@ class EncryptorDecryptor {
250259
};
251260
}
252261

262+
// Create a unique cache key for this KDF operation
253263
const newSalt = salt ?? SHARED_SALT;
264+
const cacheKey = this.#createKdfCacheKey(
265+
hashedPassword,
266+
o,
267+
newSalt,
268+
nativeScryptCrypto,
269+
);
270+
271+
// Check if there's already an ongoing KDF operation with the same parameters
272+
const existingPromise = this.#kdfPromiseCache.get(cacheKey);
273+
if (existingPromise) {
274+
return existingPromise;
275+
}
276+
277+
// Limit cache size to prevent unbounded growth
278+
if (this.#kdfPromiseCache.size >= MAX_KDF_PROMISE_CACHE_SIZE) {
279+
// Remove the oldest entry (first inserted)
280+
const firstKey = this.#kdfPromiseCache.keys().next().value;
281+
if (firstKey) {
282+
this.#kdfPromiseCache.delete(firstKey);
283+
}
284+
}
285+
286+
// Create and cache the promise for the KDF operation
287+
const kdfPromise = this.#performKdfOperation(
288+
password,
289+
o,
290+
newSalt,
291+
hashedPassword,
292+
nativeScryptCrypto,
293+
);
254294

295+
// Cache the promise and set up cleanup
296+
this.#kdfPromiseCache.set(cacheKey, kdfPromise);
297+
298+
// Clean up the cache after completion (both success and failure)
299+
// eslint-disable-next-line no-void
300+
void kdfPromise.finally(() => {
301+
this.#kdfPromiseCache.delete(cacheKey);
302+
});
303+
304+
return kdfPromise;
305+
}
306+
307+
#createKdfCacheKey(
308+
hashedPassword: string,
309+
o: EncryptedPayload['o'],
310+
salt: Uint8Array,
311+
nativeScryptCrypto?: NativeScrypt,
312+
): string {
313+
const saltStr = byteArrayToBase64(salt);
314+
const hasNative = Boolean(nativeScryptCrypto);
315+
return `${hashedPassword}:${o.N}:${o.r}:${o.p}:${o.dkLen}:${saltStr}:${hasNative}`;
316+
}
317+
318+
async #performKdfOperation(
319+
password: string,
320+
o: EncryptedPayload['o'],
321+
salt: Uint8Array,
322+
hashedPassword: string,
323+
nativeScryptCrypto?: NativeScrypt,
324+
): Promise<{ key: Uint8Array; salt: Uint8Array }> {
255325
let newKey: Uint8Array;
256326

257327
if (nativeScryptCrypto) {
258328
newKey = await nativeScryptCrypto(
259329
stringToByteArray(password),
260-
newSalt,
330+
salt,
261331
o.N,
262332
o.r,
263333
o.p,
264334
o.dkLen,
265335
);
266336
} else {
267-
newKey = await scryptAsync(password, newSalt, {
337+
newKey = await scryptAsync(password, salt, {
268338
N: o.N,
269339
r: o.r,
270340
p: o.p,
271341
dkLen: o.dkLen,
272342
});
273343
}
274344

275-
setCachedKey(hashedPassword, newSalt, newKey);
345+
setCachedKey(hashedPassword, salt, newKey);
276346

277347
return {
278348
key: newKey,
279-
salt: newSalt,
349+
salt,
280350
};
281351
}
282352
}

0 commit comments

Comments
 (0)