gsadmin/certctl
1
0
Fork 0
mirror of https://github.com/shankar0123/certctl.git synced 2026-06-13 03:39:17 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(auth): apply rbacGate to every state-changing + read handler (CRIT-1 closure)

Browse Source 
Closes the wire-layer authorization gap surfaced by the 2026-05-10 audit
(CRIT-1). Before this commit only ~24 of ~140 routes carried rbacGate
enforcement — all of them admin-only fine-grained perms (auth.session.*,
auth.oidc.*, auth.breakglass.admin, cert.bulk_revoke, crl.admin, scep.admin,
est.admin, ca.hierarchy.manage). Every catalogued legacy-CRUD perm
(cert.read/issue/revoke/delete, profile.edit/delete, issuer.edit/delete,
target.*, agent.*, plus role-mgmt verbs) was declared in
internal/domain/auth/validate.go but never wired at the router. A r-viewer
Bearer was essentially r-admin minus five verbs at the wire layer (CWE-862).

This commit:

- Adds rbacGateScoped(checker, perm, scopeType, scopeFn, h) helper to
  internal/api/router/router.go for path-bound scope resolution. Per-profile
  and per-issuer grants (Decision 2) now reach the wire layer.
- Wraps every state-changing route AND every read endpoint in router.go
  with rbacGate (global) or rbacGateScoped (path-bound). The auth-management
  routes (POST /api/v1/auth/roles, etc.) gain router-level enforcement
  in addition to the existing service-layer Authorizer check — defense in
  depth (HIGH-9 of the same audit collapses into this closure).
- Auth-exempt surfaces stay un-gated by design: login, callback, BCL,
  logout, breakglass-login, bootstrap, health, auth-info, version. Allowlist
  is documented in TestRouterRBACGateCoverage.
- Extends internal/domain/auth/validate.go CanonicalPermissions with 30 new
  perms across 12 namespaces: cert.edit; job.read, job.cancel; approval.read,
  approval.approve, approval.reject; policy.read/edit/delete;
  team.read/edit/delete; owner.read/edit/delete; notification.read/edit;
  discovery.read/run/claim; network_scan.read/edit/run;
  healthcheck.read/edit/delete/acknowledge; digest.read, digest.send;
  verification.read, verification.run; stats.read; metrics.read.
- Updates DefaultRoles for r-admin / r-operator / r-viewer / r-mcp / r-cli /
  r-agent. r-auditor gets NOTHING new — the auditor pin
  (TestAuditorRoleHoldsExactlyAuditReadAndExport) stays invariant.
- Migration 000039_audit_crit1_perms seeds the new perm rows + role grants
  per the updated DefaultRoles map. Idempotent ON CONFLICT DO NOTHING.
  Reverse migration removes role_permissions before permissions
  (ON DELETE RESTRICT on the FK).
- AST-level CI guard TestRouterRBACGateCoverage in
  internal/api/router/router_rbac_coverage_test.go walks router.go and
  asserts every state-changing + read route is wrapped (or in the
  documented allowlist). Adding a new ungated route fails CI.
- Updates docs/operator/rbac.md permission-catalogue table with the new
  namespaces + footer link to the AST CI guard.
- Updates certctl/CHANGELOG.md v2.1.0 section with the closure narrative.

Audit doc cowork/auth-bundles-audit-2026-05-10.md CRIT-1 row annotated
CLOSED 2026-05-10. Bundle's exit-gate spec lives at
cowork/auth-bundles-fixes-2026-05-10/01-crit-1-rbac-gates.md.

CRIT-2 / CRIT-3 / CRIT-4 / CRIT-5 of the same audit remain open and
continue to block the v2.1.0 tag.

Verification gate green:
- gofmt -d (no diff after gofmt -w on the touched files)
- go vet ./...
- go test -short -count=1 ./...   (all packages pass including auditor pin)
- go build ./...

HIGH-9 of the audit closes via this commit's router-layer rbacGate on
POST /api/v1/auth/keys/{id}/roles + DELETE /api/v1/auth/keys/{id}/roles/{role_id}
(defense-in-depth on top of the existing service-layer privilege check).

Refs: cowork/auth-bundles-audit-2026-05-10.md CRIT-1 HIGH-9
This commit is contained in:
shankar0123
2026-05-10 19:56:15 +00:00
parent c03d18bb1c
commit 68ca42fef1
7 changed files with 801 additions and 149 deletions
Download Patch File Download Diff File Expand all files Collapse all files
internal/api/router/router_rbac_coverage_test.go
+161
View File
@@ -0,0 +1,161 @@
package router
import (
"go/ast"
"go/parser"
"go/token"
"sort"
"strings"
"testing"
)
// TestRouterRBACGateCoverage AST-walks router.go and asserts that every
// state-changing handler registration goes through rbacGate or
// rbacGateScoped, excepting (a) protocol endpoints (ACME / SCEP / EST /
// CRL / OCSP) that authenticate via their own protocol primitives,
// (b) the bootstrap endpoint which is auth-exempt by design,
// (c) auth-info / login / logout / break-glass-login / health surfaces
// that establish identity rather than carry it.
//
// This is the ratchet that prevents 2026-05-10 audit CRIT-1 from
// regressing. A developer who registers a new state-changing handler
// (or a list endpoint) without rbacGate / rbacGateScoped fails this
// test. Update authExemptRoutes ONLY when registering a new
// auth-exempt surface, and document the addition in the commit body.
//
// See cowork/auth-bundles-audit-2026-05-10.md CRIT-1 for the closure
// history.
func TestRouterRBACGateCoverage(t *testing.T) {
// Routes whose handlers MUST stay ungated. Every entry here is a
// surface that establishes identity or is RFC-mandated unauth.
// Adding a new entry requires a justification comment.
authExemptRoutes := map[string]string{
// Identity-bearing surfaces (the gate would be circular):
"GET /api/v1/auth/me": "every caller may read their own identity",
"GET /api/v1/auth/permissions": "every caller may read the global permission catalogue",
"GET /api/v1/auth/check": "identity-probe; gating would be circular",
// Auth handshake surfaces (no identity at request time):
"GET /auth/oidc/login": "OIDC handshake start; no Bearer at this point",
"GET /auth/oidc/callback": "IdP redirects here pre-auth; cookie+state validated inside",
"POST /auth/oidc/back-channel-logout": "IdP-initiated; auth via IdP-signed logout_token in body",
"POST /auth/logout": "caller session-cookie is checked inside the handler",
"POST /auth/breakglass/login": "local-password recovery; surface invisible when disabled",
"GET /api/v1/auth/bootstrap": "day-0 admin probe; pre-admin by definition",
"POST /api/v1/auth/bootstrap": "consumes one-shot bootstrap token from body",
// Health / version / info:
"GET /health": "K8s/Docker liveness probe; cannot carry Bearer",
"GET /ready": "K8s/Docker readiness probe; cannot carry Bearer",
"GET /api/v1/auth/info": "GUI reads before login to detect auth mode",
"GET /api/v1/version": "rollout probes; pre-auth allowed",
}
// Protocol-endpoint prefixes — every r.Register against one of these
// is intentionally ungated (protocol-level auth via JWS / mTLS / CSR-
// embedded credentials). Mirrors AuthExemptDispatchPrefixes plus the
// in-router ACME paths.
protocolPrefixes := []string{
"/acme/",
"/scep",
"/.well-known/pki",
"/.well-known/est",
}
fset := token.NewFileSet()
f, err := parser.ParseFile(fset, "router.go", nil, parser.ParseComments)
if err != nil {
t.Fatalf("parse router.go: %v", err)
}
var unguarded []string
ast.Inspect(f, func(n ast.Node) bool {
call, ok := n.(*ast.CallExpr)
if !ok {
return true
}
sel, ok := call.Fun.(*ast.SelectorExpr)
if !ok || sel.Sel.Name != "Register" {
return true
}
// Reject calls that aren't r.Register (e.g. mux.Handle is filtered out
// by the SelectorExpr.X check below). The router type is `*Router`;
// we accept any selector since RegisterFunc also wraps Register.
_ = sel
if len(call.Args) < 2 {
return true
}
routeLit, ok := call.Args[0].(*ast.BasicLit)
if !ok || routeLit.Kind != token.STRING {
return true
}
route := strings.Trim(routeLit.Value, `"`)
// Only inspect routes that should be gated: state-changing
// (POST/PUT/PATCH/DELETE) or any read endpoint (GET).
if !isHTTPMethodRoute(route) {
return true
}
// Auth-exempt allowlist?
if _, ok := authExemptRoutes[route]; ok {
return true
}
// Protocol prefix?
if hasProtocolPrefix(route, protocolPrefixes) {
return true
}
// Inspect arg 1: must be rbacGate(...) or rbacGateScoped(...).
wrap, ok := call.Args[1].(*ast.CallExpr)
if !ok {
unguarded = append(unguarded, route)
return true
}
wrapName := ""
switch fn := wrap.Fun.(type) {
case *ast.Ident:
wrapName = fn.Name
case *ast.SelectorExpr:
wrapName = fn.Sel.Name
}
if wrapName != "rbacGate" && wrapName != "rbacGateScoped" {
unguarded = append(unguarded, route)
}
return true
})
if len(unguarded) > 0 {
sort.Strings(unguarded)
t.Fatalf("router.go: %d routes registered without rbacGate / rbacGateScoped (and not in authExemptRoutes / protocolPrefixes):\n %s\n\n"+
"If a new auth-exempt surface is intentional, add it to authExemptRoutes (or protocolPrefixes) "+
"with a justification comment. Otherwise wrap with rbacGate(reg.Checker, \"<perm>\", <handler>).\n\n"+
"This test pins the 2026-05-10 audit CRIT-1 closure. Removing an existing rbacGate wrap requires "+
"either (a) moving the route to authExemptRoutes here, or (b) demonstrating the new approach in "+
"the commit body.",
len(unguarded), strings.Join(unguarded, "\n "))
}
}
func isHTTPMethodRoute(route string) bool {
for _, prefix := range []string{"GET ", "POST ", "PUT ", "PATCH ", "DELETE ", "HEAD "} {
if strings.HasPrefix(route, prefix) {
return true
}
}
return false
}
func hasProtocolPrefix(route string, prefixes []string) bool {
// Strip the method token to compare against URL prefixes.
idx := strings.Index(route, " ")
if idx == -1 {
return false
}
urlPart := route[idx+1:]
for _, p := range prefixes {
if strings.HasPrefix(urlPart, p) {
return true
}
}
return false
}