From c7cd10dfe96e91582d7e341b20334464d5c51830 Mon Sep 17 00:00:00 2001 From: antirez Date: Thu, 31 Jan 2019 16:49:22 +0100 Subject: [PATCH 01/35] ACL: flags refactoring, function to describe user. --- src/acl.c | 91 ++++++++++++++++++++++++++++++++++++++++------------ src/server.h | 7 ++-- 2 files changed, 74 insertions(+), 24 deletions(-) diff --git a/src/acl.c b/src/acl.c index f112d8708..dce164f21 100644 --- a/src/acl.c +++ b/src/acl.c @@ -64,7 +64,19 @@ struct ACLCategoryItem { {"connection", CMD_CATEGORY_CONNECTION}, {"transaction", CMD_CATEGORY_TRANSACTION}, {"scripting", CMD_CATEGORY_SCRIPTING}, - {"",0} /* Terminator. */ + {NULL,0} /* Terminator. */ +}; + +struct ACLUserFlag { + const char *name; + uint64_t flag; +} ACLUserFlags[] = { + {"on", USER_FLAG_ENABLED}, + {"off", USER_FLAG_DISABLED}, + {"allkeys", USER_FLAG_ALLKEYS}, + {"allcommands", USER_FLAG_ALLCOMMANDS}, + {"nopass", USER_FLAG_NOPASS}, + {NULL,0} /* Terminator. */ }; void ACLResetSubcommandsForCommand(user *u, unsigned long id); @@ -150,7 +162,7 @@ user *ACLCreateUser(const char *name, size_t namelen) { if (raxFind(Users,(unsigned char*)name,namelen) != raxNotFound) return NULL; user *u = zmalloc(sizeof(*u)); u->name = sdsnewlen(name,namelen); - u->flags = 0; + u->flags = USER_FLAG_DISABLED; u->allowed_subcommands = NULL; u->passwords = listCreate(); u->patterns = listCreate(); @@ -360,6 +372,54 @@ sds ACLDescribeUserCommandRules(user *u) { return rules; } +/* This is similar to ACLDescribeUserCommandRules(), however instead of + * describing just the user command rules, everything is described: user + * flags, keys, passwords and finally the command rules obtained via + * the ACLDescribeUserCommandRules() function. This is the function we call + * when we want to rewrite the configuration files describing ACLs and + * in order to show users with ACL LIST. */ +sds ACLDescribeUser(user *u) { + sds res = sdsempty(); + + /* Flags. */ + for (int j = 0; ACLUserFlags[j].flag; j++) { + if (u->flags & ACLUserFlags[j].flag) { + res = sdscat(res,ACLUserFlags[j].name); + res = sdscatlen(res," ",1); + } + } + + /* Passwords. */ + listIter li; + listNode *ln; + listRewind(u->passwords,&li); + while((ln = listNext(&li))) { + sds thispass = listNodeValue(ln); + res = sdscatlen(res,">",1); + res = sdscatsds(res,thispass); + res = sdscatlen(res," ",1); + } + + /* Key patterns. */ + if (u->flags & USER_FLAG_ALLKEYS) { + res = sdscatlen(res,"~* ",3); + } else { + listRewind(u->patterns,&li); + while((ln = listNext(&li))) { + sds thispat = listNodeValue(ln); + res = sdscatlen(res,"~",1); + res = sdscatsds(res,thispat); + res = sdscatlen(res," ",1); + } + } + + /* Command rules. */ + sds rules = ACLDescribeUserCommandRules(u); + res = sdscatsds(res,rules); + sdsfree(rules); + return res; +} + /* Get a command from the original command table, that is not affected * by the command renaming operations: we base all the ACL work from that * table, so that ACLs are valid regardless of command renaming. */ @@ -500,7 +560,9 @@ int ACLSetUser(user *u, const char *op, ssize_t oplen) { if (oplen == -1) oplen = strlen(op); if (!strcasecmp(op,"on")) { u->flags |= USER_FLAG_ENABLED; + u->flags &= ~USER_FLAG_DISABLED; } else if (!strcasecmp(op,"off")) { + u->flags |= USER_FLAG_DISABLED; u->flags &= ~USER_FLAG_ENABLED; } else if (!strcasecmp(op,"allkeys") || !strcasecmp(op,"~*")) @@ -679,7 +741,7 @@ int ACLCheckUserCredentials(robj *username, robj *password) { } /* Disabled users can't login. */ - if ((u->flags & USER_FLAG_ENABLED) == 0) { + if (u->flags & USER_FLAG_DISABLED) { errno = EINVAL; return C_ERR; } @@ -874,24 +936,11 @@ void aclCommand(client *c) { addReplyBulkCString(c,"flags"); void *deflen = addReplyDeferredLen(c); int numflags = 0; - if (u->flags & USER_FLAG_ENABLED) { - addReplyBulkCString(c,"on"); - numflags++; - } else { - addReplyBulkCString(c,"off"); - numflags++; - } - if (u->flags & USER_FLAG_ALLKEYS) { - addReplyBulkCString(c,"allkeys"); - numflags++; - } - if (u->flags & USER_FLAG_ALLCOMMANDS) { - addReplyBulkCString(c,"allcommands"); - numflags++; - } - if (u->flags & USER_FLAG_NOPASS) { - addReplyBulkCString(c,"nopass"); - numflags++; + for (int j = 0; ACLUserFlags[j].flag; j++) { + if (u->flags & ACLUserFlags[j].flag) { + addReplyBulkCString(c,ACLUserFlags[j].name); + numflags++; + } } setDeferredSetLen(c,deflen,numflags); diff --git a/src/server.h b/src/server.h index 1e38b8ae6..13d29308f 100644 --- a/src/server.h +++ b/src/server.h @@ -740,9 +740,10 @@ typedef struct readyList { command ID we can set in the user is USER_COMMAND_BITS_COUNT-1. */ #define USER_FLAG_ENABLED (1<<0) /* The user is active. */ -#define USER_FLAG_ALLKEYS (1<<1) /* The user can mention any key. */ -#define USER_FLAG_ALLCOMMANDS (1<<2) /* The user can run all commands. */ -#define USER_FLAG_NOPASS (1<<3) /* The user requires no password, any +#define USER_FLAG_DISABLED (1<<1) /* The user is disabled. */ +#define USER_FLAG_ALLKEYS (1<<2) /* The user can mention any key. */ +#define USER_FLAG_ALLCOMMANDS (1<<3) /* The user can run all commands. */ +#define USER_FLAG_NOPASS (1<<4) /* The user requires no password, any provided password will work. For the default user, this also means that no AUTH is needed, and every From 0f1b06aa40daa8341e493193c9e8d859591c2e29 Mon Sep 17 00:00:00 2001 From: antirez Date: Thu, 31 Jan 2019 17:01:28 +0100 Subject: [PATCH 02/35] ACL: implement LIST and USERS subcommands. --- src/acl.c | 41 ++++++++++++++++++++++++++++++++--------- 1 file changed, 32 insertions(+), 9 deletions(-) diff --git a/src/acl.c b/src/acl.c index dce164f21..74efbcc33 100644 --- a/src/acl.c +++ b/src/acl.c @@ -917,12 +917,6 @@ void aclCommand(client *c) { } } addReply(c,shared.ok); - } else if (!strcasecmp(sub,"whoami")) { - if (c->user != NULL) { - addReplyBulkCBuffer(c,c->user->name,sdslen(c->user->name)); - } else { - addReplyNull(c); - } } else if (!strcasecmp(sub,"getuser") && c->argc == 3) { user *u = ACLGetUserByName(c->argv[2]->ptr,sdslen(c->argv[2]->ptr)); if (u == NULL) { @@ -975,13 +969,42 @@ void aclCommand(client *c) { addReplyBulkCBuffer(c,thispat,sdslen(thispat)); } } + } else if (!strcasecmp(sub,"list") || !strcasecmp(sub,"users")) { + int justnames = !strcasecmp(sub,"users"); + addReplyArrayLen(c,raxSize(Users)); + raxIterator ri; + raxStart(&ri,Users); + raxSeek(&ri,"^",NULL,0); + while(raxNext(&ri)) { + user *u = ri.data; + 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); + sds descr = ACLDescribeUser(u); + config = sdscatsds(config,descr); + sdsfree(descr); + addReplyBulkSds(c,config); + } + } + raxStop(&ri); + } else if (!strcasecmp(sub,"whoami")) { + if (c->user != NULL) { + addReplyBulkCBuffer(c,c->user->name,sdslen(c->user->name)); + } else { + addReplyNull(c); + } } else if (!strcasecmp(sub,"help")) { const char *help[] = { -"LIST -- List all the registered users.", +"LIST -- Show user details in config file format.", +"USERS -- List all the registered usernames.", "SETUSER [attribs ...] -- Create or modify a user.", -"DELUSER -- Delete a user.", "GETUSER -- Get the user details.", -"WHOAMI -- Return the current username.", +"DELUSER -- Delete a user.", +"WHOAMI -- Return the current connection username.", NULL }; addReplyHelp(c,help); From bc9b118e05722afb557cdc1631294ce985d445da Mon Sep 17 00:00:00 2001 From: antirez Date: Thu, 31 Jan 2019 17:04:42 +0100 Subject: [PATCH 03/35] ACL: don't emit useless flags in ACLDescribeUser(). --- src/acl.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/acl.c b/src/acl.c index 74efbcc33..cf46ef812 100644 --- a/src/acl.c +++ b/src/acl.c @@ -383,6 +383,10 @@ sds ACLDescribeUser(user *u) { /* Flags. */ for (int j = 0; ACLUserFlags[j].flag; j++) { + /* Skip the allcommands and allkeys flags because they'll be emitted + * later as ~* and +@all. */ + if (ACLUserFlags[j].flag == USER_FLAG_ALLKEYS || + ACLUserFlags[j].flag == USER_FLAG_ALLCOMMANDS) continue; if (u->flags & ACLUserFlags[j].flag) { res = sdscat(res,ACLUserFlags[j].name); res = sdscatlen(res," ",1); From 74b7afdf71e7e06f998e5955b4a088290534acb9 Mon Sep 17 00:00:00 2001 From: antirez Date: Thu, 31 Jan 2019 18:32:49 +0100 Subject: [PATCH 04/35] ACL: check arity of LIST / USERS subcommand. --- src/acl.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/acl.c b/src/acl.c index cf46ef812..58ffece34 100644 --- a/src/acl.c +++ b/src/acl.c @@ -973,7 +973,9 @@ void aclCommand(client *c) { addReplyBulkCBuffer(c,thispat,sdslen(thispat)); } } - } else if (!strcasecmp(sub,"list") || !strcasecmp(sub,"users")) { + } else if ((!strcasecmp(sub,"list") || !strcasecmp(sub,"users")) && + c->argc == 2) + { int justnames = !strcasecmp(sub,"users"); addReplyArrayLen(c,raxSize(Users)); raxIterator ri; From ec1aee031c847dc3929b2556633ffa33eaf3a5e2 Mon Sep 17 00:00:00 2001 From: antirez Date: Thu, 31 Jan 2019 18:33:14 +0100 Subject: [PATCH 05/35] ACL: implement DELUSER. --- src/acl.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/src/acl.c b/src/acl.c index 58ffece34..1df511329 100644 --- a/src/acl.c +++ b/src/acl.c @@ -80,6 +80,7 @@ struct ACLUserFlag { }; void ACLResetSubcommandsForCommand(user *u, unsigned long id); +void ACLResetSubcommands(user *u); /* ============================================================================= * Helper functions for the rest of the ACL implementation @@ -175,6 +176,16 @@ user *ACLCreateUser(const char *name, size_t namelen) { return u; } +/* 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) { + sdsfree(u->name); + listRelease(u->passwords); + listRelease(u->patterns); + ACLResetSubcommands(u); + zfree(u); +} + /* Given a command ID, this function set by reference 'word' and 'bit' * so that user->allowed_commands[word] will address the right word * where the corresponding bit for the provided ID is stored, and @@ -921,6 +932,44 @@ void aclCommand(client *c) { } } addReply(c,shared.ok); + } else if (!strcasecmp(sub,"deluser") && c->argc >= 3) { + int deleted = 0; + for (int j = 2; j < c->argc; j++) { + sds username = c->argv[j]->ptr; + if (!strcmp(username,"default")) { + addReplyError(c,"The 'default' user cannot be removed"); + return; + } + user *u; + if (raxRemove(Users,(unsigned char*)username, + sdslen(username), + (void**)&u)) + { + /* When a user is deleted we need to cycle the active + * connections in order to kill all the pending ones that + * are authenticated with such user. */ + ACLFreeUser(u); + listIter li; + listNode *ln; + listRewind(server.clients,&li); + while ((ln = listNext(&li)) != NULL) { + client *c = listNodeValue(ln); + if (c->user == u) { + /* We'll free the conenction asynchronously, so + * in theory to set a different user is not needed. + * However if there are bugs in Redis, soon or later + * this may result in some security hole: it's much + * more defensive to set the default user and put + * it in non authenticated mode. */ + c->user = DefaultUser; + c->authenticated = 0; + freeClientAsync(c); + } + } + deleted++; + } + } + addReplyLongLong(c,deleted); } else if (!strcasecmp(sub,"getuser") && c->argc == 3) { user *u = ACLGetUserByName(c->argv[2]->ptr,sdslen(c->argv[2]->ptr)); if (u == NULL) { From b6372f16c414e5f8a20164e6b1b82fc4c133be68 Mon Sep 17 00:00:00 2001 From: antirez Date: Fri, 1 Feb 2019 08:17:24 +0100 Subject: [PATCH 06/35] ACL: assign ACL command ID to modules commands. --- src/module.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/module.c b/src/module.c index 8954fcdf0..9c24bfe2b 100644 --- a/src/module.c +++ b/src/module.c @@ -684,6 +684,7 @@ int RM_CreateCommand(RedisModuleCtx *ctx, const char *name, RedisModuleCmdFunc c cp->rediscmd->calls = 0; dictAdd(server.commands,sdsdup(cmdname),cp->rediscmd); dictAdd(server.orig_commands,sdsdup(cmdname),cp->rediscmd); + cp->rediscmd->id = ACLGetCommandID(cmdname); /* ID used for ACL. */ return REDISMODULE_OK; } From 1769c222486d1deb33b394899eccab35be975c96 Mon Sep 17 00:00:00 2001 From: antirez Date: Fri, 1 Feb 2019 11:37:20 +0100 Subject: [PATCH 07/35] ACL: set modules help clients to the root user. It does not make much sense to limit what modules can do: the admin should instead limit what module commnads an user may call. So RedisModule_Call() and other module operations should be able to execute everything they want: the limitation should be posed by the API exported by the module itself. --- src/module.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/module.c b/src/module.c index 9c24bfe2b..81982ba76 100644 --- a/src/module.c +++ b/src/module.c @@ -2697,6 +2697,7 @@ RedisModuleCallReply *RM_Call(RedisModuleCtx *ctx, const char *cmdname, const ch /* Create the client and dispatch the command. */ va_start(ap, fmt); c = createClient(-1); + c->user = NULL; /* Root user. */ argv = moduleCreateArgvFromUserFormat(cmdname,fmt,&argc,&flags,ap); replicate = flags & REDISMODULE_ARGV_REPLICATE; va_end(ap); @@ -4660,6 +4661,7 @@ void moduleInitModulesSystem(void) { moduleKeyspaceSubscribers = listCreate(); moduleFreeContextReusedClient = createClient(-1); moduleFreeContextReusedClient->flags |= CLIENT_MODULE; + moduleFreeContextReusedClient->user = NULL; /* root user. */ moduleRegisterCoreAPI(); if (pipe(server.module_blocked_pipe) == -1) { From b8323d98e967206e2922dfc30fa2223292bfef5f Mon Sep 17 00:00:00 2001 From: antirez Date: Fri, 1 Feb 2019 12:20:09 +0100 Subject: [PATCH 08/35] ACL: skeleton and first ideas for postponed user loading. --- src/acl.c | 39 +++++++++++++++++++++++++++++++++++---- 1 file changed, 35 insertions(+), 4 deletions(-) diff --git a/src/acl.c b/src/acl.c index 1df511329..5ad891cbd 100644 --- a/src/acl.c +++ b/src/acl.c @@ -34,10 +34,19 @@ * ==========================================================================*/ rax *Users; /* Table mapping usernames to user structures. */ -user *DefaultUser; /* Global reference to the default user. - Every new connection is associated to it, if no - AUTH or HELLO is used to authenticate with a - different user. */ + +user *DefaultUser; /* Global reference to the default user. + Every new connection is associated to it, if no + AUTH or HELLO is used to authenticate with a + different user. */ + +list *UsersToLoad; /* This is a list of users found in the configuration file + that we'll need to load in the final stage of Redis + initialization, after all the modules are already + loaded. Every list element is a NULL terminated + array of SDS pointers: the first is the user name, + all the remaining pointers are ACL rules in the same + format as ACLSetUser(). */ struct ACLCategoryItem { const char *name; @@ -735,6 +744,7 @@ sds ACLDefaultUserFirstPassword(void) { /* Initialization of the ACL subsystem. */ void ACLInit(void) { Users = raxNew(); + UsersToLoad = listCreate(); DefaultUser = ACLCreateUser("default",7); ACLSetUser(DefaultUser,"+@all",-1); ACLSetUser(DefaultUser,"~*",-1); @@ -904,6 +914,27 @@ int ACLCheckCommandPerm(client *c) { return ACL_OK; } +/* ============================================================================= + * ACL loading / saving functions + * ==========================================================================*/ + +/* Given an argument vector describing a user in the form: + * + * user ... ACL rules and flags ... + * + * this function validates, and if the syntax is valid, appends + * the user definition to a list for later loading. + * + * The rules are tested for validity and if there obvious syntax errors + * the function returns C_ERR and does nothing, otherwise C_OK is returned + * and the user is appended to the list. + * + * Note that this function cannot stop in case of commands that are not found + * and, in that case, the error will be emitted later, because certain + * commands may be defined later once modules are loaded. */ +int ACLAppendUserForLoading(sds *argv, int argc) { +} + /* ============================================================================= * ACL related commands * ==========================================================================*/ From 8f16e1ea912110d5459a4ddd370a93485408311c Mon Sep 17 00:00:00 2001 From: antirez Date: Fri, 1 Feb 2019 13:02:41 +0100 Subject: [PATCH 09/35] ACL: implement ACLAppendUserForLoading(). --- src/acl.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/acl.c b/src/acl.c index 5ad891cbd..105bcfc95 100644 --- a/src/acl.c +++ b/src/acl.c @@ -933,6 +933,24 @@ int ACLCheckCommandPerm(client *c) { * and, in that case, the error will be emitted later, because certain * commands may be defined later once modules are loaded. */ int ACLAppendUserForLoading(sds *argv, int argc) { + if (argc < 2 || strcasecmp(argv[0],"user")) return C_ERR; + + /* Try to apply the user rules in a fake user to see if they + * are actually valid. */ + user fu = {0}; + user *fakeuser = &fu; + for (int j = 2; j < argc; j++) { + if (ACLSetUser(fakeuser,argv[j],sdslen(argv[j])) == C_ERR) { + if (errno != ENOENT) return C_ERR; + } + } + + /* Rules look valid, let's append the user to the list. */ + sds *copy = zmalloc(sizeof(sds)*argc); + for (int j = 1; j < argc; j++) copy[j-1] = sdsdup(argv[j]); + copy[argc-1] = NULL; + listAddNodeTail(UsersToLoad,copy); + return C_OK; } /* ============================================================================= From 21e84cdae254b715b15e48734fbd8484b2c14810 Mon Sep 17 00:00:00 2001 From: antirez Date: Mon, 4 Feb 2019 12:55:26 +0100 Subject: [PATCH 10/35] ACL: initial appending of users in user loading list. --- src/acl.c | 14 +++++++++++--- src/config.c | 5 +++++ src/server.h | 1 + 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/src/acl.c b/src/acl.c index 105bcfc95..43517300b 100644 --- a/src/acl.c +++ b/src/acl.c @@ -937,11 +937,18 @@ int ACLAppendUserForLoading(sds *argv, int argc) { /* Try to apply the user rules in a fake user to see if they * are actually valid. */ - user fu = {0}; - user *fakeuser = &fu; + char *funame = "__fakeuser__"; + user *fakeuser = ACLCreateUser(funame,strlen(funame)); + serverAssert(fakeuser != NULL); + int retval = raxRemove(Users,(unsigned char*) funame,strlen(funame),NULL); + serverAssert(retval != 0); + for (int j = 2; j < argc; j++) { if (ACLSetUser(fakeuser,argv[j],sdslen(argv[j])) == C_ERR) { - if (errno != ENOENT) return C_ERR; + if (errno != ENOENT) { + ACLFreeUser(fakeuser); + return C_ERR; + } } } @@ -950,6 +957,7 @@ int ACLAppendUserForLoading(sds *argv, int argc) { for (int j = 1; j < argc; j++) copy[j-1] = sdsdup(argv[j]); copy[argc-1] = NULL; listAddNodeTail(UsersToLoad,copy); + ACLFreeUser(fakeuser); return C_OK; } diff --git a/src/config.c b/src/config.c index 91bbdee72..b2600d2ea 100644 --- a/src/config.c +++ b/src/config.c @@ -791,6 +791,11 @@ void loadServerConfigFromString(char *config) { "Allowed values: 'upstart', 'systemd', 'auto', or 'no'"; goto loaderr; } + } else if (!strcasecmp(argv[0],"user") && argc >= 2) { + if (ACLAppendUserForLoading(argv,argc) == C_ERR) { + err = "Syntax error in user declaration"; + goto loaderr; + } } else if (!strcasecmp(argv[0],"loadmodule") && argc >= 2) { queueLoadModule(argv[1],&argv[2],argc-2); } else if (!strcasecmp(argv[0],"sentinel")) { diff --git a/src/server.h b/src/server.h index 13d29308f..3f23cee20 100644 --- a/src/server.h +++ b/src/server.h @@ -1738,6 +1738,7 @@ int ACLCheckCommandPerm(client *c); int ACLSetUser(user *u, const char *op, ssize_t oplen); sds ACLDefaultUserFirstPassword(void); uint64_t ACLGetCommandCategoryFlagByName(const char *name); +int ACLAppendUserForLoading(sds *argv, int argc); /* Sorted sets data type */ From b166c41eddec664e64c1e268266d38c3f26edaa2 Mon Sep 17 00:00:00 2001 From: antirez Date: Mon, 4 Feb 2019 13:00:38 +0100 Subject: [PATCH 11/35] ACL: make ACLAppendUserForLoading() able to report bad argument. --- src/acl.c | 14 +++++++++++--- src/config.c | 3 ++- src/server.h | 2 +- 3 files changed, 14 insertions(+), 5 deletions(-) diff --git a/src/acl.c b/src/acl.c index 43517300b..991be210e 100644 --- a/src/acl.c +++ b/src/acl.c @@ -931,9 +931,16 @@ int ACLCheckCommandPerm(client *c) { * * Note that this function cannot stop in case of commands that are not found * and, in that case, the error will be emitted later, because certain - * commands may be defined later once modules are loaded. */ -int ACLAppendUserForLoading(sds *argv, int argc) { - if (argc < 2 || strcasecmp(argv[0],"user")) return C_ERR; + * commands may be defined later once modules are loaded. + * + * When an error is detected and C_ERR is returned, the function populates + * by reference (if not set to NULL) the argc_err argument with the index + * of the argv vector that caused the error. */ +int ACLAppendUserForLoading(sds *argv, int argc, int *argc_err) { + if (argc < 2 || strcasecmp(argv[0],"user")) { + if (argc_err) *argc_err = 0; + return C_ERR; + } /* Try to apply the user rules in a fake user to see if they * are actually valid. */ @@ -947,6 +954,7 @@ int ACLAppendUserForLoading(sds *argv, int argc) { if (ACLSetUser(fakeuser,argv[j],sdslen(argv[j])) == C_ERR) { if (errno != ENOENT) { ACLFreeUser(fakeuser); + if (argc_err) *argc_err = j; return C_ERR; } } diff --git a/src/config.c b/src/config.c index b2600d2ea..c852b01b7 100644 --- a/src/config.c +++ b/src/config.c @@ -792,7 +792,8 @@ void loadServerConfigFromString(char *config) { goto loaderr; } } else if (!strcasecmp(argv[0],"user") && argc >= 2) { - if (ACLAppendUserForLoading(argv,argc) == C_ERR) { + int argc_err; + if (ACLAppendUserForLoading(argv,argc,&argc_err) == C_ERR) { err = "Syntax error in user declaration"; goto loaderr; } diff --git a/src/server.h b/src/server.h index 3f23cee20..cbf995d4e 100644 --- a/src/server.h +++ b/src/server.h @@ -1738,7 +1738,7 @@ int ACLCheckCommandPerm(client *c); int ACLSetUser(user *u, const char *op, ssize_t oplen); sds ACLDefaultUserFirstPassword(void); uint64_t ACLGetCommandCategoryFlagByName(const char *name); -int ACLAppendUserForLoading(sds *argv, int argc); +int ACLAppendUserForLoading(sds *argv, int argc, int *argc_err); /* Sorted sets data type */ From 68fd4a97fa52b1bca8e3dd8c0c2fc5b72714d824 Mon Sep 17 00:00:00 2001 From: antirez Date: Mon, 4 Feb 2019 13:04:35 +0100 Subject: [PATCH 12/35] ACL: better error reporting in users configuration errors. --- src/config.c | 6 +++++- src/server.h | 1 + 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/config.c b/src/config.c index c852b01b7..bc92a423d 100644 --- a/src/config.c +++ b/src/config.c @@ -794,7 +794,11 @@ void loadServerConfigFromString(char *config) { } else if (!strcasecmp(argv[0],"user") && argc >= 2) { int argc_err; if (ACLAppendUserForLoading(argv,argc,&argc_err) == C_ERR) { - err = "Syntax error in user declaration"; + char buf[1024]; + char *errmsg = ACLSetUserStringError(); + snprintf(buf,sizeof(buf),"Error in user declaration '%s': %s", + argv[argc_err],errmsg); + err = buf; goto loaderr; } } else if (!strcasecmp(argv[0],"loadmodule") && argc >= 2) { diff --git a/src/server.h b/src/server.h index cbf995d4e..a694a4dc2 100644 --- a/src/server.h +++ b/src/server.h @@ -1739,6 +1739,7 @@ int ACLSetUser(user *u, const char *op, ssize_t oplen); sds ACLDefaultUserFirstPassword(void); uint64_t ACLGetCommandCategoryFlagByName(const char *name); int ACLAppendUserForLoading(sds *argv, int argc, int *argc_err); +char *ACLSetUserStringError(void); /* Sorted sets data type */ From 500b3e128fc312a6aae113975b9f2f585f476500 Mon Sep 17 00:00:00 2001 From: antirez Date: Mon, 4 Feb 2019 16:35:15 +0100 Subject: [PATCH 13/35] ACL: implement ACLLoadConfiguredUsers(). --- src/acl.c | 40 ++++++++++++++++++++++++++++++++++++++++ src/server.h | 1 + 2 files changed, 41 insertions(+) diff --git a/src/acl.c b/src/acl.c index 991be210e..349f81f31 100644 --- a/src/acl.c +++ b/src/acl.c @@ -969,6 +969,46 @@ int ACLAppendUserForLoading(sds *argv, int argc, int *argc_err) { return C_OK; } +/* This function will load the configured users appended to the server + * configuration via ACLAppendUserForLoading(). On loading errors it will + * log an error and return C_ERR, otherwise C_OK will be returned. */ +int ACLLoadConfiguredUsers(void) { + listIter li; + listNode *ln; + listRewind(UsersToLoad,&li); + while ((ln = listNext(&li)) != NULL) { + sds *aclrules = listNodeValue(ln); + user *u = ACLCreateUser(aclrules[0],sdslen(aclrules[0])); + if (!u) { + serverLog(LL_WARNING, + "Error loading ACLs: user '%s' specified multiple times", + aclrules[0]); + return C_ERR; + } + + /* Load every rule defined for this user. */ + for (int j = 1; aclrules[j]; j++) { + if (ACLSetUser(u,aclrules[j],sdslen(aclrules[j])) != C_OK) { + char *errmsg = ACLSetUserStringError(); + serverLog(LL_WARNING,"Error loading ACL rule '%s' for " + "the user named '%s': %s", + aclrules[0],aclrules[j],errmsg); + return C_ERR; + } + } + + /* Having a disabled user in the configuration may be an error, + * warn about it without returning any error to the caller. */ + if (u->flags & USER_FLAG_DISABLED) { + serverLog(LL_NOTICE, "The user '%s' is disabled (there is no " + "'on' modifier in the user description). Make " + "sure this is not a configuration error.", + aclrules[0]); + } + } + return C_OK; +} + /* ============================================================================= * ACL related commands * ==========================================================================*/ diff --git a/src/server.h b/src/server.h index a694a4dc2..02104e6f0 100644 --- a/src/server.h +++ b/src/server.h @@ -1740,6 +1740,7 @@ sds ACLDefaultUserFirstPassword(void); uint64_t ACLGetCommandCategoryFlagByName(const char *name); int ACLAppendUserForLoading(sds *argv, int argc, int *argc_err); char *ACLSetUserStringError(void); +int ACLLoadConfiguredUsers(void); /* Sorted sets data type */ From 623b17425ef5af170c4bbc7aca9c07b57bc68adf Mon Sep 17 00:00:00 2001 From: antirez Date: Mon, 4 Feb 2019 16:39:07 +0100 Subject: [PATCH 14/35] ACL: load the defined users at server startup. --- src/server.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/server.c b/src/server.c index 1df63f65f..72daaedec 100644 --- a/src/server.c +++ b/src/server.c @@ -4907,6 +4907,11 @@ int main(int argc, char **argv) { linuxMemoryWarnings(); #endif moduleLoadFromQueue(); + if (ACLLoadConfiguredUsers() == C_ERR) { + serverLog(LL_WARNING, + "Critical error while loading ACLs. Exiting."); + exit(1); + } loadDataFromDisk(); if (server.cluster_enabled) { if (verifyClusterConfigWithData() == C_ERR) { From 2262dd184dc567561ef008d136e11def6e6abbe5 Mon Sep 17 00:00:00 2001 From: antirez Date: Mon, 4 Feb 2019 16:58:35 +0100 Subject: [PATCH 15/35] ACL: fix user/rule inverted error message. --- src/acl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/acl.c b/src/acl.c index 349f81f31..8211dcea3 100644 --- a/src/acl.c +++ b/src/acl.c @@ -992,7 +992,7 @@ int ACLLoadConfiguredUsers(void) { char *errmsg = ACLSetUserStringError(); serverLog(LL_WARNING,"Error loading ACL rule '%s' for " "the user named '%s': %s", - aclrules[0],aclrules[j],errmsg); + aclrules[j],aclrules[0],errmsg); return C_ERR; } } From 775bf6193d774280ed19b7cf79fee0987ec83853 Mon Sep 17 00:00:00 2001 From: antirez Date: Tue, 5 Feb 2019 10:48:17 +0100 Subject: [PATCH 16/35] ACL: implement rewriting of users in redis.conf. --- src/config.c | 33 +++++++++++++++++++++++++++++++++ src/server.c | 1 + src/server.h | 5 +++++ 3 files changed, 39 insertions(+) diff --git a/src/config.c b/src/config.c index bc92a423d..663ac404e 100644 --- a/src/config.c +++ b/src/config.c @@ -1917,6 +1917,38 @@ void rewriteConfigSaveOption(struct rewriteConfigState *state) { rewriteConfigMarkAsProcessed(state,"save"); } +/* Rewrite the user option. */ +void rewriteConfigUserOption(struct rewriteConfigState *state) { + /* If there is a user file defined we just mark this configuration + * directive as processed, so that all the lines containing users + * inside the config file gets discarded. */ + if (server.acl_filename[0] != '\0') { + rewriteConfigMarkAsProcessed(state,"user"); + return; + } + + /* Otherwise scan the list of users and rewrite every line. Note that + * in case the list here is empty, the effect will just be to comment + * all the users directive inside the config file. */ + raxIterator ri; + raxStart(&ri,Users); + raxSeek(&ri,"^",NULL,0); + while(raxNext(&ri)) { + user *u = ri.data; + sds line = sdsnew("user "); + line = sdscatsds(line,u->name); + line = sdscatlen(line," ",1); + sds descr = ACLDescribeUser(u); + line = sdscatsds(line,descr); + sdsfree(descr); + rewriteConfigRewriteLine(state,"user",line,1); + } + raxStop(&ri); + + /* Mark "user" as processed in case there are no defined users. */ + rewriteConfigMarkAsProcessed(state,"user"); +} + /* Rewrite the dir option, always using absolute paths.*/ void rewriteConfigDirOption(struct rewriteConfigState *state) { char cwd[1024]; @@ -2186,6 +2218,7 @@ int rewriteConfig(char *path) { rewriteConfigStringOption(state,"syslog-ident",server.syslog_ident,CONFIG_DEFAULT_SYSLOG_IDENT); rewriteConfigSyslogfacilityOption(state); rewriteConfigSaveOption(state); + rewriteConfigUserOption(state); rewriteConfigNumericalOption(state,"databases",server.dbnum,CONFIG_DEFAULT_DBNUM); rewriteConfigYesNoOption(state,"stop-writes-on-bgsave-error",server.stop_writes_on_bgsave_err,CONFIG_DEFAULT_STOP_WRITES_ON_BGSAVE_ERROR); rewriteConfigYesNoOption(state,"rdbcompression",server.rdb_compression,CONFIG_DEFAULT_RDB_COMPRESSION); diff --git a/src/server.c b/src/server.c index 72daaedec..de84e430e 100644 --- a/src/server.c +++ b/src/server.c @@ -2267,6 +2267,7 @@ void initServerConfig(void) { server.pidfile = NULL; server.rdb_filename = zstrdup(CONFIG_DEFAULT_RDB_FILENAME); server.aof_filename = zstrdup(CONFIG_DEFAULT_AOF_FILENAME); + server.acl_filename = zstrdup(CONFIG_DEFAULT_ACL_FILENAME); server.rdb_compression = CONFIG_DEFAULT_RDB_COMPRESSION; server.rdb_checksum = CONFIG_DEFAULT_RDB_CHECKSUM; server.stop_writes_on_bgsave_err = CONFIG_DEFAULT_STOP_WRITES_ON_BGSAVE_ERROR; diff --git a/src/server.h b/src/server.h index 02104e6f0..d2c6aa1e0 100644 --- a/src/server.h +++ b/src/server.h @@ -148,6 +148,7 @@ typedef long long mstime_t; /* millisecond time type. */ #define CONFIG_DEFAULT_RDB_SAVE_INCREMENTAL_FSYNC 1 #define CONFIG_DEFAULT_MIN_SLAVES_TO_WRITE 0 #define CONFIG_DEFAULT_MIN_SLAVES_MAX_LAG 10 +#define CONFIG_DEFAULT_ACL_FILENAME "" #define NET_IP_STR_LEN 46 /* INET6_ADDRSTRLEN is 46, but we need to be sure */ #define NET_PEER_ID_LEN (NET_IP_STR_LEN+32) /* Must be enough for ip:port */ #define CONFIG_BINDADDR_MAX 16 @@ -1337,6 +1338,8 @@ struct redisServer { /* Latency monitor */ long long latency_monitor_threshold; dict *latency_events; + /* ACLs */ + char *acl_filename; /* ACL Users file. NULL if not configured. */ /* Assert & bug reporting */ const char *assert_failed; const char *assert_file; @@ -1725,6 +1728,7 @@ void sendChildInfo(int process_type); void receiveChildInfo(void); /* acl.c -- Authentication related prototypes. */ +extern rax *Users; extern user *DefaultUser; void ACLInit(void); /* Return values for ACLCheckUserCredentials(). */ @@ -1741,6 +1745,7 @@ uint64_t ACLGetCommandCategoryFlagByName(const char *name); int ACLAppendUserForLoading(sds *argv, int argc, int *argc_err); char *ACLSetUserStringError(void); int ACLLoadConfiguredUsers(void); +sds ACLDescribeUser(user *u); /* Sorted sets data type */ From 416c640156626978843e03cf573925a75d01c67a Mon Sep 17 00:00:00 2001 From: antirez Date: Tue, 5 Feb 2019 10:52:05 +0100 Subject: [PATCH 17/35] ACL: change behavior of redefined user. Last line counts. --- src/acl.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/acl.c b/src/acl.c index 8211dcea3..db2f291c6 100644 --- a/src/acl.c +++ b/src/acl.c @@ -978,12 +978,12 @@ int ACLLoadConfiguredUsers(void) { listRewind(UsersToLoad,&li); while ((ln = listNext(&li)) != NULL) { sds *aclrules = listNodeValue(ln); - user *u = ACLCreateUser(aclrules[0],sdslen(aclrules[0])); + sds username = aclrules[0]; + user *u = ACLCreateUser(username,sdslen(username)); if (!u) { - serverLog(LL_WARNING, - "Error loading ACLs: user '%s' specified multiple times", - aclrules[0]); - return C_ERR; + u = ACLGetUserByName(username,sdslen(username)); + serverAssert(u != NULL); + ACLSetUser(u,"reset",-1); } /* Load every rule defined for this user. */ From cc116736c1306f53869510a768cb8bb4d6c6b04c Mon Sep 17 00:00:00 2001 From: antirez Date: Tue, 5 Feb 2019 17:49:52 +0100 Subject: [PATCH 18/35] ACL: ability to configure an external ACL file. --- src/config.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/config.c b/src/config.c index 663ac404e..fc9160d26 100644 --- a/src/config.c +++ b/src/config.c @@ -283,6 +283,9 @@ void loadServerConfigFromString(char *config) { } fclose(logfp); } + } else if (!strcasecmp(argv[0],"aclfile") && argc == 2) { + zfree(server.acl_filename); + server.acl_filename = zstrdup(argv[1]); } else if (!strcasecmp(argv[0],"always-show-logo") && argc == 2) { if ((server.always_show_logo = yesnotoi(argv[1])) == -1) { err = "argument must be 'yes' or 'no'"; goto loaderr; @@ -1355,6 +1358,7 @@ void configGetCommand(client *c) { config_get_string_field("cluster-announce-ip",server.cluster_announce_ip); config_get_string_field("unixsocket",server.unixsocket); config_get_string_field("logfile",server.logfile); + config_get_string_field("aclfile",server.acl_filename); config_get_string_field("pidfile",server.pidfile); config_get_string_field("slave-announce-ip",server.slave_announce_ip); config_get_string_field("replica-announce-ip",server.slave_announce_ip); @@ -2214,6 +2218,7 @@ int rewriteConfig(char *path) { rewriteConfigNumericalOption(state,"replica-announce-port",server.slave_announce_port,CONFIG_DEFAULT_SLAVE_ANNOUNCE_PORT); rewriteConfigEnumOption(state,"loglevel",server.verbosity,loglevel_enum,CONFIG_DEFAULT_VERBOSITY); rewriteConfigStringOption(state,"logfile",server.logfile,CONFIG_DEFAULT_LOGFILE); + rewriteConfigStringOption(state,"aclfile",server.acl_filename,CONFIG_DEFAULT_ACL_FILENAME); rewriteConfigYesNoOption(state,"syslog-enabled",server.syslog_enabled,CONFIG_DEFAULT_SYSLOG_ENABLED); rewriteConfigStringOption(state,"syslog-ident",server.syslog_ident,CONFIG_DEFAULT_SYSLOG_IDENT); rewriteConfigSyslogfacilityOption(state); From 7604ab7118d1154e9120ea41a88d9c214f2202c3 Mon Sep 17 00:00:00 2001 From: antirez Date: Tue, 5 Feb 2019 17:59:05 +0100 Subject: [PATCH 19/35] ACL: redis.conf: mark old ACL-alike stuff as deprecated. --- redis.conf | 39 +++++++++++++++++++++++++++++---------- 1 file changed, 29 insertions(+), 10 deletions(-) diff --git a/redis.conf b/redis.conf index 93ab9a42e..d1ced7eb3 100644 --- a/redis.conf +++ b/redis.conf @@ -493,20 +493,39 @@ replica-priority 100 ################################## SECURITY ################################### -# Require clients to issue AUTH before processing any other -# commands. This might be useful in environments in which you do not trust -# others with access to the host running redis-server. -# -# This should stay commented out for backward compatibility and because most -# people do not need auth (e.g. they run their own servers). -# # Warning: since Redis is pretty fast an outside user can try up to -# 150k passwords per second against a good box. This means that you should -# use a very strong password otherwise it will be very easy to break. +# 1 million passwords per second against a modern box. This means that you +# should use very strong passwords, otherwise they will be very easy to break. +# Note that because the password is really a shared secret between the client +# and the server, and should not be memorized by any human, the password +# can be easily a long string from /dev/urandom or whatever, so by using a +# long and unguessable password no brute force attack will be possible. + +# Instead of configuring users here in this file, it is possible to use +# a stand-alone file just listing users. The two methods cannot be mixed: +# if you configure users here and at the same time you activate the exteranl +# ACL file, the server will refuse to start. +# +# The format of the external ACL user file is exactly the same as the +# format that is used inside redis.conf to describe users. +# +# aclfile /etc/redis/users.acl + +# IMPORTANT NOTE: starting with Redis 6 "requirepass" is just a compatiblity +# layer on top of the new ACL system. The option effect will be just setting +# the password for the default user. Clients will still authenticate using +# AUTH as usually, or more explicitly with AUTH default +# if they follow the new protocol: both will work. # # requirepass foobared -# Command renaming. +# Command renaming (DEPRECATED). +# +# ------------------------------------------------------------------------ +# WARNING: avoid using this option if possible. Instead use ACLs to remove +# commands from the default user, and put them only in some admin user you +# create for administrative purposes. +# ------------------------------------------------------------------------ # # It is possible to change the name of dangerous commands in a shared # environment. For instance the CONFIG command may be renamed into something From e1e0f993d8e669bc9aad71a072fea70d98661fd1 Mon Sep 17 00:00:00 2001 From: antirez Date: Wed, 6 Feb 2019 12:39:11 +0100 Subject: [PATCH 20/35] ACL: initial design for ACLLoadFromFile() function. --- src/acl.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 79 insertions(+) diff --git a/src/acl.c b/src/acl.c index db2f291c6..1a721e628 100644 --- a/src/acl.c +++ b/src/acl.c @@ -1009,6 +1009,85 @@ int ACLLoadConfiguredUsers(void) { return C_OK; } +/* This function loads the ACL from the specified filename: every line + * is validated and shold be either empty or in the format used to specify + * users in the redis.conf configuration or in the ACL file, that is: + * + * user ... rules ... + * + * Note that this function considers comments starting with '#' as errors + * because the ACL file is meant to be rewritten, and comments would be + * lost after the rewrite. Yet empty lines are allowed to avoid being too + * strict. + * + * One important part of implementing ACL LOAD, that uses this function, is + * to avoid ending with broken rules if the ACL file is invalid for some + * reason, so the function will attempt to validate the rules before loading + * each user. For every line that will be found broken the function will + * collect an error message. All the valid lines will be correctly processed. + * + * At the end of the process, if no errors were found in the whole file then + * NULL is returned. Otherwise an SDS string describing in a single line + * a description of all the issues found is returned. */ +sds ACLLoadFromFile(const char *filename) { + FILE *fp; + char buf[1024]; + + /* Open the ACL file. */ + if ((fp = fopen(filename,"r")) == NULL) { + sds errors = sdscatprintf(sdsempty(), + "Error loading ACLs, opening file '%s': %s", + filename, strerror(errno)); + return errors; + } + + /* Load the whole file as a single string in memory. */ + sds acls = sdsempty(); + while(fgets(buf,CONFIG_MAX_LINE+1,fp) != NULL) + acls = sdscat(acls,buf); + fclose(fp); + + /* Split the file into lines and attempt to load each line. */ + int totlines; + sds *lines, errors = sdsempty(); + lines = sdssplitlen(acls,strlen(acls),"\n",1,&totlines); + + for (int i = 0; i < totlines; i++) { + sds *argv; + int argc; + int linenum = i+1; + + lines[i] = sdstrim(lines[i]," \t\r\n"); + + /* Skip blank lines */ + if (lines[i][0] == '\0') continue; + + /* Split into arguments */ + argv = sdssplitargs(lines[i],&argc); + if (argv == NULL) { + errors = sdscatprintf(errors, + "%d: unbalanced quotes in acl line.", + linenum); + continue; + } + + /* Skip this line if the resulting command vector is empty. */ + if (argc == 0) { + sdsfreesplitres(argv,argc); + continue; + } + + /* Try to process the line. */ + } + + sdsfreesplitres(lines,totlines); + if (sdslen(errors) == 0) { + sdsfree(errors); + errors = NULL; + } + return errors; +} + /* ============================================================================= * ACL related commands * ==========================================================================*/ From 0d3fb9f7f13c75c8d0c54c2faddc9e72f5dc5452 Mon Sep 17 00:00:00 2001 From: antirez Date: Wed, 6 Feb 2019 16:19:17 +0100 Subject: [PATCH 21/35] ACL: refactoring creation of unlinked users. --- src/acl.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/src/acl.c b/src/acl.c index 1a721e628..e19c26eb3 100644 --- a/src/acl.c +++ b/src/acl.c @@ -185,6 +185,23 @@ user *ACLCreateUser(const char *name, size_t namelen) { return u; } +/* This function should be called when we need an unlinked "fake" user + * we can use in order to validate ACL rules or for other similar reasons. + * The user will not get linked to the Users radix tree. The returned + * user should be released with ACLFreeUser() as usually. */ +user *ACLCreateUnlinkedUser(void) { + char username[64]; + for (int j = 0; ; j++) { + snprintf(username,sizeof(username),"__fakeuser:%d__",j); + user *fakeuser = ACLCreateUser(username,strlen(username)); + if (fakeuser == NULL) continue; + int retval = raxRemove(Users,(unsigned char*) username, + strlen(username),NULL); + serverAssert(retval != 0); + return fakeuser; + } +} + /* 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) { @@ -944,11 +961,7 @@ int ACLAppendUserForLoading(sds *argv, int argc, int *argc_err) { /* Try to apply the user rules in a fake user to see if they * are actually valid. */ - char *funame = "__fakeuser__"; - user *fakeuser = ACLCreateUser(funame,strlen(funame)); - serverAssert(fakeuser != NULL); - int retval = raxRemove(Users,(unsigned char*) funame,strlen(funame),NULL); - serverAssert(retval != 0); + user *fakeuser = ACLCreateUnlinkedUser(); for (int j = 2; j < argc; j++) { if (ACLSetUser(fakeuser,argv[j],sdslen(argv[j])) == C_ERR) { From bbdf02338d865e0bf12b004be966a76ab8a71019 Mon Sep 17 00:00:00 2001 From: antirez Date: Wed, 6 Feb 2019 16:44:55 +0100 Subject: [PATCH 22/35] ACL: now ACLLoadFromFile() validates against fake user. --- src/acl.c | 39 ++++++++++++++++++++++++++++++++++----- 1 file changed, 34 insertions(+), 5 deletions(-) diff --git a/src/acl.c b/src/acl.c index e19c26eb3..2fbc564ab 100644 --- a/src/acl.c +++ b/src/acl.c @@ -1065,6 +1065,10 @@ sds ACLLoadFromFile(const char *filename) { sds *lines, errors = sdsempty(); lines = sdssplitlen(acls,strlen(acls),"\n",1,&totlines); + /* We need a fake user to validate the rules before making changes + * to the real user mentioned in the ACL line. */ + user *fakeuser = ACLCreateUnlinkedUser(); + for (int i = 0; i < totlines; i++) { sds *argv; int argc; @@ -1079,8 +1083,8 @@ sds ACLLoadFromFile(const char *filename) { argv = sdssplitargs(lines[i],&argc); if (argv == NULL) { errors = sdscatprintf(errors, - "%d: unbalanced quotes in acl line.", - linenum); + "%d: unbalanced quotes in acl line. ", + linenum); continue; } @@ -1090,15 +1094,40 @@ sds ACLLoadFromFile(const char *filename) { continue; } - /* Try to process the line. */ + /* The line should start with the "user" keyword. */ + if (strcmp(argv[0],"user")) { + errors = sdscatprintf(errors, + "%d: line should start with user keyword. ", + linenum); + continue; + } + + /* Try to process the line using the fake user to validate iif + * the rules are able to apply cleanly. */ + ACLSetUser(fakeuser,"reset",-1); + int j; + for (j = 2; j < argc; j++) { + if (ACLSetUser(fakeuser,argv[j],sdslen(argv[j])) != C_OK) { + char *errmsg = ACLSetUserStringError(); + errors = sdscatprintf(errors, + "%d: error in ACL: %s. ", + linenum, errmsg); + continue; + } + } + if (j != argc) continue; /* Error in ACL rules, don't apply. */ + + /* We can finally lookup the user and apply the rule. */ } + ACLFreeUser(fakeuser); sdsfreesplitres(lines,totlines); if (sdslen(errors) == 0) { sdsfree(errors); - errors = NULL; + return NULL; + } else { + return sdstrim(errors," "); } - return errors; } /* ============================================================================= From 72e8a080c2d61e008a4134537b3470ed077d5ebc Mon Sep 17 00:00:00 2001 From: antirez Date: Thu, 7 Feb 2019 12:04:25 +0100 Subject: [PATCH 23/35] ACL: fix and complete ACLLoadFromFile() loading step. --- src/acl.c | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/src/acl.c b/src/acl.c index 2fbc564ab..1c6e9a0c5 100644 --- a/src/acl.c +++ b/src/acl.c @@ -1095,10 +1095,12 @@ sds ACLLoadFromFile(const char *filename) { } /* The line should start with the "user" keyword. */ - if (strcmp(argv[0],"user")) { + if (strcmp(argv[0],"user") || argc < 2) { errors = sdscatprintf(errors, - "%d: line should start with user keyword. ", + "%d: line should start with user keyword followed " + "by the username. ", linenum); + sdsfreesplitres(argv,argc); continue; } @@ -1115,9 +1117,26 @@ sds ACLLoadFromFile(const char *filename) { continue; } } - if (j != argc) continue; /* Error in ACL rules, don't apply. */ + if (j != argc) { + sdsfreesplitres(argv,argc); + continue; /* Error in ACL rules, don't apply. */ + } - /* We can finally lookup the user and apply the rule. */ + /* We can finally lookup the user and apply the rule. If the + * user already exists we always reset it to start. */ + user *u = ACLCreateUser(argv[1],sdslen(argv[1])); + if (!u) { + u = ACLGetUserByName(argv[1],sdslen(argv[1])); + serverAssert(u != NULL); + ACLSetUser(u,"reset",-1); + } + + /* Note that the same rules already applied to the fake user, so + * we just assert that everything goess well: it should. */ + for (j = 2; j < argc; j++) + serverAssert(ACLSetUser(fakeuser,argv[j],sdslen(argv[j]) == C_OK); + + sdsfreesplitres(argv,argc); } ACLFreeUser(fakeuser); From 0f0240b526a43eb1323a19f7b706bec68992073c Mon Sep 17 00:00:00 2001 From: antirez Date: Thu, 7 Feb 2019 12:20:30 +0100 Subject: [PATCH 24/35] ACL: implement LOAD subcommand plus some minor rafactoring. --- src/acl.c | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/src/acl.c b/src/acl.c index 1c6e9a0c5..d6b22f7c7 100644 --- a/src/acl.c +++ b/src/acl.c @@ -758,10 +758,9 @@ sds ACLDefaultUserFirstPassword(void) { return listNodeValue(first); } -/* Initialization of the ACL subsystem. */ -void ACLInit(void) { - Users = raxNew(); - UsersToLoad = listCreate(); +/* Initialize the default user, that will always exist for all the process + * lifetime. */ +void ACLInitDefaultUser(void) { DefaultUser = ACLCreateUser("default",7); ACLSetUser(DefaultUser,"+@all",-1); ACLSetUser(DefaultUser,"~*",-1); @@ -769,6 +768,13 @@ void ACLInit(void) { ACLSetUser(DefaultUser,"nopass",-1); } +/* Initialization of the ACL subsystem. */ +void ACLInit(void) { + Users = raxNew(); + UsersToLoad = listCreate(); + ACLInitDefaultUser(); +} + /* Check the username and password pair and return C_OK if they are valid, * otherwise C_ERR is returned and errno is set to: * @@ -1134,7 +1140,7 @@ sds ACLLoadFromFile(const char *filename) { /* Note that the same rules already applied to the fake user, so * we just assert that everything goess well: it should. */ for (j = 2; j < argc; j++) - serverAssert(ACLSetUser(fakeuser,argv[j],sdslen(argv[j]) == C_OK); + serverAssert(ACLSetUser(fakeuser,argv[j],sdslen(argv[j])) == C_OK); sdsfreesplitres(argv,argc); } @@ -1297,6 +1303,19 @@ void aclCommand(client *c) { } else { addReplyNull(c); } + } else if (!strcasecmp(sub,"load")) { + if (server.acl_filename[0] == '\0') { + addReplyError(c,"This Redis instance is not configured to use an ACL file. You may want to specify users via the ACL SETUSER command and then issue a CONFIG REWRITE (assuming you have a Redis configuration file set) in order to store users in the Redis configuration."); + return; + } else { + sds errors = ACLLoadFromFile(server.acl_filename); + if (errors == NULL) { + addReply(c,shared.ok); + } else { + addReplyError(c,errors); + sdsfree(errors); + } + } } else if (!strcasecmp(sub,"help")) { const char *help[] = { "LIST -- Show user details in config file format.", From 1790be1496d14efd6af65c8df2271e0b112740cb Mon Sep 17 00:00:00 2001 From: antirez Date: Thu, 7 Feb 2019 12:57:21 +0100 Subject: [PATCH 25/35] ACL: WIP: preserve the old config on loading errors. --- src/acl.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 66 insertions(+), 2 deletions(-) diff --git a/src/acl.c b/src/acl.c index d6b22f7c7..45c875de9 100644 --- a/src/acl.c +++ b/src/acl.c @@ -90,6 +90,7 @@ struct ACLUserFlag { void ACLResetSubcommandsForCommand(user *u, unsigned long id); void ACLResetSubcommands(user *u); +void ACLAddAllowedSubcommand(user *u, unsigned long id, const char *sub); /* ============================================================================= * Helper functions for the rest of the ACL implementation @@ -163,6 +164,11 @@ void ACLListFreeSds(void *item) { sdsfree(item); } +/* Method to duplicate list elements from ACL users password/ptterns lists. */ +void *ACLListDupSds(void *item) { + return sdsdup(item); +} + /* Create a new user with the specified name, store it in the list * of users (the Users global radix tree), and returns a reference to * the structure representing the user. @@ -178,8 +184,10 @@ user *ACLCreateUser(const char *name, size_t namelen) { u->patterns = listCreate(); listSetMatchMethod(u->passwords,ACLListMatchSds); listSetFreeMethod(u->passwords,ACLListFreeSds); + listSetDupMethod(u->passwords,ACLListDupSds); listSetMatchMethod(u->patterns,ACLListMatchSds); listSetFreeMethod(u->patterns,ACLListFreeSds); + listSetDupMethod(u->patterns,ACLListDupSds); memset(u->allowed_commands,0,sizeof(u->allowed_commands)); raxInsert(Users,(unsigned char*)name,namelen,u,NULL); return u; @@ -212,6 +220,38 @@ void ACLFreeUser(user *u) { zfree(u); } +/* Copy the user ACL rules from the source user 'src' to the destination + * user 'dst' so that at the end of the process they'll have exactly the + * same rules (but the names will continue to be the original ones). */ +void ACLCopyUser(user *dst, user *src) { + listRelease(dst->passwords); + listRelease(dst->patterns); + dst->passwords = listDup(src->passwords); + dst->patterns = listDup(src->patterns); + memcpy(dst->allowed_commands,src->allowed_commands, + sizeof(dst->allowed_commands)); + dst->flags = src->flags; + ACLResetSubcommands(dst); + /* Copy the allowed subcommands array of array of SDS strings. */ + if (src->allowed_subcommands) { + for (int j = 0; j < USER_COMMAND_BITS_COUNT; j++) { + if (src->allowed_subcommands[j]) { + for (int i = 0; src->allowed_subcommands[j][i]; i++) + { + ACLAddAllowedSubcommand(dst, j, + src->allowed_subcommands[j][i]); + } + } + } + } +} + +/* Free all the users registered in the radix tree 'users' and free the + * radix tree itself. */ +void ACLFreeUsersSet(rax *users) { + /* TODO */ +} + /* Given a command ID, this function set by reference 'word' and 'bit' * so that user->allowed_commands[word] will address the right word * where the corresponding bit for the provided ID is stored, and @@ -1043,7 +1083,10 @@ int ACLLoadConfiguredUsers(void) { * to avoid ending with broken rules if the ACL file is invalid for some * reason, so the function will attempt to validate the rules before loading * each user. For every line that will be found broken the function will - * collect an error message. All the valid lines will be correctly processed. + * collect an error message. + * + * IMPORTANT: If there is at least a single error, nothing will be loaded + * and the rules will remain exactly as they were. * * At the end of the process, if no errors were found in the whole file then * NULL is returned. Otherwise an SDS string describing in a single line @@ -1075,6 +1118,14 @@ sds ACLLoadFromFile(const char *filename) { * to the real user mentioned in the ACL line. */ user *fakeuser = ACLCreateUnlinkedUser(); + /* We do all the loading in a fresh insteance of the Users radix tree, + * so if there are errors loading the ACL file we can rollback to the + * old version. */ + rax *old_users = Users; + Users = raxNew(); + ACLInitDefaultUser(); + + /* Load each line of the file. */ for (int i = 0; i < totlines; i++) { sds *argv; int argc; @@ -1147,11 +1198,24 @@ sds ACLLoadFromFile(const char *filename) { ACLFreeUser(fakeuser); sdsfreesplitres(lines,totlines); + + /* Chec if we found errors and react accordingly. */ if (sdslen(errors) == 0) { + /* The default user pointer is referenced in different places: instead + * of replacing such occurrences it is much simpler to copy the new + * default user configuration in the old one. */ + user *new = ACLGetUserByName("default",7); + ACLCopyUser(DefaultUser,new); + ACLFreeUser(new); + raxInsert(Users,(unsigned char*)"default",7,DefaultUser,NULL); + sdsfree(errors); return NULL; } else { - return sdstrim(errors," "); + ACLFreeUsersSet(Users); + Users = old_users; + errors = sdscat(errors,"WARNING: ACL errors detected, no change to the previously active ACL rules was performed"); + return errors; } } From 7a86ba22e001d0b46bff9bf62f5c762471fd53ec Mon Sep 17 00:00:00 2001 From: antirez Date: Thu, 7 Feb 2019 16:20:42 +0100 Subject: [PATCH 26/35] ACL: fix a few ACLLoadFromFile() errors and finish ACLFreeUsersSet(). --- src/acl.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/acl.c b/src/acl.c index 45c875de9..3133bfecf 100644 --- a/src/acl.c +++ b/src/acl.c @@ -249,7 +249,7 @@ void ACLCopyUser(user *dst, user *src) { /* Free all the users registered in the radix tree 'users' and free the * radix tree itself. */ void ACLFreeUsersSet(rax *users) { - /* TODO */ + raxFreeWithCallback(users,(void(*)(void*))ACLFreeUser); } /* Given a command ID, this function set by reference 'word' and 'bit' @@ -1208,7 +1208,8 @@ sds ACLLoadFromFile(const char *filename) { ACLCopyUser(DefaultUser,new); ACLFreeUser(new); raxInsert(Users,(unsigned char*)"default",7,DefaultUser,NULL); - + raxRemove(old_users,(unsigned char*)"default",7,NULL); + ACLFreeUsersSet(old_users); sdsfree(errors); return NULL; } else { From cbed35efd34c08a7610d18269e5860cd874480f9 Mon Sep 17 00:00:00 2001 From: antirez Date: Thu, 7 Feb 2019 16:47:14 +0100 Subject: [PATCH 27/35] ACL: add assertion and fix comment typo. --- src/acl.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/acl.c b/src/acl.c index 3133bfecf..12253f6cf 100644 --- a/src/acl.c +++ b/src/acl.c @@ -1199,12 +1199,13 @@ sds ACLLoadFromFile(const char *filename) { ACLFreeUser(fakeuser); sdsfreesplitres(lines,totlines); - /* Chec if we found errors and react accordingly. */ + /* Check if we found errors and react accordingly. */ if (sdslen(errors) == 0) { /* The default user pointer is referenced in different places: instead * of replacing such occurrences it is much simpler to copy the new * default user configuration in the old one. */ user *new = ACLGetUserByName("default",7); + serverAssert(new != NULL); ACLCopyUser(DefaultUser,new); ACLFreeUser(new); raxInsert(Users,(unsigned char*)"default",7,DefaultUser,NULL); From 6a7545e4d4d3a817091c1ae0be360fae37548053 Mon Sep 17 00:00:00 2001 From: antirez Date: Thu, 7 Feb 2019 16:53:35 +0100 Subject: [PATCH 28/35] ACL: fix fgets wrong buffer size. --- src/acl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/acl.c b/src/acl.c index 12253f6cf..b17036df7 100644 --- a/src/acl.c +++ b/src/acl.c @@ -1105,7 +1105,7 @@ sds ACLLoadFromFile(const char *filename) { /* Load the whole file as a single string in memory. */ sds acls = sdsempty(); - while(fgets(buf,CONFIG_MAX_LINE+1,fp) != NULL) + while(fgets(buf,sizeof(buf),fp) != NULL) acls = sdscat(acls,buf); fclose(fp); From d26c9b5307843fd163fca0e4bd2171ede1c8effc Mon Sep 17 00:00:00 2001 From: antirez Date: Thu, 7 Feb 2019 17:00:35 +0100 Subject: [PATCH 29/35] ACL: ACLLoadFromFile(), restore DefaultUser global. --- src/acl.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/acl.c b/src/acl.c index b17036df7..4ab660e9f 100644 --- a/src/acl.c +++ b/src/acl.c @@ -1122,6 +1122,7 @@ 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; + user *old_default_user = DefaultUser; Users = raxNew(); ACLInitDefaultUser(); @@ -1198,6 +1199,7 @@ sds ACLLoadFromFile(const char *filename) { ACLFreeUser(fakeuser); sdsfreesplitres(lines,totlines); + DefaultUser = old_default_user; /* This pointer must never change. */ /* Check if we found errors and react accordingly. */ if (sdslen(errors) == 0) { From db30727547d7b48b73f92c19bebad2aef1514dfe Mon Sep 17 00:00:00 2001 From: antirez Date: Thu, 7 Feb 2019 17:07:35 +0100 Subject: [PATCH 30/35] ACL: ACLLoadFromFile(): several errors fixed to make it work. --- src/acl.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/src/acl.c b/src/acl.c index 4ab660e9f..fecd33e8a 100644 --- a/src/acl.c +++ b/src/acl.c @@ -1141,8 +1141,8 @@ sds ACLLoadFromFile(const char *filename) { argv = sdssplitargs(lines[i],&argc); if (argv == NULL) { errors = sdscatprintf(errors, - "%d: unbalanced quotes in acl line. ", - linenum); + "%s:%d: unbalanced quotes in acl line. ", + server.acl_filename, linenum); continue; } @@ -1155,8 +1155,8 @@ sds ACLLoadFromFile(const char *filename) { /* The line should start with the "user" keyword. */ if (strcmp(argv[0],"user") || argc < 2) { errors = sdscatprintf(errors, - "%d: line should start with user keyword followed " - "by the username. ", + "%s:%d should start with user keyword followed " + "by the username. ", server.acl_filename, linenum); sdsfreesplitres(argv,argc); continue; @@ -1170,14 +1170,18 @@ sds ACLLoadFromFile(const char *filename) { if (ACLSetUser(fakeuser,argv[j],sdslen(argv[j])) != C_OK) { char *errmsg = ACLSetUserStringError(); errors = sdscatprintf(errors, - "%d: error in ACL: %s. ", - linenum, errmsg); + "%s:%d: %s. ", + server.acl_filename, linenum, errmsg); continue; } } - if (j != argc) { + + /* Apply the rule to the new users set only if so far there + * are no errors, otherwise it's useless since we are going + * to discard the new users set anyway. */ + if (sdslen(errors) != 0) { sdsfreesplitres(argv,argc); - continue; /* Error in ACL rules, don't apply. */ + continue; } /* We can finally lookup the user and apply the rule. If the @@ -1192,7 +1196,7 @@ sds ACLLoadFromFile(const char *filename) { /* Note that the same rules already applied to the fake user, so * we just assert that everything goess well: it should. */ for (j = 2; j < argc; j++) - serverAssert(ACLSetUser(fakeuser,argv[j],sdslen(argv[j])) == C_OK); + serverAssert(ACLSetUser(u,argv[j],sdslen(argv[j])) == C_OK); sdsfreesplitres(argv,argc); } From 80f987726d8ccf9ffc0ce73599226e0e14f26a8a Mon Sep 17 00:00:00 2001 From: antirez Date: Thu, 7 Feb 2019 17:20:03 +0100 Subject: [PATCH 31/35] ACL: load ACL file at startup. Prevent silly configurations. --- src/acl.c | 33 +++++++++++++++++++++++++++++++++ src/server.c | 6 +----- src/server.h | 1 + 3 files changed, 35 insertions(+), 5 deletions(-) diff --git a/src/acl.c b/src/acl.c index fecd33e8a..4ae5830bd 100644 --- a/src/acl.c +++ b/src/acl.c @@ -1227,6 +1227,39 @@ sds ACLLoadFromFile(const char *filename) { } } +/* This function is called once the server is already running, modules are + * loaded, and we are ready to start, in order to load the ACLs either from + * the pending list of users defined in redis.conf, or from the ACL file. + * The function will just exit with an error if the user is trying to mix + * both the loading methods. */ +void ACLLoadUsersAtStartup(void) { + if (server.acl_filename[0] != '\0' && listLength(UsersToLoad) != 0) { + serverLog(LL_WARNING, + "Configuring Redis with users defined in redis.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 users " + "directly in your redis.conf, but not both."); + exit(1); + } + + if (ACLLoadConfiguredUsers() == C_ERR) { + serverLog(LL_WARNING, + "Critical error while loading ACLs. Exiting."); + exit(1); + } + + if (server.acl_filename[0] != '\0') { + sds errors = ACLLoadFromFile(server.acl_filename); + if (errors) { + serverLog(LL_WARNING, + "Aborting Redis startup because of ACL errors: %s", errors); + sdsfree(errors); + exit(1); + } + } +} + /* ============================================================================= * ACL related commands * ==========================================================================*/ diff --git a/src/server.c b/src/server.c index de84e430e..c257d0573 100644 --- a/src/server.c +++ b/src/server.c @@ -4908,11 +4908,7 @@ int main(int argc, char **argv) { linuxMemoryWarnings(); #endif moduleLoadFromQueue(); - if (ACLLoadConfiguredUsers() == C_ERR) { - serverLog(LL_WARNING, - "Critical error while loading ACLs. Exiting."); - exit(1); - } + ACLLoadUsersAtStartup(); loadDataFromDisk(); if (server.cluster_enabled) { if (verifyClusterConfigWithData() == C_ERR) { diff --git a/src/server.h b/src/server.h index d2c6aa1e0..59f7cbe10 100644 --- a/src/server.h +++ b/src/server.h @@ -1746,6 +1746,7 @@ int ACLAppendUserForLoading(sds *argv, int argc, int *argc_err); char *ACLSetUserStringError(void); int ACLLoadConfiguredUsers(void); sds ACLDescribeUser(user *u); +void ACLLoadUsersAtStartup(void); /* Sorted sets data type */ From af8761e4f22c41727a1b2db9109fde4a7520b0be Mon Sep 17 00:00:00 2001 From: antirez Date: Fri, 8 Feb 2019 09:52:07 +0100 Subject: [PATCH 32/35] ACL: some documentation inside redis.conf. --- redis.conf | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 88 insertions(+) diff --git a/redis.conf b/redis.conf index d1ced7eb3..71214b620 100644 --- a/redis.conf +++ b/redis.conf @@ -501,6 +501,94 @@ replica-priority 100 # can be easily a long string from /dev/urandom or whatever, so by using a # long and unguessable password no brute force attack will be possible. +# Redis ACL users are defined in the following format: +# +# user ... acl rules ... +# +# For example: +# +# user worker +@list +@connection ~jobs:* on >ffa9203c493aa99 +# +# The special username "default" is used for new connections. If this user +# has the "nopass" rule, then new connections will be immediately authenticated +# as the "default" user without the need of any password provided via the +# AUTH command. Otherwise if the "default" user is not flagged with "nopass" +# the connections will start in not authenticated state, and will require +# AUTH (or the HELLO command AUTH option) in order to be authenticated and +# start to work. +# +# The ACL rules that describe what an user can do are the following: +# +# on Enable the user: it is possible to authenticate as this user. +# off Disable the user: it's no longer possible to authenticate +# with this user, however the already authenticated connections +# will still work. +# + Allow the execution of that command +# - Disallow the execution of that command +# +@ Allow the execution of all the commands in such category +# with valid categories are like @admin, @set, @sortedset, ... +# and so forth, see the full list in the server.c file where +# the Redis command table is described and defined. +# The special category @all means all the commands, but currently +# present in the server, and that will be loaded in the future +# via modules. +# +|subcommand Allow a specific subcommand of an otherwise +# disabled command. Note that this form is not +# allowed as negative like -DEBUG|SEGFAULT, but +# only additive starting with "+". +# allcommands Alias for +@all. Note that it implies the ability to execute +# all the future commands loaded via the modules system. +# nocommands Alias for -@all. +# ~ Add a pattern of keys that can be mentioned as part of +# commands. For instance ~* allows all the keys. The pattern +# is a glob-style pattern like the one of KEYS. +# It is possible to specify multiple patterns. +# allkeys Alias for ~* +# resetkeys Flush the list of allowed keys patterns. +# > Add this passowrd to the list of valid password for the user. +# For example >mypass will add "mypass" to the list. +# This directive clears the "nopass" flag (see later). +# < Remove this password from the list of valid passwords. +# nopass All the set passwords of the user are removed, and the user +# is flagged as requiring no password: it means that every +# password will work against this user. If this directive is +# used for the default user, every new connection will be +# immediately authenticated with the default user without +# any explicit AUTH command required. Note that the "resetpass" +# directive will clear this condition. +# resetpass Flush the list of allowed passwords. Moreover removes the +# "nopass" status. After "resetpass" the user has no associated +# passwords and there is no way to authenticate without adding +# some password (or setting it as "nopass" later). +# reset Performs the following actions: resetpass, resetkeys, off, +# -@all. The user returns to the same state it has immediately +# after its creation. +# +# ACL rules can be specified in any order: for instance you can start with +# passwords, then flags, or key patterns. However note that the additive +# and subtractive rules will CHANGE MEANING depending on the ordering. +# For instance see the following example: +# +# user alice on +@all -DEBUG ~* >somepassword +# +# This will allow "alice" to use all the commands with the exception of the +# DEBUG command, since +@all added all the commands to the set of the commands +# alice can use, and later DEBUG was removed. However if we invert the order +# of two ACL rules the result will be different: +# +# user alice on -DEBUG +@all ~* >somepassword +# +# Now DEBUG was removed when alice had yet no commands in the set of allowed +# commands, later all the commands are added, so the user will be able to +# execute everything. +# +# Basically ACL rules are processed left-to-right. +# +# For more information about ACL configuration please refer to +# the Redis web site at https://redis.io/topics/acl + +# Using an external ACL file +# # Instead of configuring users here in this file, it is possible to use # a stand-alone file just listing users. The two methods cannot be mixed: # if you configure users here and at the same time you activate the exteranl From d4890c20c10d91e990b8af044a233d3e1da7bda0 Mon Sep 17 00:00:00 2001 From: antirez Date: Fri, 8 Feb 2019 11:50:39 +0100 Subject: [PATCH 33/35] ACL: ignore modules commands when adding categories. We can't trust modules commands flagging, so module commands must be always explicitly added, with the exception of +@all that will include everything. However something like +@readonly should not include command from modules that may be potentially dangerous: our categories must be safe and reliable and modules may not be like that. --- src/acl.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/acl.c b/src/acl.c index 4ae5830bd..159a3507a 100644 --- a/src/acl.c +++ b/src/acl.c @@ -313,6 +313,7 @@ int ACLSetUserCommandBitsForCategory(user *u, const char *category, int value) { dictEntry *de; while ((de = dictNext(di)) != NULL) { struct redisCommand *cmd = dictGetVal(de); + if (cmd->flags & CMD_MODULE) continue; /* Ignore modules commands. */ if (cmd->flags & cflag) { ACLSetUserCommandBit(u,cmd->id,value); ACLResetSubcommandsForCommand(u,cmd->id); From d453936b52ced6e0fbf74a59c1df4147a9870d37 Mon Sep 17 00:00:00 2001 From: antirez Date: Fri, 8 Feb 2019 12:38:41 +0100 Subject: [PATCH 34/35] ACL: add arity check in ACL command where missing. --- src/acl.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/acl.c b/src/acl.c index 159a3507a..2e2bd24a5 100644 --- a/src/acl.c +++ b/src/acl.c @@ -1403,13 +1403,13 @@ void aclCommand(client *c) { } } raxStop(&ri); - } else if (!strcasecmp(sub,"whoami")) { + } else if (!strcasecmp(sub,"whoami") && c->argc == 2) { if (c->user != NULL) { addReplyBulkCBuffer(c,c->user->name,sdslen(c->user->name)); } else { addReplyNull(c); } - } else if (!strcasecmp(sub,"load")) { + } else if (!strcasecmp(sub,"load") && c->argc == 2) { if (server.acl_filename[0] == '\0') { addReplyError(c,"This Redis instance is not configured to use an ACL file. You may want to specify users via the ACL SETUSER command and then issue a CONFIG REWRITE (assuming you have a Redis configuration file set) in order to store users in the Redis configuration."); return; From 3df1eb85ca6b8a9c26b4bf9bb79c7d6172c60032 Mon Sep 17 00:00:00 2001 From: antirez Date: Fri, 8 Feb 2019 12:40:42 +0100 Subject: [PATCH 35/35] ACL: add command fingerprint for CAT subcommand. --- src/acl.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/acl.c b/src/acl.c index 2e2bd24a5..3d73e0843 100644 --- a/src/acl.c +++ b/src/acl.c @@ -1268,7 +1268,9 @@ void ACLLoadUsersAtStartup(void) { /* ACL -- show and modify the configuration of ACL users. * ACL HELP * ACL LIST - * ACL SETUSER ... user attribs ... + * ACL USERS + * ACL CAT [] + * ACL SETUSER ... acl rules ... * ACL DELUSER * ACL GETUSER */ @@ -1429,6 +1431,8 @@ void aclCommand(client *c) { "SETUSER [attribs ...] -- Create or modify a user.", "GETUSER -- Get the user details.", "DELUSER -- Delete a user.", +"CAT -- List available categories.", +"CAT -- List commands inside category.", "WHOAMI -- Return the current connection username.", NULL };