Till now, module commands did not take part of the ACL Categories, and the only possibilities to permit module command in ACL rules were to either explicitly add each module command, or add +@all.
I would like to consider changing this by letting modules create their own ACL Categories, and also allow them to add command to existing (and even core) categories.
In order to do that, i think it's right for redis to keep a list of the allowed and blocked categories and individual commands inside the user object so that it is possible to re-compute the allowed command (bit-map) of existing users when a module is loaded.
This will also mean that we on longer needs to guess / recompute this list in ACL SAVE and ACL GETUSER.
It would also be a good idea for redis to no longer fail to apply a SETUSER if a category or a command is missing. for safety, this should only be permitted on +, while on - we will keep failing on an unrecognized category or command.
- [ ] Redis does not fail if a category or command is missing
- [x] Allow modules to register commands to existing categories. https://github.com/redis/redis/issues/11516
- [x] Allow modules to create new categories. https://github.com/redis/redis/issues/12428
- [X] Store a copy of the ACLs so they can be recomputed verbatim https://github.com/redis/redis/pull/11224
@redis/core-team / others, please share your opinions and concerns.
Comment From: madolson
I broadly agree with this, and had a backlog to post something very similar to this post. I have no real objection to modules being able to add commands to core categories.
It would also be a good idea for redis to no longer fail to apply a SETUSER if a category or a command is missing. for safety, this should only be permitted on +, while on - we will keep failing on an unrecognized category or command.
Not sure I agree with this though. This is typically an indicator of some type of of user error. I understand that maybe you have a user that could theoretically have a categorically that might have been added to a user that is now removed or other such edge cases.
Comment From: itamarhaber
I'm also in the opinion this is long overdue.
WRT not failing on +UNKNOWN, I think this is intended in cases when the module isn't loaded but the ACL contains module-specific perms.
Comment From: madolson
Can we not resolve that as long as modules are loaded before ACLs? Since modules will add both new commands as well as categories.
Comment From: madolson
Someone from AWS is going to take a look at some of these improvements.
Comment From: madolson
@oranagra the only item left here is "Redis does not fail if a category or command is missing", do we still think that is needed? I think the current acl story seems complete.
Comment From: oranagra
i'm not sure.. i think that's right. but it does go beyond just modules, so we need to check if anyone sees any problem with that.
@yossigo what do you think about changing redis to NOT fail ACL SETUSER or ACL LOAD if +@cat or +cmd is used on a command / cat that's missing? is that a breaking change? comes with any risk? or can we consider that a plain improvement?
the only risk i see is that someone has a typo and the command won't work.
Comment From: yossigo
In my opinion, it's a breaking change but a relatively small one that should be acceptable in a major version. The most significant risk I see is an incorrect rule addressing a non-existing command/group (thereby an NOP) that becomes effective due to group/command changes. But it's too theoretical and improbable to be considered seriously.
Comment From: madolson
The most significant risk I see is an incorrect rule addressing a non-existing command/group (thereby an NOP) that becomes effective due to group/command changes. But it's too theoretical and improbable to be considered seriously.
Yeah, I suppose my literal biggest concern is someone does +@all -@dangerous, but misspells dangerous. The rule removals are the ones that make me more concerned. It's pretty obvious that your application isn't working because you didn't add something, it seems less obvious that you didn't correctly remove something. This should be resolved by only allowing additions to use undefined commands and categories.
I get the ergonomics of the improvement, it'll be easier for cross version and module compatibility, but I worry a lot about just one person doing it wrong. I like validation where possible.
Comment From: yossigo
Maybe this is mostly resolved by only allowing additions to use undefined commands and categories.
@madolson I assumed it's fully resolved, not mostly resolved. If that's not the case, we should avoid it.
Comment From: madolson
I assumed it's fully resolved, not mostly resolved. If that's not the case, we should avoid it.
Yeah, I think so. I'm just very hesitant to change the expectations on ACLs too much.