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

MSC2399: Reporting that decryption keys are withheld #2399

Merged
merged 9 commits into from Jun 13, 2020

Conversation

uhoreg
Copy link
Member

@uhoreg uhoreg commented Jan 2, 2020

@uhoreg uhoreg changed the title MSC xxxx: Reporting that decryption keys are withheld MSC 2399: Reporting that decryption keys are withheld Jan 2, 2020
@uhoreg uhoreg added the proposal label Jan 2, 2020
@turt2live turt2live self-requested a review Jan 9, 2020
@ara4n
Copy link
Member

@ara4n ara4n commented Jan 9, 2020

this is looking very good to me, fwiw

@turt2live turt2live changed the title MSC 2399: Reporting that decryption keys are withheld MSC2399: Reporting that decryption keys are withheld Feb 5, 2020
Copy link
Member

@turt2live turt2live left a comment

seems sane

proposals/2399-reporting-no-key-sent.md Outdated Show resolved Hide resolved
@uhoreg
Copy link
Member Author

@uhoreg uhoreg commented Mar 19, 2020

This has been implemented and seems to be working.

@uhoreg uhoreg marked this pull request as ready for review Mar 19, 2020
@turt2live turt2live added the kind:core label Apr 20, 2020
@uhoreg
Copy link
Member Author

@uhoreg uhoreg commented Apr 22, 2020

@mscbot fcp merge

@mscbot
Copy link
Collaborator

@mscbot mscbot commented Apr 22, 2020

Team member @uhoreg has proposed to merge this. The next step is review by the rest of the tagged people:

Once at least 75% of reviewers approve (and there are no outstanding concerns), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for information about what commands tagged team members can give me.

@mscbot mscbot added disposition-merge proposed-final-comment-period labels Apr 22, 2020
@cloudyfuel
Copy link

@cloudyfuel cloudyfuel commented May 5, 2020

I think I have a better solution.

Instead of Bob being responsible for telling Alice that Alice's device doesn't send keys, the Alice's device - the root of the problem - should be responsible for this. Bob should be shown a generic status event like 'Alice didn't send you the key, this may be because she didn't verify you, blacklisted you' or no message at all on the corresponding device.

Why leak verification status, blacklisting status or if the user lost their key to random third parties the user most likely doesn't wish to communicate with? Why rely on an entity completely out of your control to tell you there's something wrong with your device? Why hide the fact that someone is probing me for keys? Seems intuitive that I should know if someone is contacting me instead of them knowing that and additionally a bunch of metadata about my setup.

Separately from this, if there's a real need for communicating that to random parties, the device could communicate that. But that makes sense only for communicating errors and only when this can point the sender to fixing the problem (rather than saying: oh there's an error, but you can't do anything about it lol).

Edit: AFAIU the key withheld events are also unencrypted, so they leak metadata to the server too. This is at least as bad and can be mitigate similarly by simply informing the user about these problems rather than needlessly sending that info to whoever via the server.

@uhoreg
Copy link
Member Author

@uhoreg uhoreg commented May 6, 2020

@cloudyfuel I'm confused by what you're saying.

Instead of Bob being responsible for telling Alice that Alice's device doesn't send keys, the Alice's device - the root of the problem - should be responsible for this.

This is what the proposal is. The Alice's device is sending a message saying that it is purposely not sending keys to Bob.

Bob should be shown a generic status event like 'Alice didn't send you the key, this may be because she didn't verify you, blacklisted you' or no message at all on the corresponding device.

This is what is done without this proposal. The generic message is "The sender's device has not sent us the keys for this message." in Riot Web.

Why leak verification status, blacklisting status or if the user lost their key to random third parties the user most likely doesn't wish to communicate with?

It's not leaked to random third parties. It's sent as a to-device message, so only the users and their homeserver admins can see it. Also, "lost their key"?

Why rely on an entity completely out of your control to tell you there's something wrong with your device?

That is not what the proposal it doing.

Why hide the fact that someone is probing me for keys? Seems intuitive that I should know if someone is contacting me instead of them knowing that and additionally a bunch of metadata about my setup.

Huh? I don't understand what this has to do with the proposal.

But that makes sense only for communicating errors and only when this can point the sender to fixing the problem (rather than saying: oh there's an error, but you can't do anything about it lol).

In practice, it has helped debug some cases of unintentional unable-to-decrypt errors by pointing to the exact cause of the issue (e.g. the sender had turned on the option to only send to verified devices without meaning to). It also indicates to the user that they should not expect to be able to read the message, so they don't have to wonder if they'll ever be able to.

Edit: AFAIU the key withheld events are also unencrypted, so they leak metadata to the server too.

The current implementation in Riot sends them unencrypted, but they could be encrypted if needed. But I don't really see it as particularly sensitive information. The messages are optional in any event, so if you're worried about leaking the data, you could simply not send it.

proposals/2399-reporting-no-key-sent.md Outdated Show resolved Hide resolved
proposals/2399-reporting-no-key-sent.md Outdated Show resolved Hide resolved
proposals/2399-reporting-no-key-sent.md Outdated Show resolved Hide resolved
proposals/2399-reporting-no-key-sent.md Outdated Show resolved Hide resolved
proposals/2399-reporting-no-key-sent.md Show resolved Hide resolved
@uhoreg uhoreg requested a review from anoadragon453 May 13, 2020
@uhoreg
Copy link
Member Author

@uhoreg uhoreg commented May 13, 2020

Added a section about key sharing.

@richvdh richvdh self-requested a review May 21, 2020
user was not in the room when the message was sent
- `m.unavailable`: sent in reply to a key request if the device that the key
is requested from does not have the requested key
- `m.no_olm`: an olm session could not be established. This may happen, for
Copy link
Member

@richvdh richvdh May 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd have gone with something slightly more generic here, since the problem is a failure to create a peer to peer encrypted channel rather than anything specific to olm (one day people might use another mechanism). Not a strong opinion though.

proposals/2399-reporting-no-key-sent.md Outdated Show resolved Hide resolved
@mscbot
Copy link
Collaborator

@mscbot mscbot commented Jun 8, 2020

🔔 This is now entering its final comment period, as per the review above. 🔔

@mscbot mscbot added final-comment-period and removed proposed-final-comment-period labels Jun 8, 2020
@mscbot
Copy link
Collaborator

@mscbot mscbot commented Jun 13, 2020

The final comment period, with a disposition to merge, as per the review above, is now complete.

@mscbot mscbot added finished-final-comment-period and removed disposition-merge final-comment-period labels Jun 13, 2020
@turt2live turt2live merged commit ffd7391 into matrix-org:master Jun 13, 2020
7 checks passed
@turt2live turt2live added spec-pr-missing and removed finished-final-comment-period labels Jun 13, 2020
@turt2live
Copy link
Member

@turt2live turt2live commented Oct 20, 2020

Spec PR: #2826

@turt2live turt2live added spec-pr-in-review and removed spec-pr-missing labels Oct 20, 2020
@turt2live turt2live added merged and removed spec-pr-in-review labels Nov 5, 2020
@turt2live
Copy link
Member

@turt2live turt2live commented Nov 5, 2020

Merged 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:core merged proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants