sql: ensure user has correct privileges when adding/removing regions #62186

arulajmani · 2021-03-18T02:27:35Z

Previously we did not account for privileges on database objects when
adding the default locality config on first region add or removing the
locality config on last region drop properly. In particular, we weren't
adding/removing the locality config on any descriptor that wasn't
visible to the user. This is bad because our validation logic expects
only and all objects in multi-region databases to have a valid locality
config. This means future accesses to such descriptors would fail
validation.

The root of this problem was the API choice here, ForEachTableDesc,
which filters out invisible descriptors. This patch instead switches
to using forEachTableInMultiRegionDatabase. While here, instead of
issuing separate requests for every table, I refactored this thing to
issue a single batch request instead.

Now that we view all the descriptors inside the database, unfiltered,
we perform privilege checks on them before proceeding with the add/drop
operation. In particular, the semantics are:

admin users are allowed to add/drop regions as they wish.
non admin-users require the CREATE privilege or must have ownership
on all the objects inside the database.

Closes #61003

Release note (sql change): ALTER DATABASE .. SET PRIMARY REGION now
requires both CREATE and ZONECONFIG privilege on all objects inside
the database when adding the first region to the database. Same for
dropping the last region using ALTER DATABASE ... DROP REGION.

cockroach-teamcity · 2021-03-18T02:27:42Z

This change is

otan

Reviewed 4 of 4 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm and @arulajmani)

pkg/ccl/logictestccl/testdata/logic_test/multi_region_privileges, line 25 at r1 (raw file):


statement ok
ALTER DATABASE  db SET PRIMARY REGION "us-east-1"

nit: extra space

pkg/ccl/logictestccl/testdata/logic_test/multi_region_privileges, line 35 at r1 (raw file):


statement error user testuser does not have CREATE or ZONECONFIG privilege on t
ALTER DATABASE db DROP REGION "us-east-1"

seems like we're missing a test on ADD REGION

pkg/ccl/logictestccl/testdata/logic_test/multi_region_privileges, line 57 at r1 (raw file):


statement ok
ALTER DATABASE  db SET PRIMARY REGION "us-east-1"

nit: extra space

pkg/sql/alter_database.go, line 371 at r1 (raw file):

	ctx context.Context, p *planner, tableDesc catalog.TableDescriptor,
) error {
	zoneConfigPrivErr := p.CheckPrivilege(ctx, tableDesc, privilege.ZONECONFIG)

i feel as if we should check each privilege and call out explicitly whether it is CREATE or ZONECONFIG that is the issue.

pkg/sql/region_util.go, line 66 at r1 (raw file):

		p.txn,
		sessiondata.InternalExecutorOverride{
			User: security.RootUserName(),

can you add a comment about why root is needed.

pkg/sql/type_change.go, line 487 at r1 (raw file):

	)
	defer cleanup()
	localPlanner := p.(*planner)

do we need a privilege check here as well, given that RBR tables are having zone configs attached?
looks like the checks are only in for final DROP or initial ADD

otan · 2021-03-18T03:20:42Z

chatted online and ADD/DROP non-first region not included so most comments can be ignored, but curious about this test failure:


        (XXUUU) PARTITION us-east-1: enum value "us-east-1" is not yet public
        datum.go:4026: in MakeDEnumFromLogicalRepresentation()

``

RichardJCai · 2021-03-18T15:09:29Z

pkg/sql/alter_database.go

+) error {
+	zoneConfigPrivErr := p.CheckPrivilege(ctx, tableDesc, privilege.ZONECONFIG)
+	createPrivErr := p.CheckPrivilege(ctx, tableDesc, privilege.CREATE)
+	if zoneConfigPrivErr != nil && createPrivErr != nil {


Since we eventually want to be more accurate with Postgres privileges and they don't have CREATE privilege for tables, maybe we should only show in the error that the user doesn't have ZONECONFIG. Or maybe we should show the same error message for ALTER TABLE LOCALITY must be owner of table %s or have CREATE privilege on table %s

Actually would it make more sense to pull out the privilege check in alter_table_locality.go (and possibly even alter_table.go into a helper function and just re-use it here so we don't duplicate privilege checks that should technically be the same?

Since we eventually want to be more accurate with Postgres privileges and they don't have CREATE privilege for tables, maybe we should only show in the error that the user doesn't have ZONECONFIG.

If we're accepting CREATE as a valid privilege to do this operation, then IMO the error message should indicate that. If we want to move more in line with Postgres then maybe we shouldn't accept CREATE here at all to begin with. Would you prefer that?

Or maybe we should show the same error message for ALTER TABLE LOCALITY must be owner of table %s or have CREATE privilege on table %s

I think that thing is busted to be honest, it should accept ZONECONFIG as a valid privilege too. I was thinking of calling this function in alter_table_locality.go, there's a TODO, I'll just address it here. That being said, I think it makes sense to add a nod to the "owner" part in the error message.

Actually would it make more sense to pull out the privilege check in alter_table_locality.go (and possibly even alter_table.go into a helper function and just re-use it here so we don't duplicate privilege checks that should technically be the same?

But aren't the privilege checks different for alter_table_locality.go and alter_table.go? IMO alter_table_locality.go should accept ZONECONFIG but alter_table.go should just be CREATE.

I was thinking we could pull out some of the shared logic for the privilege check from alter_table.go and alter_table_locality.go and use it in all three places. Something like one helper function for checking CREATE or OWNER which is used in all 3, however for alter_table_locality.go and this function here, we can wrap that helper and add an additional check for ZONECONFIG. If this turns out to be messy then I'd be okay with just creating one function for the locality privilege checks.

there's a TODO, I'll just address it here. That being said, I think it makes sense to add a nod to the "owner" part in the error message.

Changed my mind, I'll address this in a separate commit. It needs some testing as well.

Ah, I see what you mean, I'm on board. I'll do it when I address the alter_table_locality.go stuff 👍

Do we have an issue for the alter_table_locality.go privilege stuff or a general issue on the MR privilege stuff?

I did the alter_table_locality in the second commit. As for other MR privileges stuff, I still have to write them out after the whole Slack discussion.

Followups for after this thing goes in:

#62252
#62250

arulajmani · 2021-03-18T15:37:14Z

chatted online and ADD/DROP non-first region not included so most comments can be ignored, but curious about this test failure:
(XXUUU) PARTITION us-east-1: enum value "us-east-1" is not yet public
datum.go:4026: in MakeDEnumFromLogicalRepresentation()

Looks like when we GetAllDescriptors, we hydrate tables using the type descriptors read from the store -- not the uncommitted type descriptors modified previously in the transaction. Put another way, we're not seeing the promotion changes we just made. The naive fix is trivial, but I want to think about this a bit more.

arulajmani · 2021-03-18T16:40:49Z

Looks like when we GetAllDescriptors, we hydrate tables using the type descriptors read from the store -- not the uncommitted type descriptors modified previously in the transaction. Put another way, we're not seeing the promotion changes we just made. The naive fix is trivial, but I want to think about this a bit more.

Actually, turns out we weren't running the batch that modified the type descriptors until after repartitioning. It didn't matter earlier, but it does now because of how GetAllDescriptors hydrates things.

arulajmani

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm, @otan, and @RichardJCai)

pkg/ccl/logictestccl/testdata/logic_test/multi_region_privileges, line 35 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

seems like we're missing a test on ADD REGION

Upcoming patch.

pkg/sql/alter_database.go, line 371 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

i feel as if we should check each privilege and call out explicitly whether it is CREATE or ZONECONFIG that is the issue.

I don't fully follow -- you need either of those privileges, not both.

pkg/sql/region_util.go, line 66 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

can you add a comment about why root is needed.

Done.

pkg/sql/type_change.go, line 487 at r1 (raw file):

Previously, otan (Oliver Tan) wrote…

do we need a privilege check here as well, given that RBR tables are having zone configs attached?
looks like the checks are only in for final DROP or initial ADD

Upcoming patch!

arulajmani

There's a couple fundamental changes since I put this out:

Instead of accepting ZONECONFIG or CREATE, now we only accept CREATE. This is from the discussion thread on Slack.
There's a second commit that basically adds testing for ALTER TABLE LOCALITY. Functionally, nothing has changed here.

RFAL.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm, @arulajmani, @otan, and @RichardJCai)

RichardJCai

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm, @arulajmani, @otan, and @RichardJCai)

pkg/sql/alter_database.go, line 370 at r2 (raw file):

// - or have the CREATE privilege on the table.
// privilege on the table descriptor.
func (p *planner) ensureCorrectMultiRegionPrivilegesForTable(

Should this be renamed to checkCanAlterLocality or something along those lines to make it more specific.

Also I don't know if this is really a rule but I think we generally call these privilege check functions "check..."

pkg/sql/alter_database.go, line 414 at r2 (raw file):

		switch t := tbDesc.LocalityConfig.Locality.(type) {
		case *descpb.TableDescriptor_LocalityConfig_Global_:

I don't understand enough on this switch statement to review here, maybe @otan or @ajstorm can take a look at this specific part?

pkg/sql/alter_database.go, line 378 at r3 (raw file):

	}
	if !hasAdminRole {
		err := p.CheckPrivilege(ctx, tableDesc, privilege.CREATE)

Can you add a TODO here that the CREATE priv check is not inline with PG and should be removed when we refactor the privilege system

pkg/sql/type_change.go, line 487 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Upcoming patch!

Just double checking, this happens in the exec function so the correct privilege checks are done by this point right?

Previously we did not account for privileges on database objects when adding the default locality ocnfig on first region add or removing the locality config on last region drop properly. In particular, we weren't adding/removing the locality config on any descriptor that wasn't visible to the user. This is bad because our validation logic expects only and all objects in multi-region databases to have a valid locality config. This means future accesses to such descriptors would fail validation. The root of this problem was the API choice here, `ForEachTableDesc`, which filters out invisible descriptors. This patch instead switches to using `forEachTableInMultiRegionDatabase`. While here, instead of issuing separate requests for every table, I refactored this thing to issue a single batch request instead. Now that we view all the descriptors inside the database, unfiltered, we perform privilege checks on them before proceeding with the add/drop operation. In particular, the semantics are: - admin users are allowed to add/drop regions as they wish. - non admin-users require the CREATE privilege or must have ownership on all the objects inside the database. Closes cockroachdb#61003 Release note (sql change): `ALTER DATABASE .. SET PRIMARY REGION` now requires both CREATE and ZONECONFIG privilege on all objects inside the database when adding the first region to the database. Same for dropping the last region using `ALTER DATABASE ... DROP REGION`.

This patch calls into the `ensureCorrectMultiRegionPrivilegesForTable` function instead of checking privileges. This doesn't change any behavior, but I've added tests ensuring the semantics are sane. Release note: None

arulajmani

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm, @otan, and @RichardJCai)

pkg/sql/alter_database.go, line 370 at r2 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Should this be renamed to checkCanAlterLocality or something along those lines to make it more specific.

Also I don't know if this is really a rule but I think we generally call these privilege check functions "check..."

checkCanAlterLocality isn't really true though, as the same rules apply when re-partitioning a multi-region table because of a region add (more on this below). I'm tentatively changing this to checkPrivilegesForMultiRegionOperation, lemme know what you think.

pkg/sql/alter_database.go, line 414 at r2 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

I don't understand enough on this switch statement to review here, maybe @otan or @ajstorm can take a look at this specific part?

Rebase conflict, there's no functional change here.

pkg/sql/alter_database.go, line 378 at r3 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Can you add a TODO here that the CREATE priv check is not inline with PG and should be removed when we refactor the privilege system

Done.

pkg/sql/type_change.go, line 487 at r1 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Just double checking, this happens in the exec function so the correct privilege checks are done by this point right?

Not exactly -- right now, only the first region add/last region drops perform a privilege check (in the exec function). The check happens on all tables in the database as these are special operations which involve modifying the locality config on all objects inside the database.

All subsequent region adds/drops don't affect all the objects inside the database -- they only affect these "regional by row" tables. Regional by row tables need to be repartitioned based on the new set of regions and a zone config is then applied for every partition. I field #62250 to capture the work required for this scenario.

RichardJCai

Privileges LGTM

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm, @otan, and @RichardJCai)

pkg/sql/alter_database.go, line 414 at r2 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Rebase conflict, there's no functional change here.

Gotcha

arulajmani · 2021-03-22T18:06:55Z

TFTR! In @otan's words, if it's good enough for Richard its good enough for me.

bors r=richardjcai

craig · 2021-03-22T18:52:22Z

Build failed (retrying...):

GitHub CI (Cockroach)

craig · 2021-03-22T21:25:49Z

Build succeeded:

GitHub CI (Cockroach)

arulajmani requested review from otan, ajstorm and a team March 18, 2021 02:27

otan reviewed Mar 18, 2021

View reviewed changes

RichardJCai reviewed Mar 18, 2021

View reviewed changes

arulajmani force-pushed the mr-privs branch from b9a1cb9 to e78f0c8 Compare March 18, 2021 16:48

blathers-crl bot requested a review from otan March 18, 2021 16:48

arulajmani commented Mar 18, 2021

View reviewed changes

arulajmani requested a review from RichardJCai March 18, 2021 16:48

arulajmani force-pushed the mr-privs branch 3 times, most recently from 9e70ec7 to 6a508f4 Compare March 19, 2021 00:13

arulajmani commented Mar 19, 2021

View reviewed changes

RichardJCai suggested changes Mar 19, 2021

View reviewed changes

arulajmani added 2 commits March 19, 2021 14:13

sql: refactor alter table locality privileges check

e190c5a

This patch calls into the `ensureCorrectMultiRegionPrivilegesForTable` function instead of checking privileges. This doesn't change any behavior, but I've added tests ensuring the semantics are sane. Release note: None

arulajmani force-pushed the mr-privs branch from 6a508f4 to e190c5a Compare March 19, 2021 18:14

arulajmani commented Mar 19, 2021

View reviewed changes

arulajmani requested a review from RichardJCai March 19, 2021 18:15

RichardJCai approved these changes Mar 22, 2021

View reviewed changes

craig bot merged commit 7899744 into cockroachdb:master Mar 22, 2021

arulajmani mentioned this pull request Mar 23, 2021

release-21.1: sql: ensure user has correct privileges when adding/removing regions #62450

Merged

rmloveland mentioned this pull request Mar 24, 2021

Add docs for ALTER DATABASE .. SET PRIMARY REGION cockroachdb/docs#9299

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: ensure user has correct privileges when adding/removing regions #62186

sql: ensure user has correct privileges when adding/removing regions #62186

arulajmani commented Mar 18, 2021 •

edited

Loading

cockroach-teamcity commented Mar 18, 2021

otan left a comment

otan commented Mar 18, 2021 •

edited

Loading

RichardJCai Mar 18, 2021

arulajmani Mar 18, 2021

RichardJCai Mar 18, 2021 •

edited

Loading

arulajmani Mar 18, 2021

arulajmani Mar 18, 2021

RichardJCai Mar 18, 2021

arulajmani Mar 19, 2021

arulajmani Mar 19, 2021

arulajmani commented Mar 18, 2021

arulajmani commented Mar 18, 2021

arulajmani left a comment

arulajmani left a comment

RichardJCai left a comment

arulajmani left a comment

RichardJCai left a comment

arulajmani commented Mar 22, 2021

craig bot commented Mar 22, 2021

craig bot commented Mar 22, 2021

sql: ensure user has correct privileges when adding/removing regions #62186

sql: ensure user has correct privileges when adding/removing regions #62186

Conversation

arulajmani commented Mar 18, 2021 • edited Loading

cockroach-teamcity commented Mar 18, 2021

otan left a comment

Choose a reason for hiding this comment

otan commented Mar 18, 2021 • edited Loading

RichardJCai Mar 18, 2021

Choose a reason for hiding this comment

arulajmani Mar 18, 2021

Choose a reason for hiding this comment

RichardJCai Mar 18, 2021 • edited Loading

Choose a reason for hiding this comment

arulajmani Mar 18, 2021

Choose a reason for hiding this comment

arulajmani Mar 18, 2021

Choose a reason for hiding this comment

RichardJCai Mar 18, 2021

Choose a reason for hiding this comment

arulajmani Mar 19, 2021

Choose a reason for hiding this comment

arulajmani Mar 19, 2021

Choose a reason for hiding this comment

arulajmani commented Mar 18, 2021

arulajmani commented Mar 18, 2021

arulajmani left a comment

Choose a reason for hiding this comment

arulajmani left a comment

Choose a reason for hiding this comment

RichardJCai left a comment

Choose a reason for hiding this comment

arulajmani left a comment

Choose a reason for hiding this comment

RichardJCai left a comment

Choose a reason for hiding this comment

arulajmani commented Mar 22, 2021

craig bot commented Mar 22, 2021

craig bot commented Mar 22, 2021

arulajmani commented Mar 18, 2021 •

edited

Loading

otan commented Mar 18, 2021 •

edited

Loading

RichardJCai Mar 18, 2021 •

edited

Loading