-
Notifications
You must be signed in to change notification settings - Fork 24.4k
Adds pub/sub channel patterns to ACL #7993
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
Conversation
|
Thanks a lot for working on this! I agree with your choices in notes.
Will this cause existing users (when updating Redis) to lose access to pub/sub functionality? |
Yes, existing users will need to be provisioned with channel permissions as part of the upgrade. Thanks for pointing this out, this is definitely an important observation (updated the top comment to reflect that). |
|
Have you considered letting users have access to all channels unless explicitly restricted? |
|
Agreeing about the pro, but the real con imo is that ACL assumes that new users can't do anything and must be explicitly granted "permissions" to keys/commands/subcommands/categories. I'd rather maintain that assumption rather than introduce an inconsistency, but perhaps I'm wrong :) |
|
From a consumer perspective I'd argue the inconsistency already exists since currently keys must be explicitly allowed while channels are implicitly allowed (assuming one has access to a relevant command) I understand you want to remove the inconsistency, but that could be done as a breaking change in a later major release after adding this feature as a non-breaking minor release |
|
You've convinced me - let's wait for the @redis/core-team's feedback on this. |
|
My personal opinion is that we need to revamp pubsub so that it can play nicely in the keyspace, since that would also enable us to properly scale it in cluster mode. I would prefer to have the improved pubsub system support authentication using the key patterns if people want to use it, otherwise they can fallback to the older API. On this PR, I agree that the default permission should be allchannels if we go down this route. Backwards compatibility ftw. |
oranagra
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.
@itamarhaber i agree with your choices, specifically:
- PUBSUB and UNSUBSCRIBE don't need to be changed.
- If the permissions changed after subscription, do nothing about it.
- make it secure by default, breaking compatibility with existing ACL users that use PUBSUB (when they upgrade, will have to blindly add a keyword to each of the ACL rules to keep them backward compatible)
i'm not entirely sure about the literal approach for patterns.
|
One question: should we disconnect subscribers when changing the ACL pubsub rules, or should we check ACL pubsub rules in |
madolson
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.
Also, as discussed we need to update the defaults.
|
The top comment has been updated to reflect the current. @redis/core-team please review and approve for 6.2 RC1 :) |
yossigo
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.
LGTM, just a couple of... typos.
Co-authored-by: Yossi Gottlieb <yossigo@gmail.com>
|
@itamarhaber i noticed there are two checkboxes at the top not checked |
|
also, please update the top comment to reflect the final version (with all the decisions and other changes in this PR), to be used as commit comment. |
|
LGTM, I like the function |
|
@itamarhaber it looks like the new tests you added are sensitive to timing
|
Fixes redis#7923. This PR appropriates the special `&` symbol (because `@` and `*` are taken), followed by a literal value or pattern for describing the Pub/Sub patterns that an ACL user can interact with. It is similar to the existing key patterns mechanism in function (additive) and implementation (copy-pasta). It also adds the allchannels and resetchannels ACL keywords, naturally. The default user is given allchannels permissions, whereas new users get whatever is defined by the acl-pubsub-default configuration directive. For backward compatibility in 6.2, the default of this directive is allchannels but this is likely to be changed to resetchannels in the next major version for stronger default security settings. Unless allchannels is set for the user, channel access permissions are checked as follows : * Calls to both PUBLISH and SUBSCRIBE will fail unless a pattern matching the argumentative channel name(s) exists for the user. * Calls to PSUBSCRIBE will fail unless the pattern(s) provided as an argument literally exist(s) in the user's list. Such failures are logged to the ACL log. Runtime changes to channel permissions for a user with existing subscribing clients cause said clients to disconnect unless the new permissions permit the connections to continue. Note, however, that PSUBSCRIBErs' patterns are matched literally, so given the change bar:* -> b*, pattern subscribers to bar:* will be disconnected. Notes/questions: * UNSUBSCRIBE, PUNSUBSCRIBE and PUBSUB remain unprotected due to lack of reasons for touching them.
…nels permissions by default. (#8723) Background: Redis 6.2 added ACL control for pubsub channels (#7993), which were supposed to be permissive by default to retain compatibility with redis 6.0 ACL. But due to a bug, only newly created users got this `acl-pubsub-default` applied, while overwritten (updated) users got reset to `resetchannels` (denied). Since the "default" user exists before loading the config file, any ACL change to it, results in an update / overwrite. So when a "default" user is loaded from config file or include ACL file with no channels related rules, the user will not have any permissions to any channels. But other users will have default permissions to any channels. When upgraded from 6.0 with config rewrite, this will lead to "default" user channels permissions lost. When users are loaded from include file, then call "acl load", users will also lost channels permissions. Similarly, the `reset` ACL rule, would have reset the user to be denied access to any channels, ignoring `acl-pubsub-default` and breaking compatibility with redis 6.0. The implication of this fix is that it regains compatibility with redis 6.0, but breaks compatibility with redis 6.2.0 and 2.0.1. e.g. after the upgrade, the default user will regain access to pubsub channels. Other changes: Additionally this commit rename server.acl_pubusub_default to server.acl_pubsub_default and fix typo in acl tests.
…nels permissions by default. (redis#8723) Background: Redis 6.2 added ACL control for pubsub channels (redis#7993), which were supposed to be permissive by default to retain compatibility with redis 6.0 ACL. But due to a bug, only newly created users got this `acl-pubsub-default` applied, while overwritten (updated) users got reset to `resetchannels` (denied). Since the "default" user exists before loading the config file, any ACL change to it, results in an update / overwrite. So when a "default" user is loaded from config file or include ACL file with no channels related rules, the user will not have any permissions to any channels. But other users will have default permissions to any channels. When upgraded from 6.0 with config rewrite, this will lead to "default" user channels permissions lost. When users are loaded from include file, then call "acl load", users will also lost channels permissions. Similarly, the `reset` ACL rule, would have reset the user to be denied access to any channels, ignoring `acl-pubsub-default` and breaking compatibility with redis 6.0. The implication of this fix is that it regains compatibility with redis 6.0, but breaks compatibility with redis 6.2.0 and 2.0.1. e.g. after the upgrade, the default user will regain access to pubsub channels. Other changes: Additionally this commit rename server.acl_pubusub_default to server.acl_pubsub_default and fix typo in acl tests.
Fixes #7923.
This PR appropriates the special
&symbol (because@and*are taken), followed by a literal value or pattern for describing the Pub/Sub patterns that an ACL user can interact with. It is similar to the existing key patterns mechanism in function (additive) and implementation (copy-pasta). It also adds theallchannelsandresetchannelsACL keywords, naturally.The default user is given
allchannelspermissions, whereas new users get whatever is defined by the 'acl-pubsub-default' configuration directive. For backward compatibility in 6.2, the default of this directive isallchannelsbut this is likely to be changed toresetchannelsin the next major version for stronger default security settings.Unless
allchannelsis set for the user, channel access permissions are checked as follows :PUBLISHandSUBSCRIBEwill fail unless a pattern matching the argumentative channel name(s) exists for the user.PSUBSCRIBEwill fail unless the pattern(s) provided as an argument literally exist(s) in the user's list.Runtime changes to channel permissions for a user with existing subscribing clients cause said clients to disconnect unless the new permissions permit the connections to continue. Note, however, that
PSUBSCRIBErs' patterns are matched literally, so given the changebar:*->b*, pattern subscribers tobar:*will be disconnected.Notes/questions:
UNSUBSCRIBE,PUNSUBSCRIBEandPUBSUBremain unprotected due to lack of reasons for touching them.To do:
nochannelsandnokeysas aliases/instead