Skip to content

RTCEncryptionManager: Joiner key rotation grace period #4911

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
187 changes: 184 additions & 3 deletions spec/unit/matrixrtc/RTCEncrytionManager.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import { membershipTemplate, mockCallMembership } from "./mocks.ts";
import { decodeBase64, TypedEventEmitter } from "../../../src";
import { RoomAndToDeviceTransport } from "../../../src/matrixrtc/RoomAndToDeviceKeyTransport.ts";
import { type RoomKeyTransport } from "../../../src/matrixrtc/RoomKeyTransport.ts";
import type { Logger } from "../../../src/logger.ts";
import { logger, type Logger } from "../../../src/logger.ts";

describe("RTCEncryptionManager", () => {
// The manager being tested
Expand Down Expand Up @@ -61,6 +61,7 @@ describe("RTCEncryptionManager", () => {
mockTransport,
statistics,
onEncryptionKeysChanged,
logger,
);
});

Expand Down Expand Up @@ -88,6 +89,7 @@ describe("RTCEncryptionManager", () => {
});

it("Should distribute keys to members on join", async () => {
jest.useFakeTimers();
const members = [
aCallMembership("@bob:example.org", "BOBDEVICE"),
aCallMembership("@bob:example.org", "BOBDEVICE2"),
Expand Down Expand Up @@ -165,7 +167,7 @@ describe("RTCEncryptionManager", () => {
);
});

it("Should not rotate key when a user join", async () => {
it("Should not rotate key when a user join within the rotation grace period", async () => {
jest.useFakeTimers();

const members = [
Expand All @@ -174,8 +176,9 @@ describe("RTCEncryptionManager", () => {
];
getMembershipMock.mockReturnValue(members);

const gracePeriod = 15_000; // 15 seconds
// initial rollout
encryptionManager.join(undefined);
encryptionManager.join({ keyRotationGracePeriodMs: gracePeriod });
encryptionManager.onMembershipsUpdate([]);
await jest.runOnlyPendingTimersAsync();

Expand All @@ -196,6 +199,8 @@ describe("RTCEncryptionManager", () => {
];
getMembershipMock.mockReturnValue(updatedMembers);

await jest.advanceTimersByTimeAsync(8_000);

encryptionManager.onMembershipsUpdate(updatedMembers);

await jest.runOnlyPendingTimersAsync();
Expand All @@ -214,6 +219,157 @@ describe("RTCEncryptionManager", () => {
expect(statistics.counters.roomEventEncryptionKeysSent).toBe(2);
});

it("test grace period lower than delay period", async () => {
jest.useFakeTimers();

const members = [
aCallMembership("@bob:example.org", "BOBDEVICE"),
aCallMembership("@bob:example.org", "BOBDEVICE2"),
];
getMembershipMock.mockReturnValue(members);

const gracePeriod = 3_000; // 15 seconds
const useKeyDelay = 5_000; // 5 seconds
// initial rollout
encryptionManager.join({
useKeyDelay,
keyRotationGracePeriodMs: gracePeriod,
});
encryptionManager.onMembershipsUpdate([]);
await jest.runOnlyPendingTimersAsync();

onEncryptionKeysChanged.mockClear();
mockTransport.sendKey.mockClear();

// The existing members have been talking for 5mn
await jest.advanceTimersByTimeAsync(5 * 60 * 1000);

// A new member joins, that should trigger a key rotation.
members.push(aCallMembership("@carl:example.org", "CARLDEVICE"));
encryptionManager.onMembershipsUpdate([]);
await jest.runOnlyPendingTimersAsync();

// A new member joins, within the grace period, but under the delay period
members.push(aCallMembership("@david:example.org", "DAVDEVICE"));
await jest.advanceTimersByTimeAsync(3_000);
encryptionManager.onMembershipsUpdate([]);
await jest.runOnlyPendingTimersAsync();

// Wait pass the delay period
await jest.advanceTimersByTimeAsync(5_000);

// Even though the new member joined within the grace period, the key should be rotated because the delay period has passed
// and made the key too old to be reshared.

// CARLDEVICE should have received a key with index 1 and another one with index 2
expectKeyAtIndexToHaveBeenSentTo(mockTransport, 1, "@carl:example.org", "CARLDEVICE");
expectKeyAtIndexToHaveBeenSentTo(mockTransport, 2, "@carl:example.org", "CARLDEVICE");
// Of course, should not have received the first key
expectKeyAtIndexNotToHaveBeenSentTo(mockTransport, 0, "@carl:example.org", "CARLDEVICE");

// DAVDEVICE should only have received a key with index 2
expectKeyAtIndexToHaveBeenSentTo(mockTransport, 2, "@david:example.org", "DAVDEVICE");
expectKeyAtIndexNotToHaveBeenSentTo(mockTransport, 1, "@david:example.org", "DAVDEVICE");
});

it("Should rotate key when a user join past the rotation grace period", async () => {
jest.useFakeTimers();

const members = [
aCallMembership("@bob:example.org", "BOBDEVICE"),
aCallMembership("@bob:example.org", "BOBDEVICE2"),
];
getMembershipMock.mockReturnValue(members);

const gracePeriod = 15_000; // 15 seconds
// initial rollout
encryptionManager.join({ keyRotationGracePeriodMs: gracePeriod });
encryptionManager.onMembershipsUpdate([]);
await jest.runOnlyPendingTimersAsync();

onEncryptionKeysChanged.mockClear();
mockTransport.sendKey.mockClear();

const updatedMembers = [
aCallMembership("@bob:example.org", "BOBDEVICE"),
aCallMembership("@bob:example.org", "BOBDEVICE2"),
aCallMembership("@carl:example.org", "CARLDEVICE"),
];
getMembershipMock.mockReturnValue(updatedMembers);

await jest.advanceTimersByTimeAsync(gracePeriod + 1000);

encryptionManager.onMembershipsUpdate(updatedMembers);

await jest.runOnlyPendingTimersAsync();

expect(mockTransport.sendKey).toHaveBeenCalledWith(
expect.any(String),
// It should have incremented the key index
1,
// And send it to everyone
[
expect.objectContaining({ userId: "@bob:example.org", deviceId: "BOBDEVICE" }),
expect.objectContaining({ userId: "@bob:example.org", deviceId: "BOBDEVICE2" }),
expect.objectContaining({ userId: "@carl:example.org", deviceId: "CARLDEVICE" }),
],
);

expect(onEncryptionKeysChanged).toHaveBeenCalled();
await jest.advanceTimersByTimeAsync(1000);
expect(statistics.counters.roomEventEncryptionKeysSent).toBe(2);
});

it("Should rotate key when several users join within the rotation grace period", async () => {
jest.useFakeTimers();

const members = [
aCallMembership("@bob:example.org", "BOBDEVICE"),
aCallMembership("@bob:example.org", "BOBDEVICE2"),
];
getMembershipMock.mockReturnValue(members);

// initial rollout
encryptionManager.join(undefined);
encryptionManager.onMembershipsUpdate([]);
await jest.runOnlyPendingTimersAsync();

onEncryptionKeysChanged.mockClear();
mockTransport.sendKey.mockClear();

const newJoiners = [
aCallMembership("@carl:example.org", "CARLDEVICE"),
aCallMembership("@dave:example.org", "DAVEDEVICE"),
aCallMembership("@eve:example.org", "EVEDEVICE"),
aCallMembership("@frank:example.org", "FRANKDEVICE"),
aCallMembership("@george:example.org", "GEORGEDEVICE"),
];

for (const newJoiner of newJoiners) {
members.push(newJoiner);
getMembershipMock.mockReturnValue(members);
await jest.advanceTimersByTimeAsync(1_000);
encryptionManager.onMembershipsUpdate(members);
await jest.runOnlyPendingTimersAsync();
}

expect(mockTransport.sendKey).toHaveBeenCalledTimes(newJoiners.length);

for (const newJoiner of newJoiners) {
expect(mockTransport.sendKey).toHaveBeenCalledWith(
expect.any(String),
// It should not have incremented the key index
0,
// And send it to the new joiners only
expect.arrayContaining([
expect.objectContaining({ userId: newJoiner.sender, deviceId: newJoiner.deviceId }),
]),
);
}

expect(onEncryptionKeysChanged).not.toHaveBeenCalled();
});

it("Should not resend keys when no changes", async () => {
jest.useFakeTimers();

Expand Down Expand Up @@ -596,3 +752,28 @@ describe("RTCEncryptionManager", () => {
);
}
});

function expectKeyAtIndexToHaveBeenSentTo(
mockTransport: Mocked<ToDeviceKeyTransport>,
index: number,
userId: string,
deviceId: string,
) {
expect(mockTransport.sendKey).toHaveBeenCalledWith(
expect.any(String),
index,
expect.arrayContaining([expect.objectContaining({ userId, deviceId })]),
);
}
function expectKeyAtIndexNotToHaveBeenSentTo(
mockTransport: Mocked<ToDeviceKeyTransport>,
index: number,
userId: string,
deviceId: string,
) {
expect(mockTransport.sendKey).not.toHaveBeenCalledWith(
expect.any(String),
index,
expect.arrayContaining([expect.objectContaining({ userId, deviceId })]),
);
}
21 changes: 17 additions & 4 deletions src/matrixrtc/MatrixRTCSession.ts
Original file line number Diff line number Diff line change
Expand Up @@ -153,18 +153,31 @@ export interface EncryptionConfig {
/**
* The minimum time (in milliseconds) between each attempt to send encryption key(s).
* e.g. if this is set to 1000, then we will send at most one key event every second.
* @deprecated - Not used by the new encryption manager.
*/
updateEncryptionKeyThrottle?: number;

/**
* Sometimes it is necessary to rotate the encryption key after a membership update.
* For performance reasons we might not want to rotate the key immediately but allow future memberships to use the same key.
* If 5 people join in a row in less than 5 seconds, we don't want to rotate the key for each of them.
* If 5 people leave in a row in less than 5 seconds, we don't want to rotate the key for each of them.
*/
keyRotationGracePeriodMs?: number;

/**
* The delay (in milliseconds) after a member leaves before we create and publish a new key, because people
* tend to leave calls at the same time.
* @deprecated - Not used by the new encryption manager.
*/
makeKeyDelay?: number;
/**
* The delay (in milliseconds) between creating and sending a new key and starting to encrypt with it. This
* gives other a chance to receive the new key to minimise the chance they don't get media they can't decrypt.
* The total time between a member leaving and the call switching to new keys is therefore:
* makeKeyDelay + useKeyDelay
* The delay (in milliseconds) between sending a new key and starting to encrypt with it. This
* gives others a chance to receive the new key to minimize the chance they get media they can't decrypt.
*
* The higher this value is, the better it is for existing members as they will have a smoother experience.
* But it impacts new joiners: They will always have to wait `useKeyDelay` before being able to decrypt the media
* (as it will be encrypted with the new key after the delay only), even if the key is already arrived before the delay.
*/
useKeyDelay?: number;
}
Expand Down
63 changes: 46 additions & 17 deletions src/matrixrtc/RTCEncryptionManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,24 @@ export class RTCEncryptionManager implements IEncryptionManager {
* Ensures that there is only one distribute operation at a time for that call.
*/
private currentKeyDistributionPromise: Promise<void> | null = null;

/**
* The time to wait before using the outbound session after it has been distributed.
* This is to ensure that the key is delivered to all participants before it is used.
* When creating the first key, this is set to 0, so that the key can be used immediately.
*/
private delayRolloutTimeMillis = 1000;
private delayRolloutTimeMillis = 5000;

/**
* We want to avoid rolling out a new outbound key when the previous one was created less than `skipRotationGracePeriod` milliseconds ago.
* This is to avoid expensive key rotations when users quickly join the call in a row.
*
* This must be higher than `delayRolloutTimeMillis` to have an effect.
* If it is lower, the current key will always be older than the grace period.
* @private
*/
private skipRotationGracePeriod = 10_000;

/**
* If a new key distribution is being requested while one is going on, we will set this flag to true.
* This will ensure that a new round is started after the current one.
Expand Down Expand Up @@ -102,6 +114,7 @@ export class RTCEncryptionManager implements IEncryptionManager {
public join(joinConfig: EncryptionConfig | undefined): void {
this.logger?.info(`Joining room`);
this.delayRolloutTimeMillis = joinConfig?.useKeyDelay ?? 1000;
this.skipRotationGracePeriod = joinConfig?.keyRotationGracePeriodMs ?? 10_000;
this.transport.on(KeyTransportEvents.ReceivedKeys, this.onNewKeyReceived);
// Deprecate RoomKeyTransport: this can get removed.
if (this.transport instanceof RoomAndToDeviceTransport) {
Expand Down Expand Up @@ -264,26 +277,28 @@ export class RTCEncryptionManager implements IEncryptionManager {
let hasKeyChanged = false;
if (anyLeft.length > 0) {
// We need to rotate the key
const newOutboundKey: OutboundEncryptionSession = {
key: this.generateRandomKey(),
creationTS: Date.now(),
sharedWith: [],
keyId: this.nextKeyIndex(),
};
const newOutboundKey = this.createNewOutboundSession();
hasKeyChanged = true;

this.logger?.info(`creating new outbound key index:${newOutboundKey.keyId}`);
// Set this new key as the current one
this.outboundSession = newOutboundKey;

// Send
toDistributeTo = toShareWith;
outboundKey = newOutboundKey;
} else if (anyJoined.length > 0) {
// keep the same key
// XXX In the future we want to distribute a ratcheted key not the current one
toDistributeTo = anyJoined;
outboundKey = this.outboundSession!;
const now = Date.now();
const keyAge = now - this.outboundSession!.creationTS;
// If the current key is recently created (less than `skipRotationGracePeriod`), we can keep it and just distribute it to the new joiners.
if (keyAge < this.skipRotationGracePeriod) {
// keep the same key
// XXX In the future we want to distribute a ratcheted key, not the current one
this.logger?.debug(`New joiners detected, but the key is recent enough (age:${keyAge}), keeping it`);
toDistributeTo = anyJoined;
outboundKey = this.outboundSession!;
} else {
// We need to rotate the key
this.logger?.debug(`New joiners detected, rotating the key`);
const newOutboundKey = this.createNewOutboundSession();
hasKeyChanged = true;
toDistributeTo = toShareWith;
outboundKey = newOutboundKey;
}
} else {
// no changes
return;
Expand Down Expand Up @@ -314,6 +329,20 @@ export class RTCEncryptionManager implements IEncryptionManager {
}
}

private createNewOutboundSession(): OutboundEncryptionSession {
const newOutboundKey: OutboundEncryptionSession = {
key: this.generateRandomKey(),
creationTS: Date.now(),
sharedWith: [],
keyId: this.nextKeyIndex(),
};

this.logger?.info(`creating new outbound key index:${newOutboundKey.keyId}`);
// Set this new key as the current one
this.outboundSession = newOutboundKey;
return newOutboundKey;
}

private nextKeyIndex(): number {
if (this.outboundSession) {
return (this.outboundSession!.keyId + 1) % 256;
Expand Down