From 085006acea03a446d5039d009e691654490e00d0 Mon Sep 17 00:00:00 2001 From: Rafi Shamim Date: Wed, 13 Sep 2023 16:17:21 -0400 Subject: [PATCH 1/2] privilege: add REPAIRCLUSTERMETADATA system prvilege Release note: None --- pkg/sql/privilege/kind_string.go | 5 ++++- pkg/sql/privilege/privilege.go | 4 +++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/pkg/sql/privilege/kind_string.go b/pkg/sql/privilege/kind_string.go index 281472f74417..ad1b6f42752b 100644 --- a/pkg/sql/privilege/kind_string.go +++ b/pkg/sql/privilege/kind_string.go @@ -43,7 +43,8 @@ func _() { _ = x[CREATELOGIN-33] _ = x[CREATEDB-34] _ = x[CONTROLJOB-35] - _ = x[largestKind-35] + _ = x[REPAIRCLUSTERMETADATA-36] + _ = x[largestKind-36] } func (i Kind) String() string { @@ -118,6 +119,8 @@ func (i Kind) String() string { return "CREATEDB" case CONTROLJOB: return "CONTROLJOB" + case REPAIRCLUSTERMETADATA: + return "REPAIRCLUSTERMETADATA" default: return "Kind(" + strconv.FormatInt(int64(i), 10) + ")" } diff --git a/pkg/sql/privilege/privilege.go b/pkg/sql/privilege/privilege.go index 26929eb5e093..e568faa4bd9a 100644 --- a/pkg/sql/privilege/privilege.go +++ b/pkg/sql/privilege/privilege.go @@ -76,7 +76,8 @@ const ( CREATELOGIN Kind = 33 CREATEDB Kind = 34 CONTROLJOB Kind = 35 - largestKind = CONTROLJOB + REPAIRCLUSTERMETADATA Kind = 36 + largestKind = REPAIRCLUSTERMETADATA ) var isDeprecatedKind = map[Kind]bool{ @@ -165,6 +166,7 @@ var ( ALL, BACKUP, RESTORE, MODIFYCLUSTERSETTING, EXTERNALCONNECTION, VIEWACTIVITY, VIEWACTIVITYREDACTED, VIEWCLUSTERSETTING, CANCELQUERY, NOSQLLOGIN, VIEWCLUSTERMETADATA, VIEWDEBUG, EXTERNALIOIMPLICITACCESS, VIEWJOB, MODIFYSQLCLUSTERSETTING, REPLICATION, MANAGETENANT, VIEWSYSTEMTABLE, CREATEROLE, CREATELOGIN, CREATEDB, CONTROLJOB, + REPAIRCLUSTERMETADATA, } VirtualTablePrivileges = List{ALL, SELECT} ExternalConnectionPrivileges = List{ALL, USAGE, DROP} From 307136e38e5358c43254c63e81e47b8a040d2b7d Mon Sep 17 00:00:00 2001 From: Rafi Shamim Date: Wed, 13 Sep 2023 16:50:29 -0400 Subject: [PATCH 2/2] *: eliminate usages of planner.RequireAdminRole All the usages are replaced by checks for VIEWCLUSTERMETADATA or REPAIRCLUSTERMETADATA. admins implicitly have this privilege, so this should not break any existing usage. Some exceptions were made, since the focus of this change is to use this privilege for debug/repair endpoints. The following are still gated by the admin role: - ALTER ROLE on an admin - ALTER DEFAULT PRIVILEGES FOR ALL ROLES - changing a backup schedule metric Release note (sql change): SQL commands that were previously only usable by users with the admin role can now be used by users with the VIEWCLUSTERMETADATA or REPAIRCLUSTERMETADATA system privilege, depending on whether the operation is read-only or modifies state. --- pkg/ccl/backupccl/alter_backup_schedule.go | 7 ++-- pkg/ccl/backupccl/create_scheduled_backup.go | 7 +++- .../alter-schedule/schedule-options | 2 +- .../testdata/logic_test/crdb_internal | 2 +- .../testdata/logic_test/crdb_internal_tenant | 16 ++++----- .../testdata/logic_test/tenant_usage | 2 +- pkg/ccl/logictestccl/testdata/logic_test/zone | 4 +-- pkg/server/privchecker/api.go | 2 +- pkg/sql/alter_default_privileges.go | 5 ++- pkg/sql/alter_role.go | 23 +++++++++++-- pkg/sql/authorization.go | 21 ------------ pkg/sql/copy_file_upload.go | 7 +++- pkg/sql/crdb_internal.go | 31 ++++++++--------- pkg/sql/delegate/show_all_cluster_settings.go | 5 +-- pkg/sql/gossip.go | 9 +++-- .../alter_default_privileges_for_all_roles | 2 +- .../testdata/logic_test/alter_role_set | 2 +- .../testdata/logic_test/builtin_function | 4 +-- .../testdata/logic_test/crdb_internal | 18 +++++----- pkg/sql/logictest/testdata/logic_test/role | 2 +- .../testdata/logic_test/tenant_builtins | 2 +- pkg/sql/opt/cat/catalog.go | 4 --- pkg/sql/opt/optbuilder/BUILD.bazel | 1 + pkg/sql/opt/optbuilder/alter_range.go | 4 ++- pkg/sql/opt/testutils/testcat/BUILD.bazel | 1 + pkg/sql/opt/testutils/testcat/test_catalog.go | 8 ++--- pkg/sql/opt_catalog.go | 5 --- pkg/sql/repair.go | 5 +-- pkg/sql/scrub.go | 4 ++- pkg/sql/sem/builtins/BUILD.bazel | 1 + pkg/sql/sem/builtins/generator_builtins.go | 34 ++++++------------- .../sem/builtins/generator_probe_ranges.go | 17 ++++------ pkg/sql/set_zone_config.go | 7 ++-- pkg/sql/tenant_accessors.go | 4 ++- pkg/sql/tenant_gc.go | 4 ++- pkg/sql/tenant_settings.go | 7 ++-- pkg/sql/tenant_update.go | 4 ++- pkg/sql/tests/copy_file_upload_test.go | 2 +- 38 files changed, 142 insertions(+), 143 deletions(-) diff --git a/pkg/ccl/backupccl/alter_backup_schedule.go b/pkg/ccl/backupccl/alter_backup_schedule.go index ff0c5e5f87c3..bcd236ea0a6c 100644 --- a/pkg/ccl/backupccl/alter_backup_schedule.go +++ b/pkg/ccl/backupccl/alter_backup_schedule.go @@ -299,8 +299,11 @@ func processScheduleOptions( // NB: as of 20.2, schedule creation requires admin so this is duplicative // but in the future we might relax so you can schedule anything that you // can backup, but then this cluster-wide metric should be admin-only. - if err := p.RequireAdminRole(ctx, optUpdatesLastBackupMetric); err != nil { - return pgerror.Wrap(err, pgcode.InsufficientPrivilege, "") + if hasAdmin, err := p.HasAdminRole(ctx); err != nil { + return err + } else if !hasAdmin { + return pgerror.Newf(pgcode.InsufficientPrivilege, + "only users with the admin role are allowed to change %s", optUpdatesLastBackupMetric) } updatesLastBackupMetric, err := strconv.ParseBool(v) diff --git a/pkg/ccl/backupccl/create_scheduled_backup.go b/pkg/ccl/backupccl/create_scheduled_backup.go index 09c8e6529c48..b5e0146b4079 100644 --- a/pkg/ccl/backupccl/create_scheduled_backup.go +++ b/pkg/ccl/backupccl/create_scheduled_backup.go @@ -27,6 +27,8 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql" "github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo" "github.com/cockroachdb/cockroach/pkg/sql/exprutil" + "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" + "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgnotice" "github.com/cockroachdb/cockroach/pkg/sql/sem/eval" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" @@ -286,8 +288,11 @@ func doCreateBackupSchedules( // NB: as of 20.2, schedule creation requires admin so this is duplicative // but in the future we might relax so you can schedule anything that you // can backup, but then this cluster-wide metric should be admin-only. - if err := p.RequireAdminRole(ctx, optUpdatesLastBackupMetric); err != nil { + if hasAdmin, err := p.HasAdminRole(ctx); err != nil { return err + } else if !hasAdmin { + return pgerror.Newf(pgcode.InsufficientPrivilege, + "only users with the admin role are allowed to change %s", optUpdatesLastBackupMetric) } } diff --git a/pkg/ccl/backupccl/testdata/backup-restore/alter-schedule/schedule-options b/pkg/ccl/backupccl/testdata/backup-restore/alter-schedule/schedule-options index 9988ec160bd1..e789101fda22 100644 --- a/pkg/ccl/backupccl/testdata/backup-restore/alter-schedule/schedule-options +++ b/pkg/ccl/backupccl/testdata/backup-restore/alter-schedule/schedule-options @@ -116,7 +116,7 @@ let $fullID $incID with schedules as (show schedules) select id from schedules where label='datatest3' order by command->>'backup_type' asc; ---- -exec-sql user=testuser expect-error-regex=(only users with the admin role are allowed to updates_cluster_last_backup_time_metric) +exec-sql user=testuser expect-error-regex=(only users with the admin role are allowed to change updates_cluster_last_backup_time_metric) alter backup schedule $fullID set schedule option updates_cluster_last_backup_time_metric = '1'; ---- regex matches error diff --git a/pkg/ccl/logictestccl/testdata/logic_test/crdb_internal b/pkg/ccl/logictestccl/testdata/logic_test/crdb_internal index 10294182923a..4c41858bc822 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/crdb_internal +++ b/pkg/ccl/logictestccl/testdata/logic_test/crdb_internal @@ -179,7 +179,7 @@ subtest node_tenant_capabilities_cache user testuser -statement error only users with the admin role are allowed to node_tenant_capabilities_cache +statement error user testuser does not have VIEWCLUSTERMETADATA system privilege SELECT * FROM crdb_internal.node_tenant_capabilities_cache user root diff --git a/pkg/ccl/logictestccl/testdata/logic_test/crdb_internal_tenant b/pkg/ccl/logictestccl/testdata/logic_test/crdb_internal_tenant index 071376a6deb5..2d73a876238d 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/crdb_internal_tenant +++ b/pkg/ccl/logictestccl/testdata/logic_test/crdb_internal_tenant @@ -461,28 +461,28 @@ select crdb_internal.set_vmodule('') query error insufficient privilege select crdb_internal.get_vmodule() -query error pq: only users with the admin role are allowed to access the node runtime information +query error user testuser does not have VIEWCLUSTERMETADATA system privilege select * from crdb_internal.node_runtime_info query error pq: only users with the VIEWACTIVITY or VIEWACTIVITYREDACTED or ZONECONFIG privilege or the admin role can read crdb_internal.ranges_no_leases select * from crdb_internal.ranges -query error pq: only users with the admin role are allowed to read crdb_internal.gossip_nodes +query error user testuser does not have VIEWCLUSTERMETADATA system privilege select * from crdb_internal.gossip_nodes -query error pq: only users with the admin role are allowed to read crdb_internal.gossip_liveness +query error user testuser does not have VIEWCLUSTERMETADATA system privilege select * from crdb_internal.gossip_liveness -query error pq: only users with the admin role are allowed to read crdb_internal.node_metrics +query error user testuser does not have VIEWCLUSTERMETADATA system privilege select * from crdb_internal.node_metrics -query error pq: only users with the admin role are allowed to read crdb_internal.kv_node_status +query error user testuser does not have VIEWCLUSTERMETADATA system privilege select * from crdb_internal.kv_node_status -query error pq: only users with the admin role are allowed to read crdb_internal.kv_store_status +query error user testuser does not have VIEWCLUSTERMETADATA system privilege select * from crdb_internal.kv_store_status -query error pq: only users with the admin role are allowed to read crdb_internal.gossip_alerts +query error user testuser does not have VIEWCLUSTERMETADATA system privilege select * from crdb_internal.gossip_alerts # Anyone can see the executable version. @@ -638,7 +638,7 @@ subtest node_tenant_capabilities_cache user testuser -statement error only users with the admin role are allowed to node_tenant_capabilities_cache +statement error user testuser does not have VIEWCLUSTERMETADATA system privilege SELECT * FROM crdb_internal.node_tenant_capabilities_cache user root diff --git a/pkg/ccl/logictestccl/testdata/logic_test/tenant_usage b/pkg/ccl/logictestccl/testdata/logic_test/tenant_usage index a9460790e9b6..df423a2c6074 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/tenant_usage +++ b/pkg/ccl/logictestccl/testdata/logic_test/tenant_usage @@ -34,5 +34,5 @@ SELECT crdb_internal.update_tenant_resource_limits('apptenant', 1000, 100, 0, no user testuser -statement error only users with the admin role are allowed to update tenant resource limits +statement error crdb_internal.update_tenant_resource_limits\(\): user testuser does not have REPAIRCLUSTERMETADATA system privilege SELECT crdb_internal.update_tenant_resource_limits(5, 1000, 100, 0, now(), 0) diff --git a/pkg/ccl/logictestccl/testdata/logic_test/zone b/pkg/ccl/logictestccl/testdata/logic_test/zone index afb44bae9a57..f5a9b855231c 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/zone +++ b/pkg/ccl/logictestccl/testdata/logic_test/zone @@ -786,13 +786,13 @@ CREATE INDEX x ON auth.t (x) PARTITION BY LIST (x) ( user testuser # User should have no CONFIGURE ZONE abilities by default. -statement error only users with the admin role are allowed to alter system ranges +statement error user testuser does not have REPAIRCLUSTERMETADATA system privilege ALTER RANGE default CONFIGURE ZONE USING num_replicas = 3 statement error pq: user testuser does not have ZONECONFIG or CREATE privilege on database auth ALTER DATABASE auth CONFIGURE ZONE USING num_replicas = 3 -statement error pq: only users with the admin role are allowed to alter system tables +statement error user testuser does not have REPAIRCLUSTERMETADATA system privilege ALTER TABLE system.jobs CONFIGURE ZONE USING num_replicas = 3 statement error pq: user testuser does not have ZONECONFIG or CREATE privilege on relation t diff --git a/pkg/server/privchecker/api.go b/pkg/server/privchecker/api.go index 77b18daa8bb4..bc6637058aef 100644 --- a/pkg/server/privchecker/api.go +++ b/pkg/server/privchecker/api.go @@ -43,7 +43,7 @@ type CheckerForRPCHandlers interface { // Its error return is a gRPC error. RequireAdminUser(ctx context.Context) (userName username.SQLUsername, err error) - // RequireAdminRole validates the current user has the VIEWACTIVITY + // RequireViewActivityPermission validates the current user has the VIEWACTIVITY // privilege or role option. // Its error return is a gRPC error. RequireViewActivityPermission(ctx context.Context) error diff --git a/pkg/sql/alter_default_privileges.go b/pkg/sql/alter_default_privileges.go index 6fbfebfe0b26..d0b7f02f0dc0 100644 --- a/pkg/sql/alter_default_privileges.go +++ b/pkg/sql/alter_default_privileges.go @@ -140,8 +140,11 @@ func (n *alterDefaultPrivilegesNode) startExec(params runParams) error { } if n.n.ForAllRoles { - if err := params.p.RequireAdminRole(params.ctx, "ALTER DEFAULT PRIVILEGES"); err != nil { + if hasAdmin, err := params.p.HasAdminRole(params.ctx); err != nil { return err + } else if !hasAdmin { + return pgerror.Newf(pgcode.InsufficientPrivilege, + "only users with the admin role are allowed to ALTER DEFAULT PRIVILEGES FOR ALL ROLES") } } else { // You can change default privileges only for objects that will be created diff --git a/pkg/sql/alter_role.go b/pkg/sql/alter_role.go index 6989ce5dd849..280917fce380 100644 --- a/pkg/sql/alter_role.go +++ b/pkg/sql/alter_role.go @@ -32,6 +32,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/sessioninit" "github.com/cockroachdb/cockroach/pkg/sql/sqlerrors" "github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry" + "github.com/cockroachdb/cockroach/pkg/sql/syntheticprivilege" "github.com/cockroachdb/cockroach/pkg/sql/types" "github.com/cockroachdb/cockroach/pkg/util/log/eventpb" ) @@ -215,8 +216,11 @@ func (n *alterRoleNode) startExec(params runParams) error { return err } if isAdmin { - if err := params.p.RequireAdminRole(params.ctx, "ALTER ROLE admin"); err != nil { + if hasAdmin, err := params.p.HasAdminRole(params.ctx); err != nil { return err + } else if !hasAdmin { + return pgerror.Newf(pgcode.InsufficientPrivilege, + "only users with the admin role are allowed to alter another admin") } } @@ -287,8 +291,11 @@ func (p *planner) AlterRoleSet(ctx context.Context, n *tree.AlterRoleSet) (planN // modifying their own defaults unless they have CREATEROLE. This is analogous // to our restriction that prevents a user from modifying their own password. if n.AllRoles { - if err := p.RequireAdminRole(ctx, "ALTER ROLE ALL"); err != nil { + if hasAdmin, err := p.HasAdminRole(ctx); err != nil { return nil, err + } else if !hasAdmin { + return nil, pgerror.Newf(pgcode.InsufficientPrivilege, + "only users with the admin role are allowed to ALTER ROLE ALL ... SET") } } else { canAlterRoleSet, err := p.HasGlobalPrivilegeOrRoleOption(ctx, privilege.CREATEROLE) @@ -607,7 +614,17 @@ func (n *alterRoleSetNode) getRoleName( return false, username.SQLUsername{}, err } if isAdmin { - if err := params.p.RequireAdminRole(params.ctx, "ALTER ROLE admin"); err != nil { + if hasAdmin, err := params.p.HasAdminRole(params.ctx); err != nil { + return false, username.SQLUsername{}, err + } else if !hasAdmin { + return false, username.SQLUsername{}, pgerror.Newf(pgcode.InsufficientPrivilege, + "only users with the admin role are allowed to alter another admin") + } + + // Note that admins implicitly have the REPAIRCLUSTERMETADATA privilege. + if err := params.p.CheckPrivilege( + params.ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.REPAIRCLUSTERMETADATA, + ); err != nil { return false, username.SQLUsername{}, err } } diff --git a/pkg/sql/authorization.go b/pkg/sql/authorization.go index cd5f8cd2d702..1a9feac8badd 100644 --- a/pkg/sql/authorization.go +++ b/pkg/sql/authorization.go @@ -107,11 +107,6 @@ type AuthorizationAccessor interface { // HasAdminRole checks if the current session's user has admin role. HasAdminRole(ctx context.Context) (bool, error) - // RequireAdminRole is a wrapper on top of HasAdminRole. - // It errors if HasAdminRole errors or if the user isn't a super-user. - // Includes the named action in the error message. - RequireAdminRole(ctx context.Context, action string) error - // MemberOfWithAdminOption looks up all the roles (direct and indirect) that 'member' is a member // of and returns a map of role -> isAdmin. MemberOfWithAdminOption(ctx context.Context, member username.SQLUsername) (map[username.SQLUsername]bool, error) @@ -508,22 +503,6 @@ func (p *planner) HasAdminRole(ctx context.Context) (bool, error) { return p.UserHasAdminRole(ctx, p.User()) } -// RequireAdminRole implements the AuthorizationAccessor interface. -// Requires a valid transaction to be open. -func (p *planner) RequireAdminRole(ctx context.Context, action string) error { - ok, err := p.HasAdminRole(ctx) - - if err != nil { - return err - } - if !ok { - // raise error if user is not a super-user - return pgerror.Newf(pgcode.InsufficientPrivilege, - "only users with the admin role are allowed to %s", action) - } - return nil -} - // MemberOfWithAdminOption is a wrapper around the MemberOfWithAdminOption // method. func (p *planner) MemberOfWithAdminOption( diff --git a/pkg/sql/copy_file_upload.go b/pkg/sql/copy_file_upload.go index 809b2b6d8308..1961af485c36 100644 --- a/pkg/sql/copy_file_upload.go +++ b/pkg/sql/copy_file_upload.go @@ -20,6 +20,8 @@ import ( "github.com/cockroachdb/cockroach/pkg/cloud" "github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo" "github.com/cockroachdb/cockroach/pkg/sql/lexbase" + "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" + "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgwirebase" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/types" @@ -108,8 +110,11 @@ func newFileUploadMachine( c.parsingEvalCtx = c.p.EvalContext() if n.Table.Table() == NodelocalFileUploadTable { - if err := c.p.RequireAdminRole(ctx, "upload to nodelocal"); err != nil { + if hasAdmin, err := p.HasAdminRole(ctx); err != nil { return nil, err + } else if !hasAdmin { + return nil, pgerror.Newf(pgcode.InsufficientPrivilege, + "only users with the admin role are allowed to upload to nodelocal") } } diff --git a/pkg/sql/crdb_internal.go b/pkg/sql/crdb_internal.go index 13eac6c4f8ad..6f236faab3e8 100644 --- a/pkg/sql/crdb_internal.go +++ b/pkg/sql/crdb_internal.go @@ -86,6 +86,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/sqlstats" "github.com/cockroachdb/cockroach/pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil" "github.com/cockroachdb/cockroach/pkg/sql/sqlstats/sslocal" + "github.com/cockroachdb/cockroach/pkg/sql/syntheticprivilege" "github.com/cockroachdb/cockroach/pkg/sql/types" "github.com/cockroachdb/cockroach/pkg/sql/vtable" "github.com/cockroachdb/cockroach/pkg/util/admission/admissionpb" @@ -265,7 +266,7 @@ CREATE TABLE crdb_internal.node_runtime_info ( value STRING NOT NULL )`, populate: func(ctx context.Context, p *planner, _ catalog.DatabaseDescriptor, addRow func(...tree.Datum) error) error { - if err := p.RequireAdminRole(ctx, "access the node runtime information"); err != nil { + if err := p.CheckPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.VIEWCLUSTERMETADATA); err != nil { return err } @@ -2122,7 +2123,7 @@ CREATE TABLE crdb_internal.node_txn_stats ( implicit_count INT NOT NULL )`, populate: func(ctx context.Context, p *planner, _ catalog.DatabaseDescriptor, addRow func(...tree.Datum) error) error { - if err := p.RequireAdminRole(ctx, "access application statistics"); err != nil { + if err := p.CheckPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.VIEWCLUSTERMETADATA); err != nil { return err } @@ -2275,14 +2276,12 @@ CREATE TABLE crdb_internal.node_inflight_trace_spans ( operation STRING NULL -- The span's operation. )`, populate: func(ctx context.Context, p *planner, _ catalog.DatabaseDescriptor, addRow func(...tree.Datum) error) error { - hasAdmin, err := p.HasAdminRole(ctx) - if err != nil { + if err := p.CheckPrivilege( + ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.VIEWCLUSTERMETADATA, + ); err != nil { return err } - if !hasAdmin { - return pgerror.Newf(pgcode.InsufficientPrivilege, - "only users with the admin role are allowed to read crdb_internal.node_inflight_trace_spans") - } + return p.ExecCfg().AmbientCtx.Tracer.VisitSpans(func(span tracing.RegistrySpan) error { trace := span.GetFullRecording(tracingpb.RecordingVerbose) for _, rec := range trace.Flatten() { @@ -3269,7 +3268,7 @@ var crdbInternalLocalMetricsTable = virtualSchemaTable{ value FLOAT NOT NULL -- value of the metric )`, populate: func(ctx context.Context, p *planner, _ catalog.DatabaseDescriptor, addRow func(...tree.Datum) error) error { - if err := p.RequireAdminRole(ctx, "read crdb_internal.node_metrics"); err != nil { + if err := p.CheckPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.VIEWCLUSTERMETADATA); err != nil { return err } @@ -4853,7 +4852,7 @@ CREATE TABLE crdb_internal.gossip_nodes ( ) `, populate: func(ctx context.Context, p *planner, _ catalog.DatabaseDescriptor, addRow func(...tree.Datum) error) error { - if err := p.RequireAdminRole(ctx, "read crdb_internal.gossip_nodes"); err != nil { + if err := p.CheckPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.VIEWCLUSTERMETADATA); err != nil { return err } @@ -4972,7 +4971,7 @@ CREATE TABLE crdb_internal.kv_node_liveness ( ) `, populate: func(ctx context.Context, p *planner, _ catalog.DatabaseDescriptor, addRow func(...tree.Datum) error) error { - if err := p.RequireAdminRole(ctx, "read crdb_internal.node_liveness"); err != nil { + if err := p.CheckPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.VIEWCLUSTERMETADATA); err != nil { return err } @@ -5038,7 +5037,7 @@ CREATE TABLE crdb_internal.gossip_liveness ( // which is highly available. DO NOT CALL functions which require the // cluster to be healthy, such as NodesStatusServer.ListNodesInternal(). - if err := p.RequireAdminRole(ctx, "read crdb_internal.gossip_liveness"); err != nil { + if err := p.CheckPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.VIEWCLUSTERMETADATA); err != nil { return err } @@ -5114,7 +5113,7 @@ CREATE TABLE crdb_internal.gossip_alerts ( ) `, populate: func(ctx context.Context, p *planner, _ catalog.DatabaseDescriptor, addRow func(...tree.Datum) error) error { - if err := p.RequireAdminRole(ctx, "read crdb_internal.gossip_alerts"); err != nil { + if err := p.CheckPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.VIEWCLUSTERMETADATA); err != nil { return err } @@ -5435,7 +5434,7 @@ CREATE TABLE crdb_internal.kv_node_status ( ) `, populate: func(ctx context.Context, p *planner, _ catalog.DatabaseDescriptor, addRow func(...tree.Datum) error) error { - if err := p.RequireAdminRole(ctx, "read crdb_internal.kv_node_status"); err != nil { + if err := p.CheckPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.VIEWCLUSTERMETADATA); err != nil { return err } ss, err := p.extendedEvalCtx.NodesStatusServer.OptionalNodesStatusServer( @@ -5549,7 +5548,7 @@ CREATE TABLE crdb_internal.kv_store_status ( ) `, populate: func(ctx context.Context, p *planner, _ catalog.DatabaseDescriptor, addRow func(...tree.Datum) error) error { - if err := p.RequireAdminRole(ctx, "read crdb_internal.kv_store_status"); err != nil { + if err := p.CheckPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.VIEWCLUSTERMETADATA); err != nil { return err } ss, err := p.ExecCfg().NodesStatusServer.OptionalNodesStatusServer( @@ -8457,7 +8456,7 @@ CREATE TABLE crdb_internal.node_tenant_capabilities_cache ( );`, populate: func(ctx context.Context, p *planner, _ catalog.DatabaseDescriptor, addRow func(...tree.Datum) error) error { const op = "node_tenant_capabilities_cache" - if err := p.RequireAdminRole(ctx, op); err != nil { + if err := p.CheckPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.VIEWCLUSTERMETADATA); err != nil { return err } tenantCapabilitiesReader, err := p.ExecCfg().TenantCapabilitiesReader.Get(op) diff --git a/pkg/sql/delegate/show_all_cluster_settings.go b/pkg/sql/delegate/show_all_cluster_settings.go index 43e3d44fa457..d68aa84882a7 100644 --- a/pkg/sql/delegate/show_all_cluster_settings.go +++ b/pkg/sql/delegate/show_all_cluster_settings.go @@ -95,10 +95,7 @@ func (d *delegator) delegateShowTenantClusterSettingList( // privileged operation than viewing local cluster settings. So we // shouldn't be allowing with just the role option // VIEWCLUSTERSETTINGS. - // - // TODO(knz): Using admin authz for now; we may want to introduce a - // more specific role option later. - if err := d.catalog.RequireAdminRole(d.ctx, "show a tenant cluster setting"); err != nil { + if err := d.catalog.CheckPrivilege(d.ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.VIEWCLUSTERMETADATA); err != nil { return nil, err } diff --git a/pkg/sql/gossip.go b/pkg/sql/gossip.go index c4a16243678c..5940e6aee828 100644 --- a/pkg/sql/gossip.go +++ b/pkg/sql/gossip.go @@ -10,7 +10,12 @@ package sql -import "context" +import ( + "context" + + "github.com/cockroachdb/cockroach/pkg/sql/privilege" + "github.com/cockroachdb/cockroach/pkg/sql/syntheticprivilege" +) // TryClearGossipInfo implements the tree.GossipOperator interface. func (p *planner) TryClearGossipInfo(ctx context.Context, key string) (bool, error) { @@ -18,7 +23,7 @@ func (p *planner) TryClearGossipInfo(ctx context.Context, key string) (bool, err if err != nil { return false, err } - if err := p.RequireAdminRole(ctx, "try clear gossip info"); err != nil { + if err := p.CheckPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.REPAIRCLUSTERMETADATA); err != nil { return false, err } return g.TryClearInfo(key) diff --git a/pkg/sql/logictest/testdata/logic_test/alter_default_privileges_for_all_roles b/pkg/sql/logictest/testdata/logic_test/alter_default_privileges_for_all_roles index 54743231320b..d8a0a02f9e54 100644 --- a/pkg/sql/logictest/testdata/logic_test/alter_default_privileges_for_all_roles +++ b/pkg/sql/logictest/testdata/logic_test/alter_default_privileges_for_all_roles @@ -112,7 +112,7 @@ test public t7 testuser2 ALL false user testuser # Must be an admin to alter default privileges for all roles. -statement error pq: only users with the admin role are allowed to ALTER DEFAULT PRIVILEGES +statement error pq: only users with the admin role are allowed to ALTER DEFAULT PRIVILEGES FOR ALL ROLES ALTER DEFAULT PRIVILEGES FOR ALL ROLES GRANT SELECT ON TABLES TO testuser user root diff --git a/pkg/sql/logictest/testdata/logic_test/alter_role_set b/pkg/sql/logictest/testdata/logic_test/alter_role_set index a2f9b1d07dc2..1a76dfc15a04 100644 --- a/pkg/sql/logictest/testdata/logic_test/alter_role_set +++ b/pkg/sql/logictest/testdata/logic_test/alter_role_set @@ -221,7 +221,7 @@ statement error only users with the admin role are allowed to ALTER ROLE ALL ALTER ROLE ALL IN DATABASE test_set_db RESET application_name # Verify that testuser can't edit an ADMIN, even after getting CREATEROLE. -statement error only users with the admin role are allowed to ALTER ROLE admin +statement error only users with the admin role are allowed to alter another admin ALTER ROLE other_admin SET application_name = 'abc' user root diff --git a/pkg/sql/logictest/testdata/logic_test/builtin_function b/pkg/sql/logictest/testdata/logic_test/builtin_function index a97a39a61c92..3ace53ee2252 100644 --- a/pkg/sql/logictest/testdata/logic_test/builtin_function +++ b/pkg/sql/logictest/testdata/logic_test/builtin_function @@ -3038,7 +3038,7 @@ subtest crdb_internal.payloads_for_span # switch users -- this one has no permissions so expect errors user testuser -query error pq: only users with the admin role are allowed to use crdb_internal.payloads_for_span +query error user testuser does not have VIEWCLUSTERMETADATA system privilege SELECT * FROM crdb_internal.payloads_for_span(0) user root @@ -3054,7 +3054,7 @@ subtest crdb_internal.payloads_for_trace # switch users -- this one has no permissions so expect errors user testuser -query error pq: only users with the admin role are allowed to use crdb_internal.payloads_for_span +query error user testuser does not have VIEWCLUSTERMETADATA system privilege SELECT * FROM crdb_internal.payloads_for_span(0) user root diff --git a/pkg/sql/logictest/testdata/logic_test/crdb_internal b/pkg/sql/logictest/testdata/logic_test/crdb_internal index 090134494a97..5ce6d183f5ed 100644 --- a/pkg/sql/logictest/testdata/logic_test/crdb_internal +++ b/pkg/sql/logictest/testdata/logic_test/crdb_internal @@ -747,7 +747,7 @@ select crdb_internal.set_vmodule('') query error insufficient privilege select crdb_internal.get_vmodule() -query error pq: only users with the admin role are allowed to access the node runtime information +query error user testuser does not have VIEWCLUSTERMETADATA system privilege select * from crdb_internal.node_runtime_info query error pq: only users with the VIEWACTIVITY or VIEWACTIVITYREDACTED or ZONECONFIG privilege or the admin role can read crdb_internal.ranges_no_leases @@ -756,28 +756,28 @@ select * from crdb_internal.ranges query error pq: only users with the VIEWACTIVITY or VIEWACTIVITYREDACTED or ZONECONFIG privilege or the admin role can read crdb_internal.ranges_no_leases SHOW RANGES FROM TABLE foo -query error pq: only users with the admin role are allowed to read crdb_internal.gossip_nodes +query error user testuser does not have VIEWCLUSTERMETADATA system privilege select * from crdb_internal.gossip_nodes -query error pq: only users with the admin role are allowed to read crdb_internal.gossip_liveness +query error user testuser does not have VIEWCLUSTERMETADATA system privilege select * from crdb_internal.gossip_liveness -query error pq: only users with the admin role are allowed to read crdb_internal.node_metrics +query error user testuser does not have VIEWCLUSTERMETADATA system privilege select * from crdb_internal.node_metrics -query error pq: only users with the admin role are allowed to read crdb_internal.kv_node_status +query error user testuser does not have VIEWCLUSTERMETADATA system privilege select * from crdb_internal.kv_node_status -query error pq: only users with the admin role are allowed to read crdb_internal.kv_store_status +query error user testuser does not have VIEWCLUSTERMETADATA system privilege select * from crdb_internal.kv_store_status -query error pq: only users with the admin role are allowed to read crdb_internal.gossip_alerts +query error user testuser does not have VIEWCLUSTERMETADATA system privilege select * from crdb_internal.gossip_alerts -query error pq: only users with the admin role are allowed to read crdb_internal.node_inflight_trace_spans +query error user testuser does not have VIEWCLUSTERMETADATA system privilege select * from crdb_internal.node_inflight_trace_spans -query error pq: crdb_internal.check_consistency requires admin privileges +query error user testuser does not have REPAIRCLUSTERMETADATA system privilege SELECT * FROM crdb_internal.check_consistency(true, b'\x02', b'\x04') # Anyone can see the executable version. diff --git a/pkg/sql/logictest/testdata/logic_test/role b/pkg/sql/logictest/testdata/logic_test/role index edf51c22d265..6eecba64d0ef 100644 --- a/pkg/sql/logictest/testdata/logic_test/role +++ b/pkg/sql/logictest/testdata/logic_test/role @@ -1554,7 +1554,7 @@ user testuser # Verify that testuser is not allowed to edit some other role that has ADMIN. # The error message is important for this test -- we want to make sure it fails # because of a missing ADMIN power, not because of a missing CREATEROLE option. -statement error only users with the admin role are allowed to ALTER ROLE admin +statement error only users with the admin role are allowed to alter another admin ALTER ROLE other_admin NOCREATEDB subtest pw_hashes diff --git a/pkg/sql/logictest/testdata/logic_test/tenant_builtins b/pkg/sql/logictest/testdata/logic_test/tenant_builtins index f04125f65866..0514353f1f4d 100644 --- a/pkg/sql/logictest/testdata/logic_test/tenant_builtins +++ b/pkg/sql/logictest/testdata/logic_test/tenant_builtins @@ -273,7 +273,7 @@ SELECT crdb_internal.create_tenant('not-allowed') statement error user testuser does not have MANAGETENANT system privilege DROP TENANT [1] -statement error only users with the admin role are allowed to gc tenant +statement error crdb_internal.gc_tenant\(\): user testuser does not have REPAIRCLUSTERMETADATA system privilege SELECT crdb_internal.gc_tenant(314) user root diff --git a/pkg/sql/opt/cat/catalog.go b/pkg/sql/opt/cat/catalog.go index 1353f17e5ccc..df5bcc0d6014 100644 --- a/pkg/sql/opt/cat/catalog.go +++ b/pkg/sql/opt/cat/catalog.go @@ -182,10 +182,6 @@ type Catalog interface { // returns true. Returns an error if query on the `system.users` table failed HasAdminRole(ctx context.Context) (bool, error) - // RequireAdminRole checks that the current user has admin privileges. If not, - // returns an error. - RequireAdminRole(ctx context.Context, action string) error - // HasRoleOption converts the roleoption to its SQL column name and checks if // the user belongs to a role where the option has value true. Requires a // valid transaction to be open. diff --git a/pkg/sql/opt/optbuilder/BUILD.bazel b/pkg/sql/opt/optbuilder/BUILD.bazel index 6b69c784b9e2..0b00c7e74346 100644 --- a/pkg/sql/opt/optbuilder/BUILD.bazel +++ b/pkg/sql/opt/optbuilder/BUILD.bazel @@ -90,6 +90,7 @@ go_library( "//pkg/sql/sem/volatility", "//pkg/sql/sqlerrors", "//pkg/sql/sqltelemetry", + "//pkg/sql/syntheticprivilege", "//pkg/sql/types", "//pkg/util", "//pkg/util/errorutil", diff --git a/pkg/sql/opt/optbuilder/alter_range.go b/pkg/sql/opt/optbuilder/alter_range.go index 61cb87394716..a411ec637804 100644 --- a/pkg/sql/opt/optbuilder/alter_range.go +++ b/pkg/sql/opt/optbuilder/alter_range.go @@ -15,7 +15,9 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/opt" "github.com/cockroachdb/cockroach/pkg/sql/opt/memo" "github.com/cockroachdb/cockroach/pkg/sql/opt/props/physical" + "github.com/cockroachdb/cockroach/pkg/sql/privilege" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" + "github.com/cockroachdb/cockroach/pkg/sql/syntheticprivilege" "github.com/cockroachdb/cockroach/pkg/sql/types" ) @@ -24,7 +26,7 @@ func (b *Builder) buildAlterRangeRelocate( relocate *tree.RelocateRange, inScope *scope, ) (outScope *scope) { - if err := b.catalog.RequireAdminRole(b.ctx, "ALTER RANGE RELOCATE"); err != nil { + if err := b.catalog.CheckPrivilege(b.ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.REPAIRCLUSTERMETADATA); err != nil { panic(err) } diff --git a/pkg/sql/opt/testutils/testcat/BUILD.bazel b/pkg/sql/opt/testutils/testcat/BUILD.bazel index 8395f3eb142e..87f378138545 100644 --- a/pkg/sql/opt/testutils/testcat/BUILD.bazel +++ b/pkg/sql/opt/testutils/testcat/BUILD.bazel @@ -51,6 +51,7 @@ go_library( "//pkg/sql/sessiondatapb", "//pkg/sql/sqlerrors", "//pkg/sql/stats", + "//pkg/sql/syntheticprivilege", "//pkg/sql/types", "//pkg/sql/vtable", "//pkg/util/intsets", diff --git a/pkg/sql/opt/testutils/testcat/test_catalog.go b/pkg/sql/opt/testutils/testcat/test_catalog.go index 7d407dab2955..f205aa7dc383 100644 --- a/pkg/sql/opt/testutils/testcat/test_catalog.go +++ b/pkg/sql/opt/testutils/testcat/test_catalog.go @@ -36,6 +36,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sqlerrors" "github.com/cockroachdb/cockroach/pkg/sql/stats" + "github.com/cockroachdb/cockroach/pkg/sql/syntheticprivilege" "github.com/cockroachdb/cockroach/pkg/sql/types" "github.com/cockroachdb/cockroach/pkg/util/intsets" "github.com/cockroachdb/cockroach/pkg/util/treeprinter" @@ -287,6 +288,8 @@ func (tc *Catalog) CheckAnyPrivilege(ctx context.Context, o cat.Object) error { if t.Revoked { return pgerror.Newf(pgcode.InsufficientPrivilege, "user does not have privilege to access %v", t.SeqName) } + case *syntheticprivilege.GlobalPrivilege: + default: panic("invalid Object") } @@ -306,11 +309,6 @@ func (tc *Catalog) HasAdminRole(ctx context.Context) (bool, error) { return true, nil } -// RequireAdminRole is part of the cat.Catalog interface. -func (tc *Catalog) RequireAdminRole(ctx context.Context, action string) error { - return nil -} - // HasRoleOption is part of the cat.Catalog interface. func (tc *Catalog) HasRoleOption(ctx context.Context, roleOption roleoption.Option) (bool, error) { return true, nil diff --git a/pkg/sql/opt_catalog.go b/pkg/sql/opt_catalog.go index d8629455d7c4..84a9fba5fea7 100644 --- a/pkg/sql/opt_catalog.go +++ b/pkg/sql/opt_catalog.go @@ -443,11 +443,6 @@ func (oc *optCatalog) HasAdminRole(ctx context.Context) (bool, error) { return oc.planner.HasAdminRole(ctx) } -// RequireAdminRole is part of the cat.Catalog interface. -func (oc *optCatalog) RequireAdminRole(ctx context.Context, action string) error { - return oc.planner.RequireAdminRole(ctx, action) -} - // HasRoleOption is part of the cat.Catalog interface. func (oc *optCatalog) HasRoleOption( ctx context.Context, roleOption roleoption.Option, diff --git a/pkg/sql/repair.go b/pkg/sql/repair.go index 4b35cc778d81..00272d8aeaca 100644 --- a/pkg/sql/repair.go +++ b/pkg/sql/repair.go @@ -37,6 +37,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgnotice" "github.com/cockroachdb/cockroach/pkg/sql/privilege" + "github.com/cockroachdb/cockroach/pkg/sql/syntheticprivilege" "github.com/cockroachdb/cockroach/pkg/storage" "github.com/cockroachdb/cockroach/pkg/util/duration" "github.com/cockroachdb/cockroach/pkg/util/hlc" @@ -778,7 +779,7 @@ func (p *planner) ForceDeleteTableData(ctx context.Context, descID int64) error } func (p *planner) ExternalReadFile(ctx context.Context, uri string) ([]byte, error) { - if err := p.RequireAdminRole(ctx, "network I/O"); err != nil { + if err := p.CheckPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.REPAIRCLUSTERMETADATA); err != nil { return nil, err } @@ -795,7 +796,7 @@ func (p *planner) ExternalReadFile(ctx context.Context, uri string) ([]byte, err } func (p *planner) ExternalWriteFile(ctx context.Context, uri string, content []byte) error { - if err := p.RequireAdminRole(ctx, "network I/O"); err != nil { + if err := p.CheckPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.REPAIRCLUSTERMETADATA); err != nil { return err } diff --git a/pkg/sql/scrub.go b/pkg/sql/scrub.go index f539c4175660..b7621cd150b2 100644 --- a/pkg/sql/scrub.go +++ b/pkg/sql/scrub.go @@ -18,8 +18,10 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" + "github.com/cockroachdb/cockroach/pkg/sql/privilege" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sqlerrors" + "github.com/cockroachdb/cockroach/pkg/sql/syntheticprivilege" "github.com/cockroachdb/cockroach/pkg/util/hlc" "github.com/cockroachdb/errors" ) @@ -69,7 +71,7 @@ type checkOperation interface { // Scrub checks the database. // Privileges: superuser. func (p *planner) Scrub(ctx context.Context, n *tree.Scrub) (planNode, error) { - if err := p.RequireAdminRole(ctx, "SCRUB"); err != nil { + if err := p.CheckPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.REPAIRCLUSTERMETADATA); err != nil { return nil, err } return &scrubNode{n: n}, nil diff --git a/pkg/sql/sem/builtins/BUILD.bazel b/pkg/sql/sem/builtins/BUILD.bazel index cc28e74db577..73ba21b535ed 100644 --- a/pkg/sql/sem/builtins/BUILD.bazel +++ b/pkg/sql/sem/builtins/BUILD.bazel @@ -97,6 +97,7 @@ go_library( "//pkg/sql/sqltelemetry", "//pkg/sql/storageparam", "//pkg/sql/storageparam/indexstorageparam", + "//pkg/sql/syntheticprivilege", "//pkg/sql/types", "//pkg/storage", "//pkg/storage/enginepb", diff --git a/pkg/sql/sem/builtins/generator_builtins.go b/pkg/sql/sem/builtins/generator_builtins.go index ee9b9278414b..e39d456124b7 100644 --- a/pkg/sql/sem/builtins/generator_builtins.go +++ b/pkg/sql/sem/builtins/generator_builtins.go @@ -27,12 +27,14 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/opt/workloadindexrec" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" + "github.com/cockroachdb/cockroach/pkg/sql/privilege" "github.com/cockroachdb/cockroach/pkg/sql/protoreflect" "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins/builtinconstants" "github.com/cockroachdb/cockroach/pkg/sql/sem/eval" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sem/volatility" "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" + "github.com/cockroachdb/cockroach/pkg/sql/syntheticprivilege" "github.com/cockroachdb/cockroach/pkg/sql/types" "github.com/cockroachdb/cockroach/pkg/storage/enginepb" "github.com/cockroachdb/cockroach/pkg/util/arith" @@ -2087,13 +2089,11 @@ var _ eval.ValueGenerator = &checkConsistencyGenerator{} func makeCheckConsistencyGenerator( ctx context.Context, evalCtx *eval.Context, args tree.Datums, ) (eval.ValueGenerator, error) { - isAdmin, err := evalCtx.SessionAccessor.HasAdminRole(ctx) - if err != nil { + if err := evalCtx.SessionAccessor.CheckPrivilege( + ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.REPAIRCLUSTERMETADATA, + ); err != nil { return nil, err } - if !isAdmin { - return nil, pgerror.New(pgcode.InsufficientPrivilege, "crdb_internal.check_consistency requires admin privileges") - } keyFrom := roachpb.Key(*args[1].(*tree.DBytes)) keyTo := roachpb.Key(*args[2].(*tree.DBytes)) @@ -2458,17 +2458,11 @@ type payloadsForSpanGenerator struct { func makePayloadsForSpanGenerator( ctx context.Context, evalCtx *eval.Context, args tree.Datums, ) (eval.ValueGenerator, error) { - // The user must be an admin to use this builtin. - isAdmin, err := evalCtx.SessionAccessor.HasAdminRole(ctx) - if err != nil { + if err := evalCtx.SessionAccessor.CheckPrivilege( + ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.VIEWCLUSTERMETADATA, + ); err != nil { return nil, err } - if !isAdmin { - return nil, pgerror.Newf( - pgcode.InsufficientPrivilege, - "only users with the admin role are allowed to use crdb_internal.payloads_for_span", - ) - } spanID := tracingpb.SpanID(*(args[0].(*tree.DInt))) span := evalCtx.Tracer.GetActiveSpanByID(spanID) if span == nil { @@ -2561,17 +2555,11 @@ type payloadsForTraceGenerator struct { func makePayloadsForTraceGenerator( ctx context.Context, evalCtx *eval.Context, args tree.Datums, ) (eval.ValueGenerator, error) { - // The user must be an admin to use this builtin. - isAdmin, err := evalCtx.SessionAccessor.HasAdminRole(ctx) - if err != nil { + if err := evalCtx.SessionAccessor.CheckPrivilege( + ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.VIEWCLUSTERMETADATA, + ); err != nil { return nil, err } - if !isAdmin { - return nil, pgerror.Newf( - pgcode.InsufficientPrivilege, - "only users with the admin role are allowed to use crdb_internal.payloads_for_trace", - ) - } traceID := uint64(*(args[0].(*tree.DInt))) return &payloadsForTraceGenerator{traceID: traceID, planner: evalCtx.Planner}, nil } diff --git a/pkg/sql/sem/builtins/generator_probe_ranges.go b/pkg/sql/sem/builtins/generator_probe_ranges.go index 6d08d2e3b74f..303969bbec0a 100644 --- a/pkg/sql/sem/builtins/generator_probe_ranges.go +++ b/pkg/sql/sem/builtins/generator_probe_ranges.go @@ -19,13 +19,13 @@ import ( "github.com/cockroachdb/cockroach/pkg/kv" "github.com/cockroachdb/cockroach/pkg/kv/kvclient" "github.com/cockroachdb/cockroach/pkg/roachpb" - "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" - "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" + "github.com/cockroachdb/cockroach/pkg/sql/privilege" "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins/builtinconstants" "github.com/cockroachdb/cockroach/pkg/sql/sem/catid" "github.com/cockroachdb/cockroach/pkg/sql/sem/eval" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sem/volatility" + "github.com/cockroachdb/cockroach/pkg/sql/syntheticprivilege" "github.com/cockroachdb/cockroach/pkg/sql/types" "github.com/cockroachdb/cockroach/pkg/util/timeutil" "github.com/cockroachdb/cockroach/pkg/util/tracing" @@ -118,17 +118,11 @@ type probeRangeGenerator struct { func makeProbeRangeGenerator( ctx context.Context, evalCtx *eval.Context, args tree.Datums, ) (eval.ValueGenerator, error) { - // The user must be an admin to use this builtin. - isAdmin, err := evalCtx.SessionAccessor.HasAdminRole(ctx) - if err != nil { + if err := evalCtx.SessionAccessor.CheckPrivilege( + ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.REPAIRCLUSTERMETADATA, + ); err != nil { return nil, err } - if !isAdmin { - return nil, pgerror.Newf( - pgcode.InsufficientPrivilege, - "only users with the admin role are allowed to use crdb_internal.probe_range", - ) - } // Trace the query to meta2. Return it as part of the error string if the query fails. // This improves observability into a meta2 outage. We expect crdb_internal.probe_range // to be available, unless meta2 is down. @@ -140,6 +134,7 @@ func makeProbeRangeGenerator( ) defer sp.Finish() // Handle args passed in. + var err error ranges, err = kvclient.ScanMetaKVs(ctx, evalCtx.Txn, roachpb.Span{ Key: keys.MinKey, EndKey: keys.MaxKey, diff --git a/pkg/sql/set_zone_config.go b/pkg/sql/set_zone_config.go index 8b3fa2c03b5b..e744cc284a02 100644 --- a/pkg/sql/set_zone_config.go +++ b/pkg/sql/set_zone_config.go @@ -33,6 +33,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sqlerrors" "github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry" + "github.com/cockroachdb/cockroach/pkg/sql/syntheticprivilege" "github.com/cockroachdb/cockroach/pkg/sql/types" "github.com/cockroachdb/cockroach/pkg/util/errorutil" "github.com/cockroachdb/cockroach/pkg/util/log/eventpb" @@ -303,11 +304,11 @@ func checkPrivilegeForSetZoneConfig(ctx context.Context, p *planner, zs tree.Zon // an admin. Otherwise we require CREATE privileges on the database or table // in question. if zs.NamedZone != "" { - return p.RequireAdminRole(ctx, "alter system ranges") + return p.CheckPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.REPAIRCLUSTERMETADATA) } if zs.Database != "" { if zs.Database == "system" { - return p.RequireAdminRole(ctx, "alter the system database") + return p.CheckPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.REPAIRCLUSTERMETADATA) } dbDesc, err := p.Descriptors().ByNameWithLeased(p.txn).Get().Database(ctx, string(zs.Database)) if err != nil { @@ -333,7 +334,7 @@ func checkPrivilegeForSetZoneConfig(ctx context.Context, p *planner, zs tree.Zon return err } if tableDesc.GetParentID() == keys.SystemDatabaseID { - return p.RequireAdminRole(ctx, "alter system tables") + return p.CheckPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.REPAIRCLUSTERMETADATA) } // Can set ZoneConfig if user has either CREATE privilege or ZONECONFIG privilege at the Table level diff --git a/pkg/sql/tenant_accessors.go b/pkg/sql/tenant_accessors.go index d34ab1b4a493..51c99274f6e8 100644 --- a/pkg/sql/tenant_accessors.go +++ b/pkg/sql/tenant_accessors.go @@ -24,8 +24,10 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/isql" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" + "github.com/cockroachdb/cockroach/pkg/sql/privilege" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" + "github.com/cockroachdb/cockroach/pkg/sql/syntheticprivilege" "github.com/cockroachdb/errors" ) @@ -132,7 +134,7 @@ func (p *planner) LookupTenantID( ctx context.Context, tenantName roachpb.TenantName, ) (tid roachpb.TenantID, err error) { const op = "get-tenant-info" - if err := p.RequireAdminRole(ctx, op); err != nil { + if err := p.CheckPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.VIEWCLUSTERMETADATA); err != nil { return tid, err } diff --git a/pkg/sql/tenant_gc.go b/pkg/sql/tenant_gc.go index df9d547326f5..9a4e9499711c 100644 --- a/pkg/sql/tenant_gc.go +++ b/pkg/sql/tenant_gc.go @@ -20,7 +20,9 @@ import ( "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/spanconfig" "github.com/cockroachdb/cockroach/pkg/sql/isql" + "github.com/cockroachdb/cockroach/pkg/sql/privilege" "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" + "github.com/cockroachdb/cockroach/pkg/sql/syntheticprivilege" "github.com/cockroachdb/cockroach/pkg/util/hlc" "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/errors" @@ -133,7 +135,7 @@ func (p *planner) GCTenant(ctx context.Context, tenID uint64) error { if !p.extendedEvalCtx.TxnIsSingleStmt { return errors.Errorf("gc_tenant cannot be used inside a multi-statement transaction") } - if err := p.RequireAdminRole(ctx, "gc tenant"); err != nil { + if err := p.CheckPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.REPAIRCLUSTERMETADATA); err != nil { return err } info, err := GetTenantRecordByID(ctx, p.InternalSQLTxn(), roachpb.MustMakeTenantID(tenID), p.ExecCfg().Settings) diff --git a/pkg/sql/tenant_settings.go b/pkg/sql/tenant_settings.go index a7957bc3fe6f..baaa3fdeb0d6 100644 --- a/pkg/sql/tenant_settings.go +++ b/pkg/sql/tenant_settings.go @@ -19,9 +19,11 @@ import ( "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" + "github.com/cockroachdb/cockroach/pkg/sql/privilege" "github.com/cockroachdb/cockroach/pkg/sql/sem/eval" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" + "github.com/cockroachdb/cockroach/pkg/sql/syntheticprivilege" "github.com/cockroachdb/cockroach/pkg/util/log/eventpb" "github.com/cockroachdb/errors" ) @@ -170,10 +172,7 @@ func (p *planner) ShowTenantClusterSetting( // privileged operation than viewing local cluster settings. So we // shouldn't be allowing with just the role option // VIEWCLUSTERSETTINGS. - // - // TODO(knz): Using admin authz for now; we may want to introduce a - // more specific role option later. - if err := p.RequireAdminRole(ctx, "view a tenant cluster setting"); err != nil { + if err := p.CheckPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.VIEWCLUSTERMETADATA); err != nil { return nil, err } diff --git a/pkg/sql/tenant_update.go b/pkg/sql/tenant_update.go index 70e6a55f5f91..7dc616c139b0 100644 --- a/pkg/sql/tenant_update.go +++ b/pkg/sql/tenant_update.go @@ -23,8 +23,10 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/isql" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" + "github.com/cockroachdb/cockroach/pkg/sql/privilege" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" + "github.com/cockroachdb/cockroach/pkg/sql/syntheticprivilege" "github.com/cockroachdb/cockroach/pkg/util/log/logcrash" "github.com/cockroachdb/cockroach/pkg/util/protoutil" "github.com/cockroachdb/errors" @@ -151,7 +153,7 @@ func (p *planner) UpdateTenantResourceLimits( asOfConsumedRequestUnits float64, ) error { const op = "update-resource-limits" - if err := p.RequireAdminRole(ctx, "update tenant resource limits"); err != nil { + if err := p.CheckPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.REPAIRCLUSTERMETADATA); err != nil { return err } diff --git a/pkg/sql/tests/copy_file_upload_test.go b/pkg/sql/tests/copy_file_upload_test.go index 6dee2381fefc..387d828155e7 100644 --- a/pkg/sql/tests/copy_file_upload_test.go +++ b/pkg/sql/tests/copy_file_upload_test.go @@ -299,7 +299,7 @@ func TestNodelocalNotAdmin(t *testing.T) { writeFile(t, testSendFile, fileContent) err = runCopyFile(t, userDB, smithUserName, testSendFile, sql.NodelocalFileUploadTable) - expectedErr := "only users with the admin role are allowed to upload" + expectedErr := "only users with the admin role are allowed to upload to nodelocal" require.True(t, testutils.IsError(err, expectedErr)) }