feat(rbac): add zero-allocation RBAC core engine - #22
Conversation
Implements Phase 1 of the RBAC design (issue #16): command ID catalog with bulk categories, dynamic zero-allocation Bitset, role whitelist rules, rule parser, SessionContext hot-path check, and an atomically hot-swappable PolicyStore. IsAllowed benchmarks at 0 allocs/op, ~2.8 ns/op. Signed-off-by: Maximilian Hagen <git@saxy.dev>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/rbac/cmd.go (1)
20-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a consistency check across the three command registries.
AllCommands(Lines 42-46) andcommandNames(Lines 50-56) are maintained by hand, separately from theCmdXxxconst block (Lines 20-38). Nothing enforces that every constant appears in both lists. If a future command is added to the const block but forgotten inAllCommandsorcommandNames,LookupCommandand the"all"category silently diverge with no test failure, sinceTestCategoryAllCoversEveryRegisteredCommandonly checksCategory("all")againstAllCommandsitself, not against the const block.Add a small test (or a
go:generate-based single source of truth) that verifieslen(commandNames) == len(AllCommands)and that every ID inAllCommandshas a matching entry incommandNames.♻️ Suggested consistency test
func TestCommandRegistriesStayInSync(t *testing.T) { if len(commandNames) != len(AllCommands) { t.Fatalf("commandNames has %d entries, AllCommands has %d", len(commandNames), len(AllCommands)) } for _, id := range AllCommands { found := false for _, cmd := range commandNames { if cmd == id { found = true break } } if !found { t.Errorf("command id %d missing from commandNames", id) } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/rbac/cmd.go` around lines 20 - 56, Add a test for the command registries, such as TestCommandRegistriesStayInSync, that asserts commandNames and AllCommands have equal lengths and verifies every ID in AllCommands appears among commandNames values. Keep the existing CmdXxx constants and registry definitions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/rbac/parser.go`:
- Around line 64-71: Update the "~" rule handling in ParseRole to trim the
prefix, reject an empty result with the parser’s existing error behavior, and
append only non-empty namespace prefixes; preserve the special "~*" all-keys
behavior. Add a regression test in parser_test.go verifying ParseRole("x",
"~cache:", "~") returns an error.
In `@internal/rbac/session.go`:
- Around line 14-20: Fix the indentation in the SessionContext doc comment by
removing the leading tab from the continuation line so the full sentence renders
as one normal paragraph in Go documentation.
---
Nitpick comments:
In `@internal/rbac/cmd.go`:
- Around line 20-56: Add a test for the command registries, such as
TestCommandRegistriesStayInSync, that asserts commandNames and AllCommands have
equal lengths and verifies every ID in AllCommands appears among commandNames
values. Keep the existing CmdXxx constants and registry definitions unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 117cf357-dca4-481c-8247-f8e6d206ee7a
📒 Files selected for processing (12)
internal/rbac/bench_test.gointernal/rbac/cmd.gointernal/rbac/parser.gointernal/rbac/parser_test.gointernal/rbac/policy.gointernal/rbac/policy_test.gointernal/rbac/rbac.gointernal/rbac/rbac_test.gointernal/rbac/role.gointernal/rbac/role_test.gointernal/rbac/session.gointernal/rbac/session_test.go
Signed-off-by: Maximilian Hagen <git@saxy.dev>
* feat(rbac): add zero-allocation RBAC core engine (#22) * feat(rbac): add zero-allocation RBAC core engine Implements Phase 1 of the RBAC design (issue #16): command ID catalog with bulk categories, dynamic zero-allocation Bitset, role whitelist rules, rule parser, SessionContext hot-path check, and an atomically hot-swappable PolicyStore. IsAllowed benchmarks at 0 allocs/op, ~2.8 ns/op. Signed-off-by: Maximilian Hagen <git@saxy.dev> fix(pr): resolve comments chore(rbac): added test to ensure cmdList is always up2date Signed-off-by: Maximilian Hagen <git@saxy.dev> --------- Signed-off-by: Maximilian Hagen <git@saxy.dev> * feat(rbac): wire RBAC into RESP and binary servers (#23) * feat(rbac): wire RBAC into RESP and binary servers Add the ROLE command family and per-user bcrypt auth over both protocols, replacing --require-pass when an RBAC policy is loaded: - internal/rbac: Role/User policy store, rule parser (+cmd, +@cat, ~prefix), atomic hot-swap holder, and zero-alloc session gating - RESP: AUTH <user> <pass>, ROLE CREATE/SETUSER/DELUSER/DELETE/LIST/GETUSER, -NOAUTH / -NOPERM replies; nopass default-user semantics - binary: OpRoleCreate/SetUser/DelUser/Delete/List/GetUser wire codecs, MsgAuth username+password frame, ERR NOT_AUTHORIZED gating on both plaintext and TLS loops - server: --rbac-config / TELLSTONE_RBAC_CONFIG, SIGHUP hot-reload - client: AuthUser + Role* methods in the network and public client packages - example: cmd/example/role drives the full flow over the binary protocol Signed-off-by: Maximilian Hagen <git@saxy.dev> * feat(rbac): add metrics auth and per-role command counters Signed-off-by: Maximilian Hagen <git@saxy.dev> * fix(pr): resolve comments * fix(pr): resolve comments * fix(startup): reordered inits for graceful shutdown on error Signed-off-by: Maximilian Hagen <git@saxy.dev> * fix(rbac): extend documentation and client logger Signed-off-by: Maximilian Hagen <git@saxy.dev> --------- Signed-off-by: Maximilian Hagen <git@saxy.dev>
Implements Phase 1 of the RBAC design (issue #16): command ID catalog with bulk categories, dynamic zero-allocation Bitset, role whitelist rules, rule parser, SessionContext hot-path check, and an atomically hot-swappable PolicyStore.
IsAllowed benchmarks at 0 allocs/op, ~2.8 ns/op.
Description
Adds the Phase 1 core RBAC engine as a new self-contained
internal/rbac/package — the authorization primitive that later phases (ROLE commands + config
file, AUTH integration, API keys/OIDC mapping) build on. No production paths are
wired yet; this lands the model and its hot path in isolation so the
allocation/performance guarantees are proven before integration.
Component: Security / Access Control (
internal/rbac/)Type of Change:
Related Issue
#16
Technical Deep Dive & Context
What was built, per the issue's Phase 1 checklist:
cmd.go) — 17 commands mapped toiota + 1IDs(
CmdGet=1 …CmdRevoke=16; 0 is reserved as invalid, so the zero-valuebitset fails closed instead of accidentally granting
CmdGet). Bulkcategories (
read,write,readwrite,operator,maintenance,admin,all,none,login) are declared as flat command lists and expanded atbuild time.
loginis the category ofCmdAuth, kept separate so adata-plane role like
readwritecan grantAUTHwithout granting admin.admin(identity management) is deliberately split frommaintenance(FLUSH/SHUTDOWN/CONFIG) for least privilege.
Bitset(rbac.go) —[]uint64, pre-sized viaNewBitset,Set/Clear/Has.Hasis one word load + bit test with an out-of-rangeguard that returns false (fail-closed). The zero value denies everything, so
a partially-built role can never be accidentally permissive.
Role(role.go) — name + permissions + namespace whitelist. Flat, noinheritance in v1 (per issue). Immutable after construction.
SessionContext(session.go) — pinned at handshake, holds the role'sresolved permissions and namespace rules (not a pointer to the live role),
matching the issue's "copy, not reference" design.
IsAllowedis the hotpath: command bit test, then prefix scan with default-deny.
PolicyStore+ atomic hot-swap (policy.go) — a full immutable snapshot(
Roles+Users+Default) behindatomic.Pointer[PolicyStore]. Updatesbuild a complete replacement and swap in one store: readers never block, never
observe partial state, and new connections pick up the new policy at
handshake while existing sessions stay pinned.
parser.go) —+GET,+@read,-SET,-@admin,~prefix,~*. Deny always overrides allow regardless of rule order (Redis precedence).Category expansion happens at parse time, so the hot path never expands.
Design trade-offs worth calling out:
~rule makes the role a whitelist; keys notmatching a prefix are denied even for granted commands. This is the explicit
guardrail against "cache-manager can read
users:*" in the issue.~*andempty rules mean all keys — the two cases collapse to "no restriction", so no
separate wildcard flag is needed.
*in~users:*is stripped at parse time because matching isbytes.HasPrefix.This keeps the hot path allocation-free (no regex/glob compile or match) at the
cost of only supporting prefix scoping — which is all the issue requires.
SessionContext.Namespacesaliases the role'sbyte slices; safe because roles are immutable once published. Copying would
allocate per handshake and buy nothing.
Performance & Benchmarks
Microbenchmarks of the
IsAllowedhot path on AMD Ryzen 9 9950X,-benchmem:Workload: single role, one
~prefix rule, 3 runs eachIsAllowedallowedIsAlloweddenied-by-prefixMeets the issue's targets: 0 allocs/op and well under 10 ns/op. There is
no before/after comparison — the package is new and not yet on any execution
path, which is precisely why the numbers are published now rather than after
integration.
How Has This Been Tested?
go test ./internal/rbac/— 20 tests covering: bitset set/has/clear andfail-closed zero value, category expansion, deny-overrides-allow in both rule
orders,
~*wildcard, namespace default-deny (allowed + denied prefixes),parser errors (unknown command/category, malformed rules), role build
validation, session fail-closed with nil role, user→role resolution, default
role fallback, atomic hot-swap visibility, and session pinning across swaps
go test -race ./internal/rbac/— passes (atomic pointer swap exercised)go vet ./internal/rbac/— cleangofmt -l internal/rbac/— cleanChecklist
go test ./...andgo test -race ./...)go vetwarningsSummary by CodeRabbit
New Features
Tests