Begin factoring out a CryptoBackend interface (#2955)

Part of element-hq/element-web#21972. Eventually I want to replace the whole of the current `Crypto` implementation with an alternative implementation, but in order to get from here to there, I'm factoring out a common interface which will be implemented by both implementations.

I'm also determined to fix the problem where the innards of the crypto implementation are exposed to applications via the `MatrixClient.crypto` property.

It's not (yet) entirely clear what shape this interface should be, so I'm going with a minimal approach and adding things as we know we need them. This means that we need to keep the old `client.crypto` property around as well as a new `client.cryptoBackend` property. Eventually `client.crypto` will go away, but that will be a breaking change in the js-sdk.

Author: richvdh

spec/integ/megolm-integ.spec.ts

@@ -650,6 +650,131 @@ describe("megolm", () => {
         ]);
     });
 
+    describe("get|setGlobalErrorOnUnknownDevices", () => {
+        it("should raise an error if crypto is disabled", () => {
+            aliceTestClient.client["cryptoBackend"] = undefined;
+            expect(() => aliceTestClient.client.setGlobalErrorOnUnknownDevices(true)).toThrowError(
+                "encryption disabled",
+            );
+            expect(() => aliceTestClient.client.getGlobalErrorOnUnknownDevices()).toThrowError("encryption disabled");
+        });
+
+        it("should permit sending to unknown devices", async () => {
+            expect(aliceTestClient.client.getGlobalErrorOnUnknownDevices()).toBeTruthy();
+
+            aliceTestClient.expectKeyQuery({ device_keys: { "@alice:localhost": {} }, failures: {} });
+            await aliceTestClient.start();
+            const p2pSession = await establishOlmSession(aliceTestClient, testOlmAccount);
+
+            aliceTestClient.httpBackend.when("GET", "/sync").respond(200, getSyncResponse(["@bob:xyz"]));
+            await aliceTestClient.flushSync();
+
+            // start out with the device unknown - the send should be rejected.
+            aliceTestClient.httpBackend.when("POST", "/keys/query").respond(200, getTestKeysQueryResponse("@bob:xyz"));
+            aliceTestClient.httpBackend.when("POST", "/keys/query").respond(200, getTestKeysQueryResponse("@bob:xyz"));
+
+            await Promise.all([
+                aliceTestClient.client.sendTextMessage(ROOM_ID, "test").then(
+                    () => {
+                        throw new Error("sendTextMessage failed on an unknown device");
+                    },
+                    (e) => {
+                        expect(e.name).toEqual("UnknownDeviceError");
+                    },
+                ),
+                aliceTestClient.httpBackend.flushAllExpected(),
+            ]);
+
+            // enable sending to unknown devices, and resend
+            aliceTestClient.client.setGlobalErrorOnUnknownDevices(false);
+            expect(aliceTestClient.client.getGlobalErrorOnUnknownDevices()).toBeFalsy();
+
+            const room = aliceTestClient.client.getRoom(ROOM_ID)!;
+            const pendingMsg = room.getPendingEvents()[0];
+
+            const inboundGroupSessionPromise = expectSendRoomKey(
+                aliceTestClient.httpBackend,
+                "@bob:xyz",
+                testOlmAccount,
+                p2pSession,
+            );
+
+            await Promise.all([
+                aliceTestClient.client.resendEvent(pendingMsg, room),
+                expectSendMegolmMessage(aliceTestClient.httpBackend, inboundGroupSessionPromise),
+            ]);
+        });
+    });
+
+    describe("get|setGlobalBlacklistUnverifiedDevices", () => {
+        it("should raise an error if crypto is disabled", () => {
+            aliceTestClient.client["cryptoBackend"] = undefined;
+            expect(() => aliceTestClient.client.setGlobalBlacklistUnverifiedDevices(true)).toThrowError(
+                "encryption disabled",
+            );
+            expect(() => aliceTestClient.client.getGlobalBlacklistUnverifiedDevices()).toThrowError(
+                "encryption disabled",
+            );
+        });
+
+        it("should disable sending to unverified devices", async () => {
+            aliceTestClient.expectKeyQuery({ device_keys: { "@alice:localhost": {} }, failures: {} });
+            await aliceTestClient.start();
+            const p2pSession = await establishOlmSession(aliceTestClient, testOlmAccount);
+
+            // tell alice we share a room with bob
+            aliceTestClient.httpBackend.when("GET", "/sync").respond(200, getSyncResponse(["@bob:xyz"]));
+            await aliceTestClient.flushSync();
+
+            logger.log("Forcing alice to download our device keys");
+            aliceTestClient.httpBackend.when("POST", "/keys/query").respond(200, getTestKeysQueryResponse("@bob:xyz"));
+            aliceTestClient.httpBackend.when("POST", "/keys/query").respond(200, getTestKeysQueryResponse("@bob:xyz"));
+
+            await Promise.all([
+                aliceTestClient.client.downloadKeys(["@bob:xyz"]),
+                aliceTestClient.httpBackend.flush("/keys/query", 2),
+            ]);
+
+            logger.log("Telling alice to block messages to unverified devices");
+            expect(aliceTestClient.client.getGlobalBlacklistUnverifiedDevices()).toBeFalsy();
+            aliceTestClient.client.setGlobalBlacklistUnverifiedDevices(true);
+            expect(aliceTestClient.client.getGlobalBlacklistUnverifiedDevices()).toBeTruthy();
+
+            logger.log("Telling alice to send a megolm message");
+            aliceTestClient.httpBackend.when("PUT", "/send/").respond(200, { event_id: "$event_id" });
+            aliceTestClient.httpBackend.when("PUT", "/sendToDevice/m.room_key.withheld/").respond(200, {});
+
+            await Promise.all([
+                aliceTestClient.client.sendTextMessage(ROOM_ID, "test"),
+                aliceTestClient.httpBackend.flushAllExpected({ timeout: 1000 }),
+            ]);
+
+            // Now, let's mark the device as verified, and check that keys are sent to it.
+
+            logger.log("Marking the device as verified");
+            // XXX: this is an integration test; we really ought to do this via the cross-signing dance
+            const d = aliceTestClient.client.crypto!.deviceList.getStoredDevice("@bob:xyz", "DEVICE_ID")!;
+            d.verified = DeviceInfo.DeviceVerification.VERIFIED;
+            aliceTestClient.client.crypto?.deviceList.storeDevicesForUser("@bob:xyz", { DEVICE_ID: d });
+
+            const inboundGroupSessionPromise = expectSendRoomKey(
+                aliceTestClient.httpBackend,
+                "@bob:xyz",
+                testOlmAccount,
+                p2pSession,
+            );
+
+            logger.log("Asking alice to re-send");
+            await Promise.all([
+                expectSendMegolmMessage(aliceTestClient.httpBackend, inboundGroupSessionPromise).then((decrypted) => {
+                    expect(decrypted.type).toEqual("m.room.message");
+                    expect(decrypted.content!.body).toEqual("test");
+                }),
+                aliceTestClient.client.sendTextMessage(ROOM_ID, "test"),
+            ]);
+        });
+    });
+
     it("We should start a new megolm session when a device is blocked", async () => {
         aliceTestClient.expectKeyQuery({ device_keys: { "@alice:localhost": {} }, failures: {} });
         await aliceTestClient.start();

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

@@ -1147,7 +1147,7 @@ describe("userHasCrossSigningKeys", function () {
     });
 
     it("throws an error if crypto is disabled", () => {
-        aliceClient.crypto = undefined;
+        aliceClient["cryptoBackend"] = undefined;
         expect(() => aliceClient.userHasCrossSigningKeys()).toThrowError("encryption disabled");
     });
 });

spec/unit/room.spec.ts

@@ -3131,7 +3131,7 @@ describe("Room", function () {
 
     it("should load pending events from from the store and decrypt if needed", async () => {
         const client = new TestClient(userA).client;
-        client.crypto = {
+        client.crypto = client["cryptoBackend"] = {
             decryptEvent: jest.fn().mockResolvedValue({ clearEvent: { body: "enc" } }),
         } as unknown as Crypto;
         client.store.getPendingEvents = jest.fn(async (roomId) => [

src/@types/crypto.ts

@@ -14,7 +14,33 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
+import type { IClearEvent } from "../models/event";
+
 export type OlmGroupSessionExtraData = {
     untrusted?: boolean;
     sharedHistory?: boolean;
 };
+
+/**
+ * The result of a (successful) call to {@link Crypto.decryptEvent}
+ */
+export interface IEventDecryptionResult {
+    /**
+     * The plaintext payload for the event (typically containing <tt>type</tt> and <tt>content</tt> fields).
+     */
+    clearEvent: IClearEvent;
+    /**
+     * List of curve25519 keys involved in telling us about the senderCurve25519Key and claimedEd25519Key.
+     * See {@link MatrixEvent#getForwardingCurve25519KeyChain}.
+     */
+    forwardingCurve25519KeyChain?: string[];
+    /**
+     * Key owned by the sender of this event.  See {@link MatrixEvent#getSenderKey}.
+     */
+    senderCurve25519Key?: string;
+    /**
+     * ed25519 key claimed by the sender of this event. See {@link MatrixEvent#getClaimedEd25519Key}.
+     */
+    claimedEd25519Key?: string;
+    untrusted?: boolean;
+}

src/client.ts

@@ -210,6 +210,7 @@ import { UIARequest, UIAResponse } from "./@types/uia";
 import { LocalNotificationSettings } from "./@types/local_notifications";
 import { UNREAD_THREAD_NOTIFICATIONS } from "./@types/sync";
 import { buildFeatureSupportMap, Feature, ServerSupport } from "./feature";
+import { CryptoBackend } from "./common-crypto/CryptoBackend";
 
 export type Store = IStore;
 
@@ -1147,7 +1148,8 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
     public urlPreviewCache: { [key: string]: Promise<IPreviewUrlResponse> } = {};
     public identityServer?: IIdentityServerProvider;
     public http: MatrixHttpApi<IHttpOpts & { onlyData: true }>; // XXX: Intended private, used in code.
-    public crypto?: Crypto; // XXX: Intended private, used in code.
+    public crypto?: Crypto; // libolm crypto implementation. XXX: Intended private, used in code. Being replaced by cryptoBackend
+    private cryptoBackend?: CryptoBackend; // one of crypto or rustCrypto
     public cryptoCallbacks: ICryptoCallbacks; // XXX: Intended private, used in code.
     public callEventHandler?: CallEventHandler; // XXX: Intended private, used in code.
     public groupCallEventHandler?: GroupCallEventHandler;
@@ -1455,7 +1457,7 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
      * clean shutdown.
      */
     public stopClient(): void {
-        this.crypto?.stop(); // crypto might have been initialised even if the client wasn't fully started
+        this.cryptoBackend?.stop(); // crypto might have been initialised even if the client wasn't fully started
 
         if (!this.clientRunning) return; // already stopped
 
@@ -1959,7 +1961,7 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
     }
 
     /**
-     * Initialise support for end-to-end encryption in this client
+     * Initialise support for end-to-end encryption in this client, using libolm.
      *
      * You should call this method after creating the matrixclient, but *before*
      * calling `startClient`, if you want to support end-to-end encryption.
@@ -1975,7 +1977,7 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
             );
         }
 
-        if (this.crypto) {
+        if (this.cryptoBackend) {
             logger.warn("Attempt to re-initialise e2e encryption on MatrixClient");
             return;
         }
@@ -2040,7 +2042,7 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
 
         // if crypto initialisation was successful, tell it to attach its event handlers.
         crypto.registerEventHandlers(this as Parameters<Crypto["registerEventHandlers"]>[0]);
-        this.crypto = crypto;
+        this.cryptoBackend = this.crypto = crypto;
 
         // upload our keys in the background
         this.crypto.uploadDeviceKeys().catch((e) => {
@@ -2054,7 +2056,7 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
      * @returns True if end-to-end is enabled.
      */
     public isCryptoEnabled(): boolean {
-        return !!this.crypto;
+        return !!this.cryptoBackend;
     }
 
     /**
@@ -2299,21 +2301,21 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
      * @param value - whether to blacklist all unverified devices by default
      */
     public setGlobalBlacklistUnverifiedDevices(value: boolean): boolean {
-        if (!this.crypto) {
+        if (!this.cryptoBackend) {
             throw new Error("End-to-end encryption disabled");
         }
-        this.crypto.globalBlacklistUnverifiedDevices = value;
+        this.cryptoBackend.globalBlacklistUnverifiedDevices = value;
         return value;
     }
 
     /**
      * @returns whether to blacklist all unverified devices by default
      */
     public getGlobalBlacklistUnverifiedDevices(): boolean {
-        if (!this.crypto) {
+        if (!this.cryptoBackend) {
             throw new Error("End-to-end encryption disabled");
         }
-        return this.crypto.globalBlacklistUnverifiedDevices;
+        return this.cryptoBackend.globalBlacklistUnverifiedDevices;
     }
 
     /**
@@ -2327,10 +2329,10 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
      * @param value - whether error on unknown devices
      */
     public setGlobalErrorOnUnknownDevices(value: boolean): void {
-        if (!this.crypto) {
+        if (!this.cryptoBackend) {
             throw new Error("End-to-end encryption disabled");
         }
-        this.crypto.globalErrorOnUnknownDevices = value;
+        this.cryptoBackend.globalErrorOnUnknownDevices = value;
     }
 
     /**
@@ -2339,10 +2341,10 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
      * This API is currently UNSTABLE and may change or be removed without notice.
      */
     public getGlobalErrorOnUnknownDevices(): boolean {
-        if (!this.crypto) {
+        if (!this.cryptoBackend) {
             throw new Error("End-to-end encryption disabled");
         }
-        return this.crypto.globalErrorOnUnknownDevices;
+        return this.cryptoBackend.globalErrorOnUnknownDevices;
     }
 
     /**
@@ -2482,10 +2484,10 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
      * the cross-signing pseudo-device.
      */
     public userHasCrossSigningKeys(): Promise<boolean> {
-        if (!this.crypto) {
+        if (!this.cryptoBackend) {
             throw new Error("End-to-end encryption disabled");
         }
-        return this.crypto.userHasCrossSigningKeys();
+        return this.cryptoBackend.userHasCrossSigningKeys();
     }
 
     /**
@@ -7162,7 +7164,7 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
      */
     public decryptEventIfNeeded(event: MatrixEvent, options?: IDecryptOptions): Promise<void> {
         if (event.shouldAttemptDecryption() && this.isCryptoEnabled()) {
-            event.attemptDecryption(this.crypto!, options);
+            event.attemptDecryption(this.cryptoBackend!, options);
         }
 
         if (event.isBeingDecrypted()) {

src/common-crypto/CryptoBackend.ts

@@ -0,0 +1,63 @@
+/*
+Copyright 2022 The Matrix.org Foundation C.I.C.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+import type { IEventDecryptionResult } from "../@types/crypto";
+import { MatrixEvent } from "../models/event";
+
+/**
+ * Common interface for the crypto implementations
+ */
+export interface CryptoBackend {
+    /**
+     * Global override for whether the client should ever send encrypted
+     * messages to unverified devices. This provides the default for rooms which
+     * do not specify a value.
+     *
+     * If true, all unverified devices will be blacklisted by default
+     */
+    globalBlacklistUnverifiedDevices: boolean;
+
+    /**
+     * Whether sendMessage in a room with unknown and unverified devices
+     * should throw an error and not send the message. This has 'Global' for
+     * symmetry with setGlobalBlacklistUnverifiedDevices but there is currently
+     * no room-level equivalent for this setting.
+     */
+    globalErrorOnUnknownDevices: boolean;
+
+    /**
+     * Shut down any background processes related to crypto
+     */
+    stop(): void;
+
+    /**
+     * Checks if the user has previously published cross-signing keys
+     *
+     * This means downloading the devicelist for the user and checking if the list includes
+     * the cross-signing pseudo-device.
+
+     * @returns true if the user has previously published cross-signing keys
+     */
+    userHasCrossSigningKeys(): Promise<boolean>;
+
+    /**
+     * Decrypt a received event
+     *
+     * @returns a promise which resolves once we have finished decrypting.
+     * Rejects with an error if there is a problem decrypting the event.
+     */
+    decryptEvent(event: MatrixEvent): Promise<IEventDecryptionResult>;
+}

src/common-crypto/README.md

@@ -0,0 +1,4 @@
+This directory contains functionality which is common to both the legacy (libolm-based) crypto implementation,
+and the new rust-based implementation.
+
+It is an internal module, and is _not_ directly exposed to applications.