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

Element-R: edge cases around user verification #26258

Closed
4 of 6 tasks
richvdh opened this issue Sep 28, 2023 · 8 comments
Closed
4 of 6 tasks

Element-R: edge cases around user verification #26258

richvdh opened this issue Sep 28, 2023 · 8 comments
Labels
A-Element-R Issues affecting the port of Element's crypto layer to Rust O-Frequent Affects or can be seen by most users regularly or impacts most users' first experience S-Minor Impairs non-critical functionality or suitable workarounds exist T-Defect Team: Crypto

Comments

@richvdh richvdh added T-Defect S-Minor Impairs non-critical functionality or suitable workarounds exist O-Frequent Affects or can be seen by most users regularly or impacts most users' first experience Team: Crypto A-Element-R Issues affecting the port of Element's crypto layer to Rust labels Sep 28, 2023
@nao9797
Copy link

nao9797 commented Oct 6, 2023

I've had this issue for years where on my android phone I see "Encryption Upgrade Available" - Verify yourself & others to keep your chats safe. This banner at the top is very annoying and I would rather it at least stop interfering with the interface if it's a broken function that will not be fixed anytime soon. I'm sure this is already a known bug that if I actually click this banner, a message from below comes up that says "Re-Authentication Needed" and it just sits there and hangs (if left long enough) for hours without doing anything. For new users this would be frustrating and confusing, people wouldn't know if there's a problem with their phone or with their internet connection or with the app and slowly will come to realize its a problem with the app. By hitting the back arrow button you get a confirmation dialogue box "Are you sure? If you cancel now you may lose encrypted messages, etc etc" and cancelling the verification process is the only way to get back in the working part of the app. I've had this problem for 2 years or so and was expecting it to just be fixed with an update.
My opinion is that if it's going to behave in this form,
it should either

  • not interfere with the interface which can cause a disruption in the experience
  • or simply if it is going to interfere then it should work within a set amount of time, if the time-out is reached assume the process failed, set aside the banner, and put a warning elsewhere that is not disruptive with the interface.
    that may be redundant but I would highly appreciate this years-long issue to at least be temporarily fixed if not completely anytime soon, thanks

@richvdh
Copy link
Member Author

richvdh commented Oct 8, 2023

@nao9797 please open a separate issue at https://github.com/vector-im/element-android/issues describing your symptoms.

@florianduros
Copy link
Member

About the verification events in the Timeline:

  1. Two components are used:
    1. MKeyVerificationRequest
    2. MKeyVerificationConclusion
  2. MKeyVerificationConclusion is still using Crypto.checkUserTrust instead of Crypto.getUserVerificationStatus
  3. The two components need the event object to have the verificationRequest field to be set https://github.com/matrix-org/matrix-react-sdk/blob/develop/src/components/views/messages/MKeyVerificationRequest.tsx#L135
    1. https://github.com/matrix-org/matrix-js-sdk/blob/develop/src/crypto/index.ts#L1939 We need to listen to the Timeline like in the old crypto https://github.com/matrix-org/matrix-js-sdk/blob/develop/src/client.ts#L2352
    2. We need to set the RustVerificationRequest in the event object https://github.com/matrix-org/matrix-js-sdk/blob/develop/src/rust-crypto/rust-crypto.ts#L1445C6-L1445C6 like in the old crypto https://github.com/matrix-org/matrix-js-sdk/blob/develop/src/crypto/index.ts#L3749
    3. But the olmMachine doesn't know about this verification request https://github.com/matrix-org/matrix-js-sdk/blob/develop/src/rust-crypto/rust-crypto.ts#L1432 (request is undefined)

@richvdh
Copy link
Member Author

richvdh commented Oct 13, 2023

I think we need to reconsider how this bit of the UI works rather than try to replicate the current behaviour.

The two tiles behave very strangely in the face of repeated/cancelled verification requests, and page reloads, under legacy crypto.

And yes, we only keep a record of the current VerificationRequest, so we aren't going to be able to replicate the old behaviour exactly anyway.

@BillCarsonFr BillCarsonFr added the Z-Element-R-Blocker A blocker for enabling Element R by default label Nov 8, 2023
@github-actions github-actions bot added the Z-Labs label Nov 8, 2023
@BillCarsonFr
Copy link
Member

I think we need to reconsider how this bit of the UI works rather than try to replicate the current behaviour.

Fully agree with that.

The main goal of Key verification in room was to provide user-visible record of key verification.

There are problems with the current behavior: it's extremly complex to do it right.

Per nature the verification danse is live/transient and implies a complex crypto state machine.

We expect the UX layer to be able to replay historical verifications and replicate the complex crypto machine in order to display the correct outcome for an old verification.
=> Makes it an easy surface of attacks for evil homeservers to inject events in timeline to impact the displayed outcome.
=> Also the history of past verification could be confusing. If a user rotates it's key after the verification, the timeline will still display that the user is verified, which is now false.

Proposal

UX should only show a tile for m.key.verification.request event type, without any info about the outcome (user can just check the profile to get verification state), just who requested, and ignore other events related to that verification.

If there is a matching live verification ongoing, clicking on the tile should open the dedicated UX to perform the verification.
Eventually if the verification is not yet accepted, there could be the accept/decline buttons.

I think that this change should also impact legacy, mainly for simplicity and avoid having to manage two behaviors.

@BillCarsonFr
Copy link
Member

@pmaier1
Copy link

pmaier1 commented Nov 30, 2023

Thanks! I agree with the approach mentioned in #26258 (comment).

@richvdh
Copy link
Member Author

richvdh commented Dec 28, 2023

Closing in favour of the smaller issues.

@richvdh richvdh closed this as completed Dec 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Element-R Issues affecting the port of Element's crypto layer to Rust O-Frequent Affects or can be seen by most users regularly or impacts most users' first experience S-Minor Impairs non-critical functionality or suitable workarounds exist T-Defect Team: Crypto
Projects
None yet
Development

No branches or pull requests

5 participants