MatrixRTC: Refactor | Introduce a new Encryption manager (used with experimental to device transport) #4799
Conversation
fixup: bad do not commit
BillCarsonFr
force-pushed
the
valere/rtc/simple_encryption_manager
branch
from
73f738b to
3af1d3e
Compare
BillCarsonFr
force-pushed
the
valere/rtc/simple_encryption_manager
branch
from
3af1d3e to
b19c7a6
Compare
| public getEncryptionKeys(): Map<string, Array<{ key: Uint8Array; timestamp: number }>> { | ||
| // This is deprecated should be ignored. Only use by tests? | ||
| return new Map(); | ||
| } |
There was a problem hiding this comment.
The deprecation could already be added to IEncryptionManager (@deprecated)
| * 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 { |
There was a problem hiding this comment.
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 = { |
There was a problem hiding this comment.
Why is this not next to the InboundEncryptionSession
| 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(",")}`, |
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
lets move this above the method since the whole method is temporary for backwards compatibility
|
Putting this back into draft until it's ready as agreed in chapter sync. |
| 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"; |
There was a problem hiding this comment.
please don't use logger.logger in new code: we should be threading the correct logger through from the parent MatrixClient.
There was a problem hiding this comment.
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) tojoinRoomSession
|
|
||
| /** | ||
| * 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. |
There was a problem hiding this comment.
| * 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. | |
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
public/exportedsymbols have accurate TSDoc documentation.