From 9ecb66e6952bcc4ac36961885fb09699a0015a77 Mon Sep 17 00:00:00 2001 From: Valere Date: Mon, 30 Sep 2024 16:26:34 +0200 Subject: [PATCH] crypto: configure key sharing strategy based on DeviceIsolationMode (#4425) * crypto: configure key sharing strategy based on deviceIsolationMode fix eslint import error cryptoMode was renamed to deviceIsolationMode post rebase fix: Device Isolation mode name changes * Fix outdated docs referring to old cryptomode * code review: better comment for globalBlacklistUnverifiedDevices option * RoomEncryptor: Use appropriate default for getBlacklistUnverifiedDevices * do not provide a default value for DeviceIsolationMode for encryption * Update src/rust-crypto/RoomEncryptor.ts --------- Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- spec/unit/rust-crypto/RoomEncryptor.spec.ts | 121 ++++++++++++++++++-- src/crypto-api/index.ts | 5 +- src/rust-crypto/RoomEncryptor.ts | 60 +++++++--- src/rust-crypto/rust-crypto.ts | 4 +- 4 files changed, 161 insertions(+), 29 deletions(-) diff --git a/spec/unit/rust-crypto/RoomEncryptor.spec.ts b/spec/unit/rust-crypto/RoomEncryptor.spec.ts index 608d9643e..7e20a9504 100644 --- a/spec/unit/rust-crypto/RoomEncryptor.spec.ts +++ b/spec/unit/rust-crypto/RoomEncryptor.spec.ts @@ -17,6 +17,7 @@ */ import { + CollectStrategy, Curve25519PublicKey, Ed25519PublicKey, HistoryVisibility as RustHistoryVisibility, @@ -31,6 +32,7 @@ import { KeyClaimManager } from "../../../src/rust-crypto/KeyClaimManager"; import { defer } from "../../../src/utils"; import { OutgoingRequestsManager } from "../../../src/rust-crypto/OutgoingRequestsManager"; import { KnownMembership } from "../../../src/@types/membership"; +import { DeviceIsolationMode, AllDevicesIsolationMode, OnlySignedDevicesIsolationMode } from "../../../src/crypto-api"; describe("RoomEncryptor", () => { describe("History Visibility", () => { @@ -99,7 +101,7 @@ describe("RoomEncryptor", () => { getEncryptionTargetMembers: jest.fn().mockReturnValue([mockRoomMember]), shouldEncryptForInvitedMembers: jest.fn().mockReturnValue(true), getHistoryVisibility: jest.fn().mockReturnValue(HistoryVisibility.Invited), - getBlacklistUnverifiedDevices: jest.fn().mockReturnValue(false), + getBlacklistUnverifiedDevices: jest.fn().mockReturnValue(null), } as unknown as Mocked; roomEncryptor = new RoomEncryptor( @@ -111,6 +113,8 @@ describe("RoomEncryptor", () => { ); }); + const defaultDevicesIsolationMode = new AllDevicesIsolationMode(false); + it("should ensure that there is only one shareRoomKey at a time", async () => { const deferredShare = defer(); const insideOlmShareRoom = defer(); @@ -120,19 +124,19 @@ describe("RoomEncryptor", () => { await deferredShare.promise; }); - roomEncryptor.prepareForEncryption(false); + roomEncryptor.prepareForEncryption(false, defaultDevicesIsolationMode); await insideOlmShareRoom.promise; // call several times more - roomEncryptor.prepareForEncryption(false); - roomEncryptor.encryptEvent(createMockEvent("Hello"), false); - roomEncryptor.prepareForEncryption(false); - roomEncryptor.encryptEvent(createMockEvent("World"), false); + roomEncryptor.prepareForEncryption(false, defaultDevicesIsolationMode); + roomEncryptor.encryptEvent(createMockEvent("Hello"), false, defaultDevicesIsolationMode); + roomEncryptor.prepareForEncryption(false, defaultDevicesIsolationMode); + roomEncryptor.encryptEvent(createMockEvent("World"), false, defaultDevicesIsolationMode); expect(mockOlmMachine.shareRoomKey).toHaveBeenCalledTimes(1); deferredShare.resolve(); - await roomEncryptor.prepareForEncryption(false); + await roomEncryptor.prepareForEncryption(false, defaultDevicesIsolationMode); // should have been called again expect(mockOlmMachine.shareRoomKey).toHaveBeenCalledTimes(6); @@ -158,8 +162,16 @@ describe("RoomEncryptor", () => { let firstMessageFinished: string | null = null; - const firstRequest = roomEncryptor.encryptEvent(createMockEvent("Hello"), false); - const secondRequest = roomEncryptor.encryptEvent(createMockEvent("Edit of Hello"), false); + const firstRequest = roomEncryptor.encryptEvent( + createMockEvent("Hello"), + false, + defaultDevicesIsolationMode, + ); + const secondRequest = roomEncryptor.encryptEvent( + createMockEvent("Edit of Hello"), + false, + defaultDevicesIsolationMode, + ); firstRequest.then(() => { if (firstMessageFinished === null) { @@ -181,5 +193,96 @@ describe("RoomEncryptor", () => { expect(firstMessageFinished).toBe("hello"); }); + + describe("DeviceIsolationMode", () => { + type TestCase = [ + string, + { + mode: DeviceIsolationMode; + expectedStrategy: CollectStrategy; + globalBlacklistUnverifiedDevices: boolean; + }, + ]; + + const testCases: TestCase[] = [ + [ + "Share AllDevicesIsolationMode", + { + mode: new AllDevicesIsolationMode(false), + expectedStrategy: CollectStrategy.deviceBasedStrategy(false, false), + globalBlacklistUnverifiedDevices: false, + }, + ], + [ + "Share AllDevicesIsolationMode - with blacklist unverified", + { + mode: new AllDevicesIsolationMode(false), + expectedStrategy: CollectStrategy.deviceBasedStrategy(true, false), + globalBlacklistUnverifiedDevices: true, + }, + ], + [ + "Share OnlySigned - blacklist true", + { + mode: new OnlySignedDevicesIsolationMode(), + expectedStrategy: CollectStrategy.identityBasedStrategy(), + globalBlacklistUnverifiedDevices: true, + }, + ], + [ + "Share OnlySigned", + { + mode: new OnlySignedDevicesIsolationMode(), + expectedStrategy: CollectStrategy.identityBasedStrategy(), + globalBlacklistUnverifiedDevices: false, + }, + ], + [ + "Share AllDevicesIsolationMode - Verified user problems true", + { + mode: new AllDevicesIsolationMode(true), + expectedStrategy: CollectStrategy.deviceBasedStrategy(false, true), + globalBlacklistUnverifiedDevices: false, + }, + ], + [ + 'Share AllDevicesIsolationMode - with blacklist unverified - Verified user problems true"', + { + mode: new AllDevicesIsolationMode(true), + expectedStrategy: CollectStrategy.deviceBasedStrategy(true, true), + globalBlacklistUnverifiedDevices: true, + }, + ], + ]; + + let capturedSettings: CollectStrategy | undefined = undefined; + + beforeEach(() => { + capturedSettings = undefined; + mockOlmMachine.shareRoomKey.mockImplementationOnce(async (roomId, users, encryptionSettings) => { + capturedSettings = encryptionSettings.sharingStrategy; + }); + }); + + it.each(testCases)( + "prepareForEncryption should properly set sharing strategy based on crypto mode: %s", + async (_, { mode, expectedStrategy, globalBlacklistUnverifiedDevices }) => { + await roomEncryptor.prepareForEncryption(globalBlacklistUnverifiedDevices, mode); + expect(mockOlmMachine.shareRoomKey).toHaveBeenCalled(); + expect(capturedSettings).toBeDefined(); + expect(expectedStrategy.eq(capturedSettings!)).toBeTruthy(); + }, + ); + + it.each(testCases)( + "encryptEvent should properly set sharing strategy based on crypto mode: %s", + async (_, { mode, expectedStrategy, globalBlacklistUnverifiedDevices }) => { + await roomEncryptor.encryptEvent(createMockEvent("Hello"), globalBlacklistUnverifiedDevices, mode); + expect(mockOlmMachine.shareRoomKey).toHaveBeenCalled(); + expect(capturedSettings).toBeDefined(); + expect(expectedStrategy.eq(capturedSettings!)).toBeTruthy(); + }, + ); + }); }); }); diff --git a/src/crypto-api/index.ts b/src/crypto-api/index.ts index 4503a72ce..0ac684e8c 100644 --- a/src/crypto-api/index.ts +++ b/src/crypto-api/index.ts @@ -611,14 +611,13 @@ export enum DecryptionFailureCode { /** * The sender device is not cross-signed. This will only be used if the - * crypto mode is set to `CryptoMode.Invisible` or `CryptoMode.Transition`. + * device isolation mode is set to `OnlySignedDevicesIsolationMode`. */ UNSIGNED_SENDER_DEVICE = "UNSIGNED_SENDER_DEVICE", /** * We weren't able to link the message back to any known device. This will - * only be used if the crypto mode is set to `CryptoMode.Invisible` or - * `CryptoMode.Transition`. + * only be used if the device isolation mode is set to `OnlySignedDevicesIsolationMode`. */ UNKNOWN_SENDER_DEVICE = "UNKNOWN_SENDER_DEVICE", diff --git a/src/rust-crypto/RoomEncryptor.ts b/src/rust-crypto/RoomEncryptor.ts index fcda295eb..e87b8c901 100644 --- a/src/rust-crypto/RoomEncryptor.ts +++ b/src/rust-crypto/RoomEncryptor.ts @@ -36,6 +36,7 @@ import { HistoryVisibility } from "../@types/partials.ts"; import { OutgoingRequestsManager } from "./OutgoingRequestsManager.ts"; import { logDuration } from "../utils.ts"; import { KnownMembership } from "../@types/membership.ts"; +import { DeviceIsolationMode, DeviceIsolationModeKind } from "../crypto-api/index.ts"; /** * RoomEncryptor: responsible for encrypting messages to a given room @@ -119,10 +120,15 @@ export class RoomEncryptor { * * This ensures that we have a megolm session ready to use and that we have shared its key with all the devices * in the room. - * - * @param globalBlacklistUnverifiedDevices - When `true`, it will not send encrypted messages to unverified devices + * @param globalBlacklistUnverifiedDevices - When `true`, and `deviceIsolationMode` is `AllDevicesIsolationMode`, + * will not send encrypted messages to unverified devices. + * Ignored when `deviceIsolationMode` is `OnlySignedDevicesIsolationMode`. + * @param deviceIsolationMode - The device isolation mode. See {@link DeviceIsolationMode}. */ - public async prepareForEncryption(globalBlacklistUnverifiedDevices: boolean): Promise { + public async prepareForEncryption( + globalBlacklistUnverifiedDevices: boolean, + deviceIsolationMode: DeviceIsolationMode, + ): Promise { // We consider a prepareForEncryption as an encryption promise as it will potentially share keys // even if it doesn't send an event. // Usually this is called when the user starts typing, so we want to make sure we have keys ready when the @@ -130,7 +136,7 @@ export class RoomEncryptor { // If `encryptEvent` is invoked before `prepareForEncryption` has completed, the `encryptEvent` call will wait for // `prepareForEncryption` to complete before executing. // The part where `encryptEvent` shares the room key will then usually be a no-op as it was already performed by `prepareForEncryption`. - await this.encryptEvent(null, globalBlacklistUnverifiedDevices); + await this.encryptEvent(null, globalBlacklistUnverifiedDevices, deviceIsolationMode); } /** @@ -140,9 +146,16 @@ export class RoomEncryptor { * then, if an event is provided, encrypt it using the session. * * @param event - Event to be encrypted, or null if only preparing for encryption (in which case we will pre-share the room key). - * @param globalBlacklistUnverifiedDevices - When `true`, it will not send encrypted messages to unverified devices + * @param globalBlacklistUnverifiedDevices - When `true`, and `deviceIsolationMode` is `AllDevicesIsolationMode`, + * will not send encrypted messages to unverified devices. + * Ignored when `deviceIsolationMode` is `OnlySignedDevicesIsolationMode`. + * @param deviceIsolationMode - The device isolation mode. See {@link DeviceIsolationMode}. */ - public encryptEvent(event: MatrixEvent | null, globalBlacklistUnverifiedDevices: boolean): Promise { + public encryptEvent( + event: MatrixEvent | null, + globalBlacklistUnverifiedDevices: boolean, + deviceIsolationMode: DeviceIsolationMode, + ): Promise { const logger = new LogSpan(this.prefixedLogger, event ? (event.getTxnId() ?? "") : "prepareForEncryption"); // Ensure order of encryption to avoid message ordering issues, as the scheduler only ensures // events order after they have been encrypted. @@ -153,7 +166,7 @@ export class RoomEncryptor { }) .then(async () => { await logDuration(logger, "ensureEncryptionSession", async () => { - await this.ensureEncryptionSession(logger, globalBlacklistUnverifiedDevices); + await this.ensureEncryptionSession(logger, globalBlacklistUnverifiedDevices, deviceIsolationMode); }); if (event) { await logDuration(logger, "encryptEventInner", async () => { @@ -173,9 +186,16 @@ export class RoomEncryptor { * in the room. * * @param logger - a place to write diagnostics to - * @param globalBlacklistUnverifiedDevices - When `true`, it will not send encrypted messages to unverified devices + * @param globalBlacklistUnverifiedDevices - When `true`, and `deviceIsolationMode` is `AllDevicesIsolationMode`, + * will not send encrypted messages to unverified devices. + * Ignored when `deviceIsolationMode` is `OnlySignedDevicesIsolationMode`. + * @param deviceIsolationMode - The device isolation mode. See {@link DeviceIsolationMode}. */ - private async ensureEncryptionSession(logger: LogSpan, globalBlacklistUnverifiedDevices: boolean): Promise { + private async ensureEncryptionSession( + logger: LogSpan, + globalBlacklistUnverifiedDevices: boolean, + deviceIsolationMode: DeviceIsolationMode, + ): Promise { if (this.encryptionSettings.algorithm !== "m.megolm.v1.aes-sha2") { throw new Error( `Cannot encrypt in ${this.room.roomId} for unsupported algorithm '${this.encryptionSettings.algorithm}'`, @@ -251,12 +271,22 @@ export class RoomEncryptor { rustEncryptionSettings.rotationPeriodMessages = BigInt(this.encryptionSettings.rotation_period_msgs); } - // When this.room.getBlacklistUnverifiedDevices() === null, the global settings should be used - // See Room#getBlacklistUnverifiedDevices - if (this.room.getBlacklistUnverifiedDevices() ?? globalBlacklistUnverifiedDevices) { - rustEncryptionSettings.sharingStrategy = CollectStrategy.deviceBasedStrategy(true, false); - } else { - rustEncryptionSettings.sharingStrategy = CollectStrategy.deviceBasedStrategy(false, false); + switch (deviceIsolationMode.kind) { + case DeviceIsolationModeKind.AllDevicesIsolationMode: + { + // When this.room.getBlacklistUnverifiedDevices() === null, the global settings should be used + // See Room#getBlacklistUnverifiedDevices + const onlyAllowTrustedDevices = + this.room.getBlacklistUnverifiedDevices() ?? globalBlacklistUnverifiedDevices; + rustEncryptionSettings.sharingStrategy = CollectStrategy.deviceBasedStrategy( + onlyAllowTrustedDevices, + deviceIsolationMode.errorOnVerifiedUserProblems, + ); + } + break; + case DeviceIsolationModeKind.OnlySignedDevicesIsolationMode: + rustEncryptionSettings.sharingStrategy = CollectStrategy.identityBasedStrategy(); + break; } await logDuration(this.prefixedLogger, "shareRoomKey", async () => { diff --git a/src/rust-crypto/rust-crypto.ts b/src/rust-crypto/rust-crypto.ts index fd5883efe..78f921e3b 100644 --- a/src/rust-crypto/rust-crypto.ts +++ b/src/rust-crypto/rust-crypto.ts @@ -248,7 +248,7 @@ export class RustCrypto extends TypedEventEmitter { @@ -400,7 +400,7 @@ export class RustCrypto extends TypedEventEmitter