From 0c1a4b557691df2196d41835aeef5c1f29914b82 Mon Sep 17 00:00:00 2001
From: antirez <antirez@gmail.com>
Date: Tue, 4 Feb 2020 12:55:26 +0100
Subject: [PATCH] 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 916b90cf1..2eac56b47 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 bd4aed192..637ceec1e 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 */