gvsafronov/futriix
1
0
Fork 0
Code Issues Pull Requests Actions 4 Packages Projects Releases Wiki Activity

Allow SET without GET arg on write-only ACL. Allow BITFIELD GET on read-only ACL (#10148)

Browse Source 
SET is a R+W command, because it can also do `GET` on the data.
SET without GET is a write-only command.
SET with GET is a read+write command.

In #9974, we added ACL to let users define write-only access.
So when the user uses SET with GET option, and the user doesn't
have the READ permission on the key, we need to reject it,
but we rather not reject users with write-only permissions from using
the SET command when they don't use GET.

In this commit, we add a `getkeys_proc` function to control key
flags in SET command. We also add a new key spec flag (VARIABLE_FLAGS)
means that some keys might have different flags depending on arguments.

We also handle BITFIELD command, add a `bitfieldGetKeys` function.
BITFIELD GET is a READ ONLY command.
BITFIELD SET or BITFIELD INCR are READ WRITE commands.

Other changes:
1. SET GET was added in 6.2, add the missing since in set.json
2. Added tests to cover the changes in acl-v2.tcl
3. Fix some typos in server.h and cleanups in acl-v2.tcl

Co-authored-by: Madelyn Olson <madelyneolson@gmail.com>
Co-authored-by: Oran Agra <oran@redislabs.com>
This commit is contained in:
Binbin 2022-01-27 03:03:21 +08:00 committed by GitHub
parent 795ea011ba
commit d616925835
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 173 additions and 22 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
src/commands.c
View File

@ -6675,7 +6675,7 @@ struct redisCommandArg SET_Args[] = {
{"value",ARG_TYPE_STRING,-1,NULL,NULL,NULL,CMD_ARG_NONE},
{"expiration",ARG_TYPE_ONEOF,-1,NULL,NULL,NULL,CMD_ARG_OPTIONAL,.subargs=SET_expiration_Subargs},
{"condition",ARG_TYPE_ONEOF,-1,NULL,NULL,"2.6.12",CMD_ARG_OPTIONAL,.subargs=SET_condition_Subargs},
{"get",ARG_TYPE_PURE_TOKEN,-1,"GET",NULL,NULL,CMD_ARG_OPTIONAL},
{"get",ARG_TYPE_PURE_TOKEN,-1,"GET",NULL,"6.2.0",CMD_ARG_OPTIONAL},
{0}
};
@ -6806,7 +6806,7 @@ struct redisCommandArg WATCH_Args[] = {
struct redisCommand redisCommandTable[] = {
/* bitmap */
{"bitcount","Count set bits in a string","O(N)","2.6.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_BITMAP,BITCOUNT_History,BITCOUNT_tips,bitcountCommand,-2,CMD_READONLY,ACL_CATEGORY_BITMAP,{{CMD_KEY_RO|CMD_KEY_ACCESS,KSPEC_BS_INDEX,.bs.index={1},KSPEC_FK_RANGE,.fk.range={0,1,0}}},.args=BITCOUNT_Args},
{"bitfield","Perform arbitrary bitfield integer operations on strings","O(1) for each subcommand specified","3.2.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_BITMAP,BITFIELD_History,BITFIELD_tips,bitfieldCommand,-2,CMD_WRITE|CMD_DENYOOM,ACL_CATEGORY_BITMAP,{{CMD_KEY_RW|CMD_KEY_UPDATE|CMD_KEY_ACCESS,KSPEC_BS_INDEX,.bs.index={1},KSPEC_FK_RANGE,.fk.range={0,1,0}}},.args=BITFIELD_Args},
{"bitfield","Perform arbitrary bitfield integer operations on strings","O(1) for each subcommand specified","3.2.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_BITMAP,BITFIELD_History,BITFIELD_tips,bitfieldCommand,-2,CMD_WRITE|CMD_DENYOOM,ACL_CATEGORY_BITMAP,{{CMD_KEY_RW|CMD_KEY_UPDATE|CMD_KEY_ACCESS|CMD_KEY_VARIABLE_FLAGS,KSPEC_BS_INDEX,.bs.index={1},KSPEC_FK_RANGE,.fk.range={0,1,0}}},bitfieldGetKeys,.args=BITFIELD_Args},
{"bitfield_ro","Perform arbitrary bitfield integer operations on strings. Read-only variant of BITFIELD","O(1) for each subcommand specified","6.2.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_BITMAP,BITFIELD_RO_History,BITFIELD_RO_tips,bitfieldroCommand,-2,CMD_READONLY|CMD_FAST,ACL_CATEGORY_BITMAP,{{CMD_KEY_RO|CMD_KEY_ACCESS,KSPEC_BS_INDEX,.bs.index={1},KSPEC_FK_RANGE,.fk.range={0,1,0}}},.args=BITFIELD_RO_Args},
{"bitop","Perform bitwise operations between strings","O(N)","2.6.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_BITMAP,BITOP_History,BITOP_tips,bitopCommand,-4,CMD_WRITE|CMD_DENYOOM,ACL_CATEGORY_BITMAP,{{CMD_KEY_OW|CMD_KEY_UPDATE,KSPEC_BS_INDEX,.bs.index={2},KSPEC_FK_RANGE,.fk.range={0,1,0}},{CMD_KEY_RO|CMD_KEY_ACCESS,KSPEC_BS_INDEX,.bs.index={3},KSPEC_FK_RANGE,.fk.range={-1,1,0}}},.args=BITOP_Args},
{"bitpos","Find first bit set or clear in a string","O(N)","2.8.7",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_BITMAP,BITPOS_History,BITPOS_tips,bitposCommand,-3,CMD_READONLY,ACL_CATEGORY_BITMAP,{{CMD_KEY_RO|CMD_KEY_ACCESS,KSPEC_BS_INDEX,.bs.index={1},KSPEC_FK_RANGE,.fk.range={0,1,0}}},.args=BITPOS_Args},
@ -7050,7 +7050,7 @@ struct redisCommand redisCommandTable[] = {
{"mset","Set multiple keys to multiple values","O(N) where N is the number of keys to set.","1.0.1",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_STRING,MSET_History,MSET_tips,msetCommand,-3,CMD_WRITE|CMD_DENYOOM,ACL_CATEGORY_STRING,{{CMD_KEY_OW|CMD_KEY_UPDATE,KSPEC_BS_INDEX,.bs.index={1},KSPEC_FK_RANGE,.fk.range={-1,2,0}}},.args=MSET_Args},
{"msetnx","Set multiple keys to multiple values, only if none of the keys exist","O(N) where N is the number of keys to set.","1.0.1",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_STRING,MSETNX_History,MSETNX_tips,msetnxCommand,-3,CMD_WRITE|CMD_DENYOOM,ACL_CATEGORY_STRING,{{CMD_KEY_OW|CMD_KEY_INSERT,KSPEC_BS_INDEX,.bs.index={1},KSPEC_FK_RANGE,.fk.range={-1,2,0}}},.args=MSETNX_Args},
{"psetex","Set the value and expiration in milliseconds of a key","O(1)","2.6.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_STRING,PSETEX_History,PSETEX_tips,psetexCommand,4,CMD_WRITE|CMD_DENYOOM,ACL_CATEGORY_STRING,{{CMD_KEY_OW|CMD_KEY_UPDATE,KSPEC_BS_INDEX,.bs.index={1},KSPEC_FK_RANGE,.fk.range={0,1,0}}},.args=PSETEX_Args},
{"set","Set the string value of a key","O(1)","1.0.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_STRING,SET_History,SET_tips,setCommand,-3,CMD_WRITE|CMD_DENYOOM,ACL_CATEGORY_STRING,{{CMD_KEY_RW|CMD_KEY_ACCESS|CMD_KEY_UPDATE,KSPEC_BS_INDEX,.bs.index={1},KSPEC_FK_RANGE,.fk.range={0,1,0}}},.args=SET_Args},
{"set","Set the string value of a key","O(1)","1.0.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_STRING,SET_History,SET_tips,setCommand,-3,CMD_WRITE|CMD_DENYOOM,ACL_CATEGORY_STRING,{{CMD_KEY_RW|CMD_KEY_ACCESS|CMD_KEY_UPDATE|CMD_KEY_VARIABLE_FLAGS,KSPEC_BS_INDEX,.bs.index={1},KSPEC_FK_RANGE,.fk.range={0,1,0}}},setGetKeys,.args=SET_Args},
{"setex","Set the value and expiration of a key","O(1)","2.0.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_STRING,SETEX_History,SETEX_tips,setexCommand,4,CMD_WRITE|CMD_DENYOOM,ACL_CATEGORY_STRING,{{CMD_KEY_OW|CMD_KEY_UPDATE,KSPEC_BS_INDEX,.bs.index={1},KSPEC_FK_RANGE,.fk.range={0,1,0}}},.args=SETEX_Args},
{"setnx","Set the value of a key, only if the key does not exist","O(1)","1.0.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_STRING,SETNX_History,SETNX_tips,setnxCommand,3,CMD_WRITE|CMD_DENYOOM|CMD_FAST,ACL_CATEGORY_STRING,{{CMD_KEY_OW|CMD_KEY_INSERT,KSPEC_BS_INDEX,.bs.index={1},KSPEC_FK_RANGE,.fk.range={0,1,0}}},.args=SETNX_Args},
{"setrange","Overwrite part of a string at key starting at the specified offset","O(1), not counting the time taken to copy the new string in place. Usually, this string is very small so the amortized complexity is O(1). Otherwise, complexity is O(M) with M being the length of the value argument.","2.2.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_STRING,SETRANGE_History,SETRANGE_tips,setrangeCommand,4,CMD_WRITE|CMD_DENYOOM,ACL_CATEGORY_STRING,{{CMD_KEY_RW|CMD_KEY_UPDATE,KSPEC_BS_INDEX,.bs.index={1},KSPEC_FK_RANGE,.fk.range={0,1,0}}},.args=SETRANGE_Args},

4
src/commands/bitfield.json
View File

@ -6,6 +6,7 @@
"since": "3.2.0",
"arity": -2,
"function": "bitfieldCommand",
"get_keys_function": "bitfieldGetKeys",
"command_flags": [
"WRITE",
"DENYOOM"
@ -18,7 +19,8 @@
"flags": [
"RW",
"UPDATE",
"ACCESS"
"ACCESS",
"VARIABLE_FLAGS"
],
"begin_search": {
"index": {

7
src/commands/set.json
View File

@ -6,6 +6,7 @@
"since": "1.0.0",
"arity": -3,
"function": "setCommand",
"get_keys_function": "setGetKeys",
"history": [
[
"2.6.12",
@ -36,7 +37,8 @@
"flags": [
"RW",
"ACCESS",
"UPDATE"
"UPDATE",
"VARIABLE_FLAGS"
],
"begin_search": {
"index": {
@ -121,7 +123,8 @@
"name": "get",
"token": "GET",
"type": "pure-token",
"optional": true
"optional": true,
"since": "6.2.0"
}
]
}

50
src/db.c
View File

@ -1824,7 +1824,7 @@ int getKeysFromCommandWithSpecs(struct redisCommand *cmd, robj **argv, int argc,
if (cmd->flags & CMD_MODULE_GETKEYS) {
return moduleGetCommandKeysViaAPI(cmd,argv,argc,result);
} else {
if (!(getAllKeySpecsFlags(cmd, 0) & CMD_KEY_INCOMPLETE)) {
if (!(getAllKeySpecsFlags(cmd, 0) & (CMD_KEY_INCOMPLETE|CMD_KEY_VARIABLE_FLAGS))) {
int ret = getKeysUsingKeySpecs(cmd,argv,argc,search_flags,result);
if (ret >= 0)
return ret;
@ -2210,3 +2210,51 @@ int xreadGetKeys(struct redisCommand *cmd, robj **argv, int argc, getKeysResult
result->numkeys = num;
return num;
}
/* Helper function to extract keys from the SET command, which may have
* a read flag if the GET argument is passed in. */
int setGetKeys(struct redisCommand *cmd, robj **argv, int argc, getKeysResult *result) {
keyReference *keys;
UNUSED(cmd);
keys = getKeysPrepareResult(result, 1);
keys[0].pos = 1; /* We always know the position */
result->numkeys = 1;
for (int i = 3; i < argc; i++) {
char *arg = argv[i]->ptr;
if ((arg[0] == 'g' || arg[0] == 'G') &&
(arg[1] == 'e' || arg[1] == 'E') &&
(arg[2] == 't' || arg[2] == 'T') && arg[3] == '\0')
{
keys[0].flags = CMD_KEY_RW | CMD_KEY_ACCESS | CMD_KEY_UPDATE;
return 1;
}
}
keys[0].flags = CMD_KEY_OW | CMD_KEY_UPDATE;
return 1;
}
/* Helper function to extract keys from the BITFIELD command, which may be
* read-only if the BITFIELD GET subcommand is used. */
int bitfieldGetKeys(struct redisCommand *cmd, robj **argv, int argc, getKeysResult *result) {
keyReference *keys;
UNUSED(cmd);
keys = getKeysPrepareResult(result, 1);
keys[0].pos = 1; /* We always know the position */
result->numkeys = 1;
for (int i = 2; i < argc; i++) {
int remargs = argc - i - 1; /* Remaining args other than current. */
char *arg = argv[i]->ptr;
if (!strcasecmp(arg, "get") && remargs >= 2) {
keys[0].flags = CMD_KEY_RO | CMD_KEY_ACCESS;
return 1;
}
}
keys[0].flags = CMD_KEY_RW | CMD_KEY_ACCESS | CMD_KEY_UPDATE;
return 1;
}

10
src/server.h
View File

@ -264,6 +264,8 @@ extern int configOOMScoreAdjValuesDefaults[CONFIG_OOM_COUNT];
#define CMD_KEY_CHANNEL (1ULL<<8) /* PUBSUB shard channel */
#define CMD_KEY_INCOMPLETE (1ULL<<9) /* Means that the keyspec might not point
* out to all keys it should cover */
#define CMD_KEY_VARIABLE_FLAGS (1ULL<<10) /* Means that some keys might have
* different flags depending on arguments */
/* AOF states */
#define AOF_OFF 0 /* AOF is off */
@ -1892,7 +1894,7 @@ struct redisServer {
typedef struct {
int pos; /* The position of the key within the client array */
int flags; /* The flags associted with the key access, see
int flags; /* The flags associated with the key access, see
CMD_KEY_* for more information */
} keyReference;
@ -1914,7 +1916,7 @@ typedef struct {
* which is limited and doesn't fit many commands.
*
* There are two steps:
* 1. begin_search (BS): in which index should we start seacrhing for keys?
* 1. begin_search (BS): in which index should we start searching for keys?
* 2. find_keys (FK): relative to the output of BS, how can we will which args are keys?
*
* There are two types of BS:
@ -1956,7 +1958,7 @@ typedef struct {
const char *keyword;
/* An index in argv from which to start searching.
* Can be negative, which means start search from the end, in reverse
* (Example: -2 means to start in reverse from the panultimate arg) */
* (Example: -2 means to start in reverse from the penultimate arg) */
int startfrom;
} keyword;
} bs;
@ -3019,6 +3021,8 @@ int lmpopGetKeys(struct redisCommand *cmd, robj **argv, int argc, getKeysResult
int blmpopGetKeys(struct redisCommand *cmd, robj **argv, int argc, getKeysResult *result);
int zmpopGetKeys(struct redisCommand *cmd, robj **argv, int argc, getKeysResult *result);
int bzmpopGetKeys(struct redisCommand *cmd, robj **argv, int argc, getKeysResult *result);
int setGetKeys(struct redisCommand *cmd, robj **argv, int argc, getKeysResult *result);
int bitfieldGetKeys(struct redisCommand *cmd, robj **argv, int argc, getKeysResult *result);
unsigned short crc16(const char *buf, int len);

100
tests/unit/acl-v2.tcl
View File

@ -104,7 +104,7 @@ start_server {tags {"acl external:skip"}} {
r set read bar
$r2 copy read write
catch {$r2 copy write read} err
assert_match "*NOPERM*keys*" $err
assert_match "*NOPERM*keys*" $err
}
test {Test separate read and write permissions on different selectors are not additive} {
@ -131,11 +131,99 @@ start_server {tags {"acl external:skip"}} {
catch {$r2 copy read write} err
assert_match "*NOPERM*keys*" $err
catch {$r2 copy write read} err
assert_match "*NOPERM*keys*" $err
assert_match "*NOPERM*keys*" $err
}
test {Test SET with separate read permission} {
r del readstr
r ACL SETUSER set-key-permission-R on nopass %R~read* +@all
$r2 auth set-key-permission-R password
assert_equal PONG [$r2 PING]
assert_equal {} [$r2 get readstr]
# We don't have the permission to WRITE key.
assert_error {*NOPERM*keys*} {$r2 set readstr bar}
assert_error {*NOPERM*keys*} {$r2 set readstr bar get}
assert_error {*NOPERM*keys*} {$r2 set readstr bar ex 100}
assert_error {*NOPERM*keys*} {$r2 set readstr bar keepttl nx}
}
test {Test SET with separate write permission} {
r del writestr
r ACL SETUSER set-key-permission-W on nopass %W~write* +@all
$r2 auth set-key-permission-W password
assert_equal PONG [$r2 PING]
assert_equal {OK} [$r2 set writestr bar]
assert_equal {OK} [$r2 set writestr get]
# We don't have the permission to READ key.
assert_error {*NOPERM*keys*} {$r2 set get writestr}
assert_error {*NOPERM*keys*} {$r2 set writestr bar get}
assert_error {*NOPERM*keys*} {$r2 set writestr bar get ex 100}
assert_error {*NOPERM*keys*} {$r2 set writestr bar get keepttl nx}
# this probably should be `ERR value is not an integer or out of range`
assert_error {*NOPERM*keys*} {$r2 set writestr bar ex get}
}
test {Test SET with read and write permissions} {
r del readwrite_str
r ACL SETUSER set-key-permission-RW-selector on nopass %RW~readwrite* +@all
$r2 auth set-key-permission-RW-selector password
assert_equal PONG [$r2 PING]
assert_equal {} [$r2 get readwrite_str]
assert_error {ERR* not an integer *} {$r2 set readwrite_str bar ex get}
assert_equal {OK} [$r2 set readwrite_str bar]
assert_equal {bar} [$r2 get readwrite_str]
assert_equal {bar} [$r2 set readwrite_str bar2 get]
assert_equal {bar2} [$r2 get readwrite_str]
assert_equal {bar2} [$r2 set readwrite_str bar3 get ex 10]
assert_equal {bar3} [$r2 get readwrite_str]
assert_range [$r2 ttl readwrite_str] 5 10
}
test {Test BITFIELD with separate read permission} {
r del readstr
r ACL SETUSER bitfield-key-permission-R on nopass %R~read* +@all
$r2 auth bitfield-key-permission-R password
assert_equal PONG [$r2 PING]
assert_equal {0} [$r2 bitfield readstr get u4 0]
# We don't have the permission to WRITE key.
assert_error {*NOPERM*keys*} {$r2 bitfield readstr set u4 0 1}
assert_error {*NOPERM*keys*} {$r2 bitfield readstr incrby u4 0 1}
}
test {Test BITFIELD with separate write permission} {
r del writestr
r ACL SETUSER bitfield-key-permission-W on nopass %W~write* +@all
$r2 auth bitfield-key-permission-W password
assert_equal PONG [$r2 PING]
# We don't have the permission to READ key.
assert_error {*NOPERM*keys*} {$r2 bitfield writestr get u4 0}
assert_error {*NOPERM*keys*} {$r2 bitfield writestr set u4 0 1}
assert_error {*NOPERM*keys*} {$r2 bitfield writestr incrby u4 0 1}
}
test {Test BITFIELD with read and write permissions} {
r del readwrite_str
r ACL SETUSER bitfield-key-permission-RW-selector on nopass %RW~readwrite* +@all
$r2 auth bitfield-key-permission-RW-selector password
assert_equal PONG [$r2 PING]
assert_equal {0} [$r2 bitfield readwrite_str get u4 0]
assert_equal {0} [$r2 bitfield readwrite_str set u4 0 1]
assert_equal {2} [$r2 bitfield readwrite_str incrby u4 0 1]
assert_equal {2} [$r2 bitfield readwrite_str get u4 0]
}
test {Test ACL log correctly identifies the relevant item when selectors are used} {
r ACL SETUSER acl-log-test-selector on nopass
r ACL SETUSER acl-log-test-selector on nopass
r ACL SETUSER acl-log-test-selector +mget ~key (+mget ~key ~otherkey)
$r2 auth acl-log-test-selector password
@ -171,10 +259,10 @@ start_server {tags {"acl external:skip"}} {
}
test {Test ACL GETUSER response information} {
r ACL setuser selector-info -@all +get resetchannels &channel1 %R~foo1 %W~bar1 ~baz1
r ACL setuser selector-info -@all +get resetchannels &channel1 %R~foo1 %W~bar1 ~baz1
r ACL setuser selector-info (-@all +set resetchannels &channel2 %R~foo2 %W~bar2 ~baz2)
set user [r ACL GETUSER "selector-info"]
# Root selector
assert_equal "%R~foo1 %W~bar1 ~baz1" [dict get $user keys]
assert_equal "&channel1" [dict get $user channels]
@ -184,7 +272,7 @@ start_server {tags {"acl external:skip"}} {
set secondary_selector [lindex [dict get $user selectors] 0]
assert_equal "%R~foo2 %W~bar2 ~baz2" [dict get $secondary_selector keys]
assert_equal "&channel2" [dict get $secondary_selector channels]
assert_equal "-@all +set" [dict get $secondary_selector commands]
assert_equal "-@all +set" [dict get $secondary_selector commands]
}
test {Test ACL list idempotency} {

18
tests/unit/moduleapi/aclcheck.tcl
View File

@ -19,7 +19,7 @@ start_server {tags {"modules acl"}} {
}
test {test module check acl for key perm} {
# give permission for SET and block all keys but x
# give permission for SET and block all keys but x(READ+WRITE), y(WRITE), z(READ)
r acl setuser default +set resetkeys ~x %W~y %R~z
assert_equal [r aclcheck.set.check.key "*" x 5] OK
@ -49,17 +49,23 @@ start_server {tags {"modules acl"}} {
} {*DENIED CHANNEL*}
test {test module check acl in rm_call} {
# rm call check for key permission (x can be accessed)
# rm call check for key permission (x: READ + WRITE)
assert_equal [r aclcheck.rm_call set x 5] OK
# rm call check for key permission (y can't be accessed)
catch {r aclcheck.rm_call set y 5} e
assert_match {*NOPERM*} $e
assert_equal [r aclcheck.rm_call set x 6 get] 5
# 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}
# rm call check for key permission (z: only READ)
assert_error {*NOPERM*} {r aclcheck.rm_call set z 5}
assert_error {*NOPERM*} {r aclcheck.rm_call set z 6 get}
# verify that new log entry added
set entry [lindex [r ACL LOG] 0]
assert {[dict get $entry username] eq {default}}
assert {[dict get $entry context] eq {module}}
assert {[dict get $entry object] eq {y}}
assert {[dict get $entry object] eq {z}}
# rm call check for command permission
r acl setuser default -set