Unverified Commit 3193f086 authored by Shaya Potter's avatar Shaya Potter Committed by GitHub
Browse files

Unify ACL failure error messaging. (#11160)

Motivation: for applications that use RM ACL verification functions, they would
want to return errors back to the user, in ways that are consistent with Redis.
While investigating how we should return ACL errors to the user, we realized that
Redis isn't consistent, and currently returns ACL error strings in 3 primary ways.

[For the actual implications of this change, see the "Impact" section at the bottom]

1. how it returns an error when calling a command normally
   ACL_DENIED_CMD -> "this user has no permissions to run the '%s' command"
   ACL_DENIED_KEY -> "this user has no permissions to access one of the keys used as arguments"
   ACL_DENIED_CHANNEL -> "this user has no permissions to access one of the channels used as arguments"

2. how it returns an error when calling via 'acl dryrun' command
   ACL_DENIED_CMD ->  "This user has no permissions to run the '%s' command"
   ACL_DENIED_KEY -> "This user has no permissions to access the '%s' key"
   ACL_DENIED_CHANNEL -> "This user has no permissions to access the '%s' channel"

3. how it returns an error via RM_Call (and scripting is similar).
   ACL_DENIED_CMD -> "can't run this command or subcommand";
   ACL_DENIED_KEY -> "can't access at least one of the keys mentioned in the command arguments";
   ACL_DENIED_CHANNEL -> "can't publish to the channel mentioned in the command";
   
   In addition, if one wants to use RM_Call's "dry run" capability instead of the RM ACL
   functions directly, one also sees a different problem than it returns ACL errors with a -ERR,
   not a -PERM, so it can't be returned directly to the caller.

This PR modifies the code to generate a base message in a common manner with the ability
to set verbose flag for acl dry run errors, and keep it unset for normal/rm_call/script cases

```c
sds getAclErrorMessage(int acl_res, user *user, struct redisCommand *cmd, sds errored_val, int verbose) {
    switch (acl_res) {
    case ACL_DENIED_CMD:
        return sdscatfmt(sdsempty(), "User %S has no permissions to run "
                                     "the '%S' command", user->name, cmd->fullname);
    case ACL_DENIED_KEY:
        if (verbose) {
            return sdscatfmt(sdsempty(), "User %S has no permissions to access "
                                         "the '%S' key", user->name, errored_val);
        } else {
            return sdsnew("No permissions to access a key");
        }
    case ACL_DENIED_CHANNEL:
        if (verbose) {
            return sdscatfmt(sdsempty(), "User %S has no permissions to access "
                                         "the '%S' channel", user->name, errored_val);
        } else {
            return sdsnew("No permissions to access a channel");
        }
    }
```

The caller can append/prepend the message (adding NOPERM for normal/RM_Call or indicating it's within a script).

Impact:
- Plain commands, as well as scripts and RM_Call now include the user name.
- ACL DRYRUN remains the only one that's verbose (mentions the offending channel or key name)
- Changes RM_Call ACL errors from being a `-ERR` to being `-NOPERM` (besides for textual changes)
  **This somewhat a breaking change, but it only affects the RM_Call with both `C` and `E`, or `D`**
- Changes ACL errors in scripts textually from being
  `The user executing the script <old non unified text>`
  to
  `ACL failure in script: <new unified text>`
parent 56f97bfa
......@@ -1766,7 +1766,11 @@ int ACLUserCheckChannelPerm(user *u, sds channel, int is_pattern) {
return ACL_DENIED_CHANNEL;
}
/* Lower level API that checks if a specified user is able to execute a given command. */
/* Lower level API that checks if a specified user is able to execute a given command.
*
* If the command fails an ACL check, idxptr will be to set to the first argv entry that
* causes the failure, either 0 if the command itself fails or the idx of the key/channel
* that causes the failure */
int ACLCheckAllUserCommandPerm(user *u, struct redisCommand *cmd, robj **argv, int argc, int *idxptr) {
listIter li;
listNode *ln;
......@@ -2581,18 +2585,25 @@ void addACLLogEntry(client *c, int reason, int context, int argpos, sds username
}
}
const char* getAclErrorMessage(int acl_res) {
/* Notice that a variant of this code also exists on aclCommand so
* it also need to be updated on changed. */
sds getAclErrorMessage(int acl_res, user *user, struct redisCommand *cmd, sds errored_val, int verbose) {
switch (acl_res) {
case ACL_DENIED_CMD:
return "can't run this command or subcommand";
return sdscatfmt(sdsempty(), "User %S has no permissions to run "
"the '%S' command", user->name, cmd->fullname);
case ACL_DENIED_KEY:
return "can't access at least one of the keys mentioned in the command arguments";
if (verbose) {
return sdscatfmt(sdsempty(), "User %S has no permissions to access "
"the '%S' key", user->name, errored_val);
} else {
return sdsnew("No permissions to access a key");
}
case ACL_DENIED_CHANNEL:
return "can't publish to the channel mentioned in the command";
default:
return "lacking the permissions for the command";
if (verbose) {
return sdscatfmt(sdsempty(), "User %S has no permissions to access "
"the '%S' channel", user->name, errored_val);
} else {
return sdsnew("No permissions to access a channel");
}
}
serverPanic("Reached deadcode on getAclErrorMessage");
}
......@@ -2956,22 +2967,8 @@ void aclCommand(client *c) {
int idx;
int result = ACLCheckAllUserCommandPerm(u, cmd, c->argv + 3, c->argc - 3, &idx);
/* Notice that a variant of this code also exists on getAclErrorMessage so
* it also need to be updated on changed. */
if (result != ACL_OK) {
sds err = sdsempty();
if (result == ACL_DENIED_CMD) {
err = sdscatfmt(err, "This user has no permissions to run "
"the '%s' command", cmd->fullname);
} else if (result == ACL_DENIED_KEY) {
err = sdscatfmt(err, "This user has no permissions to access "
"the '%s' key", c->argv[idx + 3]->ptr);
} else if (result == ACL_DENIED_CHANNEL) {
err = sdscatfmt(err, "This user has no permissions to access "
"the '%s' channel", c->argv[idx + 3]->ptr);
} else {
serverPanic("Invalid permission result");
}
sds err = getAclErrorMessage(result, u, cmd, c->argv[idx+3]->ptr, 1);
addReplyBulkSds(c, err);
return;
}
......
......@@ -6003,7 +6003,10 @@ RedisModuleCallReply *RM_Call(RedisModuleCtx *ctx, const char *cmdname, const ch
sds object = (acl_retval == ACL_DENIED_CMD) ? sdsdup(c->cmd->fullname) : sdsdup(c->argv[acl_errpos]->ptr);
addACLLogEntry(ctx->client, acl_retval, ACL_LOG_CTX_MODULE, -1, ctx->client->user->name, object);
if (error_as_call_replies) {
sds msg = sdscatfmt(sdsempty(), "acl verification failed, %s.", getAclErrorMessage(acl_retval));
/* verbosity should be same as processCommand() in server.c */
sds acl_msg = getAclErrorMessage(acl_retval, ctx->client->user, c->cmd, c->argv[acl_errpos]->ptr, 0);
sds msg = sdscatfmt(sdsempty(), "-NOPERM %S\r\n", acl_msg);
sdsfree(acl_msg);
reply = callReplyCreateError(msg, ctx);
}
errno = EACCES;
......
......@@ -335,7 +335,9 @@ static int scriptVerifyACL(client *c, sds *err) {
int acl_retval = ACLCheckAllPerm(c, &acl_errpos);
if (acl_retval != ACL_OK) {
addACLLogEntry(c,acl_retval,ACL_LOG_CTX_LUA,acl_errpos,NULL,NULL);
*err = sdscatfmt(sdsempty(), "The user executing the script %s", getAclErrorMessage(acl_retval));
sds msg = getAclErrorMessage(acl_retval, c->user, c->cmd, c->argv[acl_errpos]->ptr, 0);
*err = sdscatsds(sdsnew("ACL failure in script: "), msg);
sdsfree(msg);
return C_ERR;
}
return C_OK;
......
......@@ -3752,28 +3752,9 @@ int processCommand(client *c) {
int acl_retval = ACLCheckAllPerm(c,&acl_errpos);
if (acl_retval != ACL_OK) {
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:
{
rejectCommandFormat(c,
"-NOPERM this user has no permissions to run "
"the '%s' command", c->cmd->fullname);
break;
}
case ACL_DENIED_KEY:
rejectCommandFormat(c,
"-NOPERM this user has no permissions to access "
"one of the keys used as arguments");
break;
case ACL_DENIED_CHANNEL:
rejectCommandFormat(c,
"-NOPERM this user has no permissions to access "
"one of the channels used as arguments");
break;
default:
rejectCommandFormat(c, "no permission");
break;
}
sds msg = getAclErrorMessage(acl_retval, c->user, c->cmd, c->argv[acl_errpos]->ptr, 0);
rejectCommandFormat(c, "-NOPERM %s", msg);
sdsfree(msg);
return C_OK;
}
......
......@@ -2805,7 +2805,7 @@ void addReplyCommandCategories(client *c, struct redisCommand *cmd);
user *ACLCreateUnlinkedUser();
void ACLFreeUserAndKillClients(user *u);
void addACLLogEntry(client *c, int reason, int context, int argpos, sds username, sds object);
const char* getAclErrorMessage(int acl_res);
sds getAclErrorMessage(int acl_res, user *user, struct redisCommand *cmd, sds errored_val, int verbose);
void ACLUpdateDefaultUserPassword(sds password);
sds genRedisInfoStringACLStats(sds info);
......
This diff is collapsed.
......@@ -93,7 +93,7 @@ start_server {tags {"acl external:skip"}} {
r AUTH psuser pspass
catch {r PUBLISH foo bar} e
set e
} {*NOPERM*channels*}
} {*NOPERM*channel*}
test {By default, only default user is not able to publish to any shard channel} {
r AUTH default pwd
......@@ -101,7 +101,7 @@ start_server {tags {"acl external:skip"}} {
r AUTH psuser pspass
catch {r SPUBLISH foo bar} e
set e
} {*NOPERM*channels*}
} {*NOPERM*channel*}
test {By default, only default user is able to subscribe to any channel} {
set rd [redis_deferring_client]
......@@ -117,7 +117,7 @@ start_server {tags {"acl external:skip"}} {
catch {$rd read} e
$rd close
set e
} {*NOPERM*channels*}
} {*NOPERM*channel*}
test {By default, only default user is able to subscribe to any shard channel} {
set rd [redis_deferring_client]
......@@ -133,7 +133,7 @@ start_server {tags {"acl external:skip"}} {
catch {$rd read} e
$rd close
set e
} {*NOPERM*channels*}
} {*NOPERM*channel*}
test {By default, only default user is able to subscribe to any pattern} {
set rd [redis_deferring_client]
......@@ -149,7 +149,7 @@ start_server {tags {"acl external:skip"}} {
catch {$rd read} e
$rd close
set e
} {*NOPERM*channels*}
} {*NOPERM*channel*}
test {It's possible to allow publishing to a subset of channels} {
r ACL setuser psuser resetchannels &foo:1 &bar:*
......@@ -554,9 +554,9 @@ start_server {tags {"acl external:skip"}} {
test "ACL requires explicit permission for scripting for EVAL_RO, EVALSHA_RO and FCALL_RO" {
r ACL SETUSER scripter on nopass +readonly
assert_equal "This user has no permissions to run the 'eval_ro' command" [r ACL DRYRUN scripter EVAL_RO "" 0]
assert_equal "This user has no permissions to run the 'evalsha_ro' command" [r ACL DRYRUN scripter EVALSHA_RO "" 0]
assert_equal "This user has no permissions to run the 'fcall_ro' command" [r ACL DRYRUN scripter FCALL_RO "" 0]
assert_match {*has no permissions to run the 'eval_ro' command*} [r ACL DRYRUN scripter EVAL_RO "" 0]
assert_match {*has no permissions to run the 'evalsha_ro' command*} [r ACL DRYRUN scripter EVALSHA_RO "" 0]
assert_match {*has no permissions to run the 'fcall_ro' command*} [r ACL DRYRUN scripter FCALL_RO "" 0]
}
test {ACL #5998 regression: memory leaks adding / removing subcommands} {
......@@ -793,7 +793,7 @@ start_server {tags {"acl external:skip"}} {
set current_invalid_channel_accesses [s acl_access_denied_channel]
r ACL setuser invalidkeyuser on >passwd resetkeys allcommands
r AUTH invalidkeyuser passwd
assert_error "*no permissions to access one of the keys*" {r get x}
assert_error "*NOPERM*key*" {r get x}
r AUTH default ""
assert {[s acl_access_denied_auth] eq $current_auth_failures}
assert {[s acl_access_denied_cmd] eq $current_invalid_cmd_accesses}
......@@ -809,7 +809,7 @@ start_server {tags {"acl external:skip"}} {
set current_invalid_channel_accesses [s acl_access_denied_channel]
r ACL setuser invalidchanneluser on >passwd resetchannels allcommands
r AUTH invalidkeyuser passwd
assert_error "*no permissions to access one of the channels*" {r subscribe x}
assert_error "*NOPERM*channel*" {r subscribe x}
r AUTH default ""
assert {[s acl_access_denied_auth] eq $current_auth_failures}
assert {[s acl_access_denied_cmd] eq $current_invalid_cmd_accesses}
......
......@@ -63,13 +63,14 @@ start_server {tags {"modules acl"}} {
# rm call check for key permission (y: only WRITE)
assert_equal [r aclcheck.rm_call set y 5] OK
assert_error {*NOPERM*} {r aclcheck.rm_call set y 5 get}
assert_error {ERR acl verification failed, can't access at least one of the keys mentioned in the command arguments.} {r aclcheck.rm_call_with_errors set y 5 get}
assert_error {*NOPERM*No permissions to access a key*} {r aclcheck.rm_call_with_errors set y 5 get}
# rm call check for key permission (z: only READ)
assert_error {*NOPERM*} {r aclcheck.rm_call set z 5}
assert_error {ERR acl verification failed, can't access at least one of the keys mentioned in the command arguments.} {r aclcheck.rm_call_with_errors set z 5}
catch {r aclcheck.rm_call_with_errors set z 5} e
assert_match {*NOPERM*No permissions to access a key*} $e
assert_error {*NOPERM*} {r aclcheck.rm_call set z 6 get}
assert_error {ERR acl verification failed, can't access at least one of the keys mentioned in the command arguments.} {r aclcheck.rm_call_with_errors set z 6 get}
assert_error {*NOPERM*No permissions to access a key*} {r aclcheck.rm_call_with_errors set z 6 get}
# verify that new log entry added
set entry [lindex [r ACL LOG] 0]
......@@ -80,10 +81,8 @@ start_server {tags {"modules acl"}} {
# rm call check for command permission
r acl setuser default -set
catch {r aclcheck.rm_call set x 5} e
assert_match {*NOPERM*} $e
catch {r aclcheck.rm_call_with_errors set x 5} e
assert_match {ERR acl verification failed, can't run this command or subcommand.} $e
assert_error {*NOPERM*} {r aclcheck.rm_call set x 5}
assert_error {*NOPERM*has no permissions to run the 'set' command*} {r aclcheck.rm_call_with_errors set x 5}
# verify that new log entry added
set entry [lindex [r ACL LOG] 0]
......
......@@ -64,14 +64,14 @@ start_server {tags {"modules"}} {
test "module getkeys-api - ACL" {
# legacy triple didn't provide flags, so they require both read and write
assert_equal "OK" [r ACL DRYRUN testuser getkeys.command key rw]
assert_equal "This user has no permissions to access the 'read' key" [r ACL DRYRUN testuser getkeys.command key read]
assert_equal "This user has no permissions to access the 'write' key" [r ACL DRYRUN testuser getkeys.command key write]
assert_match {*has no permissions to access the 'read' key*} [r ACL DRYRUN testuser getkeys.command key read]
assert_match {*has no permissions to access the 'write' key*} [r ACL DRYRUN testuser getkeys.command key write]
}
test "module getkeys-api with flags - ACL" {
assert_equal "OK" [r ACL DRYRUN testuser getkeys.command_with_flags key rw]
assert_equal "OK" [r ACL DRYRUN testuser getkeys.command_with_flags key read]
assert_equal "This user has no permissions to access the 'write' key" [r ACL DRYRUN testuser getkeys.command_with_flags key write]
assert_match {*has no permissions to access the 'write' key*} [r ACL DRYRUN testuser getkeys.command_with_flags key write]
}
test "Unload the module - getkeys" {
......
......@@ -129,15 +129,15 @@ start_server {tags {"modules"}} {
test "Module key specs: No spec, only legacy triple - ACL" {
# legacy triple didn't provide flags, so they require both read and write
assert_equal "OK" [r ACL DRYRUN testuser kspec.none rw val1]
assert_equal "This user has no permissions to access the 'read' key" [r ACL DRYRUN testuser kspec.none read val1]
assert_equal "This user has no permissions to access the 'write' key" [r ACL DRYRUN testuser kspec.none write val1]
assert_match {*has no permissions to access the 'read' key*} [r ACL DRYRUN testuser kspec.none read val1]
assert_match {*has no permissions to access the 'write' key*} [r ACL DRYRUN testuser kspec.none write val1]
}
test "Module key specs: tworanges - ACL" {
assert_equal "OK" [r ACL DRYRUN testuser kspec.tworanges read write]
assert_equal "OK" [r ACL DRYRUN testuser kspec.tworanges rw rw]
assert_equal "This user has no permissions to access the 'read' key" [r ACL DRYRUN testuser kspec.tworanges rw read]
assert_equal "This user has no permissions to access the 'write' key" [r ACL DRYRUN testuser kspec.tworanges write rw]
assert_match {*has no permissions to access the 'read' key*} [r ACL DRYRUN testuser kspec.tworanges rw read]
assert_match {*has no permissions to access the 'write' key*} [r ACL DRYRUN testuser kspec.tworanges write rw]
}
foreach cmd {kspec.none kspec.tworanges} {
......
......@@ -444,7 +444,7 @@ start_server {tags {"modules"}} {
r acl setuser default resetkeys
catch {r test.rm_call_flags DC set x 10} e
assert_match {*ERR acl verification failed, can't access at least one of the keys*} $e
assert_match {*NOPERM No permissions to access a key*} $e
r acl setuser default +@all ~*
assert_equal [r get x] $x
}
......@@ -452,4 +452,4 @@ start_server {tags {"modules"}} {
test "Unload the module - misc" {
assert_equal {OK} [r module unload misc]
}
}
\ No newline at end of file
}
......@@ -45,8 +45,7 @@ start_server {tags {"modules usercall"}} {
assert_equal [r usercall.get_acl] "off sanitize-payload ~* &* +@all -set"
# fails here as testing acl in rm call
catch {r usercall.call_with_user_flag C set x 10} e
assert_match {*ERR acl verification failed*} $e
assert_error {*NOPERM User default has no permissions*} {r usercall.call_with_user_flag C set x 10}
assert_equal [r usercall.call_with_user_flag C get x] 5
......@@ -88,7 +87,7 @@ start_server {tags {"modules usercall"}} {
# fails here in script, as rm_call will permit the eval call
catch {r usercall.call_with_user_flag C evalsha $sha_set 0} e
assert_match {*ERR The user executing the script can't run this command or subcommand script*} $e
assert_match {*ERR ACL failure in script*} $e
assert_equal [r usercall.call_with_user_flag C evalsha $sha_get 0] 1
}
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment