Simplify signed prekey handling - always save for 30 days, always save five

ts/test-electron/textsecure/AccountManager_test.ts

@@ -1,47 +1,68 @@
 // Copyright 2017-2020 Signal Messenger, LLC
 // SPDX-License-Identifier: AGPL-3.0-only
 
+import { assert } from 'chai';
+
+import { getRandomBytes } from '../../Crypto';
+import AccountManager from '../../textsecure/AccountManager';
+import { OuterSignedPrekeyType } from '../../textsecure/Types.d';
+
+/* eslint-disable @typescript-eslint/no-explicit-any */
+
 describe('AccountManager', () => {
-  let accountManager;
+  let accountManager: AccountManager;
 
   beforeEach(() => {
-    accountManager = new window.textsecure.AccountManager();
+    const server: any = {};
+    accountManager = new AccountManager(server);
   });
 
   describe('#cleanSignedPreKeys', () => {
-    let originalProtocolStorage;
+    let originalGetIdentityKeyPair: any;
-    let signedPreKeys;
+    let originalLoadSignedPreKeys: any;
+    let originalRemoveSignedPreKey: any;
+    let signedPreKeys: Array<OuterSignedPrekeyType>;
     const DAY = 1000 * 60 * 60 * 24;
 
+    const pubKey = getRandomBytes(33);
+    const privKey = getRandomBytes(32);
+
     beforeEach(async () => {
       const identityKey = window.Signal.Curve.generateKeyPair();
 
-      originalProtocolStorage = window.textsecure.storage.protocol;
+      originalGetIdentityKeyPair =
-      window.textsecure.storage.protocol = {
+        window.textsecure.storage.protocol.getIdentityKeyPair;
-        getIdentityKeyPair() {
+      originalLoadSignedPreKeys =
-          return identityKey;
+        window.textsecure.storage.protocol.loadSignedPreKeys;
-        },
+      originalRemoveSignedPreKey =
-        loadSignedPreKeys() {
+        window.textsecure.storage.protocol.removeSignedPreKey;
-          return Promise.resolve(signedPreKeys);
+
-        },
+      window.textsecure.storage.protocol.getIdentityKeyPair = async () =>
-      };
+        identityKey;
+      window.textsecure.storage.protocol.loadSignedPreKeys = async () =>
+        signedPreKeys;
     });
     afterEach(() => {
-      window.textsecure.storage.protocol = originalProtocolStorage;
+      window.textsecure.storage.protocol.getIdentityKeyPair = originalGetIdentityKeyPair;
+      window.textsecure.storage.protocol.loadSignedPreKeys = originalLoadSignedPreKeys;
+      window.textsecure.storage.protocol.removeSignedPreKey = originalRemoveSignedPreKey;
     });
 
     describe('encrypted device name', () => {
       it('roundtrips', async () => {
         const deviceName = 'v2.5.0 on Ubunto 20.04';
         const encrypted = await accountManager.encryptDeviceName(deviceName);
+        if (!encrypted) {
+          throw new Error('failed to encrypt!');
+        }
         assert.strictEqual(typeof encrypted, 'string');
         const decrypted = await accountManager.decryptDeviceName(encrypted);
 
         assert.strictEqual(decrypted, deviceName);
       });
 
-      it('handles null deviceName', async () => {
+      it('handles falsey deviceName', async () => {
-        const encrypted = await accountManager.encryptDeviceName(null);
+        const encrypted = await accountManager.encryptDeviceName('');
         assert.strictEqual(encrypted, null);
       });
     });
@@ -53,16 +74,22 @@ describe('AccountManager', () => {
           keyId: 1,
           created_at: now - DAY * 32,
           confirmed: true,
+          pubKey,
+          privKey,
         },
         {
           keyId: 2,
           created_at: now - DAY * 34,
           confirmed: true,
+          pubKey,
+          privKey,
         },
         {
           keyId: 3,
           created_at: now - DAY * 38,
           confirmed: true,
+          pubKey,
+          privKey,
         },
       ];
 
@@ -70,73 +97,57 @@ describe('AccountManager', () => {
       return accountManager.cleanSignedPreKeys();
     });
 
-    it('eliminates confirmed keys over a month old, if more than three', async () => {
+    it('eliminates oldest keys, even if recent key is unconfirmed', async () => {
       const now = Date.now();
       signedPreKeys = [
         {
           keyId: 1,
           created_at: now - DAY * 32,
           confirmed: true,
+          pubKey,
+          privKey,
         },
         {
           keyId: 2,
           created_at: now - DAY * 31,
-          confirmed: true,
+          confirmed: false,
+          pubKey,
+          privKey,
         },
         {
           keyId: 3,
           created_at: now - DAY * 24,
           confirmed: true,
+          pubKey,
+          privKey,
         },
         {
+          // Oldest, should be dropped
           keyId: 4,
           created_at: now - DAY * 38,
           confirmed: true,
+          pubKey,
+          privKey,
         },
         {
           keyId: 5,
           created_at: now - DAY,
           confirmed: true,
+          pubKey,
+          privKey,
+        },
+        {
+          keyId: 6,
+          created_at: now - DAY * 5,
+          confirmed: true,
+          pubKey,
+          privKey,
         },
       ];
 
       let count = 0;
-      window.textsecure.storage.protocol.removeSignedPreKey = keyId => {
+      window.textsecure.storage.protocol.removeSignedPreKey = async keyId => {
-        if (keyId !== 1 && keyId !== 4) {
+        if (keyId !== 4) {
-          throw new Error(`Wrong keys were eliminated! ${keyId}`);
-        }
-
-        count += 1;
-      };
-
-      await accountManager.cleanSignedPreKeys();
-      assert.strictEqual(count, 2);
-    });
-
-    it('keeps at least three unconfirmed keys if no confirmed', async () => {
-      const now = Date.now();
-      signedPreKeys = [
-        {
-          keyId: 1,
-          created_at: now - DAY * 32,
-        },
-        {
-          keyId: 2,
-          created_at: now - DAY * 44,
-        },
-        {
-          keyId: 3,
-          created_at: now - DAY * 36,
-        },
-        {
-          keyId: 4,
-          created_at: now - DAY * 20,
-        },
-      ];
-
-      let count = 0;
-      window.textsecure.storage.protocol.removeSignedPreKey = keyId => {
-        if (keyId !== 2) {
           throw new Error(`Wrong keys were eliminated! ${keyId}`);
         }
 
@@ -147,40 +158,44 @@ describe('AccountManager', () => {
       assert.strictEqual(count, 1);
     });
 
-    it('if some confirmed keys, keeps unconfirmed to addd up to three total', async () => {
+    it('Removes no keys if less than five', async () => {
       const now = Date.now();
       signedPreKeys = [
         {
           keyId: 1,
           created_at: now - DAY * 32,
           confirmed: true,
+          pubKey,
+          privKey,
         },
         {
           keyId: 2,
           created_at: now - DAY * 44,
           confirmed: true,
+          pubKey,
+          privKey,
         },
         {
           keyId: 3,
           created_at: now - DAY * 36,
+          confirmed: false,
+          pubKey,
+          privKey,
         },
         {
           keyId: 4,
           created_at: now - DAY * 20,
+          confirmed: false,
+          pubKey,
+          privKey,
         },
       ];
 
-      let count = 0;
+      window.textsecure.storage.protocol.removeSignedPreKey = async () => {
-      window.textsecure.storage.protocol.removeSignedPreKey = keyId => {
+        throw new Error('None should be removed!');
-        if (keyId !== 3) {
-          throw new Error(`Wrong keys were eliminated! ${keyId}`);
-        }
-
-        count += 1;
       };
 
       await accountManager.cleanSignedPreKeys();
-      assert.strictEqual(count, 1);
     });
   });
 });

ts/textsecure/AccountManager.ts

@@ -33,8 +33,10 @@ import { assert } from '../util/assert';
 import { getProvisioningUrl } from '../util/getProvisioningUrl';
 import { SignalService as Proto } from '../protobuf';
 
-const ARCHIVE_AGE = 30 * 24 * 60 * 60 * 1000;
+const DAY = 24 * 60 * 60 * 1000;
-const PREKEY_ROTATION_AGE = 24 * 60 * 60 * 1000;
+const MINIMUM_SIGNED_PREKEYS = 5;
+const ARCHIVE_AGE = 30 * DAY;
+const PREKEY_ROTATION_AGE = DAY;
 const PROFILE_KEY_LENGTH = 32;
 const SIGNED_KEY_GEN_BATCH_SIZE = 100;
 
@@ -321,13 +323,14 @@ export default class AccountManager extends EventTarget {
       const existingKeys = await store.loadSignedPreKeys();
       existingKeys.sort((a, b) => (b.created_at || 0) - (a.created_at || 0));
       const confirmedKeys = existingKeys.filter(key => key.confirmed);
+      const mostRecent = confirmedKeys[0];
 
       if (
-        confirmedKeys.length >= 3 &&
+        confirmedKeys.length >= 2 ||
-        isMoreRecentThan(confirmedKeys[0].created_at, PREKEY_ROTATION_AGE)
+        isMoreRecentThan(mostRecent?.created_at || 0, PREKEY_ROTATION_AGE)
       ) {
         window.log.warn(
-          'rotateSignedPreKey: 3+ confirmed keys, most recent is less than a day old. Cancelling rotation.'
+          `rotateSignedPreKey: ${confirmedKeys.length} confirmed keys, most recent was created ${mostRecent?.created_at}. Cancelling rotation.`
         );
         return;
       }
@@ -411,71 +414,49 @@ export default class AccountManager extends EventTarget {
   }
 
   async cleanSignedPreKeys() {
-    const MINIMUM_KEYS = 3;
     const store = window.textsecure.storage.protocol;
-    return store.loadSignedPreKeys().then(async allKeys => {
-      allKeys.sort((a, b) => (b.created_at || 0) - (a.created_at || 0));
-      const confirmed = allKeys.filter(key => key.confirmed);
-      const unconfirmed = allKeys.filter(key => !key.confirmed);
 
-      const recent = allKeys[0] ? allKeys[0].keyId : 'none';
+    const allKeys = await store.loadSignedPreKeys();
-      const recentConfirmed = confirmed[0] ? confirmed[0].keyId : 'none';
+    allKeys.sort((a, b) => (b.created_at || 0) - (a.created_at || 0));
-      window.log.info(`Most recent signed key: ${recent}`);
+    const confirmed = allKeys.filter(key => key.confirmed);
-      window.log.info(`Most recent confirmed signed key: ${recentConfirmed}`);
+    const unconfirmed = allKeys.filter(key => !key.confirmed);
-      window.log.info(
-        'Total signed key count:',
-        allKeys.length,
-        '-',
-        confirmed.length,
-        'confirmed'
-      );
 
-      let confirmedCount = confirmed.length;
+    const recent = allKeys[0] ? allKeys[0].keyId : 'none';
+    const recentConfirmed = confirmed[0] ? confirmed[0].keyId : 'none';
+    const recentUnconfirmed = unconfirmed[0] ? unconfirmed[0].keyId : 'none';
+    window.log.info(`cleanSignedPreKeys: Most recent signed key: ${recent}`);
+    window.log.info(
+      `cleanSignedPreKeys: Most recent confirmed signed key: ${recentConfirmed}`
+    );
+    window.log.info(
+      `cleanSignedPreKeys: Most recent unconfirmed signed key: ${recentUnconfirmed}`
+    );
+    window.log.info(
+      'cleanSignedPreKeys: Total signed key count:',
+      allKeys.length,
+      '-',
+      confirmed.length,
+      'confirmed'
+    );
 
-      // Keep MINIMUM_KEYS confirmed keys, then drop if older than a week
+    // Keep MINIMUM_SIGNED_PREKEYS keys, then drop if older than ARCHIVE_AGE
-      await Promise.all(
+    await Promise.all(
-        confirmed.map(async (key, index) => {
+      allKeys.map(async (key, index) => {
-          if (index < MINIMUM_KEYS) {
+        if (index < MINIMUM_SIGNED_PREKEYS) {
-            return;
+          return;
-          }
+        }
-          const createdAt = key.created_at || 0;
+        const createdAt = key.created_at || 0;
 
-          if (isOlderThan(createdAt, ARCHIVE_AGE)) {
+        if (isOlderThan(createdAt, ARCHIVE_AGE)) {
-            window.log.info(
+          const timestamp = new Date(createdAt).toJSON();
-              'Removing confirmed signed prekey:',
+          const confirmedText = key.confirmed ? ' (confirmed)' : '';
-              key.keyId,
+          window.log.info(
-              'with timestamp:',
+            `Removing signed prekey: ${key.keyId} with timestamp ${timestamp}${confirmedText}`
-              new Date(createdAt).toJSON()
+          );
-            );
+          await store.removeSignedPreKey(key.keyId);
-            await store.removeSignedPreKey(key.keyId);
+        }
-            confirmedCount -= 1;
+      })
-          }
+    );
-        })
-      );
-
-      const stillNeeded = MINIMUM_KEYS - confirmedCount;
-
-      // If we still don't have enough total keys, we keep as many unconfirmed
-      // keys as necessary. If not necessary, and over a week old, we drop.
-      await Promise.all(
-        unconfirmed.map(async (key, index) => {
-          if (index < stillNeeded) {
-            return;
-          }
-
-          const createdAt = key.created_at || 0;
-          if (isOlderThan(createdAt, ARCHIVE_AGE)) {
-            window.log.info(
-              'Removing unconfirmed signed prekey:',
-              key.keyId,
-              'with timestamp:',
-              new Date(createdAt).toJSON()
-            );
-            await store.removeSignedPreKey(key.keyId);
-          }
-        })
-      );
-    });
   }
 
   async createAccount(