From b515cdbdbbe21d481409c677af365a5315fb6390 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Fri, 1 Dec 2023 14:39:04 +0000 Subject: [PATCH] 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 | 48 +++++++++ spec/integ/crypto/crypto.spec.ts | 123 +++++----------------- spec/test-utils/AccountDataAccumulator.ts | 108 +++++++++++++++++++ src/rust-crypto/CrossSigningIdentity.ts | 42 +++++--- src/rust-crypto/rust-crypto.ts | 3 + 5 files changed, 213 insertions(+), 111 deletions(-) create mode 100644 spec/test-utils/AccountDataAccumulator.ts diff --git a/spec/integ/crypto/cross-signing.spec.ts b/spec/integ/crypto/cross-signing.spec.ts index c31b7d4ad9..a06efd2062 100644 --- a/spec/integ/crypto/cross-signing.spec.ts +++ b/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()", () => { diff --git a/spec/integ/crypto/crypto.spec.ts b/spec/integ/crypto/crypto.spec.ts index 1ef22bf572..5992cc8efe 100644 --- a/spec/integ/crypto/crypto.spec.ts +++ b/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", () => { - /** - * 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; + let accountDataAccumulator: AccountDataAccumulator; /** * 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> { - return new Promise((resolve) => { - // Called when the cross signing key is uploaded - 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. - } + async function awaitCrossSigningKeyUpload(key: string): Promise> { + const content = await accountDataAccumulator.interceptSetAccountData(`m.cross_signing.${key}`); + return content.encrypted; } /** @@ -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 { - return new Promise((resolve) => { - // This url is called multiple times during the secret storage bootstrap process - // When we received the newly generated key, we return it - fetchMock.put( - "express:/_matrix/client/v3/user/:userId/account_data/:type(m.secret_storage.*)", - (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 }, - ); - }); + async function awaitSecretStorageKeyStoredInAccountData(): Promise { + // eslint-disable-next-line no-constant-condition + while (true) { + const content = await accountDataAccumulator.interceptSetAccountData(":type(m.secret_storage.*)", { + repeat: 1, + overwriteRoutes: true, + }); + accountDataAccumulator.sendSyncResponseWithUpdatedAccountData(syncResponder); + if (content.key) { + return content.key; + } + } } function awaitMegolmBackupKeyUpload(): Promise> { @@ -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([ diff --git a/spec/test-utils/AccountDataAccumulator.ts b/spec/test-utils/AccountDataAccumulator.ts new file mode 100644 index 0000000000..dca35bc2cc --- /dev/null +++ b/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 = 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 { + 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. + } + } +} diff --git a/src/rust-crypto/CrossSigningIdentity.ts b/src/rust-crypto/CrossSigningIdentity.ts index c0bccec109..7a994a8d04 100644 --- a/src/rust-crypto/CrossSigningIdentity.ts +++ b/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 (await this.secretStorage.hasKey()) { + // If 4S is configured we need to update it. + 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`); } diff --git a/src/rust-crypto/rust-crypto.ts b/src/rust-crypto/rust-crypto.ts index fe8b801462..de7b368b4e 100644 --- a/src/rust-crypto/rust-crypto.ts +++ b/src/rust-crypto/rust-crypto.ts @@ -769,6 +769,7 @@ export class RustCrypto extends TypedEventEmitter