RTCEncryptionManager: Joiner key rotation grace period

spec/unit/matrixrtc/RTCEncryptionManager.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";
 import { getParticipantId } from "../../../src/matrixrtc/utils.ts";
 
 describe("RTCEncryptionManager", () => {
@@ -62,6 +62,7 @@ describe("RTCEncryptionManager", () => {
             mockTransport,
             statistics,
             onEncryptionKeysChanged,
+            logger,
         );
     });
 
@@ -89,6 +90,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"),
@@ -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,8 +177,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();
 
@@ -197,6 +200,8 @@ describe("RTCEncryptionManager", () => {
             ];
             getMembershipMock.mockReturnValue(updatedMembers);
 
+            await jest.advanceTimersByTimeAsync(8_000);
+
             encryptionManager.onMembershipsUpdate(updatedMembers);
 
             await jest.runOnlyPendingTimersAsync();
@@ -215,6 +220,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();
 
@@ -683,3 +839,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 })]),
+    );
+}

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;
 }

src/matrixrtc/RTCEncryptionManager.ts

@@ -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.
      */
-    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.
@@ -116,6 +128,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) {
@@ -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 `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;
@@ -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;