sql: allow non-admins to perform some RESTOREs #53650

solongordon · 2020-08-31T00:42:51Z

Release justification: Low risk, high reward change to existing
functionality

Release note (sql change): Non-admin users are now permitted to execute
RESTORE statements as long as the restore does not depend on implicit
credentials and the user has the appropriate privileges to create all of
the resulting database objects. For database restores, this means the
user must have the CREATEDB role privilege. For table restores, the user
must have CREATE privileges on the parent database. Full cluster
restores still require admin privileges.

cockroach-teamcity · 2020-08-31T00:42:57Z

This change is

solongordon · 2020-08-31T00:43:30Z

This still needs tests but I'd appreciate an early look from a backup and restore expert.

solongordon

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt and @pbardea)

pkg/ccl/backupccl/restore_planning.go, line 1190 at r1 (raw file):

		if !hasAdmin {
			// Do not allow full cluster restores.
			if restoreStmt.DescriptorCoverage == tree.AllDescriptors {

Is this the correct way to check for a full cluster restore?

pkg/ccl/backupccl/restore_planning.go, line 1206 at r1 (raw file):

						"only users with the CREATEDB privilege can restore databases")
				}
			}

Are there other targets I should be guarding against here? Tenant perhaps?

pbardea

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @pbardea, and @solongordon)

pkg/ccl/backupccl/restore_planning.go, line 1183 at r1 (raw file):

			}
		}

Could we move this new block into a helper? Maybe something like checkAdminRoleRequired?

pkg/ccl/backupccl/restore_planning.go, line 1190 at r1 (raw file):

Previously, solongordon (Solon) wrote…

Is this the correct way to check for a full cluster restore?

Yep

pkg/ccl/backupccl/restore_planning.go, line 1206 at r1 (raw file):

Previously, solongordon (Solon) wrote…

Are there other targets I should be guarding against here? Tenant perhaps?

Guarding against Tenant restoration seems reasonable to me. What are the current privileges we require to create a tenant?

pkg/ccl/backupccl/restore_planning.go, line 1207 at r1 (raw file):

				}
			}
			// Check that none of the sources use implicit credentials.

It looks like we're disallowing more than just the use of implicit credentials, but more generally implicit access all-together (e.g. nodelocal, http endpoints, ...).

Since that's the case, maybe we can update the comment to read "implicit access"?

dt

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt and @solongordon)

pkg/ccl/backupccl/restore_planning.go, line 1206 at r1 (raw file):

Previously, pbardea (Paul Bardea) wrote…

Guarding against Tenant restoration seems reasonable to me. What are the current privileges we require to create a tenant?

tenant should require admin and require sql codec is system tenant i think

solongordon

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt and @pbardea)

pkg/ccl/backupccl/restore_planning.go, line 1183 at r1 (raw file):

Previously, pbardea (Paul Bardea) wrote…

Could we move this new block into a helper? Maybe something like checkAdminRoleRequired?

Done.

pkg/ccl/backupccl/restore_planning.go, line 1206 at r1 (raw file):

Previously, dt (David Taylor) wrote…

tenant should require admin and require sql codec is system tenant i think

I added the admin check. Can I leave the codec check to you all?

pkg/ccl/backupccl/restore_planning.go, line 1207 at r1 (raw file):

Previously, pbardea (Paul Bardea) wrote…

It looks like we're disallowing more than just the use of implicit credentials, but more generally implicit access all-together (e.g. nodelocal, http endpoints, ...).

Since that's the case, maybe we can update the comment to read "implicit access"?

Done.

solongordon · 2020-09-01T17:42:12Z

Addressed comments and added tests. PTAL.

pbardea

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @pbardea, and @solongordon)

pkg/ccl/backupccl/restore_planning.go, line 1237 at r2 (raw file):

		return err
	}
	if !hasAdmin {

optional nit/feel free to ignore: I have a slight readability preference of short circuiting here with:

if hasAdmin {
  return nil
}

to reduce nesting

pkg/ccl/backupccl/testdata/backup-restore/permissions, line 88 at r2 (raw file):

RESTORE TABLE d.t FROM 'nodelocal://0/test/'
----
pq: only users with the admin role are allowed to RESTORE from the specified nodelocal URI

nit: newline at end of file

pkg/sql/exec_util.go, line 855 at r2 (raw file):

for tenant behavior

is this correct? I'm not seeing how this knob relates to tenancy?

pkg/sql/exec_util.go, line 859 at r2 (raw file):

	// AllowImplicitAccess allows implicit access to data sources for non-admin
	// users.
	AllowImplicitAccess bool

Hmm, what is this knob allowing us to test? Is it possible to create a non-admin user that should be allowed implicit access to data sources? My understanding was that only admins would be allow that.

solongordon

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt and @pbardea)

pkg/ccl/backupccl/restore_planning.go, line 1237 at r2 (raw file):

Previously, pbardea (Paul Bardea) wrote…

optional nit/feel free to ignore: I have a slight readability preference of short circuiting here with:
if hasAdmin {
  return nil
}
to reduce nesting

Me too, missed this when I made it a helper. Done.

pkg/sql/exec_util.go, line 855 at r2 (raw file):

Previously, pbardea (Paul Bardea) wrote…

for tenant behavior

is this correct? I'm not seeing how this knob relates to tenancy?

Oops, copy pasta. Done.

pkg/sql/exec_util.go, line 859 at r2 (raw file):

Previously, pbardea (Paul Bardea) wrote…

Hmm, what is this knob allowing us to test? Is it possible to create a non-admin user that should be allowed implicit access to data sources? My understanding was that only admins would be allow that.

Right, normally only admins are allowed implicit access. The problem is this prevents me from testing that non-admins can run RESTOREs, because normally they would not have access to nodelocal storage, which the tests rely on. The testing knob lets me disable the implicit access check so I can test the other checks, like that you need CREATEDB privileges to restore a database.

pbardea

LGTM mod last 2 minor comments.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @pbardea, and @solongordon)

pkg/ccl/backupccl/restore_planning.go, line 1265 at r3 (raw file):

	}
	knobs := p.ExecCfg().BackupRestoreTestingKnobs
	if knobs != nil && !knobs.AllowImplicitAccess {

nit: I'm a bit concerned that if we remove the knobs then we'll stop doing this check, what aboutknobs == nil || !knobs.AllowImplicitAccess?

pkg/sql/exec_util.go, line 859 at r2 (raw file):

Previously, solongordon (Solon) wrote…

Right, normally only admins are allowed implicit access. The problem is this prevents me from testing that non-admins can run RESTOREs, because normally they would not have access to nodelocal storage, which the tests rely on. The testing knob lets me disable the implicit access check so I can test the other checks, like that you need CREATEDB privileges to restore a database.

Ah - gotcha. A small comment saying that this enables use to test database permissions as a non-admin user while using nodelocal which would be reserved for only admins (outside a test).

Release justification: Low risk, high reward change to existing functionality Release note (sql change): Non-admin users are now permitted to execute RESTORE statements as long as the restore does not depend on implicit credentials and the user has the appropriate privileges to create all of the resulting database objects. For database restores, this means the user must have the CREATEDB role privilege. For table restores, the user must have CREATE privileges on the parent database. Full cluster restores still require admin privileges.

solongordon

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt and @pbardea)

pkg/ccl/backupccl/restore_planning.go, line 1265 at r3 (raw file):

Previously, pbardea (Paul Bardea) wrote…

nit: I'm a bit concerned that if we remove the knobs then we'll stop doing this check, what aboutknobs == nil || !knobs.AllowImplicitAccess?

Oh god that's not a nit, it's just broken since knobs will normally be nil. Good catch.

pkg/sql/exec_util.go, line 859 at r2 (raw file):

Previously, pbardea (Paul Bardea) wrote…

Ah - gotcha. A small comment saying that this enables use to test database permissions as a non-admin user while using nodelocal which would be reserved for only admins (outside a test).

Done.

solongordon · 2020-09-02T13:27:53Z

bors r+

craig · 2020-09-02T14:08:06Z

Build succeeded:

GitHub CI (Cockroach)

solongordon requested review from dt, pbardea and a team August 31, 2020 00:42

solongordon commented Aug 31, 2020

View reviewed changes

pbardea reviewed Aug 31, 2020

View reviewed changes

dt reviewed Aug 31, 2020

View reviewed changes

solongordon force-pushed the restore-non-admin branch from 8e79f98 to 696c82e Compare September 1, 2020 17:41

solongordon commented Sep 1, 2020

View reviewed changes

solongordon requested review from dt and pbardea September 1, 2020 17:42

pbardea reviewed Sep 1, 2020

View reviewed changes

solongordon commented Sep 1, 2020

View reviewed changes

solongordon force-pushed the restore-non-admin branch from 696c82e to 5cfa39d Compare September 1, 2020 20:35

solongordon requested a review from pbardea September 1, 2020 20:35

solongordon force-pushed the restore-non-admin branch from 5cfa39d to c8fc49e Compare September 1, 2020 20:35

pbardea approved these changes Sep 1, 2020

View reviewed changes

solongordon force-pushed the restore-non-admin branch from c8fc49e to cffe8b0 Compare September 1, 2020 21:47

solongordon commented Sep 1, 2020

View reviewed changes

craig bot merged commit 9e4387f into cockroachdb:master Sep 2, 2020

solongordon deleted the restore-non-admin branch September 2, 2020 14:27

solongordon mentioned this pull request Sep 8, 2020

sql: allow non-admin users to RESTORE #52581

Closed

pbardea mentioned this pull request Sep 14, 2020

backupccl: check privileges on types and schemas during backup #54183

Merged

jseldess mentioned this pull request Sep 15, 2020

sql: allow non-admins to perform some RESTOREs cockroachdb/docs#8349

Closed

lnhsingh mentioned this pull request Nov 3, 2020

permissions for bulk features: Changes in 20.2 cockroachdb/docs#8432

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: allow non-admins to perform some RESTOREs #53650

sql: allow non-admins to perform some RESTOREs #53650

solongordon commented Aug 31, 2020

cockroach-teamcity commented Aug 31, 2020

solongordon commented Aug 31, 2020

solongordon left a comment

pbardea left a comment

dt left a comment

solongordon left a comment

solongordon commented Sep 1, 2020

pbardea left a comment

solongordon left a comment

pbardea left a comment

solongordon left a comment

solongordon commented Sep 2, 2020

craig bot commented Sep 2, 2020

sql: allow non-admins to perform some RESTOREs #53650

sql: allow non-admins to perform some RESTOREs #53650

Conversation

solongordon commented Aug 31, 2020

cockroach-teamcity commented Aug 31, 2020

solongordon commented Aug 31, 2020

solongordon left a comment

Choose a reason for hiding this comment

pbardea left a comment

Choose a reason for hiding this comment

dt left a comment

Choose a reason for hiding this comment

solongordon left a comment

Choose a reason for hiding this comment

solongordon commented Sep 1, 2020

pbardea left a comment

Choose a reason for hiding this comment

solongordon left a comment

Choose a reason for hiding this comment

pbardea left a comment

Choose a reason for hiding this comment

solongordon left a comment

Choose a reason for hiding this comment

solongordon commented Sep 2, 2020

craig bot commented Sep 2, 2020