Rust-crypto: fix bootstrapCrossSigning on second call (#3912)

* Rust-crypto: fix `bootstrapCrossSigning` on second call

Currently, `bootstrapCrossSigning` raises an exception if it is called a second
time before secret storage is set up. It is easily fixed by checking that 4S is
set up before trying to export to 4S.

Also a few logging fixes while we're in the area.

* Factor out an `AccountDataAccumulator`

* Another test for bootstrapCrossSigning

spec/integ/crypto/cross-signing.spec.ts

@@ -36,6 +36,7 @@ import {
 } from "../../test-utils/test-data";
 import * as testData from "../../test-utils/test-data";
 import { E2EKeyResponder } from "../../test-utils/E2EKeyResponder";
+import { AccountDataAccumulator } from "../../test-utils/AccountDataAccumulator";
 
 afterEach(() => {
     // reset fake-indexeddb after each test, to make sure we don't leak connections
@@ -244,6 +245,53 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("cross-signing (%s)", (backend: s
                 `[${TEST_USER_ID}].[${TEST_DEVICE_ID}].signatures.[${TEST_USER_ID}].[ed25519:${SELF_CROSS_SIGNING_PUBLIC_KEY_BASE64}]`,
             );
         });
+
+        it("can bootstrapCrossSigning twice", async () => {
+            mockSetupCrossSigningRequests();
+
+            const authDict = { type: "test" };
+            await bootstrapCrossSigning(authDict);
+
+            // a second call should do nothing except GET requests
+            fetchMock.mockClear();
+            await bootstrapCrossSigning(authDict);
+            const calls = fetchMock.calls((url, opts) => opts.method != "GET");
+            expect(calls.length).toEqual(0);
+        });
+
+        newBackendOnly("will upload existing cross-signing keys to an established secret storage", async () => {
+            // This rather obscure codepath covers the case that:
+            //   - 4S is set up and working
+            //   - our device has private cross-signing keys, but has not published them to 4S
+            //
+            // To arrange that, we call `bootstrapCrossSigning` on our main device, and then (pretend to) set up 4S from
+            // a *different* device. Then, when we call `bootstrapCrossSigning` again, it should do the honours.
+
+            mockSetupCrossSigningRequests();
+            const accountDataAccumulator = new AccountDataAccumulator();
+            accountDataAccumulator.interceptGetAccountData();
+
+            const authDict = { type: "test" };
+            await bootstrapCrossSigning(authDict);
+
+            // Pretend that another device has uploaded a 4S key
+            accountDataAccumulator.accountDataEvents.set("m.secret_storage.default_key", { key: "key_id" });
+            accountDataAccumulator.accountDataEvents.set("m.secret_storage.key.key_id", {
+                key: "keykeykey",
+                algorithm: SECRET_STORAGE_ALGORITHM_V1_AES,
+            });
+
+            // Prepare for the cross-signing keys
+            const p = accountDataAccumulator.interceptSetAccountData(":type(m.cross_signing..*)");
+
+            await bootstrapCrossSigning(authDict);
+            await p;
+
+            // The cross-signing keys should have been uploaded
+            expect(accountDataAccumulator.accountDataEvents.has("m.cross_signing.master")).toBeTruthy();
+            expect(accountDataAccumulator.accountDataEvents.has("m.cross_signing.self_signing")).toBeTruthy();
+            expect(accountDataAccumulator.accountDataEvents.has("m.cross_signing.user_signing")).toBeTruthy();
+        });
     });
 
     describe("getCrossSigningStatus()", () => {

spec/integ/crypto/crypto.spec.ts

@@ -96,6 +96,7 @@ import {
     getTestOlmAccountKeys,
 } from "./olm-utils";
 import { ToDevicePayload } from "../../../src/models/ToDeviceMessage";
+import { AccountDataAccumulator } from "../../test-utils/AccountDataAccumulator";
 
 afterEach(() => {
     // reset fake-indexeddb after each test, to make sure we don't leak connections
@@ -2438,12 +2439,7 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("crypto (%s)", (backend: string,
     });
 
     describe("Secret Storage and Key Backup", () => {
-        /**
+        let accountDataAccumulator: AccountDataAccumulator;
-         * The account data events to be returned by the sync.
-         * Will be updated when fecthMock intercepts calls to PUT `/_matrix/client/v3/user/:userId/account_data/`.
-         * Will be used by `sendSyncResponseWithUpdatedAccountData`
-         */
-        let accountDataEvents: Map<String, any>;
 
         /**
          * Create a fake secret storage key
@@ -2456,76 +2452,19 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("crypto (%s)", (backend: string,
 
         beforeEach(async () => {
             createSecretStorageKey.mockClear();
-            accountDataEvents = new Map();
+            accountDataAccumulator = new AccountDataAccumulator();
             expectAliceKeyQuery({ device_keys: { "@alice:localhost": {} }, failures: {} });
             await startClientAndAwaitFirstSync();
         });
 
-        function mockGetAccountData() {
-            fetchMock.get(
-                `path:/_matrix/client/v3/user/:userId/account_data/:type`,
-                (url) => {
-                    const type = url.split("/").pop();
-                    const existing = accountDataEvents.get(type!);
-                    if (existing) {
-                        // return it
-                        return {
-                            status: 200,
-                            body: existing.content,
-                        };
-                    } else {
-                        // 404
-                        return {
-                            status: 404,
-                            body: { errcode: "M_NOT_FOUND", error: "Account data not found." },
-                        };
-                    }
-                },
-                { overwriteRoutes: true },
-            );
-        }
-
         /**
          * Create a mock to respond to the PUT request `/_matrix/client/v3/user/:userId/account_data/m.cross_signing.${key}`
          * Resolved when the cross signing key is uploaded
          * https://spec.matrix.org/v1.6/client-server-api/#put_matrixclientv3useruseridaccount_datatype
          */
-        function awaitCrossSigningKeyUpload(key: string): Promise<Record<string, {}>> {
+        async function awaitCrossSigningKeyUpload(key: string): Promise<Record<string, {}>> {
-            return new Promise((resolve) => {
+            const content = await accountDataAccumulator.interceptSetAccountData(`m.cross_signing.${key}`);
-                // Called when the cross signing key is uploaded
+            return content.encrypted;
-                fetchMock.put(
-                    `express:/_matrix/client/v3/user/:userId/account_data/m.cross_signing.${key}`,
-                    (url: string, options: RequestInit) => {
-                        const content = JSON.parse(options.body as string);
-                        const type = url.split("/").pop();
-                        // update account data for sync response
-                        accountDataEvents.set(type!, content);
-                        resolve(content.encrypted);
-                        return {};
-                    },
-                );
-            });
-        }
-
-        /**
-         * Send in the sync response the current account data events, as stored by `accountDataEvents`.
-         */
-        function sendSyncResponseWithUpdatedAccountData() {
-            try {
-                syncResponder.sendOrQueueSyncResponse({
-                    next_batch: 1,
-                    account_data: {
-                        events: Array.from(accountDataEvents, ([type, content]) => ({
-                            type: type,
-                            content: content,
-                        })),
-                    },
-                });
-            } catch (err) {
-                // Might fail with "Cannot queue more than one /sync response" if called too often.
-                // It's ok if it fails here, the sync response is cumulative and will contain
-                // the latest account data.
-            }
         }
 
         /**
@@ -2533,28 +2472,18 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("crypto (%s)", (backend: string,
          * Resolved when a key is uploaded (ie in `body.content.key`)
          * https://spec.matrix.org/v1.6/client-server-api/#put_matrixclientv3useruseridaccount_datatype
          */
-        function awaitSecretStorageKeyStoredInAccountData(): Promise<string> {
+        async function awaitSecretStorageKeyStoredInAccountData(): Promise<string> {
-            return new Promise((resolve) => {
+            // eslint-disable-next-line no-constant-condition
-                // This url is called multiple times during the secret storage bootstrap process
+            while (true) {
-                // When we received the newly generated key, we return it
+                const content = await accountDataAccumulator.interceptSetAccountData(":type(m.secret_storage.*)", {
-                fetchMock.put(
+                    repeat: 1,
-                    "express:/_matrix/client/v3/user/:userId/account_data/:type(m.secret_storage.*)",
+                    overwriteRoutes: true,
-                    (url: string, options: RequestInit) => {
-                        const type = url.split("/").pop();
-                        const content = JSON.parse(options.body as string);
-
-                        // update account data for sync response
-                        accountDataEvents.set(type!, content);
-
-                        if (content.key) {
-                            resolve(content.key);
-                        }
-                        sendSyncResponseWithUpdatedAccountData();
-                        return {};
-                    },
-                    { overwriteRoutes: true },
-                );
                 });
+                accountDataAccumulator.sendSyncResponseWithUpdatedAccountData(syncResponder);
+                if (content.key) {
+                    return content.key;
+                }
+            }
         }
 
         function awaitMegolmBackupKeyUpload(): Promise<Record<string, {}>> {
@@ -2565,7 +2494,7 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("crypto (%s)", (backend: string,
                     (url: string, options: RequestInit) => {
                         const content = JSON.parse(options.body as string);
                         // update account data for sync response
-                        accountDataEvents.set("m.megolm_backup.v1", content);
+                        accountDataAccumulator.accountDataEvents.set("m.megolm_backup.v1", content);
                         resolve(content.encrypted);
                         return {};
                     },
@@ -2630,7 +2559,7 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("crypto (%s)", (backend: string,
             await bootstrapPromise;
 
             // Return the newly created key in the sync response
-            sendSyncResponseWithUpdatedAccountData();
+            accountDataAccumulator.sendSyncResponseWithUpdatedAccountData(syncResponder);
 
             // Finally ensure backup is working
             await aliceClient.getCrypto()!.checkKeyBackupAndEnable();
@@ -2652,7 +2581,7 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("crypto (%s)", (backend: string,
             );
 
             it("Should create a 4S key", async () => {
-                mockGetAccountData();
+                accountDataAccumulator.interceptGetAccountData();
 
                 const awaitAccountData = awaitAccountDataUpdate("m.secret_storage.default_key");
 
@@ -2664,7 +2593,7 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("crypto (%s)", (backend: string,
                 const secretStorageKey = await awaitSecretStorageKeyStoredInAccountData();
 
                 // Return the newly created key in the sync response
-                sendSyncResponseWithUpdatedAccountData();
+                accountDataAccumulator.sendSyncResponseWithUpdatedAccountData(syncResponder);
 
                 // Finally, wait for bootstrapSecretStorage to finished
                 await bootstrapPromise;
@@ -2688,7 +2617,7 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("crypto (%s)", (backend: string,
                 await awaitSecretStorageKeyStoredInAccountData();
 
                 // Return the newly created key in the sync response
-                sendSyncResponseWithUpdatedAccountData();
+                accountDataAccumulator.sendSyncResponseWithUpdatedAccountData(syncResponder);
 
                 // Wait for bootstrapSecretStorage to finished
                 await bootstrapPromise;
@@ -2712,7 +2641,7 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("crypto (%s)", (backend: string,
                 await awaitSecretStorageKeyStoredInAccountData();
 
                 // Return the newly created key in the sync response
-                sendSyncResponseWithUpdatedAccountData();
+                accountDataAccumulator.sendSyncResponseWithUpdatedAccountData(syncResponder);
 
                 // Wait for bootstrapSecretStorage to finished
                 await bootstrapPromise;
@@ -2726,7 +2655,7 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("crypto (%s)", (backend: string,
                 await awaitSecretStorageKeyStoredInAccountData();
 
                 // Return the newly created key in the sync response
-                sendSyncResponseWithUpdatedAccountData();
+                accountDataAccumulator.sendSyncResponseWithUpdatedAccountData(syncResponder);
 
                 // Wait for bootstrapSecretStorage to finished
                 await bootstrapPromise;
@@ -2750,7 +2679,7 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("crypto (%s)", (backend: string,
                 const secretStorageKey = await awaitSecretStorageKeyStoredInAccountData();
 
                 // Return the newly created key in the sync response
-                sendSyncResponseWithUpdatedAccountData();
+                accountDataAccumulator.sendSyncResponseWithUpdatedAccountData(syncResponder);
 
                 // Wait for the cross signing keys to be uploaded
                 const [masterKey, userSigningKey, selfSigningKey] = await Promise.all([

spec/test-utils/AccountDataAccumulator.ts

@@ -0,0 +1,108 @@
+/*
+Copyright 2023 The Matrix.org Foundation C.I.C.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+import fetchMock from "fetch-mock-jest";
+import { MockOptionsMethodPut } from "fetch-mock";
+
+import { ISyncResponder } from "./SyncResponder";
+
+/**
+ *  An object which intercepts `account_data` get and set requests via fetch-mock.
+ */
+export class AccountDataAccumulator {
+    /**
+     * The account data events to be returned by the sync.
+     * Will be updated when fetchMock intercepts calls to PUT `/_matrix/client/v3/user/:userId/account_data/`.
+     * Will be used by `sendSyncResponseWithUpdatedAccountData`
+     */
+    public accountDataEvents: Map<String, any> = new Map();
+
+    /**
+     * Intercept requests to set a particular type of account data.
+     *
+     * Once it is set, its data is stored (for future return by `interceptGetAccountData` etc) and the resolved promise is
+     * resolved.
+     *
+     * @param accountDataType - type of account data to be intercepted
+     * @param opts - options to pass to fetchMock
+     * @returns a Promise which will resolve (with the content of the account data) once it is set.
+     */
+    public interceptSetAccountData(accountDataType: string, opts?: MockOptionsMethodPut): Promise<any> {
+        return new Promise((resolve) => {
+            // Called when the cross signing key is uploaded
+            fetchMock.put(
+                `express:/_matrix/client/v3/user/:userId/account_data/${accountDataType}`,
+                (url: string, options: RequestInit) => {
+                    const content = JSON.parse(options.body as string);
+                    const type = url.split("/").pop();
+                    // update account data for sync response
+                    this.accountDataEvents.set(type!, content);
+                    resolve(content);
+                    return {};
+                },
+                opts,
+            );
+        });
+    }
+
+    /**
+     * Intercept all requests to get account data
+     */
+    public interceptGetAccountData(): void {
+        fetchMock.get(
+            `express:/_matrix/client/v3/user/:userId/account_data/:type`,
+            (url) => {
+                const type = url.split("/").pop();
+                const existing = this.accountDataEvents.get(type!);
+                if (existing) {
+                    // return it
+                    return {
+                        status: 200,
+                        body: existing,
+                    };
+                } else {
+                    // 404
+                    return {
+                        status: 404,
+                        body: { errcode: "M_NOT_FOUND", error: "Account data not found." },
+                    };
+                }
+            },
+            { overwriteRoutes: true },
+        );
+    }
+
+    /**
+     * Send a sync response the current account data events.
+     */
+    public sendSyncResponseWithUpdatedAccountData(syncResponder: ISyncResponder): void {
+        try {
+            syncResponder.sendOrQueueSyncResponse({
+                next_batch: 1,
+                account_data: {
+                    events: Array.from(this.accountDataEvents, ([type, content]) => ({
+                        type: type,
+                        content: content,
+                    })),
+                },
+            });
+        } catch (err) {
+            // Might fail with "Cannot queue more than one /sync response" if called too often.
+            // It's ok if it fails here, the sync response is cumulative and will contain
+            // the latest account data.
+        }
+    }
+}

src/rust-crypto/CrossSigningIdentity.ts

@@ -57,7 +57,7 @@ export class CrossSigningIdentity {
             olmDeviceStatus.hasMaster && olmDeviceStatus.hasUserSigning && olmDeviceStatus.hasSelfSigning;
 
         // Log all relevant state for easier parsing of debug logs.
-        logger.log("bootStrapCrossSigning: starting", {
+        logger.log("bootstrapCrossSigning: starting", {
             setupNewCrossSigning: opts.setupNewCrossSigning,
             olmDeviceHasMaster: olmDeviceStatus.hasMaster,
             olmDeviceHasUserSigning: olmDeviceStatus.hasUserSigning,
@@ -66,18 +66,25 @@ export class CrossSigningIdentity {
         });
 
         if (olmDeviceHasKeys) {
-            if (!privateKeysInSecretStorage) {
+            if (!(await this.secretStorage.hasKey())) {
+                logger.warn(
+                    "bootstrapCrossSigning: Olm device has private keys, but secret storage is not yet set up; doing nothing for now.",
+                );
+                // the keys should get uploaded to 4S once that is set up.
+            } else if (!privateKeysInSecretStorage) {
                 // the device has the keys but they are not in 4S, so update it
-                logger.log("bootStrapCrossSigning: Olm device has private keys: exporting to secret storage");
+                logger.log("bootstrapCrossSigning: Olm device has private keys: exporting to secret storage");
                 await this.exportCrossSigningKeysToStorage();
             } else {
-                logger.log("bootStrapCrossSigning: Olm device has private keys and they are saved in 4S, do nothing");
+                logger.log(
+                    "bootstrapCrossSigning: Olm device has private keys and they are saved in secret storage; doing nothing",
+                );
             }
         } /* (!olmDeviceHasKeys) */ else {
             if (privateKeysInSecretStorage) {
                 // they are in 4S, so import from there
                 logger.log(
-                    "bootStrapCrossSigning: Cross-signing private keys not found locally, but they are available " +
+                    "bootstrapCrossSigning: Cross-signing private keys not found locally, but they are available " +
                         "in secret storage, reading storage and caching locally",
                 );
                 await this.olmMachine.importCrossSigningKeys(
@@ -100,7 +107,7 @@ export class CrossSigningIdentity {
                 }
             } else {
                 logger.log(
-                    "bootStrapCrossSigning: Cross-signing private keys not found locally or in secret storage, creating new keys",
+                    "bootstrapCrossSigning: Cross-signing private keys not found locally or in secret storage, creating new keys",
                 );
                 await this.resetCrossSigning(opts.authUploadDeviceSigningKeys);
             }
@@ -108,7 +115,7 @@ export class CrossSigningIdentity {
 
         // TODO: we might previously have bootstrapped cross-signing but not completed uploading the keys to the
         //   server -- in which case we should call OlmDevice.bootstrap_cross_signing. How do we know?
-        logger.log("bootStrapCrossSigning: complete");
+        logger.log("bootstrapCrossSigning: complete");
     }
 
     /** Reset our cross-signing keys
@@ -123,14 +130,21 @@ export class CrossSigningIdentity {
         // or 4S passphrase/key the process will fail in a bad state, with keys rotated but not uploaded or saved in 4S.
         const outgoingRequests: CrossSigningBootstrapRequests = await this.olmMachine.bootstrapCrossSigning(true);
 
-        // If 4S is configured we need to udpate it.
+        // If 4S is configured we need to update it.
-        if (await this.secretStorage.hasKey()) {
+        if (!(await this.secretStorage.hasKey())) {
+            logger.warn(
+                "resetCrossSigning: Secret storage is not yet set up; not exporting keys to secret storage yet.",
+            );
+            // the keys should get uploaded to 4S once that is set up.
+        } else {
             // Update 4S before uploading cross-signing keys, to stay consistent with legacy that asks
             // 4S passphrase before asking for account password.
-            // Ultimately should be made atomic and resistent to forgotten password/passphrase.
+            // Ultimately should be made atomic and resistant to forgotten password/passphrase.
+            logger.log("resetCrossSigning: exporting to secret storage");
+
             await this.exportCrossSigningKeysToStorage();
         }
-        logger.log("bootStrapCrossSigning: publishing keys to server");
+        logger.log("resetCrossSigning: publishing keys to server");
         for (const req of [
             outgoingRequests.uploadKeysRequest,
             outgoingRequests.uploadSigningKeysRequest,
@@ -151,17 +165,17 @@ export class CrossSigningIdentity {
         const exported: RustSdkCryptoJs.CrossSigningKeyExport | null = await this.olmMachine.exportCrossSigningKeys();
         /* istanbul ignore else (this function is only called when we know the olm machine has keys) */
         if (exported?.masterKey) {
-            this.secretStorage.store("m.cross_signing.master", exported.masterKey);
+            await this.secretStorage.store("m.cross_signing.master", exported.masterKey);
         } else {
             logger.error(`Cannot export MSK to secret storage, private key unknown`);
         }
         if (exported?.self_signing_key) {
-            this.secretStorage.store("m.cross_signing.self_signing", exported.self_signing_key);
+            await this.secretStorage.store("m.cross_signing.self_signing", exported.self_signing_key);
         } else {
             logger.error(`Cannot export SSK to secret storage, private key unknown`);
         }
         if (exported?.userSigningKey) {
-            this.secretStorage.store("m.cross_signing.user_signing", exported.userSigningKey);
+            await this.secretStorage.store("m.cross_signing.user_signing", exported.userSigningKey);
         } else {
             logger.error(`Cannot export USK to secret storage, private key unknown`);
         }

src/rust-crypto/rust-crypto.ts

@@ -769,6 +769,7 @@ export class RustCrypto extends TypedEventEmitter<RustCryptoEvents, RustCryptoEv
             }
 
             // Create a new storage key and add it to secret storage
+            this.logger.info("bootstrapSecretStorage: creating new secret storage key");
             const recoveryKey = await createSecretStorageKey();
             await this.addSecretStorageKeyToSecretStorage(recoveryKey);
         }
@@ -783,6 +784,8 @@ export class RustCrypto extends TypedEventEmitter<RustCryptoEvents, RustCryptoEv
             hasPrivateKeys &&
             (isNewSecretStorageKeyNeeded || !(await secretStorageContainsCrossSigningKeys(this.secretStorage)))
         ) {
+            this.logger.info("bootstrapSecretStorage: cross-signing keys not yet exported; doing so now.");
+
             const crossSigningPrivateKeys: RustSdkCryptoJs.CrossSigningKeyExport =
                 await this.olmMachine.exportCrossSigningKeys();