sql: SHOW GRANTS includes inherited roles #104119

postamar · 2023-05-30T20:58:35Z

Previously, SHOW GRANTS would only list explicitly-granted privileges.
This commit changes its output to include privileges inherited by role
membership.

Consider for example:

  CREATE DATABASE d1;
  CREATE ROLE r1;
  CREATE USER u1;
  GRANT r1 TO u1;
  SET database = d1;
  CREATE TABLE t1 (id INT8);
  GRANT ALL ON TABLE t1 TO r1;

We want SHOW GRANTS ON TABLE t1 FOR u1 to list the ALL privilege that
u1 inherits from r1.

For this purpose this commit also adds a new kv_inherited_role_members
virtual table in crdb_internal which is essentially a recursive
self-join of system.role_members run by the node user.

This commit also changes the output of SHOW GRANTS ON ROLE to list
both explicit and implicit grantees: the former are the direct result of
a GRANT ... TO ... statement, the latter comprise also all the roles
which inherit the privileges following further role membership
relationships. For this purpose it also uses the new virtual table and
therefore the current user no longer requires SELECT privileges on the
system.role_members table.

Fixes #97299.

Release note (sql change): SHOW GRANTS now lists not just privileges
explicitly granted to each role, but also roles which inherit from
those. SHOW GRANTS ON ROLE statements no longer require any privileges
and also lists implicit grantees.

blathers-crl · 2023-05-30T20:58:39Z

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

cockroach-teamcity · 2023-05-30T20:58:45Z

This change is

rafiss

this is looking good! i think we should add tests for each one of: SHOW GRANTS ON [TABLE|SCHEMA|DATABASE|TYPE|FUNCTION] that demonstrate that they show the inherited privileges too

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @postamar)

pkg/sql/crdb_internal.go line 8059 at r1 (raw file):

		// This is defined as a table instead of a more straightforward view
		// because we want the query to run under the node user.
		const query = `

have you tried testing how this performs? i've crafted a query like this before and i think it had some bad issues

i'm thinking it would be better if we used the existing role-membership cache. because of how commonly we need to look up memberships, the way the cache works is that it actually loads everything in eagerly. so if we just iterate through each user using the forEachRole helper, and then call planner.MemberOfWithAdminOption for each one, it might actually perform better

pkg/sql/logictest/testdata/logic_test/pg_catalog line 1639 at r1 (raw file):

----
classid     objid       objsubid  refclassid  refobjid    refobjsubid  deptype
4294967102  111         0         4294967105  110         14           a

what made all the OIDs change? is it from adding CrdbInternalInheritedRoleMembersTableID? (that's fine if so, though it's unpleasant)

postamar

Good point, I completely forgot to do this.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss)

pkg/sql/crdb_internal.go line 8059 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

have you tried testing how this performs? i've crafted a query like this before and i think it had some bad issues

i'm thinking it would be better if we used the existing role-membership cache. because of how commonly we need to look up memberships, the way the cache works is that it actually loads everything in eagerly. so if we just iterate through each user using the forEachRole helper, and then call planner.MemberOfWithAdminOption for each one, it might actually perform better

Agreed, this seems better in every way.

pkg/sql/logictest/testdata/logic_test/pg_catalog line 1639 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

what made all the OIDs change? is it from adding CrdbInternalInheritedRoleMembersTableID? (that's fine if so, though it's unpleasant)

Yes indeed.

rafiss

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @postamar)

pkg/sql/crdb_internal.go line 8057 at r2 (raw file):

);`,
	populate: func(ctx context.Context, p *planner, _ catalog.DatabaseDescriptor, addRow func(...tree.Datum) error) (retErr error) {
		explicit := make(map[username.SQLUsername]map[username.SQLUsername]bool)

could you add a comment for why we need explicit (or perhaps a different name would make it more clear)

pkg/sql/crdb_internal.go line 8058 at r2 (raw file):

	populate: func(ctx context.Context, p *planner, _ catalog.DatabaseDescriptor, addRow func(...tree.Datum) error) (retErr error) {
		explicit := make(map[username.SQLUsername]map[username.SQLUsername]bool)
		if err := forEachRoleMembership(ctx, p.InternalSQLTxn(), func(role, member username.SQLUsername, isAdmin bool) error {

this seems to work, but it won't use the cache. that could be fine, but if it's easy to do without changing any existing logic, i would still vote for using forEachRole and then calling MemberOfWithAdminOption for each one.

pkg/sql/delegate/show_role_grants.go line 70 at r2 (raw file):

	query := fmt.Sprintf(`
WITH

could you add some commentary to explain in english why we need both u and explicit?

rafiss

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @postamar)

pkg/sql/crdb_internal.go line 8058 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

this seems to work, but it won't use the cache. that could be fine, but if it's easy to do without changing any existing logic, i would still vote for using forEachRole and then calling MemberOfWithAdminOption for each one.

nvm, ignore this comment. i see now that we do use the cache below, and this is just for getting the explicit memberships.

postamar · 2023-06-06T16:45:53Z

Thanks for taking another look, I've added some commentary. Please do let me know if it needs to be reworded again, I've been focused on this issue for long enough that I've lost perspective somewhat.

rafiss

the new commentary is extremely helpful. thanks!

we will need to backport this, since the field teams need it, so i'm adding some labels

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @postamar)

postamar · 2023-06-06T19:14:03Z

Thanks for the review!

Is this safe to backport as-is, or do we need to cluster-setting-gate it?
Can we backport vtables? Not a blocker if not, just an annoyance.

rafiss · 2023-06-06T20:09:18Z

Yes to both:

Is this safe to backport as-is, or do we need to cluster-setting-gate it?

It seems safe to me. It is a behavior change, but it is backwards-compatible.

Can we backport vtables? Not a blocker if not, just an annoyance.

This is safe since vtables are always populated by just one node. Though if we find for some reason that it's not safe, for example, due to how the vtable is populated, then we would also need a 23.2 version gate. I don't think that concern applies here.

postamar · 2023-06-06T21:22:03Z

Thanks!

This is safe since vtables are always populated by just one node.

My worry was more that because SHOW GRANTS ON ROLE delegates to a nested query which might be processed in a distributed fashion, the node that would populate the vtable might be a different node and therefore potentially unpatched.

bors r+

rafiss · 2023-06-06T21:45:06Z

hmm i suppose that's a fair concern. let's at least do some diligence: this should now be quite easy to test using the cockroach-go-testserver-upgrade-to-master logictest config, so can i request that you add one of those? sorry for the back and forth

bors r-

craig · 2023-06-06T21:45:09Z

Canceled.

postamar · 2023-06-07T13:12:11Z

We would have to have the SHOW GRANTS performed on a node with the new binary, which is easy enough, but how do we ensure that the query gets distributed in such a way that the vtable scan takes place on an old binary? Or do we just hope that the test never flaking means that the implementation is safe?

postamar · 2023-06-07T13:32:41Z

Also, once we backport this patch to 23.1, such a test cannot fail, because it will pull the latest maintenance version of 23.1.

rafiss · 2023-06-07T14:23:10Z

Or do we just hope that the test never flaking means that the implementation is safe?

yes i think in this case we can just use the flakiness as a signal.

although one thing i would say is that since the behavior is backwards-compatible, i believe it's OK that in a mixed version state, you might see the old behavior. i just want to ensure there's no error/crash.

Also, once we backport this patch to 23.1, such a test cannot fail, because it will pull the latest maintenance version of 23.1.

hm that's a good point. well, the test will still exist in the 23.1 branch though, which would run in a 22.2/23.1 mixed state

postamar · 2023-06-07T17:24:16Z

I haven't gotten anywhere with disabling distsql (thanks anyway @mgartner ) so I'll just add the test and call it a day.

Previously, SHOW GRANTS would only list explicitly-granted privileges. This commit changes its output to include privileges inherited by role membership. Consider for example: CREATE DATABASE d1; CREATE ROLE r1; CREATE USER u1; GRANT r1 TO u1; SET database = d1; CREATE TABLE t1 (id INT8); GRANT ALL ON TABLE t1 TO r1; We want `SHOW GRANTS ON TABLE t1 FOR u1` to list the ALL privilege that u1 inherits from r1. For this purpose this commit also adds a new `kv_inherited_role_members` virtual table in `crdb_internal` which is essentially a recursive self-join of `system.role_members` run by the node user. This commit also changes the output of `SHOW GRANTS ON ROLE` to list both explicit and implicit grantees: the former are the direct result of a `GRANT ... TO ...` statement, the latter comprise also all the roles which inherit the privileges following further role membership relationships. For this purpose it also uses the new virtual table and therefore the current user no longer requires SELECT privileges on the `system.role_members` table. Fixes cockroachdb#97299. Release note (sql change): SHOW GRANTS now lists not just privileges explicitly granted to each role, but also roles which inherit from those. SHOW GRANTS ON ROLE statements no longer require any privileges and also lists implicit grantees.

blathers-crl · 2023-06-07T18:14:44Z

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

postamar · 2023-06-07T20:35:59Z

bors r+

rafiss

thanks for the extra testing!

Reviewed 15 of 19 files at r1, 5 of 7 files at r2, 1 of 2 files at r3, 2 of 2 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @postamar)

craig · 2023-06-07T20:50:06Z

Build failed:

Bazel Essential CI (Cockroach)

postamar · 2023-06-08T12:52:55Z

bors r+

craig · 2023-06-08T13:19:12Z

Build succeeded:

Bazel Essential CI (Cockroach)

blathers-crl · 2023-06-08T13:19:23Z

Encountered an error creating backports. Some common things that can go wrong:

The backport branch might have already existed.
There was a merge conflict.
The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

error creating merge commit from ab08f1d to blathers/backport-release-22.2-104119: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.2.x failed. See errors above.

error creating merge commit from ab08f1d to blathers/backport-release-23.1-104119: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.1.x failed. See errors above.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

postamar force-pushed the fix-97299 branch 4 times, most recently from 5a9e1da to 1224785 Compare June 1, 2023 19:51

postamar marked this pull request as ready for review June 1, 2023 19:51

postamar requested a review from a team June 1, 2023 19:51

postamar requested review from a team as code owners June 1, 2023 19:51

postamar requested review from mgartner and rafiss and removed request for a team and mgartner June 1, 2023 19:51

postamar force-pushed the fix-97299 branch from 1224785 to f70acfd Compare June 5, 2023 11:47

rafiss requested changes Jun 5, 2023

View reviewed changes

postamar force-pushed the fix-97299 branch from f70acfd to 98bafbe Compare June 5, 2023 19:47

postamar commented Jun 5, 2023

View reviewed changes

postamar force-pushed the fix-97299 branch 2 times, most recently from 2ed81fb to 7d94b00 Compare June 6, 2023 15:36

rafiss reviewed Jun 6, 2023

View reviewed changes

postamar force-pushed the fix-97299 branch from 7d94b00 to cba1673 Compare June 6, 2023 16:44

rafiss approved these changes Jun 6, 2023

View reviewed changes

rafiss added backport-22.2.x backport-23.1.x Flags PRs that need to be backported to 23.1 labels Jun 6, 2023

postamar force-pushed the fix-97299 branch from cba1673 to ab08f1d Compare June 7, 2023 18:14

blathers-crl bot added the O-community Originated from the community label Jun 7, 2023

rafiss approved these changes Jun 7, 2023

View reviewed changes

craig bot merged commit fcce6ba into cockroachdb:master Jun 8, 2023

This was referenced Jun 8, 2023

release-23.1: sql: SHOW GRANTS includes inherited roles #104587

Merged

release-22.2: sql: SHOW GRANTS includes inherited roles #104589

Merged

cockroach-teamcity mentioned this pull request Jun 9, 2023

PR #104119 - sql: SHOW GRANTS includes inherited roles cockroachdb/docs#17228

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: SHOW GRANTS includes inherited roles #104119

sql: SHOW GRANTS includes inherited roles #104119

postamar commented May 30, 2023 •

edited

Loading

blathers-crl bot commented May 30, 2023

cockroach-teamcity commented May 30, 2023

rafiss left a comment

postamar left a comment

rafiss left a comment

rafiss left a comment

postamar commented Jun 6, 2023

rafiss left a comment

postamar commented Jun 6, 2023

rafiss commented Jun 6, 2023

postamar commented Jun 6, 2023

rafiss commented Jun 6, 2023 •

edited

Loading

craig bot commented Jun 6, 2023

postamar commented Jun 7, 2023

postamar commented Jun 7, 2023

rafiss commented Jun 7, 2023

postamar commented Jun 7, 2023

blathers-crl bot commented Jun 7, 2023

postamar commented Jun 7, 2023

rafiss left a comment

craig bot commented Jun 7, 2023

postamar commented Jun 8, 2023

craig bot commented Jun 8, 2023

blathers-crl bot commented Jun 8, 2023

sql: SHOW GRANTS includes inherited roles #104119

sql: SHOW GRANTS includes inherited roles #104119

Conversation

postamar commented May 30, 2023 • edited Loading

blathers-crl bot commented May 30, 2023

cockroach-teamcity commented May 30, 2023

rafiss left a comment

Choose a reason for hiding this comment

postamar left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

postamar commented Jun 6, 2023

rafiss left a comment

Choose a reason for hiding this comment

postamar commented Jun 6, 2023

rafiss commented Jun 6, 2023

postamar commented Jun 6, 2023

rafiss commented Jun 6, 2023 • edited Loading

craig bot commented Jun 6, 2023

postamar commented Jun 7, 2023

postamar commented Jun 7, 2023

rafiss commented Jun 7, 2023

postamar commented Jun 7, 2023

blathers-crl bot commented Jun 7, 2023

postamar commented Jun 7, 2023

rafiss left a comment

Choose a reason for hiding this comment

craig bot commented Jun 7, 2023

postamar commented Jun 8, 2023

craig bot commented Jun 8, 2023

blathers-crl bot commented Jun 8, 2023

postamar commented May 30, 2023 •

edited

Loading

rafiss commented Jun 6, 2023 •

edited

Loading