From 3de049aeb3c5e67aa785b5fef2b83ca7dd833960 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 22 Nov 2024 15:07:41 +0000 Subject: [PATCH 1/8] Save the key backup key to secret storage When setting up secret storage, if we have a key backup key in cache (like we do for the cross signing secrets). --- src/rust-crypto/rust-crypto.ts | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/rust-crypto/rust-crypto.ts b/src/rust-crypto/rust-crypto.ts index 82760bed13..5bb1a32940 100644 --- a/src/rust-crypto/rust-crypto.ts +++ b/src/rust-crypto/rust-crypto.ts @@ -841,6 +841,20 @@ export class RustCrypto extends TypedEventEmitter Date: Fri, 22 Nov 2024 17:22:07 +0000 Subject: [PATCH 2/8] Add test --- spec/unit/rust-crypto/rust-crypto.spec.ts | 41 +++++++++++++++++++++++ src/rust-crypto/rust-crypto.ts | 4 +-- 2 files changed, 42 insertions(+), 3 deletions(-) diff --git a/spec/unit/rust-crypto/rust-crypto.spec.ts b/spec/unit/rust-crypto/rust-crypto.spec.ts index 8fedadd0d4..30922ef94a 100644 --- a/spec/unit/rust-crypto/rust-crypto.spec.ts +++ b/spec/unit/rust-crypto/rust-crypto.spec.ts @@ -705,6 +705,47 @@ describe("RustCrypto", () => { expect(resetKeyBackup.mock.calls).toHaveLength(2); }); + it("bootstrapSecretStorage saves megolm backup key if already cached", async () => { + const secretStorageCallbacks = { + getSecretStorageKey: async (keys: any, name: string) => { + return [[...Object.keys(keys.keys)][0], new Uint8Array(32)]; + }, + } as SecretStorageCallbacks; + const secretStorage = new ServerSideSecretStorageImpl(new DummyAccountDataClient(), secretStorageCallbacks); + + const rustCrypto = await makeTestRustCrypto( + { + authedRequest: jest.fn().mockImplementation((method, query) => { + if (query === "/room_keys/version") { + return Promise.resolve({ version: 1, algorithm: "dummy", auth_data: {} }); + } + return Promise.resolve({}); + }), + } as unknown as MatrixHttpApi, + testData.TEST_USER_ID, + undefined, + secretStorage, + ); + + async function createSecretStorageKey() { + return { + keyInfo: {} as AddSecretStorageKeyOpts, + privateKey: new Uint8Array(32), + }; + } + + jest.spyOn(rustCrypto, "getSessionBackupPrivateKey").mockResolvedValue(new Uint8Array(32)); + const storeSpy = jest.spyOn(secretStorage, "store"); + + await rustCrypto.bootstrapSecretStorage({ + createSecretStorageKey, + setupNewSecretStorage: true, + setupNewKeyBackup: false, + }); + + expect(storeSpy).toHaveBeenCalledWith("m.megolm_backup.v1", expect.anything()); + }); + it("isSecretStorageReady", async () => { const mockSecretStorage = { getDefaultKeyId: jest.fn().mockResolvedValue(null), diff --git a/src/rust-crypto/rust-crypto.ts b/src/rust-crypto/rust-crypto.ts index 5bb1a32940..6610025ab0 100644 --- a/src/rust-crypto/rust-crypto.ts +++ b/src/rust-crypto/rust-crypto.ts @@ -853,9 +853,7 @@ export class RustCrypto extends TypedEventEmitter Date: Mon, 25 Nov 2024 11:58:56 +0000 Subject: [PATCH 3/8] Get the key directly from the olmMachine saves converting it needlessly into a buffer to turn it back into a base64 string --- src/rust-crypto/rust-crypto.ts | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/rust-crypto/rust-crypto.ts b/src/rust-crypto/rust-crypto.ts index 6610025ab0..87f884181a 100644 --- a/src/rust-crypto/rust-crypto.ts +++ b/src/rust-crypto/rust-crypto.ts @@ -844,14 +844,15 @@ export class RustCrypto extends TypedEventEmitter Date: Mon, 25 Nov 2024 15:40:15 +0000 Subject: [PATCH 4/8] Overwrite backup keyin storage if different --- src/rust-crypto/rust-crypto.ts | 42 ++++++++++++++++++++++++++-------- 1 file changed, 32 insertions(+), 10 deletions(-) diff --git a/src/rust-crypto/rust-crypto.ts b/src/rust-crypto/rust-crypto.ts index 87f884181a..2da0693246 100644 --- a/src/rust-crypto/rust-crypto.ts +++ b/src/rust-crypto/rust-crypto.ts @@ -844,21 +844,43 @@ export class RustCrypto extends TypedEventEmitter { + const keyBackupInfo = await this.backupManager.getServerBackupInfo(); + if (!keyBackupInfo || !keyBackupInfo.version) { + logger.info("Not saving backup key to secret storage: no backup info"); + return; + } + + const backupKeys: RustSdkCryptoJs.BackupKeys = await this.olmMachine.getBackupKeys(); + if (!backupKeys.decryptionKey) { + logger.info("Not saving backup key to secret storage: no backup key"); + return; + } + + if (!decryptionKeyMatchesKeyBackupInfo(backupKeys.decryptionKey, keyBackupInfo)) { + logger.info("Not saving backup key to secret storage: decryption key does not match backup info"); + } + + const backupKeyFromStorage = await this.secretStorage.get("m.megolm_backup.v1"); + const backupKeyBase64 = backupKeys.decryptionKey.toBase64(); + + // The backup version that the key corresponds to isn't saved in 4S so if it's different, we must assume + // it's stale and overwrite. + if (backupKeyFromStorage !== backupKeyBase64) { + await this.secretStorage.store("m.megolm_backup.v1", backupKeyBase64); + } + } + /** * Add the secretStorage key to the secret storage * - The secret storage key must have the `keyInfo` field filled From aebadaa6632f8c6e890a96907ad69929f21c2cf7 Mon Sep 17 00:00:00 2001 From: David Baker Date: Tue, 26 Nov 2024 13:38:49 +0000 Subject: [PATCH 5/8] Fix test --- spec/unit/rust-crypto/rust-crypto.spec.ts | 30 ++++++++++++++++------- src/rust-crypto/rust-crypto.ts | 2 +- 2 files changed, 22 insertions(+), 10 deletions(-) diff --git a/spec/unit/rust-crypto/rust-crypto.spec.ts b/spec/unit/rust-crypto/rust-crypto.spec.ts index 30922ef94a..06667cccb1 100644 --- a/spec/unit/rust-crypto/rust-crypto.spec.ts +++ b/spec/unit/rust-crypto/rust-crypto.spec.ts @@ -713,15 +713,26 @@ describe("RustCrypto", () => { } as SecretStorageCallbacks; const secretStorage = new ServerSideSecretStorageImpl(new DummyAccountDataClient(), secretStorageCallbacks); - const rustCrypto = await makeTestRustCrypto( - { - authedRequest: jest.fn().mockImplementation((method, query) => { - if (query === "/room_keys/version") { - return Promise.resolve({ version: 1, algorithm: "dummy", auth_data: {} }); + let backupAuthData: any; + let backupAlg: string; + + const fetchMock = { + authedRequest: jest.fn().mockImplementation((method, path, query, body) => { + if (path === "/room_keys/version") { + if (method === "POST") { + backupAuthData = body["auth_data"]; + backupAlg = body["algorithm"]; + return Promise.resolve({ version: "1", algorithm: backupAlg, auth_data: backupAuthData }); + } else if (method === "GET" && backupAuthData) { + return Promise.resolve({ version: "1", algorithm: backupAlg, auth_data: backupAuthData }); } - return Promise.resolve({}); - }), - } as unknown as MatrixHttpApi, + } + return Promise.resolve({}); + }), + }; + + const rustCrypto = await makeTestRustCrypto( + fetchMock as unknown as MatrixHttpApi, testData.TEST_USER_ID, undefined, secretStorage, @@ -734,7 +745,8 @@ describe("RustCrypto", () => { }; } - jest.spyOn(rustCrypto, "getSessionBackupPrivateKey").mockResolvedValue(new Uint8Array(32)); + await rustCrypto.resetKeyBackup(); + const storeSpy = jest.spyOn(secretStorage, "store"); await rustCrypto.bootstrapSecretStorage({ diff --git a/src/rust-crypto/rust-crypto.ts b/src/rust-crypto/rust-crypto.ts index 2da0693246..a9e9f15466 100644 --- a/src/rust-crypto/rust-crypto.ts +++ b/src/rust-crypto/rust-crypto.ts @@ -844,7 +844,7 @@ export class RustCrypto extends TypedEventEmitter Date: Mon, 2 Dec 2024 10:50:41 +0000 Subject: [PATCH 6/8] Add integ test --- spec/integ/crypto/crypto.spec.ts | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/spec/integ/crypto/crypto.spec.ts b/spec/integ/crypto/crypto.spec.ts index d72b3d6a26..1a985a8f9a 100644 --- a/spec/integ/crypto/crypto.spec.ts +++ b/spec/integ/crypto/crypto.spec.ts @@ -3121,6 +3121,32 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("crypto (%s)", (backend: string, const mskId = await aliceClient.getCrypto()!.getCrossSigningKeyId(CrossSigningKey.Master)!; expect(signatures![aliceClient.getUserId()!][`ed25519:${mskId}`]).toBeDefined(); }); + + newBackendOnly("should upload existing megolm backup key to a new 4S store", async () => { + const backupKeyTo4SPromise = awaitMegolmBackupKeyUpload(); + + // we need these to set up the mocks but we don't actually care whether they + // resolve because we're not testing those things in this test. + awaitCrossSigningKeyUpload("master"); + awaitCrossSigningKeyUpload("user_signing"); + awaitCrossSigningKeyUpload("self_signing"); + awaitSecretStorageKeyStoredInAccountData(); + + mockSetupCrossSigningRequests(); + mockSetupMegolmBackupRequests("1"); + + await aliceClient.getCrypto()!.bootstrapCrossSigning({}); + await aliceClient.getCrypto()!.resetKeyBackup(); + + await aliceClient.getCrypto()!.bootstrapSecretStorage({ + setupNewSecretStorage: true, + createSecretStorageKey, + setupNewKeyBackup: false, + }); + + await backupKeyTo4SPromise; + expect(accountDataAccumulator.accountDataEvents.get("m.megolm_backup.v1")).toBeDefined(); + }); }); describe("Manage Key Backup", () => { From 9c6bd63398760dce4f943d03b681d93b00a33f77 Mon Sep 17 00:00:00 2001 From: David Baker Date: Mon, 2 Dec 2024 11:58:51 +0000 Subject: [PATCH 7/8] Test failure case for sonar --- spec/unit/rust-crypto/rust-crypto.spec.ts | 103 +++++++++++++++++----- 1 file changed, 82 insertions(+), 21 deletions(-) diff --git a/spec/unit/rust-crypto/rust-crypto.spec.ts b/spec/unit/rust-crypto/rust-crypto.spec.ts index 06667cccb1..9fd142e6e0 100644 --- a/spec/unit/rust-crypto/rust-crypto.spec.ts +++ b/spec/unit/rust-crypto/rust-crypto.spec.ts @@ -26,6 +26,7 @@ import { } from "@matrix-org/matrix-sdk-crypto-wasm"; import { mocked, Mocked } from "jest-mock"; import fetchMock from "fetch-mock-jest"; +import { sign } from "crypto"; import { RustCrypto } from "../../../src/rust-crypto/rust-crypto"; import { initRustCrypto } from "../../../src/rust-crypto"; @@ -705,13 +706,13 @@ describe("RustCrypto", () => { expect(resetKeyBackup.mock.calls).toHaveLength(2); }); - it("bootstrapSecretStorage saves megolm backup key if already cached", async () => { + describe("upload existing key backup key to new 4S store", () => { const secretStorageCallbacks = { getSecretStorageKey: async (keys: any, name: string) => { return [[...Object.keys(keys.keys)][0], new Uint8Array(32)]; }, } as SecretStorageCallbacks; - const secretStorage = new ServerSideSecretStorageImpl(new DummyAccountDataClient(), secretStorageCallbacks); + let secretStorage: ServerSideSecretStorageImpl; let backupAuthData: any; let backupAlg: string; @@ -731,31 +732,91 @@ describe("RustCrypto", () => { }), }; - const rustCrypto = await makeTestRustCrypto( - fetchMock as unknown as MatrixHttpApi, - testData.TEST_USER_ID, - undefined, - secretStorage, - ); + beforeEach(() => { + backupAuthData = undefined; + backupAlg = ""; - async function createSecretStorageKey() { - return { - keyInfo: {} as AddSecretStorageKeyOpts, - privateKey: new Uint8Array(32), - }; - } + secretStorage = new ServerSideSecretStorageImpl(new DummyAccountDataClient(), secretStorageCallbacks); + }); - await rustCrypto.resetKeyBackup(); + it("bootstrapSecretStorage saves megolm backup key if already cached", async () => { + const rustCrypto = await makeTestRustCrypto( + fetchMock as unknown as MatrixHttpApi, + testData.TEST_USER_ID, + undefined, + secretStorage, + ); - const storeSpy = jest.spyOn(secretStorage, "store"); + async function createSecretStorageKey() { + return { + keyInfo: {} as AddSecretStorageKeyOpts, + privateKey: new Uint8Array(32), + }; + } - await rustCrypto.bootstrapSecretStorage({ - createSecretStorageKey, - setupNewSecretStorage: true, - setupNewKeyBackup: false, + await rustCrypto.resetKeyBackup(); + + const storeSpy = jest.spyOn(secretStorage, "store"); + + await rustCrypto.bootstrapSecretStorage({ + createSecretStorageKey, + setupNewSecretStorage: true, + setupNewKeyBackup: false, + }); + + expect(storeSpy).toHaveBeenCalledWith("m.megolm_backup.v1", expect.anything()); }); - expect(storeSpy).toHaveBeenCalledWith("m.megolm_backup.v1", expect.anything()); + it("bootstrapSecretStorage doesn't try to save megolm backup key not in cache", async () => { + const mockOlmMachine = { + isBackupEnabled: jest.fn().mockResolvedValue(false), + sign: jest.fn().mockResolvedValue({ + asJSON: jest.fn().mockReturnValue("{}"), + }), + saveBackupDecryptionKey: jest.fn(), + crossSigningStatus: jest.fn().mockResolvedValue({ + hasMaster: true, + hasSelfSigning: true, + hasUserSigning: true, + }), + exportCrossSigningKeys: jest.fn().mockResolvedValue({ + masterKey: "sosecret", + userSigningKey: "secrets", + self_signing_key: "ssshhh", + }), + getBackupKeys: jest.fn().mockResolvedValue({}), + verifyBackup: jest.fn().mockResolvedValue({ trusted: jest.fn().mockReturnValue(false) }), + } as unknown as OlmMachine; + + const rustCrypto = new RustCrypto( + logger, + mockOlmMachine, + fetchMock as unknown as MatrixHttpApi, + TEST_USER, + TEST_DEVICE_ID, + secretStorage, + {} as CryptoCallbacks, + ); + + async function createSecretStorageKey() { + return { + keyInfo: {} as AddSecretStorageKeyOpts, + privateKey: new Uint8Array(32), + }; + } + + await rustCrypto.resetKeyBackup(); + + const storeSpy = jest.spyOn(secretStorage, "store"); + + await rustCrypto.bootstrapSecretStorage({ + createSecretStorageKey, + setupNewSecretStorage: true, + setupNewKeyBackup: false, + }); + + expect(storeSpy).not.toHaveBeenCalledWith("m.megolm_backup.v1", expect.anything()); + }); }); it("isSecretStorageReady", async () => { From 61f30997637c1bd3efb99d943f0b2a3433fc24ab Mon Sep 17 00:00:00 2001 From: David Baker Date: Mon, 2 Dec 2024 12:00:19 +0000 Subject: [PATCH 8/8] Unused import --- spec/unit/rust-crypto/rust-crypto.spec.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/spec/unit/rust-crypto/rust-crypto.spec.ts b/spec/unit/rust-crypto/rust-crypto.spec.ts index 9fd142e6e0..be6dfdb965 100644 --- a/spec/unit/rust-crypto/rust-crypto.spec.ts +++ b/spec/unit/rust-crypto/rust-crypto.spec.ts @@ -26,7 +26,6 @@ import { } from "@matrix-org/matrix-sdk-crypto-wasm"; import { mocked, Mocked } from "jest-mock"; import fetchMock from "fetch-mock-jest"; -import { sign } from "crypto"; import { RustCrypto } from "../../../src/rust-crypto/rust-crypto"; import { initRustCrypto } from "../../../src/rust-crypto";