Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

server: replace admin checks with VIEWCLUSTERMETADATA and REPAIRCLUSTERMETADATA #110609

Merged
merged 2 commits into from
Sep 22, 2023
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions pkg/ccl/backupccl/alter_backup_schedule.go
Original file line number Diff line number Diff line change
@@ -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(
10 changes: 5 additions & 5 deletions pkg/ccl/backupccl/testdata/backup-restore/schedule-privileges
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -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.
Original file line number Diff line number Diff line change
@@ -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.
14 changes: 14 additions & 0 deletions pkg/ccl/serverccl/statusccl/tenant_status_test.go
Original file line number Diff line number Diff line change
@@ -159,6 +159,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")
@@ -1508,6 +1519,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) {
25 changes: 17 additions & 8 deletions pkg/server/admin.go
Original file line number Diff line number Diff line change
@@ -1547,7 +1547,7 @@ func (s *adminServer) Events(
) (_ *serverpb.EventsResponse, retErr error) {
ctx = s.AnnotateCtx(ctx)

userName, err := s.privilegeChecker.RequireAdminUser(ctx)
err := s.privilegeChecker.RequireViewClusterMetadataPermission(ctx)
if err != nil {
// NB: not using srverrors.ServerError() here since the priv checker
// already returns a proper gRPC error status.
@@ -1560,6 +1560,10 @@ func (s *adminServer) Events(
limit = apiconstants.DefaultAPIEventLimit
}

userName, err := authserver.UserFromIncomingRPCContext(ctx)
if err != nil {
return nil, srverrors.ServerError(ctx, err)
}
r, err := s.eventsHelper(ctx, req, userName, int(limit), 0, redactEvents)
if err != nil {
return nil, srverrors.ServerError(ctx, err)
@@ -3188,7 +3192,7 @@ func (s *systemAdminServer) EnqueueRange(
ctx = authserver.ForwardSQLIdentityThroughRPCCalls(ctx)
ctx = s.AnnotateCtx(ctx)

if _, err := s.privilegeChecker.RequireAdminUser(ctx); err != nil {
if err := s.privilegeChecker.RequireRepairClusterMetadataPermission(ctx); err != nil {
// NB: not using srverrors.ServerError() here since the priv checker
// already returns a proper gRPC error status.
return nil, err
@@ -3336,7 +3340,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.privilegeChecker.RequireAdminUser(ctx)
err := s.privilegeChecker.RequireRepairClusterMetadataPermission(ctx)
if err != nil {
// NB: not using srverrors.ServerError() here since the priv checker
// already returns a proper gRPC error status.
@@ -3346,6 +3350,11 @@ func (s *systemAdminServer) SendKVBatch(
return nil, grpcstatus.Errorf(codes.InvalidArgument, "BatchRequest cannot be nil")
}

user, err := authserver.UserFromIncomingRPCContext(ctx)
if err != nil {
return nil, srverrors.ServerError(ctx, err)
}

// Emit a structured log event for the call.
jsonpb := protoutil.JSONPb{}
baJSON, err := jsonpb.Marshal(ba)
@@ -3394,7 +3403,7 @@ func (s *systemAdminServer) RecoveryCollectReplicaInfo(
) error {
ctx := stream.Context()
ctx = s.server.AnnotateCtx(ctx)
_, err := s.privilegeChecker.RequireAdminUser(ctx)
err := s.privilegeChecker.RequireViewClusterMetadataPermission(ctx)
if err != nil {
return err
}
@@ -3409,7 +3418,7 @@ func (s *systemAdminServer) RecoveryCollectLocalReplicaInfo(
) error {
ctx := stream.Context()
ctx = s.server.AnnotateCtx(ctx)
_, err := s.privilegeChecker.RequireAdminUser(ctx)
err := s.privilegeChecker.RequireViewClusterMetadataPermission(ctx)
if err != nil {
return err
}
@@ -3422,7 +3431,7 @@ func (s *systemAdminServer) RecoveryStagePlan(
ctx context.Context, request *serverpb.RecoveryStagePlanRequest,
) (*serverpb.RecoveryStagePlanResponse, error) {
ctx = s.server.AnnotateCtx(ctx)
_, err := s.privilegeChecker.RequireAdminUser(ctx)
err := s.privilegeChecker.RequireRepairClusterMetadataPermission(ctx)
if err != nil {
return nil, err
}
@@ -3435,7 +3444,7 @@ func (s *systemAdminServer) RecoveryNodeStatus(
ctx context.Context, request *serverpb.RecoveryNodeStatusRequest,
) (*serverpb.RecoveryNodeStatusResponse, error) {
ctx = s.server.AnnotateCtx(ctx)
_, err := s.privilegeChecker.RequireAdminUser(ctx)
err := s.privilegeChecker.RequireViewClusterMetadataPermission(ctx)
if err != nil {
return nil, err
}
@@ -3447,7 +3456,7 @@ func (s *systemAdminServer) RecoveryVerify(
ctx context.Context, request *serverpb.RecoveryVerifyRequest,
) (*serverpb.RecoveryVerifyResponse, error) {
ctx = s.server.AnnotateCtx(ctx)
_, err := s.privilegeChecker.RequireAdminUser(ctx)
err := s.privilegeChecker.RequireViewClusterMetadataPermission(ctx)
if err != nil {
return nil, err
}
2 changes: 1 addition & 1 deletion pkg/server/index_usage_stats.go
Original file line number Diff line number Diff line change
@@ -136,7 +136,7 @@ func (s *statusServer) ResetIndexUsageStats(
ctx = authserver.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
}

8 changes: 6 additions & 2 deletions pkg/server/job_profiler.go
Original file line number Diff line number Diff line change
@@ -19,6 +19,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/jobs"
"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
"github.com/cockroachdb/cockroach/pkg/kv"
"github.com/cockroachdb/cockroach/pkg/server/authserver"
"github.com/cockroachdb/cockroach/pkg/server/serverpb"
"github.com/cockroachdb/cockroach/pkg/server/srverrors"
"github.com/cockroachdb/cockroach/pkg/sql"
@@ -40,8 +41,7 @@ func (s *statusServer) RequestJobProfilerExecutionDetails(
ctx context.Context, req *serverpb.RequestJobProfilerExecutionDetailsRequest,
) (*serverpb.RequestJobProfilerExecutionDetailsResponse, error) {
ctx = s.AnnotateCtx(ctx)
// TODO(adityamaru): Figure out the correct privileges required to request execution details.
user, err := s.privilegeChecker.RequireAdminUser(ctx)
err := s.privilegeChecker.RequireViewClusterMetadataPermission(ctx)
if err != nil {
return nil, err
}
@@ -79,6 +79,10 @@ func (s *statusServer) RequestJobProfilerExecutionDetails(
e.addLabelledGoroutines(ctx)
e.addClusterWideTraces(ctx)

user, err := authserver.UserFromIncomingRPCContext(ctx)
if err != nil {
return nil, srverrors.ServerError(ctx, err)
}
r, err := execCfg.JobRegistry.GetResumerForClaimedJob(jobID)
if err != nil {
return nil, err
6 changes: 1 addition & 5 deletions pkg/server/privchecker/api.go
Original file line number Diff line number Diff line change
@@ -38,11 +38,6 @@ type CheckerForRPCHandlers interface {
// responsibility to convert them through srverrors.ServerError.
GetUserAndRole(ctx context.Context) (userName username.SQLUsername, isAdmin bool, err error)

// RequireAdminUser validates the current user is
// an admin user. It returns the current user's name.
// Its error return is a gRPC error.
RequireAdminUser(ctx context.Context) (userName username.SQLUsername, err error)

// RequireViewActivityPermission validates the current user has the VIEWACTIVITY
// privilege or role option.
// Its error return is a gRPC error.
@@ -52,6 +47,7 @@ type CheckerForRPCHandlers interface {
RequireViewClusterSettingOrModifyClusterSettingPermission(ctx context.Context) error
RequireViewActivityAndNoViewActivityRedactedPermission(ctx context.Context) error
RequireViewClusterMetadataPermission(ctx context.Context) error
RequireRepairClusterMetadataPermission(ctx context.Context) error
RequireViewDebugPermission(ctx context.Context) error
}

37 changes: 23 additions & 14 deletions pkg/server/privchecker/privchecker.go
Original file line number Diff line number Diff line change
@@ -44,20 +44,6 @@ type adminPrivilegeChecker struct {
makeAuthzAccessor func(opName string) (sql.AuthorizationAccessor, func())
}

// RequireAdminUser is part of the CheckerForRPCHandlers interface.
func (c *adminPrivilegeChecker) RequireAdminUser(
ctx context.Context,
) (userName username.SQLUsername, err error) {
userName, isAdmin, err := c.GetUserAndRole(ctx)
if err != nil {
return userName, srverrors.ServerError(ctx, err)
}
if !isAdmin {
return userName, ErrRequiresAdmin
}
return userName, nil
}

// RequireViewActivityPermission is part of the CheckerForRPCHandlers interface.
func (c *adminPrivilegeChecker) RequireViewActivityPermission(ctx context.Context) (err error) {
userName, isAdmin, err := c.GetUserAndRole(ctx)
@@ -199,6 +185,29 @@ func (c *adminPrivilegeChecker) RequireViewClusterMetadataPermission(
privilege.VIEWCLUSTERMETADATA)
}

// RequireRepairClusterMetadataPermission requires the user have admin
// or the VIEWCLUSTERMETADATA 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 srverrors.ServerError(ctx, err)
}
if isAdmin {
return nil
}
if hasRepairClusterMetadata, err := c.HasGlobalPrivilege(ctx, userName, privilege.REPAIRCLUSTERMETADATA); err != nil {
return srverrors.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.
2 changes: 1 addition & 1 deletion pkg/server/sql_stats.go
Original file line number Diff line number Diff line change
@@ -27,7 +27,7 @@ func (s *statusServer) ResetSQLStats(
ctx = authserver.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
}

Loading