More Complete ACL Implementation (#386)

* Introducing remaining ACL categories

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.

* Covers most of the remaining commands.

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.

* Cover all remainin commands actually in Redis.

Garnet-specific commands are still pending.

* Handle Garnet specific commands.

Introduces @garnet category.
Reworks WATCH so MS and OS are proper subcommands.

* Get us to all tests passing before moving onto cleanup and individual command ACL'ing.

* Rework BITOP to remove last subcommand byte in parsing.
Reworks parsing to remove the tuple with subcommand, as it'd always be 0 now.

* DRY up ACL checking - now we only need specific call out for sub commands, 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.

* remove CommandCategory, it is redundant

* handle some todos; bring read/write command checking into line with ACL categories, replace slow temp code with better code, add tests to catch future additions; this involves reordering RespCommand yet again

* lay groundwork for adding/removing individual commands from ACLs

* expand tests to also exercise individual commands

* knock some TODOs out, get these checks into simpler branchless range checks

* expand tests to cover individual command ACL'ing

* cleanup a number of style nits and analyzer/diagnostic warnings

* nit: typo

Co-authored-by: Lukas Maas <[email protected]>

* address feedback; fix bug, skip already processed bytes in cluster session 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

* address feedback; no reason to special case default user anymore

* Rework parsing and RespCommand so RESP subcommands have entries in RespCommand.

* DRY up testing for ACLs now that categories, commands, and subcommands are all covered.

* Restore BITOP pseudo-subcommand parsing and cleanup treatment of similar commands; address a couple nits

* Test remaining "weird" commands, including all CLUSTER subcommands.
This means we now document all of these in RespCommandsInfo.json, as they're just normal commands from the perspective of ACL'ing.

* Handle CustomXXX commands; this isn't amazing, because individual commands cannot be ACL'd, but ACL'ing all custom (with @Custom) or as part of Garnet extensions (with @garnet) works as does ACL'ing all custom non-object (with customcmd), object (with customobjcmd), and transaction (with customtxn) separately
Fixup a number of test failures and nits.

* handle some lingering TODOs and nits

* implement correct ACL reporting for the new ACL implementation; as part of this, correct some places where commands weren't tagged @fast or @slow (everything should be tagged one or the other)

* fixup remaining tests

* Change ACL description implementation to greedily remove tokens that have no impact.
This implementation isn't amazing, but ACL changing is hopefully rare and privileged enough that it is acceptable.

* lots of little cleanup nits

* cleanup formatting

* fix formatting nits in tests

* Address feedback; many style nits, restoring or correctig various error messages, and cleaning up subcommand parsing

* address feedback; use compact switch, fix some nits

* address feedback; remove dead test, correct more error messages

* some exploratory performance changes

worth about 6K PINGs / s: removes IGarnetAuthenticator check from ACL hot path simplify checking ACLs by pre-de-normalizing commands
worth about 2K PINGs / s: force inlining on hot user perms path
worth about 1K PINGs / s: hide more (unlikely) failed ACL logic behind a method call, pre-calculate NoAuth checks to remove a branch, move Span creation into places where it's actually needed, remove a pointer chase for ACL +@ALL special casing, removing -@ALL special casing as it's in the slow path

* exploratory performance changes

cache CanAuthenticate to elide a virtual call, looks like it's worth a little bit (maybe .2 - .5 us) in the no-auth case
add ACL case to RespParseStress

* address feedback

* exploratory commit; remove some authenticator checks that badrishc (and testing) showed were redundant, removes the caching of CanAuthenticate, cleanup some of the other changes from prior commits

* formatting fixes

* address feedback; add a test for user auth invalidation, fix bugs with user auth invalidation, remove some optimizations that are now not actually useful after the bugfixes

* fix formatting

benchmark/BDN.benchmark/Resp/RespParseStress.cs

@@ -5,6 +5,7 @@
 using BenchmarkDotNet.Attributes;
 using Embedded.perftest;
 using Garnet.server;
+using Garnet.server.Auth.Settings;
 
 namespace BDN.benchmark.Resp
 {
@@ -28,30 +29,54 @@ public unsafe class RespParseStress
         byte[] getRequestBuffer;
         byte* getRequestBufferPointer;
 
+        [Params(false, true)]
+        public bool UseACLs { get; set; }
+
         [GlobalSetup]
         public void GlobalSetup()
         {
-            var opt = new GarnetServerOptions
+            IAuthenticationSettings authSettings = null;
+            string aclFile = null;
+
+            try
+            {
+                if (UseACLs)
+                {
+                    aclFile = Path.GetTempFileName();
+                    File.WriteAllText(aclFile, @"user default on nopass -@all +ping +set +get");
+                    authSettings = new AclAuthenticationPasswordSettings(aclFile);
+                }
+
+                var opt = new GarnetServerOptions
+                {
+                    QuietMode = true,
+                    AuthSettings = authSettings,
+                };
+                server = new EmbeddedRespServer(opt);
+                session = server.GetRespSession();
+
+                pingRequestBuffer = GC.AllocateArray<byte>(INLINE_PING.Length * batchSize, pinned: true);
+                pingRequestBufferPointer = (byte*)Unsafe.AsPointer(ref pingRequestBuffer[0]);
+                for (int i = 0; i < batchSize; i++)
+                    INLINE_PING.CopyTo(new Span<byte>(pingRequestBuffer).Slice(i * INLINE_PING.Length));
+
+                setRequestBuffer = GC.AllocateArray<byte>(SET.Length * batchSize, pinned: true);
+                setRequestBufferPointer = (byte*)Unsafe.AsPointer(ref setRequestBuffer[0]);
+                for (int i = 0; i < batchSize; i++)
+                    SET.CopyTo(new Span<byte>(setRequestBuffer).Slice(i * SET.Length));
+
+                getRequestBuffer = GC.AllocateArray<byte>(GET.Length * batchSize, pinned: true);
+                getRequestBufferPointer = (byte*)Unsafe.AsPointer(ref getRequestBuffer[0]);
+                for (int i = 0; i < batchSize; i++)
+                    GET.CopyTo(new Span<byte>(getRequestBuffer).Slice(i * GET.Length));
+            }
+            finally
             {
-                QuietMode = true
-            };
-            server = new EmbeddedRespServer(opt);
-            session = server.GetRespSession();
-
-            pingRequestBuffer = GC.AllocateArray<byte>(INLINE_PING.Length * batchSize, pinned: true);
-            pingRequestBufferPointer = (byte*)Unsafe.AsPointer(ref pingRequestBuffer[0]);
-            for (int i = 0; i < batchSize; i++)
-                INLINE_PING.CopyTo(new Span<byte>(pingRequestBuffer).Slice(i * INLINE_PING.Length));
-
-            setRequestBuffer = GC.AllocateArray<byte>(SET.Length * batchSize, pinned: true);
-            setRequestBufferPointer = (byte*)Unsafe.AsPointer(ref setRequestBuffer[0]);
-            for (int i = 0; i < batchSize; i++)
-                SET.CopyTo(new Span<byte>(setRequestBuffer).Slice(i * SET.Length));
-
-            getRequestBuffer = GC.AllocateArray<byte>(GET.Length * batchSize, pinned: true);
-            getRequestBufferPointer = (byte*)Unsafe.AsPointer(ref getRequestBuffer[0]);
-            for (int i = 0; i < batchSize; i++)
-                GET.CopyTo(new Span<byte>(getRequestBuffer).Slice(i * GET.Length));
+                if (aclFile != null)
+                {
+                    File.Delete(aclFile);
+                }
+            }
         }
 
         [GlobalCleanup]

libs/cluster/CmdStrings.cs

@@ -13,50 +13,11 @@ static class CmdStrings
         /// <summary>
         /// Request strings
         /// </summary>
-        public static ReadOnlySpan<byte> INFO => "INFO"u8;
-        public static ReadOnlySpan<byte> CLUSTER => "CLUSTER"u8;
-        public static ReadOnlySpan<byte> NODES => "NODES"u8;
-        public static ReadOnlySpan<byte> ADDSLOTS => "ADDSLOTS"u8;
-        public static ReadOnlySpan<byte> ADDSLOTSRANGE => "ADDSLOTSRANGE"u8;
-        public static ReadOnlySpan<byte> BUMPEPOCH => "BUMPEPOCH"u8;
-        public static ReadOnlySpan<byte> BANLIST => "BANLIST"u8;
-        public static ReadOnlySpan<byte> COUNTKEYSINSLOT => "COUNTKEYSINSLOT"u8;
-        public static ReadOnlySpan<byte> DELKEYSINSLOT => "DELKEYSINSLOT"u8;
-        public static ReadOnlySpan<byte> DELKEYSINSLOTRANGE => "DELKEYSINSLOTRANGE"u8;
-        public static ReadOnlySpan<byte> DELSLOTS => "DELSLOTS"u8;
-        public static ReadOnlySpan<byte> DELSLOTSRANGE => "DELSLOTSRANGE"u8;
-        public static ReadOnlySpan<byte> FAILOVER => "FAILOVER"u8;
-        public static ReadOnlySpan<byte> FORGET => "FORGET"u8;
-        public static ReadOnlySpan<byte> GETKEYSINSLOT => "GETKEYSINSLOT"u8;
-        public static ReadOnlySpan<byte> KEYSLOT => "KEYSLOT"u8;
-        public static ReadOnlySpan<byte> HELP => "HELP"u8;
-        public static ReadOnlySpan<byte> MEET => "MEET"u8;
-        public static ReadOnlySpan<byte> MIGRATE => "MIGRATE"u8;
-        public static ReadOnlySpan<byte> MTASKS => "MTASKS"u8;
-        public static ReadOnlySpan<byte> MYID => "MYID"u8;
-        public static ReadOnlySpan<byte> MYPARENTID => "MYPARENTID"u8;
-        public static ReadOnlySpan<byte> ENDPOINT => "ENDPOINT"u8;
-        public static ReadOnlySpan<byte> REPLICAS => "REPLICAS"u8;
-        public static ReadOnlySpan<byte> REPLICATE => "REPLICATE"u8;
-        public static ReadOnlySpan<byte> SET_CONFIG_EPOCH => "SET-CONFIG-EPOCH"u8;
-        public static ReadOnlySpan<byte> SETSLOT => "SETSLOT"u8;
-        public static ReadOnlySpan<byte> SETSLOTSRANGE => "SETSLOTSRANGE"u8;
-        public static ReadOnlySpan<byte> SHARDS => "SHARDS"u8;
-        public static ReadOnlySpan<byte> SLOTS => "SLOTS"u8;
-        public static ReadOnlySpan<byte> SLOTSTATE => "SLOTSTATE"u8;
-        public static ReadOnlySpan<byte> GOSSIP => "GOSSIP"u8;
         public static ReadOnlySpan<byte> WITHMEET => "WITHMEET"u8;
-        public static ReadOnlySpan<byte> RESET => "RESET"u8;
 
         /// <summary>
         /// Internode communication cluster commands
         /// </summary>
-        public static ReadOnlySpan<byte> aofsync => "AOFSYNC"u8;
-        public static ReadOnlySpan<byte> appendlog => "APPENDLOG"u8;
-        public static ReadOnlySpan<byte> initiate_replica_sync => "INITIATE_REPLICA_SYNC"u8;
-        public static ReadOnlySpan<byte> send_ckpt_metadata => "SEND_CKPT_METADATA"u8;
-        public static ReadOnlySpan<byte> send_ckpt_file_segment => "SEND_CKPT_FILE_SEGMENT"u8;
-        public static ReadOnlySpan<byte> begin_replica_recover => "BEGIN_REPLICA_RECOVER"u8;
         public static ReadOnlySpan<byte> failstopwrites => "FAILSTOPWRITES"u8;
         public static ReadOnlySpan<byte> failreplicationoffset => "FAILREPLICATIONOFFSET"u8;
 
@@ -65,15 +26,8 @@ static class CmdStrings
         /// Response strings
         /// </summary>
         public static ReadOnlySpan<byte> RESP_OK => "+OK\r\n"u8;
-        public static ReadOnlySpan<byte> RESP_RETURN_VAL_1 => ":1\r\n"u8;
-        public static ReadOnlySpan<byte> RESP_RETURN_VAL_0 => ":0\r\n"u8;
         public static ReadOnlySpan<byte> RESP_RETURN_VAL_N1 => ":-1\r\n"u8;
 
-        /// <summary>
-        /// Response string templates
-        /// </summary>
-        public const string GenericErrMissingParam = "ERR wrong number of arguments for '{0}' command";
-
         /// <summary>
         /// Generic error respone strings, i.e. these are sent in the form "-ERR responseString\r\n"
         /// </summary>
@@ -107,12 +61,16 @@ static class CmdStrings
         /// <summary>
         /// Simple error respone strings, i.e. these are of the form "-errorString\r\n"
         /// </summary>
-        public static ReadOnlySpan<byte> RESP_ERR_NOAUTH => "NOAUTH Authentication required."u8;
         public static ReadOnlySpan<byte> RESP_ERR_CROSSLOT => "CROSSSLOT Keys in request do not hash to the same slot"u8;
         public static ReadOnlySpan<byte> RESP_ERR_CLUSTERDOWN => "CLUSTERDOWN Hash slot not served"u8;
         public static ReadOnlySpan<byte> RESP_ERR_MIGRATING => "MIGRATING"u8;
         public static ReadOnlySpan<byte> RESP_ERR_CREATE_SYNC_SESSION_ERROR => "PRIMARY-ERR Failed creating replica sync session task"u8;
         public static ReadOnlySpan<byte> RESP_ERR_RETRIEVE_SYNC_SESSION_ERROR => "PRIMARY-ERR Failed retrieving replica sync session"u8;
         public static ReadOnlySpan<byte> RESP_ERR_IOERR => "IOERR Migrate keys failed"u8;
+
+        /// <summary>
+        /// Response string templates
+        /// </summary>
+        public const string GenericErrWrongNumArgs = "ERR wrong number of arguments for '{0}' command";
     }
 }