This is an issue that was discovered while implementing ACL V2. The end result of this will be we can declare channels from module keyspecs that are checked by ACLs, and not require module developers to explicitly do ACL checks for channels.
In the current implementation, ACLs relies on keyspecs for determining which keys are being accessed and hardcodes which channels are accessed and which commands access them. This means that modules can declare keys that are checked by ACLs, but doesn't allow them to define channels that are checked.
The reason we can't use the existing keyspecs to access the channels is that: 1. Only shard channels are declared as keys, not regular channels. This is really just a hack because shard_channels are routed like regular keys are. 2. Some channel accesses are always allowed, specifically when unsubscribing. There is no way to exempt a key from having a check.
I propose we finalize the following 4 flags.
1. CHANNEL and CHANNEL_PATTERN: Flags that are used to indicate that a channel or channel pattern is being used instead of a key. Modules can use these to declare channels that will be checked by ACL permissions.
2. NO_SLOT: Flag used to indicate a key should not be assigned to a slot using the normal cluster routing. This is primarily intended to be used to differentiate shard and normal channels. Setting this flag omits keys from COMMAND GETKEYS and the legacy COMMAND command. I suppose modules could use this for some reason if they want to declare a key which have ACLs checked but may not want to route the command.
3. Introduce a keyspec called "IGNORE_ACL" to indicate that this key is not checked as part of ACLs. This will handle the unsubscribe case, and modules can use it as well if they want.
@oranagra @yossigo @guybe7 Thoughts?
Comment From: madolson
@oranagra @guybe7 Now that Redis 7 is out, care to weigh in about the best way to resolve this issue?
Comment From: oranagra
The main drive behind key-specs was to expose them to client libraries (ACL was a secondary use case). That's also true about the decision to include shard-channels in key-specs, it was mainly for the benefit of client libraries, to implicitly handle them correctly (they'll even show in the initial triplet, i.e. first,last,step).
So IMO if we do any of that, i think it should be internal, for the benefit of ACL, and not exposed in the COMMAND command. But we do need to expose them to modules, which i think could cause some confusion.
Looking at redis 6.2, ACL relied solely on moduleGetCommandKeysViaAPI in order to verify command keys on modules.
maybe we should introduce something similar for channels, instead of mixing this with key names?
i.e. a module will declare a getchannels-api flag, and will then use an RM_ChannelAtPos() calls?
Sharded channels would still be declared by modules using key-specs, and the ACL code could check both. i.e. prefer moduleGetCommandChannelsViaAPI, but when missing, resort to key-specs with CMD_KEY_CHANNEL
Edit: RM_ChannelAtPos will take a flag indicating the purpose (subscribe, publish, unsubscribe)
Comment From: madolson
We chose to include shard-channels in key-specs to handle the cluster mode routing use case, not to the clients benefit generally. We've also published a whole host of other data which is currently, AFAIK, useless to clients (RW/RO/ACCESS/etc). I think we've made a strong stance that utility is a more important consideration that conflating names. If anything, making a division between shard channels and regular channels makes much less sense than grouping shard channels in with keys (from a client perspective).
If we really feel so strongly about it, I would prefer to just rename key-spec to something like arg-spec, so we don't run into these types of issues in the future.
Comment From: oranagra
yes, we chose to ad shard-channels to key-specs for cluster routing, both for clients (mainly for the legacy triple in COMMAND, not eve for key-specs part of COMMAND), and also for internal uses in redis (doing that enabled us to reduce the amount of special handling inside redis, but i think that was a secondary win, not the main objective).
The main purpose of key-specs was to assist cluster routing, other metadata in there about keys, but i think it would be wrong to add non-key data in there (shard channels are an exception since for cluster routing they behave as keys). If we proceed this way, we may end up adding timeouts, and TTLs there too? (that's covered by the argument spec)
I think that for the purpose of ACL, with modules, we just need to follow the footsteps of the old key location mechanism (namely getkeys-api).
furthermore, the case where a wants to publish a message, it is likely that the name of the channel is not at all part of the command arguments (modules are not likely to want to expose a MODULE.PUBLISH command, instead, they'll have a command that does something else, maybe on keys, and the publish part is a side effect of the main action, using a constant channel name). So i think it's perfectly fine to let them rely on an explicit RM_ACLCheckChannelPermissions.
so i think we can let them use the built in ACL mechanism for shard-channels, since it comes for free, but require either an explicit RM_ACLCheckChannelPermissions, or getchannels-api callback, for the non-sharded channels.
maybe @yossigo can chip in?
Comment From: yossigo
I'm not sure this mess is even sortable.
We need to expose sharded channels to let clients handle routing. Using keyspecs for that is a hack because they are not keys. But it's a hack that's (a) probably easier for clients than anything else; (b) simplifies the introspection interface.
We do not need to expose channels to clients. That is similar but not identical to the key access flags situation: it's slightly more likely they'll be used, and exposing them did not cost much in terms of additional interface complexity. Changing keyspecs to argspecs to advertise channels and be consistent seems to cost more and bring even less value.
I think @oranagra's suggestion does solve the module problem and offers a reasonable, sane way for ACLs to handle that, but it's not perfect either. This leads me to wonder if we should have insisted on sharded channels to be mapped to real keys so that we could remain with the old paradigm of one global namespace for channels and one sharded namespace for keys.
Comment From: oranagra
btw, we probably need to add flags to RM_ACLCheckChannelPermissions (if the intention is for publish / subscribe / unsubscribe). i suppose we can still change the API since it was only released in an RC?
Comment From: guybe7
for what it's worth i agree with Yossi's comment, I think he has some good points
Comment From: madolson
I still strongly believe we should not allow channel permissions to be defined through the keyspec. I'm afraid we will run into more complications down the line that will make us regret further conflating channels with keyspecs. Therefor, maybe instead of defining "CHANNEL" in the keyspecs like we currently do, we replace it with a flag like "NON_KEY" that we can use to hint to clients that the argument is routed like a key but just isn't a key. We make no indication that it is any type of channel, and we can use it generally in the future if we want to route other stuff like keys.
The rest of the proposal still makes sense, we can add the new module APIs so that modules can define which positions are channels and how they are accessed.
Comment From: oranagra
i suppose i'm ok with the NON_KEY. although i don't see the problem in tagging it with CHANNEL either. not sure it'll be a problem to add other non-key key-specs in the future, and tag them with their own flag (like we did for CHANNEL).
as long as we agree that key-specs are there mainly to assist cluster routing, and that we create different mechanisms for ACL, these two suggestions are practically the same.
Comment From: yossigo
@madolson I agree, this makes sense.
Comment From: madolson
I somehow missed these updates, I think we have a clear path forward. I should have time to post a PR tomorrow.