From ec0c61da05db61fccd24905387f2ca95828902a1 Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Mon, 3 Feb 2020 15:58:28 +0200 Subject: [PATCH 01/15] fix uninitialized info_cb var in module.c --- src/module.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/module.c b/src/module.c index d922c5c20..914c50df3 100644 --- a/src/module.c +++ b/src/module.c @@ -859,6 +859,7 @@ void RM_SetModuleAttribs(RedisModuleCtx *ctx, const char *name, int ver, int api module->in_call = 0; module->in_hook = 0; module->options = 0; + module->info_cb = 0; ctx->module = module; } From 29d4a1502a80afa92079897a57836f5a7e57586b Mon Sep 17 00:00:00 2001 From: Yossi Gottlieb Date: Wed, 29 Jan 2020 21:40:02 +0200 Subject: [PATCH 02/15] TLS: Fix missing initialization in redis-cli. --- src/redis-cli.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/redis-cli.c b/src/redis-cli.c index 065c389c6..1919829e1 100644 --- a/src/redis-cli.c +++ b/src/redis-cli.c @@ -49,6 +49,7 @@ #include #ifdef USE_OPENSSL #include +#include #include #endif #include /* use sds.h from hiredis, so that only one set of sds functions will be present in the binary */ @@ -7933,6 +7934,14 @@ int main(int argc, char **argv) { parseEnv(); +#ifdef USE_OPENSSL + if (config.tls) { + ERR_load_crypto_strings(); + SSL_load_error_strings(); + SSL_library_init(); + } +#endif + /* Cluster Manager mode */ if (CLUSTER_MANAGER_MODE()) { clusterManagerCommandProc *proc = validateClusterManagerCommand(); From d2509811b7074a14113e506a434308abb7f54246 Mon Sep 17 00:00:00 2001 From: WuYunlong Date: Tue, 4 Feb 2020 16:34:11 +0800 Subject: [PATCH 03/15] Add tcl regression test in scripting.tcl to reproduce memory leak. --- tests/unit/scripting.tcl | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/unit/scripting.tcl b/tests/unit/scripting.tcl index 2543a0377..fb36d0b80 100644 --- a/tests/unit/scripting.tcl +++ b/tests/unit/scripting.tcl @@ -741,3 +741,8 @@ start_server {tags {"scripting repl"}} { } } +start_server {tags {"scripting"}} { + r script debug sync + r eval {return 'hello'} 0 + r eval {return 'hello'} 0 +} From eecfa9793e27d9aec2808e208422859dbcc49af4 Mon Sep 17 00:00:00 2001 From: WuYunlong Date: Tue, 4 Feb 2020 16:38:46 +0800 Subject: [PATCH 04/15] Fix lua related memory leak. --- src/scripting.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/scripting.c b/src/scripting.c index 9282b7fd9..a5c59b113 100644 --- a/src/scripting.c +++ b/src/scripting.c @@ -2473,6 +2473,7 @@ void ldbEval(lua_State *lua, sds *argv, int argc) { ldbLog(sdscatfmt(sdsempty()," %s",lua_tostring(lua,-1))); lua_pop(lua,1); sdsfree(code); + sdsfree(expr); return; } } From f7a94526dd93ebb56b1a21ba86a71329c4e3f6e4 Mon Sep 17 00:00:00 2001 From: Leo Murillo Date: Sun, 2 Feb 2020 02:48:00 -0600 Subject: [PATCH 05/15] Set ZSKIPLIST_MAXLEVEL to optimal value given 2^64 elements and p=0.25 --- src/server.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/server.h b/src/server.h index 8e354c03d..5d63e9b55 100644 --- a/src/server.h +++ b/src/server.h @@ -335,7 +335,7 @@ typedef long long ustime_t; /* microsecond time type. */ /* Anti-warning macro... */ #define UNUSED(V) ((void) V) -#define ZSKIPLIST_MAXLEVEL 64 /* Should be enough for 2^64 elements */ +#define ZSKIPLIST_MAXLEVEL 32 /* Should be enough for 2^64 elements */ #define ZSKIPLIST_P 0.25 /* Skiplist P = 1/4 */ /* Append only defines */ From 577fc4388b4701d78244ec37d4dd8a5e7abe35ca Mon Sep 17 00:00:00 2001 From: antirez Date: Mon, 27 Jan 2020 18:37:52 +0100 Subject: [PATCH 06/15] ACL LOG: data structures and initial functions. --- src/acl.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++- src/multi.c | 2 +- src/scripting.c | 2 +- src/server.c | 2 +- src/server.h | 2 +- 5 files changed, 54 insertions(+), 5 deletions(-) diff --git a/src/acl.c b/src/acl.c index 1f395bd3f..4391382a6 100644 --- a/src/acl.c +++ b/src/acl.c @@ -49,6 +49,8 @@ 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 *ACLLog; /* Our security log, the user is able to inspect that + using the ACL LOG command .*/ struct ACLCategoryItem { const char *name; @@ -920,6 +922,7 @@ void ACLInitDefaultUser(void) { void ACLInit(void) { Users = raxNew(); UsersToLoad = listCreate(); + ACLLog = listCreate(); ACLInitDefaultUser(); } @@ -1034,7 +1037,7 @@ user *ACLGetUserByName(const char *name, size_t namelen) { * command cannot be executed because the user is not allowed to run such * command, the second if the command is denied because the user is trying * to access keys that are not among the specified patterns. */ -int ACLCheckCommandPerm(client *c) { +int ACLCheckCommandPerm(client *c, int *keyidxptr) { user *u = c->user; uint64_t id = c->cmd->id; @@ -1094,6 +1097,7 @@ int ACLCheckCommandPerm(client *c) { } } if (!match) { + if (keyidxptr) *keyidxptr = keyidx[j]; getKeysFreeResult(keyidx); return ACL_DENIED_KEY; } @@ -1454,6 +1458,51 @@ void ACLLoadUsersAtStartup(void) { } } +/* ============================================================================= + * ACL log + * ==========================================================================*/ + +#define ACL_LOG_CTX_TOPLEVEL 0 +#define ACL_LOG_CTX_LUA 1 +#define ACL_LOG_CTX_MULTI 2 + +/* This structure defines an entry inside the ACL log. */ +typedef struct aclLogEntry { + uint64_t count; /* Number of times this happened recently. */ + int reason; /* Reason for denying the command. ACL_DENIED_*. */ + int context; /* Toplevel, Lua or MULTI/EXEC? ACL_LOG_CTX_*. */ + sds object; /* The key name or command name. */ + sds username; /* User the client is authenticated with. */ + mstime_t ctime; /* Milliseconds time of last update to this entry. */ + sds cinfo; /* Client info (last client if updated). */ +} aclLogEntry; + +void addACLLogEntry(client *c, int reason, int keypos) { + /* Create a new entry. */ + struct aclLogEntry *le = zmalloc(sizeof(*le)); + le->count = 1; + le->object = (reason == ACL_DENIED_CMD) ? sdsnew(c->cmd->name) : + sdsdup(c->argv[keypos]->ptr); + le->username = sdsdup(c->user->name); + le->ctime = mstime(); + + client *realclient = c; + if (realclient->flags & CLIENT_LUA) realclient = server.lua_caller; + + le->cinfo = catClientInfoString(sdsempty(),realclient); + if (c->flags & CLIENT_MULTI) { + le->context = ACL_LOG_CTX_MULTI; + } else if (c->flags & CLIENT_LUA) { + le->context = ACL_LOG_CTX_LUA; + } else { + le->context = ACL_LOG_CTX_TOPLEVEL; + } + + /* Add it to our list of entires. We'll have to trim the list + * to its maximum size. */ + listAddNodeHead(ACLLog, le); +} + /* ============================================================================= * ACL related commands * ==========================================================================*/ diff --git a/src/multi.c b/src/multi.c index df11225bd..640149870 100644 --- a/src/multi.c +++ b/src/multi.c @@ -177,7 +177,7 @@ void execCommand(client *c) { must_propagate = 1; } - int acl_retval = ACLCheckCommandPerm(c); + int acl_retval = ACLCheckCommandPerm(c,NULL); if (acl_retval != ACL_OK) { addReplyErrorFormat(c, "-NOPERM ACLs rules changed between the moment the " diff --git a/src/scripting.c b/src/scripting.c index a5c59b113..81e174870 100644 --- a/src/scripting.c +++ b/src/scripting.c @@ -606,7 +606,7 @@ int luaRedisGenericCommand(lua_State *lua, int raise_error) { } /* Check the ACLs. */ - int acl_retval = ACLCheckCommandPerm(c); + int acl_retval = ACLCheckCommandPerm(c,NULL); if (acl_retval != ACL_OK) { if (acl_retval == ACL_DENIED_CMD) luaPushError(lua, "The user executing the script can't run this " diff --git a/src/server.c b/src/server.c index 2b226f568..b5e27e238 100644 --- a/src/server.c +++ b/src/server.c @@ -3377,7 +3377,7 @@ int processCommand(client *c) { /* Check if the user can run this command according to the current * ACLs. */ - int acl_retval = ACLCheckCommandPerm(c); + int acl_retval = ACLCheckCommandPerm(c,NULL); if (acl_retval != ACL_OK) { flagTransaction(c); if (acl_retval == ACL_DENIED_CMD) diff --git a/src/server.h b/src/server.h index 5d63e9b55..fe27cf232 100644 --- a/src/server.h +++ b/src/server.h @@ -1824,7 +1824,7 @@ int ACLCheckUserCredentials(robj *username, robj *password); int ACLAuthenticateUser(client *c, robj *username, robj *password); unsigned long ACLGetCommandID(const char *cmdname); user *ACLGetUserByName(const char *name, size_t namelen); -int ACLCheckCommandPerm(client *c); +int ACLCheckCommandPerm(client *c, int *keyidxptr); int ACLSetUser(user *u, const char *op, ssize_t oplen); sds ACLDefaultUserFirstPassword(void); uint64_t ACLGetCommandCategoryFlagByName(const char *name); From d9b153c9f66877a659ad2c87d178490cbd377749 Mon Sep 17 00:00:00 2001 From: antirez Date: Tue, 28 Jan 2020 17:30:50 +0100 Subject: [PATCH 07/15] ACL LOG: implement ACL LOG subcommadn skeleton. --- src/acl.c | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/src/acl.c b/src/acl.c index 4391382a6..a166469d0 100644 --- a/src/acl.c +++ b/src/acl.c @@ -1477,6 +1477,11 @@ typedef struct aclLogEntry { sds cinfo; /* Client info (last client if updated). */ } aclLogEntry; +/* Adds a new entry in the ACL log, making sure to delete the old entry + * if we reach the maximum length allowed for the log. This function attempts + * to find similar entries in the current log in order to bump the counter of + * the log entry instead of creating many entries for very similar ACL + * rules issues. */ void addACLLogEntry(client *c, int reason, int keypos) { /* Create a new entry. */ struct aclLogEntry *le = zmalloc(sizeof(*le)); @@ -1518,6 +1523,7 @@ void addACLLogEntry(client *c, int reason, int keypos) { * ACL GETUSER * ACL GENPASS * ACL WHOAMI + * ACL LOG [ | RESET] */ void aclCommand(client *c) { char *sub = c->argv[1]->ptr; @@ -1704,6 +1710,36 @@ void aclCommand(client *c) { char pass[32]; /* 128 bits of actual pseudo random data. */ getRandomHexChars(pass,sizeof(pass)); addReplyBulkCBuffer(c,pass,sizeof(pass)); + } else if (!strcasecmp(sub,"log") && (c->argc == 2 || c->argc ==3)) { + long count = 10; /* Number of entries to emit by default. */ + + /* Parse the only argument that LOG may have: it could be either + * the number of entires the user wants to display, or alternatively + * the "RESET" command in order to flush the old entires. */ + if (c->argc == 3) { + if (!strcasecmp(c->argv[2]->ptr,"reset")) { + /* TODO: reset the log. */ + addReply(c,shared.ok); + return; + } else if (getLongFromObjectOrReply(c,c->argv[2],&count,NULL) + != C_OK) + { + return; + } + if (count < 0) count = 0; + } + + /* Fix the count according to the number of entries we got. */ + if ((size_t)count > listLength(ACLLog)) + count = listLength(ACLLog); + + addReplyArrayLen(c,count); + listIter li; + listNode *ln; + listRewind(ACLLog,&li); + while (count-- && (ln = listNext(&li)) != NULL) { + addReplyLongLong(c,1234); + } } else if (!strcasecmp(sub,"help")) { const char *help[] = { "LOAD -- Reload users from the ACL file.", @@ -1716,6 +1752,7 @@ void aclCommand(client *c) { "CAT -- List commands inside category.", "GENPASS -- Generate a secure user password.", "WHOAMI -- Return the current connection username.", +"LOG [ | RESET] -- Show the ACL log entries.", NULL }; addReplyHelp(c,help); From f1974d5d67a4b5489cfc4225da7bfe13ba10b563 Mon Sep 17 00:00:00 2001 From: antirez Date: Tue, 28 Jan 2020 18:04:20 +0100 Subject: [PATCH 08/15] ACL LOG: actually emit entries. --- src/acl.c | 34 ++++++++++++++++++++++++++++++---- src/server.c | 4 +++- src/server.h | 1 + 3 files changed, 34 insertions(+), 5 deletions(-) diff --git a/src/acl.c b/src/acl.c index a166469d0..5937c069c 100644 --- a/src/acl.c +++ b/src/acl.c @@ -1467,7 +1467,7 @@ void ACLLoadUsersAtStartup(void) { #define ACL_LOG_CTX_MULTI 2 /* This structure defines an entry inside the ACL log. */ -typedef struct aclLogEntry { +typedef struct ACLLogEntry { uint64_t count; /* Number of times this happened recently. */ int reason; /* Reason for denying the command. ACL_DENIED_*. */ int context; /* Toplevel, Lua or MULTI/EXEC? ACL_LOG_CTX_*. */ @@ -1475,7 +1475,7 @@ typedef struct aclLogEntry { sds username; /* User the client is authenticated with. */ mstime_t ctime; /* Milliseconds time of last update to this entry. */ sds cinfo; /* Client info (last client if updated). */ -} aclLogEntry; +} ACLLogEntry; /* Adds a new entry in the ACL log, making sure to delete the old entry * if we reach the maximum length allowed for the log. This function attempts @@ -1484,8 +1484,9 @@ typedef struct aclLogEntry { * rules issues. */ void addACLLogEntry(client *c, int reason, int keypos) { /* Create a new entry. */ - struct aclLogEntry *le = zmalloc(sizeof(*le)); + struct ACLLogEntry *le = zmalloc(sizeof(*le)); le->count = 1; + le->reason = reason; le->object = (reason == ACL_DENIED_CMD) ? sdsnew(c->cmd->name) : sdsdup(c->argv[keypos]->ptr); le->username = sdsdup(c->user->name); @@ -1737,8 +1738,33 @@ void aclCommand(client *c) { listIter li; listNode *ln; listRewind(ACLLog,&li); + mstime_t now = mstime(); while (count-- && (ln = listNext(&li)) != NULL) { - addReplyLongLong(c,1234); + ACLLogEntry *le = listNodeValue(ln); + addReplyMapLen(c,7); + addReplyBulkCString(c,"count"); + addReplyLongLong(c,le->count); + addReplyBulkCString(c,"reason"); + addReplyBulkCString(c,(le->reason == ACL_DENIED_CMD) ? + "command" : "key"); + char *ctxstr; + switch(le->context) { + case ACL_LOG_CTX_TOPLEVEL: ctxstr="toplevel"; break; + case ACL_LOG_CTX_MULTI: ctxstr="multi"; break; + case ACL_LOG_CTX_LUA: ctxstr="lua"; break; + default: ctxstr="unknown"; + } + addReplyBulkCString(c,"context"); + addReplyBulkCString(c,ctxstr); + addReplyBulkCString(c,"object"); + addReplyBulkCBuffer(c,le->object,sdslen(le->object)); + addReplyBulkCString(c,"username"); + addReplyBulkCBuffer(c,le->username,sdslen(le->username)); + addReplyBulkCString(c,"age-seconds"); + double age = (double)(now - le->ctime)/1000; + addReplyDouble(c,age); + addReplyBulkCString(c,"client-info"); + addReplyBulkCBuffer(c,le->cinfo,sdslen(le->cinfo)); } } else if (!strcasecmp(sub,"help")) { const char *help[] = { diff --git a/src/server.c b/src/server.c index b5e27e238..6968f311f 100644 --- a/src/server.c +++ b/src/server.c @@ -3377,8 +3377,10 @@ int processCommand(client *c) { /* Check if the user can run this command according to the current * ACLs. */ - int acl_retval = ACLCheckCommandPerm(c,NULL); + int acl_keypos; + int acl_retval = ACLCheckCommandPerm(c,&acl_keypos); if (acl_retval != ACL_OK) { + addACLLogEntry(c,acl_retval,acl_keypos); flagTransaction(c); if (acl_retval == ACL_DENIED_CMD) addReplyErrorFormat(c, diff --git a/src/server.h b/src/server.h index fe27cf232..228a14f34 100644 --- a/src/server.h +++ b/src/server.h @@ -1836,6 +1836,7 @@ void ACLLoadUsersAtStartup(void); void addReplyCommandCategories(client *c, struct redisCommand *cmd); user *ACLCreateUnlinkedUser(); void ACLFreeUserAndKillClients(user *u); +void addACLLogEntry(client *c, int reason, int keypos); /* Sorted sets data type */ From e271a61103b8b57371e3283eddff5ef1b7b09716 Mon Sep 17 00:00:00 2001 From: antirez Date: Wed, 29 Jan 2020 18:40:32 +0100 Subject: [PATCH 09/15] ACL LOG: group similar entries in a given time delta. --- src/acl.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 58 insertions(+), 3 deletions(-) diff --git a/src/acl.c b/src/acl.c index 5937c069c..d03599a79 100644 --- a/src/acl.c +++ b/src/acl.c @@ -1465,6 +1465,7 @@ void ACLLoadUsersAtStartup(void) { #define ACL_LOG_CTX_TOPLEVEL 0 #define ACL_LOG_CTX_LUA 1 #define ACL_LOG_CTX_MULTI 2 +#define ACL_LOG_GROUPING_MAX_TIME_DELTA 60000 /* This structure defines an entry inside the ACL log. */ typedef struct ACLLogEntry { @@ -1477,6 +1478,28 @@ typedef struct ACLLogEntry { sds cinfo; /* Client info (last client if updated). */ } ACLLogEntry; +/* This function will check if ACL entries 'a' and 'b' are similar enough + * that we should actually update the existing entry in our ACL log instead + * of creating a new one. */ +int ACLLogMatchEntry(ACLLogEntry *a, ACLLogEntry *b) { + if (a->reason != b->reason) return 0; + if (a->context != b->context) return 0; + mstime_t delta = a->ctime - b->ctime; + if (delta < 0) delta = -delta; + if (delta > ACL_LOG_GROUPING_MAX_TIME_DELTA) return 0; + if (sdscmp(a->object,b->object) != 0) return 0; + if (sdscmp(a->username,b->username) != 0) return 0; + return 1; +} + +/* Release an ACL log entry. */ +void ACLFreeLogEntry(ACLLogEntry *le) { + sdsfree(le->object); + sdsfree(le->username); + sdsfree(le->cinfo); + zfree(le); +} + /* Adds a new entry in the ACL log, making sure to delete the old entry * if we reach the maximum length allowed for the log. This function attempts * to find similar entries in the current log in order to bump the counter of @@ -1504,9 +1527,41 @@ void addACLLogEntry(client *c, int reason, int keypos) { le->context = ACL_LOG_CTX_TOPLEVEL; } - /* Add it to our list of entires. We'll have to trim the list - * to its maximum size. */ - listAddNodeHead(ACLLog, le); + /* Try to match this entry with past ones, to see if we can just + * update an existing entry instead of creating a new one. */ + long toscan = 10; /* Do a limited work trying to find duplicated. */ + listIter li; + listNode *ln; + listRewind(ACLLog,&li); + ACLLogEntry *match = NULL; + while (toscan-- && (ln = listNext(&li)) != NULL) { + ACLLogEntry *current = listNodeValue(ln); + if (ACLLogMatchEntry(current,le)) { + match = current; + listDelNode(ACLLog,ln); + listAddNodeHead(ACLLog,current); + break; + } + } + + /* If there is a match update the entry, otherwise add it as a + * new one. */ + if (match) { + /* We update a few fields of the existing entry and bump the + * counter of events for this entry. */ + sdsfree(match->cinfo); + match->cinfo = le->cinfo; + match->ctime = le->ctime; + match->count++; + + /* Release the old entry. */ + le->cinfo = NULL; + ACLFreeLogEntry(le); + } else { + /* Add it to our list of entires. We'll have to trim the list + * to its maximum size. */ + listAddNodeHead(ACLLog, le); + } } /* ============================================================================= From 943008ebac4ef23d30a2a32dd2900e25794811d5 Mon Sep 17 00:00:00 2001 From: antirez Date: Wed, 29 Jan 2020 18:51:04 +0100 Subject: [PATCH 10/15] ACL LOG: implement LOG RESET. --- src/acl.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/acl.c b/src/acl.c index d03599a79..16acd4d3e 100644 --- a/src/acl.c +++ b/src/acl.c @@ -95,6 +95,7 @@ struct ACLUserFlag { void ACLResetSubcommandsForCommand(user *u, unsigned long id); void ACLResetSubcommands(user *u); void ACLAddAllowedSubcommand(user *u, unsigned long id, const char *sub); +void ACLFreeLogEntry(void *le); /* The length of the string representation of a hashed password. */ #define HASH_PASSWORD_LEN SHA256_BLOCK_SIZE*2 @@ -1493,7 +1494,8 @@ int ACLLogMatchEntry(ACLLogEntry *a, ACLLogEntry *b) { } /* Release an ACL log entry. */ -void ACLFreeLogEntry(ACLLogEntry *le) { +void ACLFreeLogEntry(void *leptr) { + ACLLogEntry *le = leptr; sdsfree(le->object); sdsfree(le->username); sdsfree(le->cinfo); @@ -1774,7 +1776,9 @@ void aclCommand(client *c) { * the "RESET" command in order to flush the old entires. */ if (c->argc == 3) { if (!strcasecmp(c->argv[2]->ptr,"reset")) { - /* TODO: reset the log. */ + listSetFreeMethod(ACLLog,ACLFreeLogEntry); + listEmpty(ACLLog); + listSetFreeMethod(ACLLog,NULL); addReply(c,shared.ok); return; } else if (getLongFromObjectOrReply(c,c->argv[2],&count,NULL) From 82790e510f30094efecfc865ffe4c7464c50825a Mon Sep 17 00:00:00 2001 From: antirez Date: Thu, 30 Jan 2020 10:50:32 +0100 Subject: [PATCH 11/15] ACL LOG: also log ACL errors in the scripting/MULTI ctx. --- src/multi.c | 4 +++- src/scripting.c | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/multi.c b/src/multi.c index 640149870..a88e5336b 100644 --- a/src/multi.c +++ b/src/multi.c @@ -177,8 +177,10 @@ void execCommand(client *c) { must_propagate = 1; } - int acl_retval = ACLCheckCommandPerm(c,NULL); + int acl_keypos; + int acl_retval = ACLCheckCommandPerm(c,&acl_keypos); if (acl_retval != ACL_OK) { + addACLLogEntry(c,acl_retval,acl_keypos); addReplyErrorFormat(c, "-NOPERM ACLs rules changed between the moment the " "transaction was accumulated and the EXEC call. " diff --git a/src/scripting.c b/src/scripting.c index 81e174870..1007a55f7 100644 --- a/src/scripting.c +++ b/src/scripting.c @@ -606,8 +606,10 @@ int luaRedisGenericCommand(lua_State *lua, int raise_error) { } /* Check the ACLs. */ - int acl_retval = ACLCheckCommandPerm(c,NULL); + int acl_keypos; + int acl_retval = ACLCheckCommandPerm(c,&acl_keypos); if (acl_retval != ACL_OK) { + addACLLogEntry(c,acl_retval,acl_keypos); if (acl_retval == ACL_DENIED_CMD) luaPushError(lua, "The user executing the script can't run this " "command or subcommand"); From 9f6e84f6beabfc51e24580353ade795552cef8e2 Mon Sep 17 00:00:00 2001 From: antirez Date: Thu, 30 Jan 2020 11:09:50 +0100 Subject: [PATCH 12/15] ACL LOG: implement a few basic tests. --- tests/unit/acl.tcl | 87 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 87 insertions(+) diff --git a/tests/unit/acl.tcl b/tests/unit/acl.tcl index 2205d2d86..bd909d36c 100644 --- a/tests/unit/acl.tcl +++ b/tests/unit/acl.tcl @@ -141,4 +141,91 @@ start_server {tags {"acl"}} { r ACL setuser newuser -debug # The test framework will detect a leak if any. } + + test {ACL LOG shows failed command executions at toplevel} { + r ACL LOG RESET + r ACL setuser antirez >foo on +set ~object:1234 + r ACL setuser antirez +eval +multi +exec + r AUTH antirez foo + catch {r GET foo} + r AUTH default "" + set entry [lindex [r ACL LOG] 0] + assert {[dict get $entry username] eq {antirez}} + assert {[dict get $entry context] eq {toplevel}} + assert {[dict get $entry reason] eq {command}} + assert {[dict get $entry object] eq {get}} + } + + test {ACL LOG is able to test similar events} { + r AUTH antirez foo + catch {r GET foo} + catch {r GET foo} + catch {r GET foo} + r AUTH default "" + set entry [lindex [r ACL LOG] 0] + assert {[dict get $entry count] == 4} + } + + test {ACL LOG is able to log keys access violations and key name} { + r AUTH antirez foo + catch {r SET somekeynotallowed 1234} + r AUTH default "" + set entry [lindex [r ACL LOG] 0] + assert {[dict get $entry reason] eq {key}} + assert {[dict get $entry object] eq {somekeynotallowed}} + } + + test {ACL LOG RESET is able to flush the entries in the log} { + r ACL LOG RESET + assert {[llength [r ACL LOG]] == 0} + } + + test {ACL LOG can distinguish the transaction context (1)} { + r AUTH antirez foo + r MULTI + catch {r INCR foo} + catch {r EXEC} + r AUTH default "" + set entry [lindex [r ACL LOG] 0] + assert {[dict get $entry context] eq {multi}} + assert {[dict get $entry object] eq {incr}} + } + + test {ACL LOG can distinguish the transaction context (2)} { + set rd1 [redis_deferring_client] + r ACL SETUSER antirez +incr + + r AUTH antirez foo + r MULTI + r INCR object:1234 + $rd1 ACL SETUSER antirez -incr + $rd1 read + catch {r EXEC} + $rd1 close + r AUTH default "" + set entry [lindex [r ACL LOG] 0] + assert {[dict get $entry context] eq {multi}} + assert {[dict get $entry object] eq {incr}} + r ACL SETUSER antirez -incr + } + + test {ACL can log errors in the context of Lua scripting} { + r AUTH antirez foo + catch {r EVAL {redis.call('incr','foo')} 0} + r AUTH default "" + set entry [lindex [r ACL LOG] 0] + assert {[dict get $entry context] eq {lua}} + assert {[dict get $entry object] eq {incr}} + } + + test {ACL LOG can accept a numerical argument to show less entries} { + r AUTH antirez foo + catch {r INCR foo} + catch {r INCR foo} + catch {r INCR foo} + catch {r INCR foo} + r AUTH default "" + assert {[llength [r ACL LOG]] > 1} + assert {[llength [r ACL LOG 2]] == 2} + } } From 7379c78a9b584ca5f394bae7ae7139ee92fa88bb Mon Sep 17 00:00:00 2001 From: antirez Date: Tue, 4 Feb 2020 12:55:26 +0100 Subject: [PATCH 13/15] ACL LOG: log failed auth attempts. --- src/acl.c | 37 +++++++++++++++++++++++++++++-------- src/multi.c | 2 +- src/scripting.c | 2 +- src/server.c | 2 +- src/server.h | 3 ++- 5 files changed, 34 insertions(+), 12 deletions(-) diff --git a/src/acl.c b/src/acl.c index 16acd4d3e..97c00d4f8 100644 --- a/src/acl.c +++ b/src/acl.c @@ -982,6 +982,7 @@ int ACLAuthenticateUser(client *c, robj *username, robj *password) { moduleNotifyUserChanged(c); return C_OK; } else { + addACLLogEntry(c,ACL_DENIED_AUTH,0,username->ptr); return C_ERR; } } @@ -1506,17 +1507,29 @@ void ACLFreeLogEntry(void *leptr) { * if we reach the maximum length allowed for the log. This function attempts * to find similar entries in the current log in order to bump the counter of * the log entry instead of creating many entries for very similar ACL - * rules issues. */ -void addACLLogEntry(client *c, int reason, int keypos) { + * rules issues. + * + * The keypos argument is only used when the reason is ACL_DENIED_KEY, since + * it allows the function to log the key name that caused the problem. + * Similarly the username is only passed when we failed to authenticate the + * user with AUTH or HELLO, for the ACL_DENIED_AUTH reason. Otherwise + * it will just be NULL. + */ +void addACLLogEntry(client *c, int reason, int keypos, sds username) { /* Create a new entry. */ struct ACLLogEntry *le = zmalloc(sizeof(*le)); le->count = 1; le->reason = reason; - le->object = (reason == ACL_DENIED_CMD) ? sdsnew(c->cmd->name) : - sdsdup(c->argv[keypos]->ptr); - le->username = sdsdup(c->user->name); + le->username = sdsdup(reason == ACL_DENIED_AUTH ? username : c->user->name); le->ctime = mstime(); + switch(reason) { + case ACL_DENIED_CMD: le->object = sdsnew(c->cmd->name); break; + case ACL_DENIED_KEY: le->object = sdsnew(c->argv[keypos]->ptr); break; + case ACL_DENIED_AUTH: le->object = sdsnew(c->argv[0]->ptr); break; + default: le->object = sdsempty(); + } + client *realclient = c; if (realclient->flags & CLIENT_LUA) realclient = server.lua_caller; @@ -1803,9 +1816,17 @@ void aclCommand(client *c) { addReplyMapLen(c,7); addReplyBulkCString(c,"count"); addReplyLongLong(c,le->count); + addReplyBulkCString(c,"reason"); - addReplyBulkCString(c,(le->reason == ACL_DENIED_CMD) ? - "command" : "key"); + char *reasonstr; + switch(le->reason) { + case ACL_DENIED_CMD: reasonstr="command"; break; + case ACL_DENIED_KEY: reasonstr="key"; break; + case ACL_DENIED_AUTH: reasonstr="auth"; break; + } + addReplyBulkCString(c,reasonstr); + + addReplyBulkCString(c,"context"); char *ctxstr; switch(le->context) { case ACL_LOG_CTX_TOPLEVEL: ctxstr="toplevel"; break; @@ -1813,8 +1834,8 @@ void aclCommand(client *c) { case ACL_LOG_CTX_LUA: ctxstr="lua"; break; default: ctxstr="unknown"; } - addReplyBulkCString(c,"context"); addReplyBulkCString(c,ctxstr); + addReplyBulkCString(c,"object"); addReplyBulkCBuffer(c,le->object,sdslen(le->object)); addReplyBulkCString(c,"username"); diff --git a/src/multi.c b/src/multi.c index a88e5336b..cbbd2c513 100644 --- a/src/multi.c +++ b/src/multi.c @@ -180,7 +180,7 @@ void execCommand(client *c) { int acl_keypos; int acl_retval = ACLCheckCommandPerm(c,&acl_keypos); if (acl_retval != ACL_OK) { - addACLLogEntry(c,acl_retval,acl_keypos); + addACLLogEntry(c,acl_retval,acl_keypos,NULL); addReplyErrorFormat(c, "-NOPERM ACLs rules changed between the moment the " "transaction was accumulated and the EXEC call. " diff --git a/src/scripting.c b/src/scripting.c index 1007a55f7..7f64e06db 100644 --- a/src/scripting.c +++ b/src/scripting.c @@ -609,7 +609,7 @@ int luaRedisGenericCommand(lua_State *lua, int raise_error) { int acl_keypos; int acl_retval = ACLCheckCommandPerm(c,&acl_keypos); if (acl_retval != ACL_OK) { - addACLLogEntry(c,acl_retval,acl_keypos); + addACLLogEntry(c,acl_retval,acl_keypos,NULL); if (acl_retval == ACL_DENIED_CMD) luaPushError(lua, "The user executing the script can't run this " "command or subcommand"); diff --git a/src/server.c b/src/server.c index 6968f311f..2827188e0 100644 --- a/src/server.c +++ b/src/server.c @@ -3380,7 +3380,7 @@ int processCommand(client *c) { int acl_keypos; int acl_retval = ACLCheckCommandPerm(c,&acl_keypos); if (acl_retval != ACL_OK) { - addACLLogEntry(c,acl_retval,acl_keypos); + addACLLogEntry(c,acl_retval,acl_keypos,NULL); flagTransaction(c); if (acl_retval == ACL_DENIED_CMD) addReplyErrorFormat(c, diff --git a/src/server.h b/src/server.h index 228a14f34..e59fac22b 100644 --- a/src/server.h +++ b/src/server.h @@ -1820,6 +1820,7 @@ void ACLInit(void); #define ACL_OK 0 #define ACL_DENIED_CMD 1 #define ACL_DENIED_KEY 2 +#define ACL_DENIED_AUTH 3 /* Only used for ACL LOG entries. */ int ACLCheckUserCredentials(robj *username, robj *password); int ACLAuthenticateUser(client *c, robj *username, robj *password); unsigned long ACLGetCommandID(const char *cmdname); @@ -1836,7 +1837,7 @@ void ACLLoadUsersAtStartup(void); void addReplyCommandCategories(client *c, struct redisCommand *cmd); user *ACLCreateUnlinkedUser(); void ACLFreeUserAndKillClients(user *u); -void addACLLogEntry(client *c, int reason, int keypos); +void addACLLogEntry(client *c, int reason, int keypos, sds username); /* Sorted sets data type */ From ea1e1b12c94c3e17707136d0d859a610260a3bea Mon Sep 17 00:00:00 2001 From: antirez Date: Tue, 4 Feb 2020 12:58:48 +0100 Subject: [PATCH 14/15] ACL LOG: test for AUTH reason. --- tests/unit/acl.tcl | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/unit/acl.tcl b/tests/unit/acl.tcl index bd909d36c..0e6d5c66a 100644 --- a/tests/unit/acl.tcl +++ b/tests/unit/acl.tcl @@ -228,4 +228,13 @@ start_server {tags {"acl"}} { assert {[llength [r ACL LOG]] > 1} assert {[llength [r ACL LOG 2]] == 2} } + + test {ACL LOG can log failed auth attempts} { + catch {r AUTH antirez wrong-password} + set entry [lindex [r ACL LOG] 0] + assert {[dict get $entry context] eq {toplevel}} + assert {[dict get $entry reason] eq {auth}} + assert {[dict get $entry object] eq {AUTH}} + assert {[dict get $entry username] eq {antirez}} + } } From 51c1a9f8fbc12a9276489178242e498bb6ccbdba Mon Sep 17 00:00:00 2001 From: antirez Date: Tue, 4 Feb 2020 13:19:40 +0100 Subject: [PATCH 15/15] ACL LOG: make max log entries configurable. --- src/acl.c | 6 ++++++ src/config.c | 1 + src/server.h | 1 + tests/unit/acl.tcl | 11 +++++++++++ 4 files changed, 19 insertions(+) diff --git a/src/acl.c b/src/acl.c index 97c00d4f8..fa57e210c 100644 --- a/src/acl.c +++ b/src/acl.c @@ -1576,6 +1576,12 @@ void addACLLogEntry(client *c, int reason, int keypos, sds username) { /* Add it to our list of entires. We'll have to trim the list * to its maximum size. */ listAddNodeHead(ACLLog, le); + while(listLength(ACLLog) > server.acllog_max_len) { + listNode *ln = listLast(ACLLog); + ACLLogEntry *le = listNodeValue(ln); + ACLFreeLogEntry(le); + listDelNode(ACLLog,ln); + } } } diff --git a/src/config.c b/src/config.c index 0526de84d..68a9b0c0d 100644 --- a/src/config.c +++ b/src/config.c @@ -2233,6 +2233,7 @@ standardConfig configs[] = { /* Unsigned Long configs */ createULongConfig("active-defrag-max-scan-fields", NULL, MODIFIABLE_CONFIG, 1, LONG_MAX, server.active_defrag_max_scan_fields, 1000, INTEGER_CONFIG, NULL, NULL), /* Default: keys with more than 1000 fields will be processed separately */ createULongConfig("slowlog-max-len", NULL, MODIFIABLE_CONFIG, 0, LONG_MAX, server.slowlog_max_len, 128, INTEGER_CONFIG, NULL, NULL), + createULongConfig("acllog-max-len", NULL, MODIFIABLE_CONFIG, 0, LONG_MAX, server.acllog_max_len, 128, INTEGER_CONFIG, NULL, NULL), /* Long Long configs */ createLongLongConfig("lua-time-limit", NULL, MODIFIABLE_CONFIG, 0, LONG_MAX, server.lua_time_limit, 5000, INTEGER_CONFIG, NULL, NULL),/* milliseconds */ diff --git a/src/server.h b/src/server.h index e59fac22b..4b4fa1f34 100644 --- a/src/server.h +++ b/src/server.h @@ -1385,6 +1385,7 @@ struct redisServer { dict *latency_events; /* ACLs */ char *acl_filename; /* ACL Users file. NULL if not configured. */ + unsigned long acllog_max_len; /* Maximum length of the ACL LOG list. */ /* Assert & bug reporting */ const char *assert_failed; const char *assert_file; diff --git a/tests/unit/acl.tcl b/tests/unit/acl.tcl index 0e6d5c66a..fc1664a75 100644 --- a/tests/unit/acl.tcl +++ b/tests/unit/acl.tcl @@ -237,4 +237,15 @@ start_server {tags {"acl"}} { assert {[dict get $entry object] eq {AUTH}} assert {[dict get $entry username] eq {antirez}} } + + test {ACL LOG entries are limited to a maximum amount} { + r ACL LOG RESET + r CONFIG SET acllog-max-len 5 + r AUTH antirez foo + for {set j 0} {$j < 10} {incr j} { + catch {r SET obj:$j 123} + } + r AUTH default "" + assert {[llength [r ACL LOG]] == 5} + } }