From f4197e726e8dd7a0c5b8f7d7725dd14b862c9f18 Mon Sep 17 00:00:00 2001 From: Rafi Shamim Date: Wed, 13 Sep 2023 17:15:57 -0400 Subject: [PATCH 1/2] server: replace admin checks with VIEWCLUSTERMETADATA and REPAIRCLUSTERMETADATA Release note (security update): Endpoints in the admin and status server that previously required the admin role now can be used by users with the VIEWCLUSTERMETADATA or REPAIRCLUSTERMETADATA system privilege, depending on whether the endpoint is read-only or can modify state. --- .../serverccl/statusccl/tenant_status_test.go | 14 +++++ pkg/server/admin.go | 62 ++++++++++++------- pkg/server/index_usage_stats.go | 2 +- pkg/server/sql_stats.go | 2 +- pkg/server/status.go | 48 +++++++------- 5 files changed, 80 insertions(+), 48 deletions(-) diff --git a/pkg/ccl/serverccl/statusccl/tenant_status_test.go b/pkg/ccl/serverccl/statusccl/tenant_status_test.go index e716103b1c11..645bfd42e4ca 100644 --- a/pkg/ccl/serverccl/statusccl/tenant_status_test.go +++ b/pkg/ccl/serverccl/statusccl/tenant_status_test.go @@ -161,6 +161,17 @@ func testTenantSpanStats(ctx context.Context, t *testing.T, helper serverccl.Ten require.Error(t, err) require.Contains(t, err.Error(), "Forbidden") + // VIEWCLUSTERMETADATA should allow the user to see the span stats. + grantStmt := `GRANT SYSTEM VIEWCLUSTERMETADATA TO authentic_user_noadmin;` + helper.TestCluster().TenantConn(0).Exec(t, grantStmt) + + err = client.PostJSONChecked("/_status/span", &req, &resp) + require.NoError(t, err) + require.NotEmpty(t, resp.SpanToStats) + + revokeStmt := `REVOKE SYSTEM VIEWCLUSTERMETADATA FROM authentic_user_noadmin;` + helper.TestCluster().TenantConn(0).Exec(t, revokeStmt) + adminClient := helper.TestCluster().TenantHTTPClient(t, 1, true) adminClient.PostJSON("/_status/span", &req, &resp) require.Greaterf(t, resp.SpanToStats[aSpan.String()].RangeCount, int32(0), "positive range count") @@ -1511,6 +1522,9 @@ func testTenantHotRanges(_ context.Context, t *testing.T, helper serverccl.Tenan client.PostJSON("/_status/v2/hotranges", &req, &resp) require.NotEmpty(t, resp.Ranges) + + revokeStmt := `REVOKE SYSTEM VIEWCLUSTERMETADATA FROM authentic_user_noadmin;` + helper.TestCluster().TenantConn(0).Exec(t, revokeStmt) }) t.Run("test tenant hot ranges respects tenant isolation", func(t *testing.T) { diff --git a/pkg/server/admin.go b/pkg/server/admin.go index 78c8a0702c7b..e36fa47acb81 100644 --- a/pkg/server/admin.go +++ b/pkg/server/admin.go @@ -1582,7 +1582,7 @@ func (s *adminServer) Events( ) (_ *serverpb.EventsResponse, retErr error) { ctx = s.AnnotateCtx(ctx) - userName, err := s.requireAdminUser(ctx) + err := s.requireViewClusterMetadataPermission(ctx) if err != nil { // NB: not using serverError() here since the priv checker // already returns a proper gRPC error status. @@ -1595,6 +1595,10 @@ func (s *adminServer) Events( limit = defaultAPIEventLimit } + userName, err := userFromIncomingRPCContext(ctx) + if err != nil { + return nil, serverError(ctx, err) + } r, err := s.eventsHelper(ctx, req, userName, int(limit), 0, redactEvents) if err != nil { return nil, serverError(ctx, err) @@ -3241,7 +3245,7 @@ func (s *systemAdminServer) EnqueueRange( ctx = forwardSQLIdentityThroughRPCCalls(ctx) ctx = s.AnnotateCtx(ctx) - if _, err := s.requireAdminUser(ctx); err != nil { + if err := s.requireRepairClusterMetadataPermission(ctx); err != nil { // NB: not using serverError() here since the priv checker // already returns a proper gRPC error status. return nil, err @@ -3389,7 +3393,7 @@ func (s *systemAdminServer) SendKVBatch( ctx = s.AnnotateCtx(ctx) // Note: the root user will bypass SQL auth checks, which is useful in case of // a cluster outage. - user, err := s.requireAdminUser(ctx) + err := s.requireRepairClusterMetadataPermission(ctx) if err != nil { // NB: not using serverError() here since the priv checker // already returns a proper gRPC error status. @@ -3399,6 +3403,11 @@ func (s *systemAdminServer) SendKVBatch( return nil, grpcstatus.Errorf(codes.InvalidArgument, "BatchRequest cannot be nil") } + user, err := userFromIncomingRPCContext(ctx) + if err != nil { + return nil, serverError(ctx, err) + } + // Emit a structured log event for the call. jsonpb := protoutil.JSONPb{} baJSON, err := jsonpb.Marshal(ba) @@ -3447,7 +3456,7 @@ func (s *systemAdminServer) RecoveryCollectReplicaInfo( ) error { ctx := stream.Context() ctx = s.server.AnnotateCtx(ctx) - _, err := s.requireAdminUser(ctx) + err := s.requireViewClusterMetadataPermission(ctx) if err != nil { return err } @@ -3462,7 +3471,7 @@ func (s *systemAdminServer) RecoveryCollectLocalReplicaInfo( ) error { ctx := stream.Context() ctx = s.server.AnnotateCtx(ctx) - _, err := s.requireAdminUser(ctx) + err := s.requireViewClusterMetadataPermission(ctx) if err != nil { return err } @@ -3475,7 +3484,7 @@ func (s *systemAdminServer) RecoveryStagePlan( ctx context.Context, request *serverpb.RecoveryStagePlanRequest, ) (*serverpb.RecoveryStagePlanResponse, error) { ctx = s.server.AnnotateCtx(ctx) - _, err := s.requireAdminUser(ctx) + err := s.requireRepairClusterMetadataPermission(ctx) if err != nil { return nil, err } @@ -3488,7 +3497,7 @@ func (s *systemAdminServer) RecoveryNodeStatus( ctx context.Context, request *serverpb.RecoveryNodeStatusRequest, ) (*serverpb.RecoveryNodeStatusResponse, error) { ctx = s.server.AnnotateCtx(ctx) - _, err := s.requireAdminUser(ctx) + err := s.requireViewClusterMetadataPermission(ctx) if err != nil { return nil, err } @@ -3500,7 +3509,7 @@ func (s *systemAdminServer) RecoveryVerify( ctx context.Context, request *serverpb.RecoveryVerifyRequest, ) (*serverpb.RecoveryVerifyResponse, error) { ctx = s.server.AnnotateCtx(ctx) - _, err := s.requireAdminUser(ctx) + err := s.requireViewClusterMetadataPermission(ctx) if err != nil { return nil, err } @@ -3918,20 +3927,6 @@ type adminPrivilegeChecker struct { makePlanner func(opName string) (interface{}, func()) } -// requireAdminUser's error return is a gRPC error. -func (c *adminPrivilegeChecker) requireAdminUser( - ctx context.Context, -) (userName username.SQLUsername, err error) { - userName, isAdmin, err := c.getUserAndRole(ctx) - if err != nil { - return userName, serverError(ctx, err) - } - if !isAdmin { - return userName, errRequiresAdmin - } - return userName, nil -} - // requireViewActivityPermission's error return is a gRPC error. func (c *adminPrivilegeChecker) requireViewActivityPermission(ctx context.Context) (err error) { userName, isAdmin, err := c.getUserAndRole(ctx) @@ -4071,6 +4066,29 @@ func (c *adminPrivilegeChecker) requireViewClusterMetadataPermission( privilege.VIEWCLUSTERMETADATA) } +// requireRepairClusterMetadataPermission requires the user have admin +// or the REPAIRCLUSTERMETADATA system privilege and returns an error if +// the user does not have it. +func (c *adminPrivilegeChecker) requireRepairClusterMetadataPermission( + ctx context.Context, +) (err error) { + userName, isAdmin, err := c.getUserAndRole(ctx) + if err != nil { + return serverError(ctx, err) + } + if isAdmin { + return nil + } + if hasRepairClusterMetadata, err := c.hasGlobalPrivilege(ctx, userName, privilege.REPAIRCLUSTERMETADATA); err != nil { + return serverError(ctx, err) + } else if hasRepairClusterMetadata { + return nil + } + return grpcstatus.Errorf( + codes.PermissionDenied, "this operation requires the %s system privilege", + privilege.REPAIRCLUSTERMETADATA) +} + // requireViewDebugPermission requires the user have admin or the VIEWDEBUG system privilege // and returns an error if the user does not have it. func (c *adminPrivilegeChecker) requireViewDebugPermission(ctx context.Context) (err error) { diff --git a/pkg/server/index_usage_stats.go b/pkg/server/index_usage_stats.go index d342f6f78a77..addf77d6eef1 100644 --- a/pkg/server/index_usage_stats.go +++ b/pkg/server/index_usage_stats.go @@ -134,7 +134,7 @@ func (s *statusServer) ResetIndexUsageStats( ctx = forwardSQLIdentityThroughRPCCalls(ctx) ctx = s.AnnotateCtx(ctx) - if _, err := s.privilegeChecker.requireAdminUser(ctx); err != nil { + if err := s.privilegeChecker.requireRepairClusterMetadataPermission(ctx); err != nil { return nil, err } diff --git a/pkg/server/sql_stats.go b/pkg/server/sql_stats.go index 0fd95d1a820c..ecb7fe59a509 100644 --- a/pkg/server/sql_stats.go +++ b/pkg/server/sql_stats.go @@ -26,7 +26,7 @@ func (s *statusServer) ResetSQLStats( ctx = forwardSQLIdentityThroughRPCCalls(ctx) ctx = s.AnnotateCtx(ctx) - if _, err := s.privilegeChecker.requireAdminUser(ctx); err != nil { + if err := s.privilegeChecker.requireRepairClusterMetadataPermission(ctx); err != nil { return nil, err } diff --git a/pkg/server/status.go b/pkg/server/status.go index 88c1070cb534..24f7ed374db5 100644 --- a/pkg/server/status.go +++ b/pkg/server/status.go @@ -708,7 +708,7 @@ func (s *systemStatusServer) Gossip( ctx = forwardSQLIdentityThroughRPCCalls(ctx) ctx = s.AnnotateCtx(ctx) - if _, err := s.privilegeChecker.requireAdminUser(ctx); err != nil { + if err := s.privilegeChecker.requireViewClusterMetadataPermission(ctx); err != nil { // NB: not using serverError() here since the priv checker // already returns a proper gRPC error status. return nil, err @@ -736,7 +736,7 @@ func (s *systemStatusServer) EngineStats( ctx = forwardSQLIdentityThroughRPCCalls(ctx) ctx = s.AnnotateCtx(ctx) - if _, err := s.privilegeChecker.requireAdminUser(ctx); err != nil { + if err := s.privilegeChecker.requireViewClusterMetadataPermission(ctx); err != nil { // NB: not using serverError() here since the priv checker // already returns a proper gRPC error status. return nil, err @@ -875,7 +875,7 @@ func (s *systemStatusServer) CriticalNodes( ctx context.Context, req *serverpb.CriticalNodesRequest, ) (*serverpb.CriticalNodesResponse, error) { ctx = s.AnnotateCtx(ctx) - if _, err := s.privilegeChecker.requireAdminUser(ctx); err != nil { + if err := s.privilegeChecker.requireViewClusterMetadataPermission(ctx); err != nil { return nil, err } conformance, err := s.node.SpanConfigConformance( @@ -1008,7 +1008,7 @@ func (s *statusServer) Certificates( ctx = forwardSQLIdentityThroughRPCCalls(ctx) ctx = s.AnnotateCtx(ctx) - if _, err := s.privilegeChecker.requireAdminUser(ctx); err != nil { + if err := s.privilegeChecker.requireViewClusterMetadataPermission(ctx); err != nil { // NB: not using serverError() here since the priv checker // already returns a proper gRPC error status. return nil, err @@ -1129,7 +1129,7 @@ func (s *statusServer) Details( ctx = forwardSQLIdentityThroughRPCCalls(ctx) ctx = s.AnnotateCtx(ctx) - if _, err := s.privilegeChecker.requireAdminUser(ctx); err != nil { + if err := s.privilegeChecker.requireViewClusterMetadataPermission(ctx); err != nil { // NB: not using serverError() here since the priv checker // already returns a proper gRPC error status. return nil, err @@ -1170,7 +1170,7 @@ func (s *statusServer) GetFiles( ctx = forwardSQLIdentityThroughRPCCalls(ctx) ctx = s.AnnotateCtx(ctx) - if _, err := s.privilegeChecker.requireAdminUser(ctx); err != nil { + if err := s.privilegeChecker.requireViewClusterMetadataPermission(ctx); err != nil { // NB: not using serverError() here since the priv checker // already returns a proper gRPC error status. return nil, err @@ -1226,7 +1226,7 @@ func (s *statusServer) LogFilesList( ctx = forwardSQLIdentityThroughRPCCalls(ctx) ctx = s.AnnotateCtx(ctx) - if _, err := s.privilegeChecker.requireAdminUser(ctx); err != nil { + if err := s.privilegeChecker.requireViewClusterMetadataPermission(ctx); err != nil { // NB: not using serverError() here since the priv checker // already returns a proper gRPC error status. return nil, err @@ -1261,7 +1261,7 @@ func (s *statusServer) LogFile( ctx = forwardSQLIdentityThroughRPCCalls(ctx) ctx = s.AnnotateCtx(ctx) - if _, err := s.privilegeChecker.requireAdminUser(ctx); err != nil { + if err := s.privilegeChecker.requireViewClusterMetadataPermission(ctx); err != nil { // NB: not using serverError() here since the priv checker // already returns a proper gRPC error status. return nil, err @@ -1359,7 +1359,7 @@ func (s *statusServer) Logs( ctx = forwardSQLIdentityThroughRPCCalls(ctx) ctx = s.AnnotateCtx(ctx) - if _, err := s.privilegeChecker.requireAdminUser(ctx); err != nil { + if err := s.privilegeChecker.requireViewClusterMetadataPermission(ctx); err != nil { // NB: not using serverError() here since the priv checker // already returns a proper gRPC error status. return nil, err @@ -1447,7 +1447,7 @@ func (s *statusServer) Stacks( ctx = forwardSQLIdentityThroughRPCCalls(ctx) ctx = s.AnnotateCtx(ctx) - if _, err := s.privilegeChecker.requireAdminUser(ctx); err != nil { + if err := s.privilegeChecker.requireViewClusterMetadataPermission(ctx); err != nil { // NB: not using serverError() here since the priv checker // already returns a proper gRPC error status. return nil, err @@ -1480,7 +1480,7 @@ func (s *statusServer) Profile( ctx = forwardSQLIdentityThroughRPCCalls(ctx) ctx = s.AnnotateCtx(ctx) - if _, err := s.privilegeChecker.requireAdminUser(ctx); err != nil { + if err := s.privilegeChecker.requireViewClusterMetadataPermission(ctx); err != nil { // NB: not using serverError() here since the priv checker // already returns a proper gRPC error status. return nil, err @@ -1560,8 +1560,8 @@ func (s *statusServer) NodesList( ctx = s.AnnotateCtx(ctx) // The node status contains details about the command line, network - // addresses, env vars etc which are admin-only. - if _, err := s.privilegeChecker.requireAdminUser(ctx); err != nil { + // addresses, env vars etc which are privileged information. + if err := s.privilegeChecker.requireViewClusterMetadataPermission(ctx); err != nil { // NB: not using serverError() here since the priv checker // already returns a proper gRPC error status. return nil, err @@ -1835,8 +1835,8 @@ func (s *statusServer) Node( ctx = s.AnnotateCtx(ctx) // The node status contains details about the command line, network - // addresses, env vars etc which are admin-only. - if _, err := s.privilegeChecker.requireAdminUser(ctx); err != nil { + // addresses, env vars etc which are privileged information.. + if err := s.privilegeChecker.requireViewClusterMetadataPermission(ctx); err != nil { // NB: not using serverError() here since the priv checker // already returns a proper gRPC error status. return nil, err @@ -2255,8 +2255,8 @@ func (t *statusServer) TenantRanges( ctx = forwardSQLIdentityThroughRPCCalls(ctx) ctx = t.AnnotateCtx(ctx) - // The tenant range report contains replica metadata which is admin-only. - if _, err := t.privilegeChecker.requireAdminUser(ctx); err != nil { + // The tenant range report contains replica metadata which is privileged. + if err := t.privilegeChecker.requireViewClusterMetadataPermission(ctx); err != nil { return nil, err } @@ -2268,7 +2268,7 @@ func (s *systemStatusServer) TenantRanges( ) (*serverpb.TenantRangesResponse, error) { forwardSQLIdentityThroughRPCCalls(ctx) ctx = s.AnnotateCtx(ctx) - if _, err := s.privilegeChecker.requireAdminUser(ctx); err != nil { + if err := s.privilegeChecker.requireViewClusterMetadataPermission(ctx); err != nil { return nil, err } @@ -2727,7 +2727,7 @@ func (s *statusServer) KeyVisSamples( ctx context.Context, req *serverpb.KeyVisSamplesRequest, ) (*serverpb.KeyVisSamplesResponse, error) { - if _, err := s.privilegeChecker.requireAdminUser(ctx); err != nil { + if err := s.privilegeChecker.requireViewClusterMetadataPermission(ctx); err != nil { return nil, err } @@ -3471,7 +3471,7 @@ func (s *statusServer) SpanStats( ctx context.Context, req *roachpb.SpanStatsRequest, ) (*roachpb.SpanStatsResponse, error) { ctx = s.AnnotateCtx(ctx) - if _, err := s.privilegeChecker.requireAdminUser(ctx); err != nil { + if err := s.privilegeChecker.requireViewClusterMetadataPermission(ctx); err != nil { // NB: not using serverError() here since the priv checker // already returns a proper gRPC error status. return nil, err @@ -3490,7 +3490,7 @@ func (s *systemStatusServer) SpanStats( ) (*roachpb.SpanStatsResponse, error) { ctx = forwardSQLIdentityThroughRPCCalls(ctx) ctx = s.AnnotateCtx(ctx) - if _, err := s.privilegeChecker.requireAdminUser(ctx); err != nil { + if err := s.privilegeChecker.requireViewClusterMetadataPermission(ctx); err != nil { // NB: not using serverError() here since the priv checker // already returns a proper gRPC error status. return nil, err @@ -3661,7 +3661,7 @@ func (s *statusServer) JobRegistryStatus( ctx = forwardSQLIdentityThroughRPCCalls(ctx) ctx = s.AnnotateCtx(ctx) - if _, err := s.privilegeChecker.requireAdminUser(ctx); err != nil { + if err := s.privilegeChecker.requireViewClusterMetadataPermission(ctx); err != nil { // NB: not using serverError() here since the priv checker // already returns a proper gRPC error status. return nil, err @@ -3699,7 +3699,7 @@ func (s *statusServer) JobStatus( ) (*serverpb.JobStatusResponse, error) { ctx = s.AnnotateCtx(forwardSQLIdentityThroughRPCCalls(ctx)) - if _, err := s.privilegeChecker.requireAdminUser(ctx); err != nil { + if err := s.privilegeChecker.requireViewClusterMetadataPermission(ctx); err != nil { // NB: not using serverError() here since the priv checker // already returns a proper gRPC error status. return nil, err @@ -3731,7 +3731,7 @@ func (s *statusServer) TxnIDResolution( ctx context.Context, req *serverpb.TxnIDResolutionRequest, ) (*serverpb.TxnIDResolutionResponse, error) { ctx = s.AnnotateCtx(forwardSQLIdentityThroughRPCCalls(ctx)) - if _, err := s.privilegeChecker.requireAdminUser(ctx); err != nil { + if err := s.privilegeChecker.requireViewClusterMetadataPermission(ctx); err != nil { return nil, err } From 548cd02f826fd9f69788cf59747c3a24c4c57be7 Mon Sep 17 00:00:00 2001 From: Rafi Shamim Date: Wed, 13 Sep 2023 17:35:20 -0400 Subject: [PATCH 2/2] sql: remove usages of UserHasAdminRole This commit covers a few cases that were missed by an earlier commit: https://github.com/cockroachdb/cockroach/pull/110084. No release note is included since it would be redundant with the release note from that PR. Release note: None --- pkg/ccl/backupccl/alter_backup_schedule.go | 12 +++++++----- .../testdata/backup-restore/schedule-privileges | 10 +++++----- .../multi-tenant/show_create_external_connections | 6 +++--- .../testdata/show_create_external_connections | 6 +++--- pkg/sql/control_schedules.go | 13 +++++++++---- pkg/sql/repair.go | 6 +----- pkg/sql/show_create_external_connection.go | 12 +++++++----- pkg/sql/show_create_schedule.go | 11 ++++------- 8 files changed, 39 insertions(+), 37 deletions(-) diff --git a/pkg/ccl/backupccl/alter_backup_schedule.go b/pkg/ccl/backupccl/alter_backup_schedule.go index bcd236ea0a6c..ec153e15a084 100644 --- a/pkg/ccl/backupccl/alter_backup_schedule.go +++ b/pkg/ccl/backupccl/alter_backup_schedule.go @@ -24,7 +24,9 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/parser" "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/syntheticprivilege" "github.com/cockroachdb/errors" pbtypes "github.com/gogo/protobuf/types" ) @@ -127,16 +129,16 @@ func doAlterBackupSchedules( s.incJob.ScheduleID()) } - // Check that the user is admin or the owner of the schedules being altered. - isAdmin, err := p.UserHasAdminRole(ctx, p.User()) + // Check that the user has privileges or is the owner of the schedules being altered. + hasPriv, err := p.HasPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.REPAIRCLUSTERMETADATA, p.User()) if err != nil { return err } isOwnerOfFullJob := s.fullJob == nil || s.fullJob.Owner() == p.User() isOwnerOfIncJob := s.incJob == nil || s.incJob.Owner() == p.User() - if !isAdmin && !(isOwnerOfFullJob && isOwnerOfIncJob) { - return pgerror.New(pgcode.InsufficientPrivilege, "must be admin or owner of the "+ - "schedules being altered.") + if !hasPriv && !(isOwnerOfFullJob && isOwnerOfIncJob) { + return pgerror.Newf(pgcode.InsufficientPrivilege, "must be admin or the owner of the "+ + "schedules being altered, or have %s privilege", privilege.REPAIRCLUSTERMETADATA) } if s, err = processFullBackupRecurrence( diff --git a/pkg/ccl/backupccl/testdata/backup-restore/schedule-privileges b/pkg/ccl/backupccl/testdata/backup-restore/schedule-privileges index fa9ab4fdbc47..28f98eb2f68f 100644 --- a/pkg/ccl/backupccl/testdata/backup-restore/schedule-privileges +++ b/pkg/ccl/backupccl/testdata/backup-restore/schedule-privileges @@ -108,7 +108,7 @@ foocluster_admin BACKUP INTO LATEST IN 'external://foo/cluster' WITH OPTIONS (de foocluster_admin BACKUP INTO 'external://foo/cluster' WITH OPTIONS (detached) # nonadmin testuser is not allowed to drop a schedule they do not own. -exec-sql expect-error-regex=(must be admin or owner of the schedule [0-9]+ to DROP it) user=testuser +exec-sql expect-error-regex=(must have REPAIRCLUSTERMETADATA privilege or be owner of the schedule [0-9]+ to DROP it) user=testuser DROP SCHEDULE $fullID ---- regex matches error @@ -141,7 +141,7 @@ let $otherFullID $otherIncID with schedules as (show schedules) select id from schedules where label='foocluster_admin_revoke' order by command->>'backup_type' asc; ---- -exec-sql expect-error-regex=(must be admin or owner of the schedule [0-9]+ to DROP it) user=testuser +exec-sql expect-error-regex=(must have REPAIRCLUSTERMETADATA privilege or be owner of the schedule [0-9]+ to DROP it) user=testuser DROP SCHEDULE $otherFullID ---- regex matches error @@ -180,17 +180,17 @@ DROP SCHEDULE $testuserIncID; ---- # But testuser can't drop, alter, resume or pause the root owned schedules. -exec-sql expect-error-regex=(must be admin or owner of the schedule [0-9]+ to PAUSE it) user=testuser +exec-sql expect-error-regex=(must have REPAIRCLUSTERMETADATA privilege or be owner of the schedule [0-9]+ to PAUSE it) user=testuser PAUSE SCHEDULE $otherFullID ---- regex matches error -exec-sql expect-error-regex=(must be admin or owner of the schedule [0-9]+ to RESUME it) user=testuser +exec-sql expect-error-regex=(must have REPAIRCLUSTERMETADATA privilege or be owner of the schedule [0-9]+ to RESUME it) user=testuser RESUME SCHEDULE $otherFullID ---- regex matches error -exec-sql expect-error-regex=(must be admin or owner of the schedule [0-9]+ to DROP it) user=testuser +exec-sql expect-error-regex=(must have REPAIRCLUSTERMETADATA privilege or be owner of the schedule [0-9]+ to DROP it) user=testuser DROP SCHEDULE $otherFullID; ---- regex matches error diff --git a/pkg/ccl/cloudccl/externalconn/testdata/multi-tenant/show_create_external_connections b/pkg/ccl/cloudccl/externalconn/testdata/multi-tenant/show_create_external_connections index 840901baab39..f2a01adfa423 100644 --- a/pkg/ccl/cloudccl/externalconn/testdata/multi-tenant/show_create_external_connections +++ b/pkg/ccl/cloudccl/externalconn/testdata/multi-tenant/show_create_external_connections @@ -80,12 +80,12 @@ GRANT SYSTEM EXTERNALCONNECTION TO testuser query-sql user=testuser SHOW CREATE ALL EXTERNAL CONNECTIONS ---- -pq: must be admin to run `SHOW CREATE ALL EXTERNAL CONNECTIONS +pq: must have VIEWCLUSTERMETADATA privilege to run `SHOW CREATE ALL EXTERNAL CONNECTIONS` query-sql user=testuser SHOW CREATE EXTERNAL CONNECTION foo ---- -pq: must be admin or owner of the External Connection "foo" +pq: must have VIEWCLUSTERMETADATA privilege or be owner of the External Connection "foo" # Create External Connection where testuser is the owner, they should be able to SHOW this object. exec-sql user=testuser @@ -95,7 +95,7 @@ CREATE EXTERNAL CONNECTION bar AS 'nodelocal://1/foo' query-sql user=testuser SHOW CREATE ALL EXTERNAL CONNECTIONS ---- -pq: must be admin to run `SHOW CREATE ALL EXTERNAL CONNECTIONS +pq: must have VIEWCLUSTERMETADATA privilege to run `SHOW CREATE ALL EXTERNAL CONNECTIONS` # TODO(aditymaru): Synthetic privileges do not have a concept of owners. Once they do, testuser will # be able to run this query successfully since they are the owner of the External Connection object. diff --git a/pkg/ccl/cloudccl/externalconn/testdata/show_create_external_connections b/pkg/ccl/cloudccl/externalconn/testdata/show_create_external_connections index bb19510eab91..a25dd532b232 100644 --- a/pkg/ccl/cloudccl/externalconn/testdata/show_create_external_connections +++ b/pkg/ccl/cloudccl/externalconn/testdata/show_create_external_connections @@ -76,12 +76,12 @@ GRANT SYSTEM EXTERNALCONNECTION TO testuser query-sql user=testuser SHOW CREATE ALL EXTERNAL CONNECTIONS ---- -pq: must be admin to run `SHOW CREATE ALL EXTERNAL CONNECTIONS +pq: must have VIEWCLUSTERMETADATA privilege to run `SHOW CREATE ALL EXTERNAL CONNECTIONS` query-sql user=testuser SHOW CREATE EXTERNAL CONNECTION foo ---- -pq: must be admin or owner of the External Connection "foo" +pq: must have VIEWCLUSTERMETADATA privilege or be owner of the External Connection "foo" # Create External Connection where testuser is the owner, they should be able to SHOW this object. exec-sql user=testuser @@ -91,7 +91,7 @@ CREATE EXTERNAL CONNECTION bar AS 'nodelocal://1/foo' query-sql user=testuser SHOW CREATE ALL EXTERNAL CONNECTIONS ---- -pq: must be admin to run `SHOW CREATE ALL EXTERNAL CONNECTIONS +pq: must have VIEWCLUSTERMETADATA privilege to run `SHOW CREATE ALL EXTERNAL CONNECTIONS` # TODO(aditymaru): Synthetic privileges do not have a concept of owners. Once they do, testuser will # be able to run this query successfully since they are the owner of the External Connection object. diff --git a/pkg/sql/control_schedules.go b/pkg/sql/control_schedules.go index 24240f5aff56..85e3a53902c9 100644 --- a/pkg/sql/control_schedules.go +++ b/pkg/sql/control_schedules.go @@ -20,9 +20,11 @@ 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/sqltelemetry" + "github.com/cockroachdb/cockroach/pkg/sql/syntheticprivilege" "github.com/cockroachdb/errors" ) @@ -129,14 +131,17 @@ func (n *controlSchedulesNode) startExec(params runParams) error { continue // not an error if schedule does not exist } - isAdmin, err := params.p.UserHasAdminRole(params.ctx, params.p.User()) + // Check that the user has privileges or is the owner of the schedules being altered. + hasPriv, err := params.p.HasPrivilege( + params.ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.REPAIRCLUSTERMETADATA, params.p.User(), + ) if err != nil { return err } isOwner := schedule.Owner() == params.p.User() - if !isAdmin && !isOwner { - return pgerror.Newf(pgcode.InsufficientPrivilege, "must be admin or owner of the "+ - "schedule %d to %s it", schedule.ScheduleID(), n.command.String()) + if !hasPriv && !isOwner { + return pgerror.Newf(pgcode.InsufficientPrivilege, "must have %s privilege or be owner of the "+ + "schedule %d to %s it", privilege.REPAIRCLUSTERMETADATA, schedule.ScheduleID(), n.command.String()) } switch n.command { diff --git a/pkg/sql/repair.go b/pkg/sql/repair.go index b37a68859773..7be289db1a0f 100644 --- a/pkg/sql/repair.go +++ b/pkg/sql/repair.go @@ -712,13 +712,9 @@ func checkPlannerStateForRepairFunctions(ctx context.Context, p *planner, method if p.extendedEvalCtx.TxnReadOnly { return readOnlyError(method) } - hasAdmin, err := p.UserHasAdminRole(ctx, p.User()) - if err != nil { + if err := p.CheckPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.REPAIRCLUSTERMETADATA); err != nil { return err } - if !hasAdmin { - return pgerror.Newf(pgcode.InsufficientPrivilege, "admin role required for %s", method) - } return nil } diff --git a/pkg/sql/show_create_external_connection.go b/pkg/sql/show_create_external_connection.go index ec75d1b70a4f..31ee3caf921f 100644 --- a/pkg/sql/show_create_external_connection.go +++ b/pkg/sql/show_create_external_connection.go @@ -18,6 +18,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo" "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/sqltelemetry" @@ -74,12 +75,13 @@ func (p *planner) ShowCreateExternalConnection( ) (planNode, error) { var hasPrivileges bool var err error - if hasPrivileges, err = p.UserHasAdminRole(ctx, p.User()); err != nil { + if hasPrivileges, err = p.HasPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.VIEWCLUSTERMETADATA, p.User()); err != nil { return nil, err } - // If the user is not admin, and is running a `SHOW CREATE EXTERNAL CONNECTION foo` - // check if the user is the owner of the object. + // If the user does not have VIEWCLUSTERMETADATA, and is running a `SHOW + // CREATE EXTERNAL CONNECTION foo` check if the user is the owner of the + // object. exprEval := p.ExprEvaluator(externalConnectionOp) if !hasPrivileges && n.ConnectionLabel != nil { name, err := exprEval.String(ctx, n.ConnectionLabel) @@ -94,10 +96,10 @@ func (p *planner) ShowCreateExternalConnection( return nil, err } if !isOwner { - return nil, pgerror.Newf(pgcode.InsufficientPrivilege, "must be admin or owner of the External Connection %q", name) + return nil, pgerror.Newf(pgcode.InsufficientPrivilege, "must have %s privilege or be owner of the External Connection %q", privilege.VIEWCLUSTERMETADATA, name) } } else if !hasPrivileges { - return nil, pgerror.New(pgcode.InsufficientPrivilege, "must be admin to run `SHOW CREATE ALL EXTERNAL CONNECTIONS") + return nil, pgerror.Newf(pgcode.InsufficientPrivilege, "must have %s privilege to run `SHOW CREATE ALL EXTERNAL CONNECTIONS`", privilege.VIEWCLUSTERMETADATA) } sqltelemetry.IncrementShowCounter(sqltelemetry.CreateExternalConnection) diff --git a/pkg/sql/show_create_schedule.go b/pkg/sql/show_create_schedule.go index 55ddc7c4309d..8cf1eb46a6e2 100644 --- a/pkg/sql/show_create_schedule.go +++ b/pkg/sql/show_create_schedule.go @@ -18,11 +18,11 @@ import ( "github.com/cockroachdb/cockroach/pkg/jobs" "github.com/cockroachdb/cockroach/pkg/scheduledjobs" "github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo" - "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/sqltelemetry" + "github.com/cockroachdb/cockroach/pkg/sql/syntheticprivilege" "github.com/cockroachdb/cockroach/pkg/sql/types" ) @@ -85,12 +85,9 @@ func loadSchedules(params runParams, n *tree.ShowCreateSchedules) ([]*jobs.Sched func (p *planner) ShowCreateSchedule( ctx context.Context, n *tree.ShowCreateSchedules, ) (planNode, error) { - // Only admin users can execute SHOW CREATE SCHEDULE - if userIsAdmin, err := p.UserHasAdminRole(ctx, p.User()); err != nil { + // Only privileged users can execute SHOW CREATE SCHEDULE + if err := p.CheckPrivilege(ctx, syntheticprivilege.GlobalPrivilegeObject, privilege.VIEWCLUSTERMETADATA); err != nil { return nil, err - } else if !userIsAdmin { - return nil, pgerror.Newf(pgcode.InsufficientPrivilege, - "user %s does not have admin role", p.User()) } sqltelemetry.IncrementShowCounter(sqltelemetry.CreateSchedule)