Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: ACL with username #796

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

wkd-woo
Copy link
Contributor

@wkd-woo wkd-woo commented Mar 3, 2024

Description

When you apply ACLs to the from redis 6.0 and later, you can not only password the redis-server itself, but also fine-tune access permissions to users

However, there is currently no setting for acluser in redis-operator (v0.15+).

If you grant limited privileges to the default and set up an acluser with master-auth, the operator will fail to reconceal with NO AUTH.

User permissions have been added to resolve this issue.

implements(i.e. Fixes #123) -->
Fixes #ISSUE
#688
#749

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • Tests have been added/modified and all tests pass.
  • Functionality/bugs have been confirmed to be unchanged or fixed.
  • I have performed a self-review of my own code.
  • Documentation has been updated or added where necessary.

Additional Context

In actual production environments, depending on the workload, there are times when all rights are not granted to the defunct user.

For example, I work as a red engineer at a cloud service provider company in Korea, and we limit to the application development team on commands that can change engine-level tuning or topology other than DML (e.g, cluster add-node)

To that end, ACLs are used to separate accounts with master-auth, and the development team provides a redis operating environment so that it can be accessed with a default account.

However, the shape of the redis-operator currently only provides the function of locking the server itself with a 'requirepass' for the redis-server by setting only a pass word.

In the actual production operation environment, accounts with master-auth may be separately provided as in my case. (In fact, in the case of cloud service provider companies where I work, this separation of authority is also a guide to security requirements as a csp operator.)

However, in the current form of the operator, there is no function for the ACL user, so only the default user accesses the redis-server and fails reconiling due to the AUTH problem.

127.0.0.1:6379> acl list
1) "user default on nopass sanitize-payload ~* &* +@all -@admin -flushdb -keys +replconf -migrate +pfselftest +pfdebug -restore-asking -sort -flushall +client|unpause +client|unblock +client|kill +client|list +client|no-evict +sync +psync +slowlog|reset +slowlog|len +slowlog|get +monitor +lastsave +latency|graph +latency|doctor +latency|reset +latency|histogram +latency|latest +latency|history"
2) "user redisadmin on < blabla > ~* &* +@all" 

I am currently introducing this project into our operating environment, and I had to add the 'ACLuser' function to do so.

I hope the additional features I developed contribute to this project, and I make a pull request to constantly update the master branch on the redis-operator deployed in my operating environment.

One problem is that 'v1beta3' and 'crd' were not defined separately, but were modified to 'v1beta1' and 'v1beta2'.

I thought about declaring 'v1beta3' separately, but I made that choice because I felt burdened by rewriting the entire API specification. I will follow the opinion of the maintainers on this project.

TEST

before Failover

$ k exec -it pod/redis-replication-0 -- redis-cli --user <masteruser> --pass <masterpass> ROLE
Defaulted container "redis-replication" out of: redis-replication, redis-exporter
Warning: Using a password with '-a' or '-u' option on the command line interface may not be safe.
1) "master"
2) (integer) 48406
3) 1) 1) "192.168.52.254"
      2) "6379"
      3) "48406"
   2) 1) "192.168.43.206"
      2) "6379"
      3) "48406" 

$ k exec -it pod/redis-replication-1 -- redis-cli --user <masteruser> --pass <masterpass> ROLE
Defaulted container "redis-replication" out of: redis-replication, redis-exporter
Warning: Using a password with '-a' or '-u' option on the command line interface may not be safe.
1) "slave"
2) "192.168.251.23"
3) (integer) 6379
4) "connected"
5) (integer) 42787


$ k delete pod/redis-replication-0
pod "redis-replication-0" deleted

can approach with ACL user and password

after Failover

$ k exec -it pod/redis-replication-0 -- redis-cli --user <masteruser> --pass <masterpass> ROLE
Defaulted container "redis-replication" out of: redis-replication, redis-exporter
Warning: Using a password with '-a' or '-u' option on the command line interface may not be safe.
1) "slave"
2) "192.168.52.254"
3) (integer) 6379 

$ k exec -it pod/redis-replication-1 -- redis-cli --user <masteruser> --pass <masterpass> ROLE
Defaulted container "redis-replication" out of: redis-replication, redis-exporter
Warning: Using a password with '-a' or '-u' option on the command line interface may not be safe.
1) "master"
2) (integer) 100139
3) 1) 1) "192.168.43.206"
      2) "6379"
      3) "99853"
   2) 1) "192.168.251.25"
      2) "6379"
      3) "100139"

failover by sentinel(of course, they know master-auth and master-pass) and reconciled successfully

@shubham-cmyk
Copy link
Member

@wkd-woo Can you rebase the branch and resolve the conflict

@wkd-woo wkd-woo force-pushed the feature/acl-username-password branch from 3a524c9 to 262c73d Compare March 3, 2024 15:35
@wkd-woo
Copy link
Contributor Author

wkd-woo commented Mar 3, 2024

@shubham-cmyk I did :)

@wkd-woo wkd-woo force-pushed the feature/acl-username-password branch from 262c73d to d6c2f46 Compare March 4, 2024 09:12
Image string `json:"image"`
ImagePullPolicy corev1.PullPolicy `json:"imagePullPolicy,omitempty"`
Resources *corev1.ResourceRequirements `json:"resources,omitempty"`
ExistingAuthSecret *ExistingAuthSecret `json:"redisSecret,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing the field name would produce a breaking change here.
we are good with adding field but don't rename any field

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So... do you think this part needs to be modified?
@shubham-cmyk

@shubham-cmyk shubham-cmyk changed the title Feature/acl: ACL with username feat: ACL with username Mar 4, 2024
Copy link

codecov bot commented Mar 4, 2024

Codecov Report

Attention: Patch coverage is 23.80952% with 112 lines in your changes are missing coverage. Please review.

Project coverage is 35.16%. Comparing base (d121d86) to head (1ee6fbb).
Report is 10 commits behind head on master.

Files Patch % Lines
k8sutils/cluster-scaling.go 0.00% 56 Missing ⚠️
k8sutils/redis.go 0.00% 34 Missing and 1 partial ⚠️
k8sutils/statefulset.go 45.16% 15 Missing and 2 partials ⚠️
k8sutils/redis-cluster.go 83.33% 1 Missing ⚠️
k8sutils/redis-replication.go 83.33% 1 Missing ⚠️
k8sutils/redis-sentinel.go 83.33% 1 Missing ⚠️
k8sutils/redis-standalone.go 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #796      +/-   ##
==========================================
- Coverage   35.20%   35.16%   -0.04%     
==========================================
  Files          19       19              
  Lines        3213     2724     -489     
==========================================
- Hits         1131      958     -173     
+ Misses       2015     1697     -318     
- Partials       67       69       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Shubham Gupta <[email protected]>
@shubham-cmyk
Copy link
Member

@wkd-woo

We can confiure the ACL user here by the user.acl file right now ?
I have example
also
i have verified that we can login in to redis using that username here

@wkd-woo
Copy link
Contributor Author

wkd-woo commented Mar 6, 2024

@wkd-woo

We can confiure the ACL user here by the user.acl file right now ? I have example also i have verified that we can login in to redis using that username here

@shubham-cmyk
You're right. I know that even in the current feature, application can access redis with username.

The rationale for my PR is that the operator should reconcile through a separate user with master-auth, not 'default user' in the redis-server.

There are also environments where governance is established to prevent default users from having masterauth.

@wkd-woo
Copy link
Contributor Author

wkd-woo commented Mar 6, 2024

user default on nopass sanitize-payload ~* &* +@all -@admin -flushdb +lastsave -sort +monitor -flushall +pfselftest +replconf +slowlog +psync -migrate -keys -restore-asking +sync +pfdebug +latency +client|help +client|caching +client|getname +client|getredir +client|id +client|info +client|kill +client|list +client|no-evict +client|reply +client|setname +client|tracking +client|trackinginfo +client|unblock +client|unpause

For example, our organization has a default user policy as above.
Based on the criteria of CSP DBaaS(like ElastiCache, Azure Cache, ...)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants