feat(rbac): RPC permission declaration map + contract test (PR 2b of 4)#303
Conversation
🔐 Codex Security Review
Review SummaryOverall Risk: MEDIUM Findings[MEDIUM] Permission map does not prove handlers enforce permissions
[MEDIUM] Pending-migration bucket allows new unauthorised RPCs to pass CI
NotesThe reviewed diff only adds the RPC permission catalog and contract tests; I did not find command injection, SQL injection, protobuf wire-format, plugin, infrastructure, frontend, or pool-hijack changes in scope. I could not run Generated by Codex Security Review | |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c427ca79d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Infrastructure for the per-procedure authz migration. No behavior
change in this PR — the maps exist, the contract test classifies
every registered procedure, but no handler reads ProcedurePermissions
yet. PR 2c (existing-gate swap) and PR 2d (new-gate + migration) move
procedures from ProceduresPendingGate into ProcedurePermissions as
their handlers migrate to RequirePermission.
**rpc_permissions.go** — two maps:
- ProcedurePermissions{procedure → catalog key}: gated procedures.
Empty in this PR. Populated as handlers swap to RequirePermission.
- ProceduresPendingGate{procedure → note}: every authenticated
procedure on the production Connect mux that has not yet migrated.
Each entry carries a brief note about the current gate (legacy
RequireAdmin, inline role check, or "ungated"). 110 entries today;
shrinking to zero is the exit criterion for removing
middleware/admin.go.
**rpc_permissions_test.go** — the contract test:
- TestRPCContract_EveryRegisteredProcedureIsClassified uses reflection
on each generated *ServiceHandler interface to enumerate every
procedure registered on the production mux, then asserts each
appears in exactly one of UnauthenticatedProcedures,
FleetNodeAuthenticatedProcedures, ProcedurePermissions, or
ProceduresPendingGate. Adding a new RPC without classifying it
fails the test loudly; double-classification is also flagged.
- TestRPCContract_ProcedurePermissionsKeysAreInCatalog ensures every
gate key references a real catalog permission — a no-op today
because ProcedurePermissions is empty; becomes load-bearing as
PR 2c lands gates.
The registered-services list in the test mirrors the handler
registrations in cmd/fleetd/main.go. Adding a new service to
production requires adding it to that list; the build catches removal
(unused import).
Build, vet, golangci-lint, and the middleware test package all green.
…with main.go
Code review of PR 2b found that several entries in ProceduresPendingMigration
mislabel the actual current gate on the handler, which would mislead PR 2c's
migration into trusting a label that isn't backed by a real check.
- AuthService.ListUsers: labeled "SUPER_ADMIN role check"; the handler has
no role check, so any authenticated org member can enumerate users.
- Curtailment Start/Stop/Preview gates are conditional on override fields,
not blanket admin checks. UpdateCurtailmentEvent has no gate at all.
- FleetNodeAdmin Pair/Unpair/ListFleetNodeDevices/DiscoverOnFleetNode
fall through the embedded UnimplementedFleetNodeAdminServiceHandler and
return Unimplemented without reaching any role check.
Relabel each affected entry with the real state so the migration can
target the right gap.
Also:
- Rename ProceduresPendingGate -> ProceduresPendingMigration. The name
is more honest: many entries are flat-out ungated, not "pending" any
current gate.
- Add a guard so the classification test fails loudly if reflection
finds zero procedures (e.g., connect-go interface rename).
- Add a sync test that parses cmd/fleetd/main.go for v1connect handler
mounts and asserts registeredServices stays in lockstep. Without this,
adding a new service to main.go but forgetting to list it here is a
silent false-negative.
- Drop in-comment plan-unit references ("PR 2b", "follow-up PRs").
mcharles-square
force-pushed
the
feat/rbac-pr2b-rpc-permissions-infra
branch
from
4c427ca to
6bec15e
Compare
The previous `\w+v1connect` pattern locked the sync test to v1 services only. A future v2+ service mounted in main.go would silently escape the check, defeating the purpose of the guard. Reported by Codex on PR review (P2).
There was a problem hiding this comment.
Pull request overview
Adds RBAC “procedure classification” infrastructure for the Connect RPC surface by introducing exported procedure→permission/pending maps and a reflection-based contract test that enforces complete, non-overlapping classification of every RPC mounted in cmd/fleetd/main.go.
Changes:
- Introduces
ProcedurePermissionsandProceduresPendingMigrationas the canonical registry for authenticated procedures (gated vs pending migration). - Adds a contract test that reflects over generated
*ServiceHandlerinterfaces to enumerate mounted procedures and asserts every one is classified exactly once. - Adds a companion test ensuring any permission keys referenced by
ProcedurePermissionsexist in the authz catalog.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
server/internal/handlers/middleware/rpc_permissions.go |
Adds exported procedure classification maps (gated vs pending migration) using generated Connect procedure constants. |
server/internal/handlers/middleware/rpc_permissions_test.go |
Adds reflection + main.go-regex contract tests to ensure all mounted RPCs are classified and permission keys are valid. |
The previous version compared package shortnames only, so a second service interface added to an already-mounted vNconnect package (e.g. authv1connect.NewMultiOrgAuthServiceHandler alongside NewAuthServiceHandler) would silently pass the sync check even if registeredServices forgot to list it. Capture both the package shortname and the ServiceHandler short name from each mount, then compare against `<pkg>.<ServiceShortName>` derived from each registered handler's reflect.Type. A missing service now fails the test loudly regardless of whether its package was already imported. Reported by Codex on PR review (MEDIUM).
|
Re: the Codex security review:
This is deferred to PR 2d by design, not addressed here. PR 2b is infrastructure-only — it adds the
The follow-up stack:
Reviewers should treat any growth in
Addressed in commit 6f8b5f3. |
Stacked on #301 (PR 2a — resolver + middleware). Targets
feat/rbac-pr2a-resolveras base; will retarget tomainonce 2a merges.Second slice of PR 2 from the granular RBAC plan. Pure infrastructure — no behavior change. The maps exist, the contract test classifies every registered procedure, but no handler reads
ProcedurePermissionsyet.What landed
server/internal/handlers/middleware/rpc_permissions.goTwo exported maps:
ProcedurePermissions{procedure → catalog key}— gated procedures. Empty in this PR. Populated as handlers swap fromRequireAdmintoRequirePermissionin subsequent PRs.ProceduresPendingMigration{procedure → note}— every authenticated procedure on the production Connect mux that has not yet migrated. Each entry carries a brief note about the current gate:middleware.RequireAdmin— buildings (5), sites (6) — 11 proceduresinfo.Rolecheck — fleetnodeadmin (8), serverlog (1), apikey (3, via local helper) — 12 proceduresrequireAdminFromContext— curtailment writes (4) — 4 procedurescheckCanManageUser— auth user-management (3) — 3 proceduresSUPER_ADMINrole string check — auth ListUsers (1) — 1 procedureTotal: 110 entries. Shrinking this map to zero is the exit criterion for removing
middleware/admin.go.server/internal/handlers/middleware/rpc_permissions_test.goContract test that uses reflection on the generated
*ServiceHandlerinterfaces to enumerate every procedure registered on the production mux, then asserts each appears in exactly one of:interceptors.UnauthenticatedProceduresinterceptors.FleetNodeAuthenticatedProceduresProcedurePermissionsProceduresPendingMigrationAdding a new RPC without classifying it fails the test loudly. Double-classification is also flagged.
A companion test (
TestRPCContract_ProcedurePermissionsKeysAreInCatalog) ensures every gate key references a real catalog permission — a no-op today becauseProcedurePermissionsis empty; becomes load-bearing as PR 2c lands gates.What this PR does NOT do
RequireAdmintoRequirePermission— that's PR 2c (existing gates) and PR 2d (new gates).middleware/admin.go— that lands onceProceduresPendingMigrationis empty.user_organization.role_idor add the raising trigger — PR 2d.Test plan
go build ./...cleango vet ./...cleangolangci-lint run ./internal/handlers/middleware/...cleango test ./internal/handlers/middleware/...green (contract test + the existingRequirePermissionunit tests from 2a)Rollback
Reverting this PR removes two files. No schema change, no data migration, no runtime behavior change to reverse.