fix(rbac): fail closed when the anti-escalation guard cannot resolve a role

internal/auth/grant.go

@@ -1,28 +1,103 @@
 package auth
 
+// RoleResolver answers what a non-built-in role confers. It exists so the
+// anti-escalation guard can evaluate a CUSTOM role (whose permission set lives
+// in the roles table) without internal/auth importing a database layer.
+//
+// The three states are distinct and the distinction is load-bearing:
+//
+//	found=true,  err=nil  the role exists; perms is its permission set
+//	found=false, err=nil  the role PROVABLY does not exist
+//	err != nil            cannot determine (query failed, service missing)
+//
+// "Provably absent" and "cannot determine" must never be collapsed. An absent
+// role confers nothing and is rejected downstream as bad input, so the guard
+// lets it through to produce the correct 400. An indeterminate answer is a
+// denial, because the guard cannot show the grant is within the caller's
+// authority.
+type RoleResolver func(RoleID) (perms []Permission, found bool, err error)
+
 // RoleGrantsWithin reports whether every permission conferred by the
 // requested role is also held by the caller. It is the anti-privilege-
-// escalation primitive: a caller may never grant — via an API token, a
-// role assignment, or a custom role — a permission they do not themselves
+// escalation primitive: a caller may never grant, via an API token, a
+// role assignment, or a custom role, a permission they do not themselves
 // hold.
 //
-// An unknown requested role confers no permissions (BuiltInRoles miss), so
-// it is trivially "within"; the caller's downstream existence check rejects
-// it on its own. Returns false as soon as one required permission is not
-// held by the caller.
+// It resolves BUILT-IN roles only, and DENIES anything else.
+//
+// This function previously returned true (allow) for any role missing from
+// BuiltInRoles, on the reasoning that an unknown role confers nothing and a
+// downstream existence check would reject it. That reasoning does not hold for
+// custom roles: they exist, they are not in BuiltInRoles, and the downstream
+// check accepts them. The guard therefore passed for every custom role, which
+// is fail-open on the one code path whose entire job is to fail closed.
+//
+// Prefer [RoleGrantsWithinResolved] wherever custom roles are legitimate, so a
+// real custom role is checked on its actual permissions rather than refused.
 func RoleGrantsWithin(caller Identity, requested RoleID) bool {
-	def, ok := BuiltInRoles[requested]
-	if !ok {
+	return RoleGrantsWithinResolved(caller, requested, nil)
+}
+
+// RoleGrantsWithinResolved is RoleGrantsWithin with custom-role support.
+// resolve supplies the permission set for a role that is not built in; a nil
+// resolver, or a resolver reporting the role unknown, DENIES.
+//
+// Fail-closed is deliberate and load-bearing here. The alternative, treating
+// an unresolvable role as harmless because nothing currently enforces its
+// permissions, is what made this a latent escalation: the day custom-role
+// enforcement is wired, every previously-waved-through grant becomes real.
+func RoleGrantsWithinResolved(caller Identity, requested RoleID, resolve RoleResolver) bool {
+	if def, ok := BuiltInRoles[requested]; ok {
+		return holdsAll(caller, def.Permissions)
+	}
+	if resolve == nil {
+		// No way to evaluate a non-built-in role. Deny.
+		return false
+	}
+	perms, found, err := resolve(requested)
+	switch {
+	case err != nil:
+		// Cannot determine. Deny rather than guess; failing open on a
+		// database fault would be the same defect by another route.
+		return false
+	case !found:
+		// Provably absent. It confers nothing and the downstream existence
+		// check rejects it as bad input, which is the correct response for a
+		// role id that does not exist. Denying here instead would turn a
+		// client typo into an authorization error.
 		return true
+	default:
+		return holdsAll(caller, perms)
 	}
-	for _, p := range def.Permissions {
+}
+
+func holdsAll(caller Identity, perms []Permission) bool {
+	for _, p := range perms {
 		if !caller.HasPermission(p) {
 			return false
 		}
 	}
 	return true
 }
 
+// PermissionsWithin reports whether every permission in requested is held by
+// the caller. It is the same subset rule as RoleGrantsWithin applied to a raw
+// permission list, for the custom-role CREATION path: a caller must not be
+// able to author a role granting more than they themselves hold, since
+// authoring it and then assigning it is a two-step escalation that neither
+// step alone would catch.
+//
+// Returns the permissions the caller lacks, so the API can name them.
+func PermissionsWithin(caller Identity, requested []Permission) []Permission {
+	var missing []Permission
+	for _, p := range requested {
+		if !caller.HasPermission(p) {
+			missing = append(missing, p)
+		}
+	}
+	return missing
+}
+
 // RolesWithPermission returns the built-in role IDs whose definition grants p,
 // in declaration order. It is the recipient-resolution primitive for
 // permission-scoped fan-out (e.g. "every role that can approve an exception"):

internal/auth/grant_escalation_test.go

@@ -0,0 +1,108 @@
+// @spec system-rbac
+//
+// Anti-escalation guard, custom-role behaviour.
+//
+//	AC-19  TestRoleGrantsWithin_FailsClosedOnUnresolvableRole
+package auth
+
+import (
+	"errors"
+	"testing"
+)
+
+// admin identity for the caller side of the guard.
+func adminCaller() Identity  { return Identity{RoleID: RoleAdmin} }
+func viewerCaller() Identity { return Identity{RoleID: RoleViewer} }
+
+// @ac AC-19
+// AC-19 (NEGATIVE PATH): the guard denies any role it cannot resolve, and
+// evaluates a resolvable custom role on its real permissions.
+//
+// The bug this pins: RoleGrantsWithin returned true for every role missing from
+// BuiltInRoles. The comment justified it as "an unknown role confers no
+// permissions, so it is trivially within". That is true for a typo and false
+// for a custom role, which exists and whose downstream existence check passes.
+// The guard therefore waved through every custom role on both the role-assign
+// and the token-mint paths. It was inert only because custom-role permissions
+// are not yet enforced, which made it a latent escalation rather than a live
+// one: wiring enforcement would have made every waved-through grant real.
+func TestRoleGrantsWithin_FailsClosedOnUnresolvableRole(t *testing.T) {
+	t.Run("system-rbac/AC-19", func(t *testing.T) {
+		const custom = RoleID("delegated-role-manager")
+
+		// No resolver: the guard cannot prove the grant is within the caller's
+		// authority, so it must deny even for an admin caller.
+		if RoleGrantsWithin(adminCaller(), custom) {
+			t.Error("guard must DENY an unresolvable role; returning true here is the escalation")
+		}
+		if RoleGrantsWithinResolved(adminCaller(), custom, nil) {
+			t.Error("a nil resolver must deny, not allow")
+		}
+
+		// INDETERMINATE (query failed): must deny. Failing open on a database
+		// problem would be the same defect via a different route.
+		broken := func(RoleID) ([]Permission, bool, error) {
+			return nil, false, errors.New("connection refused")
+		}
+		if RoleGrantsWithinResolved(adminCaller(), custom, broken) {
+			t.Error("an indeterminate resolver result must deny, not allow")
+		}
+
+		// PROVABLY ABSENT: pass through. The role confers nothing and the
+		// downstream existence check returns 400 for an unknown role id.
+		// Denying here would turn a client typo into an authorization error,
+		// which is what the first cut of this fix did (caught by CI).
+		absent := func(RoleID) ([]Permission, bool, error) { return nil, false, nil }
+		if !RoleGrantsWithinResolved(adminCaller(), custom, absent) {
+			t.Error("a provably absent role must pass the guard so the downstream check can 400 it")
+		}
+
+		// Resolvable custom role, permissions WITHIN the caller's own set:
+		// allowed. Fail-closed must not mean "custom roles never work".
+		within := func(RoleID) ([]Permission, bool, error) {
+			return []Permission{HostRead}, true, nil
+		}
+		if !RoleGrantsWithinResolved(adminCaller(), custom, within) {
+			t.Error("admin must be able to grant a custom role whose permissions admin holds")
+		}
+
+		// Resolvable custom role granting MORE than the caller holds: denied.
+		// This is the case the old guard allowed.
+		beyond := func(RoleID) ([]Permission, bool, error) {
+			return []Permission{CredentialWrite, RemediationExecute}, true, nil
+		}
+		if RoleGrantsWithinResolved(viewerCaller(), custom, beyond) {
+			t.Error("a viewer must not be able to grant credential:write + remediation:execute via a custom role")
+		}
+
+		// Built-in roles keep working, resolver or not.
+		if !RoleGrantsWithinResolved(adminCaller(), RoleViewer, nil) {
+			t.Error("admin must still be able to grant a built-in role that is a subset of admin")
+		}
+		if RoleGrantsWithinResolved(viewerCaller(), RoleAdmin, nil) {
+			t.Error("a viewer must never be able to grant admin")
+		}
+	})
+}
+
+// @ac AC-20
+// AC-20: PermissionsWithin names exactly the permissions the caller lacks. It
+// backs the custom-role AUTHORING check, which is the other half of the fix:
+// blocking the assign step alone still lets a caller author an over-broad role
+// and wait for someone else to assign it.
+func TestPermissionsWithin_NamesWhatTheCallerLacks(t *testing.T) {
+	t.Run("system-rbac/AC-20", func(t *testing.T) {
+		if missing := PermissionsWithin(adminCaller(), []Permission{HostRead}); len(missing) != 0 {
+			t.Errorf("admin holds host:read; got missing=%v", missing)
+		}
+		missing := PermissionsWithin(viewerCaller(), []Permission{HostRead, CredentialWrite})
+		if len(missing) != 1 || missing[0] != CredentialWrite {
+			t.Errorf("viewer lacks exactly credential:write; got %v", missing)
+		}
+		// Anonymous holds nothing.
+		anon := Identity{IsAnonymous: true}
+		if got := PermissionsWithin(anon, []Permission{HostRead}); len(got) != 1 {
+			t.Errorf("anonymous must lack every permission; got %v", got)
+		}
+	})
+}

internal/server/tokens_handlers.go

@@ -46,7 +46,7 @@ func (h *handlers) PostAPIToken(w http.ResponseWriter, r *http.Request) {
 	// Anti-escalation: a caller may not mint a token more privileged than
 	// themselves. The requested role's permissions must be a subset of the
 	// caller's own. Spec api-tokens C-03 / AC-05.
-	if !auth.RoleGrantsWithin(auth.FromContext(r.Context()), auth.RoleID(req.RoleId)) {
+	if !auth.RoleGrantsWithinResolved(auth.FromContext(r.Context()), auth.RoleID(req.RoleId), h.roleResolver(r)) {
 		writeError(w, http.StatusForbidden, "authz.role_exceeds_grant", "client",
 			"cannot create a token whose role grants permissions you do not hold", false)
 		return

internal/server/users_handlers.go

@@ -129,7 +129,7 @@ func (h *handlers) PostUserRolesAssign(w http.ResponseWriter, r *http.Request, i
 	}
 	// Anti-escalation: a caller may not grant a role more privileged than
 	// themselves. Spec api-users C-05 / AC-13 (mirrors api-tokens C-03).
-	if !auth.RoleGrantsWithin(auth.FromContext(r.Context()), auth.RoleID(req.RoleId)) {
+	if !auth.RoleGrantsWithinResolved(auth.FromContext(r.Context()), auth.RoleID(req.RoleId), h.roleResolver(r)) {
 		writeError(w, http.StatusForbidden, "authz.role_exceeds_grant", "client",
 			"cannot assign a role that grants permissions you do not hold", false)
 		return
@@ -193,8 +193,15 @@ func (h *handlers) PostRolesCreate(w http.ResponseWriter, r *http.Request) {
 		_, ok := auth.Permissions[auth.Permission(perm)]
 		return ok
 	}
+	// Anti-escalation on the authoring step: the creator may not confer a
+	// permission they do not hold. Without this, authoring a role granting
+	// anything in the registry and then having it assigned is a two-step
+	// escalation that neither step catches alone.
+	caller := auth.FromContext(r.Context())
+	holds := func(perm string) bool { return caller.HasPermission(auth.Permission(perm)) }
 	role, invalid, err := h.users.CreateCustomRole(r.Context(), users.CustomRoleParams{
 		ID: req.Id, Description: req.Description, Permissions: req.Permissions, CreatedBy: created,
+		CallerHolds: holds,
 	}, validator)
 	if err != nil {
 		switch {
@@ -205,6 +212,10 @@ func (h *handlers) PostRolesCreate(w http.ResponseWriter, r *http.Request) {
 		case errors.Is(err, users.ErrRoleIDTaken):
 			writeError(w, http.StatusConflict, "users.role_id_taken", "client",
 				"role id already exists", false)
+		case errors.Is(err, users.ErrRoleExceedsGrant):
+			detail := map[string]any{"exceeds_grant": invalid}
+			writeErrorWithDetail(w, http.StatusForbidden, "authz.role_exceeds_grant", "client",
+				"cannot create a role granting permissions you do not hold", false, detail)
 		case errors.Is(err, users.ErrCustomRoleEmpty):
 			writeError(w, http.StatusBadRequest, "validation.field_required", "client",
 				"permissions must be non-empty", false)
@@ -278,3 +289,31 @@ func writeErrorWithDetail(w http.ResponseWriter, status int, code, fault, msg st
 	w.WriteHeader(status)
 	_, _ = w.Write(bs)
 }
+
+// errNoUserService marks the resolver as unable to answer, which the guard
+// treats as a denial rather than as "the role does not exist".
+var errNoUserService = errors.New("server: users service unavailable for role resolution")
+
+// roleResolver returns an auth.RoleResolver backed by the roles table, so the
+// anti-escalation guard can evaluate a CUSTOM role on its real permission set.
+//
+// It preserves the three-state contract. A query failure or a missing service
+// is an ERROR (the guard denies); a row that simply is not there is
+// found=false with no error (the guard passes it through so the downstream
+// existence check returns the correct 400 for an unknown role id).
+func (h *handlers) roleResolver(r *http.Request) auth.RoleResolver {
+	return func(id auth.RoleID) ([]auth.Permission, bool, error) {
+		if h.users == nil {
+			return nil, false, errNoUserService
+		}
+		raw, found, err := h.users.RolePermissions(r.Context(), string(id))
+		if err != nil || !found {
+			return nil, false, err
+		}
+		out := make([]auth.Permission, 0, len(raw))
+		for _, p := range raw {
+			out = append(out, auth.Permission(p))
+		}
+		return out, true, nil
+	}
+}

internal/users/roles.go

@@ -6,13 +6,17 @@ import (
 	"fmt"
 
 	"github.com/google/uuid"
+	"github.com/jackc/pgx/v5"
 )
 
 // CustomRole-related errors.
 var (
 	ErrRoleIDTaken       = errors.New("users: role id collides with a built-in or existing custom role")
 	ErrUnknownPermission = errors.New("users: role grants permission not in the registry")
 	ErrCustomRoleEmpty   = errors.New("users: custom role must grant at least one permission")
+	// ErrRoleExceedsGrant is returned when the caller tries to author a custom
+	// role granting a permission they do not themselves hold.
+	ErrRoleExceedsGrant = errors.New("users: custom role grants a permission the creator does not hold")
 )
 
 // PermissionValidator returns true if a permission id is registered.
@@ -26,6 +30,12 @@ type CustomRoleParams struct {
 	Description string
 	Permissions []string
 	CreatedBy   uuid.UUID
+	// CallerHolds reports whether the CREATOR holds a permission. Supplied by
+	// the handler from the caller's identity so this package need not import
+	// internal/auth (which would cycle). A nil CallerHolds skips the
+	// subset check and is intended ONLY for trusted internal seeding; every
+	// request-driven path must set it.
+	CallerHolds func(perm string) bool
 }
 
 // Role is the on-wire shape for built-in and custom roles.
@@ -90,6 +100,22 @@ func (s *Service) CreateCustomRole(ctx context.Context, p CustomRoleParams, vali
 	if len(invalid) > 0 {
 		return Role{}, invalid, ErrUnknownPermission
 	}
+	// Anti-escalation on the AUTHORING step. Validating that a permission
+	// exists is not the same as validating the author may confer it: without
+	// this, a caller could author a role granting anything in the registry and
+	// then have it assigned, an escalation that neither step catches alone.
+	// CallerHolds is supplied by the handler from the caller's identity.
+	if p.CallerHolds != nil {
+		var exceeds []string
+		for _, perm := range p.Permissions {
+			if !p.CallerHolds(perm) {
+				exceeds = append(exceeds, perm)
+			}
+		}
+		if len(exceeds) > 0 {
+			return Role{}, exceeds, ErrRoleExceedsGrant
+		}
+	}
 	// Built-in collision check (cheap; before the SQL round-trip).
 	for _, builtin := range []string{"viewer", "auditor", "ops_lead", "security_admin", "admin"} {
 		if p.ID == builtin {
@@ -123,3 +149,31 @@ func isUniqueViolation(err error) bool {
 	}
 	return false
 }
+
+// RolePermissions returns the permissions a role confers, and whether the role
+// exists. Built-in roles are answered from the registry by the caller; this
+// reads the roles table, which is where a CUSTOM role's permission set lives.
+//
+// It backs the anti-escalation guard (auth.RoleGrantsWithinResolved). Without
+// it the guard can only see built-in roles, and its only safe response to a
+// custom role is to deny, which would break legitimate custom-role assignment.
+//
+// It returns three states. A missing row is (nil, false, nil): provably absent.
+// A query failure is (nil, false, err): indeterminate, and the guard denies.
+// Collapsing the two would either turn a client typo into a 403 or let a
+// database fault open the guard.
+func (s *Service) RolePermissions(ctx context.Context, roleID string) ([]string, bool, error) {
+	var perms []string
+	err := s.pool.QueryRow(ctx,
+		`SELECT permissions FROM roles WHERE id = $1`, roleID).Scan(&perms)
+	if errors.Is(err, pgx.ErrNoRows) {
+		// Provably absent. Not an error: the caller needs to tell this apart
+		// from "could not determine" so an unknown role id stays a 400 and
+		// does not become a 403.
+		return nil, false, nil
+	}
+	if err != nil {
+		return nil, false, fmt.Errorf("users: role permissions: %w", err)
+	}
+	return perms, true, nil
+}