adapter: Add privileges to objects by jkosh44 · Pull Request #18700 · MaterializeInc/materialize

jkosh44 · 2023-04-10T20:39:29Z

This commit adds privileges to the following objects:

tables
views
materialized views
sources
types
secrets
connections
secrets
clusters
databases
schemas

This commit updates the on-disk stash representation and the catalog
tables/views that present this information to users.

Currently, privileges cannot be modified, and they are not looked at
when executing statements. These features will be implemented in a
future commit.

Part of MaterializeInc/database-issues#3380

Motivation

This PR adds a known-desirable feature.

Checklist

This PR has adequate test coverage / QA involvement has been duly considered.
This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
This PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way) and therefore is tagged with a T-proto label.
If this PR will require changes to cloud orchestration, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
This PR includes the following user-facing behavior changes:
- This release adds default privileges to database objects. The privileges cannot be modified and they do not affect database behavior. Those features will be added in a later commit.

This commit adds privileges to the following objects: - tables - views - materialized views - sources - types - secrets - connections - secrets - clusters - databases - schemas This commit updates the on-disk stash representation and the catalog tables/views that present this information to users. Currently, privileges cannot be modified, and they are not looked at when executing statements. These features will be implemented in a future commit. Part of #11579

def-

Thanks for extending slt. I'll check for platform-checks.

def- · 2023-04-12T21:44:05Z

Is it intentional that indexes have owners, but no privileges? I guess so since you can drop indexes, but can't insert into them directly?

def- · 2023-04-12T21:48:31Z

I also guess it's intentional that for types the privileges look like this?

materialize=> create type foo AS LIST (ELEMENT TYPE = text);
CREATE TYPE
materialize=> select * from mz_types where name = 'foo';
 id |  oid  | schema_id | name | category | owner_id |   privileges
----+-------+-----------+------+----------+----------+-----------------
 u1 | 20426 | 3         | foo  | list     | u2       | {u2=U/u2,=U/u2}

jkosh44 · 2023-04-12T23:16:52Z

Is it intentional that indexes have owners, but no privileges? I guess so since you can drop indexes, but can't insert into them directly?

Yes, the same goes for sinks.

jkosh44 · 2023-04-12T23:17:27Z

I also guess it's intentional that for types the privileges look like this?

materialize=> create type foo AS LIST (ELEMENT TYPE = text);
CREATE TYPE
materialize=> select * from mz_types where name = 'foo';
 id |  oid  | schema_id | name | category | owner_id |   privileges
----+-------+-----------+------+----------+----------+-----------------
 u1 | 20426 | 3         | foo  | list     | u2       | {u2=U/u2,=U/u2}

Yes, everyone get's Usage privileges on all types by default.

madelynnblue

Not done reading this whole thing yet, but wanted to get these comments out.

madelynnblue · 2023-04-13T00:35:14Z

+        ObjectType::Index => AclMode::empty(),
+        ObjectType::Type => AclMode::USAGE,
+        ObjectType::Role => AclMode::empty(),
+        ObjectType::Cluster => AclMode::USAGE.union(AclMode::CREATE),


Is it worth making USAGE union CREATE some rust const or Lazy?

union is a const function, so USAGE.union(CREATE) should be completely evaluated at compile time.

@parker-timmerman does that sound right?

I also just pushed a commit to switch other uses of bitor to union to take advantage of the const.

I believe in this case we'll only evaluate USAGE.union(CREATE) at compile time, if all_object_privileges is itself called in a const context, e.g. const X: AclMode = all_object_privileges(...). If it's called as part of a non-const function, then it will not. For example, in this playground we should fail to compile, if we were evaluating the const at compile time, but we're panicking at runtime, which indicates we're not.

If it's important that we're evaluating the union at compile time, then at the top of this function I'd add something like:

const USAGE_UNION_CREATE: AclMode = USAGE.union(CREATE);

which will for sure get evaluated at compile time.

I would have thought that the Rust compiler has all the information it needs to rewrite the implementation of all_object_privileges to replace all occurrences of AclMode::USAGE.union(AclMode::CREATE) with it's result.

For example, we know at compile time that AclMode::USAGE.union(AclMode::CREATE) is equivalent to (1 << 8 | 1 << 9) which can be fully evaluated at compile time. So it would make sense for the compiler to make this replacement during compilation.

I tried making a dummy example and looking at the compiled ASM, and this doesn't seem to be what's happening: https://godbolt.org/z/aKjcr8Ydr

However if I add a const variable in the function and then use that, it is replaced during compile time: https://godbolt.org/z/zMb5szj1x

So I made the separate consts and used those.

Yeah ideally the compiler would be able to do what you're describing, it has all the info it needs, but unfortunately const eval in Rust is still very much MVP.

madelynnblue · 2023-04-13T00:41:54Z

+                        MzAclItem {
+                            grantee: RoleId::Public,
+                            grantor: MZ_SYSTEM_ROLE_ID,
+                            acl_mode: AclMode::USAGE.bitor(AclMode::CREATE),


Should PUBLIC get CREATE in this? (See schema comment below.)

madelynnblue · 2023-04-13T00:44:12Z

+                        MzAclItem {
+                            grantee: RoleId::Public,
+                            grantor: MZ_SYSTEM_ROLE_ID,
+                            acl_mode: AclMode::USAGE.bitor(AclMode::CREATE),


Postgres 15 had a notable release note to

Remove PUBLIC creation permission on the public schema

Should we follow suit? If not, it should probably have a pretty strong reason. Cockroach has been getting requests to do the same as pg15, for example.

I think we definitely want to follow the latest PostgreSQL behavior for this. I've pushed an update so that the public schema created for all new databases are only created with USAGE for the public role.

I'm still thinking about the default database, schema, and cluster though. I think without CREATE privileges those aren't that useful. Additionally I think it may be nice to provide a default database and cluster that users can treat as a playground to do whatever they want in, then when they are ready they can create their own databases and clusters with stricter permissions.

@mjibson Any thoughts on this? I keep flip-flopping.

On the one hand it's better to default to higher security and let users opt into lower security by modifying the privileges. On the other hand it might be annoying if the first thing every customer has to do with a new Materialize deployment is modify the privileges on the default cluster, DB, and schema.

I think we should follow the recommended usage patterns:

first ensure that no schemas have public CREATE privileges
for every user needing to create non-temporary objects, create a schema with the same name as that user

This would also require us to:

implement $user in schema_path, which is probably easy

tell customers that they have to explicitly create schemas for non-superusers or explicitly GRANT access to existing schemas

I kinda think this is the point of RBAC? Non-superusers can't create anything until explicitly GRANTd access to an existing thing, or given their own place to make things in (CREATE SCHEMA alice AUTHORIZATION alice; although I'm not sure we support that AUTHORIZATION keyword yet?).

This is a good point, I've removed the public create privilege from the "materialize" database, "public" schema, and "default" cluster.

although I'm not sure we support that AUTHORIZATION keyword yet?

We do not, and it wasn't planned as part of this initial RBAC project.

madelynnblue · 2023-04-13T00:51:09Z

+                    MzAclItem {
+                        grantee: RoleId::Public,
+                        grantor: MZ_SYSTEM_ROLE_ID,
+                        acl_mode: AclMode::USAGE.bitor(AclMode::CREATE),


Same public concern maybe? Unclear.

def- · 2023-04-13T06:48:29Z

Do we also want \dp to work to check privileges?

materialize=> \dp
ERROR:  column "c.relacl" does not exist

def- · 2023-04-13T06:50:33Z

Is it intentional that indexes have owners, but no privileges? I guess so since you can drop indexes, but can't insert into them directly?

Yes, the same goes for sinks.

I'm not seeing a privileges column at all for sinks:

materialize=> select * from mz_sinks;
 id | oid | schema_id | name | type | connection_id | size | envelope_type | cluster_id | owner_id
----+-----+-----------+------+------+---------------+------+---------------+------------+----------
(0 rows)

jkosh44 · 2023-04-13T13:10:36Z

Do we also want \dp to work to check privileges?
materialize=> \dp
ERROR:  column "c.relacl" does not exist

Eventually it would be nice if that worked, but it's not expected to work after this PR.

jkosh44 · 2023-04-13T13:12:26Z

Is it intentional that indexes have owners, but no privileges? I guess so since you can drop indexes, but can't insert into them directly?

Yes, the same goes for sinks.

I'm not seeing a privileges column at all for sinks:
materialize=> select * from mz_sinks;
 id | oid | schema_id | name | type | connection_id | size | envelope_type | cluster_id | owner_id
----+-----+-----------+------+------+---------------+------+---------------+------------+----------
(0 rows)

I decided not to add it to mz_sinks or mz_index since it would always be empty. Do you think we should have it there?

def- · 2023-04-13T13:38:49Z

Ok, fine for me.

tr0njavolta

Docs changes 👍

…nd cluster

…-object-privileges # Conflicts: # src/adapter/src/catalog.rs # src/adapter/src/catalog/storage.rs

jkosh44 force-pushed the add-object-privileges branch 6 times, most recently from 44c9ca6 to e1238ad Compare April 11, 2023 17:02

jkosh44 changed the title ~~WIP: Add object privileges~~ adapter: Add privileges to objects Apr 11, 2023

jkosh44 force-pushed the add-object-privileges branch from e1238ad to 877f5a0 Compare April 11, 2023 17:27

jkosh44 requested a review from def- April 11, 2023 17:38

jkosh44 marked this pull request as ready for review April 11, 2023 18:02

jkosh44 requested review from a team as code owners April 11, 2023 18:02

def- approved these changes Apr 12, 2023

View reviewed changes

madelynnblue reviewed Apr 13, 2023

View reviewed changes

jkosh44 added 2 commits April 13, 2023 09:02

Fix expect typos

7f2f7ee

Remove create privilege from new public schemas

b5b3abb

Switch from bitor_assign to union

94482d0

tr0njavolta approved these changes Apr 13, 2023

View reviewed changes

jkosh44 added 2 commits April 13, 2023 15:04

Fix clippy

cc035e9

Fix test

ff6c922

constify all_bject_privileges

f3d665f

def- mentioned this pull request Apr 14, 2023

platform-checks: Add privileges validation to owners check #18758

Merged

5 tasks

jkosh44 mentioned this pull request Apr 14, 2023

adapter: Update privileges when an owner is updated #18777

Merged

5 tasks

jkosh44 added 2 commits April 15, 2023 10:40

Remove public create privileges from default user database, schema, a…

e328e9b

…nd cluster

Fix tests

15fdc31

jkosh44 mentioned this pull request Apr 16, 2023

sql: Ensure priv role refs are always valid #18783

Merged

5 tasks

madelynnblue approved these changes Apr 17, 2023

View reviewed changes

Merge branch 'main' of github.com:MaterializeInc/materialize into add…

6a2b964

…-object-privileges # Conflicts: # src/adapter/src/catalog.rs # src/adapter/src/catalog/storage.rs

jkosh44 enabled auto-merge (squash) April 17, 2023 14:24

jkosh44 added 2 commits April 17, 2023 10:54

Fix merge skew

ef57f31

Fix clippy

26f1d5a

jkosh44 merged commit eafcb25 into MaterializeInc:main Apr 17, 2023

jkosh44 deleted the add-object-privileges branch April 17, 2023 15:42

Conversation

jkosh44 commented Apr 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Checklist

Uh oh!

def- left a comment

Choose a reason for hiding this comment

Uh oh!

def- commented Apr 12, 2023

Uh oh!

def- commented Apr 12, 2023

Uh oh!

jkosh44 commented Apr 12, 2023

Uh oh!

jkosh44 commented Apr 12, 2023

Uh oh!

madelynnblue left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

def- commented Apr 13, 2023

Uh oh!

def- commented Apr 13, 2023

Uh oh!

jkosh44 commented Apr 13, 2023

Uh oh!

jkosh44 commented Apr 13, 2023

Uh oh!

def- commented Apr 13, 2023

Uh oh!

tr0njavolta left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jkosh44 commented Apr 10, 2023 •

edited

Loading