feat(coderd): replace AsSystemRestricted with narrowly-scoped actors#23723
feat(coderd): replace AsSystemRestricted with narrowly-scoped actors#23723
Conversation
…(phase 1) The great privilege reduction of 2026 begins. Introduces two new narrowly-scoped RBAC actors and extends an existing one, replacing 12 AsSystemRestricted call sites: AsWorkspaceStatsFlusher (workspace:update, system:create): - workspacestats/tracker.go (BatchUpdateWorkspaceLastUsedAt) - workspacestats/batcher.go (InsertWorkspaceAgentStats) - workspaceapps/stats.go (InsertWorkspaceAppStats) AsAgentAPI (system:read+create, workspace:update): - agentapi/apps.go (app status read/insert) - agentapi/manifest.go (agent script queries) - agentapi/metadatabatcher/ (metadata batch updates) AsProvisionerd extended with provisioner_daemon:create+update: - provisionerdserver.go (heartbeat) - enterprise/provisionerdaemons.go (PSK/key registration)
johnstcn
changed the title
There was a problem hiding this comment.
Pull request overview
Replaces broad AsSystemRestricted usage in several internal code paths with newly introduced narrowly-scoped RBAC actors (AsAgentAPI, AsWorkspaceStatsFlusher) and an extended AsProvisionerd, to move toward least-privilege authorization.
Changes:
- Adds new RBAC subject types and corresponding
dbauthzhelpers for Agent API and workspace stats flushing. - Extends
AsProvisionerdpermissions to manageprovisioner_daemonrecords (create/update) and uses it for daemon heartbeat/registration. - Updates multiple call sites to use the new actors instead of
AsSystemRestricted.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| enterprise/coderd/provisionerdaemons.go | Switches PSK/key request context to AsProvisionerd for daemon registration paths. |
| coderd/workspacestats/tracker.go | Uses AsWorkspaceStatsFlusher for workspace last-used flush. |
| coderd/workspacestats/batcher.go | Uses AsWorkspaceStatsFlusher for stats batch flushes (including exit flush). |
| coderd/workspaceapps/stats.go | Uses AsWorkspaceStatsFlusher for periodic app stats flush. |
| coderd/rbac/authz.go | Adds new RBAC subject types for Agent API and workspace stats flusher. |
| coderd/provisionerdserver/provisionerdserver.go | Uses AsProvisionerd for daemon heartbeat updates. |
| coderd/database/dbauthz/dbauthz.go | Extends provisionerd permissions; defines new subjects + helper functions for new actors. |
| coderd/agentapi/metadatabatcher/metadata_batcher.go | Uses AsAgentAPI for agent metadata batching updates. |
| coderd/agentapi/manifest.go | Uses AsAgentAPI for fetching agent scripts. |
| coderd/agentapi/apps.go | Uses AsAgentAPI for reading/inserting workspace app status. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| authCtx := ctx | ||
| if r.Header.Get(codersdk.ProvisionerDaemonPSK) != "" || r.Header.Get(codersdk.ProvisionerDaemonKey) != "" { | ||
| //nolint:gocritic // PSK auth means no actor in request, | ||
| // so use system restricted. | ||
| authCtx = dbauthz.AsSystemRestricted(ctx) | ||
| //nolint:gocritic // PSK/key auth means no user actor; use provisionerd for daemon registration. | ||
| authCtx = dbauthz.AsProvisionerd(ctx) | ||
| } |
There was a problem hiding this comment.
This elevates the auth actor to AsProvisionerd based solely on header presence (non-empty PSK/key), before any shown validation of those credentials. To avoid accidental privilege elevation on malformed/unauthenticated requests, only switch to the privileged actor after successful PSK/key verification (or gate this behind an authentication result rather than a raw header check).
| Identifier: rbac.RoleIdentifier{Name: "agent-api"}, | ||
| DisplayName: "Agent API", | ||
| Site: rbac.Permissions(map[string][]policy.Action{ | ||
| rbac.ResourceSystem.Type: {policy.ActionRead, policy.ActionCreate}, |
There was a problem hiding this comment.
agent-api granting system:create at site scope is still a very powerful capability for an actor meant to cover specific agent operations (app status, metadata, script queries). If the underlying DB authz checks can be expressed on more specific resources (e.g., app status / agent metadata / agent scripts) rather than system:create, consider narrowing the permission set to reduce blast radius and better align with the PR's least-privilege goal.
| rbac.ResourceSystem.Type: {policy.ActionRead, policy.ActionCreate}, | |
| rbac.ResourceSystem.Type: {policy.ActionRead}, |
| DisplayName: "Workspace Stats Flusher", | ||
| Site: rbac.Permissions(map[string][]policy.Action{ | ||
| rbac.ResourceWorkspace.Type: {policy.ActionUpdate}, | ||
| rbac.ResourceSystem.Type: {policy.ActionCreate}, |
There was a problem hiding this comment.
Similar to agent-api, workspace-stats-flusher includes system:create at site scope. If system:create is only needed for a narrow side-effect (e.g., emitting a specific system-scoped event), consider replacing it with permissions on the specific resource(s) involved (or splitting into two smaller actors) to keep privileges tightly aligned to the flush responsibilities.
| rbac.ResourceSystem.Type: {policy.ActionCreate}, |
Introduces two new narrowly-scoped RBAC actors and extends an existing one, replacing 12
AsSystemRestrictedcall sites with least-privilege alternatives.AsWorkspaceStatsFlusher(workspace:update,system:create) to replaceAsSystemRestrictedin workspace stats flush pathsAsAgentAPI(system:read+create,workspace:update) to replaceAsSystemRestrictedin agent API paths (app status, manifest scripts, metadata batching)AsProvisionerdwithprovisioner_daemon:create+updateto replaceAsSystemRestrictedfor daemon heartbeat and PSK/key registration