RFC: role based access control

[mberhault]

This is the initial RFC for role based access control.

The big caveat is the cost of expanding role memberships.
Until this can be done cheaply, this RFC is probably a non-starter.

[cockroach-teamcity]

This change is 

[mberhault]

Adding reviewers:

• @awoods187 for Guide-level explanation. Feel free to go over the rest, but we need to make sure this part matches what we agreed upon.
• sql folks: well, everything. The big question is the cost of lookup, see discussion in the RFC.

[mberhault]

BTW: I'm sending this out early for initial comments on the lookup problem. I will keep polishing it a bit.

[knz]

Thanks marc for tackling this feature.
So far I have a few remarks.

1. the RFC should (at least casually) outline which virtual tables must also be added for compatibility (there's a pg_catalog one and an information_schema one, IIRC)

2. the representation is indeed problematic and unreasonable. I believe the search for alternatives did not consider arrays, which we now support. The following schema is possible:

-- we use both tables, the first one as the authoritative source of direct membership
create table roles(name string primary key, members string[]);
-- and the second one for lookups, `roles` contains all the direct and indirect roles the member is part of
create table members(name string primary key, roles string[]);

3. even without arrays, the schema above can be used, by observing the benefit of a secondary table containing all the direct and indirect memberships for lookups

create table role_members(...); -- unchanged from RFC
create table role_lookup(member string primary key, role string); -- new: contains both direct and indirect memberships

then a lookup is select role from role_lookup where member=XXX, which will be an (efficient) range lookup.

4. regarding caching, the RFC does not yet consider the interaction between role membership and descriptor leases.

Say a user X has direct (not via role) permission to access table T. Our current mechanisms ensure that if the permissions on table T are changed (e.g. X doesn't have access to T any more, or user Y gets access), this translates to an update of descriptor T, which in turn bumps the version of the descriptor, which in turn ensures that any node has to wait for existing leases to expire when they want to access T, so they see the new permission configuration.
With roles this does not work well. Suppose role X has permission on T, and that doesn't change. Suppose a user A which is in role X, currently has a long-running session open and is using T in different transactions. Now consider, policy changes and A is removed from node X. How do database nodes notice this change and prevent the existing sessions from user A from accessing table T?
Without a proper mechanism that integrates table leases and roles, we would need to spell out in the RFC that the role lookup query must be ran for every use of descriptor T, even when the node already has a lease on it. This is very expensive!

I think we can solve this by leasing user/role records to nodes for caching. I'm not sure on the details though.

[knz]

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from Reviewable

[mberhault]

Thanks @knz. I've updated the RFC with:

• description of new/modified virtual tables (some todos left in there)
• mention of a "dual table" solution (actual memberships, and expanded memberships)
• expanded "problems" section of caching mentioning both transactions and descriptor leases.

I unfortunately know nothing about leases, so I'm not sure what this would look like. I'm also trying to figure out what PostgreSQL does for privilege/role manipulation and how it interacts with existing sessions/transaction/queries. I don't think we've defined our behavior well either.

[knz]

Thanks for the quick turnaround. To flesh out caching and the interaction with leases you'll probably need help from Andrei, Jordan and/or Vivek.

---

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from Reviewable

[awoods187]

Review status: all files reviewed at latest revision, 9 unresolved discussions, all commit checks successful.

---

docs/RFCS/20171118_role_based_access_control.md, line 1 at r1 (raw file):

- Feature Name: Role based access control

Nit but it should be "role-based" everywhere

---

docs/RFCS/20171118_role_based_access_control.md, line 47 at r1 (raw file):

# Summary

This feature is enterprise.

Nit I would suggest "Role-based access control is an enterprise feature."

---

docs/RFCS/20171118_role_based_access_control.md, line 51 at r1 (raw file):

We propose the addition of SQL roles to facilitate user management.

A role can be seen as a group containing one or more user or role.

This appears to be a circular definition by using role to define role. I understand you mean that a role can contain other roles but I do not think that this is clear. I would suggest defining this as a role is a group containing one or more users. Then expand that to say, a role can also contain other roles.

---

docs/RFCS/20171118_role_based_access_control.md, line 70 at r1 (raw file):

of the corresponding roles. The alternative would be granting privileges for all necessary objects,
making it likely to forget some.

I also think its useful to discuss the ability to change privileges for a role rather than each individual user.
Finally, i think we should also discuss this as part of a privacy by design solution making it easier to control the access privileges of a large number of people

---

docs/RFCS/20171118_role_based_access_control.md, line 135 at r1 (raw file):

This fails if `myrole` exists either as a role or a user.

Are there any limits on the number of roles that can be created?

---

docs/RFCS/20171118_role_based_access_control.md, line 153 at r1 (raw file):

Modification of role membership requires one of the following:
* cockroach administrator (`admin` role)

nit but this should be CockroachDB

---

docs/RFCS/20171118_role_based_access_control.md, line 231 at r1 (raw file):

`root` will have admin privileges on the `admin` role, allowing addition/removal of other users.

The `admin` role cannot be deleted, and cannot be empty.

Does this mean root can never be deleted from it too?

---

docs/RFCS/20171118_role_based_access_control.md, line 479 at r1 (raw file):

The [Caching memberships](#caching-memberships) options would make this much more efficient but would
sacrifice correctness when roles/privileges are changed within a transaction.

We can't sacrifice correctness this at all.

---

docs/RFCS/20171118_role_based_access_control.md, line 575 at r1 (raw file):

* **Pros**: near-instantaneous responsiveness to membership changes
* **Cons**: massive increase in complexity

I think we need to resolve our approach during the RFC period

---

Comments from Reviewable

[awoods187]

Review status: all files reviewed at latest revision, 10 unresolved discussions, all commit checks successful.

---

docs/RFCS/20171118_role_based_access_control.md, line 577 at r1 (raw file):

## Future improvements

We need to add control for this within the UX. Is there anything we need to do now, in the original implementation, to enable this expansion in the future?

---

Comments from Reviewable

[mberhault]

Review status: all files reviewed at latest revision, 10 unresolved discussions, all commit checks successful.

---

docs/RFCS/20171118_role_based_access_control.md, line 1 at r1 (raw file):

Previously, awoods187 (Andy Woods) wrote…

Nit but it should be "role-based" everywhere

Done. That's the only place I used the phrasing.

---

docs/RFCS/20171118_role_based_access_control.md, line 47 at r1 (raw file):

Previously, awoods187 (Andy Woods) wrote…

Nit I would suggest "Role-based access control is an enterprise feature."

Done.

---

docs/RFCS/20171118_role_based_access_control.md, line 51 at r1 (raw file):

Previously, awoods187 (Andy Woods) wrote…

This appears to be a circular definition by using role to define role. I understand you mean that a role can contain other roles but I do not think that this is clear. I would suggest defining this as a role is a group containing one or more users. Then expand that to say, a role can also contain other roles.

Done.

---

docs/RFCS/20171118_role_based_access_control.md, line 70 at r1 (raw file):

Previously, awoods187 (Andy Woods) wrote…

I also think its useful to discuss the ability to change privileges for a role rather than each individual user.
Finally, i think we should also discuss this as part of a privacy by design solution making it easier to control the access privileges of a large number of people

Done.

---

docs/RFCS/20171118_role_based_access_control.md, line 135 at r1 (raw file):

Previously, awoods187 (Andy Woods) wrote…

Are there any limits on the number of roles that can be created?

nope. added to the list of rules above.

---

docs/RFCS/20171118_role_based_access_control.md, line 153 at r1 (raw file):

Previously, awoods187 (Andy Woods) wrote…

nit but this should be CockroachDB

not done.

---

docs/RFCS/20171118_role_based_access_control.md, line 231 at r1 (raw file):

Previously, awoods187 (Andy Woods) wrote…

Does this mean root can never be deleted from it too?

yup. Added.

---

docs/RFCS/20171118_role_based_access_control.md, line 479 at r1 (raw file):

Previously, awoods187 (Andy Woods) wrote…

We can't sacrifice correctness this at all.

We'll have to sacrifice something if we don't want to be insanely slow. The right balance will have to be determined.
FYI: the way we currently do privileges also sacrifices some correctness.

---

docs/RFCS/20171118_role_based_access_control.md, line 575 at r1 (raw file):

Previously, awoods187 (Andy Woods) wrote…

I think we need to resolve our approach during the RFC period

That's why it's under "unresolved questions". These must all be resolved before the RFC can be enter FCP.

---

docs/RFCS/20171118_role_based_access_control.md, line 577 at r1 (raw file):

Previously, awoods187 (Andy Woods) wrote…

We need to add control for this within the UX. Is there anything we need to do now, in the original implementation, to enable this expansion in the future?

added a admin UI subsection to future improvements. It's already listed as "out of scope".

---

Comments from Reviewable

[nstewart]

Review status: 0 of 1 files reviewed at latest revision, 12 unresolved discussions, all commit checks successful.

---

docs/RFCS/20171118_role_based_access_control.md, line 53 at r3 (raw file):

another role.

Roles can have privileges on SQL objects. Any member of the role (directly a member,

I know this doesn't say roles can have privileges exclusively on SQL objects, we need to make sure this system can scale beyond SQL. For example, would this system let us control who can do certain operations in the admin UI ? cc @awoods187 @dianasaur323

---

docs/RFCS/20171118_role_based_access_control.md, line 141 at r3 (raw file):

This fails if `myrole` exists either as a role or a user.

### Granting and revoking privileges for a role

How are new privileges created and where do they live?

---

Comments from Reviewable

[mberhault]

Review status: 0 of 1 files reviewed at latest revision, 12 unresolved discussions, all commit checks successful.

---

docs/RFCS/20171118_role_based_access_control.md, line 53 at r3 (raw file):

Previously, nstewart (Nate) wrote…

I know this doesn't say roles can have privileges exclusively on SQL objects, we need to make sure this system can scale beyond SQL. For example, would this system let us control who can do certain operations in the admin UI ? cc @awoods187 @dianasaur323

There's no concept of privileges for the admin UI right now, and it's not within scope of this RFC to add them. That said, any privilege (including admin UI access) can be added, either applied to a specific object (eg: a table), or to a user/role (eg: user/role attributes, which are also out of scope)

---

docs/RFCS/20171118_role_based_access_control.md, line 141 at r3 (raw file):

Previously, nstewart (Nate) wrote…

How are new privileges created and where do they live?

there are no new privileges. roles can be granted the same privileges that users currently get and no changes are done in the way they are stored (they're in the database and table descriptors)

---

Comments from Reviewable

[nstewart]

docs/RFCS/20171118_role_based_access_control.md, line 53 at r3 (raw file):

Previously, mberhault (marc) wrote…

There's no concept of privileges for the admin UI right now, and it's not within scope of this RFC to add them. That said, any privilege (including admin UI access) can be added, either applied to a specific object (eg: a table), or to a user/role (eg: user/role attributes, which are also out of scope)

Right. Wasnt thinking we add them now, but just want to make sure that we had a path to support them with this system when the time comes.

---

Comments from Reviewable

[cuongdo]

cc @andreimatei @jordanlewis @vivekmenezes Could one of you provide your thoughts on how leases could interact efficiently with roles?

[dianasaur323]

docs/RFCS/20171118_role_based_access_control.md, line 53 at r3 (raw file):

Previously, nstewart (Nate) wrote…

Right. Wasnt thinking we add them now, but just want to make sure that we had a path to support them with this system when the time comes.

from the admin UI perspective, I think this LGTM for now. I believe our first step was to just persist privileges based on the roles. That being said, there could be other privileges outside of SQL at some point, so agreed that we should keep that in mind.

---

Comments from Reviewable

[awoods187]

Review status: 0 of 1 files reviewed at latest revision, 7 unresolved discussions, all commit checks successful.

---

docs/RFCS/20171118_role_based_access_control.md, line 571 at r5 (raw file):

This is the most naive way of representing memberships and maps well to the conceptual representation of roles.

This makes role manipulation cheap, with role lookups bearing the brunt of the cost.

Why is this the preferred solution? Won't role lookups be more frequent than role manipulation? Do we have an understanding of how large this cost will be? Doesn't this solution need to be considered in the context of cacheing memberships? E.g., how that is resolved would impact the prefered option

---

docs/RFCS/20171118_role_based_access_control.md, line 581 at r5 (raw file):

Expanding role membership into database/table descriptors (descriptor contains all users with privileges):
* **Pros**: regular SQL operations only require the user and descriptor
* **Cons**: more complex manipulation of roles, increased cost for every large roles

nit but I believe this should be either "very large roles" or "every large role"

---

docs/RFCS/20171118_role_based_access_control.md, line 667 at r5 (raw file):

* **Pros**: near-instantaneous responsiveness to membership changes
* **Cons**: massive increase in complexity

What is the currently preferred solution? I'm concerned with the accuracy performance tradeoff described above

---

Comments from Reviewable

[mberhault]

Review status: 0 of 1 files reviewed at latest revision, 7 unresolved discussions, all commit checks successful.

---

docs/RFCS/20171118_role_based_access_control.md, line 571 at r5 (raw file):

Previously, awoods187 (Andy Woods) wrote…

Why is this the preferred solution? Won't role lookups be more frequent than role manipulation? Do we have an understanding of how large this cost will be? Doesn't this solution need to be considered in the context of cacheing memberships? E.g., how that is resolved would impact the prefered option

It isn't, it's just the most naive approach and probably a reasonable first pass.

---

docs/RFCS/20171118_role_based_access_control.md, line 581 at r5 (raw file):

Previously, awoods187 (Andy Woods) wrote…

nit but I believe this should be either "very large roles" or "every large role"

Done.

---

docs/RFCS/20171118_role_based_access_control.md, line 667 at r5 (raw file):

Previously, awoods187 (Andy Woods) wrote…

What is the currently preferred solution? I'm concerned with the accuracy performance tradeoff described above

No idea. I'm waiting for input from people familiar with leases. Most likely the final implementation will be none of the above.

---

Comments from Reviewable

[vivekmenezes]

Nice writeup! thanks for putting it together.

[vivekmenezes]

there can be no cycles in this relationship

[mberhault]

This is mentioned in the set of rules in Role basics

[vivekmenezes]

A member cannot override the privileges granted to it from its parent role by revoking the privilege

[mberhault]

I don't know what that means. A given user has exactly the privileges granted to itself and all the roles it's a member of (directly or indirectly).

[vivekmenezes]

If you run "GRANT role1 to role2" how do you undo the relationship without dropping role2?

[vivekmenezes]

I see below that you want to use REVOKE for that.

[vivekmenezes]

Using Foreign Keys here will make it simpler to manage. Do we want to use Foreign Keys? We would need to have two columns user_member and role_member (a self referential key)

[vivekmenezes]

Or should we be using the OID type to reference these?

[mberhault]

Self-referential keys are currently a problem and don't really add anything. We have plenty of checks and cleanup to do when dropping a user, removing its role_members entries is pretty straightforward.

AFAICT, OIS is just an alias used in the virtual tables. The virtual tables section uses those, but I don't think we need them internally.

[vivekmenezes]

I think we ought to do this. By caching table schema on nodes today we run into the same correctness issues. In particular we support ATOMIC transactions on Schema but not SERIALIZABLE transactions.

[mberhault]

From a correctness perspective, we absolutely shouldn't, but then again, we shouldn't have privileges in descriptors either.
The caching memberships section is where some of those improvements are proposed, but all of them suck. There may be something we could do w.r.t descriptor leases, but this I where I need some help understanding the options.

[vivekmenezes]

Schema changes return to a user when they are fully applied. This is done by incrementing the version on the schema and waiting until there are no outstanding leases on the previous version. I think you can list this as a limitation of the current implementation and do nothing about it for 2.0 . We do not want to support schema changes (version upgrades) on system tables.

Another thought is to create another table (non system one called role_versions) that can be used by nodes needing a cache of the table data. Caching the system table data requires acquiring a lease on the role_schema table. modifying the system table will also modify the schema of the new table by publishing a new version and waiting for it to be published throughout the cluster. But I feel it's best not done in this RFC.

[andreimatei]

Review status: 0 of 1 files reviewed at latest revision, 13 unresolved discussions, all commit checks successful.

---

docs/RFCS/20171118_role_based_access_control.md, line 652 at r6 (raw file):

**TODO(mberhault)**: this section is still very vague. Some points to clarify:
* do we care about those within transactions? Do current privileges/user deletion work properly with transactions?
* how do descriptor leases work? Can we piggy-back onto those?

There's an RFC about table descriptor leases that has been updated recently; you can read there about how they work and if you have any questions I'm happy to talk about it.
But, to be honest, I don't see very clearly what the connection between privileges and descriptors (and also leases on descriptors) is. I think it's quite unfortunate that currently the privileges are stored in descriptors and thus versioned. It seems to me that privileges don't want versioning - once you add or revoke a privilege, that should dictate what the respective user can do regardless of the timestamp at which she operates (like, if my privilege has been revoked, I shouldn't be able to use AS OF SYSTEM TIME to continue reading the table).

Similarly, it seems to me that "leasing" a privilege shouldn't be a thing. Like, if I want to revoke access to evil_bitcoin_thief, I don't want to be told that the user had its privilege locked-in for another 5 minutes through a lease.

What I think is the sane thing to do here is to divorce the storage and access of privileges from table descriptors, and have a caching mechanism with TTL for them (with background refresh), plus a best-effort cache invalidation mechanism for revocations (based on gossip).
If possible, I'd stay out of allowing privilege changes in transactions, perhaps with the exception of grants on new tables where the grant is done in the same txn as the creation - that's a special case where it's probably clear enough what should happen.

---

Comments from Reviewable

[mberhault]

@vivekmenezes, @jordanlewis, @knz: thank you for the brainstorming session.
I've included the proposal in the "Determining role memberships" section and dropped some of the other things (the caching section is now gone).

I still want to put more details in there including code samples, but I'm going through docs and code first. Feel free to take a look and correct any mistakes on my part.

[vivekmenezes]

Review status: 0 of 1 files reviewed at latest revision, 13 unresolved discussions, all commit checks successful.

---

docs/RFCS/20171118_role_based_access_control.md, line 304 at r7 (raw file):

Previously, mberhault (marc) wrote…

The index isn't terribly important, but it doesn't hurt to be able to filter things by user or role faster.

As for names, they intentionally share the same namespace (see the "role basics" section). A user in postgresql (and in this RFC) is really just a role that is allowed to login and has no members.

I see. I suppose we can move forward with your plan.

---

docs/RFCS/20171118_role_based_access_control.md, line 430 at r7 (raw file):

Previously, mberhault (marc) wrote…

Sure, but we have to determine when to stop storing a given a user's expanded roles in the node-level cache. As long as a user is in the cache, we keep refreshing its roles when leases expire.
A size limit may be sufficient as even a few MBs will be a lot of users, but time-based expiration doesn't hurt.

True, but in reality that doesn't seem important in the 2.0 timeframe. I'd just create a limitation that we can address later when we have someone using cockroach with like tens of thousands of users.

---

Comments from Reviewable

[a-robinson]

Very well thought out!

[a-robinson]

https://xkcd.com/859/

[mberhault]

Done. Feeling tense?

[a-robinson]

Why shouldn't others be able to create roles?
And why shouldn't a role admin be able to drop the role they are the admin for?

[mberhault]

similar to databases, we don't have the concept of an "owner". In postgresql, you need CREATEROLE privileges which are disconnected from the "role admin" membership type.

[a-robinson]

Even indirect privileges via membership in another role?

[mberhault]

Nope, just privileges directly assigned. Memberships get automatically dropped but you need to clear out privileges first. Clarified the comment.

[a-robinson]

How does this interact with non-CCL builds? Is it just the code for managing roles that's CCL'ed?

[mberhault]

Yeah, but there's some question about code location. This is so similar to users for some bits that splitting it into CCL and OSS is really making life purposefully difficult for ourselves.

[mberhault]

But the answer is yes: create/drop/grant/revoke for roles will be CCL. Granting privileges to a role will still work as will permission checks (obviously). We do need to make sure we get a nice "you need a CCL build and license" message if you run them in the OSS binary.

I think this is a fair way of doing it but I'm open to suggestions.

[a-robinson]

We'd probably want to do this with a view to preserve compatibility with existing queries against the users table.

[mberhault]

you mean we "don't want to do this"?

[a-robinson]

Some sort of changeover/migration is fine with me, it just seems that we'll either need a view for system.users if we do move all the data or a view for system.roles if we don't move all the data.

[a-robinson]

I take it that renaming a user/role isn't possible? If it was I'd expect to see IDs be used for membership.

[mberhault]

correct. That would be the alter command which is not currently supported. Adding details along with other things about alter in out-of-scope section.
We could use IDs, but two things make this undesirable:

• migrations are already tricky, re-keying the users table would be horrible
• rename is rare and is not in the path of regular queries so it's ok to be slow

[a-robinson]

Well we could get around the re-keying issue by just adding an ID column with a secondary index on it. You're right that rename is rare, we'll just need to have very good testing to make sure we don't leave any references around to the old name. Things like foreign table references and view dependencies have had bugs in their initial implementations.

[a-robinson]

Is the ALTER ROLE statement being left out because we don't really need it yet or because we explicitly don't want it?

[mberhault]

We don't need it, the only things you can do with it is change attributes (which we don't have) and rename.
Added to out-of-scope section

[a-robinson]

We do have the ADMIN attribute, but understood.

[mberhault]

ADMIN is an option of the membership, not the role. This can be changed in REVOKE <role> FROM <user> ADMIN OPTION.

[a-robinson]

s/the some/some/

[mberhault]

Done.

[a-robinson]

I'd vote for this along with a release note, for what it's worth

[mberhault]

Except I don't think we have a good way to 1) notify users, and 2) resume the migration after the problem has been fixed.
This would involve the concept of "skippable" and "resumable" migrations. I'm voting for "just overwrite it", mostly because that's by far the safest solution.

[a-robinson]

It's not safe for someone that has an admin user and won't be able to log in as them after the upgrade.

If it changes anything, I interpret "fail the migration" as "crash the server with a log.Fatal message explaining what needs to be done".

[mberhault]

So log fatal and ask them to start with an older binary and remove the user? That's one option albeit a radical one.

[a-robinson]

Or if it's part of a cluster, just connect to one of the already running nodes and do it.

[a-robinson]

We'd like to make it so that only the node user/role can write to certain system tables (#19277). This is another nice motivating use case.

[mberhault]

Yup, I'm definitely in favor of changing that. Getting around CCL is one point in favor but the biggest one is that it's way too easy for people to shoot themselves in the foot.

[bdarnell]

Review status: 0 of 1 files reviewed at latest revision, 27 unresolved discussions, all commit checks successful.

---

docs/RFCS/20171118_role_based_access_control.md, line 674 at r8 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

Or if it's part of a cluster, just connect to one of the already running nodes and do it.

I'd also prefer a log.Fatal in this case.

---

docs/RFCS/20171118_role_based_access_control.md, line 422 at r9 (raw file):

Modifying role memberships is done through normal operations on the system tables.

However, we set `UpVersion` on the `role_members` table to trigger a descriptor version upgrade,

Using UpVersion (which was intended for schema changes) to signal changes to the contents of a table seems wrong. Can we use ModifiedSpanTrigger instead? If not, I think it might be better to create a new one instead of abusing UpVersion.

For comparison, how are non-role grants propagated? It seems like role memberships should be propagated in the same way.

---

docs/RFCS/20171118_role_based_access_control.md, line 472 at r9 (raw file):

The `admin` role is a special role corresponding to the current `root` user.
It is checked everywhere we currently check `if user == security.RootUser`.

So we will be adding special "is admin" checks everywhere? It would be nice if we could create new permissions for all admin functionality so these permissions could be granted to other users. If introducing permissions that don't correspond to tables or other schema objects is a headache we don't have to do it here, but while we're changing all the == security.RootUser checks it might be nice to introduce the new flexibility.

---

docs/RFCS/20171118_role_based_access_control.md, line 670 at r9 (raw file):

### Existence of an `admin` user before migrations

One of the migrations needed for role-based access control is the addition of the `admin` role to the

Is it important that the new role be named admin, or could we give it some name that is less likely to collide with existing users? (crdb_admin,admin_group? wheel?)

How likely is it that we will need to create other special user/role names in the future? Should we think about carving out some sort of reserved namespace?

---

docs/RFCS/20171118_role_based_access_control.md, line 674 at r9 (raw file):

We should decide what to do if a user with that name exists. Some possibilities:
* just overwrite it (clear the password hash and set the `isRole` field) and document the upgrade breakage.

Mention somewhere that you should not be able to log in with a certificate that was generated for a role instead of a user.

---

Comments from Reviewable

[mberhault]

Review status: 0 of 1 files reviewed at latest revision, 27 unresolved discussions, all commit checks successful.

---

docs/RFCS/20171118_role_based_access_control.md, line 310 at r8 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

Some sort of changeover/migration is fine with me, it just seems that we'll either need a view for system.users if we do move all the data or a view for system.roles if we don't move all the data.

I'm not sure we need one. The only external tool of ours that uses it is cockroach user get <username>. Are we concerned about other people using it? Would they be using the virtual tables?

---

docs/RFCS/20171118_role_based_access_control.md, line 322 at r8 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

Well we could get around the re-keying issue by just adding an ID column with a secondary index on it. You're right that rename is rare, we'll just need to have very good testing to make sure we don't leave any references around to the old name. Things like foreign table references and view dependencies have had bugs in their initial implementations.

Agreed.

---

docs/RFCS/20171118_role_based_access_control.md, line 674 at r8 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

I'd also prefer a log.Fatal in this case.

Works for me.

---

docs/RFCS/20171118_role_based_access_control.md, line 422 at r9 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Using UpVersion (which was intended for schema changes) to signal changes to the contents of a table seems wrong. Can we use ModifiedSpanTrigger instead? If not, I think it might be better to create a new one instead of abusing UpVersion.

For comparison, how are non-role grants propagated? It seems like role memberships should be propagated in the same way.

grants are still stored the same way, inside the db/table descriptors (they don't care if they're given to a user or role). I really don't want to expand roles memberships into the descriptors, that seems overkill. Ideally, I would like to get privileges out of the descriptors entirely. Whatever system we use here can be used for that as well.

I'm also a little iffy about using leases to do this. I should be able to start testing various methods sometime this week.

---

docs/RFCS/20171118_role_based_access_control.md, line 472 at r9 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

So we will be adding special "is admin" checks everywhere? It would be nice if we could create new permissions for all admin functionality so these permissions could be granted to other users. If introducing permissions that don't correspond to tables or other schema objects is a headache we don't have to do it here, but while we're changing all the == security.RootUser checks it might be nice to introduce the new flexibility.

I've already refactored some check to be IsSuperuser(myusername) instead of if myusername == security.RootUser. This will return true for all members of the admin role. I'll still keep root in there as it doesn't hurt to doubly make sure you always have a root user that can login (we disallow removing root from the admin role).

A future extension will be to add user (and role, same thing really) attributes. Postgresq has a superuser attribute which would be a perfect fit. I've kept those out-of-scope for now.

---

docs/RFCS/20171118_role_based_access_control.md, line 670 at r9 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Is it important that the new role be named admin, or could we give it some name that is less likely to collide with existing users? (crdb_admin,admin_group? wheel?)

How likely is it that we will need to create other special user/role names in the future? Should we think about carving out some sort of reserved namespace?

It's not that important, but I don't particularly see the appeal of a different name. The only things I can think of are: 1) collision with existing admin user on migration, and 2) collision with names from imported groups (eg: when we support LDAP, ActiveDirectory, etc...).
I'm not that attached to it though, but we need to decide for the in-review pr #20397

---

docs/RFCS/20171118_role_based_access_control.md, line 674 at r9 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Mention somewhere that you should not be able to log in with a certificate that was generated for a role instead of a user.

It's mentioned briefly in the users table (along the fact that they cannot have password). I'll add it to the Role basics section.

---

Comments from Reviewable

[mberhault]

No more unresolved questions:

• user admin exists was moved to a note on the migrations section (we log fatal)
• leases on system tables is covered in "drawbacks"

[a-robinson]

---

Reviewed 1 of 1 files at r10.
Review status: all files reviewed at latest revision, 27 unresolved discussions, some commit checks failed.

---

docs/RFCS/20171118_role_based_access_control.md, line 310 at r8 (raw file):

Previously, mberhault (marc) wrote…

I'm not sure we need one. The only external tool of ours that uses it is cockroach user get <username>. Are we concerned about other people using it? Would they be using the virtual tables?

I thought there was an implicit contract that we wouldn't break queries that rely on system tables, but maybe I'm mistaken. Someone from the SQL team would know better than me.

---

docs/RFCS/20171118_role_based_access_control.md, line 373 at r10 (raw file):

**TODO(mberhault)**: check postgres behavior w.r.t inheritance of `WITH ADMIN` option.
**TODO(mberhault)**: check postgres behavior: can we add/remote `WITH ADMIN` without changing membership?

s/remote/remove

---

docs/RFCS/20171118_role_based_access_control.md, line 386 at r10 (raw file):

* **Enterprise requirement**: valid license required.

**TODO(mberhault)**: check postgres behavior: can we add/remote `WITH ADMIN` without changing membership?

s/remote/remove

---

Comments from Reviewable

[mberhault]

Review status: 0 of 1 files reviewed at latest revision, 27 unresolved discussions.

---

docs/RFCS/20171118_role_based_access_control.md, line 310 at r8 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

I thought there was an implicit contract that we wouldn't break queries that rely on system tables, but maybe I'm mistaken. Someone from the SQL team would know better than me.

I've never heard of it but that would indeed seriously impact this.

---

docs/RFCS/20171118_role_based_access_control.md, line 373 at r10 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

s/remote/remove

Done.

---

docs/RFCS/20171118_role_based_access_control.md, line 386 at r10 (raw file):

Previously, a-robinson (Alex Robinson) wrote…

s/remote/remove

Done.

---

Comments from Reviewable

[bdarnell]

Reviewed 1 of 1 files at r11.
Review status: all files reviewed at latest revision, 24 unresolved discussions, all commit checks successful.

---

docs/RFCS/20171118_role_based_access_control.md, line 310 at r8 (raw file):

Previously, mberhault (marc) wrote…

I've never heard of it but that would indeed seriously impact this.

I don't think we guarantee backwards-compatibility for direct access to system tables; this is why we have commands like SHOW USERS which are just a wrapper around a select from system.users. Migrations here are tricky enough only considering our own internal uses, but if we can manage it we should feel free to make changes that suit our internal needs (especially in a semver-major release).

---

Comments from Reviewable

[mberhault]

Review status: all files reviewed at latest revision, 24 unresolved discussions, all commit checks successful.

---

docs/RFCS/20171118_role_based_access_control.md, line 310 at r8 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

I don't think we guarantee backwards-compatibility for direct access to system tables; this is why we have commands like SHOW USERS which are just a wrapper around a select from system.users. Migrations here are tricky enough only considering our own internal uses, but if we can manage it we should feel free to make changes that suit our internal needs (especially in a semver-major release).

This one is fairly easy, at least the fact that there's a new column (tests for migrations with backfill were a different story).
The only thing that will break is cockroach users get <foo> from older binaries. It may be worth making sure we use something other than the system tables for cli commands.

---

Comments from Reviewable

[jordanlewis]

Review status: all files reviewed at latest revision, 25 unresolved discussions, all commit checks successful.

---

docs/RFCS/20171118_role_based_access_control.md, line 201 at r8 (raw file):

Previously, mberhault (marc) wrote…

Nope, just privileges directly assigned. Memberships get automatically dropped but you need to clear out privileges first. Clarified the comment.

I think it's worth supporting a CASCADE modifier for this that does the "clearing out of privileges". This is directly analogous to many other operations in SQL.

---

docs/RFCS/20171118_role_based_access_control.md, line 111 at r11 (raw file):

* **`A ∈ B`**: user or role `A` is a member of role `B`
* **direct member**: A is a direct member of B if `A ∈ B`
* **indirect member**: A is an indirect member of B if `A ∈ C ... ∈ B` where `...` is be an arbitrary number of memberships

nit: s/is be/is/

---

Comments from Reviewable

[mberhault]

Review status: all files reviewed at latest revision, 25 unresolved discussions, all commit checks successful.

---

docs/RFCS/20171118_role_based_access_control.md, line 174 at r6 (raw file):

Previously, vivekmenezes wrote…

I see below that you want to use REVOKE for that.

Done.

---

docs/RFCS/20171118_role_based_access_control.md, line 559 at r6 (raw file):

Previously, vivekmenezes wrote…

Schema changes return to a user when they are fully applied. This is done by incrementing the version on the schema and waiting until there are no outstanding leases on the previous version. I think you can list this as a limitation of the current implementation and do nothing about it for 2.0 . We do not want to support schema changes (version upgrades) on system tables.

Another thought is to create another table (non system one called role_versions) that can be used by nodes needing a cache of the table data. Caching the system table data requires acquiring a lease on the role_schema table. modifying the system table will also modify the schema of the new table by publishing a new version and waiting for it to be published throughout the cluster. But I feel it's best not done in this RFC.

Done.

---

docs/RFCS/20171118_role_based_access_control.md, line 652 at r6 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

There's an RFC about table descriptor leases that has been updated recently; you can read there about how they work and if you have any questions I'm happy to talk about it.
But, to be honest, I don't see very clearly what the connection between privileges and descriptors (and also leases on descriptors) is. I think it's quite unfortunate that currently the privileges are stored in descriptors and thus versioned. It seems to me that privileges don't want versioning - once you add or revoke a privilege, that should dictate what the respective user can do regardless of the timestamp at which she operates (like, if my privilege has been revoked, I shouldn't be able to use AS OF SYSTEM TIME to continue reading the table).

Similarly, it seems to me that "leasing" a privilege shouldn't be a thing. Like, if I want to revoke access to evil_bitcoin_thief, I don't want to be told that the user had its privilege locked-in for another 5 minutes through a lease.

What I think is the sane thing to do here is to divorce the storage and access of privileges from table descriptors, and have a caching mechanism with TTL for them (with background refresh), plus a best-effort cache invalidation mechanism for revocations (based on gossip).
If possible, I'd stay out of allowing privilege changes in transactions, perhaps with the exception of grants on new tables where the grant is done in the same txn as the creation - that's a special case where it's probably clear enough what should happen.

Done.

---

docs/RFCS/20171118_role_based_access_control.md, line 201 at r8 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

I think it's worth supporting a CASCADE modifier for this that does the "clearing out of privileges". This is directly analogous to many other operations in SQL.

Postgres doesn't use CASCADE for user/role drops and has exactly the same behavior. I think this is because it's too easy to remove the last users with certain permissions. We could add it, but that's way out of scope.

---

docs/RFCS/20171118_role_based_access_control.md, line 111 at r11 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

nit: s/is be/is/

Done.

---

Comments from Reviewable

[mberhault]

No movement in about a week. If there are no objections, I'd like to move to FCP.

[andreimatei]

Review status: 0 of 1 files reviewed at latest revision, 25 unresolved discussions, all commit checks successful.

---

docs/RFCS/20171118_role_based_access_control.md, line 422 at r9 (raw file):

Previously, mberhault (marc) wrote…

grants are still stored the same way, inside the db/table descriptors (they don't care if they're given to a user or role). I really don't want to expand roles memberships into the descriptors, that seems overkill. Ideally, I would like to get privileges out of the descriptors entirely. Whatever system we use here can be used for that as well.

I'm also a little iffy about using leases to do this. I should be able to start testing various methods sometime this week.

Has thinking on this advanced?
It also seems to me that using the UpVersion mechanism here would be an abuse; I'd rather more directly use gossip but I don't have big arguments.

---

Comments from Reviewable

[mberhault]

Review status: 0 of 1 files reviewed at latest revision, 25 unresolved discussions, all commit checks successful.

---

docs/RFCS/20171118_role_based_access_control.md, line 422 at r9 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

Has thinking on this advanced?
It also seems to me that using the UpVersion mechanism here would be an abuse; I'd rather more directly use gossip but I don't have big arguments.

Nope. My plan is to start with a naive implementation (expand role memberships every time), then experiment with various solutions to notify the cache that a refresh is needed. Leases and gossip are both candidates.

---

Comments from Reviewable

[mberhault]

Moving to FCP. ETA for close is Wednesday Dec 20th.

[jordanlewis]

---

Reviewed 1 of 1 files at r12.
Review status: all files reviewed at latest revision, 25 unresolved discussions, all commit checks successful.

---

Comments from Reviewable