From 7519960527d8749edad6f0aa296f829fbbcf4bdc Mon Sep 17 00:00:00 2001 From: Roshan Khatri <117414976+roshkhatri@users.noreply.github.com> Date: Wed, 30 Aug 2023 13:01:24 -0700 Subject: [PATCH] Allows modules to declare new ACL categories. (#12486) This PR adds a new Module API int RM_AddACLCategory(RedisModuleCtx *ctx, const char *category_name) to add a new ACL command category. Here, we initialize the ACLCommandCategories array by allocating space for 64 categories and duplicate the 21 default categories from the predefined array 'ACLDefaultCommandCategories' into the ACLCommandCategories array while ACL initialization. Valid ACL category names can only contain alphanumeric characters, underscores, and dashes. The API when called, checks for the onload flag, category name validity, and for duplicate category name if present. If the conditions are satisfied, the API adds the new category to the trailing end of the ACLCommandCategories array and assigns the acl_categories flag bit according to the index at which the category is added. If any error is encountered the errno is set accordingly by the API. --------- Co-authored-by: Madelyn Olson --- src/acl.c | 55 ++++++++++++++++++++++++- src/module.c | 67 ++++++++++++++++++++++++++++--- src/redismodule.h | 2 + src/server.h | 3 ++ tests/modules/aclcheck.c | 53 ++++++++++++++++++++++-- tests/unit/moduleapi/aclcheck.tcl | 27 +++++++++++-- 6 files changed, 193 insertions(+), 14 deletions(-) diff --git a/src/acl.c b/src/acl.c index 5fd956d23..900746ece 100644 --- a/src/acl.c +++ b/src/acl.c @@ -59,10 +59,12 @@ static rax *commandId = NULL; /* Command name to id mapping */ static unsigned long nextid = 0; /* Next command id that has not been assigned */ +#define ACL_MAX_CATEGORIES 64 /* Maximum number of command categories */ + struct ACLCategoryItem { - const char *name; + char *name; uint64_t flag; -} ACLCommandCategories[] = { /* See redis.conf for details on each category. */ +} ACLDefaultCommandCategories[] = { /* See redis.conf for details on each category. */ {"keyspace", ACL_CATEGORY_KEYSPACE}, {"read", ACL_CATEGORY_READ}, {"write", ACL_CATEGORY_WRITE}, @@ -87,6 +89,54 @@ struct ACLCategoryItem { {NULL,0} /* Terminator. */ }; +static struct ACLCategoryItem *ACLCommandCategories = NULL; +static size_t nextCommandCategory = 0; /* Index of the next command category to be added */ + +/* Implements the ability to add to the list of ACL categories at runtime. Since each ACL category + * also requires a bit in the acl_categories flag, there is a limit to the number that can be added. + * The new ACL categories occupy the remaining bits of acl_categories flag, other than the bits + * occupied by the default ACL command categories. + * + * The optional `flag` argument allows the assignment of the `acl_categories` flag bit to the ACL category. + * When adding a new category, except for the default ACL command categories, this arguments should be `0` + * to allow the function to assign the next available `acl_categories` flag bit to the new ACL category. + * + * returns 1 -> Added, 0 -> Failed (out of space) + * + * This function is present here to gain access to the ACLCommandCategories array and add a new ACL category. + */ +int ACLAddCommandCategory(const char *name, uint64_t flag) { + if (nextCommandCategory >= ACL_MAX_CATEGORIES) return 0; + ACLCommandCategories[nextCommandCategory].name = zstrdup(name); + ACLCommandCategories[nextCommandCategory].flag = flag != 0 ? flag : (1ULL<module->onload) { + errno = EINVAL; + return REDISMODULE_ERR; + } + + if (moduleVerifyResourceName(name) == REDISMODULE_ERR) { + errno = EINVAL; + return REDISMODULE_ERR; + } + + if (ACLGetCommandCategoryFlagByName(name)) { + errno = EBUSY; + return REDISMODULE_ERR; + } + + if (ACLAddCommandCategory(name, 0)) { + ctx->module->num_acl_categories_added++; + return REDISMODULE_OK; + } else { + errno = ENOMEM; + return REDISMODULE_ERR; + } +} + /* Helper for categoryFlagsFromString(). Attempts to find an acl flag representing the provided flag string * and adds that flag to acl_categories_flags if a match is found. * @@ -2252,6 +2295,7 @@ void RM_SetModuleAttribs(RedisModuleCtx *ctx, const char *name, int ver, int api module->loadmod = NULL; module->num_commands_with_acl_categories = 0; module->onload = 1; + module->num_acl_categories_added = 0; ctx->module = module; } @@ -11937,6 +11981,13 @@ void moduleRemoveConfigs(RedisModule *module) { } } +/* Remove ACL categories added by the module when it fails to load. */ +void moduleRemoveCateogires(RedisModule *module) { + if (module->num_acl_categories_added) { + ACLCleanupCategoriesOnFailure(module->num_acl_categories_added); + } +} + /* Load all the modules in the server.loadmodule_queue list, which is * populated by `loadmodule` directives in the configuration file. * We can't load modules directly when processing the configuration file @@ -12152,6 +12203,7 @@ int moduleLoad(const char *path, void **module_argv, int module_argc, int is_loa moduleUnregisterCommands(ctx.module); moduleUnregisterSharedAPI(ctx.module); moduleUnregisterUsedAPI(ctx.module); + moduleRemoveCateogires(ctx.module); moduleRemoveConfigs(ctx.module); moduleUnregisterAuthCBs(ctx.module); moduleFreeModuleStructure(ctx.module); @@ -12424,12 +12476,14 @@ int moduleVerifyConfigFlags(unsigned int flags, configType type) { return REDISMODULE_OK; } -int moduleVerifyConfigName(sds name) { - if (sdslen(name) == 0) { - serverLogRaw(LL_WARNING, "Module config names cannot be an empty string."); +/* Verify a module resource or name has only alphanumeric characters, underscores + * or dashes. */ +int moduleVerifyResourceName(const char *name) { + if (name[0] == '\0') { return REDISMODULE_ERR; } - for (size_t i = 0 ; i < sdslen(name) ; ++i) { + + for (size_t i = 0; name[i] != '\0'; i++) { char curr_char = name[i]; if ((curr_char >= 'a' && curr_char <= 'z') || (curr_char >= 'A' && curr_char <= 'Z') || @@ -12438,7 +12492,7 @@ int moduleVerifyConfigName(sds name) { { continue; } - serverLog(LL_WARNING, "Invalid character %c in Module Config name %s.", curr_char, name); + serverLog(LL_WARNING, "Invalid character %c in Module resource name %s.", curr_char, name); return REDISMODULE_ERR; } return REDISMODULE_OK; @@ -12597,7 +12651,7 @@ int moduleConfigValidityCheck(RedisModule *module, sds name, unsigned int flags, errno = EBUSY; return REDISMODULE_ERR; } - if (moduleVerifyConfigFlags(flags, type) || moduleVerifyConfigName(name)) { + if (moduleVerifyConfigFlags(flags, type) || moduleVerifyResourceName(name)) { errno = EINVAL; return REDISMODULE_ERR; } @@ -13505,6 +13559,7 @@ void moduleRegisterCoreAPI(void) { REGISTER_API(CreateSubcommand); REGISTER_API(SetCommandInfo); REGISTER_API(SetCommandACLCategories); + REGISTER_API(AddACLCategory); REGISTER_API(SetModuleAttribs); REGISTER_API(IsModuleNameBusy); REGISTER_API(WrongArity); diff --git a/src/redismodule.h b/src/redismodule.h index 4378126e2..de533d049 100644 --- a/src/redismodule.h +++ b/src/redismodule.h @@ -968,6 +968,7 @@ REDISMODULE_API RedisModuleCommand *(*RedisModule_GetCommand)(RedisModuleCtx *ct REDISMODULE_API int (*RedisModule_CreateSubcommand)(RedisModuleCommand *parent, const char *name, RedisModuleCmdFunc cmdfunc, const char *strflags, int firstkey, int lastkey, int keystep) REDISMODULE_ATTR; REDISMODULE_API int (*RedisModule_SetCommandInfo)(RedisModuleCommand *command, const RedisModuleCommandInfo *info) REDISMODULE_ATTR; REDISMODULE_API int (*RedisModule_SetCommandACLCategories)(RedisModuleCommand *command, const char *ctgrsflags) REDISMODULE_ATTR; +REDISMODULE_API int (*RedisModule_AddACLCategory)(RedisModuleCtx *ctx, const char *name) REDISMODULE_ATTR; REDISMODULE_API void (*RedisModule_SetModuleAttribs)(RedisModuleCtx *ctx, const char *name, int ver, int apiver) REDISMODULE_ATTR; REDISMODULE_API int (*RedisModule_IsModuleNameBusy)(const char *name) REDISMODULE_ATTR; REDISMODULE_API int (*RedisModule_WrongArity)(RedisModuleCtx *ctx) REDISMODULE_ATTR; @@ -1329,6 +1330,7 @@ static int RedisModule_Init(RedisModuleCtx *ctx, const char *name, int ver, int REDISMODULE_GET_API(CreateSubcommand); REDISMODULE_GET_API(SetCommandInfo); REDISMODULE_GET_API(SetCommandACLCategories); + REDISMODULE_GET_API(AddACLCategory); REDISMODULE_GET_API(SetModuleAttribs); REDISMODULE_GET_API(IsModuleNameBusy); REDISMODULE_GET_API(WrongArity); diff --git a/src/server.h b/src/server.h index 756435664..939da4365 100644 --- a/src/server.h +++ b/src/server.h @@ -821,6 +821,7 @@ struct RedisModule { struct moduleLoadQueueEntry *loadmod; /* Module load arguments for config rewrite. */ int num_commands_with_acl_categories; /* Number of commands in this module included in acl categories */ int onload; /* Flag to identify if the call is being made from Onload (0 or 1) */ + size_t num_acl_categories_added; /* Number of acl categories added by this module. */ }; typedef struct RedisModule RedisModule; @@ -2927,6 +2928,8 @@ int ACLCheckAllPerm(client *c, int *idxptr); int ACLSetUser(user *u, const char *op, ssize_t oplen); sds ACLStringSetUser(user *u, sds username, sds *argv, int argc); uint64_t ACLGetCommandCategoryFlagByName(const char *name); +int ACLAddCommandCategory(const char *name, uint64_t flag); +void ACLCleanupCategoriesOnFailure(size_t num_acl_categories_added); int ACLAppendUserForLoading(sds *argv, int argc, int *argc_err); const char *ACLSetUserStringError(void); int ACLLoadConfiguredUsers(void); diff --git a/tests/modules/aclcheck.c b/tests/modules/aclcheck.c index 09b525cc5..b74651804 100644 --- a/tests/modules/aclcheck.c +++ b/tests/modules/aclcheck.c @@ -197,6 +197,9 @@ int commandBlockCheck(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { int result = RedisModule_CreateCommand(ctx,"command.that.should.fail", module_test_acl_category, "", 0, 0, 0); response_ok |= (result == REDISMODULE_OK); + result = RedisModule_AddACLCategory(ctx,"blockedcategory"); + response_ok |= (result == REDISMODULE_OK); + RedisModuleCommand *parent = RedisModule_GetCommand(ctx,"block.commands.outside.onload"); result = RedisModule_SetCommandACLCategories(parent, "write"); response_ok |= (result == REDISMODULE_OK); @@ -204,7 +207,8 @@ int commandBlockCheck(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { result = RedisModule_CreateSubcommand(parent,"subcommand.that.should.fail",module_test_acl_category,"",0,0,0); response_ok |= (result == REDISMODULE_OK); - /* This validates that it's not possible to create commands outside OnLoad, + /* This validates that it's not possible to create commands or add + * a new ACL Category outside OnLoad function. * thus returns an error if they succeed. */ if (response_ok) { RedisModule_ReplyWithError(ctx, "UNEXPECTEDOK"); @@ -215,12 +219,31 @@ int commandBlockCheck(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { } int RedisModule_OnLoad(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { - REDISMODULE_NOT_USED(argv); - REDISMODULE_NOT_USED(argc); if (RedisModule_Init(ctx,"aclcheck",1,REDISMODULE_APIVER_1)== REDISMODULE_ERR) return REDISMODULE_ERR; + if (argc > 1) return RedisModule_WrongArity(ctx); + + /* When that flag is passed, we try to create too many categories, + * and the test expects this to fail. In this case redis returns REDISMODULE_ERR + * and set errno to ENOMEM*/ + if (argc == 1) { + long long fail_flag = 0; + RedisModule_StringToLongLong(argv[0], &fail_flag); + if (fail_flag) { + for (size_t j = 0; j < 45; j++) { + RedisModuleString* name = RedisModule_CreateStringPrintf(ctx, "customcategory%zu", j); + if (RedisModule_AddACLCategory(ctx, RedisModule_StringPtrLen(name, NULL)) == REDISMODULE_ERR) { + RedisModule_Assert(errno == ENOMEM); + RedisModule_FreeString(ctx, name); + return REDISMODULE_ERR; + } + RedisModule_FreeString(ctx, name); + } + } + } + if (RedisModule_CreateCommand(ctx,"aclcheck.set.check.key", set_aclcheck_key,"write",0,0,0) == REDISMODULE_ERR) return REDISMODULE_ERR; @@ -265,5 +288,29 @@ int RedisModule_OnLoad(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) "write",0,0,0) == REDISMODULE_ERR) return REDISMODULE_ERR; + /* This validates that, when module tries to add a category with invalid characters, + * redis returns REDISMODULE_ERR and set errno to `EINVAL` */ + if (RedisModule_AddACLCategory(ctx,"!nval!dch@r@cter$") == REDISMODULE_ERR) + RedisModule_Assert(errno == EINVAL); + else + return REDISMODULE_ERR; + + /* This validates that, when module tries to add a category that already exists, + * redis returns REDISMODULE_ERR and set errno to `EBUSY` */ + if (RedisModule_AddACLCategory(ctx,"write") == REDISMODULE_ERR) + RedisModule_Assert(errno == EBUSY); + else + return REDISMODULE_ERR; + + if (RedisModule_AddACLCategory(ctx,"foocategory") == REDISMODULE_ERR) + return REDISMODULE_ERR; + + if (RedisModule_CreateCommand(ctx,"aclcheck.module.command.test.add.new.aclcategories", module_test_acl_category,"",0,0,0) == REDISMODULE_ERR) + return REDISMODULE_ERR; + RedisModuleCommand *test_add_new_aclcategories = RedisModule_GetCommand(ctx,"aclcheck.module.command.test.add.new.aclcategories"); + + if (RedisModule_SetCommandACLCategories(test_add_new_aclcategories, "foocategory") == REDISMODULE_ERR) + return REDISMODULE_ERR; + return REDISMODULE_OK; } diff --git a/tests/unit/moduleapi/aclcheck.tcl b/tests/unit/moduleapi/aclcheck.tcl index ae3f67156..1ea09a232 100644 --- a/tests/unit/moduleapi/aclcheck.tcl +++ b/tests/unit/moduleapi/aclcheck.tcl @@ -104,15 +104,22 @@ start_server {tags {"modules acl"}} { assert_equal [r acl DRYRUN j2 aclcheck.module.command.aclcategories.read.only.category] OK } + test {Unload the module - aclcheck} { + assert_equal {OK} [r module unload aclcheck] + } +} + +start_server {tags {"modules acl"}} { test {test existing users to have access to module commands loaded on runtime} { - assert_equal [r module unload aclcheck] OK r acl SETUSER j3 on >password -@all +@WRITE assert_equal [r module load $testmodule] OK assert_equal [r acl DRYRUN j3 aclcheck.module.command.aclcategories.write] OK + assert_equal {OK} [r module unload aclcheck] } +} +start_server {tags {"modules acl"}} { test {test existing users without permissions, do not have access to module commands loaded on runtime.} { - assert_equal [r module unload aclcheck] OK r acl SETUSER j4 on >password -@all +@READ r acl SETUSER j5 on >password -@all +@WRITE assert_equal [r module load $testmodule] OK @@ -131,7 +138,21 @@ start_server {tags {"modules acl"}} { assert_equal {User j7 has no permissions to run the 'aclcheck.module.command.aclcategories.write.function.read.category' command} $e } - test "Unload the module - aclcheck" { + test {test if foocategory acl categories is added} { + r acl SETUSER j8 on >password -@all +@foocategory + assert_equal [r acl DRYRUN j8 aclcheck.module.command.test.add.new.aclcategories] OK + } + + test {test permission compaction and simplification for categories added by a module} { + r acl SETUSER j9 on >password -@all +@foocategory -@foocategory + catch {r ACL GETUSER j9} res + assert_equal {-@all -@foocategory} [lindex $res 5] assert_equal {OK} [r module unload aclcheck] } } + +start_server {tags {"modules acl"}} { + test {test module load fails if exceeds the maximum number of adding acl categories} { + assert_error {ERR Error loading the extension. Please check the server logs.} {r module load $testmodule 1} + } +}