From 0b3c583b6c1294ac20b53c988860548d6b959d2e Mon Sep 17 00:00:00 2001 From: Yang Zhao Date: Wed, 10 Jun 2026 16:24:17 -0700 Subject: [PATCH 1/4] ACL Role Signed-off-by: Yang Zhao --- src/acl.c | 721 +++++++++++++++++++++++++++++++++- src/commands.def | 97 ++++- src/commands/acl-delrole.json | 38 ++ src/commands/acl-getrole.json | 88 +++++ src/commands/acl-getuser.json | 11 + src/commands/acl-roles.json | 30 ++ src/commands/acl-setrole.json | 42 ++ src/config.c | 10 +- src/server.h | 13 +- tests/assets/role.acl | 7 + tests/unit/acl-role.tcl | 279 +++++++++++++ 11 files changed, 1322 insertions(+), 14 deletions(-) create mode 100644 src/commands/acl-delrole.json create mode 100644 src/commands/acl-getrole.json create mode 100644 src/commands/acl-roles.json create mode 100644 src/commands/acl-setrole.json create mode 100644 tests/assets/role.acl create mode 100644 tests/unit/acl-role.tcl diff --git a/src/acl.c b/src/acl.c index fbaab9b38d3..b5970c18e7a 100644 --- a/src/acl.c +++ b/src/acl.c @@ -41,6 +41,7 @@ * ==========================================================================*/ rax *Users; /* Table mapping usernames to user structures. */ +rax *Roles; /* Table mapping role names to role structures. */ user *DefaultUser; /* Global reference to the default user. Every new connection is associated to it, if no @@ -54,6 +55,10 @@ list *UsersToLoad; /* This is a list of users found in the configuration file array of SDS pointers: the first is the user name, all the remaining pointers are ACL rules in the same format as ACLSetUser(). */ +list *RolesToLoad; /* Similar to UsersToLoad, but for ACL roles. Every list + element is a NULL terminated array of SDS pointers: + the first is the role name, all the remaining pointers + are ACL rules (no passwords/on/off). */ list *ACLLog; /* Our security log, the user is able to inspect that using the ACL LOG command .*/ @@ -191,6 +196,11 @@ static void ACLAddAllowedFirstArg(aclSelector *selector, unsigned long id, const static void ACLFreeLogEntry(void *le); static int ACLSetSelector(aclSelector *selector, const char *op, size_t oplen); static struct serverCommand *ACLLookupCommand(const char *name); +static sds ACLDescribeSelector(aclSelector *selector); +static aclSelector *aclCreateSelectorFromOpSet(const char *opset, size_t opsetlen); +static sds *ACLMergeSelectorArguments(sds *argv, int argc, int *merged_argc, int *invalid_idx); +static int ACLStringHasSpaces(const char *s, size_t len); +static void ACLRoleInvalidateCache(role *r); /* The length of the string representation of a hashed password. */ #define HASH_PASSWORD_LEN (SHA256_BLOCK_SIZE * 2) @@ -448,6 +458,8 @@ static user *ACLCreateUser(const char *name, size_t namelen) { aclSelector *s = ACLCreateSelector(SELECTOR_FLAG_ROOT); listAddNodeHead(u->selectors, s); + u->roles = listCreate(); + raxInsert(Users, (unsigned char *)name, namelen, u, NULL); return u; } @@ -478,6 +490,19 @@ void ACLFreeUser(user *u) { } listRelease(u->passwords); listRelease(u->selectors); + + /* Remove this user from all roles' member lists */ + if (u->roles) { + listIter li; + listNode *ln; + listRewind(u->roles, &li); + while ((ln = listNext(&li))) { + role *r = listNodeValue(ln); + listNode *member_ln = listSearchKey(r->members, u); + if (member_ln) listDelNode(r->members, member_ln); + } + listRelease(u->roles); + } zfree(u); } @@ -528,6 +553,243 @@ static void ACLCopyUser(user *dst, user *src) { /* if src is NULL, we set it to NULL, if not, need to increment reference count */ incrRefCount(dst->acl_string); } + /* Clean up dst's existing role memberships, then copy from src. */ + if (dst->roles) { + listIter li; + listNode *ln; + listRewind(dst->roles, &li); + while ((ln = listNext(&li))) { + role *r = listNodeValue(ln); + listNode *member_ln = listSearchKey(r->members, dst); + if (member_ln) listDelNode(r->members, member_ln); + } + listRelease(dst->roles); + } + dst->roles = listCreate(); + if (src->roles) { + listIter li; + listNode *ln; + listRewind(src->roles, &li); + while ((ln = listNext(&li))) { + role *r = listNodeValue(ln); + listAddNodeTail(dst->roles, r); + listAddNodeTail(r->members, dst); + } + } +} + +/* ============================================================================= + * ACL Role functions + * ==========================================================================*/ + +/* Create a new ACL role with the given name and register it in the Roles + * radix tree. Returns NULL if a role with the same name already exists. */ +static role *ACLCreateRole(const char *name, size_t namelen) { + if (raxFind(Roles, (unsigned char *)name, namelen, NULL)) return NULL; + + role *r = zmalloc(sizeof(*r)); + r->name = sdsnewlen(name, namelen); + r->members = listCreate(); + r->acl_string = NULL; + r->selectors = listCreate(); + + listSetFreeMethod(r->selectors, ACLListFreeSelector); + listSetDupMethod(r->selectors, ACLListDuplicateSelector); + aclSelector *s = ACLCreateSelector(SELECTOR_FLAG_ROOT); + listAddNodeHead(r->selectors, s); + + raxInsert(Roles, (unsigned char *)name, namelen, r, NULL); + return r; +} + +/* Free the memory used by an ACL role structure. Note that this function + * will not remove the role from the Roles radix tree. */ +static void ACLFreeRole(role *r) { + sdsfree(r->name); + ACLRoleInvalidateCache(r); + listRelease(r->selectors); + listRelease(r->members); + zfree(r); +} + +/* Used for raxFreeWithCallback. */ +static void ACLFreeRoleVoid(void *r) { + ACLFreeRole(r); +} + +/* Lookup a role by name. Returns NULL if not found. */ +role *ACLGetRoleByName(const char *name, size_t namelen) { + void *myrole = NULL; + raxFind(Roles, (unsigned char *)name, namelen, &myrole); + return myrole; +} + +/* Invalidate the cached acl_string for a role. */ +static void ACLRoleInvalidateCache(role *r) { + if (r->acl_string) { + decrRefCount(r->acl_string); + r->acl_string = NULL; + } +} + +/* Set ACL rules on a role. This is similar to ACLSetUser but rejects + * user-only concepts. Returns C_OK on success, C_ERR on error with + * errno set. */ +static int ACLSetRole(role *r, const char *op, ssize_t oplen) { + if (oplen == -1) oplen = strlen(op); + if (oplen == 0) return C_OK; + + /* Reject user-only operations */ + if (!strcasecmp(op, "on") || !strcasecmp(op, "off") || + !strcasecmp(op, "nopass") || !strcasecmp(op, "resetpass") || + !strcasecmp(op, "reset") || + !strcasecmp(op, "skip-sanitize-payload") || + !strcasecmp(op, "sanitize-payload") || + op[0] == '>' || op[0] == '#' || op[0] == '<' || op[0] == '!') { + errno = EINVAL; + return C_ERR; + } + + /* Reject role membership operations (roles cannot contain roles) */ + if (oplen > 7 && (!strncasecmp(op, "+@role:", 7) || !strncasecmp(op, "-@role:", 7))) { + errno = EINVAL; + return C_ERR; + } + + /* Invalidate the cached acl string */ + ACLRoleInvalidateCache(r); + + /* Handle additional selectors */ + if (op[0] == '(' && op[oplen - 1] == ')') { + aclSelector *selector = aclCreateSelectorFromOpSet(op, oplen); + if (!selector) return C_ERR; + listAddNodeTail(r->selectors, selector); + return C_OK; + } else if (!strcasecmp(op, "clearselectors")) { + listIter li; + listNode *ln; + listRewind(r->selectors, &li); + /* There has to be a root selector */ + serverAssert(listNext(&li)); + while ((ln = listNext(&li))) { + listDelNode(r->selectors, ln); + } + return C_OK; + } + + /* Apply the rule to the root selector */ + aclSelector *selector = listNodeValue(listFirst(r->selectors)); + if (ACLSetSelector(selector, op, oplen) == C_ERR) { + return C_ERR; + } + return C_OK; +} + +/* High-level function to set multiple ACL rules on a role atomically. + * Returns NULL on success, or an SDS error string on failure. */ +static sds ACLStringSetRole(role *r, sds rolename, sds *argv, int argc) { + sds error = NULL; + + /* Create a temporary role to validate all changes */ + role *tempr = zmalloc(sizeof(*tempr)); + tempr->name = sdsdup(rolename); + tempr->selectors = listCreate(); + listSetFreeMethod(tempr->selectors, ACLListFreeSelector); + listSetDupMethod(tempr->selectors, ACLListDuplicateSelector); + tempr->members = listCreate(); + tempr->acl_string = NULL; + + /* If role already exists, copy its selectors */ + if (r) { + listRelease(tempr->selectors); + tempr->selectors = listDup(r->selectors); + } else { + aclSelector *s = ACLCreateSelector(SELECTOR_FLAG_ROOT); + listAddNodeHead(tempr->selectors, s); + } + + int merged_argc = 0, invalid_idx = 0; + sds *acl_args = ACLMergeSelectorArguments(argv, argc, &merged_argc, &invalid_idx); + if (!acl_args) { + error = sdscatfmt(sdsempty(), "Unmatched parenthesis in selector definition starting at '%s'.", + (char *)argv[invalid_idx]); + ACLFreeRole(tempr); + return error; + } + + for (int j = 0; j < merged_argc; j++) { + if (ACLSetRole(tempr, acl_args[j], (ssize_t)sdslen(acl_args[j])) != C_OK) { + const char *errmsg = ACLSetStringError(); + error = sdscatfmt(sdsempty(), "Error in ACL SETROLE modifier '%s': %s", (char *)acl_args[j], errmsg); + goto cleanup; + } + } + + /* Apply changes: if role doesn't exist, create it; otherwise update it */ + if (!r) { + r = ACLCreateRole(rolename, sdslen(rolename)); + if (!r) { + error = sdsnew("Role already exists"); + goto cleanup; + } + } + + /* Copy selectors from temp role to the actual role. + * Since users hold pointers to the role, updating the role's selectors + * in-place makes the change immediately visible to all members. */ + ACLRoleInvalidateCache(r); + listRelease(r->selectors); + r->selectors = listDup(tempr->selectors); + + /* Invalidate acl_string cache for all member users */ + { + listIter li; + listNode *ln; + listRewind(r->members, &li); + while ((ln = listNext(&li))) { + user *u = listNodeValue(ln); + if (u->acl_string) { + decrRefCount(u->acl_string); + u->acl_string = NULL; + } + } + } + +cleanup: + for (int i = 0; i < merged_argc; i++) sdsfree(acl_args[i]); + zfree(acl_args); + ACLFreeRole(tempr); + return error; +} + +/* Generate a description of the role's ACL rules (similar to ACLDescribeUser + * but without passwords and flags). */ +static robj *ACLDescribeRole(role *r) { + if (r->acl_string) { + incrRefCount(r->acl_string); + return r->acl_string; + } + + sds res = sdsempty(); + + /* Selectors */ + listIter li; + listNode *ln; + listRewind(r->selectors, &li); + while ((ln = listNext(&li))) { + aclSelector *selector = (aclSelector *)listNodeValue(ln); + sds desc = ACLDescribeSelector(selector); + if (selector->flags & SELECTOR_FLAG_ROOT) { + res = sdscatfmt(res, "%s", desc); + } else { + res = sdscatfmt(res, " (%s)", desc); + } + sdsfree(desc); + } + + r->acl_string = createObject(OBJ_STRING, res); + incrRefCount(r->acl_string); + return r->acl_string; } /* Given a command ID, this function set by reference 'word' and 'bit' @@ -936,6 +1198,15 @@ robj *ACLDescribeUser(user *u) { sdsfree(default_perm); } + /* Role memberships */ + if (u->roles && listLength(u->roles) > 0) { + listRewind(u->roles, &li); + while ((ln = listNext(&li))) { + role *r = listNodeValue(ln); + res = sdscatfmt(res, " +@role:%s", r->name); + } + } + u->acl_string = createObject(OBJ_STRING, res); /* because we are returning it, have to increase count */ incrRefCount(u->acl_string); @@ -1485,6 +1756,64 @@ int ACLSetUser(user *u, const char *op, ssize_t oplen) { serverAssert(ACLSetUser(u, "off", -1) == C_OK); serverAssert(ACLSetUser(u, "clearselectors", -1) == C_OK); serverAssert(ACLSetUser(u, "-@all", -1) == C_OK); + /* Clear role memberships on reset */ + if (u->roles) { + listIter li; + listNode *ln; + listRewind(u->roles, &li); + while ((ln = listNext(&li))) { + role *r = listNodeValue(ln); + listNode *member_ln = listSearchKey(r->members, u); + if (member_ln) listDelNode(r->members, member_ln); + } + listEmpty(u->roles); + } + } else if (oplen > 7 && !strncasecmp(op, "+@role:", 7)) { + /* Add user to a role */ + const char *rolename = op + 7; + size_t rolenamelen = oplen - 7; + role *r = ACLGetRoleByName(rolename, rolenamelen); + if (!r) { + errno = ESRCH; + return C_ERR; + } + /* Check if user is already in this role */ + listIter li; + listNode *ln; + int already_member = 0; + listRewind(u->roles, &li); + while ((ln = listNext(&li))) { + if (listNodeValue(ln) == r) { + already_member = 1; + break; + } + } + if (!already_member) { + listAddNodeTail(u->roles, r); + listAddNodeTail(r->members, u); + } + } else if (oplen > 7 && !strncasecmp(op, "-@role:", 7)) { + /* Remove user from a role */ + const char *rolename = op + 7; + size_t rolenamelen = oplen - 7; + role *r = ACLGetRoleByName(rolename, rolenamelen); + if (!r) { + errno = ESRCH; + return C_ERR; + } + /* Remove role from user's list */ + listIter li; + listNode *ln; + listRewind(u->roles, &li); + while ((ln = listNext(&li))) { + if (listNodeValue(ln) == r) { + listDelNode(u->roles, ln); + break; + } + } + /* Remove user from role's member list */ + listNode *member_ln = listSearchKey(r->members, u); + if (member_ln) listDelNode(r->members, member_ln); } else { aclSelector *selector = ACLUserGetRootSelector(u); if (ACLSetSelector(selector, op, oplen) == C_ERR) { @@ -1494,9 +1823,9 @@ int ACLSetUser(user *u, const char *op, ssize_t oplen) { return C_OK; } -/* Return a description of the error that occurred in ACLSetUser() according to - * the errno value set by the function on error. */ -const char *ACLSetUserStringError(void) { +/* Return a description of the error that occurred in ACLSetUser() or + * ACLSetRole() according to the errno value set on error. */ +const char *ACLSetStringError(void) { const char *errmsg = "Wrong format"; if (errno == ENOENT) errmsg = "Unknown command or category name in ACL"; @@ -1525,6 +1854,8 @@ const char *ACLSetUserStringError(void) { errmsg = "Allowing first-arg of a subcommand is not supported"; else if (errno == ERANGE) errmsg = "The provided database ID is out of range"; + else if (errno == ESRCH) + errmsg = "The specified ACL role does not exist"; return errmsg; } @@ -1543,9 +1874,12 @@ static user *ACLCreateDefaultUser(void) { /* Initialization of the ACL subsystem. */ void ACLInit(void) { Users = raxNew(); + Roles = raxNew(); UsersToLoad = listCreate(); + RolesToLoad = listCreate(); ACLInitCommandCategories(); listSetMatchMethod(UsersToLoad, ACLListMatchLoadedUser); + listSetMatchMethod(RolesToLoad, ACLListMatchLoadedUser); ACLLog = listCreate(); DefaultUser = ACLCreateDefaultUser(); } @@ -2087,6 +2421,31 @@ int ACLCheckAllUserCommandPerm(user *u, struct serverCommand *cmd, robj **argv, } } + /* Check role selectors */ + if (u->roles) { + listIter rli; + listNode *rln; + listRewind(u->roles, &rli); + while ((rln = listNext(&rli))) { + role *r = (role *)listNodeValue(rln); + listIter sli; + listNode *sln; + listRewind(r->selectors, &sli); + while ((sln = listNext(&sli))) { + aclSelector *s = (aclSelector *)listNodeValue(sln); + int acl_retval = ACLSelectorCheckCmd(s, cmd, argv, argc, &local_idxptr, &cache, dbid); + if (acl_retval == ACL_OK) { + cleanupACLKeyResultCache(&cache); + return ACL_OK; + } + if (acl_retval > relevant_error || (acl_retval == relevant_error && local_idxptr > last_idx)) { + relevant_error = acl_retval; + last_idx = local_idxptr; + } + } + } + } + *idxptr = last_idx; cleanupACLKeyResultCache(&cache); return relevant_error; @@ -2110,6 +2469,22 @@ static list *getUpcomingChannelList(user *new, user *original) { aclSelector *s = (aclSelector *)listNodeValue(ln); if (s->flags & SELECTOR_FLAG_ALLCHANNELS) return NULL; } + /* Also check role selectors */ + if (new->roles) { + listIter rli; + listNode *rln; + listRewind(new->roles, &rli); + while ((rln = listNext(&rli))) { + role *r = (role *)listNodeValue(rln); + listIter sli; + listNode *sln; + listRewind(r->selectors, &sli); + while ((sln = listNext(&sli))) { + aclSelector *s = (aclSelector *)listNodeValue(sln); + if (s->flags & SELECTOR_FLAG_ALLCHANNELS) return NULL; + } + } + } /* Next, check if the new list of channels * is a strict superset of the original. This is done by @@ -2125,6 +2500,25 @@ static list *getUpcomingChannelList(user *new, user *original) { listAddNodeTail(upcoming, listNodeValue(lpn)); } } + /* Also collect channels from role selectors */ + if (new->roles) { + listIter rli; + listNode *rln; + listRewind(new->roles, &rli); + while ((rln = listNext(&rli))) { + role *r = (role *)listNodeValue(rln); + listIter sli; + listNode *sln; + listRewind(r->selectors, &sli); + while ((sln = listNext(&sli))) { + aclSelector *s = (aclSelector *)listNodeValue(sln); + listRewind(s->channels, &lpi); + while ((lpn = listNext(&lpi))) { + listAddNodeTail(upcoming, listNodeValue(lpn)); + } + } + } + } int match = 1; listRewind(original->selectors, &li); @@ -2321,7 +2715,7 @@ sds ACLStringSetUser(user *u, sds username, sds *argv, int argc) { for (int j = 0; j < merged_argc; j++) { if (ACLSetUser(tempu, acl_args[j], (ssize_t)sdslen(acl_args[j])) != C_OK) { - const char *errmsg = ACLSetUserStringError(); + const char *errmsg = ACLSetStringError(); error = sdscatfmt(sdsempty(), "Error in ACL SETUSER modifier '%s': %s", (char *)acl_args[j], errmsg); goto cleanup; } @@ -2395,7 +2789,7 @@ int ACLAppendUserForLoading(sds *argv, int argc, int *argc_err) { for (int j = 0; j < merged_argc; j++) { if (ACLSetUser(fakeuser, acl_args[j], sdslen(acl_args[j])) == C_ERR) { - if (errno != ENOENT) { + if (errno != ENOENT && errno != ESRCH) { ACLFreeUser(fakeuser); if (argc_err) *argc_err = j; for (int i = 0; i < merged_argc; i++) sdsfree(acl_args[i]); @@ -2444,7 +2838,7 @@ static int ACLLoadConfiguredUsers(void) { /* Load every rule defined for this user. */ for (int j = 1; aclrules[j]; j++) { if (ACLSetUser(u, aclrules[j], sdslen(aclrules[j])) != C_OK) { - const char *errmsg = ACLSetUserStringError(); + const char *errmsg = ACLSetStringError(); serverLog(LL_WARNING, "Error loading ACL rule '%s' for " "the user named '%s': %s", @@ -2466,6 +2860,67 @@ static int ACLLoadConfiguredUsers(void) { return C_OK; } +/* Append a role definition for deferred loading (from valkey.conf). */ +int ACLAppendRoleForLoading(sds *argv, int argc, int *argc_err) { + if (argc < 2 || strcasecmp(argv[0], "role")) { + if (argc_err) *argc_err = 0; + return C_ERR; + } + + if (listSearchKey(RolesToLoad, argv[1])) { + if (argc_err) *argc_err = 1; + errno = EALREADY; + return C_ERR; + } + + /* Store the role definition for later loading. */ + int merged_argc = 0, invalid_idx = 0; + sds *acl_args = ACLMergeSelectorArguments(argv + 2, argc - 2, &merged_argc, &invalid_idx); + if (!acl_args) { + if (argc_err) *argc_err = invalid_idx + 2; + return C_ERR; + } + + sds *copy = zmalloc(sizeof(sds) * (merged_argc + 2)); + copy[0] = sdsdup(argv[1]); + for (int j = 0; j < merged_argc; j++) copy[j + 1] = sdsdup(acl_args[j]); + copy[merged_argc + 1] = NULL; + listAddNodeTail(RolesToLoad, copy); + for (int i = 0; i < merged_argc; i++) sdsfree(acl_args[i]); + zfree(acl_args); + return C_OK; +} + +/* Load configured roles from RolesToLoad. Must be called before + * ACLLoadConfiguredUsers so that users can reference roles. */ +static int ACLLoadConfiguredRoles(void) { + listIter li; + listNode *ln; + listRewind(RolesToLoad, &li); + while ((ln = listNext(&li)) != NULL) { + sds *aclrules = listNodeValue(ln); + sds rolename = aclrules[0]; + + if (ACLStringHasSpaces(rolename, sdslen(rolename))) { + serverLog(LL_WARNING, "Spaces not allowed in ACL role names"); + return C_ERR; + } + + role *r = ACLGetRoleByName(rolename, sdslen(rolename)); + /* Count number of rules */ + int argc = 0; + while (aclrules[argc + 1]) argc++; + + sds error = ACLStringSetRole(r, rolename, aclrules + 1, argc); + if (error) { + serverLog(LL_WARNING, "Error loading ACL role '%s': %s", rolename, error); + sdsfree(error); + return C_ERR; + } + } + return C_OK; +} + /* This function loads the ACL from the specified filename: every line * is validated and should be either empty or in the format used to specify * users in the valkey.conf or in the ACL file, that is: @@ -2514,9 +2969,12 @@ static sds ACLLoadFromFile(const char *filename) { * so if there are errors loading the ACL file we can rollback to the * old version. */ rax *old_users = Users; + rax *old_roles = Roles; Users = raxNew(); + Roles = raxNew(); /* Load each line of the file. */ + /* First pass: load role definitions */ for (int i = 0; i < totlines; i++) { sds *argv; int argc; @@ -2540,8 +2998,67 @@ static sds ACLLoadFromFile(const char *filename) { continue; } - /* The line should start with the "user" keyword. */ - if (strcmp(argv[0], "user") || argc < 2) { + /* Only process role lines in first pass */ + if (strcmp(argv[0], "role") != 0) { + sdsfreesplitres(argv, argc); + continue; + } + + if (argc < 2) { + errors = sdscatprintf(errors, + "%s:%d: role line requires a role name. ", + server.acl_filename, linenum); + sdsfreesplitres(argv, argc); + continue; + } + + if (ACLStringHasSpaces(argv[1], sdslen(argv[1]))) { + errors = sdscatprintf(errors, "%s:%d: role name '%s' contains invalid characters. ", + server.acl_filename, linenum, argv[1]); + sdsfreesplitres(argv, argc); + continue; + } + + role *r = ACLGetRoleByName(argv[1], sdslen(argv[1])); + sds error = ACLStringSetRole(r, argv[1], argv + 2, argc - 2); + if (error) { + errors = sdscatprintf(errors, "%s:%d: %s. ", server.acl_filename, linenum, error); + sdsfree(error); + } + + sdsfreesplitres(argv, argc); + } + + /* Second pass: load user definitions */ + for (int i = 0; i < totlines; i++) { + sds *argv; + int argc; + int linenum = i + 1; + + /* Re-trim is safe since lines were already trimmed */ + if (lines[i][0] == '\0') continue; + + /* Split into arguments */ + argv = sdssplitlen(lines[i], sdslen(lines[i]), " ", 1, &argc); + if (argv == NULL) continue; /* Error already reported in first pass */ + if (argc == 0) { + sdsfreesplitres(argv, argc); + continue; + } + + /* Only process user lines in second pass */ + if (strcmp(argv[0], "user") != 0) { + /* If it's not 'user' or 'role', report error */ + if (strcmp(argv[0], "role") != 0) { + errors = sdscatprintf(errors, + "%s:%d should start with user or role keyword. ", + server.acl_filename, linenum); + } + sdsfreesplitres(argv, argc); + continue; + } + + if (argc < 2) { errors = sdscatprintf(errors, "%s:%d should start with user keyword followed " "by the username. ", @@ -2582,7 +3099,7 @@ static sds ACLLoadFromFile(const char *filename) { for (int j = 0; j < merged_argc; j++) { acl_args[j] = sdstrim(acl_args[j], "\t\r\n"); if (ACLSetUser(u, acl_args[j], sdslen(acl_args[j])) != C_OK) { - const char *errmsg = ACLSetUserStringError(); + const char *errmsg = ACLSetStringError(); if (errno == ENOENT) { /* For missing commands, we print out more information since * it shouldn't contain any sensitive information. */ @@ -2665,11 +3182,14 @@ static sds ACLLoadFromFile(const char *filename) { if (user_channels) raxFreeWithCallback(user_channels, listReleaseVoid); raxFreeWithCallback(old_users, ACLFreeUserVoid); + raxFreeWithCallback(old_roles, ACLFreeRoleVoid); sdsfree(errors); return NULL; } else { raxFreeWithCallback(Users, ACLFreeUserVoid); + raxFreeWithCallback(Roles, ACLFreeRoleVoid); Users = old_users; + Roles = old_roles; errors = sdscat(errors, "WARNING: ACL errors detected, no change to the previously active ACL rules was performed"); return errors; @@ -2688,6 +3208,25 @@ static int ACLSaveToFile(const char *filename) { /* Let's generate an SDS string containing the new version of the * ACL file. */ raxIterator ri; + + /* Write roles first */ + raxStart(&ri, Roles); + raxSeek(&ri, "^", NULL, 0); + while (raxNext(&ri)) { + role *r = ri.data; + sds role = sdsnew("role "); + role = sdscatsds(role, r->name); + role = sdscatlen(role, " ", 1); + robj *descr = ACLDescribeRole(r); + role = sdscatsds(role, objectGetVal(descr)); + decrRefCount(descr); + acl = sdscatsds(acl, role); + acl = sdscatlen(acl, "\n", 1); + sdsfree(role); + } + raxStop(&ri); + + /* Write users */ raxStart(&ri, Users); raxSeek(&ri, "^", NULL, 0); while (raxNext(&ri)) { @@ -2769,6 +3308,23 @@ void ACLLoadUsersAtStartup(void) { exit(1); } + if (server.acl_filename[0] != '\0' && listLength(RolesToLoad) != 0) { + serverLog(LL_WARNING, + "Configuring %s with roles defined in valkey.conf and at " + "the same setting an ACL file path is invalid. This setup " + "is very likely to lead to configuration errors and security " + "holes, please define either an ACL file or declare roles " + "directly in your valkey.conf, but not both.", + SERVER_TITLE); + exit(1); + } + + /* Load roles before users so that users can reference them */ + if (ACLLoadConfiguredRoles() == C_ERR) { + serverLog(LL_WARNING, "Critical error while loading ACL roles. Exiting."); + exit(1); + } + if (ACLLoadConfiguredUsers() == C_ERR) { serverLog(LL_WARNING, "Critical error while loading ACLs. Exiting."); exit(1); @@ -2784,6 +3340,12 @@ void ACLLoadUsersAtStartup(void) { } } +/* Also provide a function to load roles at startup from config */ +void ACLLoadRolesAtStartup(void) { + /* Roles are already loaded in ACLLoadUsersAtStartup before users. + * This function is a no-op placeholder for external callers. */ +} + /* ============================================================================= * ACL log * ==========================================================================*/ @@ -3198,11 +3760,45 @@ void aclCommand(client *c) { int sfields = aclAddReplySelectorDescription(c, (aclSelector *)listNodeValue(ln)); setDeferredMapLen(c, slen, sfields); } + + /* Roles */ + addReplyBulkCString(c, "roles"); + addReplyArrayLen(c, u->roles ? listLength(u->roles) : 0); + fields++; + if (u->roles) { + listRewind(u->roles, &li); + while ((ln = listNext(&li))) { + role *r = listNodeValue(ln); + addReplyBulkCBuffer(c, r->name, sdslen(r->name)); + } + } + setDeferredMapLen(c, ufields, fields); } else if ((!strcasecmp(sub, "list") || !strcasecmp(sub, "users")) && c->argc == 2) { int justnames = !strcasecmp(sub, "users"); - addReplyArrayLen(c, raxSize(Users)); + if (justnames) { + addReplyArrayLen(c, raxSize(Users)); + } else { + addReplyArrayLen(c, raxSize(Roles) + raxSize(Users)); + } raxIterator ri; + if (!justnames) { + /* List roles first in ACL LIST */ + raxStart(&ri, Roles); + raxSeek(&ri, "^", NULL, 0); + while (raxNext(&ri)) { + role *r = ri.data; + sds config = sdsnew("role "); + config = sdscatsds(config, r->name); + config = sdscatlen(config, " ", 1); + robj *descr = ACLDescribeRole(r); + config = sdscatsds(config, objectGetVal(descr)); + decrRefCount(descr); + addReplyBulkSds(c, config); + } + raxStop(&ri); + } + /* List users */ raxStart(&ri, Users); raxSeek(&ri, "^", NULL, 0); while (raxNext(&ri)) { @@ -3210,7 +3806,6 @@ void aclCommand(client *c) { if (justnames) { addReplyBulkCBuffer(c, u->name, sdslen(u->name)); } else { - /* Return information in the configuration file format. */ sds config = sdsnew("user "); config = sdscatsds(config, u->name); config = sdscatlen(config, " ", 1); @@ -3356,6 +3951,102 @@ void aclCommand(client *c) { addReplyBulkCString(c, "timestamp-last-updated"); addReplyLongLong(c, le->ctime); } + } else if (!strcasecmp(sub, "setrole") && c->argc >= 3) { + sds rolename = objectGetVal(c->argv[2]); + /* Check role name validity. */ + if (ACLStringHasSpaces(rolename, sdslen(rolename))) { + addReplyError(c, "Role names can't contain spaces or null characters"); + return; + } + + role *r = ACLGetRoleByName(rolename, sdslen(rolename)); + + sds *temp_argv = zmalloc((c->argc - 3) * sizeof(sds)); + for (int i = 3; i < c->argc; i++) temp_argv[i - 3] = objectGetVal(c->argv[i]); + + sds error = ACLStringSetRole(r, rolename, temp_argv, c->argc - 3); + zfree(temp_argv); + if (error == NULL) { + addReply(c, shared.ok); + } else { + addReplyErrorSdsSafe(c, error); + } + return; + } else if (!strcasecmp(sub, "delrole") && c->argc >= 3) { + for (int j = 2; j < c->argc; j++) { + sds rolename = objectGetVal(c->argv[j]); + role *r = ACLGetRoleByName(rolename, sdslen(rolename)); + if (!r) { + addReplyErrorFormat(c, "Role '%s' not found", rolename); + return; + } + if (listLength(r->members) > 0) { + addReplyErrorFormat(c, "Role '%s' has members. Remove all users from the role before deleting it.", + rolename); + return; + } + } + + int deleted = 0; + for (int j = 2; j < c->argc; j++) { + sds rolename = objectGetVal(c->argv[j]); + role *r; + if (raxRemove(Roles, (unsigned char *)rolename, sdslen(rolename), (void **)&r)) { + ACLFreeRole(r); + deleted++; + } + } + addReplyLongLong(c, deleted); + } else if (!strcasecmp(sub, "getrole") && c->argc == 3) { + sds rolename = objectGetVal(c->argv[2]); + role *r = ACLGetRoleByName(rolename, sdslen(rolename)); + if (!r) { + addReplyNull(c); + return; + } + + void *gfields = addReplyDeferredLen(c); + int fields = 0; + + /* Commands/keys/channels from root selector */ + aclSelector *root = listNodeValue(listFirst(r->selectors)); + fields += aclAddReplySelectorDescription(c, root); + + /* Additional selectors */ + addReplyBulkCString(c, "selectors"); + addReplyArrayLen(c, listLength(r->selectors) - 1); + fields++; + listIter li; + listNode *ln; + listRewind(r->selectors, &li); + listNext(&li); /* skip root */ + while ((ln = listNext(&li))) { + void *slen = addReplyDeferredLen(c); + int sfields = aclAddReplySelectorDescription(c, (aclSelector *)listNodeValue(ln)); + setDeferredMapLen(c, slen, sfields); + } + + /* Members */ + addReplyBulkCString(c, "members"); + addReplyArrayLen(c, listLength(r->members)); + fields++; + listRewind(r->members, &li); + while ((ln = listNext(&li))) { + user *u = listNodeValue(ln); + addReplyBulkCBuffer(c, u->name, sdslen(u->name)); + } + + setDeferredMapLen(c, gfields, fields); + } else if (!strcasecmp(sub, "roles") && c->argc == 2) { + addReplyArrayLen(c, raxSize(Roles)); + raxIterator ri; + raxStart(&ri, Roles); + raxSeek(&ri, "^", NULL, 0); + while (raxNext(&ri)) { + role *r = ri.data; + addReplyBulkCBuffer(c, r->name, sdslen(r->name)); + } + raxStop(&ri); } else if (!strcasecmp(sub, "dryrun") && c->argc >= 4) { struct serverCommand *cmd; user *u = ACLGetUserByName(objectGetVal(c->argv[2]), sdslen(objectGetVal(c->argv[2]))); @@ -3398,6 +4089,14 @@ void aclCommand(client *c) { "GENPASS []", " Generate a secure 256-bit user password. The optional `bits` argument can", " be used to specify a different size.", + "SETROLE [ ...]", + " Create or modify a role with the specified rules.", + "DELROLE ", + " Delete a role (must have no members).", + "GETROLE ", + " Get the role's details.", + "ROLES", + " List all the registered role names.", "LIST", " Show users details in config file format.", "LOAD", diff --git a/src/commands.def b/src/commands.def index ca87553cd5d..cbd8068f5e7 100644 --- a/src/commands.def +++ b/src/commands.def @@ -6852,6 +6852,31 @@ struct COMMAND_ARG ACL_CAT_Args[] = { {MAKE_ARG("category",ARG_TYPE_STRING,-1,NULL,NULL,NULL,CMD_ARG_OPTIONAL,0,NULL)}, }; +/********** ACL DELROLE ********************/ + +#ifndef SKIP_CMD_HISTORY_TABLE +/* ACL DELROLE history */ +#define ACL_DELROLE_History NULL +#endif + +#ifndef SKIP_CMD_TIPS_TABLE +/* ACL DELROLE tips */ +const char *ACL_DELROLE_Tips[] = { +"request_policy:all_nodes", +"response_policy:all_succeeded", +}; +#endif + +#ifndef SKIP_CMD_KEY_SPECS_TABLE +/* ACL DELROLE key specs */ +#define ACL_DELROLE_Keyspecs NULL +#endif + +/* ACL DELROLE argument table */ +struct COMMAND_ARG ACL_DELROLE_Args[] = { +{MAKE_ARG("rolename",ARG_TYPE_STRING,-1,NULL,NULL,NULL,CMD_ARG_MULTIPLE,0,NULL)}, +}; + /********** ACL DELUSER ********************/ #ifndef SKIP_CMD_HISTORY_TABLE @@ -6923,6 +6948,28 @@ struct COMMAND_ARG ACL_GENPASS_Args[] = { {MAKE_ARG("bits",ARG_TYPE_INTEGER,-1,NULL,NULL,NULL,CMD_ARG_OPTIONAL,0,NULL)}, }; +/********** ACL GETROLE ********************/ + +#ifndef SKIP_CMD_HISTORY_TABLE +/* ACL GETROLE history */ +#define ACL_GETROLE_History NULL +#endif + +#ifndef SKIP_CMD_TIPS_TABLE +/* ACL GETROLE tips */ +#define ACL_GETROLE_Tips NULL +#endif + +#ifndef SKIP_CMD_KEY_SPECS_TABLE +/* ACL GETROLE key specs */ +#define ACL_GETROLE_Keyspecs NULL +#endif + +/* ACL GETROLE argument table */ +struct COMMAND_ARG ACL_GETROLE_Args[] = { +{MAKE_ARG("rolename",ARG_TYPE_STRING,-1,NULL,NULL,NULL,CMD_ARG_NONE,0,NULL)}, +}; + /********** ACL GETUSER ********************/ #ifndef SKIP_CMD_HISTORY_TABLE @@ -6931,6 +6978,7 @@ commandHistory ACL_GETUSER_History[] = { {"6.2.0","Added Pub/Sub channel patterns."}, {"7.0.0","Added selectors and changed the format of key and channel patterns from a list to their rule representation."}, {"9.1.0","Added database permission rules."}, +{"10.0.0","Added roles."}, }; #endif @@ -7030,6 +7078,23 @@ struct COMMAND_ARG ACL_LOG_Args[] = { {MAKE_ARG("operation",ARG_TYPE_ONEOF,-1,NULL,NULL,NULL,CMD_ARG_OPTIONAL,2,NULL),.subargs=ACL_LOG_operation_Subargs}, }; +/********** ACL ROLES ********************/ + +#ifndef SKIP_CMD_HISTORY_TABLE +/* ACL ROLES history */ +#define ACL_ROLES_History NULL +#endif + +#ifndef SKIP_CMD_TIPS_TABLE +/* ACL ROLES tips */ +#define ACL_ROLES_Tips NULL +#endif + +#ifndef SKIP_CMD_KEY_SPECS_TABLE +/* ACL ROLES key specs */ +#define ACL_ROLES_Keyspecs NULL +#endif + /********** ACL SAVE ********************/ #ifndef SKIP_CMD_HISTORY_TABLE @@ -7050,6 +7115,32 @@ const char *ACL_SAVE_Tips[] = { #define ACL_SAVE_Keyspecs NULL #endif +/********** ACL SETROLE ********************/ + +#ifndef SKIP_CMD_HISTORY_TABLE +/* ACL SETROLE history */ +#define ACL_SETROLE_History NULL +#endif + +#ifndef SKIP_CMD_TIPS_TABLE +/* ACL SETROLE tips */ +const char *ACL_SETROLE_Tips[] = { +"request_policy:all_nodes", +"response_policy:all_succeeded", +}; +#endif + +#ifndef SKIP_CMD_KEY_SPECS_TABLE +/* ACL SETROLE key specs */ +#define ACL_SETROLE_Keyspecs NULL +#endif + +/* ACL SETROLE argument table */ +struct COMMAND_ARG ACL_SETROLE_Args[] = { +{MAKE_ARG("rolename",ARG_TYPE_STRING,-1,NULL,NULL,NULL,CMD_ARG_NONE,0,NULL)}, +{MAKE_ARG("rule",ARG_TYPE_STRING,-1,NULL,NULL,NULL,CMD_ARG_OPTIONAL|CMD_ARG_MULTIPLE,0,NULL)}, +}; + /********** ACL SETUSER ********************/ #ifndef SKIP_CMD_HISTORY_TABLE @@ -7117,15 +7208,19 @@ struct COMMAND_ARG ACL_SETUSER_Args[] = { /* ACL command table */ struct COMMAND_STRUCT ACL_Subcommands[] = { {MAKE_CMD("cat","Lists the ACL categories, or the commands inside a category.","O(1) since the categories and commands are a fixed set.","6.0.0",CMD_DOC_NONE,NULL,NULL,"server",COMMAND_GROUP_SERVER,ACL_CAT_History,0,ACL_CAT_Tips,0,aclCommand,-2,CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_SENTINEL,ACL_CATEGORY_SLOW,NULL,ACL_CAT_Keyspecs,0,NULL,1),.args=ACL_CAT_Args}, +{MAKE_CMD("delrole","Deletes one or more ACL roles. Fails if any role has members.","O(N). Where N is the number of roles to delete.","10.0.0",CMD_DOC_NONE,NULL,NULL,"server",COMMAND_GROUP_SERVER,ACL_DELROLE_History,0,ACL_DELROLE_Tips,2,aclCommand,-3,CMD_ADMIN|CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_SENTINEL,ACL_CATEGORY_ADMIN|ACL_CATEGORY_DANGEROUS|ACL_CATEGORY_SLOW,NULL,ACL_DELROLE_Keyspecs,0,NULL,1),.args=ACL_DELROLE_Args}, {MAKE_CMD("deluser","Deletes ACL users, and terminates their connections.","O(1) amortized time considering the typical user.","6.0.0",CMD_DOC_NONE,NULL,NULL,"server",COMMAND_GROUP_SERVER,ACL_DELUSER_History,0,ACL_DELUSER_Tips,2,aclCommand,-3,CMD_ADMIN|CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_SENTINEL,ACL_CATEGORY_ADMIN|ACL_CATEGORY_DANGEROUS|ACL_CATEGORY_SLOW,NULL,ACL_DELUSER_Keyspecs,0,NULL,1),.args=ACL_DELUSER_Args}, {MAKE_CMD("dryrun","Simulates the execution of a command by a user, without executing the command.","O(1).","7.0.0",CMD_DOC_NONE,NULL,NULL,"server",COMMAND_GROUP_SERVER,ACL_DRYRUN_History,0,ACL_DRYRUN_Tips,0,aclCommand,-4,CMD_ADMIN|CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_SENTINEL,ACL_CATEGORY_ADMIN|ACL_CATEGORY_DANGEROUS|ACL_CATEGORY_SLOW,NULL,ACL_DRYRUN_Keyspecs,0,NULL,3),.args=ACL_DRYRUN_Args}, {MAKE_CMD("genpass","Generates a pseudorandom, secure password that can be used to identify ACL users.","O(1)","6.0.0",CMD_DOC_NONE,NULL,NULL,"server",COMMAND_GROUP_SERVER,ACL_GENPASS_History,0,ACL_GENPASS_Tips,0,aclCommand,-2,CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_SENTINEL,ACL_CATEGORY_SLOW,NULL,ACL_GENPASS_Keyspecs,0,NULL,1),.args=ACL_GENPASS_Args}, -{MAKE_CMD("getuser","Lists the ACL rules of a user.","O(N). Where N is the number of password, command and pattern rules that the user has.","6.0.0",CMD_DOC_NONE,NULL,NULL,"server",COMMAND_GROUP_SERVER,ACL_GETUSER_History,3,ACL_GETUSER_Tips,0,aclCommand,3,CMD_ADMIN|CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_SENTINEL,ACL_CATEGORY_ADMIN|ACL_CATEGORY_DANGEROUS|ACL_CATEGORY_SLOW,NULL,ACL_GETUSER_Keyspecs,0,NULL,1),.args=ACL_GETUSER_Args}, +{MAKE_CMD("getrole","Returns the ACL rules of an ACL role.","O(N). Where N is the number of rules defined for the role.","10.0.0",CMD_DOC_NONE,NULL,NULL,"server",COMMAND_GROUP_SERVER,ACL_GETROLE_History,0,ACL_GETROLE_Tips,0,aclCommand,3,CMD_ADMIN|CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_SENTINEL,ACL_CATEGORY_ADMIN|ACL_CATEGORY_DANGEROUS|ACL_CATEGORY_SLOW,NULL,ACL_GETROLE_Keyspecs,0,NULL,1),.args=ACL_GETROLE_Args}, +{MAKE_CMD("getuser","Lists the ACL rules of a user.","O(N). Where N is the number of password, command and pattern rules that the user has.","6.0.0",CMD_DOC_NONE,NULL,NULL,"server",COMMAND_GROUP_SERVER,ACL_GETUSER_History,4,ACL_GETUSER_Tips,0,aclCommand,3,CMD_ADMIN|CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_SENTINEL,ACL_CATEGORY_ADMIN|ACL_CATEGORY_DANGEROUS|ACL_CATEGORY_SLOW,NULL,ACL_GETUSER_Keyspecs,0,NULL,1),.args=ACL_GETUSER_Args}, {MAKE_CMD("help","Returns helpful text about the different subcommands.","O(1)","6.0.0",CMD_DOC_NONE,NULL,NULL,"server",COMMAND_GROUP_SERVER,ACL_HELP_History,0,ACL_HELP_Tips,0,aclCommand,2,CMD_LOADING|CMD_STALE|CMD_SENTINEL,ACL_CATEGORY_SLOW,NULL,ACL_HELP_Keyspecs,0,NULL,0)}, {MAKE_CMD("list","Dumps the effective rules in ACL file format.","O(N). Where N is the number of configured users.","6.0.0",CMD_DOC_NONE,NULL,NULL,"server",COMMAND_GROUP_SERVER,ACL_LIST_History,0,ACL_LIST_Tips,0,aclCommand,2,CMD_ADMIN|CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_SENTINEL,ACL_CATEGORY_ADMIN|ACL_CATEGORY_DANGEROUS|ACL_CATEGORY_SLOW,NULL,ACL_LIST_Keyspecs,0,NULL,0)}, {MAKE_CMD("load","Reloads the rules from the configured ACL file.","O(N). Where N is the number of configured users.","6.0.0",CMD_DOC_NONE,NULL,NULL,"server",COMMAND_GROUP_SERVER,ACL_LOAD_History,0,ACL_LOAD_Tips,0,aclCommand,2,CMD_ADMIN|CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_SENTINEL,ACL_CATEGORY_ADMIN|ACL_CATEGORY_DANGEROUS|ACL_CATEGORY_SLOW,NULL,ACL_LOAD_Keyspecs,0,NULL,0)}, {MAKE_CMD("log","Lists recent security events generated due to ACL rules.","O(N) with N being the number of entries shown.","6.0.0",CMD_DOC_NONE,NULL,NULL,"server",COMMAND_GROUP_SERVER,ACL_LOG_History,1,ACL_LOG_Tips,0,aclCommand,-2,CMD_ADMIN|CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_SENTINEL,ACL_CATEGORY_ADMIN|ACL_CATEGORY_DANGEROUS|ACL_CATEGORY_SLOW,NULL,ACL_LOG_Keyspecs,0,NULL,1),.args=ACL_LOG_Args}, +{MAKE_CMD("roles","Lists all ACL roles.","O(N). Where N is the number of configured roles.","10.0.0",CMD_DOC_NONE,NULL,NULL,"server",COMMAND_GROUP_SERVER,ACL_ROLES_History,0,ACL_ROLES_Tips,0,aclCommand,2,CMD_ADMIN|CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_SENTINEL,ACL_CATEGORY_ADMIN|ACL_CATEGORY_DANGEROUS|ACL_CATEGORY_SLOW,NULL,ACL_ROLES_Keyspecs,0,NULL,0)}, {MAKE_CMD("save","Saves the effective ACL rules in the configured ACL file.","O(N). Where N is the number of configured users.","6.0.0",CMD_DOC_NONE,NULL,NULL,"server",COMMAND_GROUP_SERVER,ACL_SAVE_History,0,ACL_SAVE_Tips,2,aclCommand,2,CMD_ADMIN|CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_SENTINEL,ACL_CATEGORY_ADMIN|ACL_CATEGORY_DANGEROUS|ACL_CATEGORY_SLOW,NULL,ACL_SAVE_Keyspecs,0,NULL,0)}, +{MAKE_CMD("setrole","Creates and modifies an ACL role and its rules.","O(N). Where N is the number of rules provided.","10.0.0",CMD_DOC_NONE,NULL,NULL,"server",COMMAND_GROUP_SERVER,ACL_SETROLE_History,0,ACL_SETROLE_Tips,2,aclCommand,-3,CMD_ADMIN|CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_SENTINEL,ACL_CATEGORY_ADMIN|ACL_CATEGORY_DANGEROUS|ACL_CATEGORY_SLOW,NULL,ACL_SETROLE_Keyspecs,0,NULL,2),.args=ACL_SETROLE_Args}, {MAKE_CMD("setuser","Creates and modifies an ACL user and its rules.","O(N). Where N is the number of rules provided.","6.0.0",CMD_DOC_NONE,NULL,NULL,"server",COMMAND_GROUP_SERVER,ACL_SETUSER_History,3,ACL_SETUSER_Tips,2,aclCommand,-3,CMD_ADMIN|CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_SENTINEL,ACL_CATEGORY_ADMIN|ACL_CATEGORY_DANGEROUS|ACL_CATEGORY_SLOW,NULL,ACL_SETUSER_Keyspecs,0,NULL,2),.args=ACL_SETUSER_Args}, {MAKE_CMD("users","Lists all ACL users.","O(N). Where N is the number of configured users.","6.0.0",CMD_DOC_NONE,NULL,NULL,"server",COMMAND_GROUP_SERVER,ACL_USERS_History,0,ACL_USERS_Tips,0,aclCommand,2,CMD_ADMIN|CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_SENTINEL,ACL_CATEGORY_ADMIN|ACL_CATEGORY_DANGEROUS|ACL_CATEGORY_SLOW,NULL,ACL_USERS_Keyspecs,0,NULL,0)}, {MAKE_CMD("whoami","Returns the authenticated username of the current connection.","O(1)","6.0.0",CMD_DOC_NONE,NULL,NULL,"server",COMMAND_GROUP_SERVER,ACL_WHOAMI_History,0,ACL_WHOAMI_Tips,0,aclCommand,2,CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_SENTINEL,ACL_CATEGORY_SLOW,NULL,ACL_WHOAMI_Keyspecs,0,NULL,0)}, diff --git a/src/commands/acl-delrole.json b/src/commands/acl-delrole.json new file mode 100644 index 00000000000..18a5a7ee876 --- /dev/null +++ b/src/commands/acl-delrole.json @@ -0,0 +1,38 @@ +{ + "DELROLE": { + "summary": "Deletes one or more ACL roles. Fails if any role has members.", + "complexity": "O(N). Where N is the number of roles to delete.", + "group": "server", + "since": "10.0.0", + "arity": -3, + "container": "ACL", + "function": "aclCommand", + "command_flags": [ + "ADMIN", + "NOSCRIPT", + "LOADING", + "STALE", + "SENTINEL" + ], + "command_tips": [ + "REQUEST_POLICY:ALL_NODES", + "RESPONSE_POLICY:ALL_SUCCEEDED" + ], + "reply_schema": { + "type": "integer", + "description": "The number of roles deleted." + }, + "arguments": [ + { + "name": "rolename", + "type": "string", + "multiple": true + } + ], + "acl_categories": [ + "ADMIN", + "DANGEROUS", + "SLOW" + ] + } +} diff --git a/src/commands/acl-getrole.json b/src/commands/acl-getrole.json new file mode 100644 index 00000000000..4b3e1df04c8 --- /dev/null +++ b/src/commands/acl-getrole.json @@ -0,0 +1,88 @@ +{ + "GETROLE": { + "summary": "Returns the ACL rules of an ACL role.", + "complexity": "O(N). Where N is the number of rules defined for the role.", + "group": "server", + "since": "10.0.0", + "arity": 3, + "container": "ACL", + "function": "aclCommand", + "command_flags": [ + "ADMIN", + "NOSCRIPT", + "LOADING", + "STALE", + "SENTINEL" + ], + "reply_schema": { + "oneOf": [ + { + "description": "A set of ACL rule definitions for the role.", + "type": "object", + "additionalProperties": false, + "properties": { + "commands": { + "description": "Root selector's commands.", + "type": "string" + }, + "keys": { + "description": "Root selector's keys.", + "type": "string" + }, + "channels": { + "description": "Root selector's channels.", + "type": "string" + }, + "databases": { + "description": "Root selector's databases.", + "type": "string" + }, + "selectors": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "properties": { + "commands": { + "type": "string" + }, + "keys": { + "type": "string" + }, + "channels": { + "type": "string" + }, + "databases": { + "type": "string" + } + } + } + }, + "members": { + "description": "List of usernames assigned to this role.", + "type": "array", + "items": { + "type": "string" + } + } + } + }, + { + "description": "Role not found.", + "type": "null" + } + ] + }, + "arguments": [ + { + "name": "rolename", + "type": "string" + } + ], + "acl_categories": [ + "ADMIN", + "DANGEROUS", + "SLOW" + ] + } +} diff --git a/src/commands/acl-getuser.json b/src/commands/acl-getuser.json index c235d4172cd..3887dd03ba8 100644 --- a/src/commands/acl-getuser.json +++ b/src/commands/acl-getuser.json @@ -19,6 +19,10 @@ [ "9.1.0", "Added database permission rules." + ], + [ + "10.0.0", + "Added roles." ] ], "command_flags": [ @@ -89,6 +93,13 @@ } } } + }, + "roles": { + "description": "List of role names assigned to this user.", + "type": "array", + "items": { + "type": "string" + } } } }, diff --git a/src/commands/acl-roles.json b/src/commands/acl-roles.json new file mode 100644 index 00000000000..21556c5264e --- /dev/null +++ b/src/commands/acl-roles.json @@ -0,0 +1,30 @@ +{ + "ROLES": { + "summary": "Lists all ACL roles.", + "complexity": "O(N). Where N is the number of configured roles.", + "group": "server", + "since": "10.0.0", + "arity": 2, + "container": "ACL", + "function": "aclCommand", + "command_flags": [ + "ADMIN", + "NOSCRIPT", + "LOADING", + "STALE", + "SENTINEL" + ], + "reply_schema": { + "type": "array", + "description": "List of existing ACL roles.", + "items": { + "type": "string" + } + }, + "acl_categories": [ + "ADMIN", + "DANGEROUS", + "SLOW" + ] + } +} diff --git a/src/commands/acl-setrole.json b/src/commands/acl-setrole.json new file mode 100644 index 00000000000..30373ebedd6 --- /dev/null +++ b/src/commands/acl-setrole.json @@ -0,0 +1,42 @@ +{ + "SETROLE": { + "summary": "Creates and modifies an ACL role and its rules.", + "complexity": "O(N). Where N is the number of rules provided.", + "group": "server", + "since": "10.0.0", + "arity": -3, + "container": "ACL", + "function": "aclCommand", + "command_flags": [ + "ADMIN", + "NOSCRIPT", + "LOADING", + "STALE", + "SENTINEL" + ], + "command_tips": [ + "REQUEST_POLICY:ALL_NODES", + "RESPONSE_POLICY:ALL_SUCCEEDED" + ], + "reply_schema": { + "const": "OK" + }, + "arguments": [ + { + "name": "rolename", + "type": "string" + }, + { + "name": "rule", + "type": "string", + "optional": true, + "multiple": true + } + ], + "acl_categories": [ + "ADMIN", + "DANGEROUS", + "SLOW" + ] + } +} diff --git a/src/config.c b/src/config.c index 75f5ec8db25..7aaaecccd95 100644 --- a/src/config.c +++ b/src/config.c @@ -569,11 +569,19 @@ void loadServerConfigFromString(sds config) { } else if (!strcasecmp(argv[0], "user") && argc >= 2) { int argc_err; if (ACLAppendUserForLoading(argv, argc, &argc_err) == C_ERR) { - const char *errmsg = ACLSetUserStringError(); + const char *errmsg = ACLSetStringError(); snprintf(buf, sizeof(buf), "Error in user declaration '%s': %s", argv[argc_err], errmsg); err = buf; goto loaderr; } + } else if (!strcasecmp(argv[0], "role") && argc >= 2) { + int argc_err; + if (ACLAppendRoleForLoading(argv, argc, &argc_err) == C_ERR) { + const char *errmsg = ACLSetStringError(); + snprintf(buf, sizeof(buf), "Error in role declaration '%s': %s", argv[argc_err], errmsg); + err = buf; + goto loaderr; + } } else if (!strcasecmp(argv[0], "loadmodule") && argc >= 2) { moduleEnqueueLoadModule(argv[1], &argv[2], argc - 2); } else if (strchr(argv[0], '.')) { diff --git a/src/server.h b/src/server.h index 00e4a914720..263dab2bee4 100644 --- a/src/server.h +++ b/src/server.h @@ -1030,6 +1030,13 @@ typedef struct readyList { #define SELECTOR_FLAG_ALLDBS (1 << 4) /* Allow all databases */ +typedef struct role { + sds name; /* Role name */ + list *selectors; /* List of selectors (same structure as user selectors) */ + list *members; /* List of user pointers */ + robj *acl_string; /* Cached ACL string representation */ +} role; + typedef struct user { sds name; /* The username as an SDS string. */ uint32_t flags; /* See USER_FLAG_* */ @@ -1037,6 +1044,7 @@ typedef struct user { list *selectors; /* A list of selectors this user validates commands against. This list will always contain at least one selector for backwards compatibility. */ + list *roles; /* A list of roles assigned to this user. */ robj *acl_string; /* cached string represent of ACLs */ } user; @@ -3355,7 +3363,7 @@ 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); +const char *ACLSetStringError(void); robj *ACLDescribeUser(user *u); void ACLLoadUsersAtStartup(void); void addReplyCommandCategories(client *c, struct serverCommand *cmd); @@ -3366,6 +3374,9 @@ sds getAclErrorMessage(int acl_res, user *user, struct serverCommand *cmd, sds e void ACLUpdateDefaultUserPassword(sds password); sds genValkeyInfoStringACLStats(sds info); void ACLRecomputeCommandBitsFromCommandRulesAllUsers(void); +role *ACLGetRoleByName(const char *name, size_t namelen); +int ACLAppendRoleForLoading(sds *argv, int argc, int *argc_err); +void ACLLoadRolesAtStartup(void); /* Sorted sets data type */ diff --git a/tests/assets/role.acl b/tests/assets/role.acl new file mode 100644 index 00000000000..9f85bf13bd9 --- /dev/null +++ b/tests/assets/role.acl @@ -0,0 +1,7 @@ +role customer ~* &* +@all -@admin -@dangerous -@scripting +role readonly ~* &* +@read + +user alice on >alice +@role:customer +user bob on >bob +@role:readonly +user carol on >carol +@role:customer +eval +user default on nopass ~* &* +@all diff --git a/tests/unit/acl-role.tcl b/tests/unit/acl-role.tcl new file mode 100644 index 00000000000..6306925ea85 --- /dev/null +++ b/tests/unit/acl-role.tcl @@ -0,0 +1,279 @@ +start_server {tags {"acl external:skip"}} { + test {ACL ROLES - initially empty} { + r ACL ROLES + } {} + + test {ACL SETROLE - create a role} { + r ACL SETROLE myrole ~keys:* +@all -@dangerous + } {OK} + + test {ACL ROLES - lists the role} { + r ACL ROLES + } {myrole} + + test {ACL GETROLE - returns role info} { + set info [r ACL GETROLE myrole] + assert_match {*commands*} $info + assert_match {*keys*} $info + } + + test {ACL GETROLE - non-existent role returns nil} { + r ACL GETROLE nonexistent + } {} + + test {ACL SETROLE - update existing role} { + r ACL SETROLE myrole ~keys:* +@all -@dangerous -@scripting + } {OK} + + test {ACL SETROLE - rejects password operations} { + catch {r ACL SETROLE myrole >password} err + assert_match {*Error*} $err + } + + test {ACL SETROLE - rejects on/off flags} { + catch {r ACL SETROLE myrole on} err + assert_match {*Error*} $err + + catch {r ACL SETROLE myrole off} err + assert_match {*Error*} $err + } + + test {ACL SETROLE - rejects nested roles} { + r ACL SETROLE otherrole +@read + catch {r ACL SETROLE myrole +@role:otherrole} err + assert_match {*Error*} $err + } + + test {ACL SETUSER - add user to role} { + r ACL SETUSER alice on >pass123 +@role:myrole + } {OK} + + test {ACL GETUSER - shows role membership} { + set info [r ACL GETUSER alice] + set idx [lsearch $info "roles"] + set roles [lindex $info [expr {$idx + 1}]] + assert_equal $roles {myrole} + } + + test {Role permissions are effective for user} { + r AUTH alice pass123 + + r SET keys:hello world + assert_equal [r GET keys:hello] world + + catch {r SET other:key value} err + assert_match {*NOPERM*} $err + } {} {needs:reset} + + test {ACL DRYRUN respects role permissions} { + r AUTH default "" + + assert_equal [r ACL DRYRUN alice SET keys:test value] {OK} + + set result [r ACL DRYRUN alice SET other:test value] + assert_match {*no permissions*} $result + } + + test {ACL SETUSER - remove user from role} { + r ACL SETUSER alice -@role:myrole + set info [r ACL GETUSER alice] + set idx [lsearch $info "roles"] + set roles [lindex $info [expr {$idx + 1}]] + assert_equal $roles {} + } + + test {After removing from role, permissions are revoked} { + set result [r ACL DRYRUN alice SET keys:test value] + assert_match {*no permissions*} $result + } + + test {ACL DELROLE - fails if role has members} { + r ACL SETUSER bob on >pass456 +@role:otherrole + catch {r ACL DELROLE otherrole} err + assert_match {*has members*} $err + } + + test {ACL DELROLE - succeeds when no members} { + r ACL SETUSER bob -@role:otherrole + r ACL DELROLE otherrole + } {1} + + test {ACL DELROLE - non-existent role returns error} { + catch {r ACL DELROLE nonexistent} err + assert_match {*not found*} $err + } + + test {ACL DELROLE - delete multiple roles at once} { + r ACL SETROLE delA +get + r ACL SETROLE delB +set + r ACL SETROLE delC +del + assert_equal [r ACL DELROLE delA delB delC] 3 + # Verify they're all gone + assert_equal [r ACL GETROLE delA] {} + assert_equal [r ACL GETROLE delB] {} + assert_equal [r ACL GETROLE delC] {} + } + + test {Role changes are immediately visible to members} { + r ACL SETROLE liverole +@all ~* + r ACL SETUSER carol on >carolpass +@role:liverole + # Carol can do anything now + assert_equal [r ACL DRYRUN carol SET anykey value] {OK} + # Update role to restrict keys + r ACL SETROLE liverole resetkeys +@all ~restricted:* + # Carol should now only access restricted:* keys + catch {r ACL DRYRUN carol SET anykey value} err + assert_match {*no permissions*} $err + assert_equal [r ACL DRYRUN carol SET restricted:key value] {OK} + } + + test {ACL LIST includes roles} { + set list [r ACL LIST] + assert_match "role *" [lindex $list 0] + } + + test {User reset clears role memberships} { + r ACL SETUSER carol reset + set info [r ACL GETUSER carol] + set idx [lsearch $info "roles"] + set roles [lindex $info [expr {$idx + 1}]] + assert_equal $roles {} + } + + test {ACL SETUSER - referencing non-existent role fails} { + catch {r ACL SETUSER dave on >pass +@role:nosuchrole} err + assert_match {*role does not exist*} $err + } + + test {Multiple roles - each role is a separate selector with OR logic} { + r ACL SETROLE roleA +get ~a:* + r ACL SETROLE roleB +set ~b:* + r ACL SETUSER multi on >multipass +@role:roleA +@role:roleB + + # roleA allows GET on a:* keys + assert_equal [r ACL DRYRUN multi GET a:key] {OK} + # roleB allows SET on b:* keys + assert_equal [r ACL DRYRUN multi SET b:key value] {OK} + + # GET b:key is denied + set result [r ACL DRYRUN multi GET b:key] + assert_match {*no permissions*} $result + # Keys outside both roles are denied + set result [r ACL DRYRUN multi GET c:key] + assert_match {*no permissions*} $result + } + + test {Role with multiple selectors} { + # Create a role with two selectors: one for reads on r:*, one for writes on w:* + r ACL SETROLE multiselector +get ~r:* (+set ~w:*) + r ACL SETUSER msuser on >mspass +@role:multiselector + + # First selector allows GET on r:* + assert_equal [r ACL DRYRUN msuser GET r:key] {OK} + # Second selector allows SET on w:* + assert_equal [r ACL DRYRUN msuser SET w:key value] {OK} + + # Cross-selector: GET on w:* is denied (no single selector allows it) + set result [r ACL DRYRUN msuser GET w:key] + assert_match {*no permissions*} $result + # SET on r:* is also denied + set result [r ACL DRYRUN msuser SET r:key value] + assert_match {*no permissions*} $result + } + + test {User own permissions add on top of role (OR logic)} { + r ACL SETROLE onlyset +set ~data:* + r ACL SETUSER userplus on >pluspass +@role:onlyset +get ~data:* + + # Role allows SET on data:*, user's own selector allows GET on data:* + assert_equal [r ACL DRYRUN userplus SET data:key value] {OK} + assert_equal [r ACL DRYRUN userplus GET data:key] {OK} + + # Neither allows DEL + set result [r ACL DRYRUN userplus DEL data:key] + assert_match {*no permissions*} $result + } + + test {User cannot restrict role permissions} { + r ACL SETROLE permissive +@all ~* + r ACL SETUSER restricted on >rpass +@role:permissive -@admin + + # Even though user has no admin permissions, the role grants it + assert_equal [r ACL DRYRUN restricted FLUSHALL] {OK} + } + + test {Role name validation - no spaces} { + catch {r ACL SETROLE "bad name" +@all} err + assert_match {*can't contain spaces*} $err + } + + # Cleanup + test {Cleanup test users and roles} { + # Remove all non-default users (which also drops their role memberships) + foreach entry [r ACL LIST] { + if {[string match "user *" $entry]} { + set uname [lindex $entry 1] + if {$uname ne "default"} { + catch {r ACL DELUSER $uname} + } + } + } + # Now delete all roles (no members left) + foreach role [r ACL ROLES] { + catch {r ACL DELROLE $role} + } + } +} + +# Test loading roles from ACL file +set server_path [tmpdir "server.role.acl"] +exec cp -f tests/assets/role.acl $server_path +start_server [list overrides [list "dir" $server_path "aclfile" "role.acl"] tags [list "external:skip"]] { + + test {Roles loaded from ACL file} { + lsort [r ACL ROLES] + } {customer readonly} + + test {Users loaded with role assignments from ACL file} { + set info [r ACL GETUSER alice] + set idx [lsearch $info "roles"] + set roles [lindex $info [expr {$idx + 1}]] + assert_equal $roles {customer} + } + + test {Role permissions work after loading from ACL file} { + # alice has customer role: all commands except admin/dangerous/scripting + assert_equal [r ACL DRYRUN alice SET anykey value] {OK} + + set result [r ACL DRYRUN alice FLUSHALL] + assert_match {*no permissions*} $result + } + + test {User-level permissions add on top of role from ACL file} { + assert_equal [r ACL DRYRUN carol EVAL "return 1" 0] {OK} + set result [r ACL DRYRUN alice EVAL "return 1" 0] + assert_match {*no permissions*} $result + } + + test {ACL SAVE and reload preserves roles} { + r ACL SAVE + r ACL LOAD + lsort [r ACL ROLES] + } {customer readonly} +} + +# Test loading roles from valkey.conf inline directives +set conf_lines [list "role" "inlinerole ~* +@read" "user" "inlineuser on >ipass +@role:inlinerole"] +start_server [list config_lines $conf_lines tags [list "external:skip"]] { + + test {Roles loaded from valkey.conf inline directives} { + r ACL ROLES + } {inlinerole} + + test {User with role from valkey.conf works} { + assert_equal [r ACL DRYRUN inlineuser GET anykey] {OK} + + set result [r ACL DRYRUN inlineuser SET anykey value] + assert_match {*no permissions*} $result + } +} From 05ae997285cbf0cfffb7deb825a63f6d5ab74fd0 Mon Sep 17 00:00:00 2001 From: Yang Zhao Date: Mon, 15 Jun 2026 17:46:36 -0700 Subject: [PATCH 2/4] Feedbacks Signed-off-by: Yang Zhao --- src/acl.c | 112 +++++++++++++++++----------------- src/commands.def | 3 +- src/commands/acl-setuser.json | 4 ++ src/config.c | 2 +- src/server.h | 1 - tests/unit/acl-role.tcl | 7 +++ 6 files changed, 70 insertions(+), 59 deletions(-) diff --git a/src/acl.c b/src/acl.c index b5970c18e7a..d274484af39 100644 --- a/src/acl.c +++ b/src/acl.c @@ -480,6 +480,34 @@ user *ACLCreateUnlinkedUser(void) { } } +/* Remove user from all roles' member lists and release the roles list. */ +static void ACLUserClearRoles(user *u) { + if (!u->roles) return; + listIter li; + listNode *ln; + listRewind(u->roles, &li); + while ((ln = listNext(&li))) { + role *r = listNodeValue(ln); + listNode *member_ln = listSearchKey(r->members, u); + if (member_ln) listDelNode(r->members, member_ln); + } + listRelease(u->roles); + u->roles = NULL; +} + +/* Copy role assignments from src to dst, registering dst as a member of each role. */ +static void ACLCopyRoles(user *dst, user *src) { + if (!src->roles) return; + listIter li; + listNode *ln; + listRewind(src->roles, &li); + while ((ln = listNext(&li))) { + role *r = listNodeValue(ln); + listAddNodeTail(dst->roles, r); + listAddNodeTail(r->members, dst); + } +} + /* Release the memory used by the user structure. Note that this function * will not remove the user from the Users global radix tree. */ void ACLFreeUser(user *u) { @@ -491,18 +519,7 @@ void ACLFreeUser(user *u) { listRelease(u->passwords); listRelease(u->selectors); - /* Remove this user from all roles' member lists */ - if (u->roles) { - listIter li; - listNode *ln; - listRewind(u->roles, &li); - while ((ln = listNext(&li))) { - role *r = listNodeValue(ln); - listNode *member_ln = listSearchKey(r->members, u); - if (member_ln) listDelNode(r->members, member_ln); - } - listRelease(u->roles); - } + ACLUserClearRoles(u); zfree(u); } @@ -554,28 +571,9 @@ static void ACLCopyUser(user *dst, user *src) { incrRefCount(dst->acl_string); } /* Clean up dst's existing role memberships, then copy from src. */ - if (dst->roles) { - listIter li; - listNode *ln; - listRewind(dst->roles, &li); - while ((ln = listNext(&li))) { - role *r = listNodeValue(ln); - listNode *member_ln = listSearchKey(r->members, dst); - if (member_ln) listDelNode(r->members, member_ln); - } - listRelease(dst->roles); - } + ACLUserClearRoles(dst); dst->roles = listCreate(); - if (src->roles) { - listIter li; - listNode *ln; - listRewind(src->roles, &li); - while ((ln = listNext(&li))) { - role *r = listNodeValue(ln); - listAddNodeTail(dst->roles, r); - listAddNodeTail(r->members, dst); - } - } + ACLCopyRoles(dst, src); } /* ============================================================================= @@ -728,10 +726,7 @@ static sds ACLStringSetRole(role *r, sds rolename, sds *argv, int argc) { /* Apply changes: if role doesn't exist, create it; otherwise update it */ if (!r) { r = ACLCreateRole(rolename, sdslen(rolename)); - if (!r) { - error = sdsnew("Role already exists"); - goto cleanup; - } + serverAssert(r != NULL); } /* Copy selectors from temp role to the actual role. @@ -1756,18 +1751,9 @@ int ACLSetUser(user *u, const char *op, ssize_t oplen) { serverAssert(ACLSetUser(u, "off", -1) == C_OK); serverAssert(ACLSetUser(u, "clearselectors", -1) == C_OK); serverAssert(ACLSetUser(u, "-@all", -1) == C_OK); - /* Clear role memberships on reset */ - if (u->roles) { - listIter li; - listNode *ln; - listRewind(u->roles, &li); - while ((ln = listNext(&li))) { - role *r = listNodeValue(ln); - listNode *member_ln = listSearchKey(r->members, u); - if (member_ln) listDelNode(r->members, member_ln); - } - listEmpty(u->roles); - } + + ACLUserClearRoles(u); + u->roles = listCreate(); } else if (oplen > 7 && !strncasecmp(op, "+@role:", 7)) { /* Add user to a role */ const char *rolename = op + 7; @@ -1848,7 +1834,7 @@ const char *ACLSetStringError(void) { errmsg = "The password hash must be exactly 64 characters and contain " "only lowercase hexadecimal characters"; else if (errno == EALREADY) - errmsg = "Duplicate user found. A user can only be defined once in " + errmsg = "Duplicate definition found. A user or role can only be defined once in " "config files"; else if (errno == ECHILD) errmsg = "Allowing first-arg of a subcommand is not supported"; @@ -2422,7 +2408,7 @@ int ACLCheckAllUserCommandPerm(user *u, struct serverCommand *cmd, robj **argv, } /* Check role selectors */ - if (u->roles) { + if (u->roles && listLength(u->roles) > 0) { listIter rli; listNode *rln; listRewind(u->roles, &rli); @@ -2881,6 +2867,25 @@ int ACLAppendRoleForLoading(sds *argv, int argc, int *argc_err) { return C_ERR; } + /* Try to apply the role rules in a fake role to validate them. */ + role fakeRole = {0}; + fakeRole.selectors = listCreate(); + listSetFreeMethod(fakeRole.selectors, ACLListFreeSelector); + aclSelector *s = ACLCreateSelector(SELECTOR_FLAG_ROOT); + listAddNodeHead(fakeRole.selectors, s); + + for (int j = 0; j < merged_argc; j++) { + if (ACLSetRole(&fakeRole, acl_args[j], sdslen(acl_args[j])) == C_ERR) { + listRelease(fakeRole.selectors); + if (argc_err) *argc_err = j + 2; + for (int i = 0; i < merged_argc; i++) sdsfree(acl_args[i]); + zfree(acl_args); + return C_ERR; + } + } + listRelease(fakeRole.selectors); + + /* Rules look valid, store for deferred loading. */ sds *copy = zmalloc(sizeof(sds) * (merged_argc + 2)); copy[0] = sdsdup(argv[1]); for (int j = 0; j < merged_argc; j++) copy[j + 1] = sdsdup(acl_args[j]); @@ -3341,11 +3346,6 @@ void ACLLoadUsersAtStartup(void) { } /* Also provide a function to load roles at startup from config */ -void ACLLoadRolesAtStartup(void) { - /* Roles are already loaded in ACLLoadUsersAtStartup before users. - * This function is a no-op placeholder for external callers. */ -} - /* ============================================================================= * ACL log * ==========================================================================*/ diff --git a/src/commands.def b/src/commands.def index cbd8068f5e7..f84df80d68a 100644 --- a/src/commands.def +++ b/src/commands.def @@ -7149,6 +7149,7 @@ commandHistory ACL_SETUSER_History[] = { {"6.2.0","Added Pub/Sub channel patterns."}, {"7.0.0","Added selectors and key based permissions."}, {"9.1.0","Added database permission rules."}, +{"10.0.0","Added role assignment and removal."}, }; #endif @@ -7221,7 +7222,7 @@ struct COMMAND_STRUCT ACL_Subcommands[] = { {MAKE_CMD("roles","Lists all ACL roles.","O(N). Where N is the number of configured roles.","10.0.0",CMD_DOC_NONE,NULL,NULL,"server",COMMAND_GROUP_SERVER,ACL_ROLES_History,0,ACL_ROLES_Tips,0,aclCommand,2,CMD_ADMIN|CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_SENTINEL,ACL_CATEGORY_ADMIN|ACL_CATEGORY_DANGEROUS|ACL_CATEGORY_SLOW,NULL,ACL_ROLES_Keyspecs,0,NULL,0)}, {MAKE_CMD("save","Saves the effective ACL rules in the configured ACL file.","O(N). Where N is the number of configured users.","6.0.0",CMD_DOC_NONE,NULL,NULL,"server",COMMAND_GROUP_SERVER,ACL_SAVE_History,0,ACL_SAVE_Tips,2,aclCommand,2,CMD_ADMIN|CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_SENTINEL,ACL_CATEGORY_ADMIN|ACL_CATEGORY_DANGEROUS|ACL_CATEGORY_SLOW,NULL,ACL_SAVE_Keyspecs,0,NULL,0)}, {MAKE_CMD("setrole","Creates and modifies an ACL role and its rules.","O(N). Where N is the number of rules provided.","10.0.0",CMD_DOC_NONE,NULL,NULL,"server",COMMAND_GROUP_SERVER,ACL_SETROLE_History,0,ACL_SETROLE_Tips,2,aclCommand,-3,CMD_ADMIN|CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_SENTINEL,ACL_CATEGORY_ADMIN|ACL_CATEGORY_DANGEROUS|ACL_CATEGORY_SLOW,NULL,ACL_SETROLE_Keyspecs,0,NULL,2),.args=ACL_SETROLE_Args}, -{MAKE_CMD("setuser","Creates and modifies an ACL user and its rules.","O(N). Where N is the number of rules provided.","6.0.0",CMD_DOC_NONE,NULL,NULL,"server",COMMAND_GROUP_SERVER,ACL_SETUSER_History,3,ACL_SETUSER_Tips,2,aclCommand,-3,CMD_ADMIN|CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_SENTINEL,ACL_CATEGORY_ADMIN|ACL_CATEGORY_DANGEROUS|ACL_CATEGORY_SLOW,NULL,ACL_SETUSER_Keyspecs,0,NULL,2),.args=ACL_SETUSER_Args}, +{MAKE_CMD("setuser","Creates and modifies an ACL user and its rules.","O(N). Where N is the number of rules provided.","6.0.0",CMD_DOC_NONE,NULL,NULL,"server",COMMAND_GROUP_SERVER,ACL_SETUSER_History,4,ACL_SETUSER_Tips,2,aclCommand,-3,CMD_ADMIN|CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_SENTINEL,ACL_CATEGORY_ADMIN|ACL_CATEGORY_DANGEROUS|ACL_CATEGORY_SLOW,NULL,ACL_SETUSER_Keyspecs,0,NULL,2),.args=ACL_SETUSER_Args}, {MAKE_CMD("users","Lists all ACL users.","O(N). Where N is the number of configured users.","6.0.0",CMD_DOC_NONE,NULL,NULL,"server",COMMAND_GROUP_SERVER,ACL_USERS_History,0,ACL_USERS_Tips,0,aclCommand,2,CMD_ADMIN|CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_SENTINEL,ACL_CATEGORY_ADMIN|ACL_CATEGORY_DANGEROUS|ACL_CATEGORY_SLOW,NULL,ACL_USERS_Keyspecs,0,NULL,0)}, {MAKE_CMD("whoami","Returns the authenticated username of the current connection.","O(1)","6.0.0",CMD_DOC_NONE,NULL,NULL,"server",COMMAND_GROUP_SERVER,ACL_WHOAMI_History,0,ACL_WHOAMI_Tips,0,aclCommand,2,CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_SENTINEL,ACL_CATEGORY_SLOW,NULL,ACL_WHOAMI_Keyspecs,0,NULL,0)}, {0} diff --git a/src/commands/acl-setuser.json b/src/commands/acl-setuser.json index bd0dce481ef..efcd5c1fc61 100644 --- a/src/commands/acl-setuser.json +++ b/src/commands/acl-setuser.json @@ -19,6 +19,10 @@ [ "9.1.0", "Added database permission rules." + ], + [ + "10.0.0", + "Added role assignment and removal." ] ], "command_flags": [ diff --git a/src/config.c b/src/config.c index 7aaaecccd95..f1c0d7d9ffa 100644 --- a/src/config.c +++ b/src/config.c @@ -1186,7 +1186,7 @@ struct rewriteConfigState *rewriteConfigReadOldFile(char *path) { /* The following is a list of config features that are only supported in * config file parsing and are not recognized by lookupConfig */ strcasecmp(argv[0], "include") && strcasecmp(argv[0], "rename-command") && strcasecmp(argv[0], "user") && - strcasecmp(argv[0], "loadmodule") && strcasecmp(argv[0], "sentinel"))) { + strcasecmp(argv[0], "role") && strcasecmp(argv[0], "loadmodule") && strcasecmp(argv[0], "sentinel"))) { /* The line is either unparsable for some reason, for * instance it may have unbalanced quotes, may contain a * config that doesn't exist anymore, for instance a module that got diff --git a/src/server.h b/src/server.h index 263dab2bee4..ec27e8ec5cd 100644 --- a/src/server.h +++ b/src/server.h @@ -3376,7 +3376,6 @@ sds genValkeyInfoStringACLStats(sds info); void ACLRecomputeCommandBitsFromCommandRulesAllUsers(void); role *ACLGetRoleByName(const char *name, size_t namelen); int ACLAppendRoleForLoading(sds *argv, int argc, int *argc_err); -void ACLLoadRolesAtStartup(void); /* Sorted sets data type */ diff --git a/tests/unit/acl-role.tcl b/tests/unit/acl-role.tcl index 6306925ea85..127560e9657 100644 --- a/tests/unit/acl-role.tcl +++ b/tests/unit/acl-role.tcl @@ -276,4 +276,11 @@ start_server [list config_lines $conf_lines tags [list "external:skip"]] { set result [r ACL DRYRUN inlineuser SET anykey value] assert_match {*no permissions*} $result } + + test {CONFIG REWRITE preserves role directives} { + r CONFIG REWRITE + set cfg [exec cat [srv 0 config_file]] + assert_match {*role inlinerole*} $cfg + assert_match {*user inlineuser*} $cfg + } } From 063103ebccadb1e257c02796e8e3d20807ec194f Mon Sep 17 00:00:00 2001 From: Yang Zhao Date: Mon, 15 Jun 2026 21:32:59 -0700 Subject: [PATCH 3/4] Fix error match in test Signed-off-by: Yang Zhao --- tests/unit/acl.tcl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/unit/acl.tcl b/tests/unit/acl.tcl index 20e0cba12b0..1f809c4ea7d 100644 --- a/tests/unit/acl.tcl +++ b/tests/unit/acl.tcl @@ -1346,10 +1346,10 @@ start_server [list overrides [list "dir" $server_path "aclfile" "user.acl"] tags test {Test loading duplicate users in config on startup} { catch {exec $::VALKEY_SERVER_BIN --user foo --user foo} err - assert_match {*Duplicate user*} $err + assert_match {*Duplicate definition*} $err catch {exec $::VALKEY_SERVER_BIN --user default --user default} err - assert_match {*Duplicate user*} $err + assert_match {*Duplicate definition*} $err } {} {external:skip} } From 7f27dc0109fd1ba91a6e42426e091a612d1ff234 Mon Sep 17 00:00:00 2001 From: Yang Zhao Date: Tue, 16 Jun 2026 16:42:46 -0700 Subject: [PATCH 4/4] Add test coverage Signed-off-by: Yang Zhao --- tests/unit/acl-role.tcl | 215 ++++++++++++++++++++++++++++++---------- 1 file changed, 160 insertions(+), 55 deletions(-) diff --git a/tests/unit/acl-role.tcl b/tests/unit/acl-role.tcl index 127560e9657..0ccc8aced81 100644 --- a/tests/unit/acl-role.tcl +++ b/tests/unit/acl-role.tcl @@ -3,6 +3,8 @@ start_server {tags {"acl external:skip"}} { r ACL ROLES } {} + # --- ACL SETROLE --- + test {ACL SETROLE - create a role} { r ACL SETROLE myrole ~keys:* +@all -@dangerous } {OK} @@ -11,16 +13,6 @@ start_server {tags {"acl external:skip"}} { r ACL ROLES } {myrole} - test {ACL GETROLE - returns role info} { - set info [r ACL GETROLE myrole] - assert_match {*commands*} $info - assert_match {*keys*} $info - } - - test {ACL GETROLE - non-existent role returns nil} { - r ACL GETROLE nonexistent - } {} - test {ACL SETROLE - update existing role} { r ACL SETROLE myrole ~keys:* +@all -@dangerous -@scripting } {OK} @@ -44,36 +36,58 @@ start_server {tags {"acl external:skip"}} { assert_match {*Error*} $err } - test {ACL SETUSER - add user to role} { - r ACL SETUSER alice on >pass123 +@role:myrole - } {OK} - - test {ACL GETUSER - shows role membership} { - set info [r ACL GETUSER alice] - set idx [lsearch $info "roles"] - set roles [lindex $info [expr {$idx + 1}]] - assert_equal $roles {myrole} + test {ACL SETROLE - unmatched parenthesis} { + catch {r ACL SETROLE badrole (+get} err + assert_match {*Unmatched parenthesis*} $err } - test {Role permissions are effective for user} { - r AUTH alice pass123 + test {ACL SETROLE - clearselectors removes non-root selectors} { + r ACL SETROLE multisel +get ~a:* (+set ~b:*) + r ACL SETROLE multisel clearselectors +get ~a:* + # After clearselectors, only root selector remains (no extra selectors) + set info [r ACL GETROLE multisel] + set idx [lsearch $info "selectors"] + set sels [lindex $info [expr {$idx + 1}]] + assert_equal [llength $sels] 0 + } - r SET keys:hello world - assert_equal [r GET keys:hello] world + test {ACL SETROLE - role name validation - no spaces} { + catch {r ACL SETROLE "bad name" +@all} err + assert_match {*can't contain spaces*} $err + } - catch {r SET other:key value} err - assert_match {*NOPERM*} $err - } {} {needs:reset} + # --- ACL GETROLE --- - test {ACL DRYRUN respects role permissions} { - r AUTH default "" + test {ACL GETROLE - returns role info} { + set info [r ACL GETROLE myrole] + assert_match {*commands*} $info + assert_match {*keys*} $info + } - assert_equal [r ACL DRYRUN alice SET keys:test value] {OK} + test {ACL GETROLE - non-existent role returns nil} { + r ACL GETROLE nonexistent + } {} - set result [r ACL DRYRUN alice SET other:test value] - assert_match {*no permissions*} $result + test {ACL GETROLE - shows selectors and members} { + r ACL SETROLE inforole +get ~info:* (+set ~info:*) + r ACL SETUSER infouser on >infopass +@role:inforole + set info [r ACL GETROLE inforole] + # Check members list + set idx [lsearch $info "members"] + set members [lindex $info [expr {$idx + 1}]] + assert_equal $members {infouser} + # Check selectors (should have one extra selector beyond root) + set idx [lsearch $info "selectors"] + set sels [lindex $info [expr {$idx + 1}]] + assert_equal [llength $sels] 1 } + # --- ACL SETUSER --- + + test {ACL SETUSER - add user to role} { + r ACL SETUSER alice on >pass123 +@role:myrole + } {OK} + test {ACL SETUSER - remove user from role} { r ACL SETUSER alice -@role:myrole set info [r ACL GETUSER alice] @@ -82,11 +96,28 @@ start_server {tags {"acl external:skip"}} { assert_equal $roles {} } - test {After removing from role, permissions are revoked} { - set result [r ACL DRYRUN alice SET keys:test value] - assert_match {*no permissions*} $result + test {ACL SETUSER - referencing non-existent role fails} { + catch {r ACL SETUSER dave on >pass +@role:nosuchrole} err + assert_match {*role does not exist*} $err } + test {ACL SETUSER - remove from non-existent role fails} { + catch {r ACL SETUSER alice -@role:nosuchrole} err + assert_match {*role does not exist*} $err + } + + # --- ACL GETUSER --- + + test {ACL GETUSER - shows role membership} { + r ACL SETUSER alice +@role:myrole + set info [r ACL GETUSER alice] + set idx [lsearch $info "roles"] + set roles [lindex $info [expr {$idx + 1}]] + assert_equal $roles {myrole} + } + + # --- ACL DELROLE --- + test {ACL DELROLE - fails if role has members} { r ACL SETUSER bob on >pass456 +@role:otherrole catch {r ACL DELROLE otherrole} err @@ -114,6 +145,33 @@ start_server {tags {"acl external:skip"}} { assert_equal [r ACL GETROLE delC] {} } + # --- Permission checks --- + + test {Role permissions are effective for user} { + r AUTH alice pass123 + + r SET keys:hello world + assert_equal [r GET keys:hello] world + + catch {r SET other:key value} err + assert_match {*NOPERM*} $err + } {} {needs:reset} + + test {ACL DRYRUN respects role permissions} { + r AUTH default "" + + assert_equal [r ACL DRYRUN alice SET keys:test value] {OK} + + set result [r ACL DRYRUN alice SET other:test value] + assert_match {*no permissions*} $result + } + + test {After removing from role, permissions are revoked} { + r ACL SETUSER alice -@role:myrole + set result [r ACL DRYRUN alice SET keys:test value] + assert_match {*no permissions*} $result + } + test {Role changes are immediately visible to members} { r ACL SETROLE liverole +@all ~* r ACL SETUSER carol on >carolpass +@role:liverole @@ -127,24 +185,6 @@ start_server {tags {"acl external:skip"}} { assert_equal [r ACL DRYRUN carol SET restricted:key value] {OK} } - test {ACL LIST includes roles} { - set list [r ACL LIST] - assert_match "role *" [lindex $list 0] - } - - test {User reset clears role memberships} { - r ACL SETUSER carol reset - set info [r ACL GETUSER carol] - set idx [lsearch $info "roles"] - set roles [lindex $info [expr {$idx + 1}]] - assert_equal $roles {} - } - - test {ACL SETUSER - referencing non-existent role fails} { - catch {r ACL SETUSER dave on >pass +@role:nosuchrole} err - assert_match {*role does not exist*} $err - } - test {Multiple roles - each role is a separate selector with OR logic} { r ACL SETROLE roleA +get ~a:* r ACL SETROLE roleB +set ~b:* @@ -202,9 +242,29 @@ start_server {tags {"acl external:skip"}} { assert_equal [r ACL DRYRUN restricted FLUSHALL] {OK} } - test {Role name validation - no spaces} { - catch {r ACL SETROLE "bad name" +@all} err - assert_match {*can't contain spaces*} $err + test {Role with channel patterns} { + r ACL SETROLE channelrole +subscribe &news:* ~* + r ACL SETUSER chanuser on >chanpass +@role:channelrole + assert_equal [r ACL DRYRUN chanuser SUBSCRIBE news:sports] {OK} + set result [r ACL DRYRUN chanuser SUBSCRIBE private:msg] + assert_match {*no permissions*} $result + } + + # --- ACL LIST --- + + test {ACL LIST includes roles} { + set list [r ACL LIST] + assert_match "role *" [lindex $list 0] + } + + # --- User reset --- + + test {User reset clears role memberships} { + r ACL SETUSER carol reset + set info [r ACL GETUSER carol] + set idx [lsearch $info "roles"] + set roles [lindex $info [expr {$idx + 1}]] + assert_equal $roles {} } # Cleanup @@ -262,6 +322,35 @@ start_server [list overrides [list "dir" $server_path "aclfile" "role.acl"] tags } {customer readonly} } +# Test ACL file error paths for roles +set server_path [tmpdir "server.role.errors.acl"] +exec cp -f tests/assets/role.acl $server_path +start_server [list overrides [list "dir" $server_path "aclfile" "role.acl"] tags [list "external:skip"]] { + + test {ACL LOAD - role with invalid rules fails} { + set fd [open "$server_path/role.acl" w] + puts $fd "role badrole >password" + puts $fd "user default on nopass ~* &* +@all" + close $fd + catch {r ACL LOAD} err + assert_match {*Error*} $err + } + + test {ACL LOAD - role line without name fails} { + set fd [open "$server_path/role.acl" w] + puts $fd "role" + puts $fd "user default on nopass ~* &* +@all" + close $fd + catch {r ACL LOAD} err + assert_match {*requires a role name*} $err + } + + test {Restore valid ACL file} { + exec cp -f tests/assets/role.acl $server_path + r ACL LOAD + } +} + # Test loading roles from valkey.conf inline directives set conf_lines [list "role" "inlinerole ~* +@read" "user" "inlineuser on >ipass +@role:inlinerole"] start_server [list config_lines $conf_lines tags [list "external:skip"]] { @@ -284,3 +373,19 @@ start_server [list config_lines $conf_lines tags [list "external:skip"]] { assert_match {*user inlineuser*} $cfg } } + +# Test duplicate role in config on startup +test {Duplicate role in config on startup fails} { + catch {exec $::VALKEY_SERVER_BIN --role dup --role dup} err + assert_match {*Duplicate definition*} $err +} {} {external:skip} + +# Test invalid role rule in config on startup +test {Invalid role rule in config on startup fails} { + set tmpdir [tmpdir "badrole.conf"] + set fd [open "$tmpdir/valkey.conf" w] + puts $fd "role badrole >password" + close $fd + catch {exec $::VALKEY_SERVER_BIN "$tmpdir/valkey.conf"} err + assert_match {*Error in role declaration*} $err +} {} {external:skip}