Skip to content

Database-level access control#2309

Merged
hpatro merged 47 commits into
valkey-io:unstablefrom
dvkashapov:unstable
Dec 23, 2025
Merged

Database-level access control#2309
hpatro merged 47 commits into
valkey-io:unstablefrom
dvkashapov:unstable

Conversation

@dvkashapov

@dvkashapov dvkashapov commented Jul 4, 2025

Copy link
Copy Markdown
Member

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.

  1. SWAPDB
  2. SELECT
  3. MOVE - (Select command would have went through for the source database). Have access for the target database.
  4. 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.

  1. FLUSHALL - Access all databases
  2. 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

Copy link
Copy Markdown
Member Author

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

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

hwware commented Jul 4, 2025

Copy link
Copy Markdown
Contributor

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

Comment thread src/acl.c Outdated
Comment thread src/module.c Outdated
Comment thread src/multi.c Outdated
@dvkashapov dvkashapov marked this pull request as ready for review July 14, 2025 14:22
@madolson madolson requested a review from hpatro July 14, 2025 14:54
@madolson

Copy link
Copy Markdown
Member

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 hpatro left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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).

Comment thread src/acl.c
Comment thread src/acl.c Outdated
Comment thread src/acl.c Outdated
Comment thread src/acl.c Outdated
Comment thread src/acl.c Outdated
Comment thread src/acl.c Outdated
Comment thread src/acl.c Outdated
@dvkashapov

dvkashapov commented Aug 1, 2025

Copy link
Copy Markdown
Member Author

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 madolson moved this to Optional for next release candidate in Valkey 9.1 Aug 4, 2025
@madolson madolson moved this from Optional for next release candidate to Todo in Valkey 9.1 Aug 6, 2025
@madolson

madolson commented Sep 3, 2025

Copy link
Copy Markdown
Member

@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 <daniil.kashapov.ykt@gmail.com>
Signed-off-by: Daniil Kashapov <daniil.kashapov.ykt@gmail.com>
Signed-off-by: Daniil Kashapov <daniil.kashapov.ykt@gmail.com>
@dvkashapov

Copy link
Copy Markdown
Member Author

@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

madolson commented Oct 27, 2025

Copy link
Copy Markdown
Member

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

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

@codecov

codecov Bot commented Oct 27, 2025

Copy link
Copy Markdown

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

Copy link
Copy Markdown
Member

The use cases we need to have clear syntax for.

  1. User can add a databse to their existing supported databases
# Add DB 1 to user1s permissions
ACLSETUER user1 db+=1
  1. User can remove a database from their accessible databases - Do we need to support this usecase?
ACLSETUER user1 db-=1,2,3
  1. 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

Copy link
Copy Markdown
Member Author

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

hpatro commented Oct 27, 2025

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Member

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

dvkashapov commented Oct 28, 2025

Copy link
Copy Markdown
Member Author

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

hpatro commented Oct 28, 2025

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Member Author

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 <daniil.kashapov.ykt@gmail.com>
zuiderkwast pushed a commit that referenced this pull request Feb 16, 2026
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 <daniil.kashapov.ykt@gmail.com>
harrylin98 pushed a commit to harrylin98/valkey_forked that referenced this pull request Feb 19, 2026
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 <daniil.kashapov.ykt@gmail.com>
hpatro pushed a commit to hpatro/valkey that referenced this pull request Mar 5, 2026
## 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 <daniil.kashapov.ykt@gmail.com>
Signed-off-by: Harkrishn Patro <bunty.hari@gmail.com>
hpatro pushed a commit to hpatro/valkey that referenced this pull request Mar 5, 2026
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 <daniil.kashapov.ykt@gmail.com>
Signed-off-by: Harkrishn Patro <bunty.hari@gmail.com>
@dvkashapov dvkashapov removed the needs-doc-pr This change needs to update a documentation page. Remove label once doc PR is open. label May 14, 2026
enjoy-binbin added a commit to enjoy-binbin/valkey that referenced this pull request May 22, 2026
The db=, alldbs and resetdbs rules introduced in valkey-io#2309 were
missing from the ACL self-documenting block in valkey.conf.
Also update the `reset` description to include alldbs and
sanitize-payload, and add ERANGE to the documented errno values.

Signed-off-by: Binbin <binloveplay1314@qq.com>
enjoy-binbin added a commit that referenced this pull request May 23, 2026
In #2309, we ultimately decided not to set `get_dbid_args` for
MIGRATE command and removed the relevant code. But, we missed this
one, just cleanup the dead code.

Signed-off-by: Binbin <binloveplay1314@qq.com>
enjoy-binbin added a commit that referenced this pull request May 23, 2026
The db=, alldbs and resetdbs rules introduced in #2309 were
missing from the ACL self-documenting block in valkey.conf.
Also update the `reset` description to include alldbs and
sanitize-payload, and add ERANGE to the documented errno values.

Signed-off-by: Binbin <binloveplay1314@qq.com>
Co-authored-by: Daniil Kashapov <daniil.kashapov.ykt@gmail.com>
zuiderkwast pushed a commit that referenced this pull request May 31, 2026
CLUSTER FLUSHSLOT internally calls delKeysInSlot() which iterates
over all databases and removes the slot's keys from every database,
just like FLUSHALL command will touch all the databases. And it was
missing the ALL_DBS command flag introduced in #2309.

Add test to cover this case, also do a extra cleanup, replace the
manual start_multiple_servers with a single start_cluster call.

Signed-off-by: Binbin <binloveplay1314@qq.com>
valkeyrie-ops Bot pushed a commit that referenced this pull request Jun 1, 2026
CLUSTER FLUSHSLOT internally calls delKeysInSlot() which iterates
over all databases and removes the slot's keys from every database,
just like FLUSHALL command will touch all the databases. And it was
missing the ALL_DBS command flag introduced in #2309.

Add test to cover this case, also do a extra cleanup, replace the
manual start_multiple_servers with a single start_cluster call.

Signed-off-by: Binbin <binloveplay1314@qq.com>
enjoy-binbin added a commit that referenced this pull request Jun 2, 2026
copyDbIdArgs assumed the DB token was always at argv[3], so
'COPY src dst REPLACE DB n' or 'COPY src dst DB a DB b' could
bypass db= restrictions. Mirror copyCommand's parser to walk
all argv tokens, collect every DB destination, and let the ACL
checker validate each one.

DB ACL was added in #2309.

Signed-off-by: Binbin <binloveplay1314@qq.com>
eifrah-aws pushed a commit to eifrah-aws/valkey that referenced this pull request Jun 2, 2026
…io#3801)

copyDbIdArgs assumed the DB token was always at argv[3], so
'COPY src dst REPLACE DB n' or 'COPY src dst DB a DB b' could
bypass db= restrictions. Mirror copyCommand's parser to walk
all argv tokens, collect every DB destination, and let the ACL
checker validate each one.

DB ACL was added in valkey-io#2309.

Signed-off-by: Binbin <binloveplay1314@qq.com>
valkeyrie-ops Bot pushed a commit that referenced this pull request Jun 3, 2026
copyDbIdArgs assumed the DB token was always at argv[3], so
'COPY src dst REPLACE DB n' or 'COPY src dst DB a DB b' could
bypass db= restrictions. Mirror copyCommand's parser to walk
all argv tokens, collect every DB destination, and let the ACL
checker validate each one.

DB ACL was added in #2309.

Signed-off-by: Binbin <binloveplay1314@qq.com>
bandalgomsu pushed a commit to bandalgomsu/valkey that referenced this pull request Jun 4, 2026
…io#3801)

copyDbIdArgs assumed the DB token was always at argv[3], so
'COPY src dst REPLACE DB n' or 'COPY src dst DB a DB b' could
bypass db= restrictions. Mirror copyCommand's parser to walk
all argv tokens, collect every DB destination, and let the ACL
checker validate each one.

DB ACL was added in valkey-io#2309.

Signed-off-by: Binbin <binloveplay1314@qq.com>
enjoy-binbin added a commit that referenced this pull request Jun 8, 2026
Rework commandDbIdArgs so each helper returns the argv index of every dbid
argument it owns. ACLSelectorCheckCmd uses those positions to set keyidxptr
directly, replacing the cmd->proc-based offset table. The caller now reads
the dbid value via getLongLongFromObject(argv[positions[i]], ...). New
db=-checked commands only need to implement their own *DbIdArgs helper.

The fix is for COPY: the old chain fell through to 0, so the "object"
field in ACL LOG was always showed "copy", it now reports the first
denied dbid.

Based on #3801, also see it for more details, DB ACL was added in #2309.

Signed-off-by: Binbin <binloveplay1314@qq.com>
valkeyrie-ops Bot pushed a commit that referenced this pull request Jun 9, 2026
Rework commandDbIdArgs so each helper returns the argv index of every dbid
argument it owns. ACLSelectorCheckCmd uses those positions to set keyidxptr
directly, replacing the cmd->proc-based offset table. The caller now reads
the dbid value via getLongLongFromObject(argv[positions[i]], ...). New
db=-checked commands only need to implement their own *DbIdArgs helper.

The fix is for COPY: the old chain fell through to 0, so the "object"
field in ACL LOG was always showed "copy", it now reports the first
denied dbid.

Based on #3801, also see it for more details, DB ACL was added in #2309.

Signed-off-by: Binbin <binloveplay1314@qq.com>
valkeyrie-ops Bot pushed a commit that referenced this pull request Jun 10, 2026
CLUSTER FLUSHSLOT internally calls delKeysInSlot() which iterates
over all databases and removes the slot's keys from every database,
just like FLUSHALL command will touch all the databases. And it was
missing the ALL_DBS command flag introduced in #2309.

Add test to cover this case, also do a extra cleanup, replace the
manual start_multiple_servers with a single start_cluster call.

Signed-off-by: Binbin <binloveplay1314@qq.com>
valkeyrie-ops Bot pushed a commit that referenced this pull request Jun 10, 2026
copyDbIdArgs assumed the DB token was always at argv[3], so
'COPY src dst REPLACE DB n' or 'COPY src dst DB a DB b' could
bypass db= restrictions. Mirror copyCommand's parser to walk
all argv tokens, collect every DB destination, and let the ACL
checker validate each one.

DB ACL was added in #2309.

Signed-off-by: Binbin <binloveplay1314@qq.com>
valkeyrie-ops Bot pushed a commit that referenced this pull request Jun 10, 2026
Rework commandDbIdArgs so each helper returns the argv index of every dbid
argument it owns. ACLSelectorCheckCmd uses those positions to set keyidxptr
directly, replacing the cmd->proc-based offset table. The caller now reads
the dbid value via getLongLongFromObject(argv[positions[i]], ...). New
db=-checked commands only need to implement their own *DbIdArgs helper.

The fix is for COPY: the old chain fell through to 0, so the "object"
field in ACL LOG was always showed "copy", it now reports the first
denied dbid.

Based on #3801, also see it for more details, DB ACL was added in #2309.

Signed-off-by: Binbin <binloveplay1314@qq.com>
madolson pushed a commit that referenced this pull request Jun 10, 2026
#3964)

Database-level ACL #2309 introduced `alldbs` rule that was explicit for
all users and because of that previous versions no longer had the
ability to parse ACL strings produced by later versions.
Omit `alldbs` in `ACLDescribeSelector()`, that is used in `ACL
SAVE/LOAD` and `CONFIG REWRITE` command paths so that downgrades would
be possible if new feature was not used (`db=` and `resetdbs` rules).
Keep `ACL GETUSER` command's output as is and return `alldbs` in
`databases` field because of command's field-value format.
Add test to check that `ACL LIST` omits implicit `alldbs` and add check
to existing `ACL SAVE` and `CONFIG REWRITE` tests.

Fixes #3915

Signed-off-by: Daniil Kashapov <daniil.kashapov.ykt@gmail.com>
sarthakaggarwal97 added a commit to sarthakaggarwal97/valkey that referenced this pull request Jun 24, 2026
…ey-io#3888) (#345)

Rework commandDbIdArgs so each helper returns the argv index of every dbid
argument it owns. ACLSelectorCheckCmd uses those positions to set keyidxptr
directly, replacing the cmd->proc-based offset table. The caller now reads
the dbid value via getLongLongFromObject(argv[positions[i]], ...). New
db=-checked commands only need to implement their own *DbIdArgs helper.

The fix is for COPY: the old chain fell through to 0, so the "object"
field in ACL LOG was always showed "copy", it now reports the first
denied dbid.

Based on valkey-io#3801, also see it for more details, DB ACL was added in valkey-io#2309.

Signed-off-by: Binbin <binloveplay1314@qq.com>
Co-authored-by: Binbin <binloveplay1314@qq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

major-decision-approved Major decision approved by TSC team release-notes This issue should get a line item in the release notes

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

[NEW] Support database level ACL

8 participants