From 1d798cbbb5bbc8aaf0c6e6e473bca347b64464e4 Mon Sep 17 00:00:00 2001 From: Valere Date: Fri, 4 Jul 2025 18:12:32 +0200 Subject: [PATCH 01/11] RTCEncryptionManager: Joiner key rotation grace period --- .../matrixrtc/RTCEncrytionManager.spec.ts | 106 +++++++++++++++++- src/matrixrtc/MatrixRTCSession.ts | 21 +++- src/matrixrtc/RTCEncryptionManager.ts | 60 +++++++--- 3 files changed, 164 insertions(+), 23 deletions(-) diff --git a/spec/unit/matrixrtc/RTCEncrytionManager.spec.ts b/spec/unit/matrixrtc/RTCEncrytionManager.spec.ts index 730db4838b..f225ebf5d7 100644 --- a/spec/unit/matrixrtc/RTCEncrytionManager.spec.ts +++ b/spec/unit/matrixrtc/RTCEncrytionManager.spec.ts @@ -88,6 +88,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"), @@ -165,7 +166,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 = [ @@ -174,8 +175,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(); @@ -196,6 +198,8 @@ describe("RTCEncryptionManager", () => { ]; getMembershipMock.mockReturnValue(updatedMembers); + await jest.advanceTimersByTimeAsync(8_000); + encryptionManager.onMembershipsUpdate(updatedMembers); await jest.runOnlyPendingTimersAsync(); @@ -214,6 +218,104 @@ describe("RTCEncryptionManager", () => { expect(statistics.counters.roomEventEncryptionKeysSent).toBe(2); }); + 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(); diff --git a/src/matrixrtc/MatrixRTCSession.ts b/src/matrixrtc/MatrixRTCSession.ts index f04e4f94f9..2638d4a541 100644 --- a/src/matrixrtc/MatrixRTCSession.ts +++ b/src/matrixrtc/MatrixRTCSession.ts @@ -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; } diff --git a/src/matrixrtc/RTCEncryptionManager.ts b/src/matrixrtc/RTCEncryptionManager.ts index 6246d19403..c42b8b098c 100644 --- a/src/matrixrtc/RTCEncryptionManager.ts +++ b/src/matrixrtc/RTCEncryptionManager.ts @@ -53,12 +53,21 @@ export class RTCEncryptionManager implements IEncryptionManager { * Ensures that there is only one distribute operation at a time for that call. */ private currentKeyDistributionPromise: Promise | 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. + * @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. @@ -102,6 +111,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) { @@ -264,26 +274,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; @@ -314,6 +326,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; From 0bda8e02577e184958c4e42a4db4df8132d56052 Mon Sep 17 00:00:00 2001 From: Valere Date: Thu, 10 Jul 2025 16:16:10 +0200 Subject: [PATCH 02/11] Test to clarify useKeyDelay and keyRotationGracePeriodMs interference --- .../matrixrtc/RTCEncrytionManager.spec.ts | 81 ++++++++++++++++++- src/matrixrtc/RTCEncryptionManager.ts | 3 + 2 files changed, 83 insertions(+), 1 deletion(-) diff --git a/spec/unit/matrixrtc/RTCEncrytionManager.spec.ts b/spec/unit/matrixrtc/RTCEncrytionManager.spec.ts index f225ebf5d7..4ead2a1017 100644 --- a/spec/unit/matrixrtc/RTCEncrytionManager.spec.ts +++ b/spec/unit/matrixrtc/RTCEncrytionManager.spec.ts @@ -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 @@ -61,6 +61,7 @@ describe("RTCEncryptionManager", () => { mockTransport, statistics, onEncryptionKeysChanged, + logger, ); }); @@ -218,6 +219,59 @@ 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(); @@ -698,3 +752,28 @@ describe("RTCEncryptionManager", () => { ); } }); + +function expectKeyAtIndexToHaveBeenSentTo( + mockTransport: Mocked, + index: number, + userId: string, + deviceId: string, +) { + expect(mockTransport.sendKey).toHaveBeenCalledWith( + expect.any(String), + index, + expect.arrayContaining([expect.objectContaining({ userId, deviceId })]), + ); +} +function expectKeyAtIndexNotToHaveBeenSentTo( + mockTransport: Mocked, + index: number, + userId: string, + deviceId: string, +) { + expect(mockTransport.sendKey).not.toHaveBeenCalledWith( + expect.any(String), + index, + expect.arrayContaining([expect.objectContaining({ userId, deviceId })]), + ); +} diff --git a/src/matrixrtc/RTCEncryptionManager.ts b/src/matrixrtc/RTCEncryptionManager.ts index c42b8b098c..a782e03537 100644 --- a/src/matrixrtc/RTCEncryptionManager.ts +++ b/src/matrixrtc/RTCEncryptionManager.ts @@ -64,6 +64,9 @@ export class RTCEncryptionManager implements IEncryptionManager { /** * 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; From 966d9b9d5eac4fa9fc0a4e5b696faf6c471c0344 Mon Sep 17 00:00:00 2001 From: Valere Date: Thu, 17 Jul 2025 15:48:06 +0200 Subject: [PATCH 03/11] make test more configurable --- spec/unit/matrixrtc/RTCEncryptionManager.spec.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/unit/matrixrtc/RTCEncryptionManager.spec.ts b/spec/unit/matrixrtc/RTCEncryptionManager.spec.ts index ce67dcfcbc..a11b614bfe 100644 --- a/spec/unit/matrixrtc/RTCEncryptionManager.spec.ts +++ b/spec/unit/matrixrtc/RTCEncryptionManager.spec.ts @@ -200,7 +200,7 @@ describe("RTCEncryptionManager", () => { ]; getMembershipMock.mockReturnValue(updatedMembers); - await jest.advanceTimersByTimeAsync(8_000); + await jest.advanceTimersByTimeAsync(gracePeriod / 2); encryptionManager.onMembershipsUpdate(updatedMembers); @@ -229,8 +229,8 @@ describe("RTCEncryptionManager", () => { ]; getMembershipMock.mockReturnValue(members); - const gracePeriod = 3_000; // 15 seconds - const useKeyDelay = 5_000; // 5 seconds + const gracePeriod = 3_000; // 3 seconds + const useKeyDelay = gracePeriod + 2_000; // 5 seconds // initial rollout encryptionManager.join({ useKeyDelay, @@ -252,7 +252,7 @@ describe("RTCEncryptionManager", () => { // 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); + await jest.advanceTimersByTimeAsync((useKeyDelay - gracePeriod) / 2); encryptionManager.onMembershipsUpdate([]); await jest.runOnlyPendingTimersAsync(); From 65e50032bd402a3816689353648b8c4982a38ff7 Mon Sep 17 00:00:00 2001 From: Valere Date: Thu, 17 Jul 2025 15:57:14 +0200 Subject: [PATCH 04/11] rename delayRolloutTimeMillis to useKeyDelay same as config option --- src/matrixrtc/RTCEncryptionManager.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/matrixrtc/RTCEncryptionManager.ts b/src/matrixrtc/RTCEncryptionManager.ts index 7a29ffeaed..aa9ad0618b 100644 --- a/src/matrixrtc/RTCEncryptionManager.ts +++ b/src/matrixrtc/RTCEncryptionManager.ts @@ -64,15 +64,15 @@ export class RTCEncryptionManager implements IEncryptionManager { /** * 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 = 5000; + private useKeyDelay = 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. + * 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 */ @@ -127,7 +127,7 @@ 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.skipRotationGracePeriod = joinConfig?.keyRotationGracePeriodMs ?? 10_000; this.transport.on(KeyTransportEvents.ReceivedKeys, this.onNewKeyReceived); // Deprecate RoomKeyTransport: this can get removed. @@ -332,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, From efd6f0ba22d6bcf720ad85ecc3e0162cf6179ef8 Mon Sep 17 00:00:00 2001 From: Valere Date: Thu, 17 Jul 2025 15:57:56 +0200 Subject: [PATCH 05/11] rename skipRotationGracePeriod to keyRotationGracePeriodMs --- src/matrixrtc/RTCEncryptionManager.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/matrixrtc/RTCEncryptionManager.ts b/src/matrixrtc/RTCEncryptionManager.ts index aa9ad0618b..8918abef2f 100644 --- a/src/matrixrtc/RTCEncryptionManager.ts +++ b/src/matrixrtc/RTCEncryptionManager.ts @@ -69,14 +69,14 @@ export class RTCEncryptionManager implements IEncryptionManager { private useKeyDelay = 5000; /** - * We want to avoid rolling out a new outbound key when the previous one was created less than `skipRotationGracePeriod` milliseconds ago. + * 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 skipRotationGracePeriod = 10_000; + private keyRotationGracePeriodMs = 10_000; /** * If a new key distribution is being requested while one is going on, we will set this flag to true. @@ -128,7 +128,7 @@ export class RTCEncryptionManager implements IEncryptionManager { public join(joinConfig: EncryptionConfig | undefined): void { this.logger?.info(`Joining room`); this.useKeyDelay = joinConfig?.useKeyDelay ?? 1000; - this.skipRotationGracePeriod = joinConfig?.keyRotationGracePeriodMs ?? 10_000; + this.keyRotationGracePeriodMs = joinConfig?.keyRotationGracePeriodMs ?? 10_000; this.transport.on(KeyTransportEvents.ReceivedKeys, this.onNewKeyReceived); // Deprecate RoomKeyTransport: this can get removed. if (this.transport instanceof RoomAndToDeviceTransport) { @@ -300,8 +300,8 @@ export class RTCEncryptionManager implements IEncryptionManager { } else if (anyJoined.length > 0) { 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) { + // 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`); From 6d06e2456822067083c6fff100a2f55210b939bd Mon Sep 17 00:00:00 2001 From: Valere Date: Fri, 18 Jul 2025 09:41:53 +0200 Subject: [PATCH 06/11] clarify that oldMemberships is not used by RTCEncryptionManager --- .../matrixrtc/RTCEncryptionManager.spec.ts | 54 +++++++++---------- src/matrixrtc/RTCEncryptionManager.ts | 4 +- 2 files changed, 29 insertions(+), 29 deletions(-) diff --git a/spec/unit/matrixrtc/RTCEncryptionManager.spec.ts b/spec/unit/matrixrtc/RTCEncryptionManager.spec.ts index a11b614bfe..a5e2255adf 100644 --- a/spec/unit/matrixrtc/RTCEncryptionManager.spec.ts +++ b/spec/unit/matrixrtc/RTCEncryptionManager.spec.ts @@ -84,7 +84,7 @@ 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(); }); @@ -99,7 +99,7 @@ describe("RTCEncryptionManager", () => { getMembershipMock.mockReturnValue(members); encryptionManager.join(undefined); - encryptionManager.onMembershipsUpdate([]); + encryptionManager.onMembershipsUpdate(); expect(mockTransport.sendKey).toHaveBeenCalledTimes(1); expect(mockTransport.sendKey).toHaveBeenCalledWith( @@ -123,7 +123,7 @@ describe("RTCEncryptionManager", () => { getMembershipMock.mockReturnValue(members); encryptionManager.join(undefined); - encryptionManager.onMembershipsUpdate([]); + encryptionManager.onMembershipsUpdate(); expect(mockTransport.sendKey).toHaveBeenCalledTimes(1); expect(mockTransport.sendKey).toHaveBeenCalledWith( @@ -150,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); @@ -180,7 +180,7 @@ describe("RTCEncryptionManager", () => { const gracePeriod = 15_000; // 15 seconds // initial rollout encryptionManager.join({ keyRotationGracePeriodMs: gracePeriod }); - encryptionManager.onMembershipsUpdate([]); + encryptionManager.onMembershipsUpdate(); await jest.runOnlyPendingTimersAsync(); expect(mockTransport.sendKey).toHaveBeenCalledTimes(1); @@ -202,7 +202,7 @@ describe("RTCEncryptionManager", () => { await jest.advanceTimersByTimeAsync(gracePeriod / 2); - encryptionManager.onMembershipsUpdate(updatedMembers); + encryptionManager.onMembershipsUpdate(); await jest.runOnlyPendingTimersAsync(); @@ -236,7 +236,7 @@ describe("RTCEncryptionManager", () => { useKeyDelay, keyRotationGracePeriodMs: gracePeriod, }); - encryptionManager.onMembershipsUpdate([]); + encryptionManager.onMembershipsUpdate(); await jest.runOnlyPendingTimersAsync(); onEncryptionKeysChanged.mockClear(); @@ -247,13 +247,13 @@ describe("RTCEncryptionManager", () => { // A new member joins, that should trigger a key rotation. members.push(aCallMembership("@carl:example.org", "CARLDEVICE")); - encryptionManager.onMembershipsUpdate([]); + 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((useKeyDelay - gracePeriod) / 2); - encryptionManager.onMembershipsUpdate([]); + encryptionManager.onMembershipsUpdate(); await jest.runOnlyPendingTimersAsync(); // Wait pass the delay period @@ -285,7 +285,7 @@ describe("RTCEncryptionManager", () => { const gracePeriod = 15_000; // 15 seconds // initial rollout encryptionManager.join({ keyRotationGracePeriodMs: gracePeriod }); - encryptionManager.onMembershipsUpdate([]); + encryptionManager.onMembershipsUpdate(); await jest.runOnlyPendingTimersAsync(); onEncryptionKeysChanged.mockClear(); @@ -300,7 +300,7 @@ describe("RTCEncryptionManager", () => { await jest.advanceTimersByTimeAsync(gracePeriod + 1000); - encryptionManager.onMembershipsUpdate(updatedMembers); + encryptionManager.onMembershipsUpdate(); await jest.runOnlyPendingTimersAsync(); @@ -332,7 +332,7 @@ describe("RTCEncryptionManager", () => { // initial rollout encryptionManager.join(undefined); - encryptionManager.onMembershipsUpdate([]); + encryptionManager.onMembershipsUpdate(); await jest.runOnlyPendingTimersAsync(); onEncryptionKeysChanged.mockClear(); @@ -350,7 +350,7 @@ describe("RTCEncryptionManager", () => { members.push(newJoiner); getMembershipMock.mockReturnValue(members); await jest.advanceTimersByTimeAsync(1_000); - encryptionManager.onMembershipsUpdate(members); + encryptionManager.onMembershipsUpdate(); await jest.runOnlyPendingTimersAsync(); } @@ -382,20 +382,20 @@ describe("RTCEncryptionManager", () => { // initial rollout encryptionManager.join(undefined); - encryptionManager.onMembershipsUpdate([]); + encryptionManager.onMembershipsUpdate(); await jest.runOnlyPendingTimersAsync(); 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(); @@ -412,7 +412,7 @@ describe("RTCEncryptionManager", () => { getMembershipMock.mockReturnValue(members); encryptionManager.join(undefined); - encryptionManager.onMembershipsUpdate([]); + encryptionManager.onMembershipsUpdate(); await jest.advanceTimersByTimeAsync(10); expect(mockTransport.sendKey).toHaveBeenCalledTimes(1); @@ -433,7 +433,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 @@ -491,7 +491,7 @@ describe("RTCEncryptionManager", () => { getMembershipMock.mockReturnValue(members); encryptionManager.join(undefined); - encryptionManager.onMembershipsUpdate([]); + encryptionManager.onMembershipsUpdate(); await jest.advanceTimersByTimeAsync(10); mockTransport.emit( @@ -598,7 +598,7 @@ describe("RTCEncryptionManager", () => { // Let's join encryptionManager.join(undefined); - encryptionManager.onMembershipsUpdate(members); + encryptionManager.onMembershipsUpdate(); await jest.advanceTimersByTimeAsync(10); @@ -685,7 +685,7 @@ describe("RTCEncryptionManager", () => { // Let's join encryptionManager.join(undefined); - encryptionManager.onMembershipsUpdate([]); + encryptionManager.onMembershipsUpdate(); await jest.advanceTimersByTimeAsync(10); // The initial rollout @@ -701,7 +701,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 @@ -709,14 +709,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 @@ -801,7 +801,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 diff --git a/src/matrixrtc/RTCEncryptionManager.ts b/src/matrixrtc/RTCEncryptionManager.ts index 8918abef2f..f59d046f70 100644 --- a/src/matrixrtc/RTCEncryptionManager.ts +++ b/src/matrixrtc/RTCEncryptionManager.ts @@ -224,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. From 615e3fd0b181f4d42ec1efe7a3dfbfa3d2f6c0e6 Mon Sep 17 00:00:00 2001 From: Valere Date: Fri, 18 Jul 2025 10:01:31 +0200 Subject: [PATCH 07/11] improve doc --- src/matrixrtc/MatrixRTCSession.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/matrixrtc/MatrixRTCSession.ts b/src/matrixrtc/MatrixRTCSession.ts index 6fde4861a8..a72f2cdcb3 100644 --- a/src/matrixrtc/MatrixRTCSession.ts +++ b/src/matrixrtc/MatrixRTCSession.ts @@ -162,6 +162,9 @@ export interface EncryptionConfig { * 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; @@ -177,7 +180,7 @@ export interface EncryptionConfig { * * 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. + * (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; } From 6a938125c6df006a19b7efa8d86233c32c2793ce Mon Sep 17 00:00:00 2001 From: Valere Date: Fri, 18 Jul 2025 10:01:45 +0200 Subject: [PATCH 08/11] cleanup test --- .../matrixrtc/RTCEncryptionManager.spec.ts | 21 ++++--------------- 1 file changed, 4 insertions(+), 17 deletions(-) diff --git a/spec/unit/matrixrtc/RTCEncryptionManager.spec.ts b/spec/unit/matrixrtc/RTCEncryptionManager.spec.ts index a5e2255adf..508f6ba802 100644 --- a/spec/unit/matrixrtc/RTCEncryptionManager.spec.ts +++ b/spec/unit/matrixrtc/RTCEncryptionManager.spec.ts @@ -193,15 +193,9 @@ 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); - + // Carl joins, within the grace period + members.push(aCallMembership("@carl:example.org", "CARLDEVICE")); await jest.advanceTimersByTimeAsync(gracePeriod / 2); - encryptionManager.onMembershipsUpdate(); await jest.runOnlyPendingTimersAsync(); @@ -291,15 +285,8 @@ 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); - + members.push(aCallMembership("@carl:example.org", "CARLDEVICE")); await jest.advanceTimersByTimeAsync(gracePeriod + 1000); - encryptionManager.onMembershipsUpdate(); await jest.runOnlyPendingTimersAsync(); @@ -321,7 +308,7 @@ describe("RTCEncryptionManager", () => { expect(statistics.counters.roomEventEncryptionKeysSent).toBe(2); }); - it("Should rotate key when several users join within the rotation grace period", async () => { + it("Should not rotate key when several users join within the rotation grace period", async () => { jest.useFakeTimers(); const members = [ From 31b2e0bbe9df2383244b88f7c8c93b74fecf43fe Mon Sep 17 00:00:00 2001 From: Valere Date: Fri, 18 Jul 2025 10:05:39 +0200 Subject: [PATCH 09/11] more comment in test --- spec/unit/matrixrtc/RTCEncryptionManager.spec.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/spec/unit/matrixrtc/RTCEncryptionManager.spec.ts b/spec/unit/matrixrtc/RTCEncryptionManager.spec.ts index 508f6ba802..618e309862 100644 --- a/spec/unit/matrixrtc/RTCEncryptionManager.spec.ts +++ b/spec/unit/matrixrtc/RTCEncryptionManager.spec.ts @@ -214,6 +214,9 @@ 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. it("test grace period lower than delay period", async () => { jest.useFakeTimers(); From 5dc032cc75d84db71dcb66e37471c0f0bbd8225c Mon Sep 17 00:00:00 2001 From: Timo Date: Fri, 18 Jul 2025 16:15:39 +0200 Subject: [PATCH 10/11] comment additions --- spec/unit/matrixrtc/RTCEncryptionManager.spec.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/spec/unit/matrixrtc/RTCEncryptionManager.spec.ts b/spec/unit/matrixrtc/RTCEncryptionManager.spec.ts index 618e309862..fa6c319b75 100644 --- a/spec/unit/matrixrtc/RTCEncryptionManager.spec.ts +++ b/spec/unit/matrixrtc/RTCEncryptionManager.spec.ts @@ -217,6 +217,7 @@ describe("RTCEncryptionManager", () => { // 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(); @@ -256,8 +257,8 @@ describe("RTCEncryptionManager", () => { // 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. + // 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"); From 82fffeaac518d94db9cbf066295a7f76efdf2ecd Mon Sep 17 00:00:00 2001 From: Timo Date: Fri, 18 Jul 2025 16:47:45 +0200 Subject: [PATCH 11/11] cleanup runOnlyPendingTimers --- .../matrixrtc/RTCEncryptionManager.spec.ts | 28 +++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/spec/unit/matrixrtc/RTCEncryptionManager.spec.ts b/spec/unit/matrixrtc/RTCEncryptionManager.spec.ts index fa6c319b75..1f92b6ff9a 100644 --- a/spec/unit/matrixrtc/RTCEncryptionManager.spec.ts +++ b/spec/unit/matrixrtc/RTCEncryptionManager.spec.ts @@ -138,7 +138,7 @@ describe("RTCEncryptionManager", () => { }, ], ); - await jest.runOnlyPendingTimersAsync(); + await jest.advanceTimersByTimeAsync(1); // The key should have been rolled out immediately expect(onEncryptionKeysChanged).toHaveBeenCalled(); @@ -181,7 +181,7 @@ describe("RTCEncryptionManager", () => { // initial rollout encryptionManager.join({ keyRotationGracePeriodMs: gracePeriod }); encryptionManager.onMembershipsUpdate(); - await jest.runOnlyPendingTimersAsync(); + await jest.advanceTimersByTimeAsync(1); expect(mockTransport.sendKey).toHaveBeenCalledTimes(1); expect(mockTransport.sendKey).toHaveBeenCalledWith( @@ -235,7 +235,7 @@ describe("RTCEncryptionManager", () => { keyRotationGracePeriodMs: gracePeriod, }); encryptionManager.onMembershipsUpdate(); - await jest.runOnlyPendingTimersAsync(); + await jest.advanceTimersByTimeAsync(1); onEncryptionKeysChanged.mockClear(); mockTransport.sendKey.mockClear(); @@ -246,15 +246,14 @@ describe("RTCEncryptionManager", () => { // A new member joins, that should trigger a key rotation. members.push(aCallMembership("@carl:example.org", "CARLDEVICE")); encryptionManager.onMembershipsUpdate(); - await jest.runOnlyPendingTimersAsync(); + 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(); - await jest.runOnlyPendingTimersAsync(); - // Wait pass the delay period + // 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 @@ -284,17 +283,15 @@ describe("RTCEncryptionManager", () => { // initial rollout encryptionManager.join({ keyRotationGracePeriodMs: gracePeriod }); encryptionManager.onMembershipsUpdate(); - await jest.runOnlyPendingTimersAsync(); + await jest.advanceTimersByTimeAsync(1); onEncryptionKeysChanged.mockClear(); mockTransport.sendKey.mockClear(); - members.push(aCallMembership("@carl:example.org", "CARLDEVICE")); await jest.advanceTimersByTimeAsync(gracePeriod + 1000); + members.push(aCallMembership("@carl:example.org", "CARLDEVICE")); encryptionManager.onMembershipsUpdate(); - await jest.runOnlyPendingTimersAsync(); - expect(mockTransport.sendKey).toHaveBeenCalledWith( expect.any(String), // It should have incremented the key index @@ -307,6 +304,9 @@ describe("RTCEncryptionManager", () => { ], ); + // Wait for useKeyDelay to pass + await jest.advanceTimersByTimeAsync(5000); + expect(onEncryptionKeysChanged).toHaveBeenCalled(); await jest.advanceTimersByTimeAsync(1000); expect(statistics.counters.roomEventEncryptionKeysSent).toBe(2); @@ -324,7 +324,7 @@ describe("RTCEncryptionManager", () => { // initial rollout encryptionManager.join(undefined); encryptionManager.onMembershipsUpdate(); - await jest.runOnlyPendingTimersAsync(); + await jest.advanceTimersByTimeAsync(1); onEncryptionKeysChanged.mockClear(); mockTransport.sendKey.mockClear(); @@ -342,7 +342,7 @@ describe("RTCEncryptionManager", () => { getMembershipMock.mockReturnValue(members); await jest.advanceTimersByTimeAsync(1_000); encryptionManager.onMembershipsUpdate(); - await jest.runOnlyPendingTimersAsync(); + await jest.advanceTimersByTimeAsync(1); } expect(mockTransport.sendKey).toHaveBeenCalledTimes(newJoiners.length); @@ -374,7 +374,7 @@ describe("RTCEncryptionManager", () => { // initial rollout encryptionManager.join(undefined); encryptionManager.onMembershipsUpdate(); - await jest.runOnlyPendingTimersAsync(); + await jest.advanceTimersByTimeAsync(1); expect(mockTransport.sendKey).toHaveBeenCalledTimes(1); onEncryptionKeysChanged.mockClear(); @@ -811,7 +811,7 @@ describe("RTCEncryptionManager", () => { await jest.runOnlyPendingTimersAsync(); - // The key should have beed re-distributed to the room transport + // The key should have been re-distributed to the room transport expect(mockRoomTransport.sendKey).toHaveBeenCalled(); expect(mockToDeviceTransport.sendKey).toHaveBeenCalledWith( expect.any(String),