diff --git a/src/acl.c b/src/acl.c index 26b1cfd41a..a82cd330e4 100644 --- a/src/acl.c +++ b/src/acl.c @@ -1726,6 +1726,11 @@ static int ACLSelectorCheckKey(aclSelector *selector, const char *key, int keyle /* The selector can access any key */ if (selector->flags & SELECTOR_FLAG_ALLKEYS) return ACL_OK; + /* NOT_KEY entries are routing-only tokens (e.g. CLUSTERSCAN cursor, + * pub/sub channels); they are not real user keys and should bypass + * key-pattern ACL checks, consistent with how keyspecs skip them. */ + if (keyspec_flags & CMD_KEY_NOT_KEY) return ACL_OK; + listIter li; listNode *ln; listRewind(selector->patterns, &li); diff --git a/src/commands.def b/src/commands.def index 2686c74939..a37c32c7c8 100644 --- a/src/commands.def +++ b/src/commands.def @@ -11860,7 +11860,7 @@ struct COMMAND_STRUCT serverCommandTable[] = { /* cluster */ {MAKE_CMD("asking","Signals that a cluster client is following an -ASK redirect.","O(1)","3.0.0",CMD_DOC_NONE,NULL,NULL,"cluster",COMMAND_GROUP_CLUSTER,ASKING_History,0,ASKING_Tips,0,askingCommand,1,CMD_FAST,ACL_CATEGORY_CONNECTION|ACL_CATEGORY_FAST,NULL,ASKING_Keyspecs,0,NULL,0)}, {MAKE_CMD("cluster","A container for Cluster commands.","Depends on subcommand.","3.0.0",CMD_DOC_NONE,NULL,NULL,"cluster",COMMAND_GROUP_CLUSTER,CLUSTER_History,0,CLUSTER_Tips,0,NULL,-2,0,ACL_CATEGORY_SLOW,NULL,CLUSTER_Keyspecs,0,NULL,0),.subcommands=CLUSTER_Subcommands}, -{MAKE_CMD("clusterscan","Iterates over the keys in the cluster.","O(N) where N is the number of elements returned.","9.1.0",CMD_DOC_NONE,NULL,NULL,"cluster",COMMAND_GROUP_CLUSTER,CLUSTERSCAN_History,0,CLUSTERSCAN_Tips,3,clusterscanCommand,-2,CMD_READONLY|CMD_TOUCHES_ARBITRARY_KEYS,ACL_CATEGORY_KEYSPACE|ACL_CATEGORY_READ|ACL_CATEGORY_SLOW,NULL,CLUSTERSCAN_Keyspecs,1,NULL,5),.args=CLUSTERSCAN_Args}, +{MAKE_CMD("clusterscan","Iterates over the keys in the cluster.","O(N) where N is the number of elements returned.","9.1.0",CMD_DOC_NONE,NULL,NULL,"cluster",COMMAND_GROUP_CLUSTER,CLUSTERSCAN_History,0,CLUSTERSCAN_Tips,3,clusterscanCommand,-2,CMD_READONLY|CMD_TOUCHES_ARBITRARY_KEYS,ACL_CATEGORY_KEYSPACE|ACL_CATEGORY_READ|ACL_CATEGORY_SLOW,NULL,CLUSTERSCAN_Keyspecs,1,clusterscanGetKeys,5),.args=CLUSTERSCAN_Args}, {MAKE_CMD("readonly","Enables read-only queries for a connection to a Valkey replica node.","O(1)","3.0.0",CMD_DOC_NONE,NULL,NULL,"cluster",COMMAND_GROUP_CLUSTER,READONLY_History,0,READONLY_Tips,0,readonlyCommand,1,CMD_FAST|CMD_LOADING|CMD_STALE,ACL_CATEGORY_CONNECTION|ACL_CATEGORY_FAST,NULL,READONLY_Keyspecs,0,NULL,0)}, {MAKE_CMD("readwrite","Enables read-write queries for a connection to a Valkey replica node.","O(1)","3.0.0",CMD_DOC_NONE,NULL,NULL,"cluster",COMMAND_GROUP_CLUSTER,READWRITE_History,0,READWRITE_Tips,0,readwriteCommand,1,CMD_FAST|CMD_LOADING|CMD_STALE,ACL_CATEGORY_CONNECTION|ACL_CATEGORY_FAST,NULL,READWRITE_Keyspecs,0,NULL,0)}, /* connection */ diff --git a/src/commands/clusterscan.json b/src/commands/clusterscan.json index 721573cb6b..89773ffb69 100644 --- a/src/commands/clusterscan.json +++ b/src/commands/clusterscan.json @@ -6,6 +6,7 @@ "since": "9.1.0", "arity": -2, "function": "clusterscanCommand", + "get_keys_function": "clusterscanGetKeys", "command_flags": [ "READONLY", "TOUCHES_ARBITRARY_KEYS" diff --git a/src/db.c b/src/db.c index fed77e8a63..554f96133b 100644 --- a/src/db.c +++ b/src/db.c @@ -2503,9 +2503,14 @@ int getKeysFromCommandWithSpecs(struct serverCommand *cmd, /* This function returns a sanity check if the command may have keys. */ int doesCommandHaveKeys(struct serverCommand *cmd) { - return cmd->getkeys_proc || /* has getkeys_proc (non modules) */ - (cmd->flags & CMD_MODULE_GETKEYS) || /* module with GETKEYS */ - (getAllKeySpecsFlags(cmd, 1) & CMD_KEY_NOT_KEY); /* has at least one key-spec not marked as NOT_KEY */ + /* At least one key-spec not marked as NOT_KEY means the command has real keys. */ + if (getAllKeySpecsFlags(cmd, 1) & CMD_KEY_NOT_KEY) return 1; + /* If the command has getkeys_proc but all its key-specs are NOT_KEY + * (e.g. CLUSTERSCAN), the proc is only used for cluster slot routing, + * not for real key arguments. */ + if (cmd->getkeys_proc && cmd->key_specs_num > 0) return 0; + return cmd->getkeys_proc || /* has getkeys_proc (non modules) */ + (cmd->flags & CMD_MODULE_GETKEYS); /* module with GETKEYS */ } /* A simplified channel spec table that contains all of the commands @@ -3029,6 +3034,29 @@ int bitfieldGetKeys(struct serverCommand *cmd, robj **argv, int argc, getKeysRes return 1; } +/* CLUSTERSCAN getkeys function: when the cursor is "0", no slot routing is + * needed (any node can handle the initial scan call). For non-"0" cursors the + * format is "--" and the cursor itself + * contains a hashtag that routes to the correct slot, so we return it as a + * "key" for the cluster routing layer to compute the correct target node. */ +int clusterscanGetKeys(struct serverCommand *cmd, robj **argv, int argc, getKeysResult *result) { + UNUSED(cmd); + UNUSED(argc); + + /* Initial cursor: no routing needed, handle on any node. */ + if (strcmp(objectGetVal(argv[1]), "0") == 0) { + result->numkeys = 0; + return 0; + } + + /* Non-"0" cursor contains {hashtag} for routing. */ + keyReference *keys = getKeysPrepareResult(result, 1); + keys[0].pos = 1; + keys[0].flags = CMD_KEY_NOT_KEY; + result->numkeys = 1; + return 1; +} + int *selectDbIdArgs(robj **argv, int argc, int *count) { if (argc < 2) return NULL; diff --git a/src/server.h b/src/server.h index 0211264de4..6a23aaa57c 100644 --- a/src/server.h +++ b/src/server.h @@ -3826,6 +3826,7 @@ int zmpopGetKeys(struct serverCommand *cmd, robj **argv, int argc, getKeysResult int bzmpopGetKeys(struct serverCommand *cmd, robj **argv, int argc, getKeysResult *result); int setGetKeys(struct serverCommand *cmd, robj **argv, int argc, getKeysResult *result); int bitfieldGetKeys(struct serverCommand *cmd, robj **argv, int argc, getKeysResult *result); +int clusterscanGetKeys(struct serverCommand *cmd, robj **argv, int argc, getKeysResult *result); unsigned short crc16(const char *buf, int len); diff --git a/tests/unit/cluster/clusterscan.tcl b/tests/unit/cluster/clusterscan.tcl index 8369ff7ca9..f8e7cb1283 100644 --- a/tests/unit/cluster/clusterscan.tcl +++ b/tests/unit/cluster/clusterscan.tcl @@ -63,6 +63,35 @@ start_cluster 1 0 {tags {external:skip cluster}} { assert_equal $cursor "0" } + + test "CLUSTERSCAN cursor is NOT_KEY and does not break ACL key checks" { + # The CLUSTERSCAN cursor (e.g. "0" or "0-{06S}-0") is a routing token, + # not a real user key. A user with restricted key permissions (+clusterscan + # but only ~foo:*) must still be able to use CLUSTERSCAN without getting + # "NOPERM No permissions to access a key" on the cursor itself. + + # Create a user with limited key access but allowed to run clusterscan + R 0 ACL SETUSER scan_acl_leak on >pass resetkeys ~foo:* resetchannels -@all +clusterscan + set rd [valkey_deferring_client 0] + $rd AUTH scan_acl_leak pass + $rd read + + # Test 1: Initial cursor "0" should work (no key access needed) + $rd clusterscan 0 + # Should not error + $rd read + + # Test 2: Non-"0" cursor (contains {hashtag}) should also work. + # The cursor is a routing token marked NOT_KEY, not a real key, + # so ACL should not validate it against key patterns. + $rd clusterscan 0-{06S}-0 + $rd clusterscan 0-{6ZJ}-0 + # Should not error + $rd read + $rd read + + $rd close + } } # CLUSTERSCAN Tests - 3-node cluster tests @@ -758,3 +787,37 @@ start_cluster 1 1 {tags {external:skip cluster} overrides {hash-seed "fingerprin assert_equal $n [llength $all_keys] } } + +start_cluster 3 3 {tags {external:skip cluster}} { + test "CLUSTERSCAN 0 can be executed on any node without MOVED" { + # Cursor "0" is the initial scan start point and does not access any + # slot data, so it must be handled locally by any node regardless of + # slot ownership or role (primary/replica). + for {set id 0} {$id < [llength $::servers]} {incr id} { + # Plain clusterscan 0 + # slot 0 hashtag is {06S} + assert_equal [R $id clusterscan 0] {0-{06S}-0 {}} + + # clusterscan 0 with match + # "valkey" slot is 7659, hashtag is {39t} + assert_equal [R $id clusterscan 0 match valkey] {0-{39t}-0 {}} + + # clusterscan 0 with slot + # slot 16383 hashtag is {6ZJ} + assert_equal [R $id clusterscan 0 slot 16383] {0-{6ZJ}-0 {}} + + # clusterscan 0 with match and slot + # "valkey" slot is 7659, hashtag is {39t} + assert_equal [R $id clusterscan 0 match valkey slot 7659] {0-{39t}-0 {}} + + # clusterscan 0 with skipscan + # "valkey" slot is 7659, hashtag is {39t} + assert_equal [R $id clusterscan 0 match valkey slot 0] {0 {}} + assert_equal [R $id clusterscan 0 match valkey slot 5460] {0 {}} + assert_equal [R $id clusterscan 0 match valkey slot 5461] {0 {}} + assert_equal [R $id clusterscan 0 match valkey slot 10922] {0 {}} + assert_equal [R $id clusterscan 0 match valkey slot 10923] {0 {}} + assert_equal [R $id clusterscan 0 match valkey slot 16383] {0 {}} + } + } +} diff --git a/tests/unit/introspection-2.tcl b/tests/unit/introspection-2.tcl index ef5a70ce8e..58c06a381f 100644 --- a/tests/unit/introspection-2.tcl +++ b/tests/unit/introspection-2.tcl @@ -205,6 +205,22 @@ start_server {tags {"introspection"}} { assert_equal {{key1{t} {OW update}} {key2{t} {OW update}}} [r command getkeysandflags msetex 2 key1{t} val1 key2{t} val2 nx] } + test {COMMAND GETKEYS* NOT_KEY commands should report no key arguments} { + # Shard pub/sub - shard channel is NOT_KEY + assert_error {ERR The command has no key arguments} {r command getkeys ssubscribe mychannel} + assert_error {ERR The command has no key arguments} {r command getkeys sunsubscribe mychannel} + assert_error {ERR The command has no key arguments} {r command getkeys spublish mychannel mymessage} + assert_error {ERR The command has no key arguments} {r command getkeysandflags ssubscribe mychannel} + assert_error {ERR The command has no key arguments} {r command getkeysandflags sunsubscribe mychannel} + assert_error {ERR The command has no key arguments} {r command getkeysandflags spublish mychannel mymessage} + + # CLUSTERSCAN - cursor is a routing token, not a key + assert_error {ERR The command has no key arguments} {r command getkeys clusterscan 0} + assert_error {ERR The command has no key arguments} {r command getkeys clusterscan 0-{06S}-0} + assert_error {ERR The command has no key arguments} {r command getkeysandflags clusterscan 0} + assert_error {ERR The command has no key arguments} {r command getkeysandflags clusterscan 0-{06S}-0} + } + test "COMMAND LIST syntax error" { assert_error "ERR syntax error*" {r command list bad_arg} assert_error "ERR syntax error*" {r command list filterby bad_arg}