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.

Sign up for GitHub

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

MSC3760: State sub-keys #3760

Closed
wants to merge 1 commit into from
Closed

MSC3760: State sub-keys #3760

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 74 additions & 0 deletions proposals/3760-state-sub-keys.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
# MSC3760: State sub-keys
Copy link
Contributor

Choose a reason for hiding this comment

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

One thing I want to see is how much change this requires on the API level to query, filter, and fetch specific state with a specific subkey.

Currently matrix is indexing state events with a “tuple” of (type, key), this might change it to (type, key, subkey) semantically, but with most apis (and implementations) being geared towards the older model.

This MSC may need to detail the necessary API changes required to properly address state events with a subkey.


Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of this MSC overall because it breaks client data structures which have always assumed replace semantics are (event type, state key) - this PR introduces a third value to the tuple. This breaks almost everything: server databases won't have an index for the subkey, and existing code which expects to pull out a single event for a (type, state key) will break.

This will require major changes on both clients and servers, which is a lot to ask for just for live location sharing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe as a middle-ground solution between this and MSC3757, state_key could be allowed to be an array of strings? That should slightly lower the amount of breakage while still maintaining the advantages of this proposal.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe changing that to an array would be even a bigger change, and makes a type variable in a schema (either string or array), which is... fairly difficult to correctly deserialize, especially in Rust/Ruma/Serde.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's definitely simple enough to deserialize in Rust / serde.

## Problem

Currently there are three main ways to limit who can overwrite a state event:

* If a user's PL is greater than the `m.room.power_levels` `state_default` field
* If a user's PL is greater than the `m.room.power_levels` `events` field for that event type
* If a state event has a state key which begins with an `@`, then the sender's mxid must match that state key.

This is problematic if an unprivileged user needs to publish multiple state events of the same type in a room, but would like to set access control so that only they can subsequently update the event. An example of this is if a user wishes to publish multiple live location share beacons as per [MSC3489](https://github.com/matrix-org/matrix-spec-proposals/pull/3489) and [MSC3672](https://github.com/matrix-org/matrix-spec-proposals/pull/3672). They will typically not want other users in the room to be able to overwrite the state event, so we need a mechanism to prevent other peers from doing so.

## Proposal

We propose adding an optional `state_subkey` top-level field that allows multiple state events to exist which all have the same `state_key` but represent different pieces of state.

This means that the existing rules around access control to state events can be unchanged, but now one user can own multiple pieces of state that have the same `type`.

For example, to post a live location sharing beacon from [MSC3672](https://github.com/matrix-org/matrix-spec-proposals/pull/3672):

```json=
{
"type": "m.beacon_info",
"state_key": "@stefan:matrix.org", // Ensures the sender has control
"state_subkey": "asset1", // Does not clash with other events
"content": {
"m.beacon_info": {
"description": "Stefan's live location",
"timeout": 600000,
"live": true
},
"m.ts": 1436829458432,
"m.asset": {
"type": "m.self.live"
}
}
}
```

State events that are missing `state_subkey` should behave exactly as if `state_subkey` had a value of `""`.

Two state events with identical `type` and `state_key` should be treated as independent if they have different values of `state_subkey`.
Copy link
Member

Choose a reason for hiding this comment

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

Since this complicates things, perhaps it would make sense to also spell it out in terms of operational semantics. That is, to say that before this change, state events behaved like an assignment of the form

room_state[type, key] = content

And this MSC changes it to

room_state[type, key, subkey] = content

And that a missing subkey is equivalent to

room_state[type, key, ""] = content


Copy link
Member Author

Choose a reason for hiding this comment

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

Potential implementation strategy: it's possible a server implementation should implement this by storing both state_key and state_subkey in a (large enough) single database column, concatenated together, separated by underscore. This might reduce the need to update existing code, and underscore is safe to use since it is not allowed in a domain part (see MSC3757).

## 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.

This change isn't backwards compatible for clients which aren't updated as frequently. Not sure how much of a problem it is necessarily, though as a general case we aim to keep those clients functional to the best of our ability (notable exceptions being introducing new join_rules, but not taking away the join_rule field).


This change involves modifying the way the keys in a room's state map are constructed, so may be complex to implement.

This will require a new room version.

## Alternatives

We could add a suffix to the `event_type`, as originally proposed in [MSC3489](https://github.com/matrix-org/matrix-spec-proposals/pull/3489). This can make life difficult for clients who expect a fixed type, and it makes it hard to control power levels for event types.

We could stuff the mxid and subtype into the `state_key` field as proposed in [MSC3757](https://github.com/matrix-org/matrix-spec-proposals/pull/3757). This would involve changing the access control logic for state events, and objections were raised about the need to parse the state key's contents.

We could allow additional events that set up access control on a state event.

We could add a flag on the contents of the event (as originally proposed in [MSC3757](https://github.com/matrix-org/matrix-spec-proposals/pull/3757)), called e.g. `m.peer_unwritable: true` to say other users were prohibited from overwriting the event. However, this is impractical because there isn't a good value for the state_key, which needs to be unique and not subject to races from other malicious users.

## Security considerations

The rules on who can modify a state event are unchanged by this proposal, which should simplify security concerns relative to the alternatives.
Copy link

Choose a reason for hiding this comment

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

Here's one small change that might increase the impact of this proposal, at the cost of a little bit of extra implementation complexity. Why not add the restriction that "If a state event has a state subkey which begins with an @, then the sender's mxid must match that state subkey."?

This would make it possible to use state subkeys for access control with state events whose state key already has a meaning - for example m.space.child. And that would solve some in-the-wild access control problems with state updates.

Copy link

@gleachkr gleachkr Apr 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

OK, after some discussion, I'm not sure the above is right. It's not clear that it would actually be possible to add access controls to existing state event types this way without further MSCs. For example, it's not transparent what it would mean to have multiple copies of m.space.child for a single room, differentiated by their subkeys, and dealing with this scenario could mean extra work for server implementations to rejigger the hierarchy endpoint.

More generally, this proposal seems to entail that you could legally have multiple copies of something like m.room.canonical_alias, which all have an empty string as key, but which have different subkeys. Probably it's obvious that in this case the state events with subkeys should be ignored, but maybe it'd be good to make this explicit, and also to think through what changes would need to be made across the client ecosystem to implement this behavior? (or maybe that's not a worry because of the room-version bump?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not add the restriction that "If a state event has a state subkey which begins with an @, then the sender's mxid must match that state subkey."?

We'll also have to decide how to handle the case were state key and subkey contain two different MXIDs then.

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, it's not transparent what it would mean to have multiple copies of m.space.child for a single room, differentiated by their subkeys, and dealing with this scenario could mean extra work for server implementations to rejigger the hierarchy endpoint.

Emphasized part is what really complicates this idea from my POV. It's kind of obvious to me that any references to a particular state event type in older MSCs need to be taken as talking about the case where state_subkey == "", but you could still add new meaning to an m.space.child with a non-nil state_subkey client-side.

But in this case this doesn't help you very much, as the usefulness of this lies primarily in having a functioning /hierarchy endpoint. And for that you need server support. Bummer.

Copy link

Choose a reason for hiding this comment

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

And in having other clients acknowledge your space events, which you'd lose as well.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, but that could eventually be spec'ed as normative client behaviour and would be relatively easy and straightforward to do. But making /hierarchy work needs significant design.

I also just had a random interesting thought: We currently put the intended interpretation of events into the C2S API spec, but that's not really correct. In the future we could imagine having clients speaking the S2S protocol directly without even implementing C2S (not even internally, like with current client + embedded Dendrite plans). It seems there should really be a separate spec specifying the intended meaning of events.


Server implementors will need to ensure that the state_subkey has no effect on the access control of state events.

This will require a new room version, meaning it will not affect old events.

## Unstable prefix

* `state_subkey` should be referred to as `org.matrix.mscxxxx.state_subkey` until this MSC lands.
Copy link
Contributor

Choose a reason for hiding this comment

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

At risk of immutably requiring servers to be backwards compatible with this for any stable room version, I suggest adding a qualifier that this event-level key should only appear on rooms versions which experiment with this.


## Dependencies

None