Skip to content

Commit

Permalink
crypto: configure key sharing strategy based on DeviceIsolationMode (#…
Browse files Browse the repository at this point in the history 
…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 <[email protected]>
  • Loading branch information
@BillCarsonFr @richvdh
BillCarsonFr and richvdh authored Sep 30, 2024
1 parent baa6d13 commit 9ecb66e
Show file tree
Hide file tree
Showing 4 changed files with 161 additions and 29 deletions.
121 changes: 112 additions & 9 deletions spec/unit/rust-crypto/RoomEncryptor.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
*/

import {
CollectStrategy,
Curve25519PublicKey,
Ed25519PublicKey,
HistoryVisibility as RustHistoryVisibility,
Expand All @@ -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", () => {
Expand Down Expand Up @@ -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<Room>;

roomEncryptor = new RoomEncryptor(
Expand All @@ -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<void>();
const insideOlmShareRoom = defer<void>();
Expand All @@ -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);
Expand All @@ -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) {
Expand All @@ -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();
},
);
});
});
});
5 changes: 2 additions & 3 deletions src/crypto-api/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",

Expand Down
60 changes: 45 additions & 15 deletions src/rust-crypto/RoomEncryptor.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -119,18 +120,23 @@ 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<void> {
public async prepareForEncryption(
globalBlacklistUnverifiedDevices: boolean,
deviceIsolationMode: DeviceIsolationMode,
): Promise<void> {
// 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
// message is finally sent.
// 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);
}

/**
Expand All @@ -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<void> {
public encryptEvent(
event: MatrixEvent | null,
globalBlacklistUnverifiedDevices: boolean,
deviceIsolationMode: DeviceIsolationMode,
): Promise<void> {
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.
Expand All @@ -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 () => {
Expand All @@ -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<void> {
private async ensureEncryptionSession(
logger: LogSpan,
globalBlacklistUnverifiedDevices: boolean,
deviceIsolationMode: DeviceIsolationMode,
): Promise<void> {
if (this.encryptionSettings.algorithm !== "m.megolm.v1.aes-sha2") {
throw new Error(
`Cannot encrypt in ${this.room.roomId} for unsupported algorithm '${this.encryptionSettings.algorithm}'`,
Expand Down Expand Up @@ -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 () => {
Expand Down
4 changes: 2 additions & 2 deletions src/rust-crypto/rust-crypto.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ export class RustCrypto extends TypedEventEmitter<RustCryptoEvents, RustCryptoEv
throw new Error(`Cannot encrypt event in unconfigured room ${roomId}`);
}

await encryptor.encryptEvent(event, this.globalBlacklistUnverifiedDevices);
await encryptor.encryptEvent(event, this.globalBlacklistUnverifiedDevices, this.deviceIsolationMode);
}

public async decryptEvent(event: MatrixEvent): Promise<IEventDecryptionResult> {
Expand Down Expand Up @@ -400,7 +400,7 @@ export class RustCrypto extends TypedEventEmitter<RustCryptoEvents, RustCryptoEv
const encryptor = this.roomEncryptors[room.roomId];

if (encryptor) {
encryptor.prepareForEncryption(this.globalBlacklistUnverifiedDevices);
encryptor.prepareForEncryption(this.globalBlacklistUnverifiedDevices, this.deviceIsolationMode);
}
}

Expand Down

0 comments on commit 9ecb66e

Please sign in to comment.