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 A matrix spec change proposal label Jan 2, 2020
@turt2live turt2live self-requested a review January 9, 2020 16:06
@ara4n
Copy link
Member

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

Choose a reason for hiding this comment

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

seems sane

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

uhoreg commented Mar 19, 2020

This has been implemented and seems to be working.

@uhoreg uhoreg marked this pull request as ready for review March 19, 2020 21:13
@turt2live turt2live added the kind:core MSC which is critical to the protocol's success label Apr 20, 2020
@uhoreg
Copy link
Member Author

uhoreg commented Apr 22, 2020

@mscbot fcp merge

@mscbot
Copy link
Collaborator

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 Currently awaiting signoff of a majority of team members in order to enter the final comment period. labels Apr 22, 2020
@uhoreg
Copy link
Member Author

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 20:05
@uhoreg
Copy link
Member Author

uhoreg commented May 13, 2020

Added a section about key sharing.

@richvdh richvdh self-requested a review May 21, 2020 12:49
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

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 commented Jun 8, 2020

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

@mscbot mscbot added final-comment-period This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. and removed proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. labels Jun 8, 2020
@mscbot
Copy link
Collaborator

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 This MSC has entered a final comment period in interest to approval, postpone, or delete in 5 days. labels Jun 13, 2020
@turt2live turt2live merged commit ffd7391 into matrix-org:master Jun 13, 2020
@turt2live turt2live added spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec and removed finished-final-comment-period labels Jun 13, 2020
@turt2live
Copy link
Member

Spec PR: #2826

@turt2live turt2live added spec-pr-in-review A proposal which has been PR'd against the spec and is in review and removed spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec labels Oct 20, 2020
@turt2live turt2live added merged A proposal whose PR has merged into the spec! and removed spec-pr-in-review A proposal which has been PR'd against the spec and is in review labels Nov 5, 2020
@turt2live
Copy link
Member

Merged 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:core MSC which is critical to the protocol's success merged A proposal whose PR has merged into the spec! proposal A matrix spec change proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants