Database-level access control #2309
Conversation
- Implemented database permissions using `db=<id>`, `db!=<id>`, `alldbs`, `resetdbs` ACL rules - Added SELECTOR_FLAG_ALLDBS and SELECTOR_FLAG_DBLIST_NEGATED flags - Updated SELECT, MOVE, SWAPDB, FLUSHDB commands with CMD_CROSS_DB flag, FLUSHALL - CMD_ALL_DBS - Extended ACL checks with database access verification - Added ACL_DENIED_DB error type for database permission violations - Maintained backward compatibility with default access to all databases Signed-off-by: Daniil Kashapov <daniil.kashapov.ykt@gmail.com>
|
I would like to first discuss this new feature in the weekly meeting. Thanks |
Signed-off-by: Daniil Kashapov <daniil.kashapov.ykt@gmail.com>
src/module.c
Outdated
| @@ -6527,7 +6527,10 @@ ValkeyModuleCallReply *VM_Call(ValkeyModuleCtx *ctx, const char *cmdname, const | |||
| int acl_errpos; | |||
| int acl_retval; | |||
|
|
|||
| acl_retval = ACLCheckAllUserCommandPerm(user, c->cmd, c->argv, c->argc, &acl_errpos); | |||
| int dbid = (c->flag.multi) ? | |||
There was a problem hiding this comment.
Additionally for modules, we'll eventually probably want:
- A new API for checking if the user can access a DB
- New flags for CrossDB/All DBs. Might be able to just skip this for now.
There was a problem hiding this comment.
Both would be useful. Seems like API and flags won't be a lot of code, should we do this in this PR or do as a follow-up with another Issue?
| @@ -97,6 +98,15 @@ void queueMultiCommand(client *c, uint64_t cmd_flags) { | |||
| mc->argv = c->argv; | |||
| mc->argv_len = c->argv_len; | |||
| mc->slot = c->slot; | |||
|
|
|||
| if (mc->cmd->proc == selectCommand && mc->argc > 1) { | |||
There was a problem hiding this comment.
We have some other code like this in cluster.c, we probably should write helpers for each command that accesses DBs and provide a helper function. Basically all functions can access up to two DBs, so we should be able to construct an interface around that.
There was a problem hiding this comment.
I wrote an implementation that extends serverCommand with hepler function that extracts db args from command, is that what you meant and what we should use in that situation? Or you meant something else that could potentially also resolve same hardcoded commands in cluster.c?
Signed-off-by: Daniil Kashapov <daniil.kashapov.ykt@gmail.com>
|
Just discussed in the weekly meeting. It seems like we are all aligned to add more database features. We don't want folks to use database instead of true multi-tenancy, like running multiple containers, but there are still plenty of workloads that could benefit from the having access control on databases for namespacing. So we'll move forward with this feature. |
This PR is open for review. What is already done:
db=<id>,alldbs,resetdbsACL rules, for multiple dbids comma,is separating multiple IDsHere are topics that need discussion:
VM_ACLCheckCommandPermissionsto include dbid for proper permission checking?VM_ACLCheckKeyPermissionsandACLUserCheckKeyPerm()respectively?Resolved topics:
CROSS_DBandALL_DBScommands:get_dbid_argsand made ACL checks based on IDs provided byget_dbid_args.Syntax examples:
Closes: #1336