1
0
mirror of https://github.com/matrix-org/matrix-js-sdk.git synced 2025-08-07 23:02:56 +03:00

RTCEncryptionManager: Joiner key rotation grace period (#4911)

* RTCEncryptionManager: Joiner key rotation grace period

* Test to clarify useKeyDelay and keyRotationGracePeriodMs interference

* make test more configurable

* rename delayRolloutTimeMillis to useKeyDelay same as config option

* rename skipRotationGracePeriod to keyRotationGracePeriodMs

* clarify that oldMemberships is not used by RTCEncryptionManager

* improve doc

* cleanup test

* more comment in test

* comment additions

* cleanup runOnlyPendingTimers

---------

Co-authored-by: Timo <toger5@hotmail.de>
This commit is contained in:
Valere Fedronic
2025-07-18 17:16:45 +02:00
committed by GitHub
parent aa79236ce2
commit 1fcbc6ebeb
3 changed files with 276 additions and 59 deletions

View File

@@ -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";
import { getParticipantId } from "../../../src/matrixrtc/utils.ts";
describe("RTCEncryptionManager", () => {
@@ -62,6 +62,7 @@ describe("RTCEncryptionManager", () => {
mockTransport,
statistics,
onEncryptionKeysChanged,
logger,
);
});
@@ -83,12 +84,13 @@ describe("RTCEncryptionManager", () => {
encryptionManager.join(undefined);
// After join it is too early, key might be lost as no one is listening yet
expect(onEncryptionKeysChanged).not.toHaveBeenCalled();
encryptionManager.onMembershipsUpdate([]);
encryptionManager.onMembershipsUpdate();
// The key should have been rolled out immediately
expect(onEncryptionKeysChanged).toHaveBeenCalled();
});
it("Should distribute keys to members on join", async () => {
jest.useFakeTimers();
const members = [
aCallMembership("@bob:example.org", "BOBDEVICE"),
aCallMembership("@bob:example.org", "BOBDEVICE2"),
@@ -97,7 +99,7 @@ describe("RTCEncryptionManager", () => {
getMembershipMock.mockReturnValue(members);
encryptionManager.join(undefined);
encryptionManager.onMembershipsUpdate([]);
encryptionManager.onMembershipsUpdate();
expect(mockTransport.sendKey).toHaveBeenCalledTimes(1);
expect(mockTransport.sendKey).toHaveBeenCalledWith(
@@ -121,7 +123,7 @@ describe("RTCEncryptionManager", () => {
getMembershipMock.mockReturnValue(members);
encryptionManager.join(undefined);
encryptionManager.onMembershipsUpdate([]);
encryptionManager.onMembershipsUpdate();
expect(mockTransport.sendKey).toHaveBeenCalledTimes(1);
expect(mockTransport.sendKey).toHaveBeenCalledWith(
@@ -136,7 +138,7 @@ describe("RTCEncryptionManager", () => {
},
],
);
await jest.runOnlyPendingTimersAsync();
await jest.advanceTimersByTimeAsync(1);
// The key should have been rolled out immediately
expect(onEncryptionKeysChanged).toHaveBeenCalled();
@@ -148,7 +150,7 @@ describe("RTCEncryptionManager", () => {
// There are no membership change but the callMembership ts has changed (reset?)
// Resend the key
encryptionManager.onMembershipsUpdate(members);
encryptionManager.onMembershipsUpdate();
await jest.runOnlyPendingTimersAsync();
expect(mockTransport.sendKey).toHaveBeenCalledTimes(1);
@@ -166,7 +168,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 = [
@@ -175,10 +177,11 @@ describe("RTCEncryptionManager", () => {
];
getMembershipMock.mockReturnValue(members);
const gracePeriod = 15_000; // 15 seconds
// initial rollout
encryptionManager.join(undefined);
encryptionManager.onMembershipsUpdate([]);
await jest.runOnlyPendingTimersAsync();
encryptionManager.join({ keyRotationGracePeriodMs: gracePeriod });
encryptionManager.onMembershipsUpdate();
await jest.advanceTimersByTimeAsync(1);
expect(mockTransport.sendKey).toHaveBeenCalledTimes(1);
expect(mockTransport.sendKey).toHaveBeenCalledWith(
@@ -190,14 +193,10 @@ describe("RTCEncryptionManager", () => {
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);
encryptionManager.onMembershipsUpdate(updatedMembers);
// Carl joins, within the grace period
members.push(aCallMembership("@carl:example.org", "CARLDEVICE"));
await jest.advanceTimersByTimeAsync(gracePeriod / 2);
encryptionManager.onMembershipsUpdate();
await jest.runOnlyPendingTimersAsync();
@@ -215,6 +214,154 @@ describe("RTCEncryptionManager", () => {
expect(statistics.counters.roomEventEncryptionKeysSent).toBe(2);
});
// Test an edge case where the use key delay is higher than the grace period.
// This means that no matter what, the key once rolled out will be too old to be re-used for the new member that
// joined within the grace period.
// So we expect another rotation to happen in all cases where a new member joins.
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; // 3 seconds
const useKeyDelay = gracePeriod + 2_000; // 5 seconds
// initial rollout
encryptionManager.join({
useKeyDelay,
keyRotationGracePeriodMs: gracePeriod,
});
encryptionManager.onMembershipsUpdate();
await jest.advanceTimersByTimeAsync(1);
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.advanceTimersByTimeAsync(1);
// A new member joins, within the grace period, but under the delay period
members.push(aCallMembership("@david:example.org", "DAVDEVICE"));
await jest.advanceTimersByTimeAsync((useKeyDelay - gracePeriod) / 2);
encryptionManager.onMembershipsUpdate();
// Wait past the delay period
await jest.advanceTimersByTimeAsync(5_000);
// Even though the new member joined within the grace period, the key should be rotated because once the delay period has passed
// also the grace period is exceeded/the key is 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.advanceTimersByTimeAsync(1);
onEncryptionKeysChanged.mockClear();
mockTransport.sendKey.mockClear();
await jest.advanceTimersByTimeAsync(gracePeriod + 1000);
members.push(aCallMembership("@carl:example.org", "CARLDEVICE"));
encryptionManager.onMembershipsUpdate();
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" }),
],
);
// Wait for useKeyDelay to pass
await jest.advanceTimersByTimeAsync(5000);
expect(onEncryptionKeysChanged).toHaveBeenCalled();
await jest.advanceTimersByTimeAsync(1000);
expect(statistics.counters.roomEventEncryptionKeysSent).toBe(2);
});
it("Should not 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.advanceTimersByTimeAsync(1);
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();
await jest.advanceTimersByTimeAsync(1);
}
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();
@@ -226,20 +373,20 @@ describe("RTCEncryptionManager", () => {
// initial rollout
encryptionManager.join(undefined);
encryptionManager.onMembershipsUpdate([]);
await jest.runOnlyPendingTimersAsync();
encryptionManager.onMembershipsUpdate();
await jest.advanceTimersByTimeAsync(1);
expect(mockTransport.sendKey).toHaveBeenCalledTimes(1);
onEncryptionKeysChanged.mockClear();
mockTransport.sendKey.mockClear();
encryptionManager.onMembershipsUpdate(members);
encryptionManager.onMembershipsUpdate();
await jest.advanceTimersByTimeAsync(200);
encryptionManager.onMembershipsUpdate(members);
encryptionManager.onMembershipsUpdate();
await jest.advanceTimersByTimeAsync(100);
encryptionManager.onMembershipsUpdate(members);
encryptionManager.onMembershipsUpdate();
await jest.advanceTimersByTimeAsync(50);
encryptionManager.onMembershipsUpdate(members);
encryptionManager.onMembershipsUpdate();
await jest.advanceTimersByTimeAsync(100);
expect(mockTransport.sendKey).not.toHaveBeenCalled();
@@ -256,7 +403,7 @@ describe("RTCEncryptionManager", () => {
getMembershipMock.mockReturnValue(members);
encryptionManager.join(undefined);
encryptionManager.onMembershipsUpdate([]);
encryptionManager.onMembershipsUpdate();
await jest.advanceTimersByTimeAsync(10);
expect(mockTransport.sendKey).toHaveBeenCalledTimes(1);
@@ -277,7 +424,7 @@ describe("RTCEncryptionManager", () => {
];
getMembershipMock.mockReturnValue(updatedMembers);
encryptionManager.onMembershipsUpdate(updatedMembers);
encryptionManager.onMembershipsUpdate();
await jest.advanceTimersByTimeAsync(200);
// The is rotated but not rolled out yet to give time for the key to be sent
@@ -335,7 +482,7 @@ describe("RTCEncryptionManager", () => {
getMembershipMock.mockReturnValue(members);
encryptionManager.join(undefined);
encryptionManager.onMembershipsUpdate([]);
encryptionManager.onMembershipsUpdate();
await jest.advanceTimersByTimeAsync(10);
mockTransport.emit(
@@ -442,7 +589,7 @@ describe("RTCEncryptionManager", () => {
// Let's join
encryptionManager.join(undefined);
encryptionManager.onMembershipsUpdate(members);
encryptionManager.onMembershipsUpdate();
await jest.advanceTimersByTimeAsync(10);
@@ -529,7 +676,7 @@ describe("RTCEncryptionManager", () => {
// Let's join
encryptionManager.join(undefined);
encryptionManager.onMembershipsUpdate([]);
encryptionManager.onMembershipsUpdate();
await jest.advanceTimersByTimeAsync(10);
// The initial rollout
@@ -545,7 +692,7 @@ describe("RTCEncryptionManager", () => {
getMembershipMock.mockReturnValue(members);
// This should start a new key rollout
encryptionManager.onMembershipsUpdate(members);
encryptionManager.onMembershipsUpdate();
await jest.advanceTimersByTimeAsync(10);
// Now simulate a new leaver
@@ -553,14 +700,14 @@ describe("RTCEncryptionManager", () => {
getMembershipMock.mockReturnValue(members);
// The key `1` rollout is in progress
encryptionManager.onMembershipsUpdate(members);
encryptionManager.onMembershipsUpdate();
await jest.advanceTimersByTimeAsync(10);
// And another one ( plus a joiner)
const lastMembership = [aCallMembership("@bob:example.org", "BOBDEVICE3")];
getMembershipMock.mockReturnValue(lastMembership);
// The key `1` rollout is still in progress
encryptionManager.onMembershipsUpdate(lastMembership);
encryptionManager.onMembershipsUpdate();
await jest.advanceTimersByTimeAsync(10);
// Let all rollouts finish
@@ -645,7 +792,7 @@ describe("RTCEncryptionManager", () => {
// Let's join
encryptionManager.join(undefined);
encryptionManager.onMembershipsUpdate([]);
encryptionManager.onMembershipsUpdate();
await jest.advanceTimersByTimeAsync(10);
// Should have sent the key to the toDevice transport
@@ -682,3 +829,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 })]),
);
}

View File

@@ -162,18 +162,34 @@ 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.
* So we do share the key which was already used live for <5s to new joiners.
* This does result in a potential leak up to the configured time of call media.
* This has to be considered when choosing a value for this property.
*/
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 has already arrived before the delay.
*/
useKeyDelay?: number;
}

View File

@@ -60,12 +60,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.
* When creating the first key, this is set to 0 so that the key can be used immediately.
*/
private delayRolloutTimeMillis = 1000;
private useKeyDelay = 5000;
/**
* We want to avoid rolling out a new outbound key when the previous one was created less than `keyRotationGracePeriodMs` milliseconds ago.
* This is to avoid expensive key rotations when users quickly join the call in a row.
*
* This must be higher than `useKeyDelay` to have an effect.
* If it is lower, the current key will always be older than the grace period.
* @private
*/
private keyRotationGracePeriodMs = 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.
@@ -115,7 +127,8 @@ export class RTCEncryptionManager implements IEncryptionManager {
public join(joinConfig: EncryptionConfig | undefined): void {
this.logger?.info(`Joining room`);
this.delayRolloutTimeMillis = joinConfig?.useKeyDelay ?? 1000;
this.useKeyDelay = joinConfig?.useKeyDelay ?? 1000;
this.keyRotationGracePeriodMs = joinConfig?.keyRotationGracePeriodMs ?? 10_000;
this.transport.on(KeyTransportEvents.ReceivedKeys, this.onNewKeyReceived);
// Deprecate RoomKeyTransport: this can get removed.
if (this.transport instanceof RoomAndToDeviceTransport) {
@@ -211,9 +224,9 @@ export class RTCEncryptionManager implements IEncryptionManager {
/**
* Called when the membership of the call changes.
* This encryption manager is very basic, it will rotate the key everytime this is called.
* @param oldMemberships
* @param oldMemberships - This parameter is not used here, but it is kept for compatibility with the interface.
*/
public onMembershipsUpdate(oldMemberships: CallMembership[]): void {
public onMembershipsUpdate(oldMemberships: CallMembership[] = []): void {
this.logger?.trace(`onMembershipsUpdate`);
// Ensure the key is distributed. This will be no-op if the key is already being distributed to everyone.
@@ -280,26 +293,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 `keyRotationGracePeriodMs`), we can keep it and just distribute it to the new joiners.
if (keyAge < this.keyRotationGracePeriodMs) {
// 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;
@@ -317,7 +332,7 @@ export class RTCEncryptionManager implements IEncryptionManager {
// Delay a bit before using this key
// It is recommended not to start using a key immediately but instead wait for a short time to make sure it is delivered.
this.logger?.trace(`Delay Rollout for key:${outboundKey.keyId}...`);
await sleep(this.delayRolloutTimeMillis);
await sleep(this.useKeyDelay);
this.logger?.trace(`...Delayed rollout of index:${outboundKey.keyId} `);
this.addKeyToParticipant(
outboundKey.key,
@@ -330,6 +345,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;