Add per-name ACL support for custom (extension) commands#1822
Open
Ankith-Kandala wants to merge 1 commit into
Open
Add per-name ACL support for custom (extension) commands#1822Ankith-Kandala wants to merge 1 commit into
Ankith-Kandala wants to merge 1 commit into
Conversation
Garnet's ACL bitmap only covers built-in RespCommand IDs, so per-name allow/deny rules like +json.set / -json.get against extension commands were rejected by the parser (AclCommandDoesNotExistException) — leaving operators with the all-or-nothing +@Custom category. Cosmos DB's managed Garnet RBAC needs fine-grained per-name control over extension commands to ship. This change adds a per-name allow/deny set on CommandPermissionSet that lives alongside the bitmap, with deny taking precedence over any +@category that would otherwise allow the command. Custom RespCommand dispatch (CustomTxn, CustomRawStringCmd, CustomObjCmd, CustomProcedure) checks the deny set first, then the allow set, then falls back to the bitmap — matching the redis-server precedence model. Parser: unknown but syntactically valid tokens (ASCII alnum + .-_| and within 64 chars, starting alphanumeric) now route to the custom-name path instead of throwing. ACL file load is loose-by-default so an entry referencing a not-yet-loaded module doesn't break startup. Operators who want typo protection can enable acl-strict-custom-commands, which fails startup if any name is unresolved after modules load. SETUSER at runtime is always strict: a +name / -name op for a name not registered with CustomCommandManager throws ACLException. Swapping a name between allow and deny (including names loose-loaded from an ACL file) is accepted without re-checking registration. Per-user cap (MaxCustomCommandsPerUser = 512) bounds description growth and per-user memory. CAS loop on User._enabledCommands re-validates each retry, and the underlying FrozenSet rebuild is single-threaded inside the holder. New test suite test/standalone/Garnet.test.acl/Resp/ACL/CustomCommandACLTests.cs covers 36 cases across parser fallback, name validation, persistence round-trip, per-user cap, case-insensitive store, permission-set semantics, SETUSER strict validation (including the loose-loaded-name swap regression), and dispatch across all four custom command variants. The pre-existing BadInputMalformedStatement test's "+none" / "+invalid" sub-cases now load cleanly (they are syntactically valid custom names) and the test is updated to document the strict-mode path.
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds per-name ACL allow/deny rules for custom (extension) commands, including startup strictness and dispatch-time enforcement, and documents the behavior.
Changes:
- Extend ACL parsing and permission checks to support
+name/-namerules for custom commands with deny-precedence. - Add
AclStrictCustomCommandsoption to fail startup when ACL files reference unresolved custom command names (otherwise warn). - Add integration/unit tests and update docs to describe custom-command ACL semantics.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| website/docs/getting-started/configuration.md | Documents new AclStrictCustomCommands server option / CLI flag. |
| website/docs/commands/acl.md | Documents per-name ACL rules for custom (extension) commands and strictness behavior. |
| test/standalone/Garnet.test.acl/Resp/ACL/CustomCommandACLTests.cs | Adds comprehensive unit + integration coverage for per-name custom command ACLs and strict validation. |
| test/standalone/Garnet.test.acl/Resp/ACL/AclConfigurationFileTests.cs | Updates ACL-file parsing expectations now that unknown alphanumeric tokens can be treated as custom command names. |
| libs/server/Servers/GarnetServerOptions.cs | Adds AclStrictCustomCommands option to server options model. |
| libs/server/Resp/AdminCommands.cs | Routes custom command authorization through per-name allow/deny checks at dispatch. |
| libs/server/Resp/ACLCommands.cs | Makes ACL SETUSER validate newly-added custom command names against loaded modules. |
| libs/server/Custom/CustomCommandManager.cs | Tracks registered custom command names independent of RespCommandsInfo presence. |
| libs/server/ACL/User.cs | Adds per-name custom command allow/deny sets, cap enforcement, and public APIs. |
| libs/server/ACL/CommandPermissionSet.cs | Implements per-name custom allow/deny storage and evaluation with deny precedence. |
| libs/server/ACL/ACLParser.cs | Adds fallback parsing for unknown tokens as custom command names with syntax validation. |
| libs/host/defaults.conf | Adds default config entry for AclStrictCustomCommands. |
| libs/host/GarnetServer.cs | Validates unresolved custom-command ACL entries after module load; warns or fails in strict mode. |
| libs/host/Configuration/Options.cs | Adds CLI option mapping and wires it into GarnetServerOptions. |
Comment on lines
+371
to
+379
| /// <param name="customName">Custom command name. Normalized to uppercase; matching is case-insensitive.</param> | ||
| public void AddCustomCommand(string customName) | ||
| { | ||
| ArgumentNullException.ThrowIfNull(customName); | ||
|
|
||
| string normalized = customName.ToUpperInvariant(); | ||
|
|
||
| CommandPermissionSet prev = this._enabledCommands; | ||
| string descUpdate = $"+{customName.ToLowerInvariant()}"; |
Comment on lines
+326
to
+341
| foreach (var kv in acl.GetUserHandles()) | ||
| { | ||
| var u = kv.Value.User; | ||
| foreach (var name in u.CustomCommandsAllowed) | ||
| { | ||
| if (!customCommandManager.IsCustomCommandRegistered(name)) | ||
| { | ||
| unresolved.Add((kv.Key, name)); | ||
| } | ||
| } | ||
| foreach (var name in u.CustomCommandsDenied) | ||
| { | ||
| if (!customCommandManager.IsCustomCommandRegistered(name)) | ||
| { | ||
| unresolved.Add((kv.Key, name)); | ||
| } |
|
|
||
| In addition to built-in commands and `@category` rules, individual custom command names can be used directly as rules. For example, `+json.set` grants access to `JSON.SET` and `-json.get` denies access to `JSON.GET`. An explicit per-name deny takes precedence over a category-level allow, so `+@custom -json.set` allows every custom command except `JSON.SET`. Name matching is case-insensitive. | ||
|
|
||
| `ACL SETUSER` is strict at runtime: a `+name`/`-name` rule whose name is not registered with any loaded module is rejected. ACL file load is lenient by default so files are not brittle to module load order; set `acl-strict-custom-commands` to `true` if you want startup to fail when an ACL file references an unresolved custom command name. |
Comment on lines
+355
to
+360
| if (opts.AclStrictCustomCommands) | ||
| { | ||
| // Strict mode: fail closed so operators can't accidentally ship an ACL with typos | ||
| // that would silently match no command (and therefore deny by default at dispatch). | ||
| throw new GarnetException($"ACL strict mode: {unresolved.Count} unresolved (user, custom-command) entries in ACL rules. Disable acl-strict-custom-commands or load the appropriate module(s)."); | ||
| } |
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
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 #1821
Problem
Garnet's ACL system tracks per-command permissions in a fixed-size bitmap indexed by
RespCommand. That works well for built-in commands but breaks down for custom/extension commands: theirRespCommandIDs are assigned dynamically at module load and live outside the bitmap's range. The only way to grant access to an extension command today is the all-or-nothing+@customcategory. There is no way to write+json.setor-json.getfor a specific extension command, which blocks deployments that need fine-grained per-command RBAC.What this change does
Adds first-class per-name allow/deny entries for custom commands on the user's permission set, stored alongside the existing command bitmap. ACL rules now accept custom command names directly, and dispatch consults the per-name lists before falling back to the bitmap.
Highlights
CommandPermissionSetcarries twoFrozenSet<string>instances (_customAllowed,_customDenied) next to the existing bitmap. Lookup isOrdinalIgnoreCaseto matchCustomCommandManager's normalization. Custom-command dispatch checks deny first, then allow, then falls back to the bitmap.ACLParserroutes unknown but syntactically valid tokens (ASCII alphanumerics plus.,_,-,|, up to 64 chars, must start with an alphanumeric) to the custom-name path instead of throwing.UserexposesAddCustomCommand/RemoveCustomCommandwith the same CAS pattern used forAddCommand, plus a per-user cap (MaxCustomCommandsPerUser = 512) to bound description growth and per-user memory.acl-strict-custom-commandsmakes startup fail if any name is still unresolved after modules finish loading.ACL SETUSERat runtime is always strict: a new+name/-nameop must resolve againstCustomCommandManager, otherwise it throws. Names already on the user (e.g. loose-loaded from an ACL file) can be toggled allow/deny without re-checking registration.GarnetServerruns aValidateCustomCommandACLspass after modules load to surface the strict/lenient outcome to the operator.website/docs/commands/acl.mddocuments the new per-name SETUSER syntax and theacl-strict-custom-commandsflag;website/docs/getting-started/configuration.mdlists the new flag in the config table.Design notes
IsEquivalentTowas tightened to also compare per-name sets so description rationalization can't silently drop a-json.setentry.Testing
New test suite
test/standalone/Garnet.test.acl/Resp/ACL/CustomCommandACLTests.csadds 36 tests covering:DescribeUser.Allsentinel behavior including+@all -name, default fail-closed,IsEquivalentToconsiders custom sets.RespCommandsInfoaccepted, plus a regression test that allow/deny swap of loose-loaded names is still accepted after the strict check.RespCommandvariants (CustomTxn,CustomRawStringCmd,CustomObjCmd,CustomProcedure).ACL LISTround-trip.Existing test runs
Garnet.test.acl(excluding tests that require the native vector/range-index libraries): 451/451 on net8.0 and net10.0.Garnet.testcross-feature filter (ACL/Auth/Lua/Parse/Command/Permission/User): 789/789 on net10.0 (3 Azure-only tests skipped).Garnet.testbuilt-in custom-command tests: 7/7.Garnet.test.clusterACL/Auth/Custom subset: 9/9.Pre-existing test updated
AclConfigurationFileTests.BadInputMalformedStatementhad sub-cases asserting that+noneand+invalidwould throw on ACL file load. Both tokens are syntactically valid custom names under the new parser fallthrough and now load successfully under lenient mode. The sub-cases were updated to assert clean startup with the tokens present and to document that operators wanting typo protection can enableacl-strict-custom-commands. The remainingbadinput-in-user-name-position sub-case still throws and is unchanged.