diff --git a/.github/workflows/linux.yml b/.github/workflows/linux.yml index b385339..67b4e2d 100644 --- a/.github/workflows/linux.yml +++ b/.github/workflows/linux.yml @@ -16,7 +16,7 @@ jobs: strategy: matrix: - redis_version: ["7.2", "7.4"] + redis_version: ["7.2", "7.4", "unstable"] steps: - uses: actions/checkout@v4 diff --git a/.github/workflows/macos.yml b/.github/workflows/macos.yml index 04532a5..6da2646 100644 --- a/.github/workflows/macos.yml +++ b/.github/workflows/macos.yml @@ -18,7 +18,7 @@ jobs: strategy: matrix: - redis_version: ["7.2", "7.4"] + redis_version: ["7.2", "7.4", "unstable"] steps: - uses: actions/checkout@v4 diff --git a/rust_api/libmr/mod.rs b/rust_api/libmr/mod.rs index 7f6fb77..672562f 100644 --- a/rust_api/libmr/mod.rs +++ b/rust_api/libmr/mod.rs @@ -37,17 +37,12 @@ impl Default for crate::libmr_c_raw::bindings::Record { pub type RustMRError = String; -pub fn mr_init(ctx: &Context, num_threads: usize, username: Option<&str>, password: Option<&str>) { - let username = username.map(|v| CString::new(v).unwrap()); +pub fn mr_init(ctx: &Context, num_threads: usize, password: Option<&str>) { let password = password.map(|v| CString::new(v).unwrap()); unsafe { MR_Init( ctx.ctx as *mut RedisModuleCtx, num_threads, - username - .as_ref() - .map(|v| v.as_ptr()) - .unwrap_or(ptr::null_mut()) as *mut c_char, password .as_ref() .map(|v| v.as_ptr()) diff --git a/src/cluster.c b/src/cluster.c index 86f194a..46ab73f 100644 --- a/src/cluster.c +++ b/src/cluster.c @@ -36,33 +36,6 @@ #define NETWORK_TEST_COMMAND xstr(MODULE_NAME)".NETWORKTEST" #define FORCE_SHARDS_CONNECTION xstr(MODULE_NAME)".FORCESHARDSCONNECTION" -/** - * @brief Sets the ACL categories for the given command. - * @return true if the ACL categories were set successfully for the - * command was registered successfully, false otherwise. - */ -#define SetCommandAcls(ctx, cmd, acls) \ - ({ \ - bool result = false; \ - if (!RedisModule_GetCommand || !RedisModule_SetCommandACLCategories \ - || !RedisModule_AddACLCategory) { \ - result = true; \ - } \ - if (!result) { \ - struct RedisModuleCommand *command = RedisModule_GetCommand(ctx, cmd); \ - if (command != NULL) { \ - const char *categories = ((acls) == NULL || !strcmp(acls, "")) \ - ? LIBMR_ACL_COMMAND_CATEGORY_NAME \ - : acls " " LIBMR_ACL_COMMAND_CATEGORY_NAME; \ - if (RedisModule_SetCommandACLCategories(command, categories) == \ - REDISMODULE_OK) { \ - result = true; \ - } \ - } \ - } \ - result; \ - }) - /** @brief Register a new Redis command with the required ACLs. * @see RedisModule_CreateCommand * @return true if the command was registered successfully, false @@ -71,7 +44,7 @@ static inline __attribute__((always_inline)) bool RegisterRedisCommand(RedisModuleCtx *ctx, const char *name, RedisModuleCmdFunc cmdfunc, const char *strflags, - int firstkey, int lastkey, int keystep, const bool setAcls) { + int firstkey, int lastkey, int keystep) { const int ret = RedisModule_CreateCommand(ctx, name, cmdfunc, strflags, firstkey, lastkey, keystep); @@ -81,11 +54,7 @@ RegisterRedisCommand(RedisModuleCtx *ctx, const char *name, return false; } - if (!setAcls) { - return true; - } - - return SetCommandAcls(ctx, name, ""); + return true; } typedef enum NodeStatus{ @@ -118,7 +87,6 @@ typedef struct Node{ char* id; char* ip; unsigned short port; - char* username; char* password; char* unixSocket; redisAsyncContext *c; @@ -153,7 +121,6 @@ struct ClusterCtx { char myId[REDISMODULE_NODE_ID_LEN + 1]; int isOss; functionId networkTestMsgReciever; - char *username; char *password; }clusterCtx; @@ -617,10 +584,17 @@ static void MR_OnConnectCallback(const struct redisAsyncContext* c, int status){ RedisModule_Log(mr_staticCtx, "notice", "connected : %s:%d, status = %d\r\n", c->c.tcp.host, c->c.tcp.port, status); - if (n->username && n->password) { - redisAsyncCommand((redisAsyncContext*)c, NULL, NULL, "AUTH %s %s", n->username, n->password); - } else if (n->password){ + if (n->password){ + /* If password is provided to us we will use it (it means it was given to us with clusterset) */ redisAsyncCommand((redisAsyncContext*)c, NULL, NULL, "AUTH %s", n->password); + } else if (RedisModule_GetInternalSecret && !MR_IsEnterpriseBuild()) { + /* OSS deployment that support internal secret, lets use it. */ + RedisModule_ThreadSafeContextLock(mr_staticCtx); + size_t len; + const char *secret = RedisModule_GetInternalSecret(mr_staticCtx, &len); + RedisModule_Assert(secret); + redisAsyncCommand((redisAsyncContext*)c, NULL, NULL, "AUTH %s %b", "internal connection", secret, len); + RedisModule_ThreadSafeContextUnlock(mr_staticCtx); } if(n->sendClusterTopologyOnNextConnect && clusterCtx.CurrCluster->clusterSetCommand){ @@ -746,14 +720,13 @@ static Node* MR_GetNode(const char* id){ return n; } -static Node* MR_CreateNode(const char* id, const char* ip, unsigned short port, const char* username, const char* password, const char* unixSocket, size_t minSlot, size_t maxSlot){ +static Node* MR_CreateNode(const char* id, const char* ip, unsigned short port, const char* password, const char* unixSocket, size_t minSlot, size_t maxSlot){ RedisModule_Assert(!MR_GetNode(id)); Node* n = MR_ALLOC(sizeof(*n)); *n = (Node){ .id = MR_STRDUP(id), .ip = MR_STRDUP(ip), .port = port, - .username = username ? MR_STRDUP(username) : NULL, .password = password ? MR_STRDUP(password) : NULL, .unixSocket = unixSocket ? MR_STRDUP(unixSocket) : NULL, .c = NULL, @@ -847,7 +820,8 @@ static void MR_RefreshClusterData(){ Node* n = MR_GetNode(nodeId); if(!n){ - n = MR_CreateNode(nodeId, nodeIp, (unsigned short)port, clusterCtx.username, clusterCtx.password, NULL, minslot, maxslot); + /* If we have internal secret we will ignore the clusterCtx.password, we do not need it. */ + n = MR_CreateNode(nodeId, nodeIp, (unsigned short)port, RedisModule_GetInternalSecret ? NULL : clusterCtx.password, NULL, minslot, maxslot); } if (n->isMe) { @@ -964,7 +938,7 @@ static void MR_SetClusterData(RedisModuleString** argv, int argc){ Node* n = MR_GetNode(realId); if(!n){ - n = MR_CreateNode(realId, ip, port, NULL, password, NULL, minslot, maxslot); + n = MR_CreateNode(realId, ip, port, password, NULL, minslot, maxslot); } for(int i = minslot ; i <= maxslot ; ++i){ clusterCtx.CurrCluster->slots[i] = n; @@ -1349,7 +1323,7 @@ static void MR_NetworkTest(RedisModuleCtx *ctx, const char *sender_id, uint8_t t RedisModule_Log(ctx, "notice", "got a nextwork test msg"); } -int MR_ClusterInit(RedisModuleCtx* rctx, char *username, char *password) { +int MR_ClusterInit(RedisModuleCtx* rctx, char *password) { clusterCtx.CurrCluster = NULL; clusterCtx.callbacks = array_new(MR_ClusterMessageReceiver, 10); clusterCtx.nodesMsgIds = mr_dictCreate(&mr_dictTypeHeapStrings, NULL); @@ -1357,7 +1331,6 @@ int MR_ClusterInit(RedisModuleCtx* rctx, char *username, char *password) { clusterCtx.maxSlot = 0; clusterCtx.clusterSize = 1; clusterCtx.isOss = true; - clusterCtx.username = username ? MR_STRDUP(username) : NULL; clusterCtx.password = password ? MR_STRDUP(password) : NULL; memset(clusterCtx.myId, '0', REDISMODULE_NODE_ID_LEN); @@ -1373,56 +1346,57 @@ int MR_ClusterInit(RedisModuleCtx* rctx, char *username, char *password) { const char *command_flags = "readonly deny-script"; if (MR_IsEnterpriseBuild()) { command_flags = "readonly deny-script _proxy-filtered"; + } else { + if (RedisModule_GetInternalSecret) { + /* We run at a version that supports internal commands, let use it. */ + command_flags = "readonly deny-script internal"; + } } - if (RedisModule_AddACLCategory) { - if (RedisModule_AddACLCategory(rctx, LIBMR_ACL_COMMAND_CATEGORY_NAME) != REDISMODULE_OK) { - RedisModule_Log(rctx, "error", "Failed to add ACL category"); - + if (!MR_IsEnterpriseBuild()) { + /* Refresh cluster is only relevant for oss, also notice that refresh cluster + * is not considered internal and should be performed by the user. */ + if (!RegisterRedisCommand(rctx, CLUSTER_REFRESH_COMMAND, MR_ClusterRefresh, + "readonly deny-script", 0, 0, 0)) { return REDISMODULE_ERR; } } - if (!RegisterRedisCommand(rctx, CLUSTER_REFRESH_COMMAND, MR_ClusterRefresh, - command_flags, 0, 0, 0, false)) { - return REDISMODULE_ERR; - } - if (!RegisterRedisCommand(rctx, CLUSTER_SET_COMMAND, MR_ClusterSet, - command_flags, 0, 0, -1, false)) { + command_flags, 0, 0, -1)) { return REDISMODULE_ERR; } if (!RegisterRedisCommand(rctx, CLUSTER_SET_FROM_SHARD_COMMAND, MR_ClusterSetFromShard, command_flags, 0, 0, - -1, true)) { + -1)) { return REDISMODULE_ERR; } if (!RegisterRedisCommand(rctx, CLUSTER_HELLO_COMMAND, MR_ClusterHello, - command_flags, 0, 0, 0, true)) { + command_flags, 0, 0, 0)) { return REDISMODULE_ERR; } if (!RegisterRedisCommand(rctx, CLUSTER_INNER_COMMUNICATION_COMMAND, MR_ClusterInnerCommunicationMsg, command_flags, 0, 0, - 0, true)) { + 0)) { return REDISMODULE_ERR; } if (!RegisterRedisCommand(rctx, NETWORK_TEST_COMMAND, MR_NetworkTestCommand, - command_flags, 0, 0, 0, true)) { + command_flags, 0, 0, 0)) { return REDISMODULE_ERR; } if (!RegisterRedisCommand(rctx, CLUSTER_INFO_COMMAND, MR_ClusterInfoCommand, - command_flags, 0, 0, 0, true)) { + command_flags, 0, 0, 0)) { return REDISMODULE_ERR; } if (!RegisterRedisCommand(rctx, FORCE_SHARDS_CONNECTION, MR_ForceShardsConnectionCommand, command_flags, 0, 0, - 0, true)) { + 0)) { return REDISMODULE_ERR; } diff --git a/src/cluster.h b/src/cluster.h index 743f6e4..06a1c59 100644 --- a/src/cluster.h +++ b/src/cluster.h @@ -37,7 +37,7 @@ int MR_IsClusterInitialize(); size_t MR_ClusterGetSize(); -int MR_ClusterInit(RedisModuleCtx* rctx, char *username, char *password); +int MR_ClusterInit(RedisModuleCtx* rctx, char *password); size_t MR_ClusterGetSlotdByKey(const char* key, size_t len); diff --git a/src/common.h b/src/common.h index db9f274..6b95bc6 100644 --- a/src/common.h +++ b/src/common.h @@ -14,17 +14,6 @@ #error "MODULE_NAME is not defined" #endif -/** The name of the ACL category for the commands created by LibMR for - * its own operations. - * - * The user may redefine the category name by defining the macro - * LIBMR_ACL_COMMAND_CATEGORY_NAME before including this header. - */ -#ifndef LIBMR_ACL_COMMAND_CATEGORY_NAME -#define LIBMR_ACL_COMMAND_CATEGORY_NAME \ - "_" xstr(MODULE_NAME) "_libmr_internal" -#endif - typedef struct MR_RedisVersion { int redisMajorVersion; diff --git a/src/mr.c b/src/mr.c index fff578f..79875e1 100644 --- a/src/mr.c +++ b/src/mr.c @@ -1405,11 +1405,11 @@ static void MR_GetRedisVersion() { RedisModule_FreeCallReply(reply); } -int MR_Init(RedisModuleCtx* ctx, size_t numThreads, char *username, char *password) { +int MR_Init(RedisModuleCtx* ctx, size_t numThreads, char *password) { mr_staticCtx = RedisModule_GetDetachedThreadSafeContext(ctx); MR_GetRedisVersion(); - if (MR_ClusterInit(ctx, username, password) != REDISMODULE_OK) { + if (MR_ClusterInit(ctx, password) != REDISMODULE_OK) { return REDISMODULE_ERR; } diff --git a/src/mr.h b/src/mr.h index 1ee1c85..0589c81 100644 --- a/src/mr.h +++ b/src/mr.h @@ -152,7 +152,7 @@ LIBMR_API void MR_Run(Execution* e); LIBMR_API void MR_FreeExecution(Execution* e); /* Initialize mr library */ -LIBMR_API int MR_Init(struct RedisModuleCtx* ctx, size_t numThreads, char *username, char *password); +LIBMR_API int MR_Init(struct RedisModuleCtx* ctx, size_t numThreads, char *password); /* Register a new object type */ LIBMR_API int MR_RegisterObject(MRObjectType* t); diff --git a/src/redismodule.h b/src/redismodule.h index 0e96e2c..cf063bf 100644 --- a/src/redismodule.h +++ b/src/redismodule.h @@ -1324,6 +1324,7 @@ REDISMODULE_API RedisModuleRdbStream *(*RedisModule_RdbStreamCreateFromFile)(con REDISMODULE_API void (*RedisModule_RdbStreamFree)(RedisModuleRdbStream *stream) REDISMODULE_ATTR; REDISMODULE_API int (*RedisModule_RdbLoad)(RedisModuleCtx *ctx, RedisModuleRdbStream *stream, int flags) REDISMODULE_ATTR; REDISMODULE_API int (*RedisModule_RdbSave)(RedisModuleCtx *ctx, RedisModuleRdbStream *stream, int flags) REDISMODULE_ATTR; +REDISMODULE_API const char * (*RedisModule_GetInternalSecret)(RedisModuleCtx *ctx, size_t *len) REDISMODULE_ATTR; #define RedisModule_IsAOFClient(id) ((id) == UINT64_MAX) @@ -1694,6 +1695,7 @@ static int RedisModule_Init(RedisModuleCtx *ctx, const char *name, int ver, int REDISMODULE_GET_API(RdbStreamFree); REDISMODULE_GET_API(RdbLoad); REDISMODULE_GET_API(RdbSave); + REDISMODULE_GET_API(GetInternalSecret); #ifdef REDISMODULE_RLEC_API_DEFS diff --git a/tests/mr_test_module/pytests/common.py b/tests/mr_test_module/pytests/common.py index 2f2073e..febf7d2 100644 --- a/tests/mr_test_module/pytests/common.py +++ b/tests/mr_test_module/pytests/common.py @@ -89,6 +89,11 @@ def shardsConnections(env): def verifyClusterInitialized(env): for conn in shardsConnections(env): + try: + # try to promote to internal connection + conn.execute_command('debug', 'MARK-INTERNAL-CLIENT') + except Exception: + pass allConnected = False while not allConnected: res = conn.execute_command('MRTESTS.INFOCLUSTER') @@ -108,7 +113,13 @@ def initialiseCluster(env): # able to execute commands on shards, on slow envs, run with valgrind, # or mac, it is needed. env.broadcast('CONFIG', 'set', 'cluster-node-timeout', '120000') - env.broadcast('MRTESTS.FORCESHARDSCONNECTION') + for conn in shardsConnections(env): + try: + conn.execute_command('debug', 'MARK-INTERNAL-CLIENT') + except Exception as e: + print(e) + pass + conn.execute_command('MRTESTS.FORCESHARDSCONNECTION') with TimeLimit(2): verifyClusterInitialized(env) @@ -141,7 +152,11 @@ def test_func(): raise unittest.SkipTest() if skipOnVersionLowerThan: skip_if_redis_version_is_lower_than(skipOnVersionLowerThan) - envArgs['moduleArgs'] = moduleArgs or None + defaultModuleArgs = 'password' + if not is_redis_version_is_lower_than('8.0.0'): + # We provide password only if version < 8.0.0. If version is greater, we have internal command and we do not need the password. + defaultModuleArgs = None + envArgs['moduleArgs'] = moduleArgs or defaultModuleArgs envArgs['redisConfigFile'] = create_config_file(redisConfigFileContent) if redisConfigFileContent else None env = Env(**envArgs) conn = getConnectionByEnv(env) diff --git a/tests/mr_test_module/pytests/run_tests.sh b/tests/mr_test_module/pytests/run_tests.sh index e35ab03..e0842e7 100755 --- a/tests/mr_test_module/pytests/run_tests.sh +++ b/tests/mr_test_module/pytests/run_tests.sh @@ -19,4 +19,4 @@ else fi -python3 -m RLTest --verbose-information-on-failure --module $MODULE_PATH --clear-logs "$@" --oss_password "password" +python3 -m RLTest --verbose-information-on-failure --module $MODULE_PATH --clear-logs "$@" --oss_password "password" --enable-debug-command diff --git a/tests/mr_test_module/pytests/test_basic.py b/tests/mr_test_module/pytests/test_basic.py index 5b14bc2..d2d4f4e 100644 --- a/tests/mr_test_module/pytests/test_basic.py +++ b/tests/mr_test_module/pytests/test_basic.py @@ -80,54 +80,12 @@ def testRemoteTaskOnAllShards(env, conn): conn.execute_command('del', 'doc%d' % i) env.expect('lmrtest.dbsize').equal(0) -@MRTestDecorator( - redisConfigFileContent='user gooduser on >password -@all +@_MRTESTS_libmr_internal +MRTESTS.FORCESHARDSCONNECTION +MRTESTS.INFOCLUSTER\nuser default on >password +@all -@_MRTESTS_libmr_internal -MRTESTS.FORCESHARDSCONNECTION -MRTESTS.INFOCLUSTER\n', - moduleArgs='gooduser', - skipClusterInitialisation=True, - skipOnVersionLowerThan='7.4.0', - envArgs={'password': 'password'}, - # skipOnSingleShard=True, -) -def testAclSetting(env, conn): - ''' - Tests that LibMR sets the ACLs for its commands. - ''' - env.skipOnVersionSmaller('7.4.0') - acl_category = '_MRTESTS_libmr_internal' - env.expect('acl', 'cat').contains(acl_category) - - # Test that the user not allowed to run the commands, as this - # module uses the "default" user to run the commands instead. - command = 'lmrtest.dbsize' - env.expect('ACL', 'SETUSER', 'user1', 'on', '>user1p', '-@all', '+%s' % command).contains('OK') - env.expect('ACL', 'SETUSER', 'user2', 'on', '>user2p', '+@all', '-@%s' % acl_category).contains('OK') - env.expect('AUTH', 'user1', 'user1p').equal(True) - - env.expect('lmrtest.dbsize').equal(0) - - # This should succeed even though the user is not allowed to run - # the commands of libmr. This is so, because the module itself runs - # the LibMR commands as the other user specified during the load, - # which has the necessary permissions. - env.expect('AUTH', 'user2', 'user2p').equal(True) - env.expect('lmrtest.dbsize').equal(0) - -@MRTestDecorator( - redisConfigFileContent='user baduser on >password +@all -@_MRTESTS_libmr_internal +MRTESTS.FORCESHARDSCONNECTION +MRTESTS.INFOCLUSTER', - moduleArgs='baduser', - skipClusterInitialisation=True, - skipOnVersionLowerThan='7.4.0', - skipOnSingleShard=True, -) -def testAclSettingNotWorksWhenItShouldnt(env, conn): - ''' - Tests that LibMR doesn't work when the user provided for it doesn't - have the necessary permissions to run the LibMR commands. - ''' - - # This should fail as the LibMR will attempt to connect to the - # shards using the "baduser" user, which doesn't have the necessary - # permissions to run the LibMR commands. - if env.isCluster(): - with pytest.raises(ShardsConnectionTimeoutException): - initialiseCluster(env) +@MRTestDecorator(skipOnVersionLowerThan='8.0.0', skipOnCluster=False) +def testInternalCommandsAreNotAllowed(env, conn): + env.expect('MRTESTS.CLUSTERSET').error().contains('unknown command') + env.expect('MRTESTS.INNERCOMMUNICATION').error().contains('unknown command') + env.expect('MRTESTS.HELLO').error().contains('unknown command') + env.expect('MRTESTS.CLUSTERSETFROMSHARD').error().contains('unknown command') + env.expect('MRTESTS.INFOCLUSTER').error().contains('unknown command') + env.expect('MRTESTS.NETWORKTEST').error().contains('unknown command') + env.expect('MRTESTS.FORCESHARDSCONNECTION').error().contains('unknown command') diff --git a/tests/mr_test_module/pytests/test_network.py b/tests/mr_test_module/pytests/test_network.py index cc59c7b..35975f2 100644 --- a/tests/mr_test_module/pytests/test_network.py +++ b/tests/mr_test_module/pytests/test_network.py @@ -181,6 +181,11 @@ def _handle_conn(self, sock, client_addr): self.new_conns.put(conn) def _send_cluster_set(self): + try: + # try to promote to internal connection + self.env.cmd('debug', 'MARK-INTERNAL-CLIENT') + except Exception: + pass self.env.cmd('MRTESTS.CLUSTERSET', 'NO-USED', 'NO-USED', @@ -521,6 +526,11 @@ def testClusterSetAfterHelloResponseFailure(env, conn): conn.send_error('err') # hello response, sending runid # resend cluster set + try: + # try to promote to internal connection + env.cmd('debug', 'MARK-INTERNAL-CLIENT') + except Exception: + pass res = env.cmd('MRTESTS.CLUSTERSET', 'NO-USED', 'NO-USED', @@ -553,6 +563,11 @@ def testClusterSetAfterDisconnect(env, conn): conn.close() + try: + # try to promote to internal connection + env.cmd('debug', 'MARK-INTERNAL-CLIENT') + except Exception: + pass # resend cluster set res = env.cmd('MRTESTS.CLUSTERSET', 'NO-USED', diff --git a/tests/mr_test_module/src/lib.rs b/tests/mr_test_module/src/lib.rs index c57debd..04970e1 100644 --- a/tests/mr_test_module/src/lib.rs +++ b/tests/mr_test_module/src/lib.rs @@ -855,11 +855,10 @@ fn init_func(ctx: &Context, args: &[RedisString]) -> Status { } let mut args_iter = args.iter(); - let user = args_iter.next().map(|v| v.to_string()); let pass = args_iter .next() - .map_or_else(|| Some("password".to_string()), |v| Some(v.to_string())); - mr_init(ctx, 5, user.as_deref(), pass.as_deref()); + .map(|v| Some(v.to_string())).flatten(); + mr_init(ctx, 5, pass.as_deref()); KeysReader::register(); Status::Ok