MatrixRTC: Refactor | Introduce a new Encryption manager (used with experimental to device transport) #4799

BillCarsonFr · 2025-04-11T12:48:00Z

Add a new simplified EncryptionManager that will rotate the key for any membership change (join or leave or same)

There is no specific experimental flag to use it, it will use it for to device transport

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).

fixup: bad do not commit

src/matrixrtc/BasicEncryptionManager.ts

toger5 · 2025-04-15T10:36:34Z

src/matrixrtc/BasicEncryptionManager.ts

+    public getEncryptionKeys(): Map<string, Array<{ key: Uint8Array; timestamp: number }>> {
+        // This is deprecated should be ignored. Only use by tests?
+        return new Map();
+    }


The deprecation could already be added to IEncryptionManager (@deprecated)

src/matrixrtc/BasicEncryptionManager.ts

toger5 · 2025-04-15T11:18:16Z

src/matrixrtc/utils.ts

+        }, this.ttl);
+
+        const existing = entry.keys.get(item.keyId);
+        if (existing && existing.creationTS > item.creationTS) {


creationTS, timestamp, send_ts is all the same thing right?

Is it possible to call them the same (or maybe sendTs + send_ts)
(edit: oh its: sent_ts... Is that what we use in the spec.)

I don't think they are the same. CreationTS would be the time when the key is created, sent_ts is the local time when we sent it (for new joiner without rotation they would be different).
yes sent_ts is from the spec.
Maybe we need both? CreationTS is better to disambiguate keys, but sent_ts is what we want for statistics to have an estimate of the time to transport

in the spec we should make sent_ts -> creation_ts then right? the spec should focus on the disambiguation instead of the possibility to collect reasonable stats.

toger5 · 2025-04-15T11:28:12Z

src/matrixrtc/utils.ts

I am not sure the time to live is the correct concept here. It would not work if the out of order receive offset time is larger than 1s.
Can we just use a "latest key buffer"? Of course we would end up with more memory use since a 100ppl call would have 100 entries where the ttl approach probably would delete them quick enough so it would never exceed a handful.

The latest key buffer would just store the most recent (by timestamp) key for the highest (considering wrap around) index.

TBH I think we could do without it complitely as it is probably very edge casy. We also want to avoid keeping key in memory.
Only keeping the latest (1 key) might not be enough. If there was 3 keys 0, 1, 0' that are received in 0' 1 , 0, the latest would be 1 but we want 0

Only keeping the latest (1 key) might not be enough. If there was 3 keys 0, 1, 0' that are received in 0' 1 , 0, the latest would be 1 but we want 0

Yes covering all cases requies us to store all keys ever received.
What about we store all timestamps ever received? That is enough right. The disambigution is then just a "is it the most recent key we want to use" check.
Could be called keyRecencyMap.isMostRecentKeyForParticipant(userId, key.creationTs)

What do you think about this?

(somehow the context got lost here: #4799 (comment)

toger5

This is very nice to read. Thanks valere.
Left a couple of comments about additional context that we could give. maybe you think they can improve readablility even more.
But it is ready to merge without these.

Only the corepacks issue is blocking.

toger5 · 2025-05-26T17:29:21Z

package.json

+    },
+    "packageManager": "[email protected]+sha512.a6b2f7906b721bba3d67d4aff083df04dad64c399707841b7acf00f6b133b7ac24255f2652fa22ae3534329dc6180534e98d17432037ff6fd140556e2bb3137e"
 }


This looks like corepack is enabled.

toger5 · 2025-05-26T17:49:14Z

src/matrixrtc/RTCEncryptionManager.ts

+     * Will ensure that a new key is distributed and used to encrypt our media.
+     * If this function is called repeatidly, the calls will be buffered to a single key rotation.
+     */
+    private ensureMediaKey(): void {


This method name can benefit from being longer. It is not clear immediately if this ensures the keys are sent or that we do have the keys.

ensureDistributionOfLatestKey,ensureMediaKeyDistribution...?

toger5 · 2025-05-26T17:54:16Z

src/matrixrtc/RTCEncryptionManager.ts

+    RoomAndToDeviceTransport,
+} from "./RoomAndToDeviceKeyTransport.ts";
+
+type OutboundEncryptionSession = {


Why is this not next to the InboundEncryptionSession

toger5 · 2025-05-26T17:57:22Z

src/matrixrtc/RTCEncryptionManager.ts

+        try {
+            this.logger.trace(`Sending key...`);
+            await this.transport.sendKey(encodeBase64(outboundKey.key), outboundKey.keyId, toDistributeTo);
+            this.statistics.counters.roomEventEncryptionKeysSent += 1;
+            outboundKey.sharedWith.push(...toDistributeTo);
+            this.logger.trace(
+                `key index:${outboundKey.keyId} sent to ${outboundKey.sharedWith.map((m) => `${m.userId}:${m.deviceId}`).join(",")}`,


I think we need a comment, that sendKey calls encryptAndSentToDevice whihc does not wrapt the http reuquest but instead is a nonfailing operation, that will queue the to-device message.

toger5 · 2025-05-26T17:59:45Z

src/matrixrtc/utils.ts

What do you think about this?

(somehow the context got lost here: #4799 (comment)

toger5 · 2025-05-26T18:03:46Z

src/matrixrtc/RTCEncryptionManager.ts

+                sharedWith: [],
+                keyId: 0,
+            };
+            this.onEncryptionKeysChanged(


I think this can get a comment, that this is the method, that will actaully set the encryption key in the media layer.
Maybe best is a doc comment on the private property.

toger5 · 2025-05-26T18:05:43Z

src/matrixrtc/RTCEncryptionManager.ts

+
+    private onTransportChanged: (enabled: EnabledTransports) => void = () => {
+        this.logger.info("Transport change detected, restarting key distribution");
+        // Temporary for backwards compatibility


lets move this above the method since the whole method is temporary for backwards compatibility

BillCarsonFr mentioned this pull request Apr 11, 2025

DRAFT -- DO NOT MERGE -- POC for Basic Encryption Manager #4794

Closed

4 tasks

BillCarsonFr added the T-Task Tasks for the team like planning label Apr 11, 2025

refactor: New encryption manager BasicEncryptionManager for todevice

0f4e370

fixup: bad do not commit

BillCarsonFr force-pushed the valere/rtc/simple_encryption_manager branch from 73f738b to 3af1d3e Compare April 11, 2025 12:52

github-actions bot deployed to PR Documentation Preview April 11, 2025 12:53 View deployment

fix: ToDevice transport not setting the sent_ts

b19c7a6

BillCarsonFr force-pushed the valere/rtc/simple_encryption_manager branch from 3af1d3e to b19c7a6 Compare April 11, 2025 13:02

github-actions bot deployed to PR Documentation Preview April 11, 2025 13:04 View deployment

test: BasicEncryptionManager add statistics tests

6411597

github-actions bot deployed to PR Documentation Preview April 11, 2025 13:23 View deployment

BillCarsonFr marked this pull request as ready for review April 11, 2025 13:35

BillCarsonFr requested a review from a team as a code owner April 11, 2025 13:35

BillCarsonFr requested review from hughns and toger5 April 11, 2025 13:35

toger5 requested changes Apr 15, 2025

View reviewed changes

BillCarsonFr added 2 commits April 15, 2025 18:20

code review

7e0cf4d

Merge branch 'develop' into valere/rtc/simple_encryption_manager

ecf3f82

toger5 mentioned this pull request Apr 16, 2025

[ER-59] E2EE: Send media keys via to_device message element-hq/element-call#1281

Open

46 tasks

BillCarsonFr added 2 commits April 17, 2025 13:43

feat: Encryption manager just reshare on new joiner

c1b9e0f

refactor: Rename BasicEncryptionManger to RTCEncryptionManager

8ad79ef

BillCarsonFr requested a review from a team as a code owner April 17, 2025 11:45

BillCarsonFr requested a review from MidhunSureshR April 17, 2025 11:45

github-actions bot deployed to PR Documentation Preview April 17, 2025 11:47 View deployment

BillCarsonFr added 2 commits April 17, 2025 15:12

fixup: RTC experimental todevice should use new encryption mgr

ec4c466

fixup: use proper logger hierarchy

80bd66d

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

fixup: RTC rollout first key asap even if no members to send to

d79fc58

github-actions bot deployed to PR Documentation Preview April 17, 2025 13:34 View deployment

BillCarsonFr added 2 commits April 17, 2025 16:24

fixup: RTC add test for first key use

8e9af36

Merge branch 'develop' into valere/rtc/simple_encryption_manager

9b06920

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

fixup! emitting outbound key before anyone registered

a9413f9

github-actions bot deployed to PR Documentation Preview April 17, 2025 15:31 View deployment

fix: quick patch for transport switch, need test

be3c359

github-actions bot deployed to PR Documentation Preview April 18, 2025 14:37 View deployment

test: RTC encryption manager, add test for transport switch

07af3d9

github-actions bot deployed to PR Documentation Preview April 22, 2025 07:20 View deployment

BillCarsonFr mentioned this pull request Apr 25, 2025

Feature(MatrixRTC): E2E ratchet key on new joiners #4816

Draft

4 tasks

BillCarsonFr added 2 commits May 13, 2025 10:03

Merge branch 'develop' into valere/rtc/simple_encryption_manager

5e7043f

post rebase fix

cf05a8f

github-actions bot deployed to PR Documentation Preview May 13, 2025 08:09 View deployment

Merge branch 'develop' into valere/rtc/simple_encryption_manager

64cbec1

github-actions bot deployed to PR Documentation Preview May 19, 2025 14:32 View deployment

BillCarsonFr requested a review from toger5 May 19, 2025 14:46

toger5 assigned toger5 and unassigned toger5 May 26, 2025

toger5 requested changes May 26, 2025

View reviewed changes

Uh oh!

MatrixRTC: Refactor | Introduce a new Encryption manager (used with experimental to device transport) #4799

Are you sure you want to change the base?

MatrixRTC: Refactor | Introduce a new Encryption manager (used with experimental to device transport) #4799

Uh oh!

Conversation

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

Uh oh!

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

toger5 May 26, 2025 • 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!

toger5 left a comment

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!

toger5 May 26, 2025 • 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!

Uh oh!

BillCarsonFr commented Apr 11, 2025 •

edited

Loading

toger5 May 26, 2025 •

edited

Loading

toger5 May 26, 2025 •

edited

Loading