gvsafronov/futriix
1
0
Fork 0
Code Issues Pull Requests Actions 8 Packages Projects Releases Wiki Activity

Improved handling of subcommands (don't allow ACL on first-arg of a sub-command) (#10147)

Browse Source 
Recently we added extensive support for sub-commands in for redis 7.0,
this meant that the old ACL mechanism for
sub-commands wasn't needed, or actually was improved (to handle both include
and exclude control, like for commands), but only for real sub-commands.
The old mechanism in ACL was renamed to first-arg, and was able to match the
first argument of any command (including sub-commands).
We now realized that we might wanna completely delete that first-arg feature some
day, so the first step was not to give it new capabilities in 7.0 and it didn't have before.

Changes:
1. ACL: Block the first-arg mechanism on subcommands (we keep if in non-subcommands
  for backward compatibility)
2. COMMAND: When looking up a command, insist the command name doesn't contain
  extra words. Example: When a user issues `GET key` we want `lookupCommand` to return
  `getCommand` but when if COMMAND calls `lookupCommand` with `get|key` we want it to fail.

Other changes:
1. ACLSetUser: prevent a redundant command lookup
This commit is contained in:
guybe7 2022-01-22 13:09:40 +01:00 committed by GitHub
parent 55c81f2cd3
commit a6fd2a46d1
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 58 additions and 22 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

24
src/acl.c
View File

@ -1110,13 +1110,20 @@ int ACLSetSelector(aclSelector *selector, const char* op, size_t oplen) {
struct redisCommand *cmd = ACLLookupCommand(copy);
/* Check if the command exists. We can't check the
* subcommand to see if it is valid. */
* first-arg to see if it is valid. */
if (cmd == NULL) {
zfree(copy);
errno = ENOENT;
return C_ERR;
}
/* We do not support allowing first-arg of a subcommand */
if (cmd->parent) {
zfree(copy);
errno = ECHILD;
return C_ERR;
}
/* The subcommand cannot be empty, so things like DEBUG|
* are syntax errors of course. */
if (strlen(sub) == 0) {
@ -1127,7 +1134,7 @@ int ACLSetSelector(aclSelector *selector, const char* op, size_t oplen) {
if (cmd->subcommands_dict) {
/* If user is trying to allow a valid subcommand we can just add its unique ID */
struct redisCommand *cmd = ACLLookupCommand(op+1);
cmd = ACLLookupCommand(op+1);
if (cmd == NULL) {
zfree(copy);
errno = ENOENT;
@ -1138,13 +1145,11 @@ int ACLSetSelector(aclSelector *selector, const char* op, size_t oplen) {
/* If user is trying to use the ACL mech to block SELECT except SELECT 0 or
* block DEBUG except DEBUG OBJECT (DEBUG subcommands are not considered
* subcommands for now) we use the allowed_firstargs mechanism. */
struct redisCommand *cmd = ACLLookupCommand(copy);
if (cmd == NULL) {
zfree(copy);
errno = ENOENT;
return C_ERR;
}
/* Add the first-arg to the list of valid ones. */
serverLog(LL_WARNING, "Deprecation warning: Allowing a first arg of an otherwise "
"blocked command is a misuse of ACL and may get disabled "
"in the future (offender: +%s)", op+1);
ACLAddAllowedFirstArg(selector,cmd->id,sub);
}
@ -1236,6 +1241,7 @@ int ACLSetSelector(aclSelector *selector, const char* op, size_t oplen) {
* almost surely an error on the user side.
* ENODEV: The password you are trying to remove from the user does not exist.
* EBADMSG: The hash you are trying to add is not a valid hash.
* ECHILD: Attempt to allow a specific first argument of a subcommand
*/
int ACLSetUser(user *u, const char *op, ssize_t oplen) {
if (oplen == -1) oplen = strlen(op);
@ -1360,6 +1366,8 @@ const char *ACLSetUserStringError(void) {
else if (errno == EALREADY)
errmsg = "Duplicate user found. A user can only be defined once in "
"config files";
else if (errno == ECHILD)
errmsg = "Allowing first-arg of a subcommand is not supported";
return errmsg;
}

24
src/server.c
View File

@ -2812,20 +2812,32 @@ int isContainerCommandBySds(sds s) {
return has_subcommands;
}
struct redisCommand *lookupCommandLogic(dict *commands, robj **argv, int argc) {
/* Look up a command by argv and argc
*
* If `strict` is not 0 we expect argc to be exact (i.e. argc==2
* for a subcommand and argc==1 for a top-level command)
* `strict` should be used every time we want to look up a command
* name (e.g. in COMMAND INFO) rather than to find the command
* a user requested to execute (in processCommand).
*/
struct redisCommand *lookupCommandLogic(dict *commands, robj **argv, int argc, int strict) {
struct redisCommand *base_cmd = dictFetchValue(commands, argv[0]->ptr);
int has_subcommands = base_cmd && base_cmd->subcommands_dict;
if (argc == 1 || !has_subcommands) {
if (strict && argc != 1)
return NULL;
/* Note: It is possible that base_cmd->proc==NULL (e.g. CONFIG) */
return base_cmd;
} else {
} else { /* argc > 1 && has_subcommands */
if (strict && argc != 2)
return NULL;
/* Note: Currently we support just one level of subcommands */
return dictFetchValue(base_cmd->subcommands_dict, argv[1]->ptr);
}
}
struct redisCommand *lookupCommand(robj **argv, int argc) {
return lookupCommandLogic(server.commands,argv,argc);
return lookupCommandLogic(server.commands,argv,argc,0);
}
struct redisCommand *lookupCommandBySdsLogic(dict *commands, sds s) {
@ -2846,7 +2858,7 @@ struct redisCommand *lookupCommandBySdsLogic(dict *commands, sds s) {
argv[j] = &objects[j];
}
struct redisCommand *cmd = lookupCommandLogic(commands,argv,argc);
struct redisCommand *cmd = lookupCommandLogic(commands,argv,argc,1);
sdsfreesplitres(strings,argc);
return cmd;
}
@ -2876,9 +2888,9 @@ struct redisCommand *lookupCommandByCString(const char *s) {
* rewriteClientCommandVector() in order to set client->cmd pointer
* correctly even if the command was renamed. */
struct redisCommand *lookupCommandOrOriginal(robj **argv ,int argc) {
struct redisCommand *cmd = lookupCommandLogic(server.commands, argv, argc);
struct redisCommand *cmd = lookupCommandLogic(server.commands, argv, argc, 0);
if (!cmd) cmd = lookupCommandLogic(server.orig_commands, argv, argc);
if (!cmd) cmd = lookupCommandLogic(server.orig_commands, argv, argc, 0);
return cmd;
}

27
tests/unit/acl.tcl
View File

@ -327,16 +327,27 @@ start_server {tags {"acl external:skip"}} {
set e
} {*NOPERM*config|set*}
test {ACLs can include a subcommand with a specific arg} {
test {ACLs cannot include a subcommand with a specific arg} {
r ACL setuser newuser +@all -config|get
r ACL setuser newuser +config|get|appendonly
set cmdstr [dict get [r ACL getuser newuser] commands]
assert_match {*-config|get*} $cmdstr
assert_match {*+config|get|appendonly*} $cmdstr
r CONFIG GET appendonly; # Should not fail
catch {r CONFIG GET loglevel} e
catch { r ACL setuser newuser +config|get|appendonly} e
set e
} {*NOPERM*config|get*}
} {*Allowing first-arg of a subcommand is not supported*}
test {ACLs cannot exclude or include a container commands with a specific arg} {
r ACL setuser newuser +@all +config|get
catch { r ACL setuser newuser +@all +config|asdf} e
assert_match "*Unknown command or category name in ACL*" $e
catch { r ACL setuser newuser +@all -config|asdf} e
assert_match "*Unknown command or category name in ACL*" $e
} {}
test {ACLs cannot exclude or include a container command with two args} {
r ACL setuser newuser +@all +config|get
catch { r ACL setuser newuser +@all +get|key1|key2} e
assert_match "*Unknown command or category name in ACL*" $e
catch { r ACL setuser newuser +@all -get|key1|key2} e
assert_match "*Unknown command or category name in ACL*" $e
} {}
test {ACLs including of a type includes also subcommands} {
r ACL setuser newuser -@all +acl +@stream

5
tests/unit/introspection-2.tcl
View File

@ -110,4 +110,9 @@ start_server {tags {"introspection"}} {
set reply [r command list filterby pattern pf*]
assert_equal [lsort $reply] {pfadd pfcount pfdebug pfmerge pfselftest}
}
test {COMMAND INFO of invalid subcommands} {
assert_equal {{}} [r command info get|key]
assert_equal {{}} [r command info config|get|key]
}
}