From b8ca39c4c8de2fb1504629b5ec7ebfa96fc2c0a4 Mon Sep 17 00:00:00 2001 From: Valere Date: Mon, 23 Sep 2024 14:28:00 +0200 Subject: [PATCH 1/6] 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 --- spec/unit/rust-crypto/RoomEncryptor.spec.ts | 101 +++++++++++++++++++- src/rust-crypto/RoomEncryptor.ts | 47 ++++++--- src/rust-crypto/rust-crypto.ts | 4 +- 3 files changed, 138 insertions(+), 14 deletions(-) diff --git a/spec/unit/rust-crypto/RoomEncryptor.spec.ts b/spec/unit/rust-crypto/RoomEncryptor.spec.ts index 608d9643e05..e7af29835a7 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(undefined), } as unknown as Mocked; roomEncryptor = new RoomEncryptor( @@ -181,5 +183,102 @@ 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("should use Legacy as default mode", async () => { + await roomEncryptor.prepareForEncryption(false); + expect(mockOlmMachine.shareRoomKey).toHaveBeenCalled(); + expect(capturedSettings?.eq(CollectStrategy.deviceBasedStrategy(false, false))).toBeTruthy(); + }); + + 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/rust-crypto/RoomEncryptor.ts b/src/rust-crypto/RoomEncryptor.ts index fcda295eb89..b2e762c4a10 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 { AllDevicesIsolationMode, DeviceIsolationMode, DeviceIsolationModeKind } from "../crypto-api/index.ts"; /** * RoomEncryptor: responsible for encrypting messages to a given room @@ -121,8 +122,12 @@ export class RoomEncryptor { * in the room. * * @param globalBlacklistUnverifiedDevices - When `true`, it will not send encrypted messages to unverified devices + * @param deviceIsolationMode - The device isolation mode see {@link DeviceIsolationMode} */ - public async prepareForEncryption(globalBlacklistUnverifiedDevices: boolean): Promise { + public async prepareForEncryption( + globalBlacklistUnverifiedDevices: boolean, + deviceIsolationMode: DeviceIsolationMode = new AllDevicesIsolationMode(false), + ): 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 +135,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); } /** @@ -141,8 +146,13 @@ export class RoomEncryptor { * * @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 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 = new AllDevicesIsolationMode(false), + ): 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 +163,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 () => { @@ -174,8 +184,13 @@ export class RoomEncryptor { * * @param logger - a place to write diagnostics to * @param globalBlacklistUnverifiedDevices - When `true`, it will not send encrypted messages to unverified devices + * @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 +266,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 fd5883efe1f..78f921e3b55 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 Date: Wed, 25 Sep 2024 16:04:16 +0200 Subject: [PATCH 2/6] Fix outdated docs referring to old cryptomode --- src/crypto-api/index.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/crypto-api/index.ts b/src/crypto-api/index.ts index 4503a72cef6..0ac684e8c63 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", From 4f4928b7ca8407e4ad91db3f3c625e9a2a0c3362 Mon Sep 17 00:00:00 2001 From: Valere Date: Mon, 30 Sep 2024 15:36:14 +0200 Subject: [PATCH 3/6] code review: better comment for globalBlacklistUnverifiedDevices option --- src/rust-crypto/RoomEncryptor.ts | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/src/rust-crypto/RoomEncryptor.ts b/src/rust-crypto/RoomEncryptor.ts index b2e762c4a10..2326c9760f9 100644 --- a/src/rust-crypto/RoomEncryptor.ts +++ b/src/rust-crypto/RoomEncryptor.ts @@ -120,9 +120,10 @@ 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 deviceIsolationMode - The device isolation mode see {@link DeviceIsolationMode} + * @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, @@ -145,8 +146,10 @@ 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 deviceIsolationMode - The device isolation mode see {@link DeviceIsolationMode}. + * @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, @@ -183,8 +186,10 @@ 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 deviceIsolationMode - The device isolation mode see {@link DeviceIsolationMode}. + * @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, From a41b1f013bdf4e87d627000a2f298efb7c4bc6d5 Mon Sep 17 00:00:00 2001 From: Valere Date: Mon, 30 Sep 2024 15:36:45 +0200 Subject: [PATCH 4/6] RoomEncryptor: Use appropriate default for getBlacklistUnverifiedDevices --- spec/unit/rust-crypto/RoomEncryptor.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/unit/rust-crypto/RoomEncryptor.spec.ts b/spec/unit/rust-crypto/RoomEncryptor.spec.ts index e7af29835a7..122c6afed4a 100644 --- a/spec/unit/rust-crypto/RoomEncryptor.spec.ts +++ b/spec/unit/rust-crypto/RoomEncryptor.spec.ts @@ -101,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(undefined), + getBlacklistUnverifiedDevices: jest.fn().mockReturnValue(null), } as unknown as Mocked; roomEncryptor = new RoomEncryptor( From 670fe2e5d84dcc120cc280ee98b1cc0ae024592d Mon Sep 17 00:00:00 2001 From: Valere Date: Mon, 30 Sep 2024 15:44:13 +0200 Subject: [PATCH 5/6] do not provide a default value for DeviceIsolationMode for encryption --- spec/unit/rust-crypto/RoomEncryptor.spec.ts | 32 ++++++++++++--------- src/rust-crypto/RoomEncryptor.ts | 6 ++-- 2 files changed, 21 insertions(+), 17 deletions(-) diff --git a/spec/unit/rust-crypto/RoomEncryptor.spec.ts b/spec/unit/rust-crypto/RoomEncryptor.spec.ts index 122c6afed4a..7e20a9504c6 100644 --- a/spec/unit/rust-crypto/RoomEncryptor.spec.ts +++ b/spec/unit/rust-crypto/RoomEncryptor.spec.ts @@ -113,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(); @@ -122,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); @@ -160,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) { @@ -254,12 +264,6 @@ describe("RoomEncryptor", () => { }); }); - it("should use Legacy as default mode", async () => { - await roomEncryptor.prepareForEncryption(false); - expect(mockOlmMachine.shareRoomKey).toHaveBeenCalled(); - expect(capturedSettings?.eq(CollectStrategy.deviceBasedStrategy(false, false))).toBeTruthy(); - }); - it.each(testCases)( "prepareForEncryption should properly set sharing strategy based on crypto mode: %s", async (_, { mode, expectedStrategy, globalBlacklistUnverifiedDevices }) => { diff --git a/src/rust-crypto/RoomEncryptor.ts b/src/rust-crypto/RoomEncryptor.ts index 2326c9760f9..2c716abe8cd 100644 --- a/src/rust-crypto/RoomEncryptor.ts +++ b/src/rust-crypto/RoomEncryptor.ts @@ -36,7 +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 { AllDevicesIsolationMode, DeviceIsolationMode, DeviceIsolationModeKind } from "../crypto-api/index.ts"; +import { DeviceIsolationMode, DeviceIsolationModeKind } from "../crypto-api/index.ts"; /** * RoomEncryptor: responsible for encrypting messages to a given room @@ -127,7 +127,7 @@ export class RoomEncryptor { */ public async prepareForEncryption( globalBlacklistUnverifiedDevices: boolean, - deviceIsolationMode: DeviceIsolationMode = new AllDevicesIsolationMode(false), + 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. @@ -154,7 +154,7 @@ export class RoomEncryptor { public encryptEvent( event: MatrixEvent | null, globalBlacklistUnverifiedDevices: boolean, - deviceIsolationMode: DeviceIsolationMode = new AllDevicesIsolationMode(false), + 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 From 7b3cf87ca844282278939035d9613e1c384433a6 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Mon, 30 Sep 2024 15:21:44 +0100 Subject: [PATCH 6/6] Update src/rust-crypto/RoomEncryptor.ts --- src/rust-crypto/RoomEncryptor.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rust-crypto/RoomEncryptor.ts b/src/rust-crypto/RoomEncryptor.ts index 2c716abe8cd..e87b8c90139 100644 --- a/src/rust-crypto/RoomEncryptor.ts +++ b/src/rust-crypto/RoomEncryptor.ts @@ -123,7 +123,7 @@ export class RoomEncryptor { * @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} + * @param deviceIsolationMode - The device isolation mode. See {@link DeviceIsolationMode}. */ public async prepareForEncryption( globalBlacklistUnverifiedDevices: boolean,