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

MSC2260: Update the auth rules for `m.room.aliases` events #2260

Open
wants to merge 7 commits into
base: master
from
@@ -0,0 +1,66 @@
# MSC2260: Update the auth rules for `m.room.aliases` events
This conversation was marked as resolved by richvdh

This comment has been minimized.

Copy link
@ara4n

ara4n Aug 30, 2019

Member

I've stared at this MSC a bunch now, and am finding it quite hard to review. I think part of it is because it doesn't state how m.room.aliases is meant to work (or at least how i understand it to be intended to be used), and precisely how it has failed, or why the proposal fixes the way it failed while solving the goals. Thus it's hard to tell if removing the auth specialcases on m.room.aliases is the right fix.

To try to gather my thoughts I've done a short alternative proposal at https://gist.github.com/ara4n/0ec423e7c321acbb53eed04ceb4dd7df, which i think fixes up the current problems without increasing complexity further.

This comment has been minimized.

Copy link
@richvdh

richvdh Aug 30, 2019

Author Member

I've stared at this MSC a bunch now, and am finding it quite hard to review. I think part of it is because it doesn't state how m.room.aliases is meant to work (or at least how i understand it to be intended to be used), and precisely how it has failed, or why the proposal fixes the way it failed while solving the goals. Thus it's hard to tell if removing the auth specialcases on m.room.aliases is the right fix.

Yeah, part of this was due to me not really understanding the status quo when I wrote it, and partly due to there still being some open questions about the solution. I've tried to address the first part at least (with help from your bis proposal, thanks).


## Background

Currently, `m.room.aliases` is subject to specific [authorization
rules](https://matrix.org/docs/spec/rooms/v1#authorization-rules). When these
rules were introduced, the intention was that `m.room.aliases` would be
maintained as an up-to-date list of the aliases for the room. However, this has
not been successful, and in practice the `m.room.aliases` event tends to drift
out of sync with the aliases (https://github.com/matrix-org/matrix-doc/issues/2262).

Meanwhile, `m.room.aliases` is open to abuse by remote servers who can add spam
or offensive aliases (https://github.com/matrix-org/matrix-doc/issues/625).

## Proposal

`m.room.aliases` exists to advertise the aliases available for a given
room. This is an ability which should be restricted to privileged users in the
room.

Therefore, the special-case for `m.room.aliases` is to be removed from the
[authorization
rules](https://matrix.org/docs/spec/rooms/v1#authorization-rules). `m.room.aliases`
would instead be authorised following the normal rules for state events.
This conversation was marked as resolved by ara4n

This comment has been minimized.

Copy link
@ara4n

ara4n Aug 30, 2019

Member

So I wonder if it still makes sense to prevent users (or admins) from creating alias events which refer to aliases on remote servers. The argument is:

  • If it's an ordinary user, they have no right to claim that a remote server's directory points to this room.
  • If it's an admin, they might want to curate the list and advertise remote directory entries without actually going and signing up on an account on the remote server or asking someone on the server to do so.

In practice, I don't think i've ever heard in practice of a room admin feeling frustrated that they can't add a remote server's alias to their room, so my hunch is that we should keep the restriction that you can only advertise aliases from your account's server on a given room.

However, m.room.aliases should otherwise be subject to normal auth rules.

This comment has been minimized.

Copy link
@richvdh

richvdh Aug 30, 2019

Author Member

In practice, I don't think i've ever heard in practice of a room admin feeling frustrated that they can't add a remote server's alias to their room,

I can see it being useful (especially if we lock down regular users' abilities to update the alias event), but I could go either way on it.

This comment has been minimized.

Copy link
@richvdh

richvdh Sep 2, 2019

Author Member

@ara4n: I'm not really following what you meant here. Is it still relevant?

This comment has been minimized.

Copy link
@ara4n

ara4n Sep 2, 2019

Member

i was just trying to campaign to keep 4a and 4b from the auth rules, to stop users from advertises aliases on remote servers. looks like we're aligned, so it's no longer relevant.


TBD: are you still allowed to add rooms to the directory when you don't have
This conversation was marked as resolved by richvdh

This comment has been minimized.

Copy link
@ara4n

ara4n Aug 30, 2019

Member

Yes, because m.room.aliases is for advertising alt aliases for a room. It does not need to reflect the full set of entries in the room directories around the place.

This comment has been minimized.

Copy link
@richvdh

richvdh Aug 30, 2019

Author Member

yes, that wasn't the source of my concern. The bullet points below were.

the necessary power level? If so, presumably this happens without updating the
`m.room.aliases` event. So:

* Is there any mechanism for syncing the alias list if you are later given
ops?
This conversation was marked as resolved by richvdh

This comment has been minimized.

Copy link
@ara4n

ara4n Aug 30, 2019

Member

No, because if the room admin has required a PL to advertise aliases, it means they are effectively wanting to manually curate the list themselves, and so have explicitly opted out of trying to keep it in sync with the room dirs.

This comment has been minimized.

Copy link
@richvdh

richvdh Aug 30, 2019

Author Member

mmmkay, but if a user with the relevant PL adds an alias, we're going to update the event, right?

This comment has been minimized.

Copy link
@ara4n

ara4n Aug 30, 2019

Member

sure. but by saying "only admins in this room can advertise aliases", nobody will have expectations that their random personal aliases will show up in m.room.aliases


* What if another user on your server has ops? What if Eve lacks ops and
secretly adds `#offensive_alias:example.com`, and then Bob (who has ops)
adds `#nice_alias:example.com`? How do we make sure that the offensive alias
isn't published by Bob?
This conversation was marked as resolved by richvdh

This comment has been minimized.

Copy link
@ara4n

ara4n Aug 30, 2019

Member

Bob would redact the offensive alias, and add his own? I don't understand what this has to do with another user on your server having ops.

Or is this an atomicity problem? In which case, can we split m.room.aliases into being a bunch of m.room.alias events, one per alias?

This comment has been minimized.

Copy link
@richvdh

richvdh Aug 30, 2019

Author Member

It's related to atomicity, yes. One event-per-alias is #2259; the main problem with it is, as our Californian friend pointed out, that we can't delete state.

In the absence of that, I am envisaging a situation like this:

  • Eve adds an offensive alias. She does not have the relevant PL to update the aliases event, so this has no effect except to add the entry to the directory, and it probably goes completely unnoticed. There is nothing to redact.
  • Later, Bob (who is on the same server as Eve, but has the PL to update the aliases event) adds a new alias to the room.

At this point, I am envisaging an implementation where the server will automatically generate an m.room.aliases event, with Bob as the sender, listing all of the current aliases for the room. In other words, other users in the room will see "Bob added #nice_alias and #evil_alias".

An alternative impl is possible where the new aliases event is based on the previous one and just adds the new alias; however (in the absence of #2259) that screams races to me and (particularly once state resolution comes into play) I think it's likely that the list is going to get out of sync.

This comment has been minimized.

Copy link
@ara4n

ara4n Aug 30, 2019

Member

yeah, i think we just need to bite the bullet and do #2259.


Server admins will continue to be able to remove entries from the directory
even if they do not have the right to send the `aliases` event (in which case
the `m.room.aliases` event will become outdated).
This conversation was marked as resolved by richvdh

This comment has been minimized.

Copy link
@ara4n

ara4n Aug 30, 2019

Member

This is why I think we should still let ordinary users add/remove entries.

Alternatively, we could privilege the hypothetical @:server.com user from #1777 to be able to maintain the aliases on behalf of a given server (even if ordinary users aren't allowed to add/remove), which to me feels cleaner and more likely to result in the advertised aliases being kept in sync with the directories.

This comment has been minimized.

Copy link
@richvdh

richvdh Aug 30, 2019

Author Member

I'd be hesitant to block this on @:server.com.

This comment has been minimized.

Copy link
@ara4n

ara4n Aug 30, 2019

Member

@:server.com really doesn't have to be a big blocking thing though; it would be easy to add - we just need to make the decision to do so. there are so many things it unlocks (like this) we should just add it, imo.

That said, I think it's a nice-to-have for this one.


It would also be logical to allow the contents of `m.room.aliases` events to be
redacted, as per [MSC2261](https://github.com/matrix-org/matrix-doc/issues/2261).

## Tradeoffs

Perhaps allowing room admins the ability to redact malicious `aliases` events
is sufficient? Given that a malicious user could immediately publish a new
`aliases` event (even if they have been banned from the room), it seems like
that would be ineffective.

This comment has been minimized.

Copy link
@ara4n

ara4n Aug 30, 2019

Member

Yeah, in that scenario we still need the ability for the room admin to gag randoms from adding remote aliases.

This comment has been minimized.

Copy link
@ara4n

ara4n Aug 30, 2019

Member

...or require the user to be in the room in order to set a m.room.alias, so we can stop abuse by banning them, which you'd get if we fell through to the default auth rules.


Or we could just allow room admins to issue new `m.room.aliases` events
(possibly restricting them to removing aliases, though it's TBD if state res
would work reliably in this case). However, that seems to suffer the same
problem as above.

## Potential issues

1. This will bake in https://github.com/matrix-org/synapse/issues/1477 in a way
that cannot be fixed in the case that the server admin doesn't have ops in
the room.

2. This would allow room operators to add 'fake' aliases: for example, I could
create a room and declare one of its aliases to be
`#matrix:matrix.org`. It's not obvious that this will cause any problems in
practice, but it might lead to some confusion.
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.