-
Notifications
You must be signed in to change notification settings - Fork 955
Database-level access control #2309
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: unstable
Are you sure you want to change the base?
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.
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 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).
src/acl.c
Outdated
| if (keyidxptr) { | ||
| if (cmd->proc == selectCommand) | ||
| *keyidxptr = 1; | ||
| else if (cmd->proc == moveCommand) | ||
| *keyidxptr = 2; | ||
| else if (cmd->proc == swapdbCommand) | ||
| *keyidxptr = (i == 0) ? 1 : 2; | ||
| else | ||
| *keyidxptr = 0; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to handle flushdb here. This part of the code doesn't seem very readable. Let me think more and come up with something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed that this needs improvement, don't really have an idea for how to do it cleaner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to handle flushdb here
It is handled as a general case in last else if because FLUSHDB is related to current dbid
This part of the code doesn't seem very readable
Added todo here, still can't think of clean way to set keyidxptr
|
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 <[email protected]>
Signed-off-by: Daniil Kashapov <[email protected]>
Signed-off-by: Daniil Kashapov <[email protected]>
|
@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 72.45% 72.32% -0.13%
============================================
Files 128 128
Lines 70485 70674 +189
============================================
+ Hits 51068 51118 +50
- Misses 19417 19556 +139
🚀 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 <[email protected]>
|
@hpatro Thanks for the detailed points, I agree with them! However, I wonder if db level permissions configuration is necessary here? I suspect it might be a less common feature. Would love to hear your perspective on this. In regards of the expected behaviour: current implementation fits all the criteria's, but we still lack test for Also AFAIK FLUSHDB and SCAN operate on current db, so we don't need to handle them in special way right now. |
Signed-off-by: Daniil Kashapov <[email protected]>
Signed-off-by: Daniil Kashapov <[email protected]>
|
While writing test I noticed that for |
Signed-off-by: Daniil Kashapov <[email protected]>
|
@hpatro added Question: We need to handle db-level checks in |
|
@dvkashapov I think probably want to also include Also cluster-migrateslots should have the ALL_DBS flag as this is also a cover-all-dbs command right? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems mostly good on acl.c barring the resetdbs behavior and code performance on each ACL SETUSER operation.
Going through the remaining files.
src/acl.c
Outdated
| selector->flags &= ~SELECTOR_FLAG_ALLDBS; | ||
| } | ||
|
|
||
| char *dblist = zstrdup(dbs_str); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would suggest creating a sds and operate on it. improves strlen perf and more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rewrote to sds usage, PTAL
| } | ||
| } | ||
|
|
||
| sds getAclErrorMessage(int acl_res, user *user, struct serverCommand *cmd, sds errored_val, int verbose) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not particular for this PR, . Was it a security requirement behind introducing a non-verbose mode and hide the user/resource from the error response ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also thought that it would be useful for user to know..
My bad. You're right |
I also think fail-fast is better.
Not sure about the behavior of this command. Does it use the already authenticated user to perform the operation ? Does it fail if the user doesn't have access to some keys in the keyspace? If yes, we should mark it to have ALL_DBS access. |
Why would one use this check independently ?
There are two alternatives to deal with this.
I would recommend going with option 1. @allenss-amazon thoughts? |
|
@dvkashapov If you don't mind, I will update the top level comment with this #2309 (comment) as the checklist and other reviewers can signoff reading it and you can mark the checkbox as you finish implementing each of it. |
src/acl.c
Outdated
| if (errno == ERANGE || endptr == token || *endptr != '\0' || | ||
| dbid < 0 || dbid >= server.dbnum) { | ||
| return ACLDatabasePermissionError(new_dbs, dblist, EINVAL); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be better for user to understand what's incorrect in the command. WDYT ?
Current response is generic syntax error warning.
127.0.0.1:6379> config get databases
1) "databases"
2) "16"
127.0.0.1:6379> acl setuser hp db+=16
(error) ERR Error in ACL SETUSER modifier 'db+=16': Syntax error
127.0.0.1:6379> acl setuser hp db+=0
OK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, will add new errno in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems dangerous to set databases config to large value. The output buffer could be quite large and server processing time with setting acl setuser is also quite long with such high database count. This could be accounted as denial of service I believe.
% src/valkey-server --daemonize yes --logfile valkey.log --databases 100000000
% src/valkey-cli
127.0.0.1:6379> acl setuser hp on resetdbs db-=0
OK
(12.23s)
127.0.0.1:6379> acl list
<response too big and takes too long to get printed out>
|
Did a dry run locally. Few things which caught my eyes.
|
Signed-off-by: Daniil Kashapov <[email protected]>
dvkashapov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hpatro Applied some of review suggestions, will get to work on the rest of them, PTAL at my comments.
| } | ||
| } | ||
|
|
||
| sds getAclErrorMessage(int acl_res, user *user, struct serverCommand *cmd, sds errored_val, int verbose) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also thought that it would be useful for user to know..
src/acl.c
Outdated
| if (errno == ERANGE || endptr == token || *endptr != '\0' || | ||
| dbid < 0 || dbid >= server.dbnum) { | ||
| return ACLDatabasePermissionError(new_dbs, dblist, EINVAL); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, will add new errno in this case.
Signed-off-by: Daniil Kashapov <[email protected]>
I'm OK with first way, but I guess this is another major decision and we need feedback from community on that one. |
Signed-off-by: Daniil Kashapov <[email protected]>
|
Decisions from the core meeting:
|
Signed-off-by: Daniil Kashapov <[email protected]>
Signed-off-by: Daniil Kashapov <[email protected]>
Signed-off-by: Daniil Kashapov <[email protected]>
Signed-off-by: Daniil Kashapov <[email protected]>
Signed-off-by: Daniil Kashapov <[email protected]>
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
SELECToperation.(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
Without
ACL LOGsupport to flag denied permission for database, it will be difficult for user/administrator to identify malicious activity. So, I think we should extendACL LOGto log user which received denied permission error while accessing a database.Module API
VM_ACLCheckPermissions(ValkeyModuleUser *user, ValkeyModuleString **argv, int argc, int dbid);APIVM_ACLCheckCommandPermissions.Open Issues
#2309 (comment)
Resolves: #1336