Conversation
…ng it with +OK
The rule parser matched lowercase literals and ended in `_ => {}`, so any
token it did not recognise was silently ignored while the call answered
OK. `ACL SETUSER svc nocommands` and `ACL SETUSER svc OFF` -- the two
things an operator types to contain a compromised credential -- both
returned +OK and changed nothing; the account stayed live with +@ALL.
Sweeping the redis 8.6.1 grammar against both servers found the true set
was much wider than the four tokens in the issue title:
- every non-lowercase spelling of on/off/nopass/resetpass/resetkeys/
resetchannels/reset (redis compares keywords with strcasecmp)
- allkeys, allchannels, allcommands, nocommands, sanitize-payload,
skip-sanitize-payload, clearselectors: no arm at all
- %r~ %w~ %RW~ %rw~ %WR~ and bare %R dropped; %X~k %RR~k %~k % accepted
- unknown tokens (bogus, @READ, nocommand, ' on') accepted
- unknown commands (+bogus, -flushal, +, -, +|get, +config|bogus)
accepted; `-` even stored a "-" deny entry
- #hash / !hash accepted any payload (an uppercase hash then never
authenticated); <pw / !hash for an absent credential answered OK
- >pw and #hash did not clear nopass: requiring a password left the
account passwordless; nopass did not remove old passwords, so
`nopass >new` kept the old credential valid
- (...) selectors silently dropped
- a bad token mid-list still created or mutated the user
Fix, in one place:
- `parse_rule` is the single tokenizer. Keyword spelling is compared in
exactly one table lookup (`KEYWORDS`, eq_ignore_ascii_case); prefixed
payloads (passwords, patterns, channels) stay verbatim, as in redis.
- `%` flags follow redis's grammar: R/W case-insensitive, any order,
each at most once, `~` optional (missing == empty pattern).
- `+`/`-` command names are validated against `@all`, which is
identical to the COMMAND_META registry (273 == 273, verified). A
container with a subcommand table must name a real subcommand; any
other command takes the first-arg form verbatim, as redis does.
- hashes must be 64 lowercase hex; removing an absent credential is
an error; `>`/`#` clear nopass; `nopass` clears the password list.
- `AclRuleError` gains Syntax, UnknownCommand, BadPasswordHash,
NoSuchPassword, SelectorsUnsupported. Every Display string except the
selector one is redis's own text, transcribed from the oracle.
- `try_apply_setuser` applies the rule list to a COPY of the user and
commits only when every rule applied, so a rejected call -- including
a state-dependent one like `<nope` -- leaves the table byte-identical.
`validate_rules` (category-only precheck from #978) is subsumed.
`apply_setuser` delegates to it and has the same atomicity.
- Selectors are refused with "ACL selectors are not supported" rather
than dropped: accepting one silently drops a grant the operator asked
for, and moon cannot enforce it.
Composes with #978: an unknown category still errors through
allow_command/deny_command; an unknown token now errors through
parse_rule; a valid token now works.
Out of scope, noted: `~x` after `~*` (redis errors, moon appends -- no
privilege effect); ACL LIST rendering of sanitize-payload/resetchannels
and re-folding expanded categories (#981); startup abort on a bad aclfile
line (kept as WARN + skip-user, the #978 contract).
Refs moon#979. Stacked on #987 (moon#978); #987 must merge first.
author: Tin Dang
`check_key_permission` had a blanket `key_patterns.is_empty() -> deny` ahead of the "command provably names no key" check, so a user with no key patterns could not run PING, DBSIZE or any other keyless command. Redis gates only KEYED commands on key patterns: after `RESETKEYS`, `SET k 1` is `NOPERM No permissions to access a key` and `PING` is still `PONG`. This was invisible until #979 made `RESETKEYS` (and `allkeys`, and the uppercase spellings) actually take effect. The per-key loop at the bottom already denies every keyed command when the pattern list is empty (`any` over nothing is false), so removing the early return loses no protection; the only observable change is that keyless commands are permitted, as on redis. Measured (redis 8.6.1 vs this branch), user `on >pw +@all`, no `~`: PING redis=PONG moon(before)=NOPERM ... has no key permissions moon(after)=PONG SET k 1 redis=NOPERM moon(before)=NOPERM moon(after)=NOPERM Refs moon#979. author: Tin Dang
A delimited block (`BEGIN/END acl-rule-token-section`) appended after the #978 section, reusing its `acl_*` helpers, touching only `n979:*` users and keys. Every row is a moon-vs-redis comparison; the revocation rows observe ENFORCEMENT (a denied command, a refused AUTH), never a flag read-back. Rows: nocommands/NOCOMMANDS/NoCommands deny PING and SET; OFF/Off/RESET/ Reset/RESETPASS refuse AUTH; RESETKEYS denies SET and keeps PING; RESETCHANNELS denies PUBLISH; -SET (uppercase) denies SET, keeps GET; nopass then >pw2 refuses both a wrong password and the OLD one; ALLKEYS ALLCOMMANDS ALLCHANNELS grant from nothing; a +@ALL user with no key patterns keeps keyless PING and is denied SET; %rw~ and %r~ (lowercase flags) grant exactly what they name; every rejected token's error text byte-for-byte (Syntax error, Unknown command or category name, bad hash, password does not exist), each followed by a probe that the user is intact; sanitize-payload/skip-sanitize-payload/clearselectors/'' are accepted; a bad token mid-list is a whole-call no-op for a new AND an existing user; a selector is refused (moon-only assertion, no oracle parity possible). Key/channel denials use `assert_acl_both_denied` because moon's NOPERM wording for a key differs from redis's (pre-existing, not this issue). Extracted verbatim and run against the live pair: a8eb2ef (pre-fix control) PASSED=26 FAILED=52 #987 tip (4515d0d) PASSED=26 FAILED=52 (same red set) this branch PASSED=78 FAILED=0 Refs moon#979. author: Tin Dang
…top a multi-byte rule from crashing the server Brings PR #998's rule-token fix (#979) and its credential fixes (#999) onto main, reconciled with #987 (squash-merged as 6eef99b) and #992 (moon#981, 16c4213), and finishes #970. The three #998 commits are carried verbatim ahead of this one; this commit is what reconciling and finishing them took. Reconciliation with #992: - `AclTable::apply_setuser` discarded `try_apply_setuser`'s error. Under validate-then-commit, a fixture with one bad token builds NO user, so every "is denied" assertion in that test passes vacuously against an absent user -- the same trap #992 closed in its io.rs helper. It is now a #[cfg(test)] fixture that panics naming the rejected rule; every production path already calls `try_apply_setuser`. All ~40 fixtures pass, proving none of them held a rejected rule. - #998's consistency section had been merged AFTER #992's moon#981 block, which restarts both servers with an --aclfile and says it must run last. The section now runs before it. #970, finished: - `allkeys` / `~*` and `allchannels` / `&*` REPLACE the pattern list, as in redis 8.6.1 (`ACLSetSelector` empties it). `~a %R~b allkeys` used to be reported and saved as `~a %R~b ~*`; it is now `~*`, byte-identical to redis. `*` read+write is a superset of every pattern, so no permission changes and an older aclfile line loads to the same set. A pattern AFTER `~*` is still accepted (redis rejects it) so an existing `~* ~x` file keeps loading. - One key-pattern renderer (`io::key_pattern_to_rule`) behind SAVE, LIST and GETUSER. GETUSER's second copy rendered a no-access pattern as `%W~p`, which reloads as a write grant; the one renderer renders it as nothing. - `ACL SETUSER` dropped a non-UTF-8 argument via `filter_map(extract_str)` and still answered OK. It now rejects the whole call (`ACL rules must be valid UTF-8`). Crash fixed (introduced by #998, never on main): `parse_rule` sliced `&rule[1..]` before looking at the prefix, so a token whose first character is multi-byte (`ACL SETUSER u on éx`) panicked the shard thread and aborted the whole server; the same token in an aclfile would crash every boot. It now slices at the first char's `len_utf8`. Redis answers `Syntax error`, and so does moon. Verified against redis-server 8.6.1, same rows on both binaries: - #999 through AUTH, HELLO 3 AUTH and inline AUTH: main accepts a wrong password after `nopass` then `>pw` / `#hash`, and the old password after `>old nopass >new`; this branch answers WRONGPASS on each, as redis does. - #970/#979: extracted ACL sections of test-consistency.sh, main PASSED=171 FAILED=92, branch PASSED=263 FAILED=0; the new test-commands.sh `acl` category, main 9/29, branch 29/29. - #981 is fixed on main: `+@ALL -flushall` is identical in LIST, GETUSER, the aclfile, after ACL LOAD and after kill -9 plus restart. It stays so here, along with every permission set this change touches. - Mutations: `>pw` keeping nopass, `nopass` keeping hashes, and restoring the non-UTF-8 drop each turn the unit tests red. Closes #979 Closes #999 Closes #970 Refs #981 Supersedes #998 author: Tin Dang
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (7)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughChangesACL SETUSER behavior
Priority: ⬆️ High Estimated code review effort: 4 (Complex) | ~60 minutes Change: Bug fix · Severity of issue fixed: High Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Client
participant ACL_SETUSER
participant AclTable
participant ACLPersistence
participant AUTH
Client->>ACL_SETUSER: SETUSER rules
ACL_SETUSER->>AclTable: validate and apply rules
AclTable-->>ACL_SETUSER: commit or return error
ACL_SETUSER-->>Client: OK or ACL error
Client->>ACLPersistence: ACL SAVE and LOAD
ACLPersistence->>AclTable: restore serialized rules
Client->>AUTH: authenticate with credential
AUTH->>AclTable: check enabled user and password
AclTable-->>AUTH: authentication result
Merge Risk: ⚪ Minimal · up to No concrete merge-blocking regression remains; the ACL behavior and persistence changes appear ready for normal merge checks. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
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.
Closes #979 · Closes #999 · Closes #970 · Refs #981
Supersedes #998. It carries #998's three commits verbatim, rebased onto
mainpast #987 (squash6eef99b0) and #992 (16c4213f), plus one commit that reconciles and finishes them. Please close #998 after this merges. Don't merge #998 itself: it's still based on the pre-squash #987 commits.What this fixes
16c4213f)on nopassthen>realpw,AUTH fa totallywrongOK(AUTH, HELLO 3 AUTH, inline AUTH)WRONGPASSWRONGPASSon nopassthen#<sha256>, wrong passwordOKWRONGPASSWRONGPASS>oldpwthennopassthen>newpw,AUTH fb oldpwOKWRONGPASSWRONGPASS+@alluser, thennocommands, thenPINGPONGNOPERMNOPERMon >pw allkeys allcommands allchannels, thenACL LIST-@all(inert user)~* &* +@all~* &* +@all%RW~cache:* +@read, thenGET cache:1NOPERM ... no key permissions(nil)(nil)~a %R~b allkeysrendering~a %R~b(allkeys dropped)~*~*totalnonsense,+bogus,#ABC,%X~kOK(and#ABCstored as a hash)How #998 was reconciled with #992
apply_setuserswallowedtry_apply_setuser's error. Under validate-then-commit, a fixture with one bad token builds no user, so every denial assertion in that test passes vacuously. It is now a#[cfg(test)]fixture that panics and names the rejected rule. This is the same trap fix(acl): ACL SAVE writes the base polarity it holds instead of -@all for every user (moon#981) #992 closed in its io.rs helper. Every production path already callstry_apply_setuser.moon#981block, which restarts both servers with--aclfileand says it must run last. The section now runs before it.What was added on top of #998
allkeys/~*andallchannels/&*replace the pattern list, as redis does, so LIST, GETUSER and SAVE render them identically to redis. Permissions don't change.%W~p, which would reload as a write grant.OK. It is now rejected.ACL SETUSER u on éxsliced a multi-byte char, which panicked the shard thread and aborted the server. It is nowSyntax error, as on redis. Unit tests and rows in both scripts cover it.BEHAVIOUR CHANGE
ACL SETUSERnow errors on tokens it used to accept withOKand silently drop. An aclfile line containing one of them is no longer loaded: the user is absent and a WARN names the rule. See the CHANGELOG### Changedentry. A pattern after~*is still accepted, so an existing~* ~xaclfile keeps loading.Verification (redis-server 8.6.1 oracle, the same rows on both binaries)
16c4213ftest-consistency.sh(extracted verbatim)aclcategory oftest-commands.sh#981 is fixed on main.
+@all -flushallis identical in LIST, GETUSER, the aclfile, after ACL LOAD and after a kill -9 restart: GET is allowed and FLUSHALL is denied.Mutations each turn the unit tests red:
>pwkeepingnopass,nopasskeeping hashes, restoring the non-UTF-8 drop, and the multi-byte slice.Gates:
cargo fmt --check,cargo clippy --all-targets -D warningson both runtimes, andcargo test --releaseforacl:: command::acl:: command::connection:: scripting::on both runtimes.Out of scope (unchanged from #998)
~*is accepted, where redis rejects it. Rejecting it is a migration decision.ACL LISTexpands+@readinto individual commands.resetchannelsandsanitize-payloadare not rendered.src/acl/rules.rsis over the 1500-line limit. It was already 1849 lines on main, and moving the category table out is a separate refactor.Summary by CodeRabbit
ACL SETUSER, including case-insensitive keywords and support for key, command, and channel permissions.nopasshandling, including proper credential revocation and authentication enforcement.