Skip to content
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

Per participant key lost between js-sdk and EC in embedded mode #2561

Open
hughns opened this issue Aug 13, 2024 · 5 comments
Open

Per participant key lost between js-sdk and EC in embedded mode #2561

hughns opened this issue Aug 13, 2024 · 5 comments
Assignees
Labels
T-Defect Something isn't working: bugs, crashes, hangs, vulnerabilities, or other reported problems

Comments

@hughns
Copy link
Member

hughns commented Aug 13, 2024

10:27:25.999 Decryption failed for event $V41v4SurT7YQYmPqw8-Ja_aNedyzC9X_7FP6K0d8Bd0: MEGOLM_UNKNOWN_INBOUND_SESSION_ID will retry once only
...
10:27:27.000 Decryption succeeded for event $V41v4SurT7YQYmPqw8-Ja_aNedyzC9X_7FP6K0d8Bd0 after retry
...
10:27:27.000 Setting key at index 0 for @hughns:ess-ecall-poc.ems-support.element.dev:YERKDYHWCR with timestamp 1723541245901: Jq4FqHsIlxdcje1qY4hJog

Then there is no subsequent log entry like:

MatrixKeyProvider Sent new key to livekit room=!ZWXChHvbxzRiDtOPzC:matrix.org participantId=@hughns:ess-ecall-poc.ems-support.element.dev:YERKDYHWCR encryptionKeyIndex=0: Jq4FqHsIlxdcje1qY4hJog
@hughns
Copy link
Member Author

hughns commented Aug 13, 2024

Looking at possible causes:

  • Key had older timestamp: if (existingKeyAtIndex && existingKeyAtIndex.timestamp > timestamp) { but there is no Ignoring new key at index log entry
  • Key matched existing key: if (existingKeyAtIndex && keysEqual(existingKeyAtIndex.key, keyBin)) { would silently fail, but there is no previous Setting key at index entry for the same key
  • if (delayBeforeUse) { but delayBeforeUse is only true when creating your own key not consuming one from elsewhere
  • this.emit(MatrixRTCSessionEvent.EncryptionKeyChanged, keyBin, encryptionKeyIndex, participantId); failed - TODO: what is the code path from here via the widget API in to the embedded EC?

@hughns hughns changed the title Per participant key lost between js-sdk and EC Per participant key lost between js-sdk and EC in embedded mode Aug 13, 2024
@hughns hughns self-assigned this Aug 13, 2024
@hughns hughns added the T-Defect Something isn't working: bugs, crashes, hangs, vulnerabilities, or other reported problems label Aug 13, 2024
@hughns
Copy link
Member Author

hughns commented Aug 14, 2024

So, the log lines are confusing as we have two JavaScript contexts: EW and embedded EC within the iframe.

The log lines above are entirely expected within the EW context. And there is actually no-one listening in to this.emit(MatrixRTCSessionEvent.EncryptionKeyChanged, keyBin, encryptionKeyIndex, participantId); within EW.

What is supposed to happen is the that (decrypted) events get sent via the StopGapWidget in EW which in turn forward to the EC context. In that context the events are processed by the MatrixRTC code again and passed on to EC as expected.

n.b. There is an optimisation by making it so that the EW context does not track the encryption keys as it doesn't need to.

The underlying issue is that we keep hitting the condition here because we see sequences of emits like this:

  • ClientEvent.Event => event1 with isDecryptionFailure()=true
  • ClientEvent.Event => event2 with isDecryptionFailure()=false
  • MatrixEventEvent.Decrypted => event1 with isDecryptionFailure()=false

When event1 is emitted for the second time it is ignore as it appears in the timeline before event2 which has already been "seen".

@hughns
Copy link
Member Author

hughns commented Aug 14, 2024

n.b. There is an optimisation by making it so that the EW context does not track the encryption keys as it doesn't need to.

This is being tracked by #2566.

@hughns
Copy link
Member Author

hughns commented Aug 16, 2024

I've done a possible fix for this in matrix-org/matrix-react-sdk#12890 but I don't know if this is the right place/direction to fix it as it would change the semantics for other users of the widget API.

@hughns
Copy link
Member Author

hughns commented Sep 20, 2024

The conclusion is that we won't fix this until we use to-device messaging for key distribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Defect Something isn't working: bugs, crashes, hangs, vulnerabilities, or other reported problems
Projects
None yet
Development

No branches or pull requests

1 participant