-
-
Notifications
You must be signed in to change notification settings - Fork 624
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
base: develop
Are you sure you want to change the base?
Conversation
fixup: bad do not commit
73f738b
to
3af1d3e
Compare
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.
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
)
There was a problem hiding this 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 { |
There was a problem hiding this comment.
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 = { |
There was a problem hiding this comment.
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
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.
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( |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
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.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this 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) 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* 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
/exported
symbols have accurate TSDoc documentation.