From c337c0a8a49d7cb64617b0a414d05b31425666f7 Mon Sep 17 00:00:00 2001 From: Madelyn Olson <34459052+madolson@users.noreply.github.com> Date: Thu, 3 Nov 2022 10:14:56 -0700 Subject: Retain ACL categories used to generate ACL for displaying them later (#11224) Retain ACL categories used to generate ACL for displaying them later --- src/acl.c | 229 ++++++++++++++++++++++++++++---------------------------------- 1 file changed, 102 insertions(+), 127 deletions(-) (limited to 'src/acl.c') diff --git a/src/acl.c b/src/acl.c index 782254eb9..ba6b20716 100644 --- a/src/acl.c +++ b/src/acl.c @@ -135,6 +135,8 @@ typedef struct { field is NULL the user cannot mention any channel in a `PUBLISH` or [P][UNSUBSCRIBE] command, unless the flag ALLCHANNELS is set in the user. */ + sds command_rules; /* A string representation of the ordered categories and commands, this + * is used to regenerate the original ACL string for display. */ } aclSelector; void ACLResetFirstArgsForCommand(aclSelector *selector, unsigned long id); @@ -313,6 +315,7 @@ aclSelector *ACLCreateSelector(int flags) { selector->patterns = listCreate(); selector->channels = listCreate(); selector->allowed_firstargs = NULL; + selector->command_rules = sdsempty(); listSetMatchMethod(selector->patterns,ACLListMatchKeyPattern); listSetFreeMethod(selector->patterns,ACLListFreeKeyPattern); @@ -329,6 +332,7 @@ aclSelector *ACLCreateSelector(int flags) { void ACLFreeSelector(aclSelector *selector) { listRelease(selector->patterns); listRelease(selector->channels); + sdsfree(selector->command_rules); ACLResetFirstArgs(selector); zfree(selector); } @@ -339,6 +343,7 @@ aclSelector *ACLCopySelector(aclSelector *src) { dst->flags = src->flags; dst->patterns = listDup(src->patterns); dst->channels = listDup(src->channels); + dst->command_rules = sdsdup(src->command_rules); memcpy(dst->allowed_commands,src->allowed_commands, sizeof(dst->allowed_commands)); dst->allowed_firstargs = NULL; @@ -537,6 +542,63 @@ void ACLSetSelectorCommandBit(aclSelector *selector, unsigned long id, int value } } +/* Remove a rule from the retained command rules. Always match rules + * verbatim, but also remove subcommand rules if we are adding or removing the + * entire command. */ +void ACLSelectorRemoveCommandRule(aclSelector *selector, sds new_rule) { + size_t new_len = sdslen(new_rule); + char *existing_rule = selector->command_rules; + + /* Loop over the existing rules, trying to find a rule that "matches" + * the new rule. If we find a match, then remove the command from the string by + * copying the later rules over it. */ + while(existing_rule[0]) { + /* The first character of the rule is +/-, which we don't need to compare. */ + char *copy_position = existing_rule; + existing_rule += 1; + + /* Assume a trailing space after a command is part of the command, like '+get ', so trim it + * as well if the command is removed. */ + char *rule_end = strchr(existing_rule, ' '); + if (!rule_end) { + /* This is the last rule, so it it to the end of the string. */ + rule_end = existing_rule + strlen(existing_rule); + + /* This approach can leave a trailing space if the last rule is removed, + * but only if it's not the first rule, so handle that case. */ + if (copy_position != selector->command_rules) copy_position -= 1; + } + char *copy_end = rule_end; + if (*copy_end == ' ') copy_end++; + + /* Exact match or the rule we are comparing is a subcommand denoted by '|' */ + size_t existing_len = rule_end - existing_rule; + if (!memcmp(existing_rule, new_rule, min(existing_len, new_len))) { + if ((existing_len == new_len) || (existing_len > new_len && (existing_rule[new_len]) == '|')) { + /* Copy the remaining rules starting at the next rule to replace the rule to be + * deleted, including the terminating NULL character. */ + memmove(copy_position, copy_end, strlen(copy_end) + 1); + } + } + existing_rule = copy_end; + } + + /* There is now extra padding at the end of the rules, so clean that up. */ + sdsupdatelen(selector->command_rules); +} + +/* This function is resopnsible for updating the command_rules struct so that relative ordering of + * commands and categories is maintained and can be reproduced without loss. */ +void ACLUpdateCommandRules(aclSelector *selector, const char *rule, int allow) { + sds new_rule = sdsnew(rule); + sdstolower(new_rule); + + ACLSelectorRemoveCommandRule(selector, new_rule); + if (sdslen(selector->command_rules)) selector->command_rules = sdscat(selector->command_rules, " "); + selector->command_rules = sdscatfmt(selector->command_rules, allow ? "+%S" : "-%S", new_rule); + sdsfree(new_rule); +} + /* This function is used to allow/block a specific command. * Allowing/blocking a container command also applies for its subcommands */ void ACLChangeSelectorPerm(aclSelector *selector, struct redisCommand *cmd, int allow) { @@ -554,7 +616,13 @@ void ACLChangeSelectorPerm(aclSelector *selector, struct redisCommand *cmd, int } } -void ACLSetSelectorCommandBitsForCategoryLogic(dict *commands, aclSelector *selector, uint64_t cflag, int value) { +/* This is like ACLSetSelectorCommandBit(), but instead of setting the specified + * ID, it will check all the commands in the category specified as argument, + * and will set all the bits corresponding to such commands to the specified + * value. Since the category passed by the user may be non existing, the + * function returns C_ERR if the category was not found, or C_OK if it was + * found and the operation was performed. */ +void ACLSetSelectorCommandBitsForCategory(dict *commands, aclSelector *selector, uint64_t cflag, int value) { dictIterator *di = dictGetIterator(commands); dictEntry *de; while ((de = dictNext(di)) != NULL) { @@ -564,22 +632,20 @@ void ACLSetSelectorCommandBitsForCategoryLogic(dict *commands, aclSelector *sele ACLChangeSelectorPerm(selector,cmd,value); } if (cmd->subcommands_dict) { - ACLSetSelectorCommandBitsForCategoryLogic(cmd->subcommands_dict, selector, cflag, value); + ACLSetSelectorCommandBitsForCategory(cmd->subcommands_dict, selector, cflag, value); } } dictReleaseIterator(di); } -/* This is like ACLSetSelectorCommandBit(), but instead of setting the specified - * ID, it will check all the commands in the category specified as argument, - * and will set all the bits corresponding to such commands to the specified - * value. Since the category passed by the user may be non existing, the - * function returns C_ERR if the category was not found, or C_OK if it was - * found and the operation was performed. */ -int ACLSetSelectorCommandBitsForCategory(aclSelector *selector, const char *category, int value) { - uint64_t cflag = ACLGetCommandCategoryFlagByName(category); +int ACLSetSelectorCategory(aclSelector *selector, const char *category, int allow) { + uint64_t cflag = ACLGetCommandCategoryFlagByName(category + 1); if (!cflag) return C_ERR; - ACLSetSelectorCommandBitsForCategoryLogic(server.orig_commands, selector, cflag, value); + + ACLUpdateCommandRules(selector, category, allow); + + /* Set the actual command bits on the selector. */ + ACLSetSelectorCommandBitsForCategory(server.orig_commands, selector, cflag, allow); return C_OK; } @@ -616,41 +682,6 @@ int ACLCountCategoryBitsForSelector(aclSelector *selector, unsigned long *on, un return C_OK; } -sds ACLDescribeSelectorCommandRulesSingleCommands(aclSelector *selector, aclSelector *fake_selector, - sds rules, dict *commands) { - dictIterator *di = dictGetIterator(commands); - dictEntry *de; - while ((de = dictNext(di)) != NULL) { - struct redisCommand *cmd = dictGetVal(de); - int userbit = ACLGetSelectorCommandBit(selector,cmd->id); - int fakebit = ACLGetSelectorCommandBit(fake_selector,cmd->id); - if (userbit != fakebit) { - rules = sdscatlen(rules, userbit ? "+" : "-", 1); - rules = sdscatsds(rules,cmd->fullname); - rules = sdscatlen(rules," ",1); - ACLChangeSelectorPerm(fake_selector,cmd,userbit); - } - - if (cmd->subcommands_dict) - rules = ACLDescribeSelectorCommandRulesSingleCommands(selector,fake_selector,rules,cmd->subcommands_dict); - - /* Emit the first-args if there are any. */ - if (userbit == 0 && selector->allowed_firstargs && - selector->allowed_firstargs[cmd->id]) - { - for (int j = 0; selector->allowed_firstargs[cmd->id][j]; j++) { - rules = sdscatlen(rules,"+",1); - rules = sdscatsds(rules,cmd->fullname); - rules = sdscatlen(rules,"|",1); - rules = sdscatsds(rules,selector->allowed_firstargs[cmd->id][j]); - rules = sdscatlen(rules," ",1); - } - } - } - dictReleaseIterator(di); - return rules; -} - /* This function returns an SDS string representing the specified selector ACL * rules related to command execution, in the same format you could set them * back using ACL SETUSER. The function will return just the set of rules needed @@ -660,99 +691,38 @@ sds ACLDescribeSelectorCommandRulesSingleCommands(aclSelector *selector, aclSele * needed, by the other rules needed to narrow or extend what the user can do. */ sds ACLDescribeSelectorCommandRules(aclSelector *selector) { sds rules = sdsempty(); - int additive; /* If true we start from -@all and add, otherwise if - false we start from +@all and remove. */ - - /* This code is based on a trick: as we generate the rules, we apply - * them to a fake user, so that as we go we still know what are the - * bit differences we should try to address by emitting more rules. */ - aclSelector fs = {0}; - aclSelector *fake_selector = &fs; - - /* Here we want to understand if we should start with +@all and remove - * the commands corresponding to the bits that are not set in the user - * commands bitmap, or the contrary. Note that semantically the two are - * different. For instance starting with +@all and subtracting, the user + + /* We use this fake selector as a "sanity" check to make sure the rules + * we generate have the same bitmap as those on the current selector. */ + aclSelector *fake_selector = ACLCreateSelector(0); + + /* Here we want to understand if we should start with +@all or -@all. + * Note that when starting with +@all and subtracting, the user * will be able to execute future commands, while -@all and adding will just * allow the user the run the selected commands and/or categories. * How do we test for that? We use the trick of a reserved command ID bit * that is set only by +@all (and its alias "allcommands"). */ if (ACLSelectorCanExecuteFutureCommands(selector)) { - additive = 0; rules = sdscat(rules,"+@all "); ACLSetSelector(fake_selector,"+@all",-1); } else { - additive = 1; rules = sdscat(rules,"-@all "); ACLSetSelector(fake_selector,"-@all",-1); } - /* Attempt to find a good approximation for categories and commands - * based on the current bits used, by looping over the category list - * and applying the best fit each time. Often a set of categories will not - * perfectly match the set of commands into it, so at the end we do a - * final pass adding/removing the single commands needed to make the bitmap - * exactly match. A temp user is maintained to keep track of categories - * already applied. */ - aclSelector ts = {0}; - aclSelector *temp_selector = &ts; - - /* Keep track of the categories that have been applied, to prevent - * applying them twice. */ - char applied[sizeof(ACLCommandCategories)/sizeof(ACLCommandCategories[0])]; - memset(applied, 0, sizeof(applied)); - - memcpy(temp_selector->allowed_commands, - selector->allowed_commands, - sizeof(selector->allowed_commands)); - while (1) { - int best = -1; - unsigned long mindiff = INT_MAX, maxsame = 0; - for (int j = 0; ACLCommandCategories[j].flag != 0; j++) { - if (applied[j]) continue; - - unsigned long on, off, diff, same; - ACLCountCategoryBitsForSelector(temp_selector,&on,&off,ACLCommandCategories[j].name); - /* Check if the current category is the best this loop: - * * It has more commands in common with the user than commands - * that are different. - * AND EITHER - * * It has the fewest number of differences - * than the best match we have found so far. - * * OR it matches the fewest number of differences - * that we've seen but it has more in common. */ - diff = additive ? off : on; - same = additive ? on : off; - if (same > diff && - ((diff < mindiff) || (diff == mindiff && same > maxsame))) - { - best = j; - mindiff = diff; - maxsame = same; - } - } - - /* We didn't find a match */ - if (best == -1) break; - - sds op = sdsnewlen(additive ? "+@" : "-@", 2); - op = sdscat(op,ACLCommandCategories[best].name); - ACLSetSelector(fake_selector,op,-1); - - sds invop = sdsnewlen(additive ? "-@" : "+@", 2); - invop = sdscat(invop,ACLCommandCategories[best].name); - ACLSetSelector(temp_selector,invop,-1); - - rules = sdscatsds(rules,op); - rules = sdscatlen(rules," ",1); - sdsfree(op); - sdsfree(invop); + /* Apply all of the commands and categories to the fake selector. */ + int argc = 0; + sds *argv = sdssplitargs(selector->command_rules, &argc); + serverAssert(argv != NULL); - applied[best] = 1; + for(int i = 0; i < argc; i++) { + int res = ACLSetSelector(fake_selector, argv[i], -1); + serverAssert(res == C_OK); } - - /* Fix the final ACLs with single commands differences. */ - rules = ACLDescribeSelectorCommandRulesSingleCommands(selector,fake_selector,rules,server.orig_commands); + if (sdslen(selector->command_rules)) { + rules = sdscatfmt(rules, "%S ", selector->command_rules); + } + sdsfreesplitres(argv, argc); /* Trim the final useless space. */ sdsrange(rules,0,-2); @@ -770,6 +740,7 @@ sds ACLDescribeSelectorCommandRules(aclSelector *selector) { rules); serverPanic("No bitmap match in ACLDescribeSelectorCommandRules()"); } + ACLFreeSelector(fake_selector); return rules; } @@ -1017,12 +988,14 @@ int ACLSetSelector(aclSelector *selector, const char* op, size_t oplen) { { memset(selector->allowed_commands,255,sizeof(selector->allowed_commands)); selector->flags |= SELECTOR_FLAG_ALLCOMMANDS; + sdsclear(selector->command_rules); ACLResetFirstArgs(selector); } else if (!strcasecmp(op,"nocommands") || !strcasecmp(op,"-@all")) { memset(selector->allowed_commands,0,sizeof(selector->allowed_commands)); selector->flags &= ~SELECTOR_FLAG_ALLCOMMANDS; + sdsclear(selector->command_rules); ACLResetFirstArgs(selector); } else if (op[0] == '~' || op[0] == '%') { if (selector->flags & SELECTOR_FLAG_ALLKEYS) { @@ -1088,6 +1061,7 @@ int ACLSetSelector(aclSelector *selector, const char* op, size_t oplen) { return C_ERR; } ACLChangeSelectorPerm(selector,cmd,1); + ACLUpdateCommandRules(selector,cmd->fullname,1); } else { /* Split the command and subcommand parts. */ char *copy = zstrdup(op+1); @@ -1140,7 +1114,7 @@ int ACLSetSelector(aclSelector *selector, const char* op, size_t oplen) { "in the future (offender: +%s)", op+1); ACLAddAllowedFirstArg(selector,cmd->id,sub); } - + ACLUpdateCommandRules(selector,op+1,1); zfree(copy); } } else if (op[0] == '-' && op[1] != '@') { @@ -1150,9 +1124,10 @@ int ACLSetSelector(aclSelector *selector, const char* op, size_t oplen) { return C_ERR; } ACLChangeSelectorPerm(selector,cmd,0); + ACLUpdateCommandRules(selector,cmd->fullname,0); } else if ((op[0] == '+' || op[0] == '-') && op[1] == '@') { int bitval = op[0] == '+' ? 1 : 0; - if (ACLSetSelectorCommandBitsForCategory(selector,op+2,bitval) == C_ERR) { + if (ACLSetSelectorCategory(selector,op+1,bitval) == C_ERR) { errno = ENOENT; return C_ERR; } -- cgit v1.2.1