-
Notifications
You must be signed in to change notification settings - Fork 664
Add per-name ACL support for custom (extension) commands #1822
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
| // Licensed under the MIT license. | ||
|
|
||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Diagnostics; | ||
| using System.IO; | ||
| using System.Linq; | ||
|
|
@@ -302,6 +303,61 @@ private void InitializeServer() | |
| servers[i].Register(WireFormat.ASCII, Provider); | ||
|
|
||
| LoadModules(customCommandManager); | ||
|
|
||
| // ACL rules are parsed before modules load, so per-name custom-command entries land | ||
| // on users unresolved. Sweep them now against the registered modules. | ||
| ValidateCustomCommandACLs(customCommandManager); | ||
| } | ||
|
|
||
| private void ValidateCustomCommandACLs(CustomCommandManager customCommandManager) | ||
| { | ||
| if (opts.AuthSettings is not AclAuthenticationSettings) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| var acl = storeWrapper.accessControlList; | ||
| if (acl == null) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| var unresolved = new List<(string user, string name)>(); | ||
| 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)); | ||
| } | ||
|
kevin-montrose marked this conversation as resolved.
|
||
| } | ||
| } | ||
|
|
||
| if (unresolved.Count == 0) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| foreach (var (user, name) in unresolved) | ||
| { | ||
| logger?.LogWarning("ACL rule references custom command '{name}' for user '{user}' which is not registered with any loaded module", name, user); | ||
| } | ||
|
|
||
| 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)."); | ||
| } | ||
|
Comment on lines
+355
to
+360
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with this comment - add a test to cover this being thrown when expected. |
||
| } | ||
|
|
||
| private GarnetDatabase CreateDatabase(int dbId, GarnetServerOptions serverOptions, ClusterFactory clusterFactory, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -108,6 +108,9 @@ | |
| /* External ACL user file. */ | ||
| "AclFile" : null, | ||
|
|
||
| /* Refuse to start when an ACL rule references a custom (extension) command name that no loaded module has registered. If false (default), unresolved names are loaded as-is and logged as warnings. */ | ||
| "AclStrictCustomCommands" : false, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd argue this should be strict by default, preserving older behavior and also defaulting to safer behavior. |
||
|
|
||
| /* The authority of AAD authentication. */ | ||
| "AadAuthority" : "https://login.microsoftonline.com", | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -216,18 +216,33 @@ public static void ApplyACLOpToUser(ref User user, string op) | |
| // Individual commands or command|subcommand pairs | ||
| string commandName = op.Substring(1); | ||
|
|
||
| if (!TryParseCommandForAcl(commandName, out RespCommand command)) | ||
| if (TryParseCommandForAcl(commandName, out RespCommand command)) | ||
| { | ||
| throw new AclCommandDoesNotExistException(commandName); | ||
| if (op[0] == '-') | ||
| { | ||
| user.RemoveCommand(command); | ||
| } | ||
| else | ||
| { | ||
| user.AddCommand(command); | ||
| } | ||
| } | ||
|
|
||
| if (op[0] == '-') | ||
| else if (IsValidCustomCommandName(commandName)) | ||
| { | ||
| user.RemoveCommand(command); | ||
| // Modules may not be loaded yet (ACL file is parsed before LoadModules), so we | ||
| // store the name on the user and resolve it later (startup pass, SETUSER, dispatch). | ||
| if (op[0] == '-') | ||
| { | ||
| user.RemoveCustomCommand(commandName); | ||
| } | ||
| else | ||
| { | ||
| user.AddCustomCommand(commandName); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| user.AddCommand(command); | ||
| throw new AclCommandDoesNotExistException(commandName); | ||
| } | ||
| } | ||
| else if (op.Equals("~*", StringComparison.Ordinal) || op.Equals("ALLKEYS", StringComparison.OrdinalIgnoreCase)) | ||
|
|
@@ -304,6 +319,49 @@ static bool IsInvalidCommandToAcl(RespCommand command) | |
| => command == RespCommand.INVALID || command == RespCommand.NONE || command.NormalizeForACLs() != command; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Maximum length (in chars) of a custom command name accepted in an ACL rule. | ||
| /// </summary> | ||
| internal const int MaxCustomCommandNameLength = 64; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: pull this up to the top of the type |
||
|
|
||
| /// <summary> | ||
| /// Returns true if <paramref name="name"/> is a syntactically valid custom command name. | ||
| /// Strict validation prevents the unknown-name fallback from accepting RESP-meta bytes | ||
| /// or whitespace-bearing junk. Allowed: ASCII letters/digits and '.', '_', '-', '|'; | ||
| /// first character must be alphanumeric. | ||
| /// </summary> | ||
| internal static bool IsValidCustomCommandName(string name) | ||
| { | ||
| if (string.IsNullOrEmpty(name) || name.Length > MaxCustomCommandNameLength) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| char first = name[0]; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Loop like this is suboptimal - we can use some Something like: private static SearchValues<char> LegalFirstChars { get; } = SearchValues.Create("ABCDEFGHIJKLMNOPQRSTUVWXYZabdefghijklmnopqrstuvwxyz0123456789");
private static SearchValues<char> LegalRestChars { get; } = SearchValues.Create("ABCDEFGHIJKLMNOPQRSTUVWXYZabdefghijklmnopqrstuvwxyz0123456789._-|");
static bool IsValidCustomCommandName(string name)
{
if (string.IsNullOrEmpty(name) || name.Length > MaxCustomCommandNameLength)
{
return false;
}
var nameSpan = name.AsSpan();
return LegalFirstChars.Contains(nameSpan[0]) && !nameSpan[1..].ContainsAnyExcept(LegalRestChars);
} |
||
| bool firstOk = (first >= 'A' && first <= 'Z') | ||
| || (first >= 'a' && first <= 'z') | ||
| || (first >= '0' && first <= '9'); | ||
| if (!firstOk) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| foreach (char c in name) | ||
| { | ||
| bool ok = (c >= 'A' && c <= 'Z') | ||
| || (c >= 'a' && c <= 'z') | ||
| || (c >= '0' && c <= '9') | ||
| // '|' is permitted so custom names can mirror built-in subcommand notation (e.g. CLIENT|GETNAME). | ||
| || c == '.' || c == '_' || c == '-' || c == '|'; | ||
| if (!ok) | ||
| { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Lookup the <see cref="RespAclCategories"/> by equivalent string. | ||
| /// </summary> | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to see tests for this option - look at
GarnetServerConfigTests.csfor examples (things likeEnableVectorSetPrevieware the minimum). We've had problems in the past of configs "working" because the defaults are reasonable, but not actually being usable in practice.