feat(rbac): wire RBAC into RESP and binary servers - #23
Merged
Conversation
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>
|
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Nitpick comments (7)
internal/rbac/config.go (1)
99-102: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider validating the bcrypt hash format at load time.
u.Passwordis stored as raw bytes assuming it is already a bcrypt hash, per the schema comment at Line 29. There is no check that the value looks like a valid bcrypt hash (for example, the$2a$/$2b$/$2y$prefix). A malformed hash — for instance, an accidentally pasted plaintext password — will load successfully and fail silently only later, at authentication time, rather than at config load time when the operator can immediately correct it.Validate the hash format in
build()and reject the config early if it looks invalid.🤖 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/config.go` around lines 99 - 102, Update build() to validate each non-Nopass u.Password as a bcrypt hash before assigning it to hash, accepting supported bcrypt prefixes and rejecting malformed values with a configuration-load error. Preserve the existing Nopass behavior and raw hash assignment for valid passwords.internal/network/role_test.go (1)
128-203: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftShare the ROLE dispatch instead of duplicating it in the test.
The comment on Line 126 states that
rbacTestHandlermirrorsserver.networkHandler. The two implementations can diverge, and the tests then verify a handler that production does not use. That risk matters here, because this is an authorization path.Export the ROLE dispatch from the server package, or move it into a shared helper that both the server and this test call. Then the test exercises the real decoding, validation, and error mapping.
The
nilerrreports on Lines 145, 155, 158, and 174 are false positives. The handler converts domain errors into"ERR ..."wire payloads and returns a nil transport error on purpose.🤖 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/network/role_test.go` around lines 128 - 203, Replace the duplicated ROLE switch in rbacTestHandler with the server’s shared/exported ROLE dispatch so tests exercise production decoding, validation, and error mapping. Update server.networkHandler or extract its ROLE handling into a reusable helper, then have both paths call it; preserve the intentional nil transport errors after domain failures are encoded as ERR payloads.Source: Linters/SAST tools
client/client_role.go (1)
17-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a typed password option instead of raw
[][]bytetokens.
passOptionsrequires the caller to construct wire tokens such as[]byte(">password")and[]byte("nopass"). The caller must know the RESP prefix convention, and a typo produces a server-side error rather than a compile error.Accept an explicit option instead, for example a
Password stringplus aNoPass boolfield on a small options struct, and build the tokens inside the method. This is optional, and it is a breaking change to a new API, so it is cheapest to do before release.🤖 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 `@client/client_role.go` around lines 17 - 24, Update RoleSetUser to accept a typed options struct containing Password and NoPass instead of raw passOptions [][]byte, and construct the required wire tokens internally using the existing password-option conventions. Preserve the documented last-option-wins behavior and nopass password-clearing semantics while keeping validation and delegation through c.valid and c.c.RoleSetUser.config/config.go (1)
284-292: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the environment variable prefix with the other flags.
Every other option in
LoadConfigreads aTSD_-prefixed variable (TSD_ADDR,TSD_REQUIRE_PASS,TSD_TLS_CERT). This flag readsTELLSTONE_RBAC_CONFIG. Operators must then remember two prefixes. ConsiderTSD_RBAC_CONFIG, optionally withTELLSTONE_RBAC_CONFIGkept as a fallback for compatibility.♻️ Proposed change
fs.StringVar( &cfg.rbacConfig, "rbac-config", - getEnv("TELLSTONE_RBAC_CONFIG", ""), + getEnv("TSD_RBAC_CONFIG", ""), "Path to YAML/JSON RBAC policy file (roles, users, default_role); empty disables RBAC (default: none)", )The doc comment on line 122 needs the same update.
🤖 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 `@config/config.go` around lines 284 - 292, Update the rbacConfig environment lookup in LoadConfig to use TSD_RBAC_CONFIG, while retaining TELLSTONE_RBAC_CONFIG only as a compatibility fallback if appropriate. Also update the related doc comment to document the TSD_ prefix and fallback behavior consistently with the other flags.internal/resp/role_test.go (1)
15-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for key-prefix denial.
Every role in
rbacTestPolicyuses~*, sorole.AllowsKeyalways returns true and the namespace half ofIsAllowedis never exercised. Add a role restricted to one prefix, for example+get ~app:, and assert thatGET app:ksucceeds whileGET other:kreturnsNOPERM. That covers the key-scan path that the PR objectives describe.Also applies to: 87-101
🤖 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/resp/role_test.go` around lines 15 - 41, Extend rbacTestPolicy with a role/user restricted to the app: key prefix using GET permission, then add IsAllowed/command test coverage asserting GET app:k succeeds and GET other:k returns NOPERM. Ensure the assertions exercise the role’s key-prefix denial path rather than the existing ~* roles.internal/resp/server.go (1)
103-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSkip the
--require-passhash when a policy store is configured.Lines 83-84 state that
requirePassHashis ignored when a policy exists, andauthroutes toauthRBACbefore reading it.internal/network/server.goline 122 already guards the hash computation withpolicy == nil. Apply the same guard here so the two servers stay symmetric and the dead hash is not built.♻️ Proposed change
- if requirePass != "" { + if requirePass != "" && policy == nil {🤖 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/resp/server.go` around lines 103 - 112, Guard the bcrypt hash generation in the requirePass initialization block with the policy-store absence condition, matching the existing policy == nil behavior in the other server. Ensure requirePass is hashed only when no policy is configured, while preserving the current invalid-password panic behavior when hashing is performed.server/server.go (1)
146-155: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueThe SIGHUP goroutine outlives
Run.
defer signal.Stop(hup)stops signal delivery, but the channel is never closed, so thefor range hupgoroutine stays blocked afterRunreturns. The process normally exits at that point, so the impact is limited to tests that callRunrepeatedly. Close the channel aftersignal.Stop, or select on the shutdown context.♻️ Proposed change
hup := make(chan os.Signal, 1) signal.Notify(hup, syscall.SIGHUP) - defer signal.Stop(hup) + defer func() { + signal.Stop(hup) + close(hup) + }()🤖 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 `@server/server.go` around lines 146 - 155, Update the SIGHUP handling goroutine around signal.Notify and reloadRBAC so it exits when Run returns, using the existing shutdown context if available or otherwise coordinating channel closure with signal.Stop. Ensure cleanup does not race with signal delivery and preserve RBAC reload behavior while Run is active.
🤖 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 `@client/client_role.go`:
- Around line 43-56: Move the response types used by RoleList and RoleGetUser
from internal/network into the client package, then update Client.RoleList and
Client.RoleGetUser to return the client-owned types and convert the underlying
c.RoleList and c.RoleGetUser results in the wrappers. Ensure the public API no
longer exposes any network package types.
In `@cmd/example/role/main.go`:
- Around line 90-103: Update the authorization checks around alice.Get and
alice.Set to fail closed: seed users:1 with the administrator client before
testing it if a missing key can produce an error, require the allowed GET to
succeed, require denied operations to return an error containing NOT_AUTHORIZED,
and call log.Fatalf for connection, authentication, protocol, or
unexpected-success results instead of printing them as valid outcomes.
In `@go.mod`:
- Line 40: Move gopkg.in/yaml.v3 from the indirect require section into the
direct require block, removing the stale // indirect marker. Run go mod tidy
afterward and retain its resulting go.mod changes.
In `@internal/network/client_role.go`:
- Around line 112-131: Document in Client.AuthUser that credentials are sent in
plaintext at the protocol layer and require a TLS-established connection via
DialTLS. Update RoleSetUser to reject password-bearing passOptions unless the
client connection was created with DialTLS, while preserving non-password role
updates and existing authentication behavior.
In `@internal/network/client.go`:
- Around line 98-106: Update the request/response handling around errReply and
MsgResponse so failures are identified by a dedicated MessageType rather than by
inspecting an "ERR " payload prefix; return data values unchanged, including
values beginning with "ERR ", and decode error details only from the dedicated
error frame. If the protocol change cannot be made here, document the
ERR-prefixed value collision in errReply’s doc comment.
- Around line 130-132: Update all callers of the network client’s Set, Get, and
Delete methods to handle their new error returns, including ResponseNotFound,
ResponseNotAuthorized, and other ERR payloads, rather than treating those
payloads as values. Preserve existing success handling, propagate or handle
errors appropriately at each caller, and document this breaking API change in
the release notes.
In `@internal/network/role_test.go`:
- Around line 205-225: Update startRBACNetworkServer to use
net.ListenConfig.Listen with an explicit context when reserving the address, and
capture the error returned by srv.ListenAndServe in a channel instead of
discarding it. Have the readiness wait path report the captured server error
when startup fails, while preserving the existing cleanup and successful startup
behavior.
- Around line 71-99: Update TestRoleListCodec to compare each decoded namespace
byte slice with the corresponding entries[i].Namespaces value using bytes.Equal,
in addition to the existing count checks. Extend the test inputs with a
namespace token longer than 65535 bytes so EncodeRoleArgs and
DecodeRoleListResponse exercise the length-prefix boundary while preserving the
existing round-trip assertions.
In `@internal/network/role.go`:
- Around line 133-189: Update DecodeRoleListResponse so each namespace stored in
e.Namespaces is copied into independent memory before appending, rather than
retaining a subslice of payload; leave the existing bounds checks and command
handling unchanged.
- Around line 23-38: Update EncodeRoleArgs to reject inputs that exceed the
uint16 wire limits instead of truncating lengths: validate each token against
the documented 64 KiB cap and validate the argument count before encoding.
Propagate the validation error through roleRequestPayload and the affected
exported Role* client methods, updating their signatures and callers as needed;
do not emit a partially encoded payload for invalid input.
In `@internal/network/server.go`:
- Around line 629-643: Update opAuthorized to call the keyless authorization
check for all ROLE opcodes, using the same authorizedCmd behavior as the RESP
server; retain msg.Key-based checks for non-ROLE commands.
In `@internal/rbac/config.go`:
- Around line 96-104: Update the user validation in Parse before constructing
the User value: reject any user with an empty Password unless Nopass is
explicitly true, while preserving the existing conflict check for Nopass with a
non-empty password. Ensure omitted or misspelled password fields therefore fail
configuration loading instead of creating a passwordless account.
In `@internal/rbac/manager.go`:
- Around line 21-38: Protect every Store mutation helper with a sync.Mutex held
across the complete Load, validation, clone/mutation, and Store sequence. Update
CreateRole (internal/rbac/manager.go:21-38), the helper at
internal/rbac/manager.go:43-56, the helper at internal/rbac/manager.go:60-68,
and DeleteRole (internal/rbac/manager.go:73-85); lock before each s.Load() and
unlock after s.Store(p), preserving existing validation and error paths.
In `@internal/resp/role.go`:
- Around line 68-82: Update roleSetUser to reject calls with exactly the
required user and role arguments unless an explicit password option is provided;
require either a >password or nopass option before calling
rbac.PasswordFromOpts. Apply the same validation rule to the binary ROLE SETUSER
handler so omitted password options cannot create passwordless users.
In `@internal/resp/server.go`:
- Around line 527-533: Update the authentication flow around UserFor to always
perform bcrypt verification, using the user’s PasswordHash for existing users
and a package-level dummyAuthHash generated once with the policy’s bcrypt cost
when the username is unknown. Preserve authFailed behavior while ensuring
missing-user and wrong-password paths incur the same bcrypt work.
In `@README.md`:
- Around line 192-195: Update the README authentication sample to remove the
passwordless default administrator: either delete the `default` user or assign
it a least-privileged role, and set `default_role` to the role intended for
unassigned users rather than `admin`. Preserve the explicitly password-protected
`admin` and `alice` examples.
In `@server/server.go`:
- Around line 417-427: Update the argument validation in Server.roleCreate so it
accepts a role name plus at least one rule, matching the RESP handler’s
contract; retain rejection for fewer than two total arguments and leave rule
construction and role creation unchanged.
- Around line 390-405: In the operation dispatch containing network.OpRoleCreate
through network.OpRoleGetUser, add a guard that checks whether s.policy is nil
before invoking any role handler; return the existing RESP-compatible
RBAC-disabled error response (the “ERR RBAC is not enabled” path) and only
dispatch to roleCreate, roleSetUser, roleDelUser, roleDelete, roleList, or
roleGetUser when the policy store is present.
---
Nitpick comments:
In `@client/client_role.go`:
- Around line 17-24: Update RoleSetUser to accept a typed options struct
containing Password and NoPass instead of raw passOptions [][]byte, and
construct the required wire tokens internally using the existing password-option
conventions. Preserve the documented last-option-wins behavior and nopass
password-clearing semantics while keeping validation and delegation through
c.valid and c.c.RoleSetUser.
In `@config/config.go`:
- Around line 284-292: Update the rbacConfig environment lookup in LoadConfig to
use TSD_RBAC_CONFIG, while retaining TELLSTONE_RBAC_CONFIG only as a
compatibility fallback if appropriate. Also update the related doc comment to
document the TSD_ prefix and fallback behavior consistently with the other
flags.
In `@internal/network/role_test.go`:
- Around line 128-203: Replace the duplicated ROLE switch in rbacTestHandler
with the server’s shared/exported ROLE dispatch so tests exercise production
decoding, validation, and error mapping. Update server.networkHandler or extract
its ROLE handling into a reusable helper, then have both paths call it; preserve
the intentional nil transport errors after domain failures are encoded as ERR
payloads.
In `@internal/rbac/config.go`:
- Around line 99-102: Update build() to validate each non-Nopass u.Password as a
bcrypt hash before assigning it to hash, accepting supported bcrypt prefixes and
rejecting malformed values with a configuration-load error. Preserve the
existing Nopass behavior and raw hash assignment for valid passwords.
In `@internal/resp/role_test.go`:
- Around line 15-41: Extend rbacTestPolicy with a role/user restricted to the
app: key prefix using GET permission, then add IsAllowed/command test coverage
asserting GET app:k succeeds and GET other:k returns NOPERM. Ensure the
assertions exercise the role’s key-prefix denial path rather than the existing
~* roles.
In `@internal/resp/server.go`:
- Around line 103-112: Guard the bcrypt hash generation in the requirePass
initialization block with the policy-store absence condition, matching the
existing policy == nil behavior in the other server. Ensure requirePass is
hashed only when no policy is configured, while preserving the current
invalid-password panic behavior when hashing is performed.
In `@server/server.go`:
- Around line 146-155: Update the SIGHUP handling goroutine around signal.Notify
and reloadRBAC so it exits when Run returns, using the existing shutdown context
if available or otherwise coordinating channel closure with signal.Stop. Ensure
cleanup does not race with signal delivery and preserve RBAC reload behavior
while Run is active.
🪄 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: 824e7f54-aad3-4563-9bbb-dbccc19adc8a
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (33)
README.mdclient/client_role.gocmd/example/role/main.goconfig/config.gogo.modinternal/metrics/metrics_test.gointernal/network/benchmark_tls_test.gointernal/network/client.gointernal/network/client_role.gointernal/network/protocol.gointernal/network/protocol_test.gointernal/network/role.gointernal/network/role_test.gointernal/network/server.gointernal/network/server_test.gointernal/rbac/config.gointernal/rbac/config_test.gointernal/rbac/manager.gointernal/rbac/manager_test.gointernal/rbac/policy.gointernal/rbac/policy_test.gointernal/rbac/role.gointernal/rbac/session.gointernal/rbac/session_test.gointernal/rbac/user.gointernal/rbac/user_test.gointernal/resp/protocol.gointernal/resp/role.gointernal/resp/role_test.gointernal/resp/server.gointernal/resp/server_test.gointernal/shard/runner.goserver/server.go
Signed-off-by: Maximilian Hagen <git@saxy.dev>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/network/role.go`:
- Around line 117-121: Update EncodeRoleListResponse to reject inputs whose
entry count exceeds math.MaxUint16 before appending the count to the buffer,
returning the existing failure result with no encoded payload. Preserve the
current encoding behavior for valid counts.
In `@server/server.go`:
- Around line 485-489: Sort the ROLE LIST entries by their Name field after
populating entries from p.Roles and before calling
network.EncodeRoleListResponse. Preserve the existing encoding and
error-handling flow while ensuring identical requests produce a stable,
name-ordered response.
🪄 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: 033c289f-7078-4e88-ad08-80644794e9e3
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (18)
README.mdclient/client_role.gocmd/example/role/main.goconfig/config.goconfig/config_test.gogo.modinternal/network/client_role.gointernal/network/role.gointernal/network/role_test.gointernal/network/server.gointernal/rbac/config.gointernal/rbac/config_test.gointernal/rbac/manager.gointernal/rbac/policy.gointernal/resp/role.gointernal/resp/role_test.gointernal/resp/server.goserver/server.go
🚧 Files skipped from review as they are similar to previous changes (9)
- go.mod
- README.md
- cmd/example/role/main.go
- config/config.go
- internal/resp/role.go
- internal/rbac/config.go
- internal/network/client_role.go
- internal/resp/server.go
- internal/network/server.go
Signed-off-by: Maximilian Hagen <git@saxy.dev>
Signed-off-by: Maximilian Hagen <git@saxy.dev>
Signed-off-by: Maximilian Hagen <git@saxy.dev>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/network/README.md`:
- Around line 21-22: Update the payload documentation in the protocol README to
describe the actual binary framing for each request type: MsgRequest as the
operation byte, key length, TTL, key bytes, and value bytes; ROLE requests as
argument count followed by each token’s length and bytes in Value; and MsgAuth
as username length, password length, then credentials. Replace the raw-SQL
description while preserving the existing MsgError and MsgResponse behavior.
🪄 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: 1ca17393-3a97-4009-b353-6f3e4d0fe5bd
📒 Files selected for processing (9)
cmd/example/role/main.gointernal/network/README.mdinternal/network/client.gointernal/network/client_role.gointernal/network/protocol.gointernal/network/role_test.gointernal/network/server.gointernal/network/server_test.goserver/server.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/network/protocol.go
- cmd/example/role/main.go
- internal/network/client_role.go
- server/server.go
- internal/network/server.go
Saxy
added a commit
that referenced
this pull request
Aug 1, 2026
* 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>
This was referenced Aug 3, 2026
Merged
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
1 participant
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.
Description
Adds the ROLE command family and per-user bcrypt auth over both protocols, replacing --require-pass when an RBAC policy is loaded:
Component: Networking/RESP, CLI
Type of Change:
Related Issue
#16
Technical Deep Dive & Context
The policy store is an immutable snapshot behind a single
atomic.Pointerswap: readersLoad()without blocking or allocating, and writers clone → mutate → republish in one operation. ASessionContextis pinned to a connection at AUTH time and references an immutable*Role, so a SIGHUP hot-reload (which swaps the whole snapshot) never invalidates in-flight sessions; only future auth and fresh lookups observe the new policy.Authorization on the hot path is one bit test (command permission in a
Bitset) plus a key-prefix whitelist scan over raw bytes — no allocations, no locks. Fail-closed: a session with a nil role denies everything. bcrypt verification runs on a bounded worker pool off the gnet event loop so hash work never blocks request processing; a nopass user (empty hash) accepts any password, matching Redis ACL semantics.RESP and binary paths behave identically: unauthenticated data ops get
-NOAUTH/ERR INVALID_AUTH, role-denied ops get-NOPERM/ERR NOT_AUTHORIZED.--rbac-configsupersedes--require-passwhen both are set. RBAC is disabled by default, so existing deployments see zero change.Performance & Benchmarks (If Applicable)
No end-to-end server benchmark was run; RBAC is opt-in and disabled by default (zero overhead when off). The added per-op cost when enabled is one bit test + prefix scan. Gating micro-benchmark (
go test -bench=. -benchmem ./internal/rbac/):How Has This Been Tested?
go vet ./...— cleango test ./...— all passgo test -race ./internal/rbac/ ./internal/resp/ ./internal/network/— passinternal/rbac,internal/resp,internal/network(auth, nopass, gating, ROLE codecs, client methods, hot-reload)redis-cli: NOAUTH gating,AUTH admin adminsecret,ROLE CREATE/SETUSER/GETUSER/LIST, and per-role denial (aliceread-only,bobwrite-only,carol~users:*-scoped) — all returned expectedOK/NOPERMrepliescmd/example/role: admin creates a role + user, the user's GET passes and SET / out-of-namespace GET are deniedChecklist
go test ./...andgo test -race ./...)go vetwarningsSummary by CodeRabbit
New Features
Bug Fixes
Documentation