fix(rbac): fail closed when the anti-escalation guard cannot resolve a role - #765
Merged
Conversation
…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.
remyluslosius
force-pushed
the
fix/custom-role-escalation
branch
from
July 28, 2026 12:56
9108b44 to
e7c547d
Compare
remyluslosius
added a commit
that referenced
this pull request
Jul 30, 2026
…#767) * ci: local test database so DB-gated suites run before push, not in CI The DB-gated suites do not run without OPENWATCH_TEST_DSN, so `make ci-local` silently skipped them and a change could look clean locally and fail in CI. That happened three times in one day on the v0.7 branches: the report-schedule suites (#766), the RBAC role-assign and token tests (#765), and the go mod tidy drift (#762). Every one was a real defect that a local run would have caught. scripts/test-db.sh mirrors the CI service container exactly. Image and credentials match .github/workflows/go-ci.yml, because if they drift then "passes locally" stops meaning "passes in CI", which is the whole point. Two settings are load-bearing and are commented as such in the script: max_connections=400. The default 100 is not enough. The suite runs packages in parallel and internal/db/dbtest clones a template database per package, so a default container deadlocks: the first DB test times out at 120s and the rest cascade. Found this the honest way, by hitting it. It looks exactly like a real failure and is not one, which is worse than no local database at all. fsync, synchronous_commit and full_page_writes off. The database is disposable; durability buys nothing and costs most of the runtime. Port defaults to 5455 rather than 5432: on this workstation 5432 is the dev database and 5433 is hanalyx-postgres. Override with TEST_DB_PORT. The database name ends in _test so the internal/db guard accepts it without the ALLOW_NONTEST bypass, which we never want set. `make ci-local` now warns when OPENWATCH_TEST_DSN is unset and says exactly how to fix it. A silent skip is what let this through repeatedly; a loud skip is the minimum. Verified: full `go test ./internal/...` against the container is clean, and the three suites CI previously caught now run and pass locally. * ci: un-ignore scripts/test-db.sh so the Makefile target has a script The blanket `*test*.sh` in .gitignore swallowed scripts/test-db.sh. `git add` refused it, the ci-local change shipped referencing a script that was never committed, and nothing failed loudly: the PR just contained a Makefile target pointing at a file no one else would have. Same trap as the blanket `*.spec` rule that drops new packaging spec files. Negate deliberately and verify with `git check-ignore -v <path>` after adding anything to scripts/ whose name contains "test".
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The defect
RoleGrantsWithinreturned true (allow) for every role missing fromBuiltInRoles. Its comment justified this: an unknown role confers no permissions, so it is trivially within the caller's grant.That holds for a typo. It does not hold for a custom role, which exists, is not built in, and passes the downstream existence check. So the guard waved through every custom role on both paths that use it: role assignment and API-token minting. 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.HasPermissionreadsBuiltInRolesonly). That is what makes it a landmine rather than a live breach: wiring enforcement would have made every waved-through grant real, retroactively.Correcting the backlog while fixing it
The backlog says the path opens "for anyone with
role:write+role:assign". Only admin holds both, and admin already holdscredential:writeandremediation:execute, so that actor gains nothing. The reachable paths are narrower and different:token:write, whichsecurity_adminholds. Once enforcement lands, a security_admin could mint a token bound to an existing custom role granting more than they hold.Both are real. Neither is the one described.
Both halves, because either alone leaves a two-step escalation
RoleGrantsWithinResolvedtakes aRoleResolverso a legitimate 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 DB fault is deliberate: failing open there is the same defect by another route.CreateCustomRolenow rejects authoring a role granting a permission the creator lacks (403, naming them). Guarding only the assign step still lets a caller author an over-broad role and wait for someone else to assign it.Negative-tested
Reintroducing the original
return truefails AC-19:SDD
system-rbac1.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, after the AC-13 duplicate in #763.Not in scope
Wiring custom-role enforcement into
Identity.HasPermission. That is a feature, and it is now safe to build.