Skip to content
Draft
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions src/module.c
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@
#include "io_threads.h"
#include "scripting_engine.h"
#include "cluster_migrateslots.h"
#include <cerrno>
#include <dlfcn.h>
#include <sys/stat.h>
#include <sys/wait.h>
Expand Down Expand Up @@ -9993,6 +9994,40 @@ int VM_ACLCheckCommandPermissions(ValkeyModuleUser *user, ValkeyModuleString **a
return VALKEYMODULE_OK;
}

/* Checks if the command can be executed by the module context user, according to the ACLs
* associated with it.
*
* On success a VALKEYMODULE_OK is returned, otherwise
* VALKEYMODULE_ERR is returned and errno is set to the following values:
*
* * ENOENT: Specified command does not exist.
* * EACCES: Command cannot be executed, according to ACL rules; or the user is not authenticated.
* * EINVAL: Invalid module context.
* * ENOTSUP: There is no user associated with this module context.
*/
int VM_ACLCheckCommandPermissionsForCurrentUser(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int argc) {
if (ctx == NULL) {
errno = EINVAL;
return VALKEYMODULE_ERR;
} else if (ctx->user == NULL && (ctx->client == NULL || ctx->client->user == NULL)) {
errno = ENOTSUP;
return VALKEYMODULE_ERR;
}

/* If the ctx->user was set by VM_SetContextUser then use that user instead
* of the ctx->client->user. */
if (ctx->user) {
return VM_ACLCheckCommandPermissions((ValkeyModuleUser *)ctx->user, argv, argc);
}

ValkeyModuleUser user = {
.user = ctx->client->user,
.free_user = 0,
};
Comment on lines +10022 to +10025
Copy link
Member

@madolson madolson Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This entire API is because we're trying to bypass the allocation and do it on the stack here. When the API was written, we cared more about abstraction then performance. If we think performance is so critical, I would rather just ditch the abstraction and directly return the user from the module APIs.

Glancing through the code I don't see any incompatibility with that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you proposing to make ValkeyModuleUser a type alias to user?

In that case we would probably need to add a field in the user struct to distinguish between users created by modules, and users created in the core.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case we would probably need to add a field in the user struct to distinguish between users created by modules, and users created in the core.

Yes. We don't expect a large number of users, so adding an extra field should be safe. I think we even have a bit field we could use if we wanted to avoid adding any memory at all.


return VM_ACLCheckCommandPermissions(&user, argv, argc);
}

/* Check if the key can be accessed by the user according to the ACLs attached to the user
* and the flags representing the key access. The flags are the same that are used in the
* keyspec for logical operations. These flags are documented in ValkeyModule_SetCommandInfo as
Expand Down Expand Up @@ -14330,6 +14365,7 @@ void moduleRegisterCoreAPI(void) {
REGISTER_API(GetCurrentUserName);
REGISTER_API(GetModuleUserFromUserName);
REGISTER_API(ACLCheckCommandPermissions);
REGISTER_API(ACLCheckCommandPermissionsForCurrentUser);
REGISTER_API(ACLCheckKeyPermissions);
REGISTER_API(ACLCheckChannelPermissions);
REGISTER_API(ACLAddLogEntry);
Expand Down
4 changes: 4 additions & 0 deletions src/valkeymodule.h
Original file line number Diff line number Diff line change
Expand Up @@ -1844,6 +1844,9 @@ VALKEYMODULE_API ValkeyModuleUser *(*ValkeyModule_GetModuleUserFromUserName)(Val
VALKEYMODULE_API int (*ValkeyModule_ACLCheckCommandPermissions)(ValkeyModuleUser *user,
ValkeyModuleString **argv,
int argc) VALKEYMODULE_ATTR;
VALKEYMODULE_API int (*ValkeyModule_ACLCheckCommandPermissionsForCurrentUser)(ValkeyModuleCtx *ctx,
ValkeyModuleString **argv,
int argc) VALKEYMODULE_ATTR;
VALKEYMODULE_API int (*ValkeyModule_ACLCheckKeyPermissions)(ValkeyModuleUser *user,
ValkeyModuleString *key,
int flags) VALKEYMODULE_ATTR;
Expand Down Expand Up @@ -2295,6 +2298,7 @@ static int ValkeyModule_Init(ValkeyModuleCtx *ctx, const char *name, int ver, in
VALKEYMODULE_GET_API(GetCurrentUserName);
VALKEYMODULE_GET_API(GetModuleUserFromUserName);
VALKEYMODULE_GET_API(ACLCheckCommandPermissions);
VALKEYMODULE_GET_API(ACLCheckCommandPermissionsForCurrentUser);
VALKEYMODULE_GET_API(ACLCheckKeyPermissions);
VALKEYMODULE_GET_API(ACLCheckChannelPermissions);
VALKEYMODULE_GET_API(ACLAddLogEntry);
Expand Down
58 changes: 39 additions & 19 deletions tests/modules/aclcheck.c
Original file line number Diff line number Diff line change
Expand Up @@ -82,17 +82,21 @@ int publish_aclcheck_channel(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, in
}

/* A wrap for RM_Call that check first that the command can be executed */
int rm_call_aclcheck_cmd(ValkeyModuleCtx *ctx, ValkeyModuleUser *user, ValkeyModuleString **argv, int argc) {
int rm_call_aclcheck_context_user_cmd(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int argc) {
if (argc < 2) {
return ValkeyModule_WrongArity(ctx);
}

/* Check that the command can be executed */
int ret = ValkeyModule_ACLCheckCommandPermissions(user, argv + 1, argc - 1);
int ret = ValkeyModule_ACLCheckCommandPermissionsForCurrentUser(ctx, argv + 1, argc - 1);
if (ret != 0) {
ValkeyModule_ReplyWithError(ctx, "DENIED CMD");
/* Add entry to ACL log */
ValkeyModuleString *user_name = ValkeyModule_GetCurrentUserName(ctx);
ValkeyModuleUser *user = ValkeyModule_GetModuleUserFromUserName(user_name);
ValkeyModule_ACLAddLogEntry(ctx, user, argv[1], VALKEYMODULE_ACL_LOG_CMD);
ValkeyModule_FreeModuleUser(user);
ValkeyModule_FreeString(ctx, user_name);
return VALKEYMODULE_OK;
}

Expand All @@ -109,15 +113,31 @@ int rm_call_aclcheck_cmd(ValkeyModuleCtx *ctx, ValkeyModuleUser *user, ValkeyMod
return VALKEYMODULE_OK;
}

int rm_call_aclcheck_cmd_default_user(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int argc) {
ValkeyModuleString *user_name = ValkeyModule_GetCurrentUserName(ctx);
ValkeyModuleUser *user = ValkeyModule_GetModuleUserFromUserName(user_name);
int rm_call_aclcheck_cmd(ValkeyModuleCtx *ctx, ValkeyModuleUser *user, ValkeyModuleString **argv, int argc) {
if (argc < 2) {
return ValkeyModule_WrongArity(ctx);
}

int res = rm_call_aclcheck_cmd(ctx, user, argv, argc);
/* Check that the command can be executed */
int ret = ValkeyModule_ACLCheckCommandPermissions(user, argv + 1, argc - 1);
if (ret != 0) {
ValkeyModule_ReplyWithError(ctx, "DENIED CMD");
/* Add entry to ACL log */
ValkeyModule_ACLAddLogEntry(ctx, user, argv[1], VALKEYMODULE_ACL_LOG_CMD);
return VALKEYMODULE_OK;
}

ValkeyModule_FreeModuleUser(user);
ValkeyModule_FreeString(ctx, user_name);
return res;
const char* cmd = ValkeyModule_StringPtrLen(argv[1], NULL);

ValkeyModuleCallReply* rep = ValkeyModule_Call(ctx, cmd, "v", argv + 2, (size_t)argc - 2);
if (!rep) {
ValkeyModule_ReplyWithError(ctx, "NULL reply returned");
} else {
ValkeyModule_ReplyWithCallReply(ctx, rep);
ValkeyModule_FreeCallReply(rep);
}

return VALKEYMODULE_OK;
}

int rm_call_aclcheck_cmd_module_user(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int argc) {
Expand Down Expand Up @@ -199,14 +219,14 @@ int commandBlockCheck(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int argc)

result = ValkeyModule_AddACLCategory(ctx,"blockedcategory");
response_ok |= (result == VALKEYMODULE_OK);

ValkeyModuleCommand *parent = ValkeyModule_GetCommand(ctx,"block.commands.outside.onload");
result = ValkeyModule_SetCommandACLCategories(parent, "write");
response_ok |= (result == VALKEYMODULE_OK);

result = ValkeyModule_CreateSubcommand(parent,"subcommand.that.should.fail",module_test_acl_category,"",0,0,0);
response_ok |= (result == VALKEYMODULE_OK);

/* This validates that it's not possible to create commands or add
* a new ACL Category outside OnLoad function.
* thus returns an error if they succeed. */
Expand All @@ -224,7 +244,7 @@ int ValkeyModule_OnLoad(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int arg
return VALKEYMODULE_ERR;

if (argc > 1) return ValkeyModule_WrongArity(ctx);

/* When that flag is passed, we try to create too many categories,
* and the test expects this to fail. In this case the server returns VALKEYMODULE_ERR
* and set errno to ENOMEM*/
Expand Down Expand Up @@ -274,7 +294,7 @@ int ValkeyModule_OnLoad(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int arg
if (ValkeyModule_CreateCommand(ctx,"aclcheck.publish.check.channel", publish_aclcheck_channel,"",0,0,0) == VALKEYMODULE_ERR)
return VALKEYMODULE_ERR;

if (ValkeyModule_CreateCommand(ctx,"aclcheck.rm_call.check.cmd", rm_call_aclcheck_cmd_default_user,"",0,0,0) == VALKEYMODULE_ERR)
if (ValkeyModule_CreateCommand(ctx,"aclcheck.rm_call.check.cmd", rm_call_aclcheck_context_user_cmd,"",0,0,0) == VALKEYMODULE_ERR)
return VALKEYMODULE_ERR;

if (ValkeyModule_CreateCommand(ctx,"aclcheck.rm_call.check.cmd.module.user", rm_call_aclcheck_cmd_module_user,"",0,0,0) == VALKEYMODULE_ERR)
Expand All @@ -292,25 +312,25 @@ int ValkeyModule_OnLoad(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int arg
* the server returns VALKEYMODULE_ERR and set errno to `EINVAL` */
if (ValkeyModule_AddACLCategory(ctx,"!nval!dch@r@cter$") == VALKEYMODULE_ERR)
ValkeyModule_Assert(errno == EINVAL);
else
else
return VALKEYMODULE_ERR;

/* This validates that, when module tries to add a category that already exists,
* the server returns VALKEYMODULE_ERR and set errno to `EBUSY` */
if (ValkeyModule_AddACLCategory(ctx,"write") == VALKEYMODULE_ERR)
ValkeyModule_Assert(errno == EBUSY);
else
else
return VALKEYMODULE_ERR;

if (ValkeyModule_AddACLCategory(ctx,"foocategory") == VALKEYMODULE_ERR)
return VALKEYMODULE_ERR;

if (ValkeyModule_CreateCommand(ctx,"aclcheck.module.command.test.add.new.aclcategories", module_test_acl_category,"",0,0,0) == VALKEYMODULE_ERR)
return VALKEYMODULE_ERR;
ValkeyModuleCommand *test_add_new_aclcategories = ValkeyModule_GetCommand(ctx,"aclcheck.module.command.test.add.new.aclcategories");

if (ValkeyModule_SetCommandACLCategories(test_add_new_aclcategories, "foocategory") == VALKEYMODULE_ERR)
return VALKEYMODULE_ERR;

return VALKEYMODULE_OK;
}
Loading