From 3b74b55084b7e902c7e54603b3d6122b2a31d6fa Mon Sep 17 00:00:00 2001 From: Huang Zhw Date: Tue, 6 Apr 2021 04:13:20 +0800 Subject: [PATCH] Fix "default" and overwritten / reset users will not have pubsub channels permissions by default. (#8723) Background: Redis 6.2 added ACL control for pubsub channels (#7993), which were supposed to be permissive by default to retain compatibility with redis 6.0 ACL. But due to a bug, only newly created users got this `acl-pubsub-default` applied, while overwritten (updated) users got reset to `resetchannels` (denied). Since the "default" user exists before loading the config file, any ACL change to it, results in an update / overwrite. So when a "default" user is loaded from config file or include ACL file with no channels related rules, the user will not have any permissions to any channels. But other users will have default permissions to any channels. When upgraded from 6.0 with config rewrite, this will lead to "default" user channels permissions lost. When users are loaded from include file, then call "acl load", users will also lost channels permissions. Similarly, the `reset` ACL rule, would have reset the user to be denied access to any channels, ignoring `acl-pubsub-default` and breaking compatibility with redis 6.0. The implication of this fix is that it regains compatibility with redis 6.0, but breaks compatibility with redis 6.2.0 and 2.0.1. e.g. after the upgrade, the default user will regain access to pubsub channels. Other changes: Additionally this commit rename server.acl_pubusub_default to server.acl_pubsub_default and fix typo in acl tests. --- src/acl.c | 4 +++- src/config.c | 2 +- src/server.h | 2 +- tests/assets/user.acl | 3 ++- tests/unit/acl.tcl | 46 ++++++++++++++++++++++++++++++++++++++++--- 5 files changed, 50 insertions(+), 7 deletions(-) diff --git a/src/acl.c b/src/acl.c index 16f8606d3..6f8ab4055 100644 --- a/src/acl.c +++ b/src/acl.c @@ -245,7 +245,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 = USER_FLAG_DISABLED | server.acl_pubusub_default; + u->flags = USER_FLAG_DISABLED | server.acl_pubsub_default; u->allowed_subcommands = NULL; u->passwords = listCreate(); u->patterns = listCreate(); @@ -1000,6 +1000,8 @@ int ACLSetUser(user *u, const char *op, ssize_t oplen) { serverAssert(ACLSetUser(u,"resetpass",-1) == C_OK); serverAssert(ACLSetUser(u,"resetkeys",-1) == C_OK); serverAssert(ACLSetUser(u,"resetchannels",-1) == C_OK); + if (server.acl_pubsub_default & USER_FLAG_ALLCHANNELS) + serverAssert(ACLSetUser(u,"allchannels",-1) == C_OK); serverAssert(ACLSetUser(u,"off",-1) == C_OK); serverAssert(ACLSetUser(u,"sanitize-payload",-1) == C_OK); serverAssert(ACLSetUser(u,"-@all",-1) == C_OK); diff --git a/src/config.c b/src/config.c index 0ddb343b0..9861c5f52 100644 --- a/src/config.c +++ b/src/config.c @@ -2470,7 +2470,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_pubusub_default, USER_FLAG_ALLCHANNELS, NULL, NULL), + createEnumConfig("acl-pubsub-default", NULL, MODIFIABLE_CONFIG, acl_pubsub_default_enum, server.acl_pubsub_default, USER_FLAG_ALLCHANNELS, NULL, NULL), createEnumConfig("sanitize-dump-payload", NULL, MODIFIABLE_CONFIG, sanitize_dump_payload_enum, server.sanitize_dump_payload, SANITIZE_DUMP_NO, NULL, NULL), /* Integer configs */ diff --git a/src/server.h b/src/server.h index 98f8ea05f..2739d9824 100644 --- a/src/server.h +++ b/src/server.h @@ -1596,7 +1596,7 @@ struct redisServer { sds requirepass; /* Remember the cleartext password set with the old "requirepass" directive for backward compatibility with Redis <= 5. */ - int acl_pubusub_default; /* Default ACL pub/sub channels flag */ + int acl_pubsub_default; /* Default ACL pub/sub channels flag */ /* Assert & bug reporting */ int watchdog_period; /* Software watchdog period in ms. 0 = off */ /* System hardware info */ diff --git a/tests/assets/user.acl b/tests/assets/user.acl index 2f065dab6..67303512c 100644 --- a/tests/assets/user.acl +++ b/tests/assets/user.acl @@ -1,2 +1,3 @@ user alice on allcommands allkeys >alice -user bob on -@all +@set +acl ~set* >bob \ No newline at end of file +user bob on -@all +@set +acl ~set* >bob +user default on nopass ~* +@all diff --git a/tests/unit/acl.tcl b/tests/unit/acl.tcl index 175984274..c03eec27c 100644 --- a/tests/unit/acl.tcl +++ b/tests/unit/acl.tcl @@ -461,14 +461,44 @@ exec cp -f tests/assets/user.acl $server_path start_server [list overrides [list "dir" $server_path "aclfile" "user.acl"]] { # user alice on allcommands allkeys >alice # user bob on -@all +@set +acl ~set* >bob + # user default on nopass ~* +@all - test "Alice: can excute all command" { + test {default: load from include file, can access any channels} { + r SUBSCRIBE foo + r PSUBSCRIBE bar* + r UNSUBSCRIBE + r PUNSUBSCRIBE + r PUBLISH hello world + } + + test {default: with config acl-pubsub-default allchannels after reset, can access any channels} { + r ACL setuser default reset on nopass ~* +@all + r SUBSCRIBE foo + r PSUBSCRIBE bar* + r UNSUBSCRIBE + r PUNSUBSCRIBE + r PUBLISH hello world + } + + test {default: with config acl-pubsub-default resetchannels after reset, can not access any channels} { + r CONFIG SET acl-pubsub-default resetchannels + r ACL setuser default reset on nopass ~* +@all + catch {r SUBSCRIBE foo} e + assert_match {*NOPERM*} $e + catch {r PSUBSCRIBE bar*} e + assert_match {*NOPERM*} $e + catch {r PUBLISH hello world} e + assert_match {*NOPERM*} $e + r CONFIG SET acl-pubsub-default resetchannels + } + + test {Alice: can execute all command} { r AUTH alice alice assert_equal "alice" [r acl whoami] r SET key value } - test "Bob: just excute @set and acl command" { + test {Bob: just execute @set and acl command} { r AUTH bob bob assert_equal "bob" [r acl whoami] assert_equal "3" [r sadd set 1 2 3] @@ -476,7 +506,7 @@ start_server [list overrides [list "dir" $server_path "aclfile" "user.acl"]] { set e } {*NOPERM*} - test "ACL load and save" { + test {ACL load and save} { r ACL setuser eve +get allkeys >eve on r ACL save @@ -494,3 +524,13 @@ start_server [list overrides [list "dir" $server_path "aclfile" "user.acl"]] { set e } {*NOPERM*} } + +start_server {overrides {user "default on nopass ~* +@all"}} { + test {default: load from config file, can access any channels} { + r SUBSCRIBE foo + r PSUBSCRIBE bar* + r UNSUBSCRIBE + r PUNSUBSCRIBE + r PUBLISH hello world + } +}