From 20c33fe6a8fce2edb3e4158bc10bde2c3740a25b Mon Sep 17 00:00:00 2001 From: Binbin Date: Thu, 13 Jan 2022 02:05:14 +0800 Subject: [PATCH] Show subcommand full name in error log / ACL LOG (#10105) Use `getFullCommandName` to get the full name of the command. It can also get the full name of the subcommand, like "script|help". Before: ``` > SCRIPT HELP (error) NOPERM this user has no permissions to run the 'help' command or its subcommand > ACL LOG 7) "object" 8) "help" ``` After: ``` > SCRIPT HELP (error) NOPERM this user has no permissions to run the 'script|help' command > ACL LOG 7) "object" 8) "script|help" ``` Fix #10094 --- src/acl.c | 2 +- src/server.c | 6 +++- tests/unit/acl.tcl | 76 ++++++++++++++++++++++++---------------------- 3 files changed, 46 insertions(+), 38 deletions(-) diff --git a/src/acl.c b/src/acl.c index 19b357c9e..aff48afd4 100644 --- a/src/acl.c +++ b/src/acl.c @@ -1904,7 +1904,7 @@ void addACLLogEntry(client *c, int reason, int context, int argpos, sds username le->object = object; } else { switch(reason) { - case ACL_DENIED_CMD: le->object = sdsnew(c->cmd->name); break; + case ACL_DENIED_CMD: le->object = getFullCommandName(c->cmd); break; case ACL_DENIED_KEY: le->object = sdsdup(c->argv[argpos]->ptr); break; case ACL_DENIED_CHANNEL: le->object = sdsdup(c->argv[argpos]->ptr); break; case ACL_DENIED_AUTH: le->object = sdsdup(c->argv[0]->ptr); break; diff --git a/src/server.c b/src/server.c index 07b75b92e..36143caa5 100644 --- a/src/server.c +++ b/src/server.c @@ -3439,10 +3439,14 @@ int processCommand(client *c) { addACLLogEntry(c,acl_retval,(c->flags & CLIENT_MULTI) ? ACL_LOG_CTX_MULTI : ACL_LOG_CTX_TOPLEVEL,acl_errpos,NULL,NULL); switch (acl_retval) { case ACL_DENIED_CMD: + { + sds cmdname = getFullCommandName(c->cmd); rejectCommandFormat(c, "-NOPERM this user has no permissions to run " - "the '%s' command or its subcommand", c->cmd->name); + "the '%s' command", cmdname); + sdsfree(cmdname); break; + } case ACL_DENIED_KEY: rejectCommandFormat(c, "-NOPERM this user has no permissions to access " diff --git a/tests/unit/acl.tcl b/tests/unit/acl.tcl index 660e841fb..87f6fb46a 100644 --- a/tests/unit/acl.tcl +++ b/tests/unit/acl.tcl @@ -64,7 +64,7 @@ start_server {tags {"acl external:skip"}} { test {By default users are not able to access any command} { catch {r SET foo bar} e set e - } {*NOPERM*} + } {*NOPERM*set*} test {By default users are not able to access any key} { r ACL setuser newuser +set @@ -161,17 +161,14 @@ start_server {tags {"acl external:skip"}} { test {In transaction queue publish/subscribe/psubscribe to unauthorized channel will fail} { r ACL setuser psuser +multi +discard r MULTI - catch {r PUBLISH notexits helloworld} e + assert_error {*NOPERM*channel*} {r PUBLISH notexits helloworld} r DISCARD - assert_match {*NOPERM*} $e r MULTI - catch {r SUBSCRIBE notexits foo:1} e + assert_error {*NOPERM*channel*} {r SUBSCRIBE notexits foo:1} r DISCARD - assert_match {*NOPERM*} $e r MULTI - catch {r PSUBSCRIBE notexits:* bar:*} e + assert_error {*NOPERM*channel*} {r PSUBSCRIBE notexits:* bar:*} r DISCARD - assert_match {*NOPERM*} $e } test {It's possible to allow subscribing to a subset of channels} { @@ -285,7 +282,7 @@ start_server {tags {"acl external:skip"}} { r INCR mycounter ; # Should not raise an error catch {r PING} e set e - } {*NOPERM*} + } {*NOPERM*ping*} test {ACLs can include or exclude whole classes of commands} { r ACL setuser newuser -@all +@set +acl @@ -296,7 +293,7 @@ start_server {tags {"acl external:skip"}} { catch {r SET foo bar} e r ACL setuser newuser allcommands; # Undo commands ACL set e - } {*NOPERM*} + } {*NOPERM*set*} test {ACLs can include single subcommands} { r ACL setuser newuser +@all -client @@ -308,7 +305,7 @@ start_server {tags {"acl external:skip"}} { r CLIENT SETNAME foo ; # Should not fail catch {r CLIENT KILL type master} e set e - } {*NOPERM*} + } {*NOPERM*client|kill*} test {ACLs can exclude single subcommands, case 1} { r ACL setuser newuser +@all -client|kill @@ -318,7 +315,7 @@ start_server {tags {"acl external:skip"}} { r CLIENT SETNAME foo ; # Should not fail catch {r CLIENT KILL type master} e set e - } {*NOPERM*} + } {*NOPERM*client|kill*} test {ACLs can exclude single subcommands, case 2} { r ACL setuser newuser -@all +acl +config -config|set @@ -328,7 +325,7 @@ start_server {tags {"acl external:skip"}} { r CONFIG GET loglevel; # Should not fail catch {r CONFIG SET loglevel debug} e set e - } {*NOPERM*} + } {*NOPERM*config|set*} test {ACLs can include a subcommand with a specific arg} { r ACL setuser newuser +@all -config|get @@ -339,7 +336,7 @@ start_server {tags {"acl external:skip"}} { r CONFIG GET appendonly; # Should not fail catch {r CONFIG GET loglevel} e set e - } {*NOPERM*} + } {*NOPERM*config|get*} test {ACLs including of a type includes also subcommands} { r ACL setuser newuser -@all +acl +@stream @@ -354,7 +351,7 @@ start_server {tags {"acl external:skip"}} { r SELECT 0 catch {r SELECT 1} e set e - } {*NOPERM*} + } {*NOPERM*select*} test {ACLs can block all DEBUG subcommands except one} { r ACL setuser newuser -@all +acl +incr +debug|object @@ -364,7 +361,7 @@ start_server {tags {"acl external:skip"}} { r DEBUG OBJECT key catch {r DEBUG SEGFAULT} e set e - } {*NOPERM*} + } {*NOPERM*debug*} test {ACLs set can include subcommands, if already full command exists} { r ACL setuser bob +memory|doctor @@ -396,8 +393,7 @@ start_server {tags {"acl external:skip"}} { r ACL setuser alice >passwd1 on r AUTH alice passwd1 - catch {r MEMORY DOCTOR} e - assert_match {*NOPERM*} $e + assert_error {*NOPERM*memory|doctor*} {r MEMORY DOCTOR} r MEMORY STATS ;# should work # Validate the commands have got engulfed to -memory. @@ -405,10 +401,8 @@ start_server {tags {"acl external:skip"}} { set cmdstr [dict get [r ACL getuser alice] commands] assert_equal {+@all -memory} $cmdstr - catch {r MEMORY DOCTOR} e - assert_match {*NOPERM*} $e - catch {r MEMORY STATS} e - assert_match {*NOPERM*} $e + assert_error {*NOPERM*memory|doctor*} {r MEMORY DOCTOR} + assert_error {*NOPERM*memory|stats*} {r MEMORY STATS} # Appending to the existing access string of alice. r ACL setuser alice -@all @@ -422,10 +416,8 @@ start_server {tags {"acl external:skip"}} { r AUTH alice passwd1 - catch {r GET key} e - assert_match {*NOPERM*} $e - catch {r MEMORY STATS} e - assert_match {*NOPERM*} $e + assert_error {*NOPERM*get*} {r GET key} + assert_error {*NOPERM*memory|stats*} {r MEMORY STATS} # Auth newuser before the next test r AUTH newuser passwd1 @@ -486,7 +478,7 @@ start_server {tags {"acl external:skip"}} { r ACL setuser antirez +eval +multi +exec r ACL setuser antirez resetchannels +publish r AUTH antirez foo - catch {r GET foo} + assert_error "*NOPERM*get*" {r GET foo} r AUTH default "" set entry [lindex [r ACL LOG] 0] assert {[dict get $entry username] eq {antirez}} @@ -495,14 +487,29 @@ start_server {tags {"acl external:skip"}} { assert {[dict get $entry object] eq {get}} } + test "ACL LOG shows failed subcommand executions at toplevel" { + r ACL LOG RESET + r ACL DELUSER demo + r ACL SETUSER demo on nopass + r AUTH demo "" + assert_error "*NOPERM*script|help*" {r SCRIPT HELP} + r AUTH default "" + set entry [lindex [r ACL LOG] 0] + assert_equal [dict get $entry username] {demo} + assert_equal [dict get $entry context] {toplevel} + assert_equal [dict get $entry reason] {command} + assert_equal [dict get $entry object] {script|help} + } + test {ACL LOG is able to test similar events} { + r ACL LOG RESET 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} + assert {[dict get $entry count] == 3} } test {ACL LOG is able to log keys access violations and key name} { @@ -662,12 +669,9 @@ start_server [list overrides [list "dir" $server_path "aclfile" "user.acl"] tags 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 + assert_error {*NOPERM*channel*} {r SUBSCRIBE foo} + assert_error {*NOPERM*channel*} {r PSUBSCRIBE bar*} + assert_error {*NOPERM*channel*} {r PUBLISH hello world} r CONFIG SET acl-pubsub-default resetchannels } @@ -683,7 +687,7 @@ start_server [list overrides [list "dir" $server_path "aclfile" "user.acl"] tags assert_equal "3" [r sadd set 1 2 3] catch {r SET key value} e set e - } {*NOPERM*} + } {*NOPERM*set*} test {ACL load and save} { r ACL setuser eve +get allkeys >eve on @@ -701,7 +705,7 @@ start_server [list overrides [list "dir" $server_path "aclfile" "user.acl"] tags r GET key catch {r SET key value} e set e - } {*NOPERM*} + } {*NOPERM*set*} test {ACL load and save with restricted channels} { r AUTH alice alice @@ -719,7 +723,7 @@ start_server [list overrides [list "dir" $server_path "aclfile" "user.acl"] tags catch {r publish test1 bar} e r ACL deluser harry set e - } {*NOPERM*} + } {*NOPERM*channel*} } set server_path [tmpdir "resetchannels.acl"]