Skip to content

Conversation

@itamarhaber
Copy link
Member

@itamarhaber itamarhaber commented Oct 30, 2020

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 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.

To do:

  • Decide whether to add/use nochannels and nokeys as aliases/instead

@itamarhaber itamarhaber added state:major-decision Requires core team consensus state:needs-doc-pr requires a PR to redis-doc repository labels Oct 30, 2020
@asleire
Copy link

asleire commented Oct 31, 2020

Thanks a lot for working on this! I agree with your choices in notes.

The default user is given allchannels permissions, whereas new users get nochannels by default. Unless allchannels is set for the user, channel access permissions are checked as follows

Will this cause existing users (when updating Redis) to lose access to pub/sub functionality?

@itamarhaber
Copy link
Member Author

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).

@asleire
Copy link

asleire commented Nov 1, 2020

Have you considered letting users have access to all channels unless explicitly restricted?
Pro: Avoid breaking change
Con: Default pattern ACL would differ from default keys ACL -- but that's already the case

@itamarhaber
Copy link
Member Author

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 :)

@asleire
Copy link

asleire commented Nov 1, 2020

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

@itamarhaber
Copy link
Member Author

You've convinced me - let's wait for the @redis/core-team's feedback on this.

@madolson
Copy link
Contributor

madolson commented Nov 3, 2020

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.

Copy link
Member

@oranagra oranagra left a 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:

  1. PUBSUB and UNSUBSCRIBE don't need to be changed.
  2. If the permissions changed after subscription, do nothing about it.
  3. 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.

@soloestoy
Copy link
Contributor

soloestoy commented Nov 24, 2020

One question: should we disconnect subscribers when changing the ACL pubsub rules, or should we check ACL pubsub rules in pubsubPublishMessage to publish a channel to an specific subscriber to avoid the subscriber's ACL rule changed?

Copy link
Contributor

@madolson madolson left a 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.

@itamarhaber
Copy link
Member Author

The top comment has been updated to reflect the current.

@redis/core-team please review and approve for 6.2 RC1 :)

Copy link
Collaborator

@yossigo yossigo left a 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>
oranagra
oranagra previously approved these changes Nov 29, 2020
@oranagra
Copy link
Member

@itamarhaber i noticed there are two checkboxes at the top not checked

@oranagra
Copy link
Member

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.

@soloestoy
Copy link
Contributor

LGTM, I like the function ACLKillPubsubClientsIfNeeded, which can kill clients only affected by channel reduce.

@itamarhaber itamarhaber merged commit c1b1e8c into redis:unstable Dec 1, 2020
@oranagra
Copy link
Member

oranagra commented Dec 2, 2020

@itamarhaber it looks like the new tests you added are sensitive to timing
https://github.com/redis/redis/runs/1483449657?check_suite_focus=true
can you please look into it and fix?
it's probably that you need to either the actions that are run by a deferred client don't get executed before the next command.
solutions in order of preference:

  1. $rd read (if possible)
  2. wait_for_condition on some server info / stat
  3. $rd flush and after 100

@oranagra oranagra added the release-notes indication that this issue needs to be mentioned in the release notes label Dec 9, 2020
@oranagra oranagra mentioned this pull request Jan 13, 2021
JackieXie168 pushed a commit to JackieXie168/redis that referenced this pull request Mar 2, 2021
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.
oranagra pushed a commit that referenced this pull request Apr 5, 2021
…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.
JackieXie168 pushed a commit to JackieXie168/redis that referenced this pull request Sep 8, 2021
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus state:needs-doc-pr requires a PR to redis-doc repository

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[NEW] ACL pattern support for pub/sub

6 participants