Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

privilege,backupccl: introduce a RESTORE privilege #86918

Merged
merged 1 commit into from
Aug 29, 2022
Merged

privilege,backupccl: introduce a RESTORE privilege #86918

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 10 additions & 9 deletions pkg/ccl/backupccl/backup_planning.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,9 @@ const (
// backupPartitionDescriptorPrefix is the file name prefix for serialized
// BackupPartitionDescriptor protos.
backupPartitionDescriptorPrefix = "BACKUP_PART"

deprecatedPrivilegesPreamble = "The existing privileges are being deprecated " +
"in favour of a fine-grained privilege model explained here <link>. In a future release, to run"
)

type tableAndIndex struct {
Expand Down Expand Up @@ -353,10 +356,10 @@ func checkBackupDestinationPrivileges(ctx context.Context, p sql.PlanHookState,
return nil
}

// privilegesDeprecationNotice returns a notice that outlines the deprecation of
// the existing privilege model for backups, and the steps to take to adopt the
// new privilege model, based on the type of backup being run.
func privilegesDeprecationNotice(
// backupPrivilegesDeprecationNotice returns a notice that outlines the
// deprecation of the existing privilege model for backups, and the steps to
// take to adopt the new privilege model, based on the type of backup being run.
func backupPrivilegesDeprecationNotice(
p sql.PlanHookState, backupStmt *annotatedBackupStatement, targetDescs []catalog.Descriptor,
) string {
if backupStmt.Targets == nil {
Expand All @@ -366,8 +369,6 @@ func privilegesDeprecationNotice(
// If a user is running `BACKUP DATABASE` buffer all the databases that will
// require the `BACKUP` privilege.
var notice string
preamble := "The existing privileges required for backup are being deprecated " +
"in favour of a fine-grained privilege model explained here <link>. In a future release, to run"
if backupStmt.Targets.Databases != nil {
dbsRequireBackupPrivilege := make([]string, 0)
for _, desc := range targetDescs {
Expand All @@ -379,7 +380,7 @@ func privilegesDeprecationNotice(

notice = fmt.Sprintf("%s BACKUP DATABASE, user %s will exclusively require the "+
"BACKUP privilege on database %s.",
preamble, p.User().Normalized(), strings.Join(dbsRequireBackupPrivilege, ", "))
deprecatedPrivilegesPreamble, p.User().Normalized(), strings.Join(dbsRequireBackupPrivilege, ", "))
} else if backupStmt.Targets.Tables.TablePatterns != nil {
// If a user is running `BACKUP TABLE` buffer all the tables that will require the `BACKUP` privilege.
tablesRequireBackupPrivilege := make([]string, 0)
Expand All @@ -392,7 +393,7 @@ func privilegesDeprecationNotice(

notice = fmt.Sprintf("%s BACKUP TABLE, user %s will exclusively require the "+
"BACKUP privilege on tables: %s.",
preamble,
deprecatedPrivilegesPreamble,
p.User().Normalized(),
strings.Join(tablesRequireBackupPrivilege, ", "))
}
Expand Down Expand Up @@ -494,7 +495,7 @@ func checkPrivilegesForBackup(
//
// TODO(adityamaru): Delete deprecated privilege checks in 23.1. Users will be
// required to have the appropriate `BACKUP` privilege instead.
notice := privilegesDeprecationNotice(p, backupStmt, targetDescs)
notice := backupPrivilegesDeprecationNotice(p, backupStmt, targetDescs)
p.BufferClientNotice(ctx, pgnotice.Newf("%s", notice))

for _, desc := range targetDescs {
Expand Down
177 changes: 139 additions & 38 deletions pkg/ccl/backupccl/restore_planning.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,9 @@ func allocateDescriptorRewrites(
}
}

var shouldBufferDeprecatedPrivilegeNotice bool
databasesWithDeprecatedPrivileges := make(map[string]struct{})

// Fail fast if the necessary databases don't exist or are otherwise
// incompatible with this restore.
if err := sql.DescsTxn(ctx, p.ExecCfg(), func(ctx context.Context, txn *kv.Txn, col *descs.Collection) error {
Expand Down Expand Up @@ -309,8 +312,11 @@ func allocateDescriptorRewrites(
if err != nil {
return err
}
if err := p.CheckPrivilege(ctx, parentDB, privilege.CREATE); err != nil {
if usesDeprecatedPrivileges, err := checkRestorePrivilegesOnDatabase(ctx, p, parentDB); err != nil {
return err
} else if usesDeprecatedPrivileges {
shouldBufferDeprecatedPrivilegeNotice = true
databasesWithDeprecatedPrivileges[parentDB.GetName()] = struct{}{}
}

// See if there is an existing schema with the same name.
Expand Down Expand Up @@ -378,8 +384,11 @@ func allocateDescriptorRewrites(
return errors.Wrapf(err,
"failed to lookup parent DB %d", errors.Safe(parentID))
}
if err := p.CheckPrivilege(ctx, parentDB, privilege.CREATE); err != nil {
if usesDeprecatedPrivileges, err := checkRestorePrivilegesOnDatabase(ctx, p, parentDB); err != nil {
return err
} else if usesDeprecatedPrivileges {
shouldBufferDeprecatedPrivilegeNotice = true
databasesWithDeprecatedPrivileges[parentDB.GetName()] = struct{}{}
}

// We're restoring a table and not its parent database. We may block
Expand Down Expand Up @@ -465,8 +474,11 @@ func allocateDescriptorRewrites(
// need to create the type.

// Ensure that the user has the correct privilege to create types.
if err := p.CheckPrivilege(ctx, parentDB, privilege.CREATE); err != nil {
if usesDeprecatedPrivileges, err := checkRestorePrivilegesOnDatabase(ctx, p, parentDB); err != nil {
return err
} else if usesDeprecatedPrivileges {
shouldBufferDeprecatedPrivilegeNotice = true
databasesWithDeprecatedPrivileges[parentDB.GetName()] = struct{}{}
}

// Create a rewrite entry for the type.
Expand Down Expand Up @@ -531,6 +543,15 @@ func allocateDescriptorRewrites(
return nil, err
}

if shouldBufferDeprecatedPrivilegeNotice {
dbNames := make([]string, 0, len(databasesWithDeprecatedPrivileges))
for dbName := range databasesWithDeprecatedPrivileges {
dbNames = append(dbNames, dbName)
}
p.BufferClientNotice(ctx, pgnotice.Newf("%s RESTORE TABLE, user %s will exclusively require the RESTORE privilege on databases %s",
deprecatedPrivilegesPreamble, p.User(), strings.Join(dbNames, ", ")))
}

// Allocate new IDs for each database and table.
//
// NB: we do this in a standalone transaction, not one that covers the
Expand Down Expand Up @@ -653,12 +674,12 @@ func getDatabaseIDAndDesc(
func dropDefaultUserDBs(ctx context.Context, execCfg *sql.ExecutorConfig) error {
return sql.DescsTxn(ctx, execCfg, func(ctx context.Context, txn *kv.Txn, col *descs.Collection) error {
ie := execCfg.InternalExecutor
_, err := ie.Exec(ctx, "drop-defaultdb", nil, "DROP DATABASE IF EXISTS defaultdb")
_, err := ie.Exec(ctx, "drop-defaultdb", txn, "DROP DATABASE IF EXISTS defaultdb")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah...going to pull this one out maybe or backport it for sure. Possibly related thread - https://cockroachlabs.slack.com/archives/C2C5FKPPB/p1661202884691609

if err != nil {
return err
}

_, err = ie.Exec(ctx, "drop-postgres", nil, "DROP DATABASE IF EXISTS postgres")
_, err = ie.Exec(ctx, "drop-postgres", txn, "DROP DATABASE IF EXISTS postgres")
if err != nil {
return err
}
Expand Down Expand Up @@ -1101,40 +1122,11 @@ func restorePlanHook(
return fn, jobs.BulkJobExecutionResultHeader, nil, false, nil
}

func checkPrivilegesForRestore(
ctx context.Context, restoreStmt *tree.Restore, p sql.PlanHookState, from [][]string,
// checkRestoreDestinationPrivileges iterates over the External Storage URIs and
// ensures the user has adequate privileges to use each of them.
func checkRestoreDestinationPrivileges(
ctx context.Context, p sql.PlanHookState, from [][]string,
) error {
hasAdmin, err := p.HasAdminRole(ctx)
if err != nil {
return err
}
if hasAdmin {
return nil
}
// Do not allow full cluster restores.
if restoreStmt.DescriptorCoverage == tree.AllDescriptors {
return pgerror.Newf(
pgcode.InsufficientPrivilege,
"only users with the admin role are allowed to restore full cluster backups")
}
// Do not allow tenant restores.
if restoreStmt.Targets.TenantID.IsSet() {
return pgerror.Newf(
pgcode.InsufficientPrivilege,
"only users with the admin role can perform RESTORE TENANT")
}
// Database restores require the CREATEDB privileges.
if len(restoreStmt.Targets.Databases) > 0 {
hasCreateDB, err := p.HasRoleOption(ctx, roleoption.CREATEDB)
if err != nil {
return err
}
if !hasCreateDB {
return pgerror.Newf(
pgcode.InsufficientPrivilege,
"only users with the CREATEDB privilege can restore databases")
}
}
if p.ExecCfg().ExternalIODirConfig.EnableNonAdminImplicitAndArbitraryOutbound {
return nil
}
Expand Down Expand Up @@ -1172,9 +1164,118 @@ func checkPrivilegesForRestore(
}
}
}

return nil
}

// checkRestorePrivilegesOnDatabase check that the user has adequate privileges
// on the parent database to restore schema objects into the database. This is
// used to check the privileges required for a `RESTORE TABLE`.
func checkRestorePrivilegesOnDatabase(
ctx context.Context, p sql.PlanHookState, parentDB catalog.DatabaseDescriptor,
) (shouldBufferNotice bool, err error) {
if err := p.CheckPrivilege(ctx, parentDB, privilege.RESTORE); err == nil {
return false, nil
}

if err := p.CheckPrivilege(ctx, parentDB, privilege.CREATE); err != nil {
notice := fmt.Sprintf("%s RESTORE TABLE, user %s will exclusively require the "+
"RESTORE privilege on database %s.", deprecatedPrivilegesPreamble, p.User().Normalized(), parentDB.GetName())
p.BufferClientNotice(ctx, pgnotice.Newf("%s", notice))
return false, errors.WithHint(err, notice)
}

return true, nil
}

// checkPrivilegesForRestore checks that the user has sufficient privileges to
// run a cluster or a database restore. A table restore requires us to know the
// parent database we will be writing to and so that happens at a later stage of
// restore planning.
//
// This method is also responsible for checking the privileges on the
// destination URIs the restore is reading from.
func checkPrivilegesForRestore(
ctx context.Context, restoreStmt *tree.Restore, p sql.PlanHookState, from [][]string,
) error {
// If the user is admin no further checks need to be performed.
hasAdmin, err := p.HasAdminRole(ctx)
if err != nil {
return err
}
if hasAdmin {
return nil
}

{
// Cluster and tenant restores require the `RESTORE` system privilege for
// non-admin users.
requiresRestoreSystemPrivilege := restoreStmt.DescriptorCoverage == tree.AllDescriptors ||
restoreStmt.Targets.TenantID.IsSet()

var hasRestoreSystemPrivilege bool
if p.ExecCfg().Settings.Version.IsActive(ctx, clusterversion.SystemPrivilegesTable) {
err := p.CheckPrivilegeForUser(ctx, syntheticprivilege.GlobalPrivilegeObject,
privilege.RESTORE, p.User())
hasRestoreSystemPrivilege = err == nil
}

if requiresRestoreSystemPrivilege && hasRestoreSystemPrivilege {
return checkRestoreDestinationPrivileges(ctx, p, from)
} else if requiresRestoreSystemPrivilege && !hasRestoreSystemPrivilege {
return pgerror.Newf(pgcode.InsufficientPrivilege,
"only users with the admin role or the RESTORE system privilege are allowed to perform"+
" a cluster restore")
}
}

var hasRestoreSystemPrivilege bool
// If running a database restore, check that the user has the `RESTORE` system
// privilege.
//
// TODO(adityamaru): In 23.1 a missing `RESTORE` privilege should return an
// error. In 22.2 we continue to check for old style privileges and role
// options.
if len(restoreStmt.Targets.Databases) > 0 {
if p.ExecCfg().Settings.Version.IsActive(ctx, clusterversion.SystemPrivilegesTable) {
err := p.CheckPrivilegeForUser(ctx, syntheticprivilege.GlobalPrivilegeObject,
privilege.RESTORE, p.User())
hasRestoreSystemPrivilege = err == nil
}
}

if hasRestoreSystemPrivilege {
return checkRestoreDestinationPrivileges(ctx, p, from)
}

// The following checks are to maintain compatability with pre-22.2 privilege
// requirements to run the backup. If we have failed to find the appropriate
// `RESTORE` privileges, we default to our old-style privilege checks and
// buffer a notice urging users to switch to `RESTORE` privileges.
//
// TODO(adityamaru): Delete deprecated privilege checks in 23.1. Users will be
// required to have the appropriate `RESTORE` privilege instead.
//
// Database restores require the CREATEDB privileges.
if len(restoreStmt.Targets.Databases) > 0 {
notice := fmt.Sprintf("%s RESTORE DATABASE, user %s will exclusively require the "+
"RESTORE system privilege.", deprecatedPrivilegesPreamble, p.User().Normalized())
p.BufferClientNotice(ctx, pgnotice.Newf("%s", notice))

hasCreateDB, err := p.HasRoleOption(ctx, roleoption.CREATEDB)
if err != nil {
return err
}
if !hasCreateDB {
return errors.WithHint(pgerror.Newf(
pgcode.InsufficientPrivilege,
"only users with the CREATEDB privilege can restore databases"), notice)
}
}

return checkRestoreDestinationPrivileges(ctx, p, from)
}

func checkClusterRegions(
ctx context.Context, p sql.PlanHookState, typesByID map[descpb.ID]*typedesc.Mutable,
) error {
Expand Down
16 changes: 8 additions & 8 deletions pkg/ccl/backupccl/testdata/backup-restore/backup-permissions
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,13 +63,13 @@ exec-sql user=testuser
BACKUP DATABASE d INTO 'userfile:///test-nonroot-db';
----
pq: user testuser does not have SELECT privilege on relation t
HINT: The existing privileges required for backup are being deprecated in favour of a fine-grained privilege model explained here <link>. In a future release, to run BACKUP DATABASE, user testuser will exclusively require the BACKUP privilege on database d.
HINT: The existing privileges are being deprecated in favour of a fine-grained privilege model explained here <link>. In a future release, to run BACKUP DATABASE, user testuser will exclusively require the BACKUP privilege on database d.

exec-sql user=testuser
BACKUP TABLE d.t INTO 'userfile:///test-nonroot-table';
----
pq: user testuser does not have SELECT privilege on relation t
HINT: The existing privileges required for backup are being deprecated in favour of a fine-grained privilege model explained here <link>. In a future release, to run BACKUP TABLE, user testuser will exclusively require the BACKUP privilege on tables: t.
HINT: The existing privileges are being deprecated in favour of a fine-grained privilege model explained here <link>. In a future release, to run BACKUP TABLE, user testuser will exclusively require the BACKUP privilege on tables: t.

exec-sql
REVOKE SYSTEM BACKUP FROM testuser;
Expand All @@ -87,7 +87,7 @@ exec-sql user=testuser
BACKUP DATABASE d INTO 'userfile:///test-nonroot-db'
----
pq: user testuser does not have SELECT privilege on relation t
HINT: The existing privileges required for backup are being deprecated in favour of a fine-grained privilege model explained here <link>. In a future release, to run BACKUP DATABASE, user testuser will exclusively require the BACKUP privilege on database d.
HINT: The existing privileges are being deprecated in favour of a fine-grained privilege model explained here <link>. In a future release, to run BACKUP DATABASE, user testuser will exclusively require the BACKUP privilege on database d.

# Grant the user `BACKUP` on the database.
exec-sql
Expand All @@ -111,7 +111,7 @@ exec-sql user=testuser
BACKUP TABLE d.t INTO 'userfile:///test-nonroot-table'
----
pq: user testuser does not have SELECT privilege on relation t
HINT: The existing privileges required for backup are being deprecated in favour of a fine-grained privilege model explained here <link>. In a future release, to run BACKUP TABLE, user testuser will exclusively require the BACKUP privilege on tables: t.
HINT: The existing privileges are being deprecated in favour of a fine-grained privilege model explained here <link>. In a future release, to run BACKUP TABLE, user testuser will exclusively require the BACKUP privilege on tables: t.

# Set the default privileges on the database and create a new table.
exec-sql
Expand Down Expand Up @@ -160,19 +160,19 @@ exec-sql user=testuser
BACKUP TABLE d.t INTO 'userfile:///test-nonroot-table'
----
pq: user testuser does not have SELECT privilege on relation t
HINT: The existing privileges required for backup are being deprecated in favour of a fine-grained privilege model explained here <link>. In a future release, to run BACKUP TABLE, user testuser will exclusively require the BACKUP privilege on tables: t.
HINT: The existing privileges are being deprecated in favour of a fine-grained privilege model explained here <link>. In a future release, to run BACKUP TABLE, user testuser will exclusively require the BACKUP privilege on tables: t.

exec-sql user=testuser
BACKUP TABLE d.t2 INTO 'userfile:///test-nonroot-table'
----
pq: user testuser does not have SELECT privilege on relation t2
HINT: The existing privileges required for backup are being deprecated in favour of a fine-grained privilege model explained here <link>. In a future release, to run BACKUP TABLE, user testuser will exclusively require the BACKUP privilege on tables: t2.
HINT: The existing privileges are being deprecated in favour of a fine-grained privilege model explained here <link>. In a future release, to run BACKUP TABLE, user testuser will exclusively require the BACKUP privilege on tables: t2.

exec-sql user=testuser
BACKUP TABLE d.t3 INTO 'userfile:///test-nonroot-table'
----
pq: user testuser does not have SELECT privilege on relation t3
HINT: The existing privileges required for backup are being deprecated in favour of a fine-grained privilege model explained here <link>. In a future release, to run BACKUP TABLE, user testuser will exclusively require the BACKUP privilege on tables: t3.
HINT: The existing privileges are being deprecated in favour of a fine-grained privilege model explained here <link>. In a future release, to run BACKUP TABLE, user testuser will exclusively require the BACKUP privilege on tables: t3.

exec-sql
GRANT BACKUP ON TABLE d.t,d.t2,d.t3 TO testuser;
Expand Down Expand Up @@ -200,7 +200,7 @@ exec-sql user=testuser
BACKUP TABLE d.foo.t1 INTO 'userfile:///test-nonroot-table'
----
pq: user testuser does not have USAGE privilege on schema foo
HINT: The existing privileges required for backup are being deprecated in favour of a fine-grained privilege model explained here <link>. In a future release, to run BACKUP TABLE, user testuser will exclusively require the BACKUP privilege on tables: t1.
HINT: The existing privileges are being deprecated in favour of a fine-grained privilege model explained here <link>. In a future release, to run BACKUP TABLE, user testuser will exclusively require the BACKUP privilege on tables: t1.

exec-sql
GRANT BACKUP ON TABLE foo.t1 TO testuser;
Expand Down
Loading