redis-operator Default user with too many privileges

Expected behaviour

There needs to be an admin user for the use in administrative operations. And we could make the default user -@dangerous.

Actual behaviour

The default user is has global privileges and this might cause issue if there is compromize of the application

Steps to reproduce the behaviour

Login to the shell redis-cli with connection to the master redis pod using default user and issue...

FLUSHALL

This will delete everything. !!!DANGEROUS!!!

Sep 01 '22 07:09 samof76

@ese in fact in the next release of the redis...

# Command renaming (DEPRECATED).
#
# ------------------------------------------------------------------------
# WARNING: avoid using this option if possible. Instead use ACLs to remove
# commands from the default user, and put them only in some admin user you
# create for administrative purposes.
# ------------------------------------------------------------------------

Sep 01 '22 16:09 samof76

@ese I am starting to work on this feature, here is excerpt....

redisops-admin will the admin user with -@all +@admin with privileges
Defaults to nopass but if the password is passed through auth.secretPath
default user will not be touched by either the operator or the sentinel
default user ACL could be updated using the init container or left alone

And by default default user will be -@admin

Sep 06 '22 10:09 samof76

@ese do let me know how you feel about this?

Sep 06 '22 11:09 samof76

@dmamolina your thoughts too.

Sep 06 '22 11:09 samof76

I agree in principle, but there is a lot of work needed with the operator to make it multi-user and ACL aware. Not to mention it would then break all Redis <5.x deployments, as ACL is not supported there.

If someone wants to tackle adding ACL support to the operator, that'd make me very happy!

Sep 06 '22 17:09 szelenka

@szelenka if we are still supporting the 5.x of redis, then this change #430 will definitely not work. But we are taking a call that we will only support north of 6.x then I would be really happy make the changes for ACL.

Anyways I will start working on the preliminary changes, and send in a PR.

cc: @ese

Sep 07 '22 03:09 samof76

So the approach i am going to take is that...

  auth:
    redisOpsAdmin: kungfoo
    secretPath: foopass

if we do not set the redisOpsAdmin it will default to default and no ACLs will applied, and use secretPath(if set) to set the default user password
if we do pass kungfoo(or anything other than default) we would set -@all +@admin on the kungfoo set the password from foopass. And in the case default could be managed independently, thinking a initContainer with volume mounts could manage users.

Sep 07 '22 04:09 samof76

But a more elaborate plan for ACL would be to have something likes this...

  auth
  - name: user1
    password:
      valueFrom:
      ...
    acl:
       valueFrom:
      ...
  - name: user2
 ...

But that depends on the call we would want to take.

Sep 07 '22 04:09 samof76

I like your second approach. This would allow for multiple users with various roles, and also allow the admin to rotate user passwords by updating the k8s secret.

The thing that we got caught up on previously, was how does one rotate the redisOpsAdmin password from the Operator? We'd need (1) the existing password to authenticate to the redis cluster and (2) the desired new password. Somewhere in the CRD we'd need to specify this.

Zalando has something similar with Postgres, where the Operator manages this rotation and updating k8s secrets: https://github.com/zalando/postgres-operator/blob/master/docs/administrator.md#password-rotation-in-k8s-secrets

Sep 07 '22 14:09 szelenka

@szelenka please check if the change i taking is in the right direction

Sep 08 '22 07:09 samof76

  auth:
    admin:
      name: admin1
      passwords:
        - valueFrom(or value)
        - <multiple for the sake of rotation>
      acl:
    users:
    - name: foo
      passwords:
        - valueFrom(or value)
        - <multiple for the sake of rotation>
      acl: "+@all -@dangerous"
    ...
    ...

Sep 08 '22 07:09 samof76

Thanks @samof76,

I am still thinking about this feature but I have quick notes for the moment:

Redis 6 is the minimum version required for the operator. I will add a note in the README for it. It's better to focus on the last versions than wide support on versions since we don't have either a lot of bandwidth
Managing multiple users and securing the deployment by default is a very good feature IMHO. I would mantain certain isolation logic between redisops-admin, the user to manage the instances by the operator, and the other users.
What want you get regarding with password rotation?
- Have the operator rotating the password at intervals updating secret manifests in kubernetes along with the user password in redis?
- Have a mechanisms to allow users rotating the password updating the redis-failover manifests?
I think that we should not break the redis-failover spec so we should add a new field instead changing the spec of the current auth field

Sep 08 '22 14:09 ese

@ese

RE: password rotation, I'm in the middle of figuring out how to satisfy this STIG (without LDAP): https://www.stigviewer.com/stig/redis_enterprise_6.x/2021-11-23/finding/V-251428

This essentially suggests the operator has a policy for lifetime of accounts used to connect to Redis. In our case these are all non-interactive accounts:

Password lifetime limits for non-interactive accounts: Minimum 24 hours, maximum 365 days
Number of password changes before an old one may be reused: Minimum of five

The challenge with the operator, is that we'd need some sort of grace-period for the services using these accounts to migrate to the new account/password. It'd be nice if we could specify these in the CRD:

password-rotation-interval : between 24h and 356d
password-rotation-grace-period : < 5d

Then have the operator responsible for populating the non-interactive account passwords and storing them in a k8s Secret as blue/green approach. Where each user has two accounts (blue & green) stored in the same k8s Secret:

apiVersion: v1
kind: Secret
type: Opaque
metadata:
  name: redis-operator-user-prefix
  ownerReference:
  - ...
data:
  username: user-prefix-N-b
  password: XXX

When it comes time to rotate the password, it'd need to happen at (rotation-interval - grace-period), which would update the Secret with the newly generated password from the redis-operator.

When paired with stakater/Reloader this would gracefully rotate all Pods which have this Secret attached. Where the application would then read the new settings when it restarts; and the Pods which have not restarted (yet) will still be able to connect with the old credentials until the grace-period expires.

Then after grace-period expires, the operator would delete the previous account from Redis. For Pods which have not restarted to pick up the new account/password, they will get AUTH errors. The operator would actually be rotating accounts each time the password needs to be rotated.. which works fine for apps connecting from within k8s, but apps outside may have difficultly knowing when the user/pass has been changed.

Sep 08 '22 16:09 szelenka

@ese @szelenka so this is what i have come up for the auth section...

  auth:
    secretPath: redis-auth
    admin:
      name: admin1
      passwords:
        - valueFrom(or value)
        - <multiple for the sake of rotation>
      acl:
    users:
    - name: foo
      passwords:
        - valueFrom(or value)
        - <multiple for the sake of rotation>
      acl: "+@all -@dangerous"
    ...
    ...

If auth is not set everything will work as it works now, nothing is disturbed
Keep the secretPath intact with deprecation warning, to ensure folks move to the latest model
If secretPath is set all else ignore and the code will just work as it works today, no change to any workflows All of the following assumes secretPath is not set
If admin segment is present, and default is the admin everything works like what its right now... no change.
If admin segment is present, and default is not the admin new admin is user for all operations, that admin will have the following ACL, set in the defaultAdminACL in the defaults.go

user mon on allkeys -@all +client +ping +info +config|get +cluster|info +slowlog +latency +memory +select +get +scan +xinfo +type +pfcount +strlen +llen +scard +zcard +hlen +xlen +eval +@admin >password1 >password2

The user does not have default set, we would assume default has nopass, with +@all -@dangerous ACL

Now over to the users, might require a little more thought around this and assumptions are made based on the fact that user will come and go(removing i have not though about as yet, any ideas are welcome)

User will be added at run time, with password and ACL using ACL setuser api.
Similar to CustomConfig section.

Sep 09 '22 06:09 samof76

Is it better to add this as a new map and say call it like auth_new and add a deprecation notice for auth?

Sep 09 '22 07:09 samitpal

@samitpal is there any benefit in doing so?

Sep 09 '22 07:09 samof76

From users perspective it might be more intuitive choosing between an old auth and new auth model?

Sep 09 '22 07:09 samitpal

@samitpal Yes would make sense to explicit, i agree makes sense to have authv2 instead of the auth_new, so the segments will look like the following...

  auth:
    secretPath: redis-auth
  authv2:
    admin:
      name: admin1
      passwords:
        - valueFrom(or value)
        - <multiple for the sake of rotation>
      acl: -@all +@admin ...(rest of the acl for replication)
    users:
    - name: foo
      passwords:
        - valueFrom(or value)
        - <multiple for the sake of rotation>
      acl: "+@all -@dangerous"
    ...
    ...

I also feel that since the admin acl is key to the operator as well, we should not make it explicit.

cc: @ese @szelenka

Sep 09 '22 07:09 samof76

From my point of view, there are a couple of features mentioned here that I would maintain isolated due to the complexity.

Redis auth management. Create the users and ACLs in the cluster
Password rotation. Generate passwords, rotation strategies, and exposure for clients. IMHO I would open other issue/discussion and pull requests for it.

Regarding the spec I like the approach of maintaining the auth field adding the new parameters and deprecating secretPath I mostly agree with: https://github.com/spotahome/redis-operator/issues/457#issuecomment-1241545430

Just a comment

Admin user as mandatory and required by the operator must not allow configuring more than the password. Why allow setting the ACLs and name? This user is intended to allow the operator to manage the cluster and should not be used by users or services. Anyway, I'm not strong against allowing that definition.
Users are the configurable list for all the use cases you want to use your Redis cluster, this can be users for people and services with a variety of ACLs

something like this:

  auth:
    secretPath: redis-auth
    admin:
      passoword:
        value: 
        valueFrom:
    users:
    - name: foo
      password:
        value: 
        valueFrom:
      acl: "+@all -@dangerous"
    ...
    ...

Sep 15 '22 21:09 ese

To post my 2 cents here.. I haven't read the whole thread, but wouldn't it be possible to introduce CRD's for this purpose? For example, a RedisFailoverUser.

Personally I'm not a fan of "authv2" and introducing a CRD aligns with decoupled user management of the RedisFailover CRD. This way you're very easily able to deploy a new user, w/o updating the RedisFailover manifest.