Skip to content

Commit

Permalink
Merge #86632 #86738 #86918
Browse files Browse the repository at this point in the history 
86632: gen: add schemachanger to code generation target r=Xiang-Gu a=ajwerner

Before this, dev gen go did not regenerate schemachanger code.

Release justification: build tooling only change.

Release note: None

86738: bulk: fix incorrect memory handling in the kvBuf r=stevendanna a=adityamaru

This change fixes a memory accounting bug where
we were appending a KV to the kvBuf without accounting
for its memory.

Release note (bug fix): adds a missing memory accounting
call when appending a KV to the underlying kvBuf

Release justification: low risk bug fix that prevents an
import from panicking on non-release builds

86918: privilege,backupccl: introduce a RESTORE privilege r=stevendanna a=adityamaru

This change introduces a RESTORE privilege that is grantable
as a system, or database level privilege. The purpose
of this privilege is to offer a fine-grained permission model
for users that wish to run restores. First let us outline the
existing privilege model that governs backups:

Cluster restores - admin only.

Database restores - users must have CREATEDB role option.

Table restores - users must have CREATE on the database we
are restoring into.

With the new fine grained permission model we would like to stop
having to grant users privileges such as CREATE and CREATEDB only
for the purpose of being able to restore a target. To this effect
the new privilege model that will govern restores is:

Cluster backups - user requires the system RESTORE privilege

Database backups - user requires the system RESTORE privilege

Table backups - user requires the database RESTORE privilege

Note, admins will ofcourse continue to bypass all these checks.
This diff does not change the privilege checks we perform on the backup
destination URIs related to IMPLICIT authentication. That will be done
as a follow-up.

In 22.2 to prevent breaking user flows we will continue to respect the
old privilege model, but emit a notice indicating our plans to replace
this model in 23.1. At which point users will need to be granted the
appropriate BACKUP privileges.

Informs: #86263

Release note (sql change): This change introduces a new RESTORE
privilege that is grantable as a system or database level privilege.
Users can opt-in to the new privilege model by granting the appropriate
privileges as per the following model:

Cluster backups - user requires the system RESTORE privilege

Database backups - user requires the system RESTORE privilege

Table backups - user requires the database RESTORE privilege

In 22.2 we will continue to respect the old privilege model, but will
completely swithover to the RESTORE privilege in 23.1.

Release justification: high impact change to offer fine grained privileges
for bulk operations

Co-authored-by: Andrew Werner <[email protected]>
Co-authored-by: adityamaru <[email protected]>
  • Loading branch information
@ajwerner @adityamaru
3 people committed Aug 29, 2022
4 parents cb57def + 1443da7 + f3f5cc5 + 9f447c5 commit 00aa1c4
Show file tree
Hide file tree
Showing 19 changed files with 448 additions and 129 deletions.
19 changes: 10 additions & 9 deletions pkg/ccl/backupccl/backup_planning.go
View file
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 @@ -354,10 +357,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 @@ -367,8 +370,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 @@ -380,7 +381,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 @@ -393,7 +394,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 @@ -495,7 +496,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
173 changes: 137 additions & 36 deletions pkg/ccl/backupccl/restore_planning.go
View file
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 @@ -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
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

0 comments on commit 00aa1c4

Please sign in to comment.