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

MSC4019: Encrypted event relationships #4019

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

Conversation

tusooa
Copy link
Contributor

@tusooa tusooa commented May 20, 2023

Signed-off-by: tusooa <[email protected]>
@tusooa tusooa changed the title Encrypted relationships MSC4019: Encrypted relationships May 20, 2023
@tusooa tusooa changed the title MSC4019: Encrypted relationships MSC4019: Encrypted event relationships May 20, 2023
@tusooa tusooa force-pushed the tusooa/encrypted-relationships branch from 6c50199 to 4b7fb4d Compare May 20, 2023 20:33
Signed-off-by: tusooa <[email protected]>
@tusooa tusooa force-pushed the tusooa/encrypted-relationships branch from 4b7fb4d to 6816a05 Compare May 20, 2023 20:35
@tusooa
Copy link
Contributor Author

tusooa commented May 20, 2023

cc matrix-org/matrix-spec#660

@uhoreg uhoreg added proposal A matrix spec change proposal client-server Client-Server API kind:feature MSC for not-core and not-maintenance stuff needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. labels May 21, 2023
Comment on lines +29 to +35
## Potential issues

According to the existing specs, clients might ignore the `m.relates_to` in the encrypted payload of
an event, causing rendering issues for them. Thus, clients that supports `m.room.relationship_encryption`
MAY send the `m.relates_to` in both the cleartext part and the encrypted payload for rooms that has this state event.
Clients that do so SHOULD inform their users of this, and SHOULD allow the users to choose whether they
want to send the `m.relates_to` in the cleartext part.
Copy link
Member

Choose a reason for hiding this comment

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

Obviously, server-side aggregations and the /relationships API will not work any more with this change.

These exist for a reason: clients are heavily reliant on them for things like finding the reactions and threads which relate to a particular event.

It seems to me that these are "Potential issues" which need discussing here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For my use case (1-to-1 chats and small groups of at most 10 people), aggregations from server side just do not help much. Most used relation is reply, second most editing, and tiny little bits of reactions. Reply and editing chains wouldn't be spanned across like 100s of events.

The sole purpose of this is to sacrifice the ability for the server to track relationships in order to achieve better privacy.

Made this explicit in the potential issues section.

Copy link
Contributor

@FSG-Cat FSG-Cat May 22, 2023

Choose a reason for hiding this comment

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

@richvdh A somewhat recent discussion in Client Dev Room revealed no matrix clients use aggregations for Reactions. If a client does nobody was able to present one in that conversation. (Date of Discussion is Feburary 21nd of 2023. According to Central European Summer Time.)

So the concern that Reactions would break is if that conversation was accurate incorrect for most users.

Due to that reactions wont break a MSC like this is safe to move forward for Reactions atleast if that conversation was accurate. As we are trying to prove a negative in this case its up to whoever wants to do the work to find a counterexample.

Edit: lets update this comment. So i dug up that MSC2677 and this specific comment is where the call is made that relations in the case of reactions are not aggregated and this PR to synapse removes the code from synapse in line with the MSC.

Copy link
Member

Choose a reason for hiding this comment

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

that Relations specifically are not aggregated

I think you mean reactions.

Copy link
Member

Choose a reason for hiding this comment

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

You're right, reactions are not aggregated, I'd forgotten. Nevertheless the concern remains around other relation types (especially edits).

Copy link
Contributor

@FSG-Cat FSG-Cat May 23, 2023

Choose a reason for hiding this comment

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

@clokep is correct about what i intended to communicate. And yes its a valid concern to be concerned about other relation types.

Choose a reason for hiding this comment

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

The key field for reactions makes sense to encrypt in private conversations. However, I think that fields denoting the relationship itself should always be unencrypted. It doesn't help with just server aggregation. Bots and clients will have it way easier getting relevant events on demand rather than needing a full sync cached and searched through. It could be made the default and no extra toggle (m.room.relationship_encryption) would be needed; if a room is encrypted, all other relationship fields are stored inside the encrypted content, whereas the event_id and rel_type remain outside, unencrypted.

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 key field for reactions makes sense to encrypt in private conversations. However, I think that fields denoting the relationship itself should always be unencrypted. It doesn't help with just server aggregation. Bots and clients will have it way easier getting relevant events on demand rather than needing a full sync cached and searched through. It could be made the default and no extra toggle (m.room.relationship_encryption) would be needed; if a room is encrypted, all other relationship fields are stored inside the encrypted content, whereas the event_id and rel_type remain outside, unencrypted.

I am talking about the possibility for clients to not rely on servers to keep record of event relationships. And it's optional. Clients that do want to rely on servers can simply not support it.

@tusooa
Copy link
Contributor Author

tusooa commented May 22, 2023

Regarding implementation, libkazv currently sends all outbound relations encrypted in encrypted rooms.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client-server Client-Server API kind:feature MSC for not-core and not-maintenance stuff 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants