-
Notifications
You must be signed in to change notification settings - Fork 148
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
Retry decryption when new key index received #1272
Conversation
🦋 Changeset detectedLatest commit: 455470f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
src/e2ee/worker/FrameCryptor.ts
Outdated
if (this.keys.getKeySet(keyIndex) && this.keys.hasValidKey) { | ||
if (keyIndex !== this.keys.getCurrentKeyIndex()) { | ||
workerLogger.debug(`received frame with new key index ${keyIndex}`, this.logContext); | ||
this.keys.setCurrentKeyIndex(keyIndex); |
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 a frame with an outdated key arrives late, we wouldn't want to override the key index set on the key provider. The key provider should stay the authority on what's considered the current active key index.
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.
Okay.
So, instead I could make the FrameCryptor
track the last received key index independent of the key provider.
Or, as a complete alternative: would you prefer it if the validity of keys was tracked on a per key index basis rather than having the single hasValidKey
property?
This would mean that we wouldn't need to track the last index used. What do you think?
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 you set a new key index via keys.setCurrentKeyIndex
the key state gets reset anyways, I'm not sure I completely follow what the use case for this change would be
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.
(assuming that the key manager calls keyProvider.setCurrentKeyIndex
when appropriate)
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.
So, instead I could make the
FrameCryptor
track the last received key index independent of the key provider.
I've updated this PR to do this.
Or, as a complete alternative: would you prefer it if the validity of keys was tracked on a per key index basis rather than having the single
hasValidKey
property?
I am doing another branch to show this right now. I'll link it here when ready.
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.
Or, as a complete alternative: would you prefer it if the validity of keys was tracked on a per key index basis rather than having the single
hasValidKey
property?I am doing another branch to show this right now. I'll link it here when ready.
Roughly it would look like this: main...hughns:client-sdk-js:track-key-validity-per-index
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.
thanks, could you elaborate on when you're running into the issue currently? The original description says
Currently a receiving FrameCryptor will never retry decryption once the failureTolerance is hit and no new key are received.
So you're receiving a bunch of frames on which decryption fails after you've set the latest/newest key ?
Yes, the scenario is:
- a new participant joins a room.
- the existing participants generate new sender keys (with a new key index) and distribute them out of bands (via Matrix) to the new participant and the existing participants.
- the existing participants wait a period of time (some seconds) before using the new keys and continue sending media using the previous keys.
- the existing participants continue to send media using the previous keys so that the call is not interrupted.
- the new participant is receiving frames on the previous keys that it will never get the keys for.
- the timeout expires and the existing participants start sending frames using the new keys.
- the new participant should then be able to decode the frames it receives with the new keys.
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.
got it, so the key management layer only distributes keys but does not distribute currently expected key indices?
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.
so the key management layer only distributes keys but does not distribute currently expected key indices?
Correct.
The key management layer can also invalidate keys. So, once a participant is in the call and has at least one valid key they would know what the expected set of indices are. They correspond to the set of keys for which: they have received a key; and the key has not yet been invalidated.
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.
thanks for clarifying.
The general intent was for the key management layer to announce the expected active key, but maybe that was a misinformed decision.
conceptually I prefer the valid state per key, as the proposal in this PR right now taps into the key provider state from within the cryptor, which I'd like to avoid as much as possible.
The proposal for the per key valid state currently breaks backwards compatibility, in order to avoid that you'd have to
- make the key indices optional and default to currentKeyIndex
- keep the
hasValidKey
getter - if
resetKeyStatus
is called without an index argument, reset the key status for all keys
Superseded by #1284 |
This is to be applied on top of #1271
i.e. the change can be previewed at hughns/client-sdk-js@unit-test-key-handling...hughns:client-sdk-js:retry-decryption-on-new-key-index
Currently a receiving
FrameCryptor
will never retry decryption once thefailureTolerance
is hit and no new key are received.This change tracks the last received key index and causes the key status to be reset when a different index is received that on the last frame.