Add ACL DIGEST command - #4446
Conversation
A controller that manages a server's ACL by writing an aclfile and calling ACL LOAD has no way to confirm which revision the server loaded. Users and password hashes can be compared, since ACL GETUSER returns the hashes verbatim, but rules cannot: ACL GETUSER reports the server's normalized form while the controller only holds the original file text. A permission only change, same users and same passwords with different rules, cannot be confirmed at all. ACL DIGEST replies with a fingerprint of the rules currently in effect, as a hex string, so two revisions can be told apart without parsing any rule on the client side. For every user the SHA256 of its name and its rule string is computed, and the per user digests are combined with XOR. XOR is commutative, so the result does not depend on the order the users are visited in and the digest does not have to rely on the radix tree keeping them sorted. XOR also cancels out two equal values, so the name is hashed together with the rules to keep a pair of users having the very same rules from contributing nothing. The hashed content of a user is the line ACL LIST reports for it without the leading "user " keyword, so it covers the flags, the passwords, the commands, the keys and the channels. Any edit to any user moves the digest, and the reply stays the same size however large the ACL gets. The reply of ACL LOAD is left unchanged, since changing it would break existing clients. Signed-off-by: melancholictheory <selimvhorst@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan includes up to 10 reviews per rolling hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe PR adds ChangesACL digest
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The ACL DIGEST change is merge-ready after normal checks and review; no actionable merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant Client
participant ACLCommand
participant ACLDigest
participant ACLUsers
Client->>ACLCommand: Execute ACL DIGEST
ACLCommand->>ACLDigest: Compute ACL fingerprint
ACLDigest->>ACLUsers: Read user names and ACL descriptions
ACLUsers-->>ACLDigest: Return effective ACL state
ACLDigest-->>ACLCommand: Return hexadecimal fingerprint
ACLCommand-->>Client: Return ACL digest
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
![valkey-review-bot[bot]](https://avatars.githubusercontent.com/in/3633497?s=60&v=4){kind=small}
| "LOADING", | ||
| "STALE", | ||
| "SENTINEL" | ||
| ], |
There was a problem hiding this comment.
This is node-local state, but there is no request-policy tip telling cluster clients to query every node. ACL SETUSER, ACL DELUSER, and ACL SAVE all declare REQUEST_POLICY:ALL_NODES, and read-only node-local introspection such as SLOWLOG GET pairs that with NONDETERMINISTIC_OUTPUT. Without the same metadata here, a cluster-aware client can route ACL DIGEST to one arbitrary node and miss an ACL revision that differs elsewhere. Add command_tips with REQUEST_POLICY:ALL_NODES and NONDETERMINISTIC_OUTPUT.
There was a problem hiding this comment.
The comparisons are accurate: ACL SETUSER, ACL DELUSER and ACL SAVE do carry REQUEST_POLICY:ALL_NODES, and SLOWLOG GET does pair it with NONDETERMINISTIC_OUTPUT. I left both tips off on purpose.
NONDETERMINISTIC_OUTPUT would say the wrong thing here. The definition in command-tips is that calls "may yield different results with the same arguments and data", which is what INFO, CLIENT LIST and TTL do. ACL DIGEST is the opposite: the same ACL state always produces the same reply. That is the contract of the command and there is a test asserting it. The tip tells a client the reply is not meant to be compared, and comparing it is the only thing the command is for.
REQUEST_POLICY:ALL_NODES is a fairer question, but I do not think it belongs on this command alone. The rest of the ACL read family carries no tips at all: ACL LIST, ACL USERS, ACL GETUSER, ACL WHOAMI, and ACL LOAD too. ACL DIGEST is node local in exactly the way ACL LIST is. Tagging only the digest puts a cluster client in an odd spot, where it fans out ACL DIGEST, finds two nodes disagreeing, and then cannot fan out ACL LIST the same way to see what differs. There is also no useful RESPONSE_POLICY for a set of hex strings, so it would end up as SPECIAL.
If node-local ACL introspection should be fanned out, I would rather do it as one change across the whole family than on the single new command, and I can open that separately. If a maintainer wants the tip here now, say so and I will add it.
|
A few things I checked while writing this, so a reviewer does not have to chase them.
The space between the name and the rules is safe as a separator because the server rejects a username containing one. The command flags and ACL categories are copied from
The valkey-doc PR is on me, I will open it once the command shape here is settled. |
zuiderkwast
left a comment
zuiderkwast
left a comment
There was a problem hiding this comment.
Very nice PR.
Very thorough tests. (I would have accepted less. 😆)
zuiderkwast
added
the
release-notes
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## unstable #4446 +/- ##
============================================
+ Coverage 78.77% 78.79% +0.01%
============================================
Files 170 170
Lines 89782 89807 +25
============================================
+ Hits 70725 70761 +36
+ Misses 19057 19046 -11
🚀 New features to boost your workflow:
|
The reasoning about ordering and about the shape of the hashed string belongs in the pull request and the issue rather than in the code. Keep the note about the username being hashed along with the rules, since that is the part a later change could drop without an obvious reason. Signed-off-by: melancholictheory <selimvhorst@gmail.com>
zuiderkwast
left a comment
zuiderkwast
left a comment
There was a problem hiding this comment.
LGTM, thanks!
We'll need for the majory decision in the issue before merging. (We'll auto-approve after two weeks if no TSC members have any objections.)
enjoy-binbin
left a comment
enjoy-binbin
left a comment
There was a problem hiding this comment.
Top comment LGTM, did not review the code.
|
+1 for the context of core team approval, API seems fine. |
|
Correcting myself: I wrote above that the valkey-doc PR was still to come, but it was already open by then. It is valkey-io/valkey-doc#469, and it has had no review yet. It adds Nothing breaks if it lands first or sits: the doc Makefile builds the intersection of the Markdown pages and the command JSON under |
4 participants
Fixes #4355
A controller that manages a server's ACL by writing an aclfile and calling
ACL LOADhas no wayto confirm which revision the server actually loaded. Comparing users and password hashes works,
since
ACL GETUSERreturns the hashes verbatim, but comparing rules does not.ACL GETUSERreports the server's normalized form while the controller only holds the original file text, so
matching the two means reimplementing the ACL parser and keeping it in step with the server. A
permission-only change, where the users and the passwords stay the same and only the rules move,
cannot be confirmed at all.
ACL DIGESTreturns a fingerprint of the rules currently in effect, as a hex string:Read it before and after a
LOADand you know whether the revision you wrote is the one running,and the comparison needs no rule parsing on the client side.
How it works
For every user the SHA256 of its name and its rule string is computed, and the per user digests
are combined with XOR, as @zuiderkwast suggested in the issue.
XOR is commutative, so the result does not depend on the order the users are visited in. They are
kept in a radix tree and are already sorted, but the digest does not have to rely on that. XOR
also cancels out two equal values, so hashing the rules alone would let a pair of users with the
very same rules contribute nothing. The name is hashed together with the rules to keep each user
distinct, and there is a test for that case.
What gets hashed per user is the line
ACL LISTreports for it, minus the leadinguserkeyword,so it covers the flags, the passwords, the commands, the keys and the channels. Any edit to any
user moves the digest, and the reply stays the same size however large the ACL gets.
The reply of
ACL LOADis left alone, since changing it would break existing clients.ACL DIGESTcarries the same flags and categories asACL LISTandACL USERS. It is built fromthe same data, and anyone who can run
ACL LOADalready has those permissions.The hex encoding loop moved out of
ACLHashPasswordinto a small helper, so the new code reusesit rather than repeating it.
Testing
New cases in
tests/unit/acl.tcl:ACL LOADof an equivalent file, written in a different order and with the aliases of thesame rules, leaves the digest untouched, while a permission-only edit moves it
Documentation for the command will follow in a valkey-doc PR.