RTCEncryptionManager: Joiner key rotation grace period #4911

BillCarsonFr · 2025-07-08T14:52:11Z

Fixes https://github.com/element-hq/voip-internal/issues/371

Checklist

Tests written for new code (and old code if feasible).
New or updated public/exported symbols have accurate TSDoc documentation.
Linter and other CI checks pass.
Sign-off given on the changes (see CONTRIBUTING.md).

…ith_grace

toger5

The implementation looks very simple (In terms of: done in a way that is easy to follow along) and sane. I do have questions on the tests. So I need a bit of help (with comments) to understand what is supposed to happen.

toger5 · 2025-07-16T09:56:26Z

spec/unit/matrixrtc/RTCEncrytionManager.spec.ts

+            const gracePeriod = 15_000; // 15 seconds
            // initial rollout
-            encryptionManager.join(undefined);
+            encryptionManager.join({ keyRotationGracePeriodMs: gracePeriod });


this we can just move into the join() method?
Since we use the object config the key is written out and the reason for the value is explained

toger5 · 2025-07-16T09:56:51Z

spec/unit/matrixrtc/RTCEncrytionManager.spec.ts

+            const gracePeriod = 15_000; // 15 seconds
            // initial rollout
-            encryptionManager.join(undefined);
+            encryptionManager.join({ keyRotationGracePeriodMs: gracePeriod });


Suggested change

const gracePeriod = 15_000; // 15 seconds

// initial rollout

encryptionManager.join(undefined);

encryptionManager.join({ keyRotationGracePeriodMs: gracePeriod });

encryptionManager.join({ keyRotationGracePeriodMs: 15_000 });

toger5 · 2025-07-16T10:08:25Z

spec/unit/matrixrtc/RTCEncrytionManager.spec.ts

+            const gracePeriod = 3_000; // 15 seconds
+            const useKeyDelay = 5_000; // 5 seconds
+            // initial rollout
+            encryptionManager.join({
+                useKeyDelay,
+                keyRotationGracePeriodMs: gracePeriod,
+            });


Suggested change

const gracePeriod = 3_000; // 15 seconds

const useKeyDelay = 5_000; // 5 seconds

// initial rollout

encryptionManager.join({

useKeyDelay,

keyRotationGracePeriodMs: gracePeriod,

});

// initial rollout

encryptionManager.join({

useKeyDelay: 3_000, // 3 seconds

keyRotationGracePeriodMs: 5_000, // 5 seconds

});

The comment seemed wrong (3 vs 15 s)

toger5 · 2025-07-16T10:10:20Z

spec/unit/matrixrtc/RTCEncrytionManager.spec.ts

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


Why can we just pass [] as the old memberships here?

I am thinking of sth like:

const oldMembers = [...members]; members.push(aCallMembership("@carl:example.org", "CARLDEVICE")); encryptionManager.onMembershipsUpdate(oldMembers);

Just to make sure the test env is more correct to the real world. Even if the curren impl also works with []
(also makes the test easier to read if we dont know exactly what the onMembershipsUpdate param means)

I might as well deprecate it? as it is not used

ah in that case yes!

toger5 · 2025-07-16T10:11:15Z

spec/unit/matrixrtc/RTCEncrytionManager.spec.ts

+            // 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([]);


As above.
Shouldn't the old memberships be the correct prev memberships, so the test more properly represents a real world scenario?

toger5 · 2025-07-16T13:04:52Z

src/matrixrtc/MatrixRTCSession.ts

+     * 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.


Suggested change

* If 5 people leave 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.

toger5 · 2025-07-16T13:06:28Z

src/matrixrtc/MatrixRTCSession.ts

+     *
+     * 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.


Suggested change

* (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.

toger5 · 2025-07-16T13:07:33Z

src/matrixrtc/RTCEncryptionManager.ts

    /**
     * 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;


is this the same as useKeyDelay. If so we should rename one of them.

toger5 · 2025-07-16T13:08:44Z

src/matrixrtc/RTCEncryptionManager.ts

+     * If it is lower, the current key will always be older than the grace period.
+     * @private
+     */
+    private skipRotationGracePeriod = 10_000;


same here keyRotationGracePeriodMs

toger5 · 2025-07-16T13:09:38Z

src/matrixrtc/RTCEncryptionManager.ts

        this.delayRolloutTimeMillis = joinConfig?.useKeyDelay ?? 1000;
+        this.skipRotationGracePeriod = joinConfig?.keyRotationGracePeriodMs ?? 10_000;


This should totally be:

Suggested change

this.delayRolloutTimeMillis = joinConfig?.useKeyDelay ?? 1000;

this.skipRotationGracePeriod = joinConfig?.keyRotationGracePeriodMs ?? 10_000;

this.useKeyDelay = joinConfig?.useKeyDelay ?? 1000;

this.keyRotationGracePeriodMs = joinConfig?.keyRotationGracePeriodMs ?? 10_000;

Or the other names from this class

…ith_grace

BillCarsonFr force-pushed the valere/rtc/encryption_mgr/rotate_joiner_with_grace branch from cd3bde8 to 1e5efc3 Compare July 8, 2025 14:52

github-actions bot deployed to PR Documentation Preview July 8, 2025 14:54 View deployment

BillCarsonFr added the T-Task Tasks for the team like planning label Jul 8, 2025

BillCarsonFr marked this pull request as ready for review July 8, 2025 15:22

BillCarsonFr requested a review from a team as a code owner July 8, 2025 15:22

BillCarsonFr requested review from hughns and toger5 July 8, 2025 15:22

RTCEncryptionManager: Joiner key rotation grace period

1d798cb

BillCarsonFr force-pushed the valere/rtc/encryption_mgr/rotate_joiner_with_grace branch from 1e5efc3 to 1d798cb Compare July 8, 2025 15:24

github-actions bot deployed to PR Documentation Preview July 8, 2025 15:25 View deployment

Test to clarify useKeyDelay and keyRotationGracePeriodMs interference

0bda8e0

github-actions bot deployed to PR Documentation Preview July 10, 2025 14:17 View deployment

Merge branch 'develop' into valere/rtc/encryption_mgr/rotate_joiner_w…

a23c629

…ith_grace

github-actions bot deployed to PR Documentation Preview July 10, 2025 14:34 View deployment

toger5 requested changes Jul 16, 2025

View reviewed changes

Merge branch 'develop' into valere/rtc/encryption_mgr/rotate_joiner_w…

46f3e89

…ith_grace

github-actions bot deployed to PR Documentation Preview July 17, 2025 07:43 View deployment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

RTCEncryptionManager: Joiner key rotation grace period #4911

RTCEncryptionManager: Joiner key rotation grace period #4911

BillCarsonFr commented Jul 8, 2025 •

edited

Loading

Uh oh!

toger5 left a comment •

edited

Loading

Uh oh!

toger5 Jul 16, 2025

Uh oh!

toger5 Jul 16, 2025

Uh oh!

toger5 Jul 16, 2025

Uh oh!

toger5 Jul 16, 2025

Uh oh!

toger5 Jul 16, 2025

Uh oh!

BillCarsonFr Jul 16, 2025

Uh oh!

toger5 Jul 17, 2025

Uh oh!

toger5 Jul 16, 2025

Uh oh!

toger5 Jul 16, 2025

Uh oh!

toger5 Jul 16, 2025

Uh oh!

toger5 Jul 16, 2025

Uh oh!

toger5 Jul 16, 2025

Uh oh!

toger5 Jul 16, 2025

Uh oh!

toger5 Jul 16, 2025

Uh oh!

Uh oh!

	* (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.

		this.delayRolloutTimeMillis = joinConfig?.useKeyDelay ?? 1000;
		this.skipRotationGracePeriod = joinConfig?.keyRotationGracePeriodMs ?? 10_000;

Uh oh!

RTCEncryptionManager: Joiner key rotation grace period #4911

Are you sure you want to change the base?

RTCEncryptionManager: Joiner key rotation grace period #4911

Conversation

BillCarsonFr commented Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

toger5 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BillCarsonFr commented Jul 8, 2025 •

edited

Loading

toger5 left a comment •

edited

Loading