From 135c6e48fbfaf6c954e787b0b2f4726c301ad81d Mon Sep 17 00:00:00 2001 From: Valere Date: Wed, 4 Oct 2023 15:22:00 +0200 Subject: [PATCH 01/26] Handle backup secret gossip --- src/rust-crypto/index.ts | 4 ++++ src/rust-crypto/rust-crypto.ts | 42 +++++++++++++++++++++++++++++++++- 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/src/rust-crypto/index.ts b/src/rust-crypto/index.ts index 3288f66e58b..a0d65a01d55 100644 --- a/src/rust-crypto/index.ts +++ b/src/rust-crypto/index.ts @@ -73,6 +73,10 @@ export async function initRustCrypto( rustCrypto.onUserIdentityUpdated(userId), ); + await olmMachine.registerReceiveSecretCallback((name: string, base64: string) => + rustCrypto.onReceiveSecret(name, base64), + ); + // Tell the OlmMachine to think about its outgoing requests before we hand control back to the application. // // This is primarily a fudge to get it to correctly populate the `users_for_key_query` list, so that future diff --git a/src/rust-crypto/rust-crypto.ts b/src/rust-crypto/rust-crypto.ts index 22f6f70be03..371a59cc81a 100644 --- a/src/rust-crypto/rust-crypto.ts +++ b/src/rust-crypto/rust-crypto.ts @@ -70,7 +70,7 @@ import { TypedReEmitter } from "../ReEmitter"; import { randomString } from "../randomstring"; import { ClientStoppedError } from "../errors"; import { ISignatures } from "../@types/signed"; -import { encodeBase64 } from "../common-crypto/base64"; +import { decodeBase64, encodeBase64 } from "../common-crypto/base64"; import { DecryptionError } from "../crypto/algorithms"; const ALL_VERIFICATION_METHODS = ["m.sas.v1", "m.qr_code.scan.v1", "m.qr_code.show.v1", "m.reciprocate.v1"]; @@ -1350,6 +1350,46 @@ export class RustCrypto extends TypedEventEmitter { + logger.debug(`onReceiveSecret: Received secret ${name}`); + if (name === "m.megolm_backup.v1") { + // We need the current version, and it's a good time to check if trusted + const info = (await this.backupManager.checkKeyBackupAndEnable(true))?.backupInfo; + if (info?.version) { + const backupDecryptionKey = RustSdkCryptoJs.BackupDecryptionKey.fromBase64(base64); + + const authPublickey = (info.auth_data as Curve25519AuthData)?.public_key; + const backupMatchesSavedPrivateKey = + authPublickey === backupDecryptionKey.megolmV1PublicKey.publicKeyBase64; + + if (!backupMatchesSavedPrivateKey) { + logger.debug(`onReceiveSecret: backup decryption key does not match current backup version`); + // just ignore the secret + return; + } + + await this.storeSessionBackupPrivateKey(decodeBase64(base64), info.version); + // XXXX at this point we should probably try to download the backup and import the keys, + // or at least retry for the current decryption failures? + // Maybe add some signaling when a new secret is received, and let clients handle it? + // as it's where the restore from backup APIs are + } + } + } + /** * Handle a live event received via /sync. * See {@link ClientEventHandlerMap#event} From 3bdc74e1cf130e99d565a08afc15640187adf1c6 Mon Sep 17 00:00:00 2001 From: Valere Date: Thu, 5 Oct 2023 17:55:29 +0200 Subject: [PATCH 02/26] use getSecretsFromInbox --- src/rust-crypto/index.ts | 11 +++++++++-- src/rust-crypto/rust-crypto.ts | 6 +++--- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/src/rust-crypto/index.ts b/src/rust-crypto/index.ts index a0d65a01d55..e0eca086df9 100644 --- a/src/rust-crypto/index.ts +++ b/src/rust-crypto/index.ts @@ -73,8 +73,15 @@ export async function initRustCrypto( rustCrypto.onUserIdentityUpdated(userId), ); - await olmMachine.registerReceiveSecretCallback((name: string, base64: string) => - rustCrypto.onReceiveSecret(name, base64), + // check first if some secrets are pending for process. The registerReceiveSecretCallback will be + // triggered only for new secrets. + const pendingValues: string[] = await olmMachine.getSecretsFromInbox("m.megolm_backup.v1"); + pendingValues.forEach((value) => { + rustCrypto.onReceiveSecret("m.megolm_backup.v1", value); + }); + + await olmMachine.registerReceiveSecretCallback((name: string, value: string) => + rustCrypto.onReceiveSecret(name, value), ); // Tell the OlmMachine to think about its outgoing requests before we hand control back to the application. diff --git a/src/rust-crypto/rust-crypto.ts b/src/rust-crypto/rust-crypto.ts index 371a59cc81a..51c29865548 100644 --- a/src/rust-crypto/rust-crypto.ts +++ b/src/rust-crypto/rust-crypto.ts @@ -1363,13 +1363,13 @@ export class RustCrypto extends TypedEventEmitter { + public async onReceiveSecret(name: string, value: string): Promise { logger.debug(`onReceiveSecret: Received secret ${name}`); if (name === "m.megolm_backup.v1") { // We need the current version, and it's a good time to check if trusted const info = (await this.backupManager.checkKeyBackupAndEnable(true))?.backupInfo; if (info?.version) { - const backupDecryptionKey = RustSdkCryptoJs.BackupDecryptionKey.fromBase64(base64); + const backupDecryptionKey = RustSdkCryptoJs.BackupDecryptionKey.fromBase64(value); const authPublickey = (info.auth_data as Curve25519AuthData)?.public_key; const backupMatchesSavedPrivateKey = @@ -1381,7 +1381,7 @@ export class RustCrypto extends TypedEventEmitter Date: Mon, 9 Oct 2023 11:03:29 +0200 Subject: [PATCH 03/26] add gossip test --- spec/integ/crypto/olm-utils.ts | 125 +++++++++- spec/integ/crypto/verification.spec.ts | 327 ++++++++++++++++++++++++- 2 files changed, 448 insertions(+), 4 deletions(-) diff --git a/spec/integ/crypto/olm-utils.ts b/spec/integ/crypto/olm-utils.ts index 34e2e6df754..e14ce0d0088 100644 --- a/spec/integ/crypto/olm-utils.ts +++ b/spec/integ/crypto/olm-utils.ts @@ -17,10 +17,11 @@ limitations under the License. import Olm from "@matrix-org/olm"; import anotherjson from "another-json"; -import { IContent, IDeviceKeys, IEvent, MatrixClient } from "../../../src"; +import { IContent, IDeviceKeys, IDownloadKeyResult, IEvent, Keys, MatrixClient, SigningKeys } from "../../../src"; import { IE2EKeyReceiver } from "../../test-utils/E2EKeyReceiver"; import { ISyncResponder } from "../../test-utils/SyncResponder"; import { syncPromise } from "../../test-utils/test-utils"; +import { KeyBackupInfo } from "../../../src/crypto-api"; /** * @module @@ -60,6 +61,96 @@ export function getTestOlmAccountKeys(olmAccount: Olm.Account, userId: string, d return testDeviceKeys; } +export type BootstrapTestAccountData = { + /** A full key query response with cross-signing and device keys and all signatures */ + downloadKeys: Partial; + masterKeyOlmSigning: Olm.PkSigning; + masterKeyPublicKeyBase64: string; +}; + +export function bootstrapCrossSigningTestOlmAccount( + olmAccount: Olm.Account, + userId: string, + deviceId: string, + keyBackupInfo?: KeyBackupInfo, +): Partial { + const olmAliceMSK = new global.Olm.PkSigning(); + const masterPrivkey = olmAliceMSK.generate_seed(); + const masterPubkey = olmAliceMSK.init_with_seed(masterPrivkey); + + const olmAliceUSK = new global.Olm.PkSigning(); + const userPrivkey = olmAliceUSK.generate_seed(); + const userPubkey = olmAliceUSK.init_with_seed(userPrivkey); + + const olmAliceSSK = new global.Olm.PkSigning(); + const sskPrivkey = olmAliceSSK.generate_seed(); + const sskPubkey = olmAliceSSK.init_with_seed(sskPrivkey); + + const mskInfo: Keys = { + user_id: userId, + usage: ["master"], + keys: { + ["ed25519:" + masterPubkey]: masterPubkey, + }, + }; + + const sskInfo: Partial = { + user_id: userId, + usage: ["self_signing"], + keys: { + ["ed25519:" + sskPubkey]: sskPubkey, + }, + }; + const sskSig = olmAliceMSK.sign(anotherjson.stringify(sskInfo)); + sskInfo.signatures = { + [userId]: { + ["ed25519:" + masterPubkey]: sskSig, + }, + }; + + const uskInfo: Partial = { + user_id: userId, + usage: ["user_signing"], + keys: { + ["ed25519:" + userPubkey]: userPubkey, + }, + }; + const uskSig = olmAliceMSK.sign(anotherjson.stringify(uskInfo)); + uskInfo.signatures = { + [userId]: { + ["ed25519:" + masterPubkey]: uskSig, + }, + }; + + const deviceKeys = getTestOlmAccountKeys(olmAccount, userId, deviceId); + + const copy = Object.assign({}, deviceKeys); + delete copy.signatures; + const crossSignature = olmAliceSSK.sign(anotherjson.stringify(copy)); + + // add the signature + deviceKeys.signatures![userId]["ed25519:" + sskPubkey] = crossSignature; + + if (keyBackupInfo?.auth_data) { + const unsignedAuthData = Object.assign({}, keyBackupInfo?.auth_data); + delete unsignedAuthData.signatures; + const backupSignature = olmAliceMSK.sign(anotherjson.stringify(unsignedAuthData)); + + keyBackupInfo.auth_data.signatures = { + [userId]: { + ["ed25519:" + masterPubkey]: backupSignature, + }, + }; + } + + return { + master_keys: { [userId]: mskInfo }, + user_signing_keys: { [userId]: uskInfo as SigningKeys }, + self_signing_keys: { [userId]: sskInfo as SigningKeys }, + device_keys: { [userId]: { [deviceId]: deviceKeys } }, + }; +} + /** start an Olm session with a given recipient */ export async function createOlmSession( olmAccount: Olm.Account, @@ -218,6 +309,38 @@ export function encryptGroupSessionKey(opts: { }); } +export function encryptSecretSend(opts: { + sender: string; + /** recipient's user id */ + recipient: string; + /** the recipient's curve25519 key */ + recipientCurve25519Key: string; + /** the recipient's ed25519 key */ + recipientEd25519Key: string; + /** sender's olm account */ + olmAccount: Olm.Account; + /** sender's olm session with the recipient */ + p2pSession: Olm.Session; + requestId: string; + secret: string; +}): ToDeviceEvent { + const senderKeys = JSON.parse(opts.olmAccount.identity_keys()); + return encryptOlmEvent({ + sender: opts.sender, + senderKey: senderKeys.curve25519, + senderSigningKey: senderKeys.ed25519, + recipient: opts.recipient, + recipientCurve25519Key: opts.recipientCurve25519Key, + recipientEd25519Key: opts.recipientEd25519Key, + p2pSession: opts.p2pSession, + plaincontent: { + request_id: opts.requestId, + secret: opts.secret, + }, + plaintype: "m.secret.send", + }); +} + /** * Establish an Olm Session with the test user * diff --git a/spec/integ/crypto/verification.spec.ts b/spec/integ/crypto/verification.spec.ts index 84be643278f..817edc68a9e 100644 --- a/spec/integ/crypto/verification.spec.ts +++ b/spec/integ/crypto/verification.spec.ts @@ -26,6 +26,7 @@ import Olm from "@matrix-org/olm"; import { createClient, CryptoEvent, + DeviceVerification, IContent, ICreateClientOpts, IEvent, @@ -54,10 +55,12 @@ import { } from "../../test-utils/test-utils"; import { SyncResponder } from "../../test-utils/SyncResponder"; import { + BACKUP_DECRYPTION_KEY_BASE64, BOB_ONE_TIME_KEYS, BOB_SIGNED_CROSS_SIGNING_KEYS_DATA, BOB_SIGNED_TEST_DEVICE_DATA, BOB_TEST_USER_ID, + CURVE25519_KEY_BACKUP_DATA, MASTER_CROSS_SIGNING_PUBLIC_KEY_BASE64, SIGNED_CROSS_SIGNING_KEYS_DATA, SIGNED_TEST_DEVICE_DATA, @@ -69,7 +72,16 @@ import { import { mockInitialApiRequests } from "../../test-utils/mockEndpoints"; import { E2EKeyResponder } from "../../test-utils/E2EKeyResponder"; import { E2EKeyReceiver } from "../../test-utils/E2EKeyReceiver"; -import { createOlmSession, encryptGroupSessionKey, encryptMegolmEvent, ToDeviceEvent } from "./olm-utils"; +import { + bootstrapCrossSigningTestOlmAccount, + createOlmSession, + encryptGroupSessionKey, + encryptMegolmEvent, + encryptSecretSend, + ToDeviceEvent, +} from "./olm-utils"; +import { KeyBackupInfo } from "../../../src/crypto-api"; +import { encodeBase64 } from "../../../src/crypto/olmlib"; // The verification flows use javascript timers to set timeouts. We tell jest to use mock timer implementations // to ensure that we don't end up with dangling timeouts. @@ -1106,6 +1118,236 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("verification (%s)", (backend: st ); }); + describe("Secrets are gossiped after verification", () => { + // we use a legacy olm session as the existing session, + // this will give us access to low level olm functions in order to + // simulate a backup key request with proper olm encryption. + let testOlmAccount: Olm.Account; + const olmDeviceId = "OLM_DEVICE"; + let usermasterPubKey: string; + let backupInfo: KeyBackupInfo; + + beforeEach(async () => { + // create a test olm device which we will use to communicate with alice. We use libolm to implement this. + await Olm.init(); + testOlmAccount = new Olm.Account(); + testOlmAccount.create(); + + backupInfo = { + algorithm: "m.megolm_backup.v1.curve25519-aes-sha2", + version: "1", + auth_data: { + public_key: "hSDwCYkwp1R0i33ctD73Wg2/Og0mOBr066SpjqqbTmo", + }, + }; + + const bootstrapped = bootstrapCrossSigningTestOlmAccount( + testOlmAccount, + TEST_USER_ID, + olmDeviceId, + backupInfo, + ); + + e2eKeyResponder.addDeviceKeys(bootstrapped.device_keys![TEST_USER_ID]![olmDeviceId]); + e2eKeyResponder.addCrossSigningData(bootstrapped); + + usermasterPubKey = Object.values(bootstrapped.master_keys![TEST_USER_ID].keys)[0]; + + aliceClient = await startTestClient(); + syncResponder.sendOrQueueSyncResponse(getSyncResponse([TEST_USER_ID])); + await syncPromise(aliceClient); + // DeviceList has a sleep(5) which we need to make happen + await jest.advanceTimersByTimeAsync(10); + + // The client should now know about the olm device + const devices = await aliceClient.getCrypto()!.getUserDeviceInfo([TEST_USER_ID]); + expect(devices.get(TEST_USER_ID)!.keys()).toContain(olmDeviceId); + }); + + afterEach(async () => { + if (aliceClient !== undefined) { + aliceClient.stopClient(); + } + + if (testOlmAccount) { + testOlmAccount.free(); + } + // Allow in-flight things to complete before we tear down the test + await jest.runAllTimersAsync(); + + fetchMock.mockReset(); + }); + + newBackendOnly("Should request cross signing keys after verification", async () => { + const requestPromises = mockUpSecretRequestAndGetPromises(); + + await doInteractiveVerification(); + + // The secret must have been requested + await requestPromises.get("m.cross_signing.master"); + await requestPromises.get("m.cross_signing.user_signing"); + await requestPromises.get("m.cross_signing.self_signing"); + }); + + newBackendOnly("Should accept the backup decryption key gossip if valid", async () => { + const requestPromises = mockUpSecretRequestAndGetPromises(); + + await doInteractiveVerification(); + + const requestId = await requestPromises.get("m.megolm_backup.v1"); + + const p2pSession = await createOlmSession(testOlmAccount, e2eKeyReceiver); + + const toDeviceEvent = encryptSecretSend({ + sender: aliceClient.getUserId()!, + recipient: aliceClient.getUserId()!, + recipientCurve25519Key: e2eKeyReceiver.getDeviceKey(), + recipientEd25519Key: e2eKeyReceiver.getSigningKey(), + p2pSession: p2pSession, + olmAccount: testOlmAccount, + requestId: requestId!, + secret: BACKUP_DECRYPTION_KEY_BASE64, + }); + + const expectBackupCheck = new Promise((resolve) => { + fetchMock.get( + "express:/_matrix/client/v3/room_keys/version", + (url, request) => { + resolve(undefined); + return backupInfo; + }, + { + overwriteRoutes: true, + }, + ); + }); + fetchMock.get("express:/_matrix/client/v3/room_keys/keys", CURVE25519_KEY_BACKUP_DATA); + + // The dummy device sends the secret + returnToDeviceMessageFromSync(toDeviceEvent); + + await expectBackupCheck; + + // We are lacking a way to signal that the secret has been received, so we wait a bit.. + jest.useRealTimers(); + await new Promise((resolve) => { + setTimeout(resolve, 500); + }); + jest.useFakeTimers(); + + // the backup secret should be cached + const cachedKey = await aliceClient.getCrypto()!.getSessionBackupPrivateKey(); + expect(cachedKey).toBeTruthy(); + expect(encodeBase64(cachedKey!)).toEqual(BACKUP_DECRYPTION_KEY_BASE64); + }); + + newBackendOnly("Should not accept the backup decryption key gossip if version not matching", async () => { + const requestPromises = mockUpSecretRequestAndGetPromises(); + + await doInteractiveVerification(); + + const requestId = await requestPromises.get("m.megolm_backup.v1"); + + const p2pSession = await createOlmSession(testOlmAccount, e2eKeyReceiver); + + const toDeviceEvent = encryptSecretSend({ + sender: aliceClient.getUserId()!, + recipient: aliceClient.getUserId()!, + recipientCurve25519Key: e2eKeyReceiver.getDeviceKey(), + recipientEd25519Key: e2eKeyReceiver.getSigningKey(), + p2pSession: p2pSession, + olmAccount: testOlmAccount, + requestId: requestId!, + secret: BACKUP_DECRYPTION_KEY_BASE64, + }); + + // This is another backup info, with a different version and public key. + const otherBackup = { + algorithm: "m.megolm_backup.v1.curve25519-aes-sha2", + version: "2", + auth_data: { + public_key: "70+eIej1KpOeB803tyjpxvFP57ypLQCnnlHJkT6GZEg", + }, + }; + + const expectBackupCheck = new Promise((resolve) => { + fetchMock.get( + "express:/_matrix/client/v3/room_keys/version", + (url, request) => { + resolve(undefined); + return otherBackup; + }, + { + overwriteRoutes: true, + }, + ); + }); + + // The dummy device sends the secret + returnToDeviceMessageFromSync(toDeviceEvent); + + await expectBackupCheck; + + // We are lacking a way to signal that the secret has been received, so we wait a bit.. + jest.useRealTimers(); + await new Promise((resolve) => { + setTimeout(resolve, 500); + }); + jest.useFakeTimers(); + + // the backup secret should be cached + const cachedKey = await aliceClient.getCrypto()!.getSessionBackupPrivateKey(); + expect(cachedKey).toBeNull(); + }); + + /** + * Do an interactive verification between alice and the dummy device. + */ + async function doInteractiveVerification(): Promise { + // Do a QR code verification for simplicity + + // Alice sends a m.key.verification.request + const [, request] = await Promise.all([ + expectSendToDeviceMessage("m.key.verification.request"), + aliceClient.getCrypto()!.requestDeviceVerification(TEST_USER_ID, olmDeviceId), + ]); + const transactionId = request.transactionId!; + + // The dummy device replies with an m.key.verification.ready, indicating it can show a QR code + returnToDeviceMessageFromSync( + buildReadyMessage(transactionId, ["m.qr_code.show.v1", "m.reciprocate.v1"], olmDeviceId), + ); + await waitForVerificationRequestChanged(request); + + const currentDeviceKey = e2eKeyReceiver.getSigningKey(); + // the dummy device shows a QR code + const sharedSecret = "SUPERSEKRET"; + const qrCodeBuffer = buildQRCodeMode1(transactionId, usermasterPubKey, currentDeviceKey, sharedSecret); + + // Alice scans the QR code + const sendToDevicePromise = expectSendToDeviceMessage("m.key.verification.start"); + const verifier = await request.scanQRCode(qrCodeBuffer); + + await sendToDevicePromise; + + const verificationPromise = verifier.verify(); + // the dummy device confirms that Alice scanned the QR code, by replying with a done + returnToDeviceMessageFromSync(buildDoneMessage(transactionId)); + + // Alice also replies with a 'done' + await expectSendToDeviceMessage("m.key.verification.done"); + + // ... and the whole thing should be done! + await verificationPromise; + + // The other device should now be verified. + const otherDevice = (await aliceClient.getCrypto()!.getUserDeviceInfo([TEST_USER_ID])) + .get(TEST_USER_ID)! + .get(olmDeviceId); + expect(otherDevice?.verified).toEqual(DeviceVerification.Verified); + } + }); + async function startTestClient(opts: Partial = {}): Promise { const client = createClient({ baseUrl: TEST_HOMESERVER_URL, @@ -1167,6 +1409,57 @@ function expectSendToDeviceMessage(msgtype: string): Promise<{ messages: any }> }); } +function mockUpSecretRequestAndGetPromises(): Map> { + let mskRequested: (requestId: string) => void; + const awaitMskRequest: Promise = new Promise((resolve) => { + mskRequested = resolve; + }); + let uskRequested: (requestId: string) => void; + const awaitUskRequested: Promise = new Promise((resolve) => { + uskRequested = resolve; + }); + let sskRequested: (requestId: string) => void; + const awaitSskRequested: Promise = new Promise((resolve) => { + sskRequested = resolve; + }); + let backupKeyRequested: (requestId: string) => void; + const awaitBackupKeyRequested: Promise = new Promise((resolve) => { + backupKeyRequested = resolve; + }); + + fetchMock.put( + new RegExp(`/_matrix/client/(r0|v3)/sendToDevice/m.secret.request`), + (url: string, opts: RequestInit): MockResponse => { + const messages = JSON.parse(opts.body as string).messages[TEST_USER_ID]; + // rust crypto broadcasts to all devices, old crypto to a specific device, take the first one + const content = Object.values(messages)[0] as any; //messages[TEST_DEVICE_ID] ? messages[TEST_DEVICE_ID] : messages['*']; + if (content.action == "request") { + const name = content.name; + const requestId = content.request_id; + // console.warn(`Requested secret ${name} with id ${requestId}`); + if (name == "m.cross_signing.user_signing") { + uskRequested(requestId); + } else if (name == "m.cross_signing.master") { + mskRequested(requestId); + } else if (name == "m.cross_signing.self_signing") { + sskRequested(requestId); + } else if (name == "m.megolm_backup.v1") { + backupKeyRequested(requestId); + } + } + return {}; + }, + { overwriteRoutes: true }, + ); + + const promiseMap = new Map>(); + promiseMap.set("m.cross_signing.master", awaitMskRequest); + promiseMap.set("m.cross_signing.self_signing", awaitSskRequested); + promiseMap.set("m.cross_signing.user_signing", awaitUskRequested); + promiseMap.set("m.megolm_backup.v1", awaitBackupKeyRequested); + return promiseMap; +} + /** wait for the verification request to emit a 'Change' event */ function waitForVerificationRequestChanged(request: VerificationRequest): Promise { return new Promise((resolve) => { @@ -1212,11 +1505,15 @@ function buildRequestMessage(transactionId: string): { type: string; content: ob } /** build an m.key.verification.ready to-device message originating from the dummy device */ -function buildReadyMessage(transactionId: string, methods: string[]): { type: string; content: object } { +function buildReadyMessage( + transactionId: string, + methods: string[], + fromDevice?: string, +): { type: string; content: object } { return { type: "m.key.verification.ready", content: { - from_device: TEST_DEVICE_ID, + from_device: fromDevice || TEST_DEVICE_ID, methods: methods, transaction_id: transactionId, }, @@ -1332,3 +1629,27 @@ function buildQRCode(transactionId: string, key1Base64: string, key2Base64: stri // truncate to the right length return qrCodeBuffer.subarray(0, idx); } + +function buildQRCodeMode1( + transactionId: string, + key1Base64: string, + key2Base64: string, + sharedSecret: string, +): Uint8Array { + // https://spec.matrix.org/v1.7/client-server-api/#qr-code-format + + const qrCodeBuffer = Buffer.alloc(150); // oversize + let idx = 0; + idx += qrCodeBuffer.write("MATRIX", idx, "ascii"); + idx = qrCodeBuffer.writeUInt8(0x02, idx); // version + idx = qrCodeBuffer.writeUInt8(0x01, idx); // mode + idx = qrCodeBuffer.writeInt16BE(transactionId.length, idx); + idx += qrCodeBuffer.write(transactionId, idx, "ascii"); + + idx += Buffer.from(key1Base64, "base64").copy(qrCodeBuffer, idx); + idx += Buffer.from(key2Base64, "base64").copy(qrCodeBuffer, idx); + idx += qrCodeBuffer.write(sharedSecret, idx); + + // truncate to the right length + return qrCodeBuffer.subarray(0, idx); +} From 5e6527d3d3369cbf03b254f6912142bb82e2d8cc Mon Sep 17 00:00:00 2001 From: Valere Date: Mon, 9 Oct 2023 17:02:55 +0200 Subject: [PATCH 04/26] use delete secret API --- src/rust-crypto/rust-crypto.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/rust-crypto/rust-crypto.ts b/src/rust-crypto/rust-crypto.ts index 51c29865548..0e29521065b 100644 --- a/src/rust-crypto/rust-crypto.ts +++ b/src/rust-crypto/rust-crypto.ts @@ -1386,6 +1386,9 @@ export class RustCrypto extends TypedEventEmitter Date: Thu, 12 Oct 2023 18:37:51 +0200 Subject: [PATCH 05/26] fix logger --- src/rust-crypto/rust-crypto.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/rust-crypto/rust-crypto.ts b/src/rust-crypto/rust-crypto.ts index eec8b3aaac7..74c49f30d08 100644 --- a/src/rust-crypto/rust-crypto.ts +++ b/src/rust-crypto/rust-crypto.ts @@ -1371,7 +1371,7 @@ export class RustCrypto extends TypedEventEmitter { - logger.debug(`onReceiveSecret: Received secret ${name}`); + this.logger.debug(`onReceiveSecret: Received secret ${name}`); if (name === "m.megolm_backup.v1") { // We need the current version, and it's a good time to check if trusted const info = (await this.backupManager.checkKeyBackupAndEnable(true))?.backupInfo; @@ -1383,7 +1383,7 @@ export class RustCrypto extends TypedEventEmitter Date: Thu, 12 Oct 2023 19:21:06 +0200 Subject: [PATCH 06/26] better comment and cleaning --- spec/integ/crypto/olm-utils.ts | 34 ++++++++++++++++++----- spec/integ/crypto/verification.spec.ts | 37 ++++++++++---------------- src/rust-crypto/index.ts | 5 ++-- src/rust-crypto/rust-crypto.ts | 19 +++++++++---- 4 files changed, 58 insertions(+), 37 deletions(-) diff --git a/spec/integ/crypto/olm-utils.ts b/spec/integ/crypto/olm-utils.ts index e14ce0d0088..4b540e9be0e 100644 --- a/spec/integ/crypto/olm-utils.ts +++ b/spec/integ/crypto/olm-utils.ts @@ -61,13 +61,22 @@ export function getTestOlmAccountKeys(olmAccount: Olm.Account, userId: string, d return testDeviceKeys; } -export type BootstrapTestAccountData = { - /** A full key query response with cross-signing and device keys and all signatures */ - downloadKeys: Partial; - masterKeyOlmSigning: Olm.PkSigning; - masterKeyPublicKeyBase64: string; -}; - +/** + * Bootstrap cross signing for the given Olm account. + * Will generate the cross signing keys and sign them with the master key, and returns the `IDownloadKeyResult` + * that can be directly fed into a test e2eKeyResponder. + * + * The cross-signing keys are randomly generated, similar to how the olm account keys are generated. There may not + * be any value in using static vectors, as the device keys change at every test run. + * + * If a `KeyBackupInfo` is provided, the `auth_data` will be signed with the master key, + * the backup will be then trusted after verification. + * + * @param olmAccount The Olm account object to use for signing the device keys. + * @param userId The user ID to associate with the device keys. + * @param deviceId The device ID to associate with the device keys. + * @returns A valid keys/query response that can be fed into a test e2eKeyResponder. + */ export function bootstrapCrossSigningTestOlmAccount( olmAccount: Olm.Account, userId: string, @@ -101,6 +110,7 @@ export function bootstrapCrossSigningTestOlmAccount( ["ed25519:" + sskPubkey]: sskPubkey, }, }; + // sign the ssk with the msk const sskSig = olmAliceMSK.sign(anotherjson.stringify(sskInfo)); sskInfo.signatures = { [userId]: { @@ -115,6 +125,8 @@ export function bootstrapCrossSigningTestOlmAccount( ["ed25519:" + userPubkey]: userPubkey, }, }; + + // sign the usk with the msk const uskSig = olmAliceMSK.sign(anotherjson.stringify(uskInfo)); uskInfo.signatures = { [userId]: { @@ -122,6 +134,7 @@ export function bootstrapCrossSigningTestOlmAccount( }, }; + // get the device keys and sign them with the ssk (the device is then cross signed) const deviceKeys = getTestOlmAccountKeys(olmAccount, userId, deviceId); const copy = Object.assign({}, deviceKeys); @@ -131,6 +144,7 @@ export function bootstrapCrossSigningTestOlmAccount( // add the signature deviceKeys.signatures![userId]["ed25519:" + sskPubkey] = crossSignature; + // if we have a key backup info, sign it with the msk if (keyBackupInfo?.auth_data) { const unsignedAuthData = Object.assign({}, keyBackupInfo?.auth_data); delete unsignedAuthData.signatures; @@ -309,6 +323,12 @@ export function encryptGroupSessionKey(opts: { }); } +/** + * Test utility to correctly encrypt a secret send event to a test device using the provided p2p session. + * + * @param opts - the options for the secret send event + * @returns the to-device event, ready to be returned in a sync response for the test device. + */ export function encryptSecretSend(opts: { sender: string; /** recipient's user id */ diff --git a/spec/integ/crypto/verification.spec.ts b/spec/integ/crypto/verification.spec.ts index 7e899a40045..9cd4942da27 100644 --- a/spec/integ/crypto/verification.spec.ts +++ b/spec/integ/crypto/verification.spec.ts @@ -1376,7 +1376,9 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("verification (%s)", (backend: st const currentDeviceKey = e2eKeyReceiver.getSigningKey(); // the dummy device shows a QR code const sharedSecret = "SUPERSEKRET"; - const qrCodeBuffer = buildQRCodeMode1(transactionId, usermasterPubKey, currentDeviceKey, sharedSecret); + // use mode 0x01, self-verifying in which the current device does trust the master key + const mode = 0x01; + const qrCodeBuffer = buildQRCode(transactionId, usermasterPubKey, currentDeviceKey, sharedSecret, mode); // Alice scans the QR code const sendToDevicePromise = expectSendToDeviceMessage("m.key.verification.start"); @@ -1463,6 +1465,14 @@ function expectSendToDeviceMessage(msgtype: string): Promise<{ messages: any }> }); } +/** + * Utility to add all needed mocks for secret requesting (to device of type `m.secret.request`). + * + * The following secrets are mocked: `m.cross_signing.master`, `m.cross_signing.self_signing`, + * `m.cross_signing.user_signing`, `m.megolm_backup.v1`. + * + * @returns a map of secret name to promise that will resolve when the secret is requested. + */ function mockUpSecretRequestAndGetPromises(): Map> { let mskRequested: (requestId: string) => void; const awaitMskRequest: Promise = new Promise((resolve) => { @@ -1490,7 +1500,6 @@ function mockUpSecretRequestAndGetPromises(): Map> { if (content.action == "request") { const name = content.name; const requestId = content.request_id; - // console.warn(`Requested secret ${name} with id ${requestId}`); if (name == "m.cross_signing.user_signing") { uskRequested(requestId); } else if (name == "m.cross_signing.master") { @@ -1665,30 +1674,12 @@ function buildDoneMessage(transactionId: string) { }; } -function buildQRCode(transactionId: string, key1Base64: string, key2Base64: string, sharedSecret: string): Uint8Array { - // https://spec.matrix.org/v1.7/client-server-api/#qr-code-format - - const qrCodeBuffer = Buffer.alloc(150); // oversize - let idx = 0; - idx += qrCodeBuffer.write("MATRIX", idx, "ascii"); - idx = qrCodeBuffer.writeUInt8(0x02, idx); // version - idx = qrCodeBuffer.writeUInt8(0x02, idx); // mode - idx = qrCodeBuffer.writeInt16BE(transactionId.length, idx); - idx += qrCodeBuffer.write(transactionId, idx, "ascii"); - - idx += Buffer.from(key1Base64, "base64").copy(qrCodeBuffer, idx); - idx += Buffer.from(key2Base64, "base64").copy(qrCodeBuffer, idx); - idx += qrCodeBuffer.write(sharedSecret, idx); - - // truncate to the right length - return qrCodeBuffer.subarray(0, idx); -} - -function buildQRCodeMode1( +function buildQRCode( transactionId: string, key1Base64: string, key2Base64: string, sharedSecret: string, + mode = 0x02, ): Uint8Array { // https://spec.matrix.org/v1.7/client-server-api/#qr-code-format @@ -1696,7 +1687,7 @@ function buildQRCodeMode1( let idx = 0; idx += qrCodeBuffer.write("MATRIX", idx, "ascii"); idx = qrCodeBuffer.writeUInt8(0x02, idx); // version - idx = qrCodeBuffer.writeUInt8(0x01, idx); // mode + idx = qrCodeBuffer.writeUInt8(mode, idx); // mode idx = qrCodeBuffer.writeInt16BE(transactionId.length, idx); idx += qrCodeBuffer.write(transactionId, idx, "ascii"); diff --git a/src/rust-crypto/index.ts b/src/rust-crypto/index.ts index 629753b0722..321c43add52 100644 --- a/src/rust-crypto/index.ts +++ b/src/rust-crypto/index.ts @@ -75,13 +75,14 @@ export async function initRustCrypto( rustCrypto.onUserIdentityUpdated(userId), ); - // check first if some secrets are pending for process. The registerReceiveSecretCallback will be - // triggered only for new secrets. + // Check if there are any key backup secrets pending processing. There may be multiple secrets to process if several devices have gossiped them. + // The `registerReceiveSecretCallback` function will only be triggered for new secrets. If the client is restarted before processing them, the secrets will need to be manually handled. const pendingValues: string[] = await olmMachine.getSecretsFromInbox("m.megolm_backup.v1"); pendingValues.forEach((value) => { rustCrypto.onReceiveSecret("m.megolm_backup.v1", value); }); + // Register a callback to be notified when a new secret is received, as for now only the key backup secret is supported (the cross signing secrets are handled automatically by the OlmMachine) await olmMachine.registerReceiveSecretCallback((name: string, value: string) => rustCrypto.onReceiveSecret(name, value), ); diff --git a/src/rust-crypto/rust-crypto.ts b/src/rust-crypto/rust-crypto.ts index 74c49f30d08..2f09fe3a7c7 100644 --- a/src/rust-crypto/rust-crypto.ts +++ b/src/rust-crypto/rust-crypto.ts @@ -1373,12 +1373,20 @@ export class RustCrypto extends TypedEventEmitter { this.logger.debug(`onReceiveSecret: Received secret ${name}`); if (name === "m.megolm_backup.v1") { - // We need the current version, and it's a good time to check if trusted - const info = (await this.backupManager.checkKeyBackupAndEnable(true))?.backupInfo; - if (info?.version) { + // Currently we only receive the decryption key without any key backup version, it is important to + // check that the secret is valid and matches the current version before storing it. + // We force a check to ensure to have the latest version. We also want to check that the backup is trusted + // as we don't want to store the secret if the backup is not trusted, and eventually import megolm keys later fron an untrusted backup. + const backupCheck = await this.backupManager.checkKeyBackupAndEnable(true); + if (backupCheck?.backupInfo?.version && backupCheck.trustInfo.trusted) { const backupDecryptionKey = RustSdkCryptoJs.BackupDecryptionKey.fromBase64(value); - const authPublickey = (info.auth_data as Curve25519AuthData)?.public_key; + const authPublickey = (backupCheck.backupInfo.auth_data as Curve25519AuthData)?.public_key; + if (!authPublickey) { + this.logger.debug(`onReceiveSecret: malformed or unsupported backup auth data`); + return; + } + const backupMatchesSavedPrivateKey = authPublickey === backupDecryptionKey.megolmV1PublicKey.publicKeyBase64; @@ -1388,13 +1396,14 @@ export class RustCrypto extends TypedEventEmitter Date: Thu, 12 Oct 2023 19:24:28 +0200 Subject: [PATCH 07/26] free the pkSigning --- spec/integ/crypto/olm-utils.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/spec/integ/crypto/olm-utils.ts b/spec/integ/crypto/olm-utils.ts index 4b540e9be0e..5a1a39e2adf 100644 --- a/spec/integ/crypto/olm-utils.ts +++ b/spec/integ/crypto/olm-utils.ts @@ -157,6 +157,11 @@ export function bootstrapCrossSigningTestOlmAccount( }; } + // clean the olm resources as we don't need them anymore + olmAliceMSK.free(); + olmAliceSSK.free(); + olmAliceUSK.free(); + return { master_keys: { [userId]: mskInfo }, user_signing_keys: { [userId]: uskInfo as SigningKeys }, From 89be8cb8aca3e0acee7306f33a82fc61c1529f6a Mon Sep 17 00:00:00 2001 From: Valere Date: Thu, 12 Oct 2023 19:32:37 +0200 Subject: [PATCH 08/26] fix typo --- src/rust-crypto/rust-crypto.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/rust-crypto/rust-crypto.ts b/src/rust-crypto/rust-crypto.ts index 2f09fe3a7c7..05b843fdfd3 100644 --- a/src/rust-crypto/rust-crypto.ts +++ b/src/rust-crypto/rust-crypto.ts @@ -1374,9 +1374,9 @@ export class RustCrypto extends TypedEventEmitter Date: Fri, 13 Oct 2023 00:21:07 +0200 Subject: [PATCH 09/26] add missing mocks --- spec/unit/rust-crypto/rust-crypto.spec.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/spec/unit/rust-crypto/rust-crypto.spec.ts b/spec/unit/rust-crypto/rust-crypto.spec.ts index 0a28eeb7b5b..323d9abeedf 100644 --- a/spec/unit/rust-crypto/rust-crypto.spec.ts +++ b/spec/unit/rust-crypto/rust-crypto.spec.ts @@ -64,6 +64,8 @@ describe("initRustCrypto", () => { return { registerRoomKeyUpdatedCallback: jest.fn(), registerUserIdentityUpdatedCallback: jest.fn(), + getSecretsFromInbox: jest.fn().mockResolvedValue([]), + registerReceiveSecretCallback: jest.fn(), outgoingRequests: jest.fn(), } as unknown as Mocked; } From 08672ba42fdcb74d8459ab60a59b6b07a40d61e3 Mon Sep 17 00:00:00 2001 From: Valere Date: Fri, 13 Oct 2023 01:01:47 +0200 Subject: [PATCH 10/26] improve coverage --- spec/unit/rust-crypto/rust-crypto.spec.ts | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/spec/unit/rust-crypto/rust-crypto.spec.ts b/spec/unit/rust-crypto/rust-crypto.spec.ts index 323d9abeedf..e1aab536dc3 100644 --- a/spec/unit/rust-crypto/rust-crypto.spec.ts +++ b/spec/unit/rust-crypto/rust-crypto.spec.ts @@ -64,7 +64,7 @@ describe("initRustCrypto", () => { return { registerRoomKeyUpdatedCallback: jest.fn(), registerUserIdentityUpdatedCallback: jest.fn(), - getSecretsFromInbox: jest.fn().mockResolvedValue([]), + getSecretsFromInbox: jest.fn().mockResolvedValue(["dGhpc2lzYWZha2VzZWNyZXQ="]), registerReceiveSecretCallback: jest.fn(), outgoingRequests: jest.fn(), } as unknown as Mocked; @@ -110,6 +110,24 @@ describe("initRustCrypto", () => { expect(OlmMachine.initialize).toHaveBeenCalledWith(expect.anything(), expect.anything(), undefined, undefined); }); + + it("Should get secrets from inbox on start", async () => { + const testOlmMachine = makeTestOlmMachine() as OlmMachine; + jest.spyOn(OlmMachine, "initialize").mockResolvedValue(testOlmMachine); + + await initRustCrypto( + logger, + {} as MatrixClient["http"], + TEST_USER, + TEST_DEVICE_ID, + {} as ServerSideSecretStorage, + {} as CryptoCallbacks, + "storePrefix", + "storePassphrase", + ); + + expect(testOlmMachine.getSecretsFromInbox).toHaveBeenCalledWith("m.megolm_backup.v1"); + }); }); describe("RustCrypto", () => { From a6d1e11b9149101eb139eea3f3377575c572c341 Mon Sep 17 00:00:00 2001 From: Valere Date: Fri, 13 Oct 2023 08:28:38 +0200 Subject: [PATCH 11/26] better var name --- src/rust-crypto/rust-crypto.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/rust-crypto/rust-crypto.ts b/src/rust-crypto/rust-crypto.ts index 05b843fdfd3..afe1d1e9e5b 100644 --- a/src/rust-crypto/rust-crypto.ts +++ b/src/rust-crypto/rust-crypto.ts @@ -1387,10 +1387,10 @@ export class RustCrypto extends TypedEventEmitter Date: Fri, 13 Oct 2023 09:21:57 +0200 Subject: [PATCH 12/26] quick refactoring --- src/rust-crypto/backup.ts | 25 +++++++++++++++++++++++++ src/rust-crypto/rust-crypto.ts | 13 ++----------- 2 files changed, 27 insertions(+), 11 deletions(-) diff --git a/src/rust-crypto/backup.ts b/src/rust-crypto/backup.ts index 9ecb68b3574..fa47557882d 100644 --- a/src/rust-crypto/backup.ts +++ b/src/rust-crypto/backup.ts @@ -106,6 +106,31 @@ export class RustBackupManager extends TypedEventEmitter { + if (info.algorithm !== "m.megolm_backup.v1.curve25519-aes-sha2") { + logger.warn("backupMatchesPrivateKey: Unsupported backup algorithm", info.algorithm); + return false; + } + + let backupDecryptionKey: RustSdkCryptoJs.BackupDecryptionKey; + try { + backupDecryptionKey = RustSdkCryptoJs.BackupDecryptionKey.fromBase64(privateKeyBase64); + } catch (e) { + logger.warn("backupMatchesPrivateKey: Invalid backup decryption key", e); + return false; + } + + return ( + (info.auth_data as Curve25519AuthData)?.public_key === backupDecryptionKey.megolmV1PublicKey.publicKeyBase64 + ); + } + /** * Re-check the key backup and enable/disable it as appropriate. * diff --git a/src/rust-crypto/rust-crypto.ts b/src/rust-crypto/rust-crypto.ts index afe1d1e9e5b..84e772bec00 100644 --- a/src/rust-crypto/rust-crypto.ts +++ b/src/rust-crypto/rust-crypto.ts @@ -1379,18 +1379,9 @@ export class RustCrypto extends TypedEventEmitter Date: Fri, 13 Oct 2023 10:06:08 +0200 Subject: [PATCH 13/26] add more tests --- spec/integ/crypto/olm-utils.ts | 24 ++-- spec/integ/crypto/verification.spec.ts | 192 ++++++++++++++++--------- src/rust-crypto/backup.ts | 2 +- src/rust-crypto/rust-crypto.ts | 1 + 4 files changed, 140 insertions(+), 79 deletions(-) diff --git a/spec/integ/crypto/olm-utils.ts b/spec/integ/crypto/olm-utils.ts index 5a1a39e2adf..d0378705e73 100644 --- a/spec/integ/crypto/olm-utils.ts +++ b/spec/integ/crypto/olm-utils.ts @@ -69,19 +69,20 @@ export function getTestOlmAccountKeys(olmAccount: Olm.Account, userId: string, d * The cross-signing keys are randomly generated, similar to how the olm account keys are generated. There may not * be any value in using static vectors, as the device keys change at every test run. * - * If a `KeyBackupInfo` is provided, the `auth_data` will be signed with the master key, - * the backup will be then trusted after verification. + * If some`KeyBackupInfo` are provided, the `auth_data` will be signed with the master key, + * the backup will be then trusted after verification. For testing purpose several backups can be provided. * - * @param olmAccount The Olm account object to use for signing the device keys. - * @param userId The user ID to associate with the device keys. - * @param deviceId The device ID to associate with the device keys. + * @param olmAccount - The Olm account object to use for signing the device keys. + * @param userId - The user ID to associate with the device keys. + * @param deviceId - The device ID to associate with the device keys. + * @param keyBackupInfo - The key backup info to sign with the master key. * @returns A valid keys/query response that can be fed into a test e2eKeyResponder. */ export function bootstrapCrossSigningTestOlmAccount( olmAccount: Olm.Account, userId: string, deviceId: string, - keyBackupInfo?: KeyBackupInfo, + keyBackupInfo: KeyBackupInfo[] = [], ): Partial { const olmAliceMSK = new global.Olm.PkSigning(); const masterPrivkey = olmAliceMSK.generate_seed(); @@ -144,18 +145,19 @@ export function bootstrapCrossSigningTestOlmAccount( // add the signature deviceKeys.signatures![userId]["ed25519:" + sskPubkey] = crossSignature; - // if we have a key backup info, sign it with the msk - if (keyBackupInfo?.auth_data) { - const unsignedAuthData = Object.assign({}, keyBackupInfo?.auth_data); + // if we have some key backup info, sign them with the msk + + keyBackupInfo.forEach((info) => { + const unsignedAuthData = Object.assign({}, info.auth_data); delete unsignedAuthData.signatures; const backupSignature = olmAliceMSK.sign(anotherjson.stringify(unsignedAuthData)); - keyBackupInfo.auth_data.signatures = { + info.auth_data.signatures = { [userId]: { ["ed25519:" + masterPubkey]: backupSignature, }, }; - } + }); // clean the olm resources as we don't need them anymore olmAliceMSK.free(); diff --git a/spec/integ/crypto/verification.spec.ts b/spec/integ/crypto/verification.spec.ts index 9cd4942da27..3406ed6b597 100644 --- a/spec/integ/crypto/verification.spec.ts +++ b/spec/integ/crypto/verification.spec.ts @@ -1179,7 +1179,30 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("verification (%s)", (backend: st let testOlmAccount: Olm.Account; const olmDeviceId = "OLM_DEVICE"; let usermasterPubKey: string; - let backupInfo: KeyBackupInfo; + + const matchingBackupInfo: KeyBackupInfo = { + algorithm: "m.megolm_backup.v1.curve25519-aes-sha2", + version: "1", + auth_data: { + public_key: "hSDwCYkwp1R0i33ctD73Wg2/Og0mOBr066SpjqqbTmo", + }, + }; + + const nonMatchingBackupInfo: KeyBackupInfo = { + algorithm: "m.megolm_backup.v1.curve25519-aes-sha2", + version: "1", + auth_data: { + public_key: "EjDwCYkwp1R0i33ctD73Wg2/Og0mOBr066Spjqqaqqo", + }, + }; + + const unknownAlgorithmBackupInfo: KeyBackupInfo = { + algorithm: "m.megolm_backup.foo_bar", + version: "1", + auth_data: { + public_key: "EjDwCYkwp1R0i33ctD73Wg2/Og0mOBr066Spjqqaqqo", + }, + }; beforeEach(async () => { // create a test olm device which we will use to communicate with alice. We use libolm to implement this. @@ -1187,20 +1210,10 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("verification (%s)", (backend: st testOlmAccount = new Olm.Account(); testOlmAccount.create(); - backupInfo = { - algorithm: "m.megolm_backup.v1.curve25519-aes-sha2", - version: "1", - auth_data: { - public_key: "hSDwCYkwp1R0i33ctD73Wg2/Og0mOBr066SpjqqbTmo", - }, - }; - - const bootstrapped = bootstrapCrossSigningTestOlmAccount( - testOlmAccount, - TEST_USER_ID, - olmDeviceId, - backupInfo, - ); + const bootstrapped = bootstrapCrossSigningTestOlmAccount(testOlmAccount, TEST_USER_ID, olmDeviceId, [ + matchingBackupInfo, + nonMatchingBackupInfo, + ]); e2eKeyResponder.addDeviceKeys(bootstrapped.device_keys![TEST_USER_ID]![olmDeviceId]); e2eKeyResponder.addCrossSigningData(bootstrapped); @@ -1250,37 +1263,53 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("verification (%s)", (backend: st const requestId = await requestPromises.get("m.megolm_backup.v1"); - const p2pSession = await createOlmSession(testOlmAccount, e2eKeyReceiver); + await sendBackupGossipAndExpectVersion(requestId!, BACKUP_DECRYPTION_KEY_BASE64, matchingBackupInfo); - const toDeviceEvent = encryptSecretSend({ - sender: aliceClient.getUserId()!, - recipient: aliceClient.getUserId()!, - recipientCurve25519Key: e2eKeyReceiver.getDeviceKey(), - recipientEd25519Key: e2eKeyReceiver.getSigningKey(), - p2pSession: p2pSession, - olmAccount: testOlmAccount, - requestId: requestId!, - secret: BACKUP_DECRYPTION_KEY_BASE64, + // We are lacking a way to signal that the secret has been received, so we wait a bit.. + jest.useRealTimers(); + await new Promise((resolve) => { + setTimeout(resolve, 500); }); + jest.useFakeTimers(); - const expectBackupCheck = new Promise((resolve) => { - fetchMock.get( - "express:/_matrix/client/v3/room_keys/version", - (url, request) => { - resolve(undefined); - return backupInfo; - }, - { - overwriteRoutes: true, - }, - ); + // the backup secret should be cached + const cachedKey = await aliceClient.getCrypto()!.getSessionBackupPrivateKey(); + expect(cachedKey).toBeTruthy(); + expect(encodeBase64(cachedKey!)).toEqual(BACKUP_DECRYPTION_KEY_BASE64); + }); + + newBackendOnly("Should not accept the backup decryption key gossip if private key do not match", async () => { + const requestPromises = mockUpSecretRequestAndGetPromises(); + + await doInteractiveVerification(); + + const requestId = await requestPromises.get("m.megolm_backup.v1"); + + await sendBackupGossipAndExpectVersion(requestId!, BACKUP_DECRYPTION_KEY_BASE64, nonMatchingBackupInfo); + + // We are lacking a way to signal that the secret has been received, so we wait a bit.. + jest.useRealTimers(); + await new Promise((resolve) => { + setTimeout(resolve, 500); }); - fetchMock.get("express:/_matrix/client/v3/room_keys/keys", CURVE25519_KEY_BACKUP_DATA); + jest.useFakeTimers(); - // The dummy device sends the secret - returnToDeviceMessageFromSync(toDeviceEvent); + // the backup secret should not be cached + const cachedKey = await aliceClient.getCrypto()!.getSessionBackupPrivateKey(); + expect(cachedKey).toBeNull(); + }); - await expectBackupCheck; + newBackendOnly("Should not accept the backup decryption key gossip if backup not trusted", async () => { + const requestPromises = mockUpSecretRequestAndGetPromises(); + + await doInteractiveVerification(); + + const requestId = await requestPromises.get("m.megolm_backup.v1"); + + const infoCopy = Object.assign({}, matchingBackupInfo); + delete infoCopy.auth_data.signatures; + + await sendBackupGossipAndExpectVersion(requestId!, BACKUP_DECRYPTION_KEY_BASE64, infoCopy); // We are lacking a way to signal that the secret has been received, so we wait a bit.. jest.useRealTimers(); @@ -1289,19 +1318,66 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("verification (%s)", (backend: st }); jest.useFakeTimers(); - // the backup secret should be cached + // the backup secret should not be cached const cachedKey = await aliceClient.getCrypto()!.getSessionBackupPrivateKey(); - expect(cachedKey).toBeTruthy(); - expect(encodeBase64(cachedKey!)).toEqual(BACKUP_DECRYPTION_KEY_BASE64); + expect(cachedKey).toBeNull(); }); - newBackendOnly("Should not accept the backup decryption key gossip if version not matching", async () => { + newBackendOnly("Should not accept the backup decryption key gossip if backup algorithm unknown", async () => { const requestPromises = mockUpSecretRequestAndGetPromises(); await doInteractiveVerification(); const requestId = await requestPromises.get("m.megolm_backup.v1"); + await sendBackupGossipAndExpectVersion( + requestId!, + BACKUP_DECRYPTION_KEY_BASE64, + unknownAlgorithmBackupInfo, + ); + + // We are lacking a way to signal that the secret has been received, so we wait a bit.. + jest.useRealTimers(); + await new Promise((resolve) => { + setTimeout(resolve, 500); + }); + jest.useFakeTimers(); + + // the backup secret should not be cached + const cachedKey = await aliceClient.getCrypto()!.getSessionBackupPrivateKey(); + expect(cachedKey).toBeNull(); + }); + + newBackendOnly("Should not accept an invalid backup decryption key", async () => { + const requestPromises = mockUpSecretRequestAndGetPromises(); + + await doInteractiveVerification(); + + const requestId = await requestPromises.get("m.megolm_backup.v1"); + + await sendBackupGossipAndExpectVersion(requestId!, "InvalidSecret", matchingBackupInfo); + + // We are lacking a way to signal that the secret has been received, so we wait a bit.. + jest.useRealTimers(); + await new Promise((resolve) => { + setTimeout(resolve, 500); + }); + jest.useFakeTimers(); + + // the backup secret should not be cached + const cachedKey = await aliceClient.getCrypto()!.getSessionBackupPrivateKey(); + expect(cachedKey).toBeNull(); + }); + + /** + * Common test setup for gossiping secrets. + * Creates a peer to peer session, sends the secret, mockup the version API, send the secret back from sync, then await for the backup check. + */ + async function sendBackupGossipAndExpectVersion( + requestId: string, + secret: string, + expectBackup: KeyBackupInfo, + ) { const p2pSession = await createOlmSession(testOlmAccount, e2eKeyReceiver); const toDeviceEvent = encryptSecretSend({ @@ -1312,24 +1388,15 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("verification (%s)", (backend: st p2pSession: p2pSession, olmAccount: testOlmAccount, requestId: requestId!, - secret: BACKUP_DECRYPTION_KEY_BASE64, + secret: secret, }); - // This is another backup info, with a different version and public key. - const otherBackup = { - algorithm: "m.megolm_backup.v1.curve25519-aes-sha2", - version: "2", - auth_data: { - public_key: "70+eIej1KpOeB803tyjpxvFP57ypLQCnnlHJkT6GZEg", - }, - }; - const expectBackupCheck = new Promise((resolve) => { fetchMock.get( "express:/_matrix/client/v3/room_keys/version", (url, request) => { resolve(undefined); - return otherBackup; + return expectBackup; }, { overwriteRoutes: true, @@ -1337,22 +1404,13 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("verification (%s)", (backend: st ); }); + fetchMock.get("express:/_matrix/client/v3/room_keys/keys", CURVE25519_KEY_BACKUP_DATA); + // The dummy device sends the secret returnToDeviceMessageFromSync(toDeviceEvent); await expectBackupCheck; - - // We are lacking a way to signal that the secret has been received, so we wait a bit.. - jest.useRealTimers(); - await new Promise((resolve) => { - setTimeout(resolve, 500); - }); - jest.useFakeTimers(); - - // the backup secret should be cached - const cachedKey = await aliceClient.getCrypto()!.getSessionBackupPrivateKey(); - expect(cachedKey).toBeNull(); - }); + } /** * Do an interactive verification between alice and the dummy device. diff --git a/src/rust-crypto/backup.ts b/src/rust-crypto/backup.ts index fa47557882d..f39bdca9d4f 100644 --- a/src/rust-crypto/backup.ts +++ b/src/rust-crypto/backup.ts @@ -112,7 +112,7 @@ export class RustBackupManager extends TypedEventEmitter { + public backupMatchesPrivateKey(info: KeyBackupInfo, privateKeyBase64: string): boolean { if (info.algorithm !== "m.megolm_backup.v1.curve25519-aes-sha2") { logger.warn("backupMatchesPrivateKey: Unsupported backup algorithm", info.algorithm); return false; diff --git a/src/rust-crypto/rust-crypto.ts b/src/rust-crypto/rust-crypto.ts index 84e772bec00..7b3c30572b0 100644 --- a/src/rust-crypto/rust-crypto.ts +++ b/src/rust-crypto/rust-crypto.ts @@ -1386,6 +1386,7 @@ export class RustCrypto extends TypedEventEmitter Date: Fri, 13 Oct 2023 15:53:16 +0200 Subject: [PATCH 14/26] Review, format and comments --- spec/integ/crypto/olm-utils.ts | 11 ++-- spec/integ/crypto/verification.spec.ts | 70 +++++++++++--------------- src/rust-crypto/backup.ts | 10 ++-- src/rust-crypto/rust-crypto.ts | 4 +- 4 files changed, 42 insertions(+), 53 deletions(-) diff --git a/spec/integ/crypto/olm-utils.ts b/spec/integ/crypto/olm-utils.ts index d0378705e73..f912f8059d0 100644 --- a/spec/integ/crypto/olm-utils.ts +++ b/spec/integ/crypto/olm-utils.ts @@ -63,19 +63,20 @@ export function getTestOlmAccountKeys(olmAccount: Olm.Account, userId: string, d /** * Bootstrap cross signing for the given Olm account. + * * Will generate the cross signing keys and sign them with the master key, and returns the `IDownloadKeyResult` * that can be directly fed into a test e2eKeyResponder. * * The cross-signing keys are randomly generated, similar to how the olm account keys are generated. There may not * be any value in using static vectors, as the device keys change at every test run. * - * If some`KeyBackupInfo` are provided, the `auth_data` will be signed with the master key, - * the backup will be then trusted after verification. For testing purpose several backups can be provided. + * If some `KeyBackupInfo` are provided, the `auth_data` of each backup info will be signed with the + * master key, meaning the backups will be then trusted after verification. * * @param olmAccount - The Olm account object to use for signing the device keys. * @param userId - The user ID to associate with the device keys. * @param deviceId - The device ID to associate with the device keys. - * @param keyBackupInfo - The key backup info to sign with the master key. + * @param keyBackupInfo - Optional key backup infos to sign with the master key. * @returns A valid keys/query response that can be fed into a test e2eKeyResponder. */ export function bootstrapCrossSigningTestOlmAccount( @@ -146,7 +147,6 @@ export function bootstrapCrossSigningTestOlmAccount( deviceKeys.signatures![userId]["ed25519:" + sskPubkey] = crossSignature; // if we have some key backup info, sign them with the msk - keyBackupInfo.forEach((info) => { const unsignedAuthData = Object.assign({}, info.auth_data); delete unsignedAuthData.signatures; @@ -337,6 +337,7 @@ export function encryptGroupSessionKey(opts: { * @returns the to-device event, ready to be returned in a sync response for the test device. */ export function encryptSecretSend(opts: { + /** the sender's user id */ sender: string; /** recipient's user id */ recipient: string; @@ -348,7 +349,9 @@ export function encryptSecretSend(opts: { olmAccount: Olm.Account; /** sender's olm session with the recipient */ p2pSession: Olm.Session; + /** The requestId of the secret request that this secret send is replying. */ requestId: string; + /** The secret value */ secret: string; }): ToDeviceEvent { const senderKeys = JSON.parse(opts.olmAccount.identity_keys()); diff --git a/spec/integ/crypto/verification.spec.ts b/spec/integ/crypto/verification.spec.ts index 3406ed6b597..ceb44264c0d 100644 --- a/spec/integ/crypto/verification.spec.ts +++ b/spec/integ/crypto/verification.spec.ts @@ -44,7 +44,7 @@ import { Verifier, VerifierEvent, } from "../../../src/crypto-api/verification"; -import { escapeRegExp } from "../../../src/utils"; +import { defer, escapeRegExp } from "../../../src/utils"; import { awaitDecryption, CRYPTO_BACKENDS, @@ -1173,8 +1173,8 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("verification (%s)", (backend: st }); describe("Secrets are gossiped after verification", () => { - // we use a legacy olm session as the existing session, - // this will give us access to low level olm functions in order to + // We use a legacy olm session as the existing session. + // This will give us access to low level olm functions in order to // simulate a backup key request with proper olm encryption. let testOlmAccount: Olm.Account; const olmDeviceId = "OLM_DEVICE"; @@ -1232,13 +1232,10 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("verification (%s)", (backend: st }); afterEach(async () => { - if (aliceClient !== undefined) { - aliceClient.stopClient(); - } - if (testOlmAccount) { - testOlmAccount.free(); - } + aliceClient?.stopClient(); + testOlmAccount?.free(); + // Allow in-flight things to complete before we tear down the test await jest.runAllTimersAsync(); @@ -1246,7 +1243,7 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("verification (%s)", (backend: st }); newBackendOnly("Should request cross signing keys after verification", async () => { - const requestPromises = mockUpSecretRequestAndGetPromises(); + const requestPromises = mockSecretRequestAndGetPromises(); await doInteractiveVerification(); @@ -1257,7 +1254,7 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("verification (%s)", (backend: st }); newBackendOnly("Should accept the backup decryption key gossip if valid", async () => { - const requestPromises = mockUpSecretRequestAndGetPromises(); + const requestPromises = mockSecretRequestAndGetPromises(); await doInteractiveVerification(); @@ -1279,7 +1276,7 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("verification (%s)", (backend: st }); newBackendOnly("Should not accept the backup decryption key gossip if private key do not match", async () => { - const requestPromises = mockUpSecretRequestAndGetPromises(); + const requestPromises = mockSecretRequestAndGetPromises(); await doInteractiveVerification(); @@ -1300,7 +1297,7 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("verification (%s)", (backend: st }); newBackendOnly("Should not accept the backup decryption key gossip if backup not trusted", async () => { - const requestPromises = mockUpSecretRequestAndGetPromises(); + const requestPromises = mockSecretRequestAndGetPromises(); await doInteractiveVerification(); @@ -1324,7 +1321,7 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("verification (%s)", (backend: st }); newBackendOnly("Should not accept the backup decryption key gossip if backup algorithm unknown", async () => { - const requestPromises = mockUpSecretRequestAndGetPromises(); + const requestPromises = mockSecretRequestAndGetPromises(); await doInteractiveVerification(); @@ -1349,7 +1346,7 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("verification (%s)", (backend: st }); newBackendOnly("Should not accept an invalid backup decryption key", async () => { - const requestPromises = mockUpSecretRequestAndGetPromises(); + const requestPromises = mockSecretRequestAndGetPromises(); await doInteractiveVerification(); @@ -1524,31 +1521,20 @@ function expectSendToDeviceMessage(msgtype: string): Promise<{ messages: any }> } /** - * Utility to add all needed mocks for secret requesting (to device of type `m.secret.request`). + * Utility to add all needed mocks for secret requesting (to-device of type `m.secret.request`). * * The following secrets are mocked: `m.cross_signing.master`, `m.cross_signing.self_signing`, * `m.cross_signing.user_signing`, `m.megolm_backup.v1`. * - * @returns a map of secret name to promise that will resolve when the secret is requested. + * @returns a map of secret name to promise that will resolve (with the id of the secret request) when the secret is requested. */ -function mockUpSecretRequestAndGetPromises(): Map> { - let mskRequested: (requestId: string) => void; - const awaitMskRequest: Promise = new Promise((resolve) => { - mskRequested = resolve; - }); - let uskRequested: (requestId: string) => void; - const awaitUskRequested: Promise = new Promise((resolve) => { - uskRequested = resolve; - }); - let sskRequested: (requestId: string) => void; - const awaitSskRequested: Promise = new Promise((resolve) => { - sskRequested = resolve; - }); - let backupKeyRequested: (requestId: string) => void; - const awaitBackupKeyRequested: Promise = new Promise((resolve) => { - backupKeyRequested = resolve; - }); +function mockSecretRequestAndGetPromises(): Map> { + let mskRequestDefer = defer(); + let sskRequestDefer = defer(); + let uskRequestDefer = defer(); + let backupKeyRequestDefer = defer(); + fetchMock.put( new RegExp(`/_matrix/client/(r0|v3)/sendToDevice/m.secret.request`), (url: string, opts: RequestInit): MockResponse => { @@ -1559,13 +1545,13 @@ function mockUpSecretRequestAndGetPromises(): Map> { const name = content.name; const requestId = content.request_id; if (name == "m.cross_signing.user_signing") { - uskRequested(requestId); + uskRequestDefer.resolve(requestId); } else if (name == "m.cross_signing.master") { - mskRequested(requestId); + mskRequestDefer.resolve(requestId); } else if (name == "m.cross_signing.self_signing") { - sskRequested(requestId); + sskRequestDefer.resolve(requestId); } else if (name == "m.megolm_backup.v1") { - backupKeyRequested(requestId); + backupKeyRequestDefer.resolve(requestId); } } return {}; @@ -1574,10 +1560,10 @@ function mockUpSecretRequestAndGetPromises(): Map> { ); const promiseMap = new Map>(); - promiseMap.set("m.cross_signing.master", awaitMskRequest); - promiseMap.set("m.cross_signing.self_signing", awaitSskRequested); - promiseMap.set("m.cross_signing.user_signing", awaitUskRequested); - promiseMap.set("m.megolm_backup.v1", awaitBackupKeyRequested); + promiseMap.set("m.cross_signing.master", mskRequestDefer.promise); + promiseMap.set("m.cross_signing.self_signing", sskRequestDefer.promise); + promiseMap.set("m.cross_signing.user_signing", uskRequestDefer.promise); + promiseMap.set("m.megolm_backup.v1", backupKeyRequestDefer.promise); return promiseMap; } diff --git a/src/rust-crypto/backup.ts b/src/rust-crypto/backup.ts index f39bdca9d4f..79f35e6d1a5 100644 --- a/src/rust-crypto/backup.ts +++ b/src/rust-crypto/backup.ts @@ -118,17 +118,17 @@ export class RustBackupManager extends TypedEventEmitter Date: Fri, 13 Oct 2023 15:59:05 +0200 Subject: [PATCH 15/26] refactor move more logic to backup.ts --- spec/integ/crypto/olm-utils.ts | 4 +- spec/integ/crypto/verification.spec.ts | 10 ++--- src/rust-crypto/backup.ts | 59 ++++++++++++++++++++------ src/rust-crypto/rust-crypto.ts | 27 +++--------- 4 files changed, 59 insertions(+), 41 deletions(-) diff --git a/spec/integ/crypto/olm-utils.ts b/spec/integ/crypto/olm-utils.ts index f912f8059d0..3f8144fff5f 100644 --- a/spec/integ/crypto/olm-utils.ts +++ b/spec/integ/crypto/olm-utils.ts @@ -63,14 +63,14 @@ export function getTestOlmAccountKeys(olmAccount: Olm.Account, userId: string, d /** * Bootstrap cross signing for the given Olm account. - * + * * Will generate the cross signing keys and sign them with the master key, and returns the `IDownloadKeyResult` * that can be directly fed into a test e2eKeyResponder. * * The cross-signing keys are randomly generated, similar to how the olm account keys are generated. There may not * be any value in using static vectors, as the device keys change at every test run. * - * If some `KeyBackupInfo` are provided, the `auth_data` of each backup info will be signed with the + * If some `KeyBackupInfo` are provided, the `auth_data` of each backup info will be signed with the * master key, meaning the backups will be then trusted after verification. * * @param olmAccount - The Olm account object to use for signing the device keys. diff --git a/spec/integ/crypto/verification.spec.ts b/spec/integ/crypto/verification.spec.ts index ceb44264c0d..03ae6254f85 100644 --- a/spec/integ/crypto/verification.spec.ts +++ b/spec/integ/crypto/verification.spec.ts @@ -1232,7 +1232,6 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("verification (%s)", (backend: st }); afterEach(async () => { - aliceClient?.stopClient(); testOlmAccount?.free(); @@ -1529,12 +1528,11 @@ function expectSendToDeviceMessage(msgtype: string): Promise<{ messages: any }> * @returns a map of secret name to promise that will resolve (with the id of the secret request) when the secret is requested. */ function mockSecretRequestAndGetPromises(): Map> { + const mskRequestDefer = defer(); + const sskRequestDefer = defer(); + const uskRequestDefer = defer(); + const backupKeyRequestDefer = defer(); - let mskRequestDefer = defer(); - let sskRequestDefer = defer(); - let uskRequestDefer = defer(); - let backupKeyRequestDefer = defer(); - fetchMock.put( new RegExp(`/_matrix/client/(r0|v3)/sendToDevice/m.secret.request`), (url: string, opts: RequestInit): MockResponse => { diff --git a/src/rust-crypto/backup.ts b/src/rust-crypto/backup.ts index 79f35e6d1a5..ad7542815e9 100644 --- a/src/rust-crypto/backup.ts +++ b/src/rust-crypto/backup.ts @@ -108,27 +108,23 @@ export class RustBackupManager extends TypedEventEmitter { + // Currently we only receive the decryption key without any key backup version, it is important to + // check that the secret is valid for the current version before storing it. + // We force a check to ensure to have the latest version. We also want to check that the backup is trusted + // as we don't want to store the secret if the backup is not trusted, and eventually import megolm keys later from an untrusted backup. + const backupCheck = await this.checkKeyBackupAndEnable(true); + if (backupCheck?.backupInfo?.version && backupCheck.trustInfo.trusted) { + try { + const backupDecryptionKey = RustSdkCryptoJs.BackupDecryptionKey.fromBase64(secret); + const privateKeyMatches = this.backupInfoMatchesBackupDecryptionKey( + backupCheck.backupInfo, + backupDecryptionKey, + ); + if (!privateKeyMatches) { + logger.debug(`onReceiveSecret: backup decryption key does not match current backup version`); + // just ignore the secret + return false; + } + logger.info(`onReceiveSecret: Received matching secret ${secret}, store it.`); + + await this.olmMachine.saveBackupDecryptionKey(backupDecryptionKey, backupCheck.backupInfo.version); + return true; + } catch (e) { + logger.warn("backupMatchesPrivateKey: Invalid backup decryption key", e); + } + } + + return false; + } + private keyBackupCheckInProgress: Promise | null = null; /** Helper for `checkKeyBackup` */ diff --git a/src/rust-crypto/rust-crypto.ts b/src/rust-crypto/rust-crypto.ts index 3dfa6b2de8f..fdba73e1c62 100644 --- a/src/rust-crypto/rust-crypto.ts +++ b/src/rust-crypto/rust-crypto.ts @@ -70,7 +70,7 @@ import { TypedReEmitter } from "../ReEmitter"; import { randomString } from "../randomstring"; import { ClientStoppedError } from "../errors"; import { ISignatures } from "../@types/signed"; -import { decodeBase64, encodeBase64 } from "../common-crypto/base64"; +import { encodeBase64 } from "../common-crypto/base64"; import { DecryptionError } from "../crypto/algorithms"; const ALL_VERIFICATION_METHODS = ["m.sas.v1", "m.qr_code.scan.v1", "m.qr_code.show.v1", "m.reciprocate.v1"]; @@ -1373,30 +1373,17 @@ export class RustCrypto extends TypedEventEmitter { this.logger.debug(`onReceiveSecret: Received secret ${name}`); if (name === "m.megolm_backup.v1") { - // Currently we only receive the decryption key without any key backup version, it is important to - // check that the secret is valid for the current version before storing it. - // We force a check to ensure to have the latest version. We also want to check that the backup is trusted - // as we don't want to store the secret if the backup is not trusted, and eventually import megolm keys later from an untrusted backup. - const backupCheck = await this.backupManager.checkKeyBackupAndEnable(true); - if (backupCheck?.backupInfo?.version && backupCheck.trustInfo.trusted) { - const privateKeyMatches = this.backupManager.backupMatchesPrivateKey(backupCheck.backupInfo, value); - - if (!privateKeyMatches) { - this.logger.debug(`onReceiveSecret: backup decryption key does not match current backup version`); - // just ignore the secret - return; - } - this.logger.info(`onReceiveSecret: Received matching secret ${name}, store it.`); + const isHandled = await this.backupManager.handleBackupSecretReceived(value); + + if (isHandled) { + // The secret is valid and stored, clear the inbox. + // Important to call this after storing the secret as good hygiene. + await this.olmMachine.deleteSecretsFromInbox("m.megolm_backup.v1"); - await this.storeSessionBackupPrivateKey(decodeBase64(value), backupCheck.backupInfo.version); // XXXX at this point we should probably try to download the backup and import the keys, // or at least retry for the current decryption failures? // Maybe add some signaling when a new secret is received, and let clients handle it? // as it's where the restore from backup APIs are - - // The secret is valid and stored, clear the inbox. - // Important to call this after storing the secret as good hygiene. - await this.olmMachine.deleteSecretsFromInbox("m.megolm_backup.v1"); } } } From 68870704816b14ba9ddd5571c481a40f3a6c7ff6 Mon Sep 17 00:00:00 2001 From: Valere Date: Fri, 13 Oct 2023 17:12:28 +0200 Subject: [PATCH 16/26] poll secret inbox --- src/rust-crypto/index.ts | 11 +++++------ src/rust-crypto/rust-crypto.ts | 35 +++++++++++++++++++++++++--------- 2 files changed, 31 insertions(+), 15 deletions(-) diff --git a/src/rust-crypto/index.ts b/src/rust-crypto/index.ts index 321c43add52..8440052fd6f 100644 --- a/src/rust-crypto/index.ts +++ b/src/rust-crypto/index.ts @@ -77,14 +77,13 @@ export async function initRustCrypto( // Check if there are any key backup secrets pending processing. There may be multiple secrets to process if several devices have gossiped them. // The `registerReceiveSecretCallback` function will only be triggered for new secrets. If the client is restarted before processing them, the secrets will need to be manually handled. - const pendingValues: string[] = await olmMachine.getSecretsFromInbox("m.megolm_backup.v1"); - pendingValues.forEach((value) => { - rustCrypto.onReceiveSecret("m.megolm_backup.v1", value); - }); + rustCrypto.checkSecrets("m.megolm_backup.v1"); // Register a callback to be notified when a new secret is received, as for now only the key backup secret is supported (the cross signing secrets are handled automatically by the OlmMachine) - await olmMachine.registerReceiveSecretCallback((name: string, value: string) => - rustCrypto.onReceiveSecret(name, value), + await olmMachine.registerReceiveSecretCallback((name: string, _value: string) => + // Instead of directly checking the secret value, we poll the inbox to get all values for that secret type. + // Once we have all the values, we can safely clear the secret inbox. + rustCrypto.checkSecrets(name), ); // Tell the OlmMachine to think about its outgoing requests before we hand control back to the application. diff --git a/src/rust-crypto/rust-crypto.ts b/src/rust-crypto/rust-crypto.ts index fdba73e1c62..9034c54d4ea 100644 --- a/src/rust-crypto/rust-crypto.ts +++ b/src/rust-crypto/rust-crypto.ts @@ -1370,22 +1370,39 @@ export class RustCrypto extends TypedEventEmitter { + private async handleSecretReceived(name: string, value: string): Promise { this.logger.debug(`onReceiveSecret: Received secret ${name}`); if (name === "m.megolm_backup.v1") { const isHandled = await this.backupManager.handleBackupSecretReceived(value); - if (isHandled) { - // The secret is valid and stored, clear the inbox. - // Important to call this after storing the secret as good hygiene. - await this.olmMachine.deleteSecretsFromInbox("m.megolm_backup.v1"); + // XXXX at this point we should probably try to download the backup and import the keys, + // or at least retry for the current decryption failures? + // Maybe add some signaling when a new secret is received, and let clients handle it? + // as it's where the restore from backup APIs are exposed. - // XXXX at this point we should probably try to download the backup and import the keys, - // or at least retry for the current decryption failures? - // Maybe add some signaling when a new secret is received, and let clients handle it? - // as it's where the restore from backup APIs are + return isHandled; + } + return false; + } + + /** + * Called when a new secret is received in the rust secret inbox. + * + * Will poll the secret inbox and handle the secrets received. + * + * @param name - The name of the secret received. + * @returns `true` if the secret has been handled and saved, `false` otherwise. + */ + public async checkSecrets(name: string): Promise { + const pendingValues: string[] = await this.olmMachine.getSecretsFromInbox("m.megolm_backup.v1"); + for (const value of pendingValues) { + if (await this.handleSecretReceived(name, value)) { + break; } } + + // Important to call this after handling the secrets as good hygiene. + await this.olmMachine.deleteSecretsFromInbox(name); } /** From 35a26fa4a093522c48bb67050fb929b8891561e5 Mon Sep 17 00:00:00 2001 From: Valere Date: Mon, 16 Oct 2023 11:59:26 +0200 Subject: [PATCH 17/26] missing mock --- spec/unit/rust-crypto/rust-crypto.spec.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/unit/rust-crypto/rust-crypto.spec.ts b/spec/unit/rust-crypto/rust-crypto.spec.ts index e1aab536dc3..e7cbdb2c659 100644 --- a/spec/unit/rust-crypto/rust-crypto.spec.ts +++ b/spec/unit/rust-crypto/rust-crypto.spec.ts @@ -65,6 +65,7 @@ describe("initRustCrypto", () => { registerRoomKeyUpdatedCallback: jest.fn(), registerUserIdentityUpdatedCallback: jest.fn(), getSecretsFromInbox: jest.fn().mockResolvedValue(["dGhpc2lzYWZha2VzZWNyZXQ="]), + deleteSecretsFromInbox: jest.fn(), registerReceiveSecretCallback: jest.fn(), outgoingRequests: jest.fn(), } as unknown as Mocked; From a4c1747f926fe0beb8f57968456b80bf33f430d3 Mon Sep 17 00:00:00 2001 From: Valere Date: Tue, 17 Oct 2023 16:52:35 +0200 Subject: [PATCH 18/26] Update src/rust-crypto/rust-crypto.ts Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- src/rust-crypto/rust-crypto.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rust-crypto/rust-crypto.ts b/src/rust-crypto/rust-crypto.ts index 9034c54d4ea..babf77d4c03 100644 --- a/src/rust-crypto/rust-crypto.ts +++ b/src/rust-crypto/rust-crypto.ts @@ -1394,7 +1394,7 @@ export class RustCrypto extends TypedEventEmitter { - const pendingValues: string[] = await this.olmMachine.getSecretsFromInbox("m.megolm_backup.v1"); + const pendingValues: string[] = await this.olmMachine.getSecretsFromInbox(name); for (const value of pendingValues) { if (await this.handleSecretReceived(name, value)) { break; From 97dbcddfffebc3c8e87cde19c97ec36c9fa095b1 Mon Sep 17 00:00:00 2001 From: Valere Date: Tue, 17 Oct 2023 16:53:04 +0200 Subject: [PATCH 19/26] Update src/rust-crypto/rust-crypto.ts Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- src/rust-crypto/rust-crypto.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rust-crypto/rust-crypto.ts b/src/rust-crypto/rust-crypto.ts index babf77d4c03..13a4d878343 100644 --- a/src/rust-crypto/rust-crypto.ts +++ b/src/rust-crypto/rust-crypto.ts @@ -1365,7 +1365,7 @@ export class RustCrypto extends TypedEventEmitter Date: Tue, 17 Oct 2023 16:53:30 +0200 Subject: [PATCH 20/26] Update src/rust-crypto/rust-crypto.ts Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- src/rust-crypto/rust-crypto.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rust-crypto/rust-crypto.ts b/src/rust-crypto/rust-crypto.ts index 13a4d878343..ea29c9b7e0b 100644 --- a/src/rust-crypto/rust-crypto.ts +++ b/src/rust-crypto/rust-crypto.ts @@ -1375,7 +1375,7 @@ export class RustCrypto extends TypedEventEmitter Date: Tue, 17 Oct 2023 16:55:10 +0200 Subject: [PATCH 21/26] Update src/rust-crypto/backup.ts Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- src/rust-crypto/backup.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rust-crypto/backup.ts b/src/rust-crypto/backup.ts index ad7542815e9..e0ba3bbe8b3 100644 --- a/src/rust-crypto/backup.ts +++ b/src/rust-crypto/backup.ts @@ -153,7 +153,7 @@ export class RustBackupManager extends TypedEventEmitter { - // Currently we only receive the decryption key without any key backup version, it is important to + // Currently we only receive the decryption key without any key backup version. It is important to // check that the secret is valid for the current version before storing it. // We force a check to ensure to have the latest version. We also want to check that the backup is trusted // as we don't want to store the secret if the backup is not trusted, and eventually import megolm keys later from an untrusted backup. From c455eff623dc1a2cf038f8ad66d6d91c230a1a01 Mon Sep 17 00:00:00 2001 From: Valere Date: Tue, 17 Oct 2023 16:57:01 +0200 Subject: [PATCH 22/26] Update src/rust-crypto/rust-crypto.ts Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- src/rust-crypto/rust-crypto.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rust-crypto/rust-crypto.ts b/src/rust-crypto/rust-crypto.ts index ea29c9b7e0b..7f2ba3d7683 100644 --- a/src/rust-crypto/rust-crypto.ts +++ b/src/rust-crypto/rust-crypto.ts @@ -1373,7 +1373,7 @@ export class RustCrypto extends TypedEventEmitter { this.logger.debug(`onReceiveSecret: Received secret ${name}`); if (name === "m.megolm_backup.v1") { - const isHandled = await this.backupManager.handleBackupSecretReceived(value); + return await this.backupManager.handleBackupSecretReceived(value); // XXX at this point we should probably try to download the backup and import the keys, // or at least retry for the current decryption failures? From e0b55ccda1f3428ee7f7351ff262d776f956352d Mon Sep 17 00:00:00 2001 From: Valere Date: Wed, 18 Oct 2023 09:47:20 +0200 Subject: [PATCH 23/26] code review --- src/rust-crypto/backup.ts | 79 ++++++++++++++++++---------------- src/rust-crypto/rust-crypto.ts | 13 +++--- 2 files changed, 46 insertions(+), 46 deletions(-) diff --git a/src/rust-crypto/backup.ts b/src/rust-crypto/backup.ts index e0ba3bbe8b3..8b3901a44f1 100644 --- a/src/rust-crypto/backup.ts +++ b/src/rust-crypto/backup.ts @@ -106,27 +106,6 @@ export class RustBackupManager extends TypedEventEmitter { this.logger.debug(`onReceiveSecret: Received secret ${name}`); if (name === "m.megolm_backup.v1") { return await this.backupManager.handleBackupSecretReceived(value); - // XXX at this point we should probably try to download the backup and import the keys, // or at least retry for the current decryption failures? // Maybe add some signaling when a new secret is received, and let clients handle it? // as it's where the restore from backup APIs are exposed. - - return isHandled; } return false; } @@ -1391,12 +1387,13 @@ export class RustCrypto extends TypedEventEmitter { const pendingValues: string[] = await this.olmMachine.getSecretsFromInbox(name); for (const value of pendingValues) { if (await this.handleSecretReceived(name, value)) { + // If we have a valid secret for that name there is no point of processing the other secrets values. + // It's probably the same secret shared by another device. break; } } From 4f3803a28c89adb8956e3e1e0609e9238dd767f6 Mon Sep 17 00:00:00 2001 From: Valere Date: Thu, 19 Oct 2023 12:06:18 +0200 Subject: [PATCH 24/26] fix comment --- spec/integ/crypto/verification.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/integ/crypto/verification.spec.ts b/spec/integ/crypto/verification.spec.ts index 03ae6254f85..56a1335e8b9 100644 --- a/spec/integ/crypto/verification.spec.ts +++ b/spec/integ/crypto/verification.spec.ts @@ -1609,7 +1609,7 @@ function buildRequestMessage(transactionId: string): { type: string; content: ob }; } -/** build an m.key.verification.ready to-device message originating from the dummy device */ +/** build an m.key.verification.ready to-device message originating from the given `fromDevice` (default to `TEST_DEVICE_ID` if not provided) */ function buildReadyMessage( transactionId: string, methods: string[], From 61843b6ab2e1b9b4b2737fa9236cdab0078ae788 Mon Sep 17 00:00:00 2001 From: Valere Date: Thu, 19 Oct 2023 14:13:44 +0200 Subject: [PATCH 25/26] remove comment Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- spec/integ/crypto/verification.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/integ/crypto/verification.spec.ts b/spec/integ/crypto/verification.spec.ts index 56a1335e8b9..56ef62b19fb 100644 --- a/spec/integ/crypto/verification.spec.ts +++ b/spec/integ/crypto/verification.spec.ts @@ -1538,7 +1538,7 @@ function mockSecretRequestAndGetPromises(): Map> { (url: string, opts: RequestInit): MockResponse => { const messages = JSON.parse(opts.body as string).messages[TEST_USER_ID]; // rust crypto broadcasts to all devices, old crypto to a specific device, take the first one - const content = Object.values(messages)[0] as any; //messages[TEST_DEVICE_ID] ? messages[TEST_DEVICE_ID] : messages['*']; + const content = Object.values(messages)[0] as any; if (content.action == "request") { const name = content.name; const requestId = content.request_id; From 5eb50c305cc600f8ae3d4953dd78cea4cb75999f Mon Sep 17 00:00:00 2001 From: Valere Date: Thu, 19 Oct 2023 14:24:57 +0200 Subject: [PATCH 26/26] quick factorise --- src/rust-crypto/backup.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/rust-crypto/backup.ts b/src/rust-crypto/backup.ts index 8b3901a44f1..784e5b59995 100644 --- a/src/rust-crypto/backup.ts +++ b/src/rust-crypto/backup.ts @@ -95,11 +95,9 @@ export class RustBackupManager extends TypedEventEmitter