sql: move privileges to a system table.

[mberhault]

DB/Table privileges are currently stored in database/table descriptors.
This means that a privilege change on a system table is similar to a schema change, conflicting with the lease logic.

It would be preferable to store them in their own system table. Keyed either by object ID (meaning privileges would be only for existing objects), or by name with optional globs (mysql does this, allowing privileges to be set on db.*).

The privileges on the privilege table itself would need to be ignored, with logic coming from the privileges on parent objects. eg: to add a privilege on table foo.bar, a user would need GRANT privileges on database foo, as opposed to INSERT privileges on system.privilege.

[petermattis]

I assume we'd check privileges in the privilege table using the gossiped system span. That will be mildly surprising from a user perspective as a permission change on a table will propagate asynchronously through the cluster.

[mberhault]

Yup. I think this will be the case with all config-type stuff.
The same happens for zone configs (granted, that's less time sensitive), and will happen when we add accounting and quotas (not particularly time sensitive either, but imagine a case where you bump the limit up because your writes are blocked. it'll take some propagation delay to have the new limit everywhere).

As long as we have the privilege manipulation commands use the real data, and have sufficient docs/warning about propagation delay, I'm not sure it's much of a concern.

[spencerkimball]

Removing 1.0 label. @mberhault what's the legit priority for this?

[mberhault]

I still think this would be nice, but post 1.0 is definitely fine.

[mberhault]

This is marked as "future improvement" for role-based access control

[mberhault]

CC: @awoods187

[petermattis]

@mberhault Should we close this issue, or move to Later?

[mberhault]

Given the disadvantages of moving privileges out of the descriptors, I propose we close this entirely, I can't see a good case being made for this anytime soon.

[vivekmenezes]

@mberhault I think we still need this because a change in privileges will only apply to the use of a descriptor for the future. Older versions of the descriptor through AS OF SYSTEM TIME will not use the current privileges.

[ajwerner]

We're in a sad state of affairs for a few reasons. A simple example of the problem is as follows:

root@localhost:26257/defaultdb> CREATE TABLE t (k STRING PRIMARY KEY);
CREATE TABLE
root@localhost:26257/defaultdb> CREATE USER u;
CREATE USER 1
root@localhost:26257/defaultdb> GRANT SELECT ON t TO u;
GRANT
root@localhost:26257/defaultdb> SELECT cluster_logical_timestamp();
    cluster_logical_timestamp
+--------------------------------+
  1575674919409559662.0000000000

Now for user u:

u@:26257/defaultdb> SELECT * FROM t;
ERROR: user u does not have SELECT privilege on relation t
SQLSTATE: 42501
u@:26257/defaultdb> SELECT * FROM t AS OF SYSTEM TIME 1575674919409559662.0000000000;
  k  
+---+

Fun! Even worse is that it works for dropped tables.

root@localhost:26257/defaultdb> DROP TABLE t;
DROP TABLE

u@:26257/defaultdb> SELECT * FROM t;
ERROR: relation "t" does not exist
u@:26257/defaultdb> SELECT * FROM t AS OF SYSTEM TIME 1575674919409559662.0000000000;
  k  
+---+

Even worse is that our user model is keyed by the string name.

root@localhost:26257/defaultdb> DROP USER u;
DROP USER 1
root@localhost:26257/defaultdb> CREATE USER u;
CREATE USER 1

A more sound user model would have a unique, internal identifier for each user such that a new user of the same name as an old user does not inherit the historical permissions of the old user of the same name.

A short term solution could be to use the current permissions on the table for authorization checks. Such an approach has a problem if the table has since been dropped. Currently it is useful to be able to access a dropped table using historical timestamps to recovery accidentally deleted data. Today this is done using historical reads, perhaps with BACKUP ... AS OF SYSTEM TIME. If we were to disallow all reads against dropped tables then we would prevent such recovery.

Two options to deal with historical reads on dropped tables would be to

1. implement sql: create an admin owned trash database for dropped tables #26476. In this approach, the table will still exist and thus still has active authentication information. This seems like a better approach.
2. use an ExportRequest to look up the state of authozation at the time that the table was dropped and use that to perform the check. This would be better than what we have now but isn't wonderful.

A better long-term solution would be to store authorization information outside of the table descriptor to join access rules to users (which are hopefully keyed but something other than just the string name).

[github-actions]

We have marked this issue as stale because it has been inactive for
18 months. If this issue is still relevant, removing the stale label
or adding a comment will keep it active. Otherwise, we'll close it in
10 days to keep the issue queue tidy. Thank you for your contribution
to CockroachDB!

[ajwerner]

Moving the privileges to a system table does not feel like the obvious approach. Given that, I'm going to close this.