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

hpatro · 2021-04-15T18:58:36Z

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.

Scenario:

Create user with access to single channel.

127.0.0.1:6379> acl list
1) "user default on nopass sanitize-payload ~* +@all"
127.0.0.1:6379> acl setuser foo resetchannels &test
OK
127.0.0.1:6379> acl list
1) "user default on nopass sanitize-payload ~* +@all"
2) "user foo off &test -@all"

Persist to aclfile.

127.0.0.1:6379> acl save
OK

aclfile content after saving it.

cat /tmp/users.acl
user default on nopass sanitize-payload ~* +@all
user foo off &test -@all

Load the aclfile

127.0.0.1:6379> acl load

Possible scenarios:

Scenario 1:(acl-pubsub-default (allchannels) → default redis behaviour)
resetchannels -> acl-pubsub-default (allchannels)

"user icaro off resetchannels &test -@all"
Output: (error) ERR Error in ACL SETUSER modifier '&test': Adding a pattern after the * pattern (or the 'allchannels' flag) is not valid and does not have any effect. Try 'nochannels' to start with an empty list of channels

"user icaro off allchannels &test -@all"
Output: (error) ERR Error in ACL SETUSER modifier '&test': Adding a pattern after the * pattern (or the 'allchannels' flag) is not valid and does not have any effect. Try 'nochannels' to start with an empty list of channels

"user icaro off nochannels &test -@all" --> work(only &test permission)
Output: "user icaro off nochannels &test -@all"

"user icaro off -@all" --> work(default behaviour -> all permissions for channels)
Output: "user icaro off *& -@all"

"user icaro &* off -@all" --> work(all permissions for channels)
Output: "user icaro off *& -@all"

"user icaro allchannels off -@all" --> work(all permissions for channels)
Output: "user icaro off *& -@all"

"user icaro resetchannels off -@all" --> work(default behaviour -> all permissions for channels)
Output: "user icaro off *& -@all"

"user icaro nochannels off -@all" --> work(no channels permissions)
Output: "user icaro off nochannels -@all"
 
Scenario 2:(acl-pubsub-default (nochannels))
acl-pubsub-default nochannels

resetchannels -> acl-pubsub-default (nochannels)

"user icaro off resetchannels &test -@all" --> work(only &test permission)
Output: "user icaro off nochannels &test -@all"

"user icaro off allchannels &test -@all"
Output: (error) ERR Error in ACL SETUSER modifier '&test': Adding a pattern after the * pattern (or the 'allchannels' flag) is not valid and does not have any effect. Try 'nochannels' to start with an empty list of channels

"user icaro off nochannels &test -@all" --> work(only &test permission)
Output: "user icaro off nochannels &test -@all"

"user icaro off -@all" --> work(default behaviour -> no channels permissions)
Output: "user icaro off nochannels -@all"

"user icaro &* off -@all" --> work(all permissions for channels)
Output: "user icaro off *& -@all"

"user icaro allchannels off -@all" --> work(all permissions for channels)
Output: "user icaro off *& -@all"

"user icaro resetchannels off -@all" --> work(default behaviour -> no channels permissions)
Output: "user icaro off nochannels -@all"

"user icaro nochannels off -@all" --> work(no channels permissions)
Output: "user icaro off nochannels -@all"

hpatro · 2021-04-15T18:59:01Z

@madolson @itamarhaber Please take a look.

src/acl.c

src/config.c

src/server.h

src/config.c

madolson · 2021-04-15T23:44:38Z

This change also broke another test

src/acl.c

huangzhw · 2021-04-16T13:52:30Z

Currently resetchannels means removes all channel patterns from the list of Pub/Sub channel patterns the user can access, not reset to default. I think main cause of this bug is when acl-pubsub-default allchannels, the default is allchannels, but the ACLDescribeUser thinks resetchannels is default. It will not output resetchannels. So if we always output resetchannels it will be OK.
Or I misunderstand and miss something?

hpatro · 2021-04-16T14:24:36Z

@huangzhw Yeah the idea is similar, but we should output nochannels as none of the other similar values are stored/output i.e. reset or resetkeys each of these moves the user to the default state. Hence, upon calling resetchannels we should be outputting nochannels instead of resetchannels.
Does this makes sense?

huangzhw · 2021-04-17T08:54:08Z

Yes, I understand. Is there any principles that reset* can't be stored/output?

oranagra · 2021-04-18T16:53:04Z

sorry to join the party late, not sure what i missed but i wanna point out:

in the unstable branch this doesn't crush the server anymore, that was fixed by Fix "default" and overwritten / reset users will not have pubsub channels permissions by default. #8723. so it seems that the details in the comments, and maybe the design of the solution is outdated.
acl load now fails with an error (instead of crashing).
we must obviously fix this ASAP (blocker for v6.2.2)
resetchannels specifies that it removes all channels (and that's what it did in both 6.2 and unstable). i don't think we wanna change it's meaning or add a new nochannels. *note that the reset directive is currently setting the default, i.e. respecting acl-pubsub-default).

I think the difficulty resolving the remaining issue is that the mechanism of loading ACL file doesn't necessarily knows the server configuration (acl-pubsub-default) at the time that ACL file was saved (if at all it was generated and not manually crafted), and it may have different value in the server that loads the acl file.

so i think we need to choose between:

the acl file will be very explicit (i.e. acl save always store either allchannels or nochannels in each line). i think this is compatible with old files, which are executed after a reset, so it'll respect the current value of acl-pubsub-default config (assuming the acl file doesn't mess with channels at all).
loosen the error validation in some way (in the spirit of Remove acl subcommand validation if fully added command exists. #8483), in the sense that loading an ACL file will respect the content of the acl file and try to close the gap on some contradictions.

i prefer 1. but maybe we'll need 2. too in order to gracefully handle acl files generated by earlier versions of redis 6.2.
let me know if i'm overlooking something.

oranagra · 2021-04-18T20:33:26Z

@madolson @hpatro @huangzhw @itamarhaber due to the urgency of the matter (we planned on releasing 6.2.2 tomorrow), and since i'm not fully sure if i'm missing anything in this PR, i created a simplified one based on this one, in which i reverted all the interface changes and just kept the new tests and the modification to ACLDescribeUser (to always use resetchannels).
please have a look at https://github.com/redis/redis/pull/8811/files

if we're all happy, i'll push that commit here close the other PR.

huangzhw · 2021-04-18T22:44:36Z

@oranagra #8811 is exactly what I mean in previous replies.
The main cause of this bug is that resetchannels is not default behavior (based on acl-pubsub-default).
Other reset* is default behavior when users are created, so no output is OK.

hpatro · 2021-04-19T08:22:38Z

@oranagra The concern which I had was resetchannels is ambiguous. reset means to move something to default state. And the way channels have been implemented, it means the user should move to acl-pubsub-default state i.e. allchannels by default.
This was the motivation for introduction of nochannels which removes all of the ambiguity around it.

With the suggested PR, we will still be in a state if a user applies resetchannels it moves to no access to channels state irrespective of acl-pubsub-default value.

oranagra · 2021-04-19T08:42:38Z

@hpatro thanks for clearing that out, but i think that resetchannels must remain as it was (denying all channels), since it was how it was both implemented and documented, so changing it is a bigger breaking-change (for someone not using ACL SAVE).

this acl-pubsub-default feature is indeed odd and it makes certain things behave differently than one would imagine. i wish we had ACL for pubsub in the initial redis 6.0 and we didn't have to add that feature..

but anyway i think it is still be clearer that resetchannels works similarly to resetkeys, in the sense that it removes all access. and not doing a logical "reset" to default behavior.
i.e. i think that would also been a good choice even if we had this discussion before releasing 6.2 and being obligated to backwards compatibility with it.

anyway, we're a bit short in time. do you see any problem with what's in the current form of the other PR?

hpatro · 2021-04-19T08:48:26Z

@oranagra Apart from that the PR looks fine to me.

oranagra · 2021-04-19T09:25:22Z

@hpatro i pushed the commits into this PR, and edited the top comment to reflect the change and understandings.

I'll use this top comment (first part of it) as the squash-merge commit comment, and also refer to it in the release notes.

please have a look and edit to improve.

hpatro · 2021-04-19T09:51:28Z

@oranagra LGTM.

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 <harkrisp@amazon.com> Co-authored-by: Oran Agra <oran@redislabs.com>

ACL channels permission handling for save/load scenario.

b839e21

madolson reviewed Apr 15, 2021

View reviewed changes

src/acl.c Outdated Show resolved Hide resolved

src/config.c Outdated Show resolved Hide resolved

src/server.h Outdated Show resolved Hide resolved

src/config.c Show resolved Hide resolved

huangzhw reviewed Apr 16, 2021

View reviewed changes

src/acl.c Outdated Show resolved Hide resolved

Default user state to be not affected by acl-pubsub-default.

c175de2

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

revert interface changes

a30770e

oranagra mentioned this pull request Apr 18, 2021

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

Closed

typo

93828ad

oranagra approved these changes Apr 19, 2021

View reviewed changes

oranagra merged commit 7a3d148 into redis:unstable Apr 19, 2021

oranagra mentioned this pull request Apr 19, 2021

Release 6.2.2 #8817

Merged

slavak mentioned this pull request May 21, 2023

Don't disconnect all clients in ACL LOAD #12171

Merged

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

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

Uh oh!

Conversation

hpatro commented Apr 15, 2021 • edited by oranagra Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hpatro commented Apr 15, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

madolson commented Apr 15, 2021

Uh oh!

Uh oh!

huangzhw commented Apr 16, 2021

Uh oh!

hpatro commented Apr 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

huangzhw commented Apr 17, 2021

Uh oh!

oranagra commented Apr 18, 2021

Uh oh!

oranagra commented Apr 18, 2021

Uh oh!

huangzhw commented Apr 18, 2021

Uh oh!

hpatro commented Apr 19, 2021

Uh oh!

oranagra commented Apr 19, 2021

Uh oh!

hpatro commented Apr 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oranagra commented Apr 19, 2021

Uh oh!

hpatro commented Apr 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hpatro commented Apr 15, 2021 •

edited by oranagra

Loading

hpatro commented Apr 16, 2021 •

edited

Loading

hpatro commented Apr 19, 2021 •

edited

Loading