Database-level access control#2309
Conversation
|
I would like to first discuss this new feature in the weekly meeting. Thanks |
|
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
left a comment
There was a problem hiding this comment.
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 ofdb=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
FLUSHALLandFLUSHDBthe 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).
|
Thanks for review @hpatro! |
|
@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.
This is inconsistent with the rest of the ACL system. Today everything is positive grants. Lots of people think that if you do
From the end user perspective, the FLUSH always happens synchronously. The data is just freed async. I think this behavior is fine.
I think this is less readable than the original proposal, but it is more consistent. I don't know how strongly I feel.
|
- 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>
|
@madolson @hpatro Hello! It would be awesome to merge this in 9.1! |
|
In the discussion, it came up that we are missing the following other database features:
|
Codecov Report❌ Patch coverage is
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
🚀 New features to boost your workflow:
|
|
The use cases we need to have clear syntax for.
@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? |
|
Awesome! This is really descriptive and answers a lot of questions. Proposed syntax is good, I'm OK with it! Answering your first message:
We can make an assumption that
I will take a look at MONITOR implementation, maybe we can make it per-db with default of all databases.
Yes, FLUSHALL needs all database access.
DBLIST would be useful when we will have named databases, yes.
I don't know much about channels right now, so I can't say how hard it would be to make them per db.
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.
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? |
|
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.
It's still per-selector, maybe the example suggested above brought in some ambiguity. |
|
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. |
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. |
That has been the case with all other restrictions (commands / categories / channels). We allow the operation even if one selector succeeds. |
|
OK, then I suggest we focus on |
Signed-off-by: Daniil Kashapov <daniil.kashapov.ykt@gmail.com>
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>
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>
## 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>
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>
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>
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>
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>
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>
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>
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>
…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>
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>
…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>
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>
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>
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>
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>
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>
#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>
…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>
API changes and user behavior:
Default is
alldbspermissions.Database Permissions (
db=)resetdbs)The user needs to have access to both the commands and db(s) part of the command to run these commands.
The user needs to have access to both the commands and all databases (
alldbs) to run these commands.New client connection gets established to DB 0 by default. Authentication and authorisation are decoupled and the user can connect/authenticate and further perform
SELECTor other operation that do not access keyspace.(Do we want to extend HELLO?) Alternative suggestion by @madolson: Extend
HELLOcommand 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 LOGto log user which received denied permission error while accessing a database.Module API
int VM_ACLCheckPermissions(ValkeyModuleUser *user, ValkeyModuleString **argv, int argc, int dbid, ValkeyModuleACLLogEntryReason *denial_reason);VM_ACLCheckCommandPermissions().Resolves: #1336