ACL channels permission handling for save/load scenario. (#8794)

In the initial release of Redis 6.2 setting a user to only allow pubsub access to
a specific channel, and doing ACL SAVE, resulted in an assertion when
ACL LOAD was used. This was later changed by #8723 (not yet released),
but still not properly resolved (now it errors instead of crash).

The problem is that the server that generates an ACL file, doesn't know what
would be the setting of the acl-pubsub-default config in the server that will load it.
so ACL SAVE needs to always start with resetchannels directive.

This should still be compatible with old acl files (from redis 6.0), and ones from earlier
versions of 6.2 that didn't mess with channels.

Co-authored-by: Harkrishn Patro <harkrisp@amazon.com>
Co-authored-by: Oran Agra <oran@redislabs.com>

4 files changed, 99 insertions, 1 deletions

diff --git a/redis.conf b/redis.conf
index e16fc4bbb..a7bfec83e 100644
--- a/redis.conf
+++ b/redis.conf
@@ -917,7 +917,7 @@ acllog-max-len 128
 # order to provide better out-of-the-box Pub/Sub security. Therefore, it is
 # recommended that you explicitly define Pub/Sub permissions for all users
 # rather then rely on implicit default values. Once you've set explicit
-# Pub/Sub for all exisitn users, you should uncomment the following line.
+# Pub/Sub for all existing users, you should uncomment the following line.
 #
 # acl-pubsub-default resetchannels
 
diff --git a/src/acl.c b/src/acl.c
index 6f8ab4055..6a2ade646 100644
--- a/src/acl.c
+++ b/src/acl.c
@@ -652,6 +652,7 @@ sds ACLDescribeUser(user *u) {
     if (u->flags & USER_FLAG_ALLCHANNELS) {
         res = sdscatlen(res,"&* ",3);
     } else {
+        res = sdscatlen(res,"resetchannels ",14);
         listRewind(u->channels,&li);
         while((ln = listNext(&li))) {
             sds thispat = listNodeValue(ln);
diff --git a/tests/assets/nodefaultuser.acl b/tests/assets/nodefaultuser.acl
new file mode 100644
index 000000000..2557c7fe7
--- /dev/null
+++ b/tests/assets/nodefaultuser.acl
@@ -0,0 +1,2 @@
+user alice on nopass ~* +@all
+user bob on nopass ~* &* +@all
\ No newline at end of file
diff --git a/tests/unit/acl.tcl b/tests/unit/acl.tcl
index c03eec27c..3424c044f 100644
--- a/tests/unit/acl.tcl
+++ b/tests/unit/acl.tcl
@@ -113,6 +113,30 @@ start_server {tags {"acl"}} {
         set e
     } {*NOPERM*channel*}
 
+    test {Validate subset of channels is prefixed with resetchannels flag} {
+        r ACL setuser hpuser on nopass resetchannels &foo +@all
+
+        # Verify resetchannels flag is prefixed before the channel name(s)
+        set users [r ACL LIST]
+        set curruser "hpuser"
+        foreach user [lshuffle $users] {
+            if {[string first $curruser $user] != -1} {
+                assert_equal {user hpuser on nopass resetchannels &foo +@all} $user
+            }
+        }
+
+        # authenticate as hpuser
+        r AUTH hpuser pass
+
+        assert_equal {0} [r PUBLISH foo bar]
+        catch {r PUBLISH bar game} e
+
+        # Falling back to psuser for the below tests
+        r AUTH psuser pspass
+        r ACL deluser hpuser
+        set e
+    } {*NOPERM*channel*}
+
     test {In transaction queue publish/subscribe/psubscribe to unauthorized channel will fail} {
         r ACL setuser psuser +multi +discard
         r MULTI
@@ -523,8 +547,79 @@ start_server [list overrides [list "dir" $server_path "aclfile" "user.acl"]] {
         catch {r SET key value} e
         set e
     } {*NOPERM*}
+
+    test {ACL load and save with restricted channels} {
+        r AUTH alice alice
+        r ACL setuser harry on nopass resetchannels &test +@all ~*
+        r ACL save
+
+        # ACL load will free user and kill clients
+        r ACL load
+        catch {r ACL LIST} e
+        assert_match {*I/O error*} $e
+
+        reconnect
+        r AUTH harry anything
+        r publish test bar
+        catch {r publish test1 bar} e
+        r ACL deluser harry
+        set e
+    } {*NOPERM*}
 }
 
+set server_path [tmpdir "resetchannels.acl"]
+exec cp -f tests/assets/nodefaultuser.acl $server_path
+exec cp -f tests/assets/default.conf $server_path
+start_server [list overrides [list "dir" $server_path "acl-pubsub-default" "resetchannels" "aclfile" "nodefaultuser.acl"]] {
+
+    test {Default user has access to all channels irrespective of flag} {
+        set channelinfo [dict get [r ACL getuser default] channels]
+        assert_equal "*" $channelinfo
+        set channelinfo [dict get [r ACL getuser alice] channels]
+        assert_equal "" $channelinfo
+    }
+
+    test {Update acl-pubsub-default, existing users shouldn't get affected} {
+        set channelinfo [dict get [r ACL getuser default] channels]
+        assert_equal "*" $channelinfo
+        r CONFIG set acl-pubsub-default allchannels
+        r ACL setuser mydefault
+        set channelinfo [dict get [r ACL getuser mydefault] channels]
+        assert_equal "*" $channelinfo
+        r CONFIG set acl-pubsub-default resetchannels
+        set channelinfo [dict get [r ACL getuser mydefault] channels]
+        assert_equal "*" $channelinfo
+    }
+
+    test {Single channel is valid} {
+        r ACL setuser onechannel &test
+        set channelinfo [dict get [r ACL getuser onechannel] channels]
+        assert_equal test $channelinfo
+        r ACL deluser onechannel
+    }
+
+    test {Single channel is not valid with allchannels} {
+        r CONFIG set acl-pubsub-default allchannels
+        catch {r ACL setuser onechannel &test} err
+        r CONFIG set acl-pubsub-default resetchannels
+        set err
+    } {*start with an empty list of channels*}
+}
+
+set server_path [tmpdir "resetchannels.acl"]
+exec cp -f tests/assets/nodefaultuser.acl $server_path
+exec cp -f tests/assets/default.conf $server_path
+start_server [list overrides [list "dir" $server_path "acl-pubsub-default" "resetchannels" "aclfile" "nodefaultuser.acl"]] {
+
+    test {Only default user has access to all channels irrespective of flag} {
+        set channelinfo [dict get [r ACL getuser default] channels]
+        assert_equal "*" $channelinfo
+        set channelinfo [dict get [r ACL getuser alice] channels]
+        assert_equal "" $channelinfo
+    }
+}
+
+
 start_server {overrides {user "default on nopass ~* +@all"}} {
     test {default: load from config file, can access any channels} {
         r SUBSCRIBE foo