More Complete ACL Implementation #386
Conversation
An attempt has been made to cover all commands, but more remain. A number of TODOs remain, but this gives a rough shape of how it'd work and how individual Commands or SubCommands would end up ACL'd too.
As part of this, needed to actually enumerate all commands in RespCommands which implies a lot of future cleanup in parsing and command processesing. This also uncovered a number of different gaps in implemented RESP commands, which were not addressed unless doing so was necessary for ACL testing.
Garnet-specific commands are still pending.
Introduces @garnet category. Reworks WATCH so MS and OS are proper subcommands.
… command ACL'ing.
Reworks parsing to remove the tuple with subcommand, as it'd always be 0 now.
…ands, while the common case is consolidated into an upfront check. Note that SET (and pseudo-variants SETEXNX, etc.) is a special case, as it lacks subcommands but has multiple command types. That could be cleaned up later, but has performance implications - so leaving it for future work.
…CL categories, replace slow temp code with better code, add tests to catch future additions; this involves reordering RespCommand yet again
|
There's still some more to do here (more testing, subcommands, some cleanup) , but I'm going on vacation for a week and this is pretty dang close to how it's going to actually look when done. So taking it off draft, and marking ready for review (if not merging). /cc @mgravell @NickCraver - this is relevant to your interests. |
kevin-montrose
changed the title
There was a problem hiding this comment.
I think this PR has too many changes to vet, maybe we need to break it into smaller PRs. I think the first is good but very hard to review because the surface area is huge. I think the idea of having a bitmap to match command permissions is good. There might I couple alternatives to tradeoff between CPU cycles or memory. We need to separate general fixes from clear ACL enchantments and avoid pushing parsing inside the execution methods.
Yeah, it's gigantic - ACLs touch literally everything. Excluding tests, it is trending smaller as things DRY up. A bunch of random-ish fixes are somewhat unfortunate, as a big part of testing this is making sure that failed ACLs don't bork the command stream - and I encountered a number of cases of fairly "brittle" command implementations. I'm open to breaking those out into separate PRs once ACLs are in a good place, but I needed some confidence that the ACL design would work for all commands upfront. |
…ssion ACL validation address feedback; break CommandPermissionSet out into separate file address feedback; increment correct statistic, fixing bug address feedback; adding count is unnecessary in many places, remove that post-DRY-ing
|
Great work overall. Shaping up really nicely, thanks! |
…lar commands; address a couple nits
This means we now document all of these in RespCommandsInfo.json, as they're just normal commands from the perspective of ACL'ing.
…rt of this, correct some places where commands weren't tagged @fast or @slow (everything should be tagged one or the other)
…have no impact. This implementation isn't amazing, but ACL changing is hopefully rare and privileged enough that it is acceptable.
… ACL test commands that have been corrected
|
Benchmark (Release mode): Framework: net8.0Branch: main (on Kevin's fork)Running with 1 threads... Branch: aclImprovements (on Kevin's fork)Running with 1 threads... SummaryLooks like we are at around 54% performance regression at the moment. Need to drill deeper into why. Edit: looks like there is too much variance across runs so take above with a grain of salt. |
There was a problem hiding this comment.
There are some issues related to code style. Please use compact switch statements where possible.
There are some protocol compatibility issues with the responses generated for invalid input in cluster and bitmap commands.
Thanks for refactoring the cluster commands to make them compatible with the improved ACL implementation.
…or messages, and cleaning up subcommand parsing
I reproduce this regression on my latest too, I'll spend some time digging (now that all feedback is addressed) and update when I have an explanation/fix. |
| return false; | ||
|
|
||
| while (!RespWriteUtils.WriteError($"ERR Unknown subcommand or wrong number of arguments for COMMAND.", ref dcurr, dend)) |
There was a problem hiding this comment.
Error should be in the format of CmdStrings.GenericErrUnknownSubCommand
|
|
||
| // Handle COMMAND COUNT | ||
| if (count > 0 && (subCommand.SequenceEqual(CmdStrings.COUNT) || subCommand.SequenceEqual(CmdStrings.count))) | ||
| while (!RespWriteUtils.WriteError($"ERR Unknown subcommand or wrong number of arguments for COMMAND COUNT.", ref dcurr, dend)) |
There was a problem hiding this comment.
Error should be in the format of CmdStrings.GenericErrWrongNumArgs
| /// have a matching RespCommandInfo objects defined in RespCommandsInfo | ||
| /// </summary> | ||
| [Test] | ||
| public void CommandsInfoCoverageTest() |
There was a problem hiding this comment.
This test was removed from main, we decided not to have it enforced. Alternatively, we can add an Ignore attribute
main:
PR:
|
Doesn't appear to be allocations based on local profiling. Might be useful to add Doing some quick and dirty benchmarking with the Embedded.perftest example, it looks like it's mostly the cost of doing the On my box, .NET 8 with PGO disabled I'm getting about...
Total loss ~30%, and a solid ~25% (~15K I'll noodle on speeding this up in the morning. |
I have updated the branch with |
|
Mkay, because I can't leave well enough alone despite it being after 11 PM here... One relatively simple approach would be to slap a I need to think on if there's any point in keeping the bit-packed version around; if not, we could just initialize the int[] with the "truth" and then there's not even an update. Another, much less simple, approach would be to lean in to That might ultimately be a bit cleaner, but it'd require a bunch of additional changes on top of this already gigantic change. There's also an indirection in there which makes me not so confident the perf would be any better. So I'll keep pursuing the first approach. Of course, if there are any other clever ideas, I'd be happy to hear them. |
I'm not sure we need to do this. The bitmap is nice and tight. There might be a simpler way to get back the performance. Can look into it tomorrow. |
This sketches out a more proper ACL implementation - one with categories beyond @admin, and where individual commands can be ACL'd.
Basic idea is to use
RespCommandsInfoas a mechanism to discover command ACLs, and attach a bitfield to eachUserto track individual commands rather than categories.Outstanding TODOs:
+get)-config|get)CommandCategory- it is redundantSET(with EX, NX, etc. but notSETEX) are still special cased as they weren't realized as subCmds+customcmdworks)This is explicitly not going to implement keyspace patterns. I feel like that should be a separate enhancement, as it's pretty substantial on it's own.
The biggest incidental change here is a substantial refactoring of
RespCommandand the various switches over it. This is, IMO, the cleanest way to get a canonical value to test against ACLs with. As a consequence,RespCommandhas a bunch more entries as most things that were previously "sub commands" (but not in the RESP sense) are now actual enum values.There's one additional implementation detail to call out, which is how ACL descriptions are maintained. So that ACL descriptions behave more or less like you'd expect (that is
+@foo +bar -barbecomes+@foo) a greedy approach is taken where, upon update, a trial removal of each token is made to see if it impacts the effective ACL set. This is pretty allocate-y, and while that could be improved I think ACL manipulation is rare and locked down enough that clarity was a bit more important. It's worth a stringent review, however. Code is in User.RationalizeACLDescription.