Fix "default" and overwritten / reset users will not have pubsub channels permissions by default. by huangzhw · Pull Request #8723 · redis/redis

huangzhw · 2021-03-30T10:49:05Z

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

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. Additionally this commit rename server.acl_pubusub_default to server.acl_pubsub_default and fix typo in acl tests.

oranagra

@itamarhaber FYI

oranagra · 2021-03-30T20:29:01Z

src/acl.c

            u = ACLGetUserByName(argv[1],sdslen(argv[1]));
            serverAssert(u != NULL);
            ACLSetUser(u,"reset",-1);
+            u->flags = u->flags | server.acl_pubsub_default;


@huangzhw do you think maybe it would be better to move this into ACLSetUser("reset")?

the docs say this about "reset":

The user returns to the same state it has immediately after its creation.

@oranagra Thie will be a bigger step. If we add this in reset, the behavior will not be compatible with 6.2. Although it will conform to the docs. WDYT which is preferred?

Another way this bug affect not default user:

127.0.0.1:6380> auth hello hello OK 127.0.0.1:6380> publish hello world (integer) 0 127.0.0.1:6380> acl load OK 127.0.0.1:6380> publish hello world (error) NOPERM this user has no permissions to access one of the channels used as arguments

I think this is a bug we must fix.
it'll change the (buggy) behavior, but i don't see any other way.
i don't think we can document that buggy behavior and keep it (your example above is a clear indication of that).

if we did it'll look something like

when creating a new user that didn't exist before, acl-pubsub-default is respected, but when overriding an existing user, or the default user, all channels are denied unless explicitly allowed.

same goes for reset:

reset Performs the following actions: resetpass, resetkeys, off, resetchannels,
-@all. The user returns to the same state it has immediately
after its creation (except for publish channels that would be denied even when acl-pubsub-default allows them)

this makes no sense.

@huangzhw , while we're waiting for the core team's approval, if you agree that reset should do that, let's make that change in the PR.
i think it'll conform with the docs, and also be more compatible with redis 6.0.

I suppose something like this should do the trick:

serverAssert(ACLSetUser(u,"resetkeys",-1) == C_OK); serverAssert(ACLSetUser(u,"resetchannels",-1) == C_OK); + if (server.acl_pubusub_default & USER_FLAG_ALLCHANNELS) + serverAssert(ACLSetUser(u,"allchannels",-1) == C_OK);

@oranagra Agree. Change made. And I add more tests.

oranagra · 2021-03-30T21:00:42Z

@redis/core-team this is a behavior change, please approve or suggest alternative.
had this been released as part of 6.2.0 GA, it would have been great, but fixing it now can a behavior change.
question is if a mention in the release notes is enough, or do we have other alternatives?

when considering the behavior change, please consider my suggestion for applying this fix on the reset ACL statement.

itamarhaber · 2021-04-04T12:43:10Z

Thanks @huangzhw - this is an important fix and I agree with @oranagra's reset suggestion.

permissons of channels after reset with different config.

yossigo · 2021-04-04T20:32:16Z

Given that this is a new bug in a new feature just recently released, I think it makes total sense to fix it at the cost of a breaking change.

In the initial release of Redis 6.2 setting a user to only allow pubsub access to a specific channel, and doing ACL SAVE, resulted in an assertion when ACL LOAD was used. This was later changed by #8723 (not yet released), but still not properly resolved (now it errors instead of crash). The problem is that the server that generates an ACL file, doesn't know what would be the setting of the acl-pubsub-default config in the server that will load it. so ACL SAVE needs to always start with resetchannels directive. This should still be compatible with old acl files (from redis 6.0), and ones from earlier versions of 6.2 that didn't mess with channels. Co-authored-by: Harkrishn Patro <[email protected]> Co-authored-by: Oran Agra <[email protected]>

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

In the initial release of Redis 6.2 setting a user to only allow pubsub access to a specific channel, and doing ACL SAVE, resulted in an assertion when ACL LOAD was used. This was later changed by redis#8723 (not yet released), but still not properly resolved (now it errors instead of crash). The problem is that the server that generates an ACL file, doesn't know what would be the setting of the acl-pubsub-default config in the server that will load it. so ACL SAVE needs to always start with resetchannels directive. This should still be compatible with old acl files (from redis 6.0), and ones from earlier versions of 6.2 that didn't mess with channels. Co-authored-by: Harkrishn Patro <[email protected]> Co-authored-by: Oran Agra <[email protected]>

oranagra added the release-notes indication that this issue needs to be mentioned in the release notes label Mar 30, 2021

oranagra previously approved these changes Mar 30, 2021

View reviewed changes

oranagra added the approval-needed Waiting for core team approval to be merged label Mar 30, 2021

oranagra added the state:major-decision Requires core team consensus label Mar 30, 2021

Move set allchannels to ACLSetUser("reset") and add tests to test

51630f0

permissons of channels after reset with different config.

huangzhw dismissed oranagra’s stale review via 51630f0 April 4, 2021 14:35

oranagra approved these changes Apr 4, 2021

View reviewed changes

yossigo approved these changes Apr 4, 2021

View reviewed changes

oranagra approved these changes Apr 5, 2021

View reviewed changes

madolson approved these changes Apr 5, 2021

View reviewed changes

oranagra merged commit 3b74b55 into redis:unstable Apr 5, 2021

oranagra changed the title ~~Fix "default" user will not have channels permissions by default.~~ Fix "default" and overwritten / reset users will not have pubsub channels permissions by default. Apr 5, 2021

huangzhw deleted the acl branch April 5, 2021 23:09

michilehr mentioned this pull request Apr 13, 2021

Slave has flag disconnected but master link status is up #8778

Closed

This was referenced Apr 18, 2021

ACL channels permission handling for save/load scenario. #8794

Merged

ACL LOAD fails on files generated by ACL SAVE when specific channels are allowed #8811

Closed

oranagra mentioned this pull request Apr 19, 2021

Release 6.2.2 #8817

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix "default" and overwritten / reset users will not have pubsub channels permissions by default.#8723

Fix "default" and overwritten / reset users will not have pubsub channels permissions by default.#8723
oranagra merged 2 commits intoredis:unstablefrom
huangzhw:acl

huangzhw commented Mar 30, 2021 •

edited by oranagra

Loading

Uh oh!

oranagra left a comment

Uh oh!

oranagra Mar 30, 2021

Uh oh!

huangzhw Mar 30, 2021 •

edited

Loading

Uh oh!

oranagra Mar 31, 2021 •

edited

Loading

Uh oh!

oranagra Apr 4, 2021

Uh oh!

huangzhw Apr 4, 2021

Uh oh!

oranagra commented Mar 30, 2021 •

edited

Loading

Uh oh!

itamarhaber commented Apr 4, 2021

Uh oh!

yossigo commented Apr 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

huangzhw commented Mar 30, 2021 • edited by oranagra Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

oranagra Mar 30, 2021

Choose a reason for hiding this comment

Uh oh!

huangzhw Mar 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oranagra Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oranagra Apr 4, 2021

Choose a reason for hiding this comment

Uh oh!

huangzhw Apr 4, 2021

Choose a reason for hiding this comment

Uh oh!

oranagra commented Mar 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

itamarhaber commented Apr 4, 2021

Uh oh!

yossigo commented Apr 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

huangzhw commented Mar 30, 2021 •

edited by oranagra

Loading

huangzhw Mar 30, 2021 •

edited

Loading

oranagra Mar 31, 2021 •

edited

Loading

oranagra commented Mar 30, 2021 •

edited

Loading