From a43b6922d1e37d60acf63484b7057299c9bf584d Mon Sep 17 00:00:00 2001 From: Harkrishn Patro <30795839+hpatro@users.noreply.github.com> Date: Sun, 30 Jan 2022 11:02:55 +0100 Subject: [PATCH] Set default channel permission to resetchannels for 7.0 (#10181) For backwards compatibility in 6.x, channels default permission was set to `allchannels` however with 7.0, we should modify it and the default value should be `resetchannels` for better security posture. Also, with selectors in ACL, a client doesn't have to set channel rules everytime and by default the value will be `resetchannels`. Before this change ``` 127.0.0.1:6379> acl list 1) "user default on nopass ~* &* +@all" 127.0.0.1:6379> acl setuser hp on nopass +@all ~* OK 127.0.0.1:6379> acl list 1) "user default on nopass ~* &* +@all" 2) "user hp on nopass ~* &* +@all" 127.0.0.1:6379> acl setuser hp1 on nopass -@all (%R~sales*) OK 127.0.0.1:6379> acl list 1) "user default on nopass ~* &* +@all" 2) "user hp on nopass ~* &* +@all" 3) "user hp1 on nopass &* -@all (%R~sales* &* -@all)" ``` After this change ``` 127.0.0.1:6379> acl list 1) "user default on nopass ~* &* +@all" 127.0.0.1:6379> acl setuser hp on nopass +@all ~* OK 127.0.0.1:6379> acl list 1) "user default on nopass ~* &* +@all" 2) "user hp on nopass ~* resetchannels +@all" 127.0.0.1:6379> acl setuser hp1 on nopass -@all (%R~sales*) OK 127.0.0.1:6379> acl list 1) "user default on nopass ~* &* +@all" 2) "user hp on nopass ~* resetchannels +@all" 3) "user hp1 on nopass resetchannels -@all (%R~sales* resetchannels -@all)" ``` --- redis.conf | 10 +----- src/config.c | 2 +- tests/assets/user.acl | 6 ++-- tests/unit/acl-v2.tcl | 6 ++-- tests/unit/acl.tcl | 83 ++++++++++++++++++++++++++++++------------- 5 files changed, 67 insertions(+), 40 deletions(-) diff --git a/redis.conf b/redis.conf index b1b775358..ad37bbe33 100644 --- a/redis.conf +++ b/redis.conf @@ -1007,15 +1007,7 @@ acllog-max-len 128 # allchannels: grants access to all Pub/Sub channels # resetchannels: revokes access to all Pub/Sub channels # -# To ensure backward compatibility while upgrading Redis 6.0, acl-pubsub-default -# defaults to the 'allchannels' permission. -# -# Future compatibility note: it is very likely that in a future version of Redis -# the directive's default of 'allchannels' will be changed to 'resetchannels' in -# 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 existing users, you should uncomment the following line. +# From Redis 7.0, acl-pubsub-default defaults to 'resetchannels' permission. # # acl-pubsub-default resetchannels diff --git a/src/config.c b/src/config.c index 482a29617..a52e00f66 100644 --- a/src/config.c +++ b/src/config.c @@ -2811,7 +2811,7 @@ standardConfig configs[] = { createEnumConfig("maxmemory-policy", NULL, MODIFIABLE_CONFIG, maxmemory_policy_enum, server.maxmemory_policy, MAXMEMORY_NO_EVICTION, NULL, NULL), createEnumConfig("appendfsync", NULL, MODIFIABLE_CONFIG, aof_fsync_enum, server.aof_fsync, AOF_FSYNC_EVERYSEC, NULL, NULL), createEnumConfig("oom-score-adj", NULL, MODIFIABLE_CONFIG, oom_score_adj_enum, server.oom_score_adj, OOM_SCORE_ADJ_NO, NULL, updateOOMScoreAdj), - createEnumConfig("acl-pubsub-default", NULL, MODIFIABLE_CONFIG, acl_pubsub_default_enum, server.acl_pubsub_default, SELECTOR_FLAG_ALLCHANNELS, NULL, NULL), + createEnumConfig("acl-pubsub-default", NULL, MODIFIABLE_CONFIG, acl_pubsub_default_enum, server.acl_pubsub_default, 0, NULL, NULL), createEnumConfig("sanitize-dump-payload", NULL, DEBUG_CONFIG | MODIFIABLE_CONFIG, sanitize_dump_payload_enum, server.sanitize_dump_payload, SANITIZE_DUMP_NO, NULL, NULL), createEnumConfig("enable-protected-configs", NULL, IMMUTABLE_CONFIG, protected_action_enum, server.enable_protected_configs, PROTECTED_ACTION_ALLOWED_NO, NULL, NULL), createEnumConfig("enable-debug-command", NULL, IMMUTABLE_CONFIG, protected_action_enum, server.enable_debug_cmd, PROTECTED_ACTION_ALLOWED_NO, NULL, NULL), diff --git a/tests/assets/user.acl b/tests/assets/user.acl index 67303512c..926ac54f6 100644 --- a/tests/assets/user.acl +++ b/tests/assets/user.acl @@ -1,3 +1,3 @@ -user alice on allcommands allkeys >alice -user bob on -@all +@set +acl ~set* >bob -user default on nopass ~* +@all +user alice on allcommands allkeys &* >alice +user bob on -@all +@set +acl ~set* &* >bob +user default on nopass ~* &* +@all diff --git a/tests/unit/acl-v2.tcl b/tests/unit/acl-v2.tcl index 8f4ac83cc..72ea44c3a 100644 --- a/tests/unit/acl-v2.tcl +++ b/tests/unit/acl-v2.tcl @@ -20,12 +20,12 @@ start_server {tags {"acl external:skip"}} { assert_match "*NOPERM*keys*" $err } - test {Test ACL selectors by default have no permissions (except channels)} { + test {Test ACL selectors by default have no permissions} { r ACL SETUSER selector-default reset () set user [r ACL GETUSER "selector-default"] assert_equal 1 [llength [dict get $user selectors]] assert_equal "" [dict get [lindex [dict get $user selectors] 0] keys] - assert_equal "&*" [dict get [lindex [dict get $user selectors] 0] channels] + assert_equal "" [dict get [lindex [dict get $user selectors] 0] channels] assert_equal "-@all" [dict get [lindex [dict get $user selectors] 0] commands] } @@ -44,7 +44,7 @@ start_server {tags {"acl external:skip"}} { catch {r ACL SETUSER selector-syntax on (this-is-invalid)} e assert_match "*ERR Error in ACL SETUSER modifier '(*)*Syntax*" $e - catch {r ACL SETUSER selector-syntax on (&fail)} e + catch {r ACL SETUSER selector-syntax on (&* &fail)} e assert_match "*ERR Error in ACL SETUSER modifier '(*)*Adding a pattern after the*" $e assert_equal "" [r ACL GETUSER selector-syntax] diff --git a/tests/unit/acl.tcl b/tests/unit/acl.tcl index 24f069313..494c3847e 100644 --- a/tests/unit/acl.tcl +++ b/tests/unit/acl.tcl @@ -81,42 +81,70 @@ start_server {tags {"acl external:skip"}} { set e } {*NOPERM*key*} - test {By default users are able to publish to any channel} { + test {By default, only default user is able to publish to any channel} { + r AUTH default pwd + r PUBLISH foo bar r ACL setuser psuser on >pspass +acl +client +@pubsub r AUTH psuser pspass - r PUBLISH foo bar - } {0} + catch {r PUBLISH foo bar} e + set e + } {*NOPERM*channels*} - test {By default users are able to publish to any shard channel} { + test {By default, only default user is not able to publish to any shard channel} { + r AUTH default pwd r SPUBLISH foo bar - } {0} + r AUTH psuser pspass + catch {r SPUBLISH foo bar} e + set e + } {*NOPERM*channels*} - test {By default users are able to subscribe to any channel} { + test {By default, only default user is able to subscribe to any channel} { set rd [redis_deferring_client] - $rd AUTH psuser pspass + $rd AUTH default pwd $rd read $rd SUBSCRIBE foo assert_match {subscribe foo 1} [$rd read] - $rd close - } {0} - - test {By default users are able to subscribe to any shard channel} { - set rd [redis_deferring_client] + $rd UNSUBSCRIBE + $rd read $rd AUTH psuser pspass $rd read + $rd SUBSCRIBE foo + catch {$rd read} e + $rd close + set e + } {*NOPERM*channels*} + + test {By default, only default user is able to subscribe to any shard channel} { + set rd [redis_deferring_client] + $rd AUTH default pwd + $rd read $rd SSUBSCRIBE foo assert_match {ssubscribe foo 1} [$rd read] - $rd close - } {0} - - test {By default users are able to subscribe to any pattern} { - set rd [redis_deferring_client] + $rd SUNSUBSCRIBE + $rd read $rd AUTH psuser pspass $rd read + $rd SSUBSCRIBE foo + catch {$rd read} e + $rd close + set e + } {*NOPERM*channels*} + + test {By default, only default user is able to subscribe to any pattern} { + set rd [redis_deferring_client] + $rd AUTH default pwd + $rd read $rd PSUBSCRIBE bar* assert_match {psubscribe bar\* 1} [$rd read] + $rd PUNSUBSCRIBE + $rd read + $rd AUTH psuser pspass + $rd read + $rd PSUBSCRIBE bar* + catch {$rd read} e $rd close - } {0} + set e + } {*NOPERM*channels*} test {It's possible to allow publishing to a subset of channels} { r ACL setuser psuser resetchannels &foo:1 &bar:* @@ -675,10 +703,10 @@ start_server {tags {"acl external:skip"}} { set server_path [tmpdir "server.acl"] exec cp -f tests/assets/user.acl $server_path -start_server [list overrides [list "dir" $server_path "aclfile" "user.acl"] tags [list "external:skip"]] { - # user alice on allcommands allkeys >alice - # user bob on -@all +@set +acl ~set* >bob - # user default on nopass ~* +@all +start_server [list overrides [list "dir" $server_path "acl-pubsub-default" "allchannels" "aclfile" "user.acl"] tags [list "external:skip"]] { + # user alice on allcommands allkeys &* >alice + # user bob on -@all +@set +acl ~set* &* >bob + # user default on nopass ~* &* +@all test {default: load from include file, can access any channels} { r SUBSCRIBE foo @@ -760,7 +788,7 @@ start_server [list overrides [list "dir" $server_path "aclfile" "user.acl"] tags 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"] tags [list "external:skip"]] { +start_server [list overrides [list "dir" $server_path "aclfile" "nodefaultuser.acl"] tags [list "external:skip"]] { test {Default user has access to all channels irrespective of flag} { set channelinfo [dict get [r ACL getuser default] channels] @@ -811,7 +839,14 @@ start_server [list overrides [list "dir" $server_path "acl-pubsub-default" "rese start_server {overrides {user "default on nopass ~* +@all"} tags {"external:skip"}} { - test {default: load from config file, can access any channels} { + test {default: load from config file, without channel permission default user can't access any channels} { + catch {r SUBSCRIBE foo} e + set e + } {*NOPERM*channel*} +} + +start_server {overrides {user "default on nopass ~* &* +@all"} tags {"external:skip"}} { + test {default: load from config file with all channels permissions} { r SUBSCRIBE foo r PSUBSCRIBE bar* r UNSUBSCRIBE