diff --git a/spec/unit/matrixrtc/RTCEncryptionManager.spec.ts b/spec/unit/matrixrtc/RTCEncryptionManager.spec.ts index 19c5be8ee7..70e53993d1 100644 --- a/spec/unit/matrixrtc/RTCEncryptionManager.spec.ts +++ b/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, ); }); @@ -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, + 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/MatrixRTCSession.ts b/src/matrixrtc/MatrixRTCSession.ts index 4da3dc5ab2..943ed6a0ca 100644 --- a/src/matrixrtc/MatrixRTCSession.ts +++ b/src/matrixrtc/MatrixRTCSession.ts @@ -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; } diff --git a/src/matrixrtc/RTCEncryptionManager.ts b/src/matrixrtc/RTCEncryptionManager.ts index c4f653f2ba..f59d046f70 100644 --- a/src/matrixrtc/RTCEncryptionManager.ts +++ b/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 | 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;