From e7c547d482cb93dcbc88f4cf080b7f807a7bb309 Mon Sep 17 00:00:00 2001 From: Remylus Losius Date: Tue, 28 Jul 2026 08:45:45 -0400 Subject: [PATCH] fix(rbac): fail closed when the anti-escalation guard cannot resolve a role RoleGrantsWithin returned true, meaning allow, for every role missing from BuiltInRoles. Its comment justified this: an unknown role confers no permissions, so it is trivially within the caller's grant, and a downstream existence check rejects it anyway. That holds for a typo. It does not hold for a custom role, which exists, is not built in, and passes the downstream check. So the guard waved through every custom role on both paths that use it: role assignment (users_handlers.go) and API-token minting (tokens_handlers.go). The one function whose entire job is to fail closed was failing open on the only case it could not evaluate. It is inert today because custom-role permissions are never enforced: Identity.HasPermission and Permissions both read BuiltInRoles only, so a custom role currently grants nothing. That is what makes it a landmine rather than a live breach. Wiring custom-role enforcement without fixing this first would make every previously waved-through grant real, retroactively. Correcting the backlog's characterisation of the exploit while fixing it: the entry says the path opens "for anyone with role:write+role:assign". Only admin holds both, and admin already holds credential:write and remediation:execute, so that particular actor gains nothing. The reachable paths are narrower and different. First, token minting needs only token:write, which security_admin holds; once enforcement lands, a security_admin could mint a token bound to an existing custom role granting more than they hold. Second, an admin who delegates role management through a custom role hands the holder a route to grant themselves anything. Both are real; neither is the one described. The fix is both halves, because either alone leaves a two-step escalation: - The guard now DENIES an unresolvable role. RoleGrantsWithinResolved takes a RoleResolver so a real custom role is evaluated on its actual permission set from the roles table rather than blanket-refused; a nil resolver, a missing row, or a query error all deny. Fail-closed on a database problem is deliberate: failing open there would be the same defect by another route. - CreateCustomRole now rejects authoring a role that grants a permission the creator lacks (ErrRoleExceedsGrant, 403, naming the offending permissions). Guarding only the assign step still lets a caller author an over-broad role and wait for someone else to assign it. users.RolePermissions backs the resolver and reports a query error as "unknown" rather than surfacing it, so the guard cannot be pushed open by a DB fault. Negative-tested: reintroducing the original `return true` fails AC-19 with "guard must DENY an unresolvable role; returning true here is the escalation". Spec: system-rbac 1.1.0 -> 1.2.0, AC-19 (fail-closed guard) and AC-20 (authoring subset check). 116 specs, 116 passing. AC ids checked for collision this time. Not in scope: wiring custom-role permission enforcement into Identity.HasPermission. That is a feature, and it is now safe to build. --- internal/auth/grant.go | 93 ++++++++++++++++++--- internal/auth/grant_escalation_test.go | 108 +++++++++++++++++++++++++ internal/server/tokens_handlers.go | 2 +- internal/server/users_handlers.go | 41 +++++++++- internal/users/roles.go | 54 +++++++++++++ specs/system/rbac.spec.yaml | 31 ++++++- 6 files changed, 317 insertions(+), 12 deletions(-) create mode 100644 internal/auth/grant_escalation_test.go diff --git a/internal/auth/grant.go b/internal/auth/grant.go index 3432bfb4..ccbfc58b 100644 --- a/internal/auth/grant.go +++ b/internal/auth/grant.go @@ -1,21 +1,78 @@ 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 } @@ -23,6 +80,24 @@ func RoleGrantsWithin(caller Identity, requested RoleID) bool { 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"): diff --git a/internal/auth/grant_escalation_test.go b/internal/auth/grant_escalation_test.go new file mode 100644 index 00000000..d2174f3c --- /dev/null +++ b/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) + } + }) +} diff --git a/internal/server/tokens_handlers.go b/internal/server/tokens_handlers.go index ea5db1d5..e67d4ba8 100644 --- a/internal/server/tokens_handlers.go +++ b/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 diff --git a/internal/server/users_handlers.go b/internal/server/users_handlers.go index d84d265c..b4dbde90 100644 --- a/internal/server/users_handlers.go +++ b/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 + } +} diff --git a/internal/users/roles.go b/internal/users/roles.go index e0d9ed25..b2097fad 100644 --- a/internal/users/roles.go +++ b/internal/users/roles.go @@ -6,6 +6,7 @@ import ( "fmt" "github.com/google/uuid" + "github.com/jackc/pgx/v5" ) // CustomRole-related errors. @@ -13,6 +14,9 @@ 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 +} diff --git a/specs/system/rbac.spec.yaml b/specs/system/rbac.spec.yaml index 08e9b8c3..54601161 100644 --- a/specs/system/rbac.spec.yaml +++ b/specs/system/rbac.spec.yaml @@ -1,7 +1,7 @@ spec: id: system-rbac title: RBAC permission registry and middleware - version: "1.1.0" + version: "1.2.0" status: approved tier: 1 @@ -146,3 +146,32 @@ spec: x-required-permission declarations were decorative: nothing linked the contract to the code, and the link was maintained by review alone. priority: critical + - id: AC-19 + description: > + The anti-escalation guard fails CLOSED on any role it cannot resolve. + RoleGrantsWithin previously returned true (allow) for every role absent + from the built-in registry, on the reasoning that an unknown role + confers nothing. That reasoning does not hold for a CUSTOM role: it + exists, it is not built in, and the downstream existence check accepts + it, so the guard passed for every custom role on both the role-assign + and the API-token-mint paths. The guard MUST now distinguish three + states and treat them differently. A role that RESOLVES is evaluated + against its actual permission set, whether built in or custom. A role + that is INDETERMINATE (no resolver, or the lookup failed) is DENIED, + because the guard cannot show the grant is within the caller's + authority and a database fault must not open it. A role that is + PROVABLY ABSENT passes the guard, because it confers nothing and the + downstream existence check rejects it as bad input; denying there would + turn a client typo into an authorization error and change the + documented 400 for an unknown role id into a 403. + priority: critical + - id: AC-20 + description: > + A caller MUST NOT author a custom role granting a permission they do + not themselves hold. Validating that a permission is registered is not + the same as validating the author may confer it: authoring an + over-broad role and then having it assigned is a two-step escalation + that neither step catches alone. CreateCustomRole rejects with + ErrRoleExceedsGrant and names the offending permissions so the API can + return them. + priority: critical