When the ACL lines are parsed, it looks like the commands and keys supplied are treated as global (with keys affecting all the commands that take keys). Just checking: is this the case, or am I missing something?
So for example, if I enter the following line: user biscuit on >password -@all +ping +auth +rpush ~one +lpop ~two when Redis saves it to the ACL file, it is reordered to user biscuit on >password ~one ~two -@all +rpush +ping +auth +lpop.
So my question is: is it possible to structure the ACLs to limit the keys available to a particular command? So achieving one ACL that has +rpush ~one and +lpop ~two ?
Or to achieve this do I need to create multiple users, which are resticted to a specific command and key/s combination?
Comment From: antirez
Hello @theDogOfPavlov! No that's currently not possible. However my idea about how this should be solved, is the ability to create meta users. Basically this just consists on adding a new operator for ACL SETUSER that makes a given user also able to execute anything that another list of users can execute:
ACL SETUSER mymetauser >somepassword on $+user1 $+user2 $+user3
(The $- operator should also exist). When the user is checked against a given command execution, if it fails per se to execute the command, all the users referenced by this user via $+ are also checked. If at least one can execute the command, the permission is granted.
In this way it is possible to create much complex ACL users for the minority of users needing this, without making the ACL subsystem more complex.
Pinging @yossigo @oranagra @madolson @soloestoy and other core team members that usually do low level work just to tell that I really believe this is the direction we should take instead of moving into an ACL monster :-D full of complex capabilities.
Comment From: theDogOfPavlov
That works! Thanks for the quick response, and keep up the great work. Redis is an awesome tool!
Comment From: theDogOfPavlov
Or (just a thought) you could have SETGROUP with the same ACL rules, and then on the SETUSER rule have a group membership tag...
Comment From: theDogOfPavlov
ACL SETGROUP cake -@all +rpush ~one
ACL SETGROUP pie -@all +lpop ~two
ACL SETUSER cheese $cake $pie
Comment From: antirez
@theDogOfPavlov yep that would be the same, but a group is just a user without a password set... so why to make any distinction?
Comment From: theDogOfPavlov
Technically, there's no reason at all. But from a useability point of view users and groups fit the standard RBAC approach to auth, so is the normal model most admins will be already familiar with: https://en.wikipedia.org/wiki/Role-based_access_control
Comment From: madolson
I remember we talked about this at one point, and I think the idea of metausers (In my mind I just think of them as roles) make sense. If a user has at least one role that can execute the specific command, then that user can execute the command.
The things I didn't like about metausers:
1. It was easy to accidentally pollute the namespace, since you might end up wanting quite a few users with similar sounding names. This is especially hard since setuser
The alternative of extending the ACL syntax doesn't sound like a lot of complexity. You could have a simple syntax like $+role1 starts adding to a role named role1, and $-role1 would delete the role. Or add a new command specifically for defining sub roles.
To do what was mentioned earlier with cheese cakes.
ACL SETUSER cheese $+cake -@all +rpush ~one $+pie -@all -lpop ~two
ACL SETUSER cheese $-cake $-pie
and the other approach:
ACL SETUSER cheese
ACL SETROLE cheese cake -@all +rpush ~one
ACL SETROLE cheese pie -@all -lpop ~two
ACL DELROLE cheese pie cake
If the goal is strictly to provide better access patterns, I think meta users are probably strictly worse than extending the syntax. I see meta users also serving a broader function as being able to create roles that many users can assume though, and I think that is also a compelling use case. So my honest opinion is I would kind of like both, but metausers are probably the simplest solution that is comprehensive.
w.r.t. to the setgroup and metausers, other major databases like postgres don't really make a distinction between users and groups (besides passwords), so I think it would make sense for Redis to not make a distinction either.
Comment From: theDogOfPavlov
Hey @madolson, good Analysis!
As far as the last paragraph though, I'd say that the reason that postgres doesn't distinguish between users and groups is that the grants allow more granularity over the operation and database/table/field that can be accessed. It's a single user, with multiple grants that sequentially add/remove privileges.
So maybe if this is the model you chaps are more interested in, how about porting it over en masse, which would then keep the existing approach using setuser additively, but would add a separator and a key selector to the command token:
ACL SETUSER cheese -@all
ACL SETUSER cheese +rpush[one] +rpush[three] +rpush[four]
ACL SETUSER cheese +lpop[two]
ACL DELUSER cheese
No extra commands, minimal extra parsing effort
Comment From: yossigo
The way I see it, meta-users or roles can be useful in some cases but they also add an extra complexity which is not required to solve the original problem.
I think it's basically down to turning ACLs into what their name implies - a LIST. So to address the original (simple) example, I'd do something like this:
ACL SETUSER cheese -@all +rpush ~one | -@all +lpop ~two
This is a simple example that can also be easily expressed with roles, because the ACLs are self contained and inclusive. When evaluating we simply iterate all ACLs looking for a match. This is also why this correlates well with roles/grant based approaches, as they're also inclusive.
However, we also need to consider (now or in the future) more complex scenarios where this breaks - and that's mainly where we want to set exceptions (especially on key names which are unbounded). In those cases it becomes much more powerful to treat the ACL as a rule base (order matters) and we can end up with something like this:
ACL SETUSER user +rpush ~audit:* | ! ~audit:* | +@all ~*
In this case we allow RPUSH to audit:* keys only, prevent any other access to those keys (! implies a terminal rule in this case) and grants everything else.
I'm not sure it would make sense to do all of this at once, but I think we should at least keeps this in mind and avoid directions that would close the door on this.
Comment From: theDogOfPavlov
yup, that works too!
When can I have it? ;)
Comment From: madolson
Status: This is a complex feature that we still need to build consensus on. This is a bad item to ask for help on since it will need a lot of design work. I'll assign it to myself for now.
Comment From: madolson
@redis/core-team I want to propose the following recommendation for implementing a V2 access control.
Let's start with some definitions, so that I can summarize the next section quickly.
selector: A set of options that matches commands, keys, and channels.
allow selector: A selector that if matched, will allow a command to be executed. Today Redis users have a single allow selector.
deny selector: A selector that if matched, will prevent a command from being executed.
Role: Permissions that can be assumed by a user. I don't use this term explicitly, since typically users have to actually explicitly assume a role. In most security systems users don't automatically have multiple roles on them.
ACL policy: A collection of allow and deny selectors that are evaluated together that are named and can be attached to multiple users. A single policy such as "deny dangerous" could then be applied to all users, and could be updated independently.
Named selector: This is a selector which has a unique identifier, I use
So to express our current system we would say that that we have users that each have a single allow selector. So I want to propose three additions: - [ ] 1. https://github.com/redis/redis/pull/9195 Introduce ACL policies and allow attaching/removing them to users. The user transitively gains all of the selectors attaches to the policies. - [ ] 2. Allow policies and users to have multiple selectors by introducing named selectors. Selectors can be added in a single statement. - [ ] 3. Make it explicit that there are allow/deny selectors. Deny controls take precedence over allow controls.
Addition 1 adds more re-usability and more structure, but adds a layer of abstraction that isn't great. Addition 2 makes it so that people that just want users can just have users, and makes policies more concise. Addition 3 is just a very common feature seen in a lot of other security systems. It also makes it easier to be explicit about what commands a user CANNOT execute.
We could technically get away with either 1 or 2 to introduce complex access controls, but I think the consensus is that we could add both.
Suggested implementation details + examples
## New subcommands introduced
ACL SETPOLICY <policyname> <acl_options> ...
ACL DELPOLICY <policyname>
## New acl options
### Attach the given policy onto a user.
$+ <policyname>
### Remove the given policy from a user
$- <policyname>
### Add/update a new access control to the user with the given name
|+<selector_id> <acl_options> ... |
### Remove a named selector from a user from the user
|-<selector_id>
### Change the type of selector to be an ALLOW selector
ALLOW
### Change the type of selector to be a DENY selector
DENY
Example 1: Attaching a policy to a user
### Hari has the shared policy permissions for app1
### as well as specific access to his own namespace.
ACL SETPOLICY app-1-readonly +@read ~app-1::*
ACL SETUSER hari $+app-1-readonly +@read ~hari::*
AUTH hari
get app-1::foo # ALLOWED by policy match
get hari::foo # ALLOWED by inline policy match
set app-1::foo bar # DENIED because of no allow policy
### Chris doesn't have any peronsal permissions, and
### just get's permissions for app-1.
ACL SETUSER chris $+app-1-readonly
AUTH Chris
get app-1::foo # ALLOWED by inline policy match
get hari::foo # DENIED because of no allow policy
set app-1::foo bar # DENIED because of no allow policy
Example 2: Creating a complex access control on a user, composed of multiple selectors
ACL SETUSER Mads on |+access-1 +@readonly ~* | |+access-2 +write ~write::* |
AUTH Mads
get app-1::foo # ALLOWED by "access-1" match
set write::foo # ALLOWED by "access-2" match
set app-1::foo # DENIED because of no allow policy
Example 3: Creating a deny policy
### We create a new deny policy, so even though
### Hari is allowed to execute the command, the deny
### policy blocks it.
ACL SETPOLICY DENY-DANGEROUS BLOCK -@all +@dangerous
ACL SETUSER Hari $+DENY-DANGEROUS +@all ~:*
AUTH hari
Flushall # DENIED by deny policy
Comment From: yossigo
@madolson I mostly agree with the above, but I think there are still a few clarifications / fine tunings to make.
access control: A set of options that match commands. This is what users have today, and are composed of individual operations like +write or ~*.
What we have today are selectors - they match an operation based on command, key and channel. I think that before we're dealing with more complex access controls or policies, we need to support more expressive complex selectors, i.e. "commands
Like today, selectors can use negative form but that does not imply a DENY resolution but simply exclusion of commands / keys / etc. It's currently supported for commands but not for key or channel patterns.
allow access control: An access control that if matched, will allow a command to be executed. deny access control: An access control that if matched, will prevent a command from being executed.
I'm not 100% sure about this one. Having DENY access controls basically turns ACLs into rule bases where the order of policies matter, and it can become more difficult to manage and less intuitive when thinking in terms of RBAC.
For example: with a DENY policy it's easy to block all non-admin from accessing a set of privileged keys, and make sure that's the case as long as the policy is attached to all non-admin users BEFORE any other policy. Without a DENY policy we need to be sure that no selector accidentally matches those keys.
Role: Permissions that can be assumed by a user. I don't use this term explicitly, since typically users have to actually explicitly assume a role. In most security systems users don't automatically have multiple roles on them. ACL policy: A collection of allow and deny policies that can be attached to users. You would share these between multiple users, and then just update the policy. You would update the access controls in a policy just like a user. (This is basically what Hari meant by role in his original ACL role PR, the only thing that will really change is the naming)
Just to make sure we're on the same page here --
- A policy is a set of ACLs (ordered if we have a deny, unordered if we don't)
- A policy may be named
- A policy may refer to another policy
- A user is a form of policy - we can directly specify a set of ACLs to it, refer to other named policies, or both.
Is that accurate?
Comment From: madolson
@yossigo I agree with almost everything, except that I'm not saying that policies need to be explicitly ordered to introduce deny selectors. I was instead proposing that deny selectors take precedence over allow selectors. All deny selectors are evaluated first, then the allow selectors are executed. I think deny selectors are important, but I really don't want to introduce ordering.
I'll update the above to change the terminology to selector, as I think that term is better.
Comment From: hpatro
@madolson Regarding ALLOW/BLOCK selector, Should we make it a mandatory parameter in the SETPOLICY (create) API ?
ACL SETPOLICY <policyname> <selector> <acl_options>
# Example
ACL SETPOLICY app-readonly ALLOW +@read ~app-1::*
ACL SETPOLICY app-dangerous BLOCK +@dangerous
Comment From: madolson
@hpatro I don't think it needs to be required, it just has to be one of the arguments.
Comment From: madolson
Following up from another thread, I'm going to propose a new alternative implementation for ACL V2 which I will call "fine grain key access". The core idea is to extend the description of the way keys are accessed to also include describing which keys users have write access to and which keys users have read access to. Let me explore this with an example:
USER +@all -@dangerous %I~insert::* %DR~delete::* %R~readonly::* %W~writeonly::* ~::readwrite*
The new keyword %[*]~ introduces specific permissions on the keyspace. The operations are as follows:
[R]ead: Read data from the value. [W]rite: Updates a value, composite of insert + delete + update. (E.G. SET, HMSET, most write commands tbf) - [I]nsert: Only inserts data into a datastructure, can't overwrite data. (E.g. LPUSH, SADD) - [D]elete: Only deletes data from a datastructure, can't add/modify data. (E.g. DEL, LPOP)
In this model, a user would be able to execute:
GET readonly::foo
SET writeonly::foo bar
APPEND readwrite::foo
DEL delete::foo
LPUSH insert::foo
LMOVE delete::list insert::list LEFT LEFT
but not the following commands
GET writeonly::foo
SET readonly::foo bar
APPEND readonly::foo
APPEND writeonly::foo
The thinking is that it is easier for end users to reason about how keys are being accessed as opposed to what operations can be executed on which keys. It also gives more control over commands like SUNIONSTORE, since you can now specify which keys you can write into. This also dovetails well with the work that @guybe7 did to declare the way the various keys are being accessed, since he did most of the heavy lifting already.
This "proposal" come with some oddities though. There are some commands which "touch" keys but don't need to declare them, like FLUSHALL. I would expect to execute flush all I would need write permissions + FLUSHALL permissions.
I honestly think this minor tweak might be sufficient for almost all of the use cases I have heard about. (A not-solved use case is that someone wanted access to all keys but a specific subset, but that could be implemented with an explicit deny variant of the proposed new operations if we wanted). Implementing read/write access to keys is much simpler than the roles/complex access controls we've talked about.
@yossigo @oranagra I'm not sure I have preferred option at the moment. The more I think about access control, the more I'm hesitant to add a lot of complexity without a clear understanding of the use cases. I am leaning towards suggesting let's just add read/write access to keys for Redis 7, and not add anything else.
@theDogOfPavlov Would really like your input here. I know we haven't been fast at implementing these changes, but I think your input would be great here.
Comment From: theDogOfPavlov
Firstly, thanks for all the hard work with thinking this through!
As a little background, I'm a contract CTO/developer/architect who's worked on hundreds of different gigs, and unsurprisingly in recent years many of those have used redis within the mix somewhere.
For a truly simple, atomic architecture, the current approach to ACLs is fine, for example:
user node1 on >password -@all +ping +auth +rpush ~one user node2 on >password -@all +ping +auth +lpop ~one
But as soon as I have a node within the architecture that needs to perform different commands on different keys, then the current approach to ACLs means that they can perform all the enabled commands on all the enabled keys. Which is where this issue started from originally.
For the projects I've been involved in, it would be a big benefit to be able to limit the commands to specific keys. In regard to the actual detail of what may be the optimal way to achieve this with the redis configuration (and indeed, the volume of dev time to implement it), I don't actually have a strong opinion on.
From the perspective of taking a read/write approach to ACLs, I can see where this may simplfy things, but I can also see where this might have unintended consequences. For example, by giving someone the ability to write to a key, you also let them delete it (however, this isn't a big deal: they can happily write garbage to it anyway). Or I can see that there may be a need for the ability to write to a key, but not read from it. Say multiple nodes sending sensitive data to a list, and the nodes shouldn't be able to see each others output. I've not thought about this in depth yet, but I wonder if there are other edge cases like this too?
Does that help at all?
Comment From: yossigo
@theDogOfPavlov If I understand correctly, you're basically saying that binding a specific set of commands to a specific set of keys is the real pain point, and generalizing to read vs. write is of lesser value. Is that right? This is what I personally consider the real ACL limitation, just wanted to be sure my understanding is not biased.
@madolson I'm also not sure I have a preferred option.
Initially I thought read/write may simplify things but what we're really trying to do here is map Redis behavior to the classic subject/object/operation ACL model - and when reconsidering this I realize that referring to commands as operations may after all be more straight forward.
How would we map read vs. write vs. read+write to the different supported commands? Even sticking to just read-only vs. read-write will probably result with lots of edge cases.
Staying with commands avoids this ambiguity, and we have the option of using @read or @write to refer to read vs. write operations, although with somewhat limited granularity.
Comment From: theDogOfPavlov
@yossigo yes, that's it. The current approach to enabling individual commands allows them to be controlled really granularly, which is perfect. If this could also be applied to the keys they affect too, then that would complete the functionality for rich ACLs.
Comment From: madolson
@theDogOfPavlov would you mind talking through why it would be a big benefit of restricting specific commands to specific keys?
@yossigo It would require more work on our end, but we could also add an "update" section which could restrict users from executing commands which delete keys as a side effect. This would be a weaker form of write, and include commands like "xadd,append,bitops".
My current thinking is that we should decide if we really want granular command permissions in ACLs or not. Granular command access seems like a very comprehensive solution, but I think it's much more complex. I'm not really seeing the value of it at the moment.
Comment From: theDogOfPavlov
@madolson of course: my pleasure.
I see it as a logical next-step in the authentication model that has already been implemented. I've no idea which came first, granular enabling/disabling of commands or authenticated users. But once redis had both, it was possible to start to protect the data in redis from accidental or deliberate access or damage.
At the same time, Redis has evolved beyond being a key server, and a common use I see these days is as a simple, highspeed message queue. In an MQ environment, it is common to have a manager that arbitrates requests, and agents that receive them from one queue, processes it, and then dispatches the response in a different queue.
Without an ACL that supports granular commands and keys, a compromised agent would be able to add requests to the inbound queue, and read responses from other nodes.
Comment From: madolson
The more I stare at this issue, the more I don't like trying to extend the command based approach to have a more comprehensive syntax. I've also talked to several senior database engineer folks at AWS, and they also thought the current system was counter intuitive. The primary three reasons being:
- Commands with multiple keys that do different operations will never be handled, such as copy or LMOVE. LUA scripts will always be an unsafe operation since there is no way to scope the keys down. (Similar argument for modules)
- Any syntax we come up with will be complex. Either the set of selectors will need to be ordered or we need to introduce some type of logic defining the resolution of the various selectors. Metausers was the best attempt at making it simple, but I honestly think it just made it confusing. I think our tenet of simplicity should be pushing us against introducing a more complex syntax here.
- Many of the common use cases become extremely convoluted in this approach. Granting read-only access to some set of data becomes verbose in that you're granting access to a large number of read commands + a new keyspace.
So I'm going to advocate for for the key based proposal I mentioned above. This is largely copied from there, but with more detail and I tried to think through more of the edge cases.
The core idea is to extend the description of the way keys are accessed to also include describing which keys users have write access to and which keys users have read access to. Let me explain this with an example:
USER +@all -@dangerous %I~insert::* %DR~delete::* %R~readonly::* %W~writeonly::* ~::readwrite*
The new keyword %[*]~ introduces specific permissions on the keyspaces. The * is replaced by characters representing the type of permissions, the options are as follows:
[R]ead: Read data from the value (E.G. Get, ZRange) [W]rite: Updates a value, composite of insert + delete. (E.G. SET, HMSET, most write commands tbf) - [I]nsert: Only inserts data into a value, can't overwrite or delete data. (E.g. LPUSH, SADD) - [D]elete: Only deletes data from a value, can't overwrite or add data. (E.g. DEL, LPOP, HDEL)
In this example, a user would be able to execute:
GET readonly::foo
SET writeonly::foo bar
APPEND readwrite::foo
DEL delete::foo
LPUSH insert::foo
LMOVE delete::list insert::list LEFT LEFT
but not the following commands
GET writeonly::foo
SET readonly::foo bar
APPEND readonly::foo
APPEND writeonly::foo
There is no "update" since Redis almost never makes a distinction between update + insert. HMSET for example either inserts or updates data. Having insert and delete separate though is useful to solve message/passing use cases with lists and to a lesser extent streams.
A plethora of examples including some interesting cases:
XADD: Since this command can explicitly trim data, it requires full write permission even though just insert would be expected.
FLUSHALL: No permissions required here outside of commands. The reasonable expectation is that you would need write permissions on all keys, I don't propose changing that here.
EVAL/EVAL_RO: Handled trivially as we just check if the keys have corresponding full access or read access.
LPOP: Requires read and delete permission. Note that this is different from what is in mainline, and I'm not sure I really get why.
LPUSH: Just requires INSERT permissions, as it can't delete data from the list.
COPY: Requires reading from the source and writing to the target.
SET: Requires READ + WRITE, because we added GET.
GETEX: Also requires Read + write, since you can change/delete the expire.
EXPIRE: Just requires WRITE as you would expect.
XTRIM: Just requires Delete, as it doesn't return any data.
SADD: Just requires Insert, as it can only add an item and can't overwrite anything.
ZADD: Requires WRITE, unlike SADD you can overwrite the score.
ZRANGESTORE: Source is just READ, Destination is write since it will overwrite the target.
XGROUPREAD: The only real odd case I found, since this is READ+WRITE.
PFADD: Technically this is rewriting a string, so it requires the full "WRITE" permissions.
A note about publish/subscribe. We may want to split the channel permissions so that a user can split a user from being only able to subscribe vs only able to publish. @redis/core-team @hpatro @theDogOfPavlov Thoughts?
Comment From: theDogOfPavlov
Warning: random unstructured thoughts.
I think there are two bits at play here: the internal implementation, and the external UX presented to the tin-monkeys that will be configuring redis.
As a tin-monkey in this situation, I am happy to say that I know nothing of the internals of Redis, and that I’m just interested in a UX that is granular and makes some form of sense to me. Concise and intuitive would be good, but I’ll settle for consistent.
In the SQL world, rich ACLs are delivered through the UX as a sequence of GRANT/REVOKE sequences (where privilege and object are a many-to-one relationship):
GRANT [privilege,…] ON object TO {user|role};
The current redis approach is the equivalent of the following (where privilege and object are a many-to-many relationship):
GRANT [privilege,…] ON [object,…] TO user;
As a scumbag tin-monkey, I just want to get closer to the SQL approach.
Comment From: oranagra
I"m a bit fuzzy on the details (Write vs Insert vs Delete), but I do agree this approach is better than specifying permitted / forbidden combinations of key-names and commands.
Your example of LMOVE is maybe a perfect example, i.e. if you wanna allow LMOVE to read from one set of keys and write to the other, but not the other way around, you have no way to do that by specifying a combinations of key names and commands.
I also like the fact that it makes the read and write flags we added to the key-specs (#8324) useful.
We'll probably have to change the mechanism that extracts the key names for commands to use the key-specs, and delete the old getKeysFromCommand (@guybe7 FYI).
This is maybe another indication that the distinction between Write / Insert / Delete is too complex (we'll either have to give that up, or modify the keyspecs metadata, before it's too late to change).
@madolson Can you please specify why you think this distinction is needed, and it's not enough to just have read and write?
Comment From: QuChen88
I have a few thoughts here -
-
Defining the set of ACL rules based on
readonlyandwriteonly (insert/delete)permissions is counter-intuitive from a client developer point of view, who is typically looking at a set of redis client APIs that are command-based. Say as a client developer, the fact that I know I have read-only access to keys that are named with prefixread-onlydoesn't really help me determine which client commands I can effectively use to program my application without in-depth knowledge of the redis commands' internal implementations. -
There are a lot of edge cases in the internals of redis command execution that significantly complicates the validation logic of the proposed ACL rules, and gets very confusing for the users to understand what is allowed and what is not allowed. More specifically, there are several scenarios that I can think of here:
a) If a user with read-only permission does a GET on a key that has already been expired, the GET operation will actually translate into a DEL operation which deletes the key. In this case, wouldn't the ACL policy be violated and should get rejected even though the command itself is intended to be read-only?
b) If a user who has write access to key1 tries to write a new value to it, and causes redis server to go above maxmemory limit, this can in fact trigger eviction in redis which will end up deleting key2 which the user doesn't have access to. Wouldn't this break the intended ACL policy that he only has write access to key1, not key2?
c) If a user has a read-only access, then can the user apply a write command which doesn't end up modifying the key itself? For example, doing LPOP on an empty list doesn't modify the redis server keyspace at all, but still returns NIL so it essentially acts like a read-only command to the user. Given the fact that LPOP is a write command, this is very counter-intuitive as the user should not be able to run that command, but the command technically doesn't do write in some special situations...
d) How do you categorize commands which are neither flagged as read-only nor write? For example, publish or script which can both generate memory in redis and cause evictions of keys in any random keyspace?
All in all, I would still prefer the previous idea of defining ACL access rules based on commands (GET, SET, etc) rather than the intended effect of applying a command (i.e. read-only or write-only etc). That way, there are a lot less ambiguity in terms of what the user can do and avoids a lot of complex edge cases in special commands having special side-effects. If the pain point here is that we want to restrict access to certain set of keys, then let's focus on solving that only and avoid over-complicating the problem.
Comment From: oranagra
Regarding [a] the deletion of an already expired key is certainly not considered an action of the read command that just detected it. they key was already logically non exiting, and there's no way for users to access it anymore.
Same goes for [b], the deletion of a key by the eviction mechanism is the responsibility of the person who configured eviction, from permissions perspective it's not really an action of the command that happen to trigger it.
About [c], LPOP can be flagged as both read and write, it does read / extract data from redis and also modifies the data set. The fact that in some cases it fails (be it WRONGTYPE or an empty list), doesn't make it a read-only command.
Regarding [d], many commands that don't act on keys names (such as CLIENT, DBSIZE, SCRIPT, and even FLUSHDB) will need to be controlled by the old mechanism of just blocking commands, that's not gonna change.
Comment From: YaacovHazan
@oranagra I think the point that @QuChen88 tried to clarify is that we can always have the right answer for each case, but for the users, it always needs to become with a "good" explanation. For users, especially the ones that are not familiar with the internal of Redis, it is much more clear and intuitive to apply permissions based on command rather than write/read access.
The only (I think) case that this approach doesn't cover is the multi-key commands (like LMOVE). for that we can: a. In addition to the command-based access, we can add (now or in the future) the read/write access, as @madolson suggested. b. Look for a specific solution for these cases, where per command we can control part of it. for example:
For the general command-based we will use:
%SET~setkeys::*
For special cases like LMOVE we will use:
%LMOVE|read~lmovesrckeys::*
%LMOVE|write~lmovedstkeys::*
@oranagra @madolson WDYT?
Comment From: zhugezy
In general I'd prefer command-based approach rather than the R/W category approach for its clarity. More over, when focusing on commands & keys (ignoring others, such as channels, passwords or else), currently it has been a command-based approach in Redis-6.0 ACL system. From my perspective, it's not a good idea guiding users to a different approach, which is destroying users' cognition of Redis ACL. Like what @theDogOfPavlov said,
As a scumbag tin-monkey, I just want to get closer to the SQL approach.
Same as here, I just want to get closer to current ACL approach, with more powerful features. ===========================👇Some detailed thoughts👇==================================
a. In addition to the command-based access, we can add (now or in the future) the read/write access,
I think it's not a good idea mixing these two kinds of approaches because it makes things more complex. @YaacovHazan what do you think?
The only (I think) case that this approach doesn't cover is the multi-key commands (like LMOVE).
I'm not quite sure why command-based approach isn't working on multi-key commands. Currently it's an 'and' check, e.g. LMOVE key1 key2 works only when user have all of these +LMOVE ~key1 ~key2 access. Could you explain it in detail for me? thanks.
Comment From: oranagra
@zhugezy the problem with LMOVE (described here) is that if you wanna give a certain user only read access to certain keys, and write access to other keys, you can't do that by just limiting which commands can be used on each set of keys (since LMOVE does both read and write, and you may want to restrict just the write portion of it)
Comment From: QuChen88
@oranagra Just to clarify, for the case of commands like LPOP or RPOP, it normally acts like a write command if the list is not empty. But when the list is empty, the return value is NIL to the client, which doesn't signify that an error has occurred, but it tells the caller that the list is empty (which reveals the value of the item), so IMO it is acting the same as a read-only command.
There are a lot of other redis commands that falls into this category, not just LPOP and RPOP and LMPOP. For example, for command like SETNX, MSETNX and HSETNX, where it does a no-op and returns 0 if the key already exists with a value. So when the key doesn't exist, those commands performs actual write operation, but when the key does exist, it is a read-only operation which tells the caller that the key already exists. Another example is LPUSHX and RPUSHX, where the command only pushes element onto the list if the list already exists, otherwise it is a no-op and returns 0 (telling the client that the list doesn't exist). And other examples include SPOP, SREM, HDEL, ZPOPMIN, ZPOPMAX, ZREM, and the list goes on...
Since there are a lot of such commands that exists in Redis, I would not ignore it. Whereas the counter-argument here is only for LMOVE and RPUSHLPOP commands, that is a couple of very specific commands and I would argue that if the user wants to pop element off of one list and add onto another, they do need to have access to both lists to run this command, so we should be OK with specifying that he can do LMOVE on list1 and list2 simply. Essentially, I am still in favor of doing command-based rules.
Comment From: madolson
Love the discussion, so many ideas!
@YaacovHazan There are quite a few edge cases to handle for your b approach. I think this is all the edge cases for RW:
The commands that use multiple keys with different R/W permissions: * COPY: A read and write key * SORT + Store: A read key and optional write key * PFMERGE: Multiple read keys and a single write key * GEOSEARCHSTORE (and GEORADIUS + Store): A read and a write key. * S(DIFF|INTER|UNION)STORE: Multiple read keys and a single write key * Z(DIFF|INTER|UNION)STORE: Multiple read keys and a single write key * ZRANGESTORE: A single read key and a single write key. * BITOP: Multiple read keys and a single write key
There are a couple of more examples in my comment for Oran as well, but they are more subtle. Based on my reading of your comment, it sounded like you wanted to implement both improvements (allow defining multiple access controls per user AND key based permissions), just wanted to confirm my understanding.
@oranagra The argument for insertion/deletion is that it rounds out the remaining edge cases. I'll concede it's not really necessary, but I wanted to make sure I was handling @theDogOfPavlov 's use case, which was pushing and popping from keys (which isn't by just splitting read and write).
The commands that use multiple keys with different Read/write/insert/delete permissions: * LMOVE (and RPOPLPUSH): One key with insertion one with deletion. * SMOVE: Delete from one set and move to another set. * RENAME: One key with deletion and one with insertion.
Note, these edge cases can all be handled with just a third category, let's call it [D]elete. So a RWD is likely sufficient for these edge cases.
My thought about adding [I]nsert is that it allows creating permissions that can't be destructive of data. I don't feel as strongly about this point. EDIT: Now that I've actually gone and made a spreadsheet of commands, like most of the write commands are [I]nsert, which makes it sound like a lot more work.
@QuChen88 We talked about all these points in the office, and I think Oran did a good job responding. To your last point about, I don't think the current state of a key changes what logical operations you can execute on it. In a relational database, if you do a delete * from table, but the table is empty so it returns 0, that doesn't suddenly make it a read operation since it could have deleted data.
Defining the set of ACL rules based on readonly and writeonly (insert/delete) permissions is counter-intuitive from a client developer point of view, who is typically looking at a set of redis client APIs that are command-based. Say as a client developer, the fact that I know I have read-only access to keys that are named with prefix read-only doesn't really help me determine which client commands I can effectively use to program my application without in-depth knowledge of the redis commands' internal implementations.
That is the downside of the proposal. You also don't HAVE to use this if you don't want to, just grant RW permissions on all keys. The command approach still exists. This is for power use cases only.
@theDogOfPavlov @zhugezy I want to highlight one thing, the command based ACL definitions aren't going away. For most users, I expect that they will continue to just specify which commands they want a user to be able to execute and leave it at that. The two extensions, extended command based ACLs and key based permissions, are both just ways to enable more advanced use cases. The reason I'm proposing adding the key based permissions is that it sounds like a simple addition that adds a lot of semantic power to ACLs. I really want to know if that makes sense to you, because I might be out of touch with the people who are going to be configuring it.
Comment From: guybe7
for the record, here's draft code for extracting keys using key specs
int getKeysUsingKeySpecs(struct redisCommand *cmd, robj **argv, int argc, getKeysResult *result) {
int j, i, k = 0, last, first, step, *keys;
for (j = 0; j < cmd->key_specs_num; j++) {
keySpec *spec = cmd->key_specs+j;
serverAssert(spec->begin_search_type!=KSPEC_BS_INVALID);
if (spec->flags & CMD_KEY_INCOMPLETE) {
result->numkeys = 0;
return 0;
}
first = 0;
if (spec->begin_search_type == KSPEC_BS_INDEX) {
first = spec->bs.index.pos;
} else { /* KSPEC_BS_IKEYWORD */
int start_index = spec->bs.keyword.startfrom > 0 ? spec->bs.keyword.startfrom : argc+spec->bs.keyword.startfrom;
int end_index = spec->bs.keyword.startfrom > 0 ? argc-1: 0;
for (i = start_index; i != end_index; i = start_index <= end_index ? i + 1 : i - 1) {
if (i >= argc)
break;
if (!strcasecmp((char*)argv[i]->ptr,spec->bs.keyword.keyword)) {
first = i+1;
break;
}
}
}
if (!first) {
result->numkeys = 0;
return 0;
}
if (spec->find_keys_type == KSPEC_FK_RANGE) {
step = spec->fk.range.keystep;
if (spec->fk.range.lastkey >= 0) {
last = first + spec->fk.range.lastkey;
} else {
if (!spec->fk.range.limit) {
last = argc + spec->fk.range.lastkey;
} else {
serverAssert(spec->fk.range.lastkey == -1);
last = first + ((argc-first)/spec->fk.range.limit + spec->fk.range.lastkey);
}
}
} else {
step = spec->fk.keynum.keystep;
long long numkeys;
if (!string2ll(argv[first+spec->fk.keynum.keynumidx]->ptr,sdslen(argv[first+spec->fk.keynum.keynumidx]->ptr),&numkeys)) {
result->numkeys = 0;
return 0;
}
first += spec->fk.keynum.firstkey;
last = first + (int)numkeys-1;
}
int count = ((last - first)+1);
keys = getKeysPrepareResult(result, count);
for (i = first; i <= last; i += step) {
if (i >= argc) {
/* Modules commands, and standard commands with a not fixed number
* of arguments (negative arity parameter) do not have dispatch
* time arity checks, so we need to handle the case where the user
* passed an invalid number of arguments here. In this case we
* return no keys and expect the command implementation to report
* an arity or syntax error. */
if (cmd->flags & CMD_MODULE || cmd->arity < 0) {
result->numkeys = 0;
return 0;
} else {
serverPanic("Redis built-in command declared keys positions not matching the arity requirements.");
}
}
keys[k++] = i;
}
}
result->numkeys = k;
return k;
}
int getKeysFromCommandWithSpecs(struct redisCommand *cmd, robj **argv, int argc, getKeysResult *result) {
if (cmd->flags & CMD_MODULE_GETKEYS) {
return moduleGetCommandKeysViaAPI(cmd,argv,argc,result);
} else {
int ret = getKeysUsingKeySpecs(cmd,argv,argc,result);
if (ret) {
return ret;
} else if (!(cmd->flags & CMD_MODULE) && cmd->getkeys_proc) {
return cmd->getkeys_proc(cmd,argv,argc,result);
} else {
return 0;
}
}
}
and in genericGetKeys:
keys = getKeysPrepareResult(result, numkeys);
result->numkeys = numkeys;
if (storeKeyOfs) {
keys[0] = storeKeyOfs;
keys++;
}
/* Add all key positions for argv[firstKeyOfs...n] to keys[] */
for (i = 0; i < num; i++) keys[i] = firstKeyOfs+(i*keyStep);
//if (storeKeyOfs) keys[num] = storeKeyOfs;
return result->numkeys;
}
Comment From: yossigo
We have several interleaved sub-discussions taking place here (if you scroll up enough), which I think we should break down and handle individually.
Should we express privileges as commands or as more granular permissions?
There are clear advantages to defining access in terms of granular read / write / insert / delete operations (or a variant of them): * In some cases, it is a more natural and generic way to express security policies * Aligned with how other systems do that * Potentially more relevant for modules, Lua, multi-key commands, etc.
There are also disadvantages: * Not consistent with current ACLs * Mapping of some Redis commands and behaviors (e.g. expire, eviction) will not be trivial or self explanatory. Many ad-hoc decisions and pedantic documentation will potentially be required to deal with that.
Can we think of permission-based ACLs a replacement for command-based ACLs? I think not, for two major reasons: 1. Backwards compatibility. 2. In some cases it simply makes sense, e.g. block administrative commands, etc.
So, this in turn becomes another disadvantage for the permissions based approach: it'll live side-by-side commands so ACLs become more complex.
Nevertheless, I still think we should support that.
Should ACL selectors be inclusive only, or do we also support exclusion / negation?
As of 6.2, the commands selector supports both inclusion and exclusion. The selector for keys only supports inclusion.
Do we need to extend the keys selector to support exclusion? It's tempting to make it possible to express something like all keys except secret:*, but I don't think we can justify more complexity just for that.
Users vs Roles
The concepts of roles, policies and meta-users all came up to make ACLs composable and reusable, and avoid repetition of the same ACL configuration across users.
I propose to leave this part out of scope, for these reasons: * Avoid more complexity (we have enough already) * Roles don't add new capabilities, just simplify existing ones. But our charter right now is really to extend ACLs to make them more useful.
(How) do we support binding specific operations/permissions to specific keys (objects)?
This is the biggest limitation with the current ACL design, as was advocated multiple times by @theDogOfPavlov and others.
I think it's completely orthogonal to whether we assign commands or granular operations to keys and ideally it should apply to both in the same way.
The only way to support this is to think of ACLs as a set of arbitrary number of (granted-privileges, keys) tuples. @madolson basically proposed the same thing in the last proposal, but was assuming that "granted-privileges" refers to the granular privileges only and not to commands.
I'd like to suggest extending that proposal into something like this:
USER (@+all -@dangerous ~readwrite::*) (%I ~insert::*) (+xadd ~streams::*)
Every user has a set of one or more (ACL Entries). If only one entry is specified (like today) the parenthesis can be avoided, for backwards compatibility.
Every ACL Entry contains:
* Zero or more command selectors, identical to how we handle them today. If none specified, the default is +@all.
* Zero or one privilege selector (%) followed by privileges as proposed by @madolson . If none specified, the default is %RW (read-write).
* Zero or more key selectors, like we do now. If none specified, the default is ~*.
I think this approach will offer backwards compatibility, will solve the problem of associating specific commands to specific keys, and will also introduce permissions as an alternative to commands.
I haven't given enough thought to possible implementation complexities, but I tend to think it should not be very different than other proposals as every ACL entry is evaluated individually, the same way we do today (or with permissions, if @madolson's proposal was adopted).
Comment From: theDogOfPavlov
@yossigo as a syntax, this works just fine, and delivers the granularity that I'm looking for.
Comment From: madolson
@yossigo I'm more or less onboard with that, but I think some nuances still need to be thought through a bit.
I still think having multiple selectors is "unnecessary" in that it appears to give more options but I think it'll just help people shoot themselves in the foot. Questions like, "how do you update a selector?", "Do all selectors need to match or just one of them", "If I have multiple selectors on the same keyspace are they merged?", "If I have insert permissions on one set of keys and write permissions on another set other keys, does that allow me to use copy correctly?" all come to mind, and unlike the current implementation it's less obvious what happens. Two people mentioned that they want this to be like traditional RDMS databases, which although this looks similar it's not.
Having a separate "privilege" selector misses one of the points of key based permissions. Following up with the LMOVE example, you need to be able to say "srcKey" has delete permissions and "destKey" has insert permissions, if you want to have fully fine grain control of the operation. Your proposal is framing it as a complete alternative to commands, which I think is an option, but I don't think adds some but less value. I think instead the permissions should be tied to the keys.
I also don't think the implicit default for secondary selectors should be +@all, since (%I ~insert::*) would allow config/shutdown etc. I think the default should always be -@all unless explicitly indicated otherwise.
Comment From: madolson
So I'll try to address all of my above comments with a counter-suggestion.
Key based permissions
We will introduce three sub-permissions on keys, read + write + delete. The goal here is to make it easy to restrict specific types of operations on key values and to allow defining permissions that work with modules. This functionality also lets you reason about "future proofing operations". Key based permissions do not replace command permissions.
~<pattern>is just a shorthand for full permission on the pattern.%(R|W|D)~<pattern>will grant the specified permissions for that pattern.- To reset, use
resetkeysjust as before. Patterns will be merged as expected. All key accesses will be marked using the new version of the table. %(R|W|D~*is valid syntax, and maps to granting the specified access to the entire keyspace.
Multiple selectors
Selectors allowing defining specific sets of allowed commands on different key spaces. Users will have a default selector as well as any number of additional selectors. Only the main selector can be modified, all remaining selectors are immutable.
- All new selectors by default are -@all and
resetkeys. (Can we please introducenokeysas an alias to resetkeys now?) - Selectors are de-duplicated if they are identical..
- non-default Selectors are immutable. The only way to update them is with a new keyword,
noselectors. Therefor to make minor changes requires fully resetting them and adding a new ones. Callingnoselectorsdoes not remove permissions from the default selector. This dramatically reduces the chance of accidentally updating a selector to something you don't expect, since you are being explicit about the new state.
Roles
For now I'm going to agree and say they are unnecessary. We definitely could implement them, and I don't think they would be all that complex, I'm just as sure about it.
Deny permissions
Also going to agree with Yossi, originally I thought they were important for multiple selectors, but most people think they are overkill. The intention was mostly to prevent accidentally granting access to flushall or something, but it seems no one is worried about that.
Example
## User load
USER +info +client resetkeys (%W~insert::*) (+xadd ~streams::*)
## SETUSER command
ACL SETUSER noselectors (%WR~insert::*) (+xadd ~streams::*)
Comment From: yossigo
@madolson I think your last proposal is the best one we've had so far. The only thing that is still somewhat confusing is the concept of default selectors. Do they serve any major purpose? For backwards compatibility, we could just support the old syntax.
Comment From: hpatro
@madolson Some of my thoughts:
As per your example:
## User load
USER +info +client resetkeys (%W~insert::*) (+xadd ~streams::*)
- IIUC, As per your suggestion we will have subpermissions on keys or else regular set of commands on keys. Do we need to introduce new subpermissions? Can't we reuse the existing categories and introduce another missing category
+@deleteto achieve the same? This would still align with multi access control list for a user and reuse the categories feature as well as reduction of a newer terminology for user.
For e.g.:
## User load
USER +info +client resetkeys (+@write ~insert::*) (+xadd ~streams::*)
- The suggested approach is basically a hybrid of current approach and keys based permission. In a single SETUSER operation, I believe the older model would also be available for the customer.
Do you think the below one is a valid syntax ?
## User load
USER -flushall +@read (+@write ~insert::*) (+xadd ~streams::*)
-
Regarding mandatory
noselectors. Every client has to get the user's existing access string and then update (append/remove/modify) it as per the need. Agreed it's safe but two calls for single operation. Should we let the API open for both append/update and the client choose as per their needs? -
Roles (Easier visualization/manageability) and Deny permissions (Power usage) Both are two way door and can be extend on the above change as well.
I was vouching for roles as that let's you update only a particular selector.
Comment From: madolson
@yossigo The thinking behind the default selector is if you are using ACL SETUSER you can add a selector while still having the existing one. Otherwise you end up with having to do something bold like reset + adding back the current selector + adding the new one. I agree it's not strictly needed for anything.
Comment From: madolson
A note here, I am working on a PoC to draft. I should have it ready in a couple of days to get more feedback on the specifics.
Comment From: oranagra
anyone who follows this, may want to follow #9974. p.s. i'm setting it to resolve this issue (even though it doesn't actually add "meta users", since that's the design we agreed on to handle the limitations this issue came to solve). let me know if i'm wrong.