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(argo-cd): add authentication for builtin Redis #2616

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

afrimberger
Copy link

@afrimberger afrimberger commented Mar 30, 2024

This PR introduces authentication for the builtin Redis. It can be enabled by setting redis.auth.enabled=true. The secrets are auto-generated and stored in a secret.

Checklist:

  • I have bumped the chart version according to versioning
  • I have updated the documentation according to documentation
  • I have updated the chart changelog with all the changes that come with this pull request according to changelog.
  • Any new values are backwards compatible and/or have sensible default.
  • I have signed off all my commits as required by DCO.
  • My build is green (troubleshooting builds).

@@ -0,0 +1,60 @@
# lookup existing secret
{{- $secretName := include "argo-cd.redis.fullname" . -}}
{{- $secretObj := (lookup "v1" "Secret" .Release.Namespace $secretName) | default dict }}
Copy link
Member

Choose a reason for hiding this comment

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

Using lookup inside the Argo ecosystem is IMHO a bad idea. Argo CD uses helm only as a template engine (argocd only executes helm template ...).
And helm template does not support looking up values for security reasons.

Copy link
Author

@afrimberger afrimberger Mar 31, 2024

Choose a reason for hiding this comment

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

Hi @mkilchhofer, Thanks for bringing up this topic! If we're talking about ArgoCD in general, I fully agree. However, in this particular case the Helm Chart is meant for bootstrapping ArgoCD. Basically, this can be achieved in two ways:

  1. Install directly via Helm -> Lookup works
  2. ArgoCD manages ArgoCD -> ignoreDifferences for the password fields needs to be specified

I would really love to see a more secure default installation of ArgoCD. Especially, because not all clusters support NetworkPolicies out of the box (e.g. Flannel doesn't).

I suggest to add an example for ignoreDifferences and some explanation to the README to address the questions arising by using lookup. WDYT?

Copy link
Member

@mkilchhofer mkilchhofer Apr 1, 2024

Choose a reason for hiding this comment

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

I mean changing the default installation without being in-line with with upstrean kustomize manifests is not what we want.
Upstream is our reference for the default installation as mentioned in the charts README.

I appreciate a more secure default installation but it has to be promoted first as a PR in the upstream repo (https://github.com/argoproj/argo-cd).

Copy link
Author

Choose a reason for hiding this comment

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

According to the Chart's README:

Hence, it makes sense to try to achieve a similar output of rendered .yaml resources when calling helm template using the default settings in values.yaml

According to this, the default should be redis.auth.enabled=false (as it is in the current implementation). Additionally, a user can easily enable a more secure installation by setting redis.auth.enabled to true.
Sounds reasonable to me.

@mkilchhofer
Copy link
Member

Hi @afrimberger sorry to get back to you soo late. We were working intensively on the recent release to address exactly this Authentication issue.

See:

We are now open for further improvement on this section, can you check which parts of your PR will work with the new setup?

@afrimberger
Copy link
Author

Hi @mkilchhofer, a security advisory can sometimes speed up things. Well played :-).

We are now open for further improvement on this section, can you check which parts of your PR will work with the new setup?

According to the least privileges principle the ArgoCD user should not have admin rights. The Redis ACLs could look like this (non HA):

user default on +@all -@admin -@dangerous ~* &* >{{ $defaultUserPassword | b64dec }}
user admin on +@all ~* &* >{{ $adminUserPassword | b64dec }}

To allow an admin user to connect to Redis for debugging purposes, at least two passwords need to be generated.

One option would be to only restrict the default user. Though, this would restrict the debug capabilities.

Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@mkilchhofer
Copy link
Member

@github-actions: Not stale

@afrimberger Maybe you can file an issue upstream (https://github.com/argoproj/argo-cd/issues) to enhance the redis access even more?

Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

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

Successfully merging this pull request may close these issues.

3 participants