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

MSC3073: Role based access control #3073

Closed
wants to merge 30 commits into from
Closed

MSC3073: Role based access control #3073

wants to merge 30 commits into from

Conversation

erkinalp
Copy link

@erkinalp erkinalp commented Mar 25, 2021

This proposal adds role-based access-control into Matrix. Alternative to #2812.

Rendered

Signed-off by: Erkin Alp Güney <[email protected]>
Signed-off by: Erkin Alp Güney <[email protected]>
Signed-off by: Erkin Alp Güney <[email protected]>
Signed-off by: Erkin Alp Güney <[email protected]>
@erkinalp erkinalp changed the title MSC3072: Role based access control MSC3073: Role based access control Mar 25, 2021
@turt2live turt2live added kind:feature MSC for not-core and not-maintenance stuff proposal A matrix spec change proposal proposal-in-review labels Mar 25, 2021
Signed-off by: Erkin Alp Güney <[email protected]>
Signed-off by: Erkin Alp Güney <[email protected]>
Signed-off by: Erkin Alp Güney <[email protected]>
Signed-off by: Erkin Alp Güney <[email protected]>
Signed-off by: Erkin Alp Güney <[email protected]>
Copy link
Contributor

@kevincox kevincox left a comment

Choose a reason for hiding this comment

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

Overall I think this is a good proposal in the right direction, but it has some work need to polish it.

One concern is that I am confused by the algorithm with multiple roles. It isn't stated very clearly how the permissions of multiple roles are defined. At the beginning of the document it seems to imply that the order is important with a tristate. But the "Precise changes to v6's authorization rules" seems to imply that any match is enough to grant the permission. I think this needs to be clarified.

proposals/3073-rbac.md Outdated Show resolved Hide resolved
* Flag-style permissions are tri-state.
* Invite, kick, ban permissions are scoped, instead of being flags.
* The top role is implicit and called `T`, which is commonly known as the top type in the type
theory.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Most people don't know type theory. I would rather use something that the average user can understand such as "owner", "full_access" or similar.

proposals/3073-rbac.md Outdated Show resolved Hide resolved
proposals/3073-rbac.md Outdated Show resolved Hide resolved
Comment on lines 115 to 121
### Inheritance

Roles can list parent roles with `m.parent` attribute. This shall take an unordered list of role
identifiers. The permissions of the role in question and the recursive parents of role shall be
OR-ed to get the effective permission of that role. Inheritance cylcles, if they arise, shall be
broken by assuming the role that has created before-most wrt state resolution is the top-most in
the inheritance tree.
Copy link
Contributor

Choose a reason for hiding this comment

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

I find this hard to understand. You say the permissions are "OR-ed". I take this to mean the role will have permission if either it or its parent parent has permission. Right?

Does this mean that a child can not have less permission than its parent? Why not?

Copy link
Author

Choose a reason for hiding this comment

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

Does this mean that a child can not have less permission than its parent? Why not?

Yes. If you want to inhibit a permission, you will append a role to that specific user (twice in a row, due to the truth table of flags, it is allowed to add the roles repeatedly) inhibiting that, rather than using inheritance.

Copy link
Contributor

Choose a reason for hiding this comment

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

So the reason is to ensure that there is only one way to remove permissions? If so can we state the reasoning in the doc?

Copy link
Author

Choose a reason for hiding this comment

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

ensure that there is only one way to remove permissions

Indeed, inability to indirectly remove permissions is a balance against ability to lock-out.

proposals/3073-rbac.md Show resolved Hide resolved
proposals/3073-rbac.md Outdated Show resolved Hide resolved
proposals/3073-rbac.md Outdated Show resolved Hide resolved

All three properties are arrays of globs ([MSC2810](https://github.com/matrix-org/matrix-doc/pull/2810))
which are matched against role IDs (state keys of `m.role` events). All 3 arrays default to empty, implying
that all related actions are denied. Arrays are ordered and are matched as first-allowed wins.
Copy link
Contributor

Choose a reason for hiding this comment

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

The order doesn't matter since any match will give you the same permission.


`m.assign` denotes which roles a user in the applicable role will be able to assign (add) to users in
the room. This would be done through the `m.roles` property of the target user's membership event. The user
is able to target themselves.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a DoS footgun opportunity. Imagine that a a room has a "timeout" role which mutes the user. This rule is unnecessarily verbose and removes permissions from a wide variety of things including speaking, critically it removes the ability to remove roles. The intention is that you can give it to a misbehaving user as a warning, then remove it after a delay. You give your mods permission to add and remove this. Now your mods can throw a coup by giving the owner a "timeout" such that it overrides the permissions they have. I think it will be common for people to flip a ton of permissions to deny even if they would be denied anyways just to be sure.

  1. We should consider making T special such that its grants can't be overridden. (Although this conflicts with the permission drops proposed in MSC2199: Canonical DMs #2199) Otherwise users will shoot themselves in the foot and lock themselves out of critical permissions in a room.
  2. The order at which a role is added is very important and this permission doesn't say anything about it.
  3. We probably also need to include which users you can assign roles to.

Copy link
Author

Choose a reason for hiding this comment

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

Otherwise users will shoot themselves in the foot and lock themselves out of critical permissions in a room.

As a basic principle of access control, lockout is more preferable to uncontrollable absolute power. If you accidentally lock out the room, just create a new one and invite the members from old room

The order at which a role is added is very important and this permission doesn't say anything about it.

List-scoped and glob-scoped permissions will behave slightly differently from flag-style permissions. I am going to add that.

Copy link
Author

@erkinalp erkinalp Mar 28, 2021

Choose a reason for hiding this comment

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

We probably also need to include which users you can assign roles to.

That will be resolved in authorisation rules, and my current idea is only allowing granting permissions that you actually possess to users that have permissions that are perfect subset of yours, and revoking permissions from users that have permissions that are perfect subset of yours.

Copy link
Contributor

Choose a reason for hiding this comment

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

As a basic principle of access control, lockout is more preferable to uncontrollable absolute power

Is it? I mean if you have other ways to access the info but I can imagine that a large number of people will "own" rooms without have access to the server's DB. For example Discord and FB Messenger both don't let you lock yourself out. I think there needs to be someone who can solve this issue. It may just be the T role but I think we at least need something.

Just create a new one and invite the members from old room

This isn't "just" if you are a public room with a hundred or so members and now don't have a way to communicate with the current members. I guess with the current permissions you can't block yourself from seeing the member list but it is possible that we add permissions for that in the future. But even if you can invite all the members it looks just like a phishing attack.

Copy link
Author

Choose a reason for hiding this comment

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

I guess with the current permissions you can't block yourself from seeing the member list but it is possible that we add permissions for that in the future

Hiding the member and ban lists and permission changes will provide no security benefit, just cause confusion. I can think of no organisation that you would need to hide the underlying power structure to survive.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is barely relevant to my argument. There is a lot of state in a room and moving to a new one can be a serious undertaking even if you know who was in the room previous. You lose history, links, aliases are now bound to the old room and you need new ones (which probably aren't as obvious).

There is a reason that (as far as I am aware) no other messaging apps let the owner lock themselves out. It is bad UX. I understand that in some cases you need to trade UX for security, but this doesn't seem like a place where it is worthwhile.

I believe that wherever possible things should be undoable. We are just humans and humans make mistakes.

erkinalp and others added 10 commits March 28, 2021 07:55
Signed-off by: Erkin alp Güney <[email protected]>

Co-authored-by: Kevin Cox <[email protected]>
Signed-off by: Erkin Alp Güney <[email protected]>

Co-authored-by: Kevin Cox <[email protected]>
Signed-off by: Erkin Alp Güney <[email protected]>
Signed-off by: Erkin Alp Güney <[email protected]>
Signed-off by: Erkin Alp Güney <[email protected]>
Comment on lines +123 to +124
* If a selector is in the whitelist in one rule and in the blacklist in a later rule of the same kind,
then the entry is removed from the effective whitelist and added to the effective blacklist.
Copy link
Contributor

Choose a reason for hiding this comment

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

These lists are lists of globs so you can't really remove a single item from them due to overlaps. Would it make sense to rewrite this as matching each list separately and how to combine the results of matching?

Copy link
Author

Choose a reason for hiding this comment

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

I asked in the CS stack exchange and they told globs are REs in nature, which means you can optimise them (as intersection or union of regular languages are also regular languages).

Copy link
Contributor

Choose a reason for hiding this comment

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

Globs are a subset of regular expressions and even regular expressions don't have a clean way to actually represent the removal of one element. There are libraries that can optimize the matching based on set operations but it still seems like a weird way to describe it as now you have the set of expressions and two sets of blacklists.

However I see that is is now described as being added to the effective blacklist. Since the blacklist takes precedence over the whitelist does it make sense just to say that it is added to the blacklist, and ignore the bit about removing from the whitelist?

proposals/3073-rbac.md Outdated Show resolved Hide resolved
| Second operand | | | |
| `true` | `true` | `true` | `true` |
| `false` | `null` | `false ` | `false` |
| `null` | `true ` | `false` | `null` |
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused why true + false = null. Not only is it asymmetric with false + true = true but it doesn't seem to actually change anything as null + X = false + X other than + null (which just propagates) and IIUC a result of null is equivalent to false as far as permissions go (denied).

erkinalp and others added 3 commits March 29, 2021 08:43
Signed-off by: Erkin Alp Güney <[email protected]>

Co-authored-by: Kevin Cox <[email protected]>
Signed-off by: Erkin Alp Güney <[email protected]>
Signed-off by: Erkin Alp Güney <[email protected]>
erkinalp added 2 commits May 1, 2021 10:50
Signed-off by: Erkin Alp Güney <[email protected]>
Retraction is the commonly used term
Signed-off by: Erkin Alp Güney <[email protected]>
@turt2live turt2live added the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label Jun 8, 2021
@richvdh richvdh deleted the branch matrix-org:master August 27, 2021 18:24
@richvdh richvdh closed this Aug 27, 2021
@turt2live turt2live added abandoned A proposal where the author/shepherd is not responsive and removed proposal-in-review labels Aug 27, 2021
@turt2live
Copy link
Member

We took a look into why this is marked as abandoned/closed, and it turns out that when we did some shuffling in 2021 the PR got closed because the target branch was deleted. The MSC was subsequently marked as abandoned because we couldn't fix it.

If the author wishes to continue with this MSC, they will need to change the base branch to old_master and reopen it (we can't).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abandoned A proposal where the author/shepherd is not responsive 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.

4 participants