From 3c59476cf70cbbee857590ea0a90ffbb8e5d089d Mon Sep 17 00:00:00 2001 From: Florian Duros Date: Fri, 23 Jun 2023 15:10:54 +0200 Subject: [PATCH] Element-R: Store cross signing keys in secret storage (#3498) * Store cross signing keys in secret storage * Update `bootstrapSecretStorage` doc * Throw error when `createSecretStorageKey` is not set * Move mocking functions * Store cross signing keys and user signing keys * Fix `awaitCrossSigningKeyUpload` documentation * Remove useless comment * Fix formatting after merge conflict --- spec/integ/crypto/cross-signing.spec.ts | 38 +-------- spec/integ/crypto/crypto.spec.ts | 107 ++++++++++++++++++++---- spec/test-utils/mockEndpoints.ts | 25 ++++++ src/client.ts | 3 + src/crypto-api.ts | 9 +- src/rust-crypto/rust-crypto.ts | 86 +++++++++++++++---- 6 files changed, 201 insertions(+), 67 deletions(-) diff --git a/spec/integ/crypto/cross-signing.spec.ts b/spec/integ/crypto/cross-signing.spec.ts index 16ba6df1d48..4043379e1e7 100644 --- a/spec/integ/crypto/cross-signing.spec.ts +++ b/spec/integ/crypto/cross-signing.spec.ts @@ -19,7 +19,8 @@ import "fake-indexeddb/auto"; import { IDBFactory } from "fake-indexeddb"; import { CRYPTO_BACKENDS, InitCrypto } from "../../test-utils/test-utils"; -import { createClient, MatrixClient, IAuthDict, UIAuthCallback } from "../../../src"; +import { createClient, IAuthDict, MatrixClient } from "../../../src"; +import { mockSetupCrossSigningRequests } from "../../test-utils/mockEndpoints"; afterEach(() => { // reset fake-indexeddb after each test, to make sure we don't leak connections @@ -62,45 +63,14 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("cross-signing (%s)", (backend: s }); /** - * Mock the requests needed to set up cross signing - * - * Return `{}` for `GET _matrix/client/r0/user/:userId/account_data/:type` request - * Return `{}` for `POST _matrix/client/v3/keys/signatures/upload` request (named `upload-sigs` for fetchMock check) - * Return `{}` for `POST /_matrix/client/(unstable|v3)/keys/device_signing/upload` request (named `upload-keys` for fetchMock check) - */ - function mockSetupCrossSigningRequests(): void { - // have account_data requests return an empty object - fetchMock.get("express:/_matrix/client/r0/user/:userId/account_data/:type", {}); - - // we expect a request to upload signatures for our device ... - fetchMock.post({ url: "path:/_matrix/client/v3/keys/signatures/upload", name: "upload-sigs" }, {}); - - // ... and one to upload the cross-signing keys (with UIA) - fetchMock.post( - // legacy crypto uses /unstable/; /v3/ is correct - { - url: new RegExp("/_matrix/client/(unstable|v3)/keys/device_signing/upload"), - name: "upload-keys", - }, - {}, - ); - } - - /** - * Create cross-signing keys, publish the keys - * Mock and bootstrap all the required steps + * Create cross-signing keys and publish the keys * * @param authDict - The parameters to as the `auth` dict in the key upload request. * @see https://spec.matrix.org/v1.6/client-server-api/#authentication-types */ async function bootstrapCrossSigning(authDict: IAuthDict): Promise { - const uiaCallback: UIAuthCallback = async (makeRequest) => { - await makeRequest(authDict); - }; - - // now bootstrap cross signing, and check it resolves successfully await aliceClient.getCrypto()?.bootstrapCrossSigning({ - authUploadDeviceSigningKeys: uiaCallback, + authUploadDeviceSigningKeys: (makeRequest) => makeRequest(authDict).then(() => undefined), }); } diff --git a/spec/integ/crypto/crypto.spec.ts b/spec/integ/crypto/crypto.spec.ts index 89ff7da9d8b..fd751e57f7a 100644 --- a/spec/integ/crypto/crypto.spec.ts +++ b/spec/integ/crypto/crypto.spec.ts @@ -50,8 +50,9 @@ import { ISyncResponder, SyncResponder } from "../../test-utils/SyncResponder"; import { escapeRegExp } from "../../../src/utils"; import { downloadDeviceToJsDevice } from "../../../src/rust-crypto/device-converter"; import { flushPromises } from "../../test-utils/flushPromises"; -import { mockInitialApiRequests } from "../../test-utils/mockEndpoints"; -import { SECRET_STORAGE_ALGORITHM_V1_AES } from "../../../src/secret-storage"; +import { mockInitialApiRequests, mockSetupCrossSigningRequests } from "../../test-utils/mockEndpoints"; +import { AddSecretStorageKeyOpts, SECRET_STORAGE_ALGORITHM_V1_AES } from "../../../src/secret-storage"; +import { CryptoCallbacks } from "../../../src/crypto-api"; const ROOM_ID = "!room:id"; @@ -530,6 +531,27 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("crypto (%s)", (backend: string, }; } + /** + * Create the {@link CryptoCallbacks} + */ + function createCryptoCallbacks(): CryptoCallbacks { + // Store the cached secret storage key and return it when `getSecretStorageKey` is called + let cachedKey: { keyId: string; key: Uint8Array }; + const cacheSecretStorageKey = (keyId: string, keyInfo: AddSecretStorageKeyOpts, key: Uint8Array) => { + cachedKey = { + keyId, + key, + }; + }; + + const getSecretStorageKey = () => Promise.resolve<[string, Uint8Array]>([cachedKey.keyId, cachedKey.key]); + + return { + cacheSecretStorageKey, + getSecretStorageKey, + }; + } + beforeEach( async () => { // anything that we don't have a specific matcher for silently returns a 404 @@ -542,6 +564,7 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("crypto (%s)", (backend: string, userId: "@alice:localhost", accessToken: "akjgkrgjs", deviceId: "xzcvb", + cryptoCallbacks: createCryptoCallbacks(), }); /* set up listeners for /keys/upload and /sync */ @@ -2187,16 +2210,16 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("crypto (%s)", (backend: string, }); /** - * Create a mock to respond to the PUT request `/_matrix/client/r0/user/:userId/account_data/:type` + * Create a mock to respond to the PUT request `/_matrix/client/r0/user/:userId/account_data/:type(m.secret_storage.*)` * Resolved when a key is uploaded (ie in `body.content.key`) * https://spec.matrix.org/v1.6/client-server-api/#put_matrixclientv3useruseridaccount_datatype */ - function awaitKeyStoredInAccountData(): Promise { + function awaitSecretStorageKeyStoredInAccountData(): Promise { return new Promise((resolve) => { // This url is called multiple times during the secret storage bootstrap process // When we received the newly generated key, we return it fetchMock.put( - "express:/_matrix/client/r0/user/:userId/account_data/:type", + "express:/_matrix/client/r0/user/:userId/account_data/:type(m.secret_storage.*)", (url: string, options: RequestInit) => { const content = JSON.parse(options.body as string); @@ -2211,6 +2234,25 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("crypto (%s)", (backend: string, }); } + /** + * Create a mock to respond to the PUT request `/_matrix/client/r0/user/:userId/account_data/m.cross_signing.${key}` + * Resolved when the cross signing key is uploaded + * https://spec.matrix.org/v1.6/client-server-api/#put_matrixclientv3useruseridaccount_datatype + */ + function awaitCrossSigningKeyUpload(key: string): Promise> { + return new Promise((resolve) => { + // Called when the cross signing key is uploaded + fetchMock.put( + `express:/_matrix/client/r0/user/:userId/account_data/m.cross_signing.${key}`, + (url: string, options: RequestInit) => { + const content = JSON.parse(options.body as string); + resolve(content.encrypted); + return {}; + }, + ); + }); + } + /** * Send in the sync response the provided `secretStorageKey` into the account_data field * The key is set for the `m.secret_storage.default_key` and `m.secret_storage.key.${secretStorageKey}` events @@ -2249,12 +2291,14 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("crypto (%s)", (backend: string, await startClientAndAwaitFirstSync(); }); - newBackendOnly("should do no nothing if createSecretStorageKey is not set", async () => { - await aliceClient.getCrypto()!.bootstrapSecretStorage({ setupNewSecretStorage: true }); - - // No key was created - expect(createSecretStorageKey).toHaveBeenCalledTimes(0); - }); + newBackendOnly( + "should throw an error if we are unable to create a key because createSecretStorageKey is not set", + async () => { + await expect( + aliceClient.getCrypto()!.bootstrapSecretStorage({ setupNewSecretStorage: true }), + ).rejects.toThrow("unable to create a new secret storage key, createSecretStorageKey is not set"); + }, + ); newBackendOnly("should create a new key", async () => { const bootstrapPromise = aliceClient @@ -2262,7 +2306,7 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("crypto (%s)", (backend: string, .bootstrapSecretStorage({ setupNewSecretStorage: true, createSecretStorageKey }); // Wait for the key to be uploaded in the account data - const secretStorageKey = await awaitKeyStoredInAccountData(); + const secretStorageKey = await awaitSecretStorageKeyStoredInAccountData(); // Return the newly created key in the sync response sendSyncResponse(secretStorageKey); @@ -2283,7 +2327,7 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("crypto (%s)", (backend: string, const bootstrapPromise = aliceClient.getCrypto()!.bootstrapSecretStorage({ createSecretStorageKey }); // Wait for the key to be uploaded in the account data - const secretStorageKey = await awaitKeyStoredInAccountData(); + const secretStorageKey = await awaitSecretStorageKeyStoredInAccountData(); // Return the newly created key in the sync response sendSyncResponse(secretStorageKey); @@ -2307,7 +2351,7 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("crypto (%s)", (backend: string, .bootstrapSecretStorage({ setupNewSecretStorage: true, createSecretStorageKey }); // Wait for the key to be uploaded in the account data - let secretStorageKey = await awaitKeyStoredInAccountData(); + let secretStorageKey = await awaitSecretStorageKeyStoredInAccountData(); // Return the newly created key in the sync response sendSyncResponse(secretStorageKey); @@ -2321,7 +2365,7 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("crypto (%s)", (backend: string, .bootstrapSecretStorage({ setupNewSecretStorage: true, createSecretStorageKey }); // Wait for the key to be uploaded in the account data - secretStorageKey = await awaitKeyStoredInAccountData(); + secretStorageKey = await awaitSecretStorageKeyStoredInAccountData(); // Return the newly created key in the sync response sendSyncResponse(secretStorageKey); @@ -2333,5 +2377,38 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("crypto (%s)", (backend: string, expect(createSecretStorageKey).toHaveBeenCalledTimes(2); }, ); + + newBackendOnly("should upload cross signing keys", async () => { + mockSetupCrossSigningRequests(); + + // Before setting up secret-storage, bootstrap cross-signing, so that the client has cross-signing keys. + await aliceClient.getCrypto()?.bootstrapCrossSigning({}); + + // Now, when we bootstrap secret-storage, the cross-signing keys should be uploaded. + const bootstrapPromise = aliceClient + .getCrypto()! + .bootstrapSecretStorage({ setupNewSecretStorage: true, createSecretStorageKey }); + + // Wait for the key to be uploaded in the account data + const secretStorageKey = await awaitSecretStorageKeyStoredInAccountData(); + + // Return the newly created key in the sync response + sendSyncResponse(secretStorageKey); + + // Wait for the cross signing keys to be uploaded + const [masterKey, userSigningKey, selfSigningKey] = await Promise.all([ + awaitCrossSigningKeyUpload("master"), + awaitCrossSigningKeyUpload("user_signing"), + awaitCrossSigningKeyUpload("self_signing"), + ]); + + // Finally, wait for bootstrapSecretStorage to finished + await bootstrapPromise; + + // Expect the cross signing master key to be uploaded and to be encrypted with `secretStorageKey` + expect(masterKey[secretStorageKey]).toBeDefined(); + expect(userSigningKey[secretStorageKey]).toBeDefined(); + expect(selfSigningKey[secretStorageKey]).toBeDefined(); + }); }); }); diff --git a/spec/test-utils/mockEndpoints.ts b/spec/test-utils/mockEndpoints.ts index a4c162867fe..bd5bb819a42 100644 --- a/spec/test-utils/mockEndpoints.ts +++ b/spec/test-utils/mockEndpoints.ts @@ -28,3 +28,28 @@ export function mockInitialApiRequests(homeserverUrl: string) { filter_id: "fid", }); } + +/** + * Mock the requests needed to set up cross signing + * + * Return `{}` for `GET _matrix/client/r0/user/:userId/account_data/:type` request + * Return `{}` for `POST _matrix/client/v3/keys/signatures/upload` request (named `upload-sigs` for fetchMock check) + * Return `{}` for `POST /_matrix/client/(unstable|v3)/keys/device_signing/upload` request (named `upload-keys` for fetchMock check) + */ +export function mockSetupCrossSigningRequests(): void { + // have account_data requests return an empty object + fetchMock.get("express:/_matrix/client/r0/user/:userId/account_data/:type", {}); + + // we expect a request to upload signatures for our device ... + fetchMock.post({ url: "path:/_matrix/client/v3/keys/signatures/upload", name: "upload-sigs" }, {}); + + // ... and one to upload the cross-signing keys (with UIA) + fetchMock.post( + // legacy crypto uses /unstable/; /v3/ is correct + { + url: new RegExp("/_matrix/client/(unstable|v3)/keys/device_signing/upload"), + name: "upload-keys", + }, + {}, + ); +} diff --git a/src/client.ts b/src/client.ts index d3e6c4524db..5297eee9d88 100644 --- a/src/client.ts +++ b/src/client.ts @@ -367,6 +367,9 @@ export interface ICreateClientOpts { */ useE2eForGroupCall?: boolean; + /** + * Crypto callbacks provided by the application + */ cryptoCallbacks?: ICryptoCallbacks; /** diff --git a/src/crypto-api.ts b/src/crypto-api.ts index 1a78f11b434..967fc8a61a6 100644 --- a/src/crypto-api.ts +++ b/src/crypto-api.ts @@ -192,12 +192,17 @@ export interface CryptoApi { isSecretStorageReady(): Promise; /** - * Bootstrap the secret storage by creating a new secret storage key and store it in the secret storage. + * Bootstrap the secret storage by creating a new secret storage key, add it in the secret storage and + * store the cross signing keys in the secret storage. * - * - Do nothing if an AES key is already stored in the secret storage and `setupNewKeyBackup` is not set; * - Generate a new key {@link GeneratedSecretStorageKey} with `createSecretStorageKey`. + * Only if `setupNewSecretStorage` is set or if there is no AES key in the secret storage * - Store this key in the secret storage and set it as the default key. * - Call `cryptoCallbacks.cacheSecretStorageKey` if provided. + * - Store the cross signing keys in the secret storage if + * - the cross signing is ready + * - a new key was created during the previous step + * - or the secret storage already contains the cross signing keys * * @param opts - Options object. */ diff --git a/src/rust-crypto/rust-crypto.ts b/src/rust-crypto/rust-crypto.ts index 033063bc437..1b791f3cafd 100644 --- a/src/rust-crypto/rust-crypto.ts +++ b/src/rust-crypto/rust-crypto.ts @@ -387,42 +387,96 @@ export class RustCrypto implements CryptoBackend { createSecretStorageKey, setupNewSecretStorage, }: CreateSecretStorageOpts = {}): Promise { - // If createSecretStorageKey is not set, we stop - if (!createSecretStorageKey) return; + // If an AES Key is already stored in the secret storage and setupNewSecretStorage is not set + // we don't want to create a new key + const isNewSecretStorageKeyNeeded = setupNewSecretStorage || !(await this.secretStorageHasAESKey()); - // See if we already have an AES secret-storage key. - const secretStorageKeyTuple = await this.secretStorage.getKey(); + if (isNewSecretStorageKeyNeeded) { + if (!createSecretStorageKey) { + throw new Error("unable to create a new secret storage key, createSecretStorageKey is not set"); + } - if (secretStorageKeyTuple) { - const [, keyInfo] = secretStorageKeyTuple; + // Create a new storage key and add it to secret storage + const recoveryKey = await createSecretStorageKey(); + await this.addSecretStorageKeyToSecretStorage(recoveryKey); + } - // If an AES Key is already stored in the secret storage and setupNewSecretStorage is not set - // we don't want to create a new key - if (keyInfo.algorithm === SECRET_STORAGE_ALGORITHM_V1_AES && !setupNewSecretStorage) { - return; + const crossSigningStatus: RustSdkCryptoJs.CrossSigningStatus = await this.olmMachine.crossSigningStatus(); + const hasPrivateKeys = + crossSigningStatus.hasMaster && crossSigningStatus.hasSelfSigning && crossSigningStatus.hasUserSigning; + + // If we have cross-signing private keys cached, store them in secret + // storage if they are not there already. + if ( + hasPrivateKeys && + (isNewSecretStorageKeyNeeded || !(await secretStorageContainsCrossSigningKeys(this.secretStorage))) + ) { + const crossSigningPrivateKeys: RustSdkCryptoJs.CrossSigningKeyExport = + await this.olmMachine.exportCrossSigningKeys(); + + if (!crossSigningPrivateKeys.masterKey) { + throw new Error("missing master key in cross signing private keys"); } - } - const recoveryKey = await createSecretStorageKey(); + if (!crossSigningPrivateKeys.userSigningKey) { + throw new Error("missing user signing key in cross signing private keys"); + } + if (!crossSigningPrivateKeys.self_signing_key) { + throw new Error("missing self signing key in cross signing private keys"); + } + + await this.secretStorage.store("m.cross_signing.master", crossSigningPrivateKeys.masterKey); + await this.secretStorage.store("m.cross_signing.user_signing", crossSigningPrivateKeys.userSigningKey); + await this.secretStorage.store("m.cross_signing.self_signing", crossSigningPrivateKeys.self_signing_key); + } + } + + /** + * Add the secretStorage key to the secret storage + * - The secret storage key must have the `keyInfo` field filled + * - The secret storage key is set as the default key of the secret storage + * - Call `cryptoCallbacks.cacheSecretStorageKey` when done + * + * @param secretStorageKey - The secret storage key to add in the secret storage. + */ + private async addSecretStorageKeyToSecretStorage(secretStorageKey: GeneratedSecretStorageKey): Promise { // keyInfo is required to continue - if (!recoveryKey.keyInfo) { - throw new Error("missing keyInfo field in the secret storage key created by createSecretStorageKey"); + if (!secretStorageKey.keyInfo) { + throw new Error("missing keyInfo field in the secret storage key"); } const secretStorageKeyObject = await this.secretStorage.addKey( SECRET_STORAGE_ALGORITHM_V1_AES, - recoveryKey.keyInfo, + secretStorageKey.keyInfo, ); + await this.secretStorage.setDefaultKeyId(secretStorageKeyObject.keyId); this.cryptoCallbacks.cacheSecretStorageKey?.( secretStorageKeyObject.keyId, secretStorageKeyObject.keyInfo, - recoveryKey.privateKey, + secretStorageKey.privateKey, ); } + /** + * Check if a secret storage AES Key is already added in secret storage + * + * @returns True if an AES key is in the secret storage + */ + private async secretStorageHasAESKey(): Promise { + // See if we already have an AES secret-storage key. + const secretStorageKeyTuple = await this.secretStorage.getKey(); + + if (!secretStorageKeyTuple) return false; + + const [, keyInfo] = secretStorageKeyTuple; + + // Check if the key is an AES key + return keyInfo.algorithm === SECRET_STORAGE_ALGORITHM_V1_AES; + } + /** * Implementation of {@link CryptoApi#getCrossSigningStatus} */