-
Notifications
You must be signed in to change notification settings - Fork 378
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
MSC3757: Restricting who can overwrite a state event #3757
base: main
Are you sure you want to change the base?
Conversation
This comment was marked as duplicate.
This comment was marked as duplicate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One nit, else this looks sound.
Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>
Co-authored-by: Travis Ralston <travisr@matrix.org>
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this a confusing way of phrasing the current restrictions.
I'd say that there are essentially two current restrictions, imposed by rules 7 and 8 in the auth rules.
In short:
- The user's PL must not be less than the "required power level" for that event.
- In addition, if a state event has a state key which begins with an
@
, then the sender's mxid must match that state key.
The spec for m.room.power_levels
defines required power level, but in summary, it is determined by the setting in m.room.power_levels
events
field for that event type if present; otherwise, it is determined by the state_default
field.
@@ -0,0 +1,118 @@ | |||
# MSC3757: Restricting who can overwrite a state event. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is actually derestricting who can overwrite a state event.
# MSC3757: Restricting who can overwrite a state event. | |
# MSC3757: Relaxing the restrictions on who can overwrite a state event. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true for MSC3779, but not as much for this one; the former bypasses PL restrictions for setting state that belongs to the sender (where "belongs to" = the state key starts with the sender's MXID), but this MSC does not.
The restriction proposed by this MSC is to prevent state that belongs to a particular user from being overwritten by other, equally-powerful users.
The only PL restriction that's relaxed by this MSC is for allowing more powerful users to overwrite state that doesn't belong to them, for the sake of having a tool against state graffiti.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. Still, I find the title a bit confusing. How about:
# MSC3757: Restricting who can overwrite a state event. | |
# MSC3757: Extending the set of write-protected state keys |
or something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH I informally refer to this MSC as "the owned state MSC" despite that being the title of MSC3779 😉
Since one of the distinguishing differences of this MSC over 3779 is the ability for admins to manage others' state, maybe we could call it
# MSC3757: Restricting who can overwrite a state event. | |
# MSC3757: State event ownership (with admin management) |
?
Practically speaking, this means modifying the [authorization rules](https://spec.matrix.org/v1.2/rooms/v9/#authorization-rules) such that rule 8: | ||
|
||
> If the event has a `state_key` that starts with an `@` and does not match the `sender`, reject. | ||
|
||
becomes: | ||
|
||
> If the event has a `state_key` that starts with an `@`, and the substring before the first `_` that follows the first `:` (or end of string) does not match the `sender`, reject - unless the sender's powerlevel is greater than the event type's *required power level*. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last time we changed the auth rules, there was a lot of discussion around "let's never do this again without test vectors for the auth rules" (cf matrix-org/matrix-spec#1710, matrix-org/matrix-spec#1642).
The apparent confusion over what the current auth rules mean, and what they should say after the change, makes me think that we should start some progress in that direction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Complement tests should be a step in the right direction.
## Potential issues | ||
|
||
None yet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remain concerned that this MSC opens the door to a proliferation of state events that we cannot delete, and that faster-joins can't help with. I would like to see some discussion of this in the MSC.
Admittedly, we have this problem to some extent today, in that there is nothing stopping users creating lots of state events with different types, but:
- (a) such behaviour is currently restricted to room moderators, because it relies on their power level being
state_default
or higher. This MSC would change that, by allowing any user to (say) create millions ofm.beacon_info
events. - (b) there is currently no specced usecase that requires multiple state events per user, and introducing one feels like a step-change. Arguably that is not a problem of this MSC per se (and rather MSCs that make use of it such as MSC3672), but I think we'll need to at least set some expectations here.
This is not a veto from me, but I do think we need to consider this properly and at least set out guidelines for future MSCs that build on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- (a) ... This MSC would change that, by allowing any user to (say) create millions of
m.beacon_info
events.
This is what MSC3779 proposes, not this one.
To clarify, the effective auth rule changes proposed by this MSC are to:
- grant write access on state events with state keys of the form
<sender>_<anySuffix>
to all users- N.B.
<sender>
is the MXID of the user sending the state event
- N.B.
- grant write access on state events with state keys of the form
<mxid>
&<mxid>_<anySuffix>
to users whose PL is greater than that of<mxid>
's PL- N.B.
<mxid>
is any MXID
- N.B.
The event-sending restrictions imposed by m.room.power_levels
remain in effect.
What this MSC does allow is creating lots of state events with different state key suffixes -- but that would be limited to room moderators, just like how creating lots of state events with different types currently is.
- (b) there is currently no specced usecase that requires multiple state events per user, and introducing one feels like a step-change.
As a start, MSC4143 makes an explicit mention to this one (namely, in the section regarding MatrixRTC room state), and is what has driven interest in this MSC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mscbot concern Does not work with all MXIDs
@mscbot concern Does not work with all MXIDs Reposting due to matrix-org/mscbot-python#26 not yet being deployed. |
} | ||
``` | ||
|
||
Since `:` is not permitted in the localpart and `_` is not permitted in the domain part of an mxid (see [Historical User IDs](https://spec.matrix.org/v1.2/appendices/#historical-user-ids)), it is not possible to craft an mxid that matches the beginning of a state key constructed for another user's mxid, so state keys restricted to one owner can never be overwritten by another user. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While the spec specifies a grammar for server names, are we SURE that servers verify that grammar? While officially underscores are invalid in domain names and you can't order a TLS certificate for it nowadays, such domains still exist in the wild and wildcard certificates even allow using TLS with them. For example: https://my_sarisari_store.typepad.com/
I think relying on underscores as a separator is a rather scary trap and I wouldn't bet on all currently developed Matrix servers rejecting such server names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ac24510 adds this to the "Potential issues" section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about using @
as the separator? Since the separator will never be the first character in the state key, it shouldn't be possible to confuse it with the user ID sigil.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
could also be a good separator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick process note: there appear to be several lengthy comments not contained to threads on this MSC. I haven't reviewed those because github makes it difficult to track replies - please move them to the MSC text or into a thread for better visibility.
On the Apache voting scale, I'm at a -1.0 for how the MSC is currently written. A stronger rationale for why this is needed now and strong discounting of a top-level ACL structure would change this to a -0.9 (sorry, I'm just not in favour of string packing here).
This is problematic if a 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), for instance one per device. 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MSC3489 and MSC3672 are not prioritized for inclusion right now, which raises questions about why this MSC is put up for FCP if we're not going to use it for a while. A question around whether the implementation is suitably deployed also comes to mind, though I'm not certain enough to raise that as a FCP-blocking concern.
An update to the introduction to better list out all the features which benefit from this MSC feels needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll let @AndrewFerr talk more about his use case, but broadly I think its for per-device call state within a room.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its for per-device call state within a room.
That's correct; more precisely, it's for per-device call membership state (as a user can join a call from multiple devices at once).
Currently, the call membership state is an array, with one element per participating device. The problem is that updating that state array is prone to race conditions, as adding/removing an entry to the array is dependent on the current value of the array.
This MSC allows for using one state event per device, which each device can freely update without risk of any other device (or user, for that matter) overwriting it at the same time.
## Proposal | ||
|
||
Therefore, we need a different way to state that a given state event may only | ||
be written by its owner. **We propose that if a state event's state_key *starts with* a matrix ID (followed by an underscore), only the sender with that matrix ID (or higher PL users) can set the state event.** This is an extension of the current behaviour where state events may be overwritten only if the sender's mxid *exactly equals* the state_key. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Proposal language should generally be assertive: "The state_key restrictions are changed to..." rather than "The state key should be fixed". This would also resolve a question about who 'we' is.
(please also limit lines to ~100 characters for easier review)
|
||
* 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find the @
prefix restriction a bit questionable, for the record. The only specified state event which uses it is m.room.member
, which the auth rules validate state_key
well before the restriction is checked. Non-spec usage of the state key already applies tricks like prefixing user IDs with _
to ensure they aren't hit by the restriction, and I'm not really convinced that location sharing/beacons need to pack a user ID into the state key anyways (see https://github.com/matrix-org/matrix-spec-proposals/pull/3757/files#r1103877363 ).
This has me leaning towards removing the @
prefix restriction entirely, but I recognize that's not exactly a helpful opinion for this MSC to tackle. Creating stronger arguments for why the dependent features require the user ID in the state key I think would help, though.
|
||
## Security considerations | ||
|
||
This change requires a new room version, so will not affect old events. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mention of requiring a new room version should be at the top of the Proposal section. How I've done this in the past is literally:
In a future room version...
[actual proposal]
@mscbot concern Introduction does not sufficiently list benefiting MSCs/features. |
Complement tests are added by matrix-org/complement#733. |
There've been some issues/concerns raised about the state key string-packing:
Adding a top-level event field to signify the "owner" of an event would help solve these issues, and is akin to the idea of state sub-keys proposed by MSC3760. However, that approach was dismissed by https://github.com/matrix-org/matrix-spec-proposals/pull/3757/files#r848454293 & #3757 (comment) due to the friction of redefining state events & state resolution. So it seems like either option (string-packed state keys vs a top-level owner field) has problems. Should the goal be to decide which option is less problematic, or to arrive at a compromise / a different option? |
- Recommend version 10 as it is the current default - Allow higher versions as this MSC isn't tied to any single version
Also add sub-headers for each issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use cases need more fleshing out, particularly to justify why arbitrary bytes as a suffix are a good idea (they likely aren't). Assuming the reasons are "flexibility", then at the very least we should be aggressively restricting which arbitrary bytes we allow, e.g [a-z0-9]
.
## Proposal | ||
|
||
Therefore, we need a different way to state that a given state event may only | ||
be written by its owner. **We propose that if a state event's `state_key` *starts with* a matrix ID (followed by an underscore), only the sender with that matrix ID (or higher PL users) can set the state event.** This is an extension of the current behaviour where state events may be overwritten only if the sender's mxid *exactly equals* the `state_key`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it valid to have an empty string as the suffix? E.g @kegan:matrix.org_
? What about having arbitrary unicode characters? Null bytes? Please let's be sensible and force sensible restrictions on this new user-defined variable, lest we end up in another hell of poor validation causing problems for clients/servers.
I would suggest a very strong restriction of [a-z0-9]
unless there are good reasons to allow other characters (there almost certainly are not).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Allowing an empty suffix probably won't hurt. I can imagine cases of the suffix being a property that may take on an empty/nullish value.
For a device ID to be usable as a suffix, the suffix charset must include all characters that can be used as a device ID. Unfortunately, there's no specced device ID charset that I can find, and in practice it is quite broad:
- Device IDs generated by QR code logins (at least on Element X Android) can contain
/
and+
. /_matrix/client/v3/login
allows setting a custom device ID of any string, and the Synapse implementation allows all sorts of characters (and lets me set a device ID ofa b ?!@#$%^&*()-=_+/
).
So if we want device ID suffixes now, maybe defining a suffix charset is premature, unless this MSC also defines a stricter device ID charset. But that raises the issue of what to with existing device IDs that don't follow the charset...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is surely too literal though. You can map device IDs deterministically to a valid character set, at its most basic SHA256(device_id).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good idea. I was worried that the input size of device IDs would be too large to safely hash, but the point is to avoid hash collisions, the chance of which should still be small.
Then, this MSC can do away with trying to define a suffix length / charset that can fit a raw device ID.
I will still propose a few non-word characters to be in the suffix charset, because it may be handy to have a suffix containing multiple properties that are easy to separate with a non-word character (eg. @user:hs_prop1_prop2_prop3
). The spec already uses a charset of [0-9a-zA-Z.=_-]
for email login secrets/tokens, and would be a decent charset to use here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thought, a handy consequence of not restricting the suffix charset is the ability to prefix any existing state key with a user's MXID to scope its ownership to that user. With a restricted suffix charset, there may be some state keys that would be invalid as a suffix.
IMO as long as the charset of state keys in general is left unrestricted, there's little benefit in restricting the suffix charset, unless doing so is a step towards restricting the general state key charset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that there's no point in restricting these specific state keys when all other state keys are still arbitrary strings. To take advantage of this MSC, you must give users permission to send state events of certain types. That means users will be able to send unprefixed state keys too, which will not have any character set restrictions.
Restricting state keys in general might be a good idea to do in combination with MSC2828
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't agree with the attitude of "we don't have good validation, so let's not add validation". There's zero reason to punt this down the line to another MSC. It's absolutely trivial to expand the character set or length limits in another MSC. It's very difficult to restrict it once there are client implementations in the wild relying on there being no restrictions. Validation is important to reduce the attack surface of any newly added features. The suffix string will be stored in new places where the state key is not (e.g DBs will likely either have this as a column, or indexed in such a way to allow efficient ordered lookups), which means there will be new code written to read this input. Validate the input, please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the intent of a restricted suffix charset is to ease parsing out the user ID prefix, then the usefulness of the restriction might be limited by how the prefix must be able to contain any printable ASCII character, due to having to support historical user IDs. Also, since the parsing is concerned more with the user ID prefix than with the suffix, locking down the suffix charset might not impact much.
Besides, the "suffix string" isn't really meant to be a new kind of syntax for state keys, but is just a result of the semantic of user-scoped state keys via string packing. It's perhaps better to think of a scoped state key not as a user ID + a suffix, but as an ordinary state key prefixed by a user ID; or rather, that this MSC proposes allowing state keys to optionally be prefixed by a user ID. Being prefixed doesn't change the nature of the content of the rest of the state key, but applying different restrictions to prefixed & unprefixed state keys would imply that it does.
Though if restricting the charset is non-negotiable, then maybe a compromise is to apply a broad charset, like all printable ASCII characters.
Link to the MSC: matrix-org/matrix-spec-proposals#3757 --------- Co-authored-by: Quentin Gliech <quenting@element.io>
- Update auth rules to enforce the suffix limit - Remove potential issue of long user IDs, as that is now solved
Rendered
Implementations:
Written by @ara4n , with contributions from @Johennes and @andybalaam .
FCP tickyboxes