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

MSC3779: "Owned" State Events #3779

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

Conversation

andybalaam
Copy link
Contributor

@andybalaam andybalaam commented Apr 20, 2022

Rendered

Allow normal users to create state events if the state_key is prefixed with their MXID.

Written after discussions with @ara4n and @dbkr

@andybalaam andybalaam changed the title MSC0000: "Owned" State Events MSC3779: "Owned" State Events Apr 20, 2022
@uhoreg uhoreg added requires-room-version An idea which will require a bump in room version proposal A matrix spec change proposal 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 Apr 20, 2022
@andybalaam andybalaam marked this pull request as ready for review May 3, 2022 12:16
@progval

This comment was marked as duplicate.


By restricting these events to those with a state_key unique to the sender, we prevent unauthorised users from modifying room topics or other similar state, and we make it clear who is responsible for any such state.

If an individual user is sending too many state events, the server should apply rate limiting.
Copy link

Choose a reason for hiding this comment

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

Sending many state events will still impact the current state of the room, which comes during an initial sync.

Also an initial sync will contains state event like those one, which can be pretty outdated, to take the example of an old location sharing. Has this been considered?

(my feeling is like it look a bit of a hack to use the state of a room to send data such as "user location sharing state" to the room)

Copy link
Contributor Author

@andybalaam andybalaam May 19, 2022

Choose a reason for hiding this comment

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

Sending many state events will still impact the current state of the room, which comes during an initial sync.
Also an initial sync will contains state event like those one, which can be pretty outdated, to take the example of an old location sharing. Has this been considered?

Yes, this was considered, and the I think the right answer is to allow deleting state. It is already possible for a user with sufficient permissions to create huge numbers of state events, so this problem already exists to some extent. I tried to call out this problem in 8d68c29.

Copy link
Member

Choose a reason for hiding this comment

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

Deleting state events is pretty much science fiction at this point (as far as I'm aware) and it's not clear how and whether it can be done safely. So I'm not sure it's a good idea if we have to rely on it ever happening.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(my feeling is like it look a bit of a hack to use the state of a room to send data such as "user location sharing state" to the room)

I initially felt similarly but was convinced by conversations with others that this is exactly what state is for: things that are true now, and you shouldn't need to go searching in the timeline to find out about.

We shouldn't let the lack of state deletion bias us away from using state as it was intended: instead we should implement state deletion!

Copy link
Member

Choose a reason for hiding this comment

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

state deletion is not particularly scifi; we should just get on and do it. for instance, hiding deleted state events from the CS API would be trivial.

Copy link
Member

Choose a reason for hiding this comment

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

Is there a document detailing this somewhere? Depending on what exactly we mean by state deletion and how it's implemented, I can envision iffy interactions with state resolution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Somewhat late, there is a document! #3901

Copy link
Member

Choose a reason for hiding this comment

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

It is already possible for a user with sufficient permissions to create huge numbers of state events, so this problem already exists to some extent.

While this is true, there is a pretty big difference between allowing moderators to attack a room and allowing anyone who has joined to do so.

Yes, this was considered, and the I think the right answer is to allow deleting state.

I think it's worth noting that this is a recovery mechanism, rather than something that prevents an attack. For state deletion which is purely CS API side this will still affect federated joins (and DB usage, and state resolution perf).

My main concern is that since these state events will basically be invisible to clients, so even if the room has an active moderator who knows the dangers and how to deal with it, the moderator wouldn't even notice it happening. This would allow a malicious client to slowly create an unbounded number of state events in the room, which would a) make syncs very very slow even with lazy loading, and b) make federated joins incredibly slow, and possibly wouldn't ever complete with enough state events. The latter would not be fixable even with deleting of state over CS API.

Copy link
Member

Choose a reason for hiding this comment

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

Would MSC3757 be a preferred alternative, then? It aims to simply restrict who can overwrite an "owned" state event, meaning it still provides protection against users writing arbitrary/unspecced state events via required PLs for event types.


## Proposal

We propose allowing unprivileged users to create "owned" state events, where the `state_key` equals their MXID, or starts with their MXID plus underscore.
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 could probably get behind @toger5 's suggestion to make state_key an array instead of requiring parsing it.

Copy link
Member

Choose a reason for hiding this comment

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

I have to disagree with @toger5 on the array solution. Someone has brought this up in the past, but it seems like this would require a monumental effort from every Matrix client/server/app in the ecosystem to support the new room version:

  • Currently, room state is a replicated data structure that behaves like Map<EventType, Map<Key, Object>>. To my knowledge this is roughly how all Matrix SDKs store it internally.
  • But this data structure isn't just an implementation detail of SDKs. Quite often, it becomes part of their public APIs. All kinds of application code gets built on top of "Matrix room state" as a data structure.
  • If we introduce subkeys, then every implementation has to swap the data structure out for something more like Map<EventType, Map<Array<Key>, Object>>. (In languages such as JS this gets even more complicated, because arrays can't be used as referentially transparent map keys.) Potentially everything up to application code has to be adapted to this change—even a good number of "trivial" Matrix bots would stop working in the new room version.

Overall, this feels like too little benefit for such a difficult transition path. The key space for room state is already infinite, so we don't really benefit from having "more" state keys to play with either.

As for what suggestion I would make, if the issue is that:

  1. We don't want Matrix to assign semantics to state keys outside of the m.* event type namespace, at least by default
  2. We don't want members to be able to claim ownership of arbitrary state keys and "squat" them

Then maybe we can combine the "magic state key" approach and the ownership field approach into a solution where:

  • State keys with ownership semantics are always namespaced
  • A series of "opt-in" actions have to be taken before ownership semantics will actually apply to a given state key. For example, a room admin has to specify in the power levels which state event types may use ownership semantics, and then the member posting the state event has to set the ownership field and use a namespaced state key.
  • Certain event types in the m.* namespace, such as MatrixRTC events, could be specified to use ownership semantics by default in a new room version

Or perhaps, an even more minimal solution would get rid of the namespacing, and just rely on opting-in to ownership for an event type to mitigate squatting. Then it would be up to the specifications of each event type with ownership semantics to say how they avoid letting the application get DOS'd by squatters.

Copy link
Member

Choose a reason for hiding this comment

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

For example, a room admin has to specify in the power levels which state event types may use ownership semantics

I would advise against this specific approach as it sounds like it would be incredibly brittle in the face of state resets. Imagine if a state reset chooses the "wrong" power level and now suddenly a large subset of events lose ownership semantics.

> #### Required Power Level
> A given event type has an associated *required power level*. This is given by the current `m.room.power_levels` event. The event type is either listed explicitly in the events section or given by either `state_default` **(for non-owned state events)** or `events_default` **(for message events and owned state events)**. **Owned state events are events with a `state_key` that equals the sender's MXID, or starts with the sender's MXID plus an underscore.**

Additionally, to improve clarity, we propose that the relevant section be explicitly mentioned in the room version definition, section "3.4 Authorisation rules". Rule 7 should be changed to read:
Copy link
Member

Choose a reason for hiding this comment

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

Additionally, Rule 8 should be removed:

If the event has a state_key that starts with an @ and does not match the sender, reject.

...as that would override Rule 7 by rejecting all owned state events with a state key of <sender>_<something>.

Copy link
Member

Choose a reason for hiding this comment

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

Scratch that -- Rule 8 is still needed for preventing users from setting state they don't own.

It could be reworded as such:

If the event has a state_key that starts with an @ and is not an owned state event (see "m.room.power_levels" in the Room Events section of the Client-Server API)", reject.

Or if we want to re-state the definition of an owned state event instead of referencing it:

If the event has a state_key that starts with an @ and does not match the sender or start with the sender plus an underscore, reject.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 requires-room-version An idea which will require a bump in room version
Projects
None yet
Development

Successfully merging this pull request may close these issues.