Set default channel permission to resetchannels for 7.0

[hpatro]

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]

@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 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]

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]

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

[hpatro]

@yossigo @soloestoy Any thoughts ?

[soloestoy]

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]

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]

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]

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]

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