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

MSC3996: Encrypted mentions-only rooms. #3996

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

clokep
Copy link
Contributor

@clokep clokep commented Apr 13, 2023


If the decrypted ciphertext contains a `m.has_mentions` property it should be ignored.

## Potential issues
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is missing a section about abuse potential -- what happens if someone just says every message has a mentions on it... wasting the bandwidth, power, and CPU of others in the room?

Copy link
Member

Choose a reason for hiding this comment

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

It would be nice if clients could surface this field being set (to moderators/admins only?) so the abuse wouldn't be so invisible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The field is unencrypted so yeah I agree -- this is mostly up to clients or moderation tooling exposing some aggregate information on this? I think I just need to add some text. 😄

@clokep clokep marked this pull request as ready for review April 13, 2023 18:16
@turt2live turt2live added push e2e proposal A matrix spec change proposal client-server Client-Server API kind:maintenance MSC which clarifies/updates existing spec needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. labels Apr 13, 2023
Copy link
Member

@anoadragon453 anoadragon453 left a comment

Choose a reason for hiding this comment

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

This seems like a happy medium between metadata privacy and resource usage, and a step up over today's situation.

proposals/3996-encrypted-mentions-only-rooms.md Outdated Show resolved Hide resolved
proposals/3996-encrypted-mentions-only-rooms.md Outdated Show resolved Hide resolved

If the decrypted ciphertext contains a `m.has_mentions` property it should be ignored.

## Potential issues
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice if clients could surface this field being set (to moderators/admins only?) so the abuse wouldn't be so invisible.

clokep and others added 2 commits April 20, 2023 12:00
Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>
Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>
"value": "m.room.encrypted"
}
],
"actions": ["notify"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@giomfo had wondered if a "push, but don't increase unread count" action would be more appropriate here. I think that we don't need that here, but it could be useful?

I'd be curious of thoughts from other client developers!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@erikjohnston said on MSC3952:

I think its worth noting that this means that servers (still) won't be able to differentiate between notifications and mentions. This mostly affects how often we push to devices, but also means that we can't make /notifications?only=highlight API be useful again

I think this applies to this conversation as well. If we added a highlight tweak to this rule then homeservers could push the event (and return it it as part of /notifications?only=highlight) and allow clients to reparse push rules and either downgrade the highlight -> notifications (thus ignoring the event).

This would potentially mean that /notifications?only=highlight would return a lot of not-useful data, but it could be interested to consider.

The other potential solution here is the ideas in element-hq/element-web#6874.

@@ -0,0 +1,193 @@
# MSC3996: Encrypted mentions-only rooms
Copy link
Member

@kegsay kegsay May 23, 2023

Choose a reason for hiding this comment

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

I like this proposal overall, but it isn't clear that this is the right tradeoff in terms of privacy. Earlier proposals for putting cleartext user IDs were rejected, so what amount of metadata leakage is acceptable?

  • Cleartext user IDs. Rejected.
  • No metadata leakage at all ever.
  • This proposal -> Boolean flag to indicate some kind of @ mention. This leaks the same as cleartext user IDs in the case of a 1:1 DM.
  • Some kind of K-Anonymity ala haveibeenpwned.com which leaks to the server that a group of users should be notified. Leaks that one of that group (or potentially multiple) have been @ mentioned.
  • The receiver decrypts and then "bookmarks" the event if it is a direct @ mention. This may help to cut down the number of unnecessary operations as each device doesn't need to repeat work, though it remains O(n) where n=number of devices for that user. This leaks which messages are @ mentions to the user's server if this bookmarking is automatic and in the clear, whilst leaking no metadata to federated servers.
  • Something else entirely?

Until we get clarity on the table stakes for metadata leakage, I can't in good faith say this is the best proposal. I am personally of the opinion that we should be leaking metadata to provide a better UX, as the inability to reliably present @ mentions in encrypted rooms has significant effects on daily users e.g causing people to immediately action those messages just in case they get lost in scrollback and are then unfindable. This also has negative effects on users who have a 2nd+ account which they use infrequently and just want to check it occasionally to see who has @ mentioned them - the current UX is incomplete due to the inability to accurately go through E2EE rooms.

We already leak a lot of metadata today, but proposals like MSC4014: Pseudonymous Identities would go some way to help obscure this metadata by providing a layer of indirection on events (you would be @ mentioning epehemeral per-room per-user keys, not static user IDs).

@@ -0,0 +1,193 @@
# MSC3996: Encrypted mentions-only rooms
Copy link
Member

@ara4n ara4n Jun 6, 2023

Choose a reason for hiding this comment

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

Something feels wrong here. As per #3952 (comment), i believe we have no choice but for clients to receive all E2EE events (and so get pushed for them). They need this to:

  • Accurately display unread state (given they don't know what the event type is of events until they've decrypted them, and so whether that event is displayable)
  • Check for keyword mentions
  • Normal mentions (modulo this MSC)
  • Generate FTS indexes.

The fact that current clients don't do this for E2EE rooms is very much a bug (and means that you can't trust unread count or highlight counts in E2EE rooms).

Yes, this means that bandwidth is O(N) with the number of E2EE messages, but this is acceptable because you can spider the rooms in the background to search for lost mentions (and index for FTS) - so it doesn't have to slow down the UX of app. Also, the volume of E2EE rooms is typically much lower and more bounded than large spammy unencrypted public chatrooms. Empirically, on a poweruser account like mine, being pushed in the bg for every E2EE message seems acceptable in terms of battery/bandwidth use - and can have the advantage of using the pushes to prepopulate the local storage cache on the client so up-to-date history appears instantly when you open the app.

It's also how Signal, WhatsApp(?) and Threema work - and it's very hard to justify why we would expose more metadata on the server for E2EE messages (either via this or MSC3952) given other apps give an acceptable UX without doing so.

So: I believe this MSC is premature optimisation and needlessly leaks metadata. Instead, we should push the clients for all E2EE (given they need it to operate anyway), and if that needs to be optimised, only then should we give the option of sacrificing metadata privacy (and keyword mentions) if that's unacceptable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(This was mentioned in a private channel, but I'll mention it here for transparency since I did a poor job of giving context on why this MSC exists.)

This MSC was written to formalize the idea of whether it would make sense to expose any additional metadata (the bare minimum). The trade-offs might not make sense, but I wanted to remove that conversation from MSC3952 to avoid blocking that MSC further. Thanks for starting the conversation here! 👍

Currently I have no plans to implement this MSC, as you said it has downsides of more metadata leakage and doesn't necessarily give us all the features we want.

I was hoping those who were advocating for exposing all the mentions information in MSC3952 might be part of the conversation here, but so far this MSC has generated little interest.

In the meantime we're exploring other ways to properly push all E2EE messages while accounting for rooms that are muted or set to mentions & keywords only, etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

I finally got some time to actually read through this and understand what it's saying. I think part of why I at least haven't chimed in is that the MSC is somewhat confusingly named. I think that "Unencrypted Mention Indicator in Encrypted Rooms" or something would be more clear as to what this is about.

As for what Beeper is planning on doing to resolve the concerns we had with MSC3952, we are just putting the mentions in plaintext anyway. (We are using the presence of that field to determine whether or not the intentional mentions code path should be used to evaluate push rules.) Currently, only our bridges are utilizing this MSC to ensure that our users get notifications for mentions from other networks.

Example of what we've done in mautrix-go: https://github.com/mautrix/go/blob/3e840e962e24e90a439f91ce0cdf5e7251008413/crypto/encryptmegolm.go#LL133-L135. Whether or not to include the mentions in plaintext is configurable. We have that enabled for at least the whatsapp bridge right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think part of why I at least haven't chimed in is that the MSC is somewhat confusingly named. I think that "Unencrypted Mention Indicator in Encrypted Rooms" or something would be more clear as to what this is about.

I suppose it should really be "Supporting mentions-only encrypted room" or something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client-server Client-Server API e2e kind:maintenance MSC which clarifies/updates existing spec needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. proposal A matrix spec change proposal push
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants