Database-level access control by dvkashapov · Pull Request #2309 · valkey-io/valkey

dvkashapov · 2025-07-04T12:18:20Z

API changes and user behavior:

Default behavior for database access.

Default is alldbs permissions.

Database Permissions (`db=`)

Accessing particular database

> ACL SETUSER test1 on +@all ~* resetdbs db=0,1 nopass
"user test1 on nopass sanitize-payload ~* resetchannels db=0,1 +@all"

(Same behavior without usage of resetdbs)

> ACL SETUSER test1 on +@all ~* db=0,1 nopass
"user test1 on nopass sanitize-payload ~* resetchannels db=0,1 +@all"

Multiple selector can be provided

> ACL SETUSER test1 on nopass (db=0,1 +@write +select ~*) (db=2,3 +@read +select ~*)
"user test1 on nopass sanitize-payload resetchannels alldbs -@all (~* resetchannels db=0,1 -@all +@write +select) (~* resetchannels db=2,3 -@all +@read +select)"

Restricting special commands which access databases as part of the command.

The user needs to have access to both the commands and db(s) part of the command to run these commands.

SWAPDB
SELECT
MOVE - (Select command would have went through for the source database). Have access for the target database.
COPY

Restricting special commands which doesn't specify database number, however, accesses multiple databases.

The user needs to have access to both the commands and all databases (alldbs) to run these commands.

FLUSHALL - Access all databases
CLUSTER commands that access all databases:
- CANCELSLOTMIGRATIONS
- MIGRATESLOTS

New connection establishment behavior
New client connection gets established to DB 0 by default. Authentication and authorisation are decoupled and the user can connect/authenticate and further perform SELECT or other operation that do not access keyspace.

(Do we want to extend HELLO?) Alternative suggestion by @madolson: Extend HELLO command to pass the dbid to which the user should get connected after authentication if they have right set of permission. I think it will become a long poll for adoption.

Observability
Extend ACL LOG to log user which received denied permission error while accessing a database.
Module API

Introduce module API int VM_ACLCheckPermissions(ValkeyModuleUser *user, ValkeyModuleString **argv, int argc, int dbid, ValkeyModuleACLLogEntryReason *denial_reason);
Stop support of VM_ACLCheckCommandPermissions().

Resolves: #1336

dvkashapov · 2025-07-04T12:21:24Z

Hello! I would greatly appreciate any feedback from ACL experts)
@xbasel @hpatro @madolson

hwware · 2025-07-04T14:45:41Z

I would like to first discuss this new feature in the weekly meeting. Thanks

src/acl.c

src/module.c

src/multi.c

madolson · 2025-07-14T14:55:52Z

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.

hpatro

Thanks for working on this @JoBeR007.

High level comment:

I would prefer us to come up with a symbol for DB, let's say ^ and use that instead of db= as prefix to have the same experience as other Then it would look like,

All database access: ACL SETUSER alice ^*
Selective database access: ACL SETUSER bob ^1 ^2

With DB I find providing both ALLOW and DENY would be helpful. Possibly extend it to other resources in a separate PR.

Restrict admin db access via (-^?):

Allow all except db 0: ACL SETUSER alice -^0

For FLUSHALL and FLUSHDB the command can get accepted as ASYNC and while it's getting executed the permission could change. I think the correct behavior is to still process the command completely even if the permission has changed at a later point. (The PR has the correct behavior).

src/acl.c

dvkashapov · 2025-08-01T06:04:22Z

Thanks for review @hpatro!
I see where you're coming from in terms of one token per symbol. Do you think originally proposed syntax (db=<list> / db!=<list>) is worse than one token per symbol? For me explicit list seemed like a better idea in terms of specifying multiple selectors because it provided better readability and seemed more compact.
Also about ALLOW and DENY ACL: I suggest in this PR we add only ALLOW policies and as a separate PR I will add DENY policy for different rules and from that point we will think about usefulness of each DENY policy.

madolson · 2025-09-03T19:53:22Z

@dvkashapov Just to be transparent with you. This PR was opened at a time when we were working on the 9.0 release, and so we weren't going to merge this feature in to it. Now that the RC candidates are mostly done we can start reviewing this again for the next release. I think we'll have more time now to review it and make progress.

With DB I find providing both ALLOW and DENY would be helpful. Possibly extend it to other resources in a separate PR.

This is inconsistent with the rest of the ACL system. Today everything is positive grants. Lots of people think that if you do -get +@string, the negation of get overrules the adding of the string category. That isn't correct, as you will still have access to get. Having allows and denies for databases will just add on to that confusion. If we want to have negative policies, we should have a more generic solution. I'm fine with another issue for this though.

For FLUSHALL and FLUSHDB the command can get accepted as ASYNC and while it's getting executed the permission could change. I think the correct behavior is to still process the command completely even if the permission has changed at a later point. (The PR has the correct behavior).

From the end user perspective, the FLUSH always happens synchronously. The data is just freed async. I think this behavior is fine.

All database access: ACL SETUSER alice ^* Selective database access: ACL SETUSER bob ^1 ^2

I think this is less readable than the original proposal, but it is more consistent. I don't know how strongly I feel.

I see where you're coming from in terms of one token per symbol. Do you think originally proposed syntax (db= / db!=) is worse than one token per symbol? For me explicit list seemed like a better idea in terms of specifying multiple selectors because it provided better readability and seemed more compact.
Also about ALLOW and DENY ACL: I suggest in this PR we add only ALLOW policies and as a separate PR I will add DENY policy for different rules and from that point we will think about usefulness of each DENY policy.

- 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 <[email protected]>

Signed-off-by: Daniil Kashapov <[email protected]>

dvkashapov · 2025-10-21T12:25:26Z

@madolson @hpatro Hello! It would be awesome to merge this in 9.1!
I think I will rewrite to ACL SETUSER alice ^dbid this week. There's some talks about named databases and in that case something like ACL SETUSER alice ^db_name1 ^db_name2 looks better than ACL SETUSER bob db=db_name1,db_name2, what do you think?

madolson · 2025-10-27T14:54:58Z

In the discussion, it came up that we are missing the following other database features:

No database listed for ACL LOG
No database listed for MONITOR, you can parse this and track which is annoying
No database listed for COMMAND LOG
Monitor will show information for all databases. (Maybe fine)
Does FLUSHALL need all database access?
Do we need a DBLIST operation?
There was an ask to split pub/sub based off of databases, [NEW] Add complete isolation between databases (pub/sub) #1868.
Add support for named databases [NEW] Introduce database logical names #1601

codecov · 2025-10-27T15:13:59Z

Codecov Report

❌ Patch coverage is 95.02262% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.79%. Comparing base (b7a7ef8) to head (e7907a5).
⚠️ Report is 5 commits behind head on unstable.

Files with missing lines	Patch %	Lines
src/module.c	61.53%	10 Missing ⚠️
src/acl.c	99.23%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #2309      +/-   ##
============================================
- Coverage     73.84%   73.79%   -0.05%     
============================================
  Files           125      125              
  Lines         68898    69345     +447     
============================================
+ Hits          50878    51174     +296     
- Misses        18020    18171     +151

Files with missing lines	Coverage Δ
src/commands.def	`100.00% <ø> (ø)`
src/db.c	`93.28% <100.00%> (+0.21%)`	⬆️
src/intset.c	`100.00% <100.00%> (ø)`
src/multi.c	`96.98% <100.00%> (+0.11%)`	⬆️
src/server.c	`88.73% <100.00%> (+<0.01%)`	⬆️
src/server.h	`100.00% <ø> (ø)`
src/sort.c	`94.82% <100.00%> (+0.01%)`	⬆️
src/acl.c	`92.61% <99.23%> (+0.52%)`	⬆️
src/module.c	`25.51% <61.53%> (+0.02%)`	⬆️

... and 14 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

madolson · 2025-10-27T15:20:04Z

The use cases we need to have clear syntax for.

User can add a databse to their existing supported databases

# Add DB 1 to user1s permissions
ACLSETUER user1 db+=1

User can remove a database from their accessible databases - Do we need to support this usecase?

ACLSETUER user1 db-=1,2,3

User can override all of their existing databases with new databases

ACLSETUER user1 resetdbs db+=1,2,3,4

@hpatro Will followup to try to help clarify the most consistent way to implement the syntax.

There is one more open question, do we want to make the default secure by default. For Redis 7, we changed the default permissions of an ACL user from allchannels to nochannels. Do we want to do the same thing here?

dvkashapov · 2025-10-27T16:17:26Z

Awesome! This is really descriptive and answers a lot of questions. Proposed syntax is good, I'm OK with it! Answering your first message:

ACL LOG, MONITOR, COMMANDLOG etc.

We can make an assumption that @admin commands need all database access to execute, seems to cover those cases but may be unintuitive, in this case we can return error with helpful message.

Monitor will show information for all databases

I will take a look at MONITOR implementation, maybe we can make it per-db with default of all databases.

Does FLUSHALL need all database access

Yes, FLUSHALL needs all database access.

Do we need a DBLIST operation?

DBLIST would be useful when we will have named databases, yes.

There was an ask to split pub/sub based off of databases

I don't know much about channels right now, so I can't say how hard it would be to make them per db.

Add support for named databases

In terms of ACL for named databases, if we decide to make them like an alias for some dbid, then implementation won't change much. We would only need to resolve name to dbid.

Do we want to make the default secure by default?

This will be good for security but it will be a breaking change for all existing ACL's for all users, maybe this is too much.

Important question: We're going with per-user permissions, not per-selector like currently in PR?

hpatro · 2025-10-27T18:02:15Z

We were just trying to think through in the weekly meeting what other gaps exists around databases support. @dvkashapov doesn't need to be worked upon regarding this change.

Important question: We're going with per-user permissions, not per-selector like currently in PR?

It's still per-selector, maybe the example suggested above brought in some ambiguity.

allenss-amazon · 2025-10-28T00:02:47Z

I started an issue for dealing with ACL issues for search module. #2764

Seems like this work intersects with that, in particular, the current search code would not enforce these ACL capabilities and would cause security issues.

dvkashapov · 2025-10-28T09:04:15Z

still per-selector

But then db+=1,2,3 would only apply to root selector, and will not be useful for user with >1 selector, same for db-=1,2,3 case. Do we want to give users ability to edit individual selector? Then we would need to identify them with some id's and add ACL LIST-SELECTORS user.

hpatro · 2025-10-28T17:13:39Z

still per-selector

But then db+=1,2,3 would only apply to root selector, and will not be useful for user with >1 selector, same for db-=1,2,3 case. Do we want to give users ability to edit individual selector?

That has been the case with all other restrictions (commands / categories / channels). We allow the operation even if one selector succeeds.

dvkashapov · 2025-10-29T06:21:56Z

OK, then I suggest we focus on db+=1,2,3 syntax, and while I do that - lets discuss db-=1,2,3, are we targeting to add more negative ACL's in future releases? If there's no request from community then maybe this is not needed?

Signed-off-by: Daniil Kashapov <[email protected]>

hpatro

LGTM.

@dvkashapov could you benchmark with certain db permissions added to the default user and measure the performance change for SET/GET commands? I assume it will be a minimal drop in rps.

tests/unit/moduleapi/aclcheck.tcl

src/intset.c

src/acl.c

src/commands/move.json

src/commands/cluster-getkeysinslot.json

src/commands/README.md

madolson · 2025-12-20T00:38:12Z

src/acl.c

    }
 }

 sds getAclErrorMessage(int acl_res, user *user, struct serverCommand *cmd, sds errored_val, int verbose) {


No, it was to avoid a breaking change at some point. The dry-run needed to give some more information, but we didn't want to change the response.

Signed-off-by: Daniil Kashapov <[email protected]>

dvkashapov · 2025-12-20T14:41:42Z

I appreciate the time and input from everyone who reviewed this, thank you! I addressed every review suggestions, PTAL.

@hpatro @madolson while merging changes from unstable I discovered one problem: Lua now uses only ValkeyModule API and we wouldn't be able to do extraction of dbid from client, previously in script_lua.c we did something like this:
int dbid = (ctx->client->flag.multi) ? ctx->client->mstate->transaction_db_id : ctx->client->db->id;
But now we have to use public API, which is why I had to modify int VM_GetSelectedDb(ValkeyModuleCtx *ctx);:

/* Return the currently selected DB.
 * When inside a MULTI/EXEC transaction, returns transaction_db_id which tracks
 * the DB selected within the transaction (may differ from client->db->id).
 * Otherwise, returns the client's actual DB. */
int VM_GetSelectedDb(ValkeyModuleCtx *ctx) {
    return (ctx->client->flag.multi) ? ctx->client->mstate->transaction_db_id : ctx->client->db->id;
}

Can we do that to preserve correct ACL checks inside scripts?

Could you benchmark with certain db permissions added to the default user and measure the performance change for SET/GET commands? I assume it will be a minimal drop in rps.

Definitely, I will do the benchmarking early next week!

madolson

The code looks really good. I looked more deeply into a few things and have some minor suggestions, other than that it looks good!

tests/unit/acl-v2.tcl

src/commands/cluster-countkeysinslot.json

src/module.c

src/commands/migrate.json

src/acl.c

madolson · 2025-12-20T15:22:35Z

Can we do that to preserve correct ACL checks inside scripts?

My initial reaction is that seems fine.

Signed-off-by: Daniil Kashapov <[email protected]>

dvkashapov · 2025-12-22T12:28:54Z

Benchmarks!

To exaggerate the situation I used small data sizes, databases 128 and user that has explicit list of 128 databases so each access check needs to do log2(128)=7 comparisons.
TLDR: Even in this situation the performance differences between the two setups are negligible and well within typical benchmark variance.

Configs and benchmarks

AMD Zen4 x86 32vCPU 64gb RAM
Config

databases 128
save ""
appendonly no
rdbcompression no
activedefrag no
maxclients 1000
io-threads (1 or 9)
protected-mode no
hz 10
maxmemory 3gb

valkey is pinned to even vCPUs for less contention between io-threads
Benchmarks

taskset -c 16-31 ./src/valkey-benchmark --duration 180 -r 3000000 -d 16 -P 1 -c 384 -t SET --threads 96 --warmup 30

taskset -c 16-31 ./src/valkey-benchmark --duration 180 -r 3000000 -d 16 -P 1 -c 384 -t GET --threads 96 --warmup 30

taskset -c 16-31 ./src/valkey-benchmark --duration 180 -r 3000000 -d 16 -P 10 -c 384 -t SET --threads 96 --warmup 30

taskset -c 16-31 ./src/valkey-benchmark --duration 180 -r 3000000 -d 16 -P 10 -c 384 -t GET --threads 96 --warmup 30

Results DB LEVEL ACL with 128 explicit databases list

DB LEVEL ACL (user with explicit db list of 128 database IDs)

SET IO 1 P 1 D 16

Summary:
throughput summary: 62701.67 requests per second
latency summary (msec):
        avg       min       p50       p95       p99       max
      6.105     5.480     6.055     6.199     7.439    16.863

GET IO 1 P 1 D 16

Summary:
throughput summary: 63302.68 requests per second
latency summary (msec):
        avg       min       p50       p95       p99       max
      6.046     5.008     5.999     6.135     6.231    12.527

SET IO 1 P 10 D 16

Summary:
throughput summary: 472118.09 requests per second
latency summary (msec):
        avg       min       p50       p95       p99       max
      8.111     7.264     8.063     8.215     8.303    16.735

GET IO 1 P 10 D 16

Summary:
throughput summary: 518410.44 requests per second
latency summary (msec):
        avg       min       p50       p95       p99       max
      7.386     6.608     7.343     7.503     7.599    15.359

SET IO 9 P 1 D 16

Summary:
throughput summary: 467143.03 requests per second
latency summary (msec):
        avg       min       p50       p95       p99       max
      0.798     0.024     0.535     4.263     4.943    33.919

GET IO 9 P 1 D 16

Summary:
throughput summary: 464477.75 requests per second
latency summary (msec):
        avg       min       p50       p95       p99       max
      0.802     0.016     0.535     4.319     4.999    34.303

SET IO 9 P 10 D 16

Summary:
throughput summary: 1345698.00 requests per second
latency summary (msec):
        avg       min       p50       p95       p99       max
      2.839     0.032     1.999     6.231    10.783    39.551

GET IO 9 P 10 D 16

Summary:
throughput summary: 1975641.75 requests per second
latency summary (msec):
        avg       min       p50       p95       p99       max
      1.928     0.040     1.399     5.511     7.519    38.943

Results UNSTABLE with default user

UNSTABLE (user with default permissions)

SET IO 1 P 1 D 16

Summary:
throughput summary: 61326.57 requests per second
latency summary (msec):
        avg       min       p50       p95       p99       max
      6.244     5.336     6.199     6.375     7.623    17.503

GET IO 1 P 1 D 16

Summary:
throughput summary: 65044.25 requests per second
latency summary (msec):
        avg       min       p50       p95       p99       max
      5.884     5.240     5.855     5.983     6.063    12.199

SET IO 1 P 10 D 16

Summary:
throughput summary: 476723.19 requests per second
latency summary (msec):
        avg       min       p50       p95       p99       max
      8.034     7.008     7.983     8.111     8.207    17.647

GET IO 1 P 10 D 16

Summary:
throughput summary: 521056.66 requests per second
latency summary (msec):
        avg       min       p50       p95       p99       max
      7.348     6.552     7.303     7.471     7.567    15.175

SET IO 9 P 1 D 16

Summary:
throughput summary: 470560.84 requests per second
latency summary (msec):
        avg       min       p50       p95       p99       max
      0.792     0.024     0.527     4.263     4.911    32.671

GET IO 9 P 1 D 16

Summary:
throughput summary: 452110.12 requests per second
latency summary (msec):
        avg       min       p50       p95       p99       max
      0.824     0.024     0.527     4.383     6.799    37.951

SET IO 9 P 10 D 16

Summary:
throughput summary: 1340728.38 requests per second
latency summary (msec):
        avg       min       p50       p95       p99       max
      2.849     0.048     1.983     6.231    10.895    39.007

GET IO 9 P 10 D 16

Summary:
throughput summary: 2006724.50 requests per second
latency summary (msec):
        avg       min       p50       p95       p99       max
      1.898     0.040     1.391     5.487     6.975    35.583

Test Case	Throughput Difference	Latency Difference
SET IO 1 P 1	+2.2% (ACL)	-2.3% (ACL better)
GET IO 1 P 1	-2.7% (ACL)	+2.8% (ACL worse)
SET IO 1 P 10	-1.0% (ACL)	+1.0% (ACL worse)
GET IO 1 P 10	-0.5% (ACL)	+0.5% (ACL worse)
SET IO 9 P 1	-0.7% (ACL)	+0.8% (ACL worse)
GET IO 9 P 1	+2.7% (ACL)	-2.7% (ACL better)
SET IO 9 P 10	+0.4% (ACL)	-0.4% (ACL better)
GET IO 9 P 10	-1.6% (ACL)	+1.6% (ACL worse)

hpatro · 2025-12-22T16:35:10Z

Benchmarking seems good to me.

@madolson / @zuiderkwast I think we are good to merge this out. Any final thoughts ?

madolson · 2025-12-22T17:45:03Z

I think we are good to merge this out. Any final thoughts ?

The dbnum issue still seems open?

hpatro · 2025-12-22T20:41:28Z

I think we are good to merge this out. Any final thoughts ?

The dbnum issue still seems open?

Like I mentioned in the thread and offline, we can stick with the current approach and can relax the validation if users ask for it.

hpatro · 2025-12-22T23:42:23Z

Discussed the upper bound dbnum issue further with @madolson. It makes devs lives easier to keep it open. This allows ACL rules to be used across clusters with different dbnum value. Hence, we should remove the restriction for ACL rule to follow server.dbnum and keep these two decoupled.

Signed-off-by: Daniil Kashapov <[email protected]>

dvkashapov · 2025-12-23T02:55:45Z

Replaced check with dbid > INT32_MAX, so we chan rule out definitely incorrect configurations as dbnum max value is INT32_MAX.

hpatro · 2025-12-23T03:36:20Z

@dvkashapov Could you submit a PR to update the ACL topic page https://valkey.io/topics/acl with this change?

dvkashapov · 2025-12-23T04:05:56Z

Thanks everyone for making this happen!

Could you submit a PR to update the ACL topic page https://valkey.io/topics/acl with this change?

Yes, definitely.

@Write

## API changes and user behavior: - [x] Default behavior for database access. Default is `alldbs` permissions. ### Database Permissions (`db=`) - [x] Accessing particular database ``` > ACL SETUSER test1 on +@ALL ~* resetdbs db=0,1 nopass "user test1 on nopass sanitize-payload ~* resetchannels db=0,1 +@ALL" ``` - [x] (Same behavior without usage of `resetdbs`) ``` > ACL SETUSER test1 on +@ALL ~* db=0,1 nopass "user test1 on nopass sanitize-payload ~* resetchannels db=0,1 +@ALL" ``` - [x] Multiple selector can be provided ``` > ACL SETUSER test1 on nopass (db=0,1 +@Write +select ~*) (db=2,3 +@READ +select ~*) "user test1 on nopass sanitize-payload resetchannels alldbs -@ALL (~* resetchannels db=0,1 -@ALL +@Write +select) (~* resetchannels db=2,3 -@ALL +@READ +select)" ``` - [x] Restricting special commands which access databases as part of the command. The user needs to have access to both the commands and db(s) part of the command to run these commands. 1. SWAPDB 2. SELECT 3. MOVE - (Select command would have went through for the source database). Have access for the target database. 4. COPY - [x] Restricting special commands which doesn't specify database number, however, accesses multiple databases. The user needs to have access to both the commands and all databases (`alldbs`) to run these commands. 1. FLUSHALL - Access all databases 2. CLUSTER commands that access all databases: - CANCELSLOTMIGRATIONS - MIGRATESLOTS - [x] New connection establishment behavior New client connection gets established to DB 0 by default. Authentication and authorisation are decoupled and the user can connect/authenticate and further perform `SELECT` or other operation that do not access keyspace. (Do we want to extend HELLO?) Alternative suggestion by @madolson: Extend `HELLO` command to pass the dbid to which the user should get connected after authentication if they have right set of permission. I think it will become a long poll for adoption. - [x] Observability Extend `ACL LOG` to log user which received denied permission error while accessing a database. - [x] Module API * Introduce module API `int VM_ACLCheckPermissions(ValkeyModuleUser *user, ValkeyModuleString **argv, int argc, int dbid, ValkeyModuleACLLogEntryReason *denial_reason);` * Stop support of `VM_ACLCheckCommandPermissions()`. Resolves: valkey-io#1336 --------- Signed-off-by: Daniil Kashapov <[email protected]>

@Write

## API changes and user behavior: - [x] Default behavior for database access. Default is `alldbs` permissions. ### Database Permissions (`db=`) - [x] Accessing particular database ``` > ACL SETUSER test1 on +@ALL ~* resetdbs db=0,1 nopass "user test1 on nopass sanitize-payload ~* resetchannels db=0,1 +@ALL" ``` - [x] (Same behavior without usage of `resetdbs`) ``` > ACL SETUSER test1 on +@ALL ~* db=0,1 nopass "user test1 on nopass sanitize-payload ~* resetchannels db=0,1 +@ALL" ``` - [x] Multiple selector can be provided ``` > ACL SETUSER test1 on nopass (db=0,1 +@Write +select ~*) (db=2,3 +@READ +select ~*) "user test1 on nopass sanitize-payload resetchannels alldbs -@ALL (~* resetchannels db=0,1 -@ALL +@Write +select) (~* resetchannels db=2,3 -@ALL +@READ +select)" ``` - [x] Restricting special commands which access databases as part of the command. The user needs to have access to both the commands and db(s) part of the command to run these commands. 1. SWAPDB 2. SELECT 3. MOVE - (Select command would have went through for the source database). Have access for the target database. 4. COPY - [x] Restricting special commands which doesn't specify database number, however, accesses multiple databases. The user needs to have access to both the commands and all databases (`alldbs`) to run these commands. 1. FLUSHALL - Access all databases 2. CLUSTER commands that access all databases: - CANCELSLOTMIGRATIONS - MIGRATESLOTS - [x] New connection establishment behavior New client connection gets established to DB 0 by default. Authentication and authorisation are decoupled and the user can connect/authenticate and further perform `SELECT` or other operation that do not access keyspace. (Do we want to extend HELLO?) Alternative suggestion by @madolson: Extend `HELLO` command to pass the dbid to which the user should get connected after authentication if they have right set of permission. I think it will become a long poll for adoption. - [x] Observability Extend `ACL LOG` to log user which received denied permission error while accessing a database. - [x] Module API * Introduce module API `int VM_ACLCheckPermissions(ValkeyModuleUser *user, ValkeyModuleString **argv, int argc, int dbid, ValkeyModuleACLLogEntryReason *denial_reason);` * Stop support of `VM_ACLCheckCommandPermissions()`. Resolves: valkey-io#1336 --------- Signed-off-by: Daniil Kashapov <[email protected]>

At one point in development there was assumption that intset of database IDs would have IDs only in `[0, server.dbnum-1]` range, but [here](#2309 (comment)) we decided to change upper bound to `INT32_MAX` for ACL compatibility reasons between nodes. Because of that change, assumption is not true anymore and we should explicitly check each database list to contain access to full `[0, server.dbnum-1]` range. Also added test for that. Signed-off-by: Daniil Kashapov <[email protected]>

At one point in development there was assumption that intset of database IDs would have IDs only in `[0, server.dbnum-1]` range, but [here](valkey-io#2309 (comment)) we decided to change upper bound to `INT32_MAX` for ACL compatibility reasons between nodes. Because of that change, assumption is not true anymore and we should explicitly check each database list to contain access to full `[0, server.dbnum-1]` range. Also added test for that. Signed-off-by: Daniil Kashapov <[email protected]>

hwware added the major-decision-pending Major decision pending by TSC team label Jul 4, 2025

madolson reviewed Jul 7, 2025

View reviewed changes

src/acl.c Outdated Show resolved Hide resolved

src/module.c Outdated Show resolved Hide resolved

src/multi.c Outdated Show resolved Hide resolved

dvkashapov marked this pull request as ready for review July 14, 2025 14:22

madolson requested a review from hpatro July 14, 2025 14:54

dvkashapov mentioned this pull request Jul 15, 2025

valkey-cli: Fix dbid reset after error in MULTI #2357

Closed

hpatro reviewed Jul 31, 2025

View reviewed changes

madolson added this to Valkey 9.1 Aug 4, 2025

madolson moved this to Optional for next release candidate in Valkey 9.1 Aug 4, 2025

madolson moved this from Optional for next release candidate to Todo in Valkey 9.1 Aug 6, 2025

dvkashapov mentioned this pull request Oct 6, 2025

Add ValkeyModule_ACLCheckCommandPermissionsForCurrentUser API #2690

Closed

dvkashapov added 3 commits October 21, 2025 16:38

Fix selector flag assignment and apply clang-format

97a27c5

Signed-off-by: Daniil Kashapov <[email protected]>

Extend serverCommand with get_dbid_args, unify ACL checks

b2fd61a

Signed-off-by: Daniil Kashapov <[email protected]>

dvkashapov force-pushed the unstable branch from e3eca62 to b2fd61a Compare October 21, 2025 11:46

Run generate commands

b4adb23

Signed-off-by: Daniil Kashapov <[email protected]>

hpatro approved these changes Dec 19, 2025

View reviewed changes

tests/unit/moduleapi/aclcheck.tcl Outdated Show resolved Hide resolved

src/intset.c Outdated Show resolved Hide resolved

src/acl.c Outdated Show resolved Hide resolved

src/acl.c Show resolved Hide resolved

src/acl.c Show resolved Hide resolved

src/acl.c Show resolved Hide resolved

madolson reviewed Dec 20, 2025

View reviewed changes

dvkashapov added 3 commits December 20, 2025 15:45

Apply review suggestions

8ce4a75

Signed-off-by: Daniil Kashapov <[email protected]>

Merge remote-tracking branch 'upstream/unstable' into unstable

41703e4

Signed-off-by: Daniil Kashapov <[email protected]>

Update lua and module api to use new VM_ACL check

1411b8a

Signed-off-by: Daniil Kashapov <[email protected]>

madolson reviewed Dec 20, 2025

View reviewed changes

tests/unit/acl-v2.tcl Outdated Show resolved Hide resolved

src/commands/cluster-countkeysinslot.json Outdated Show resolved Hide resolved

src/module.c Show resolved Hide resolved

src/commands/migrate.json Outdated Show resolved Hide resolved

src/acl.c Outdated Show resolved Hide resolved

dvkashapov added 4 commits December 21, 2025 10:16

Merge remote-tracking branch 'upstream/unstable' into unstable

19ae1b7

Signed-off-by: Daniil Kashapov <[email protected]>

Apply review suggestions

755678a

Signed-off-by: Daniil Kashapov <[email protected]>

Del test for COUNTKEYSINSLOT

da84977

Signed-off-by: Daniil Kashapov <[email protected]>

Delete get_dbid_args for MIGRATE

5666547

Signed-off-by: Daniil Kashapov <[email protected]>

Del dbid >= server.dbnum

e7907a5

Signed-off-by: Daniil Kashapov <[email protected]>

hpatro added the needs-doc-pr This change needs to update a documentation page. Remove label once doc PR is open. label Dec 23, 2025

hpatro merged commit 122070c into valkey-io:unstable Dec 23, 2025
57 checks passed

github-project-automation bot moved this from In Progress to Done in Valkey 9.1 Dec 23, 2025

enjoy-binbin added the release-notes This issue should get a line item in the release notes label Dec 23, 2025

dvkashapov mentioned this pull request Feb 16, 2026

Fix database ACL check for alldbs commands #3215

Merged

Conversation

dvkashapov commented Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

API changes and user behavior:

Database Permissions (db=)

Uh oh!

dvkashapov commented Jul 4, 2025

Uh oh!

hwware commented Jul 4, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

madolson commented Jul 14, 2025

Uh oh!

hpatro left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dvkashapov commented Aug 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

madolson commented Sep 3, 2025

Uh oh!

dvkashapov commented Oct 21, 2025

Uh oh!

madolson commented Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

madolson commented Oct 27, 2025

Uh oh!

dvkashapov commented Oct 27, 2025

Uh oh!

hpatro commented Oct 27, 2025

Uh oh!

allenss-amazon commented Oct 28, 2025

Uh oh!

dvkashapov commented Oct 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hpatro commented Oct 28, 2025

Uh oh!

dvkashapov commented Oct 29, 2025

Uh oh!

hpatro left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

madolson Dec 20, 2025

Choose a reason for hiding this comment

Uh oh!

dvkashapov commented Dec 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

madolson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dvkashapov commented Jul 4, 2025 •

edited

Loading

Database Permissions (`db=`)

dvkashapov commented Aug 1, 2025 •

edited

Loading

madolson commented Oct 27, 2025 •

edited

Loading

codecov bot commented Oct 27, 2025 •

edited

Loading

dvkashapov commented Oct 28, 2025 •

edited

Loading

dvkashapov commented Dec 20, 2025 •

edited

Loading

dvkashapov commented Dec 23, 2025 •

edited

Loading