Skip to content

Commit

Permalink
server: replace admin checks with VIEWCLUSTERMETADATA and REPAIRCLUST…
Browse files Browse the repository at this point in the history 
…ERMETADATA

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.
  • Loading branch information
@rafiss @yuzefovich
rafiss authored and yuzefovich committed Sep 25, 2023
1 parent b08ec85 commit f4197e7
Show file tree
Hide file tree
Showing 5 changed files with 80 additions and 48 deletions.
14 changes: 14 additions & 0 deletions pkg/ccl/serverccl/statusccl/tenant_status_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down Expand Up @@ -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) {
Expand Down
62 changes: 40 additions & 22 deletions pkg/server/admin.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand All @@ -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)
Expand Down Expand Up @@ -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
}
Expand All @@ -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
}
Expand All @@ -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
}
Expand All @@ -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
}
Expand All @@ -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
}
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion pkg/server/index_usage_stats.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
2 changes: 1 addition & 1 deletion pkg/server/sql_stats.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
48 changes: 24 additions & 24 deletions pkg/server/status.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}

Expand All @@ -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
}

Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}

Expand Down

0 comments on commit f4197e7

Please sign in to comment.