Skip to content

Set default channel permission to resetchannels for 7.0#10181

Merged
oranagra merged 3 commits into
redis:unstablefrom
hpatro:default_pubsub_channel_perm
Jan 30, 2022
Merged

Set default channel permission to resetchannels for 7.0#10181
oranagra merged 3 commits into
redis:unstablefrom
hpatro:default_pubsub_channel_perm

Conversation

@hpatro

@hpatro hpatro commented Jan 25, 2022

Copy link
Copy Markdown
Contributor

For backwards compatibility in 6.x, channels default permission was set to allchannels however with 7.0, we should modify it and the default value should be resetchannels for better security posture. Also, with selectors in ACL, a client doesn't have to set channel rules everytime and by default the value will be resetchannels.

This is a breaking change, users that are badly affected by it, can easily revert the config back to the old default.

Before this change

127.0.0.1:6379> acl list
1) "user default on nopass ~* &* +@all"
127.0.0.1:6379>  acl setuser hp on nopass +@all ~*
OK
127.0.0.1:6379> acl list
1) "user default on nopass ~* &* +@all"
2) "user hp on nopass ~* &* +@all"
127.0.0.1:6379>  acl setuser hp1 on nopass -@all (%R~sales*)
OK
127.0.0.1:6379> acl list
1) "user default on nopass ~* &* +@all"
2) "user hp on nopass ~* &* +@all"
3) "user hp1 on nopass &* -@all (%R~sales* &* -@all)"

After this change

127.0.0.1:6379> acl list
1) "user default on nopass ~* &* +@all"
127.0.0.1:6379> acl setuser hp on nopass +@all ~*
OK
127.0.0.1:6379> acl list
1) "user default on nopass ~* &* +@all"
2) "user hp on nopass ~* resetchannels +@all"
127.0.0.1:6379> acl setuser hp1 on nopass -@all (%R~sales*)
OK
127.0.0.1:6379> acl list
1) "user default on nopass ~* &* +@all"
2) "user hp on nopass ~* resetchannels +@all"
3) "user hp1 on nopass resetchannels -@all (%R~sales* resetchannels -@all)"

@oranagra

Copy link
Copy Markdown
Member

@hpatro isn't that a serious breaking change?
if we didn't want to do that in 6.2, why would we want to do it in 7.0?
just because it's a major version and we're allowed more breakage?

maybe we can find the middle ground for different default in selectors vs the root selector?
@redis/core-team PTAL

@hpatro

hpatro commented Jan 25, 2022

Copy link
Copy Markdown
Contributor Author

@hpatro isn't that a serious breaking change? if we didn't want to do that in 6.2, why would we want to do it in 7.0? just because it's a major version and we're allowed more breakage?

maybe we can find the middle ground for different default in selectors vs the root selector? @redis/core-team PTAL

@oranagra I agree this is a breaking change. I believe it's good to talk about it before the major release and come to a decision.

7.0 provides this opportunity to make this breaking change to increase the security posture of all newly created users. For customers updating from 6.0 to 7.0 using acl file shouldn't have any problem as the state of channel permission would already be persisted in the file.

@madolson

madolson commented Jan 25, 2022

Copy link
Copy Markdown
Contributor

I wanted to avoid different permissions on selectors vs root permissions, since I think that will just add more room for confusion. I think what a lot of people want to do is just write some permissions for a root user, and then wrap in parentheses and have it work the same way.

Since this is for security, and we're already introducing a bunch of backwards breaking changes to make it more secure by default (module/debug/config flags). I would be in favor of this change and make a stand that this is the version we care about security. We can also advise users to explicitly add this to their config file to retain the current behavior.

@itamarhaber

Copy link
Copy Markdown
Member

I agree with the reasoning to make it more secure and having consistent use of selectors.

@hpatro

hpatro commented Jan 26, 2022

Copy link
Copy Markdown
Contributor Author

@yossigo @soloestoy Any thoughts ?

@soloestoy

Copy link
Copy Markdown
Contributor

I agree it's better to make it more secure. But I feel uncomfortable about the breaking change, especially we did a lot of compatibility work to avoid breaking change in multi-part AOF feature.

@hpatro

hpatro commented Jan 27, 2022

Copy link
Copy Markdown
Contributor Author

Note: We can advise user(s) in the release notes to use acl-pubsub-default as allchannels to preserve the same behaviour as before.

@yossigo

yossigo commented Jan 27, 2022

Copy link
Copy Markdown
Collaborator

I'm also in favor of changing the default, despite being a breaking change. My arguments are:

  • In the long run, it's a more intuitive and secure setting - I think we agree on that.
  • We've already stated in c1b1e8c that this is our intention, and that acl-pubsub-default is introduced to ease the transition and avoid breakage in 6.2.
  • This only affects those users who use ACL with channels and don't happen to use an ACL file, and they can fix it with a single config change.

@soloestoy To me, this is different because:

  • AOF is a much more prevalent feature.
  • Breakage would not be immediately apparent (maybe only after some time the wrong files are backed up).
  • The impact can be much more significant (potential data loss).

@oranagra

Copy link
Copy Markdown
Member

I was initially against it, but after thinking about the argument Yossi presented, i changed my mind.

the bottom line here, is that like the blocking of DEBUG, MODULE and CONFIG SET dir, there's really no other solution; we have to choose between security and backwards compatibility. they simply contradict each other.

so either we leave it like it was forever, or we introduce a breaking when some day.
that day could be today (or actually early next week).
i just regret not starting that discussion months ago.

@oranagra oranagra added breaking-change This change can potentially break existing application 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 labels Jan 27, 2022
@oranagra

Copy link
Copy Markdown
Member

seems like we have a quorum, and out of time for 7.0 RC1.
merging this.

@oranagra oranagra merged commit a43b692 into redis:unstable Jan 30, 2022
@hpatro hpatro deleted the default_pubsub_channel_perm branch January 30, 2022 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking-change This change can potentially break existing application 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

Archived in project

Development

Successfully merging this pull request may close these issues.

6 participants