Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 32 commits into
base: develop
Choose a base branch
from

Conversation

BillCarsonFr
Copy link
Member

@BillCarsonFr BillCarsonFr commented Apr 11, 2025

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

@BillCarsonFr BillCarsonFr added the T-Task Tasks for the team like planning label Apr 11, 2025
Comment on lines 84 to 87
public getEncryptionKeys(): Map<string, Array<{ key: Uint8Array; timestamp: number }>> {
// This is deprecated should be ignored. Only use by tests?
return new Map();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@toger5 toger5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* 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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

RoomAndToDeviceTransport,
} from "./RoomAndToDeviceKeyTransport.ts";

type OutboundEncryptionSession = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this not next to the InboundEncryptionSession

Comment on lines 284 to 290
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(",")}`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

sharedWith: [],
keyId: 0,
};
this.onEncryptionKeysChanged(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@dbkr
Copy link
Member

dbkr commented Jun 30, 2025

Putting this back into draft until it's ready as agreed in chapter sync.

@dbkr dbkr marked this pull request as draft June 30, 2025 12:07
import { type CallMembership } from "./CallMembership.ts";
import { decodeBase64, encodeBase64 } from "../base64.ts";
import { type IKeyTransport, type KeyTransportEventListener, KeyTransportEvents } from "./IKeyTransport.ts";
import { logger as rootLogger, type Logger } from "../logger.ts";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please don't use logger.logger in new code: we should be threading the correct logger through from the parent MatrixClient.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@toger5 toger5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did a sync (call) review session.

Open todos:

  • solve reemission (possible missing keys) situation
    • either by adding a stop gap key buffer to the new encryption manager
    • or by forcing to allocate + subscribe on joinRoomSession (passing the matrixKeyProvider (as a keySink interface) to joinRoomSession


/**
* 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* If this function is called repeatidly, the calls will be buffered to a single key rotation.
* If this function is called repeatedly, the calls will be buffered to a single key rotation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Task Tasks for the team like planning
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants