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
| participantId: ParticipantId, | ||
| ) => void, | ||
| ) { | ||
| this.logger = logger.getChild("BasicEncryptionManager"); |
There was a problem hiding this comment.
use the parent logger concept here (pass the logger down via a prop)
| * A simple encryption manager. | ||
| * This manager is basic becasue it will rotate the keys for any membership change. | ||
| * There is no ratchetting, or time based rotation. | ||
| * It works with to-device transport. |
There was a problem hiding this comment.
It should work with both right: room & toDevice
| /** | ||
| * There is a possibility that keys arrive in wrong order. | ||
| * For example after a quick join/leave/join, there will be 2 keys of index 0 distributed and | ||
| * it they are received in wrong order the stream won't be decryptable. |
There was a problem hiding this comment.
| * it they are received in wrong order the stream won't be decryptable. | |
| * if they are received in wrong order the stream won't be decryptable. |
| 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)
| } | ||
| } | ||
|
|
||
| private nextKeyId(): number { |
There was a problem hiding this comment.
| private nextKeyId(): number { | |
| private nextKeyIndex(): number { |
| }, this.ttl); | ||
|
|
||
| const existing = entry.keys.get(item.keyId); | ||
| if (existing && existing.creationTS > item.creationTS) { |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
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.