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

new multiple sources doesn't work with helm-secrets #11866

Open
3 tasks done
brandonguigo opened this issue Jan 2, 2023 · 35 comments
Open
3 tasks done

new multiple sources doesn't work with helm-secrets #11866

brandonguigo opened this issue Jan 2, 2023 · 35 comments
Labels
bug Something isn't working

Comments

@brandonguigo
Copy link

brandonguigo commented Jan 2, 2023

Checklist:

  • I've searched in the docs and FAQ for my answer: https://bit.ly/argocd-faq.
  • I've included steps to reproduce the bug.
  • I've pasted the output of argocd version.

Describe the bug

When using as values files in helm application with secrets, using the "secrets+age-import:///age-secret-key/keys.txt" file uri, the ref is not replaced with the file path.

To Reproduce

apiVersion: argoproj.io/v1alpha1
kind: Application
metadata:
  name: namespace-config
spec:
  project: project
  destination:
    name: in-cluster
    namespace: staging
  source:
    - repoURL: repo2
      targetRevision: main
      ref: config
    - repoURL: repo # Can point to either a Helm chart repo or a git repo.
      targetRevision: HEAD  # For Helm, this refers to the chart version.
      path: path # This has no meaning for Helm charts pulled directly from a Helm repo instead of git.
      helm:
        valueFiles:
          - values.yaml
          - $config/namespace-config/default/values.namespace.yaml
          - secrets+age-import:///age-secret-key/keys.txt?$config/namespace-config/default/secrets.namespace.yaml
          - $config/namespace-config/staging/values.namespace.yaml
          - secrets+age-import:///age-secret-key/keys.txt?$config/namespace-config/staging/secrets.namespace.yaml
      ignoreMissingValueFiles: false
  syncPolicy:
    automated: # automated sync by default retries failed attempts 5 times with following delays between attempts ( 5s, 10s, 20s, 40s, 80s ); retry controlled using `retry` field.
      prune: true # Specifies if resources should be pruned during auto-syncing ( false by default ).
      selfHeal: true # Specifies if partial app sync should be executed when resources are changed only in target Kubernetes cluster and no git change detected ( false by default ).
      allowEmpty: false # Allows deleting all application resources during automatic syncing ( false by default ).
    syncOptions:     # Sync options which modifies sync behavior
      - Validate=false # disables resource validation (equivalent to 'kubectl apply --validate=false') ( true by default ).
      - CreateNamespace=true # Namespace Auto-Creation ensures that namespace specified as the application destination exists in the destination cluster.
      - PrunePropagationPolicy=foreground # Supported policies are background, foreground and orphan.
      - PruneLast=true # Allow the ability for resource pruning to happen as a final, implicit wave of a sync operation
    # The retry feature is available since v1.7
    retry:
      limit: 5 # number of failed sync attempt retries; unlimited number of attempts if less than 0
      backoff:
        duration: 5s # the amount to back off. Default unit is seconds, but could also be a duration (e.g. "2m", "1h")
        factor: 2 # a factor to multiply the base duration after each failed retry
        maxDuration: 3m # the maximum amount of time allowed for the backoff strategy
  # RevisionHistoryLimit limits the number of items kept in the application's revision history, which is used for
  # informational purposes as well as for rollbacks to previous versions. This should only be changed in exceptional
  # circumstances. Setting to zero will store no history. This will reduce storage used. Increasing will increase the
  # space used to store the history, so we do not recommend increasing it.
  revisionHistoryLimit: 1

Expected behavior

ArgoCD should replace $config by the directory path

Screenshots

Version

Paste the output from `argocd version` here.
2.6.0-rc1 (i have no ingress on the argocd deployment so argocd version don't work)

Logs

Paste any relevant application logs here.
@brandonguigo brandonguigo added the bug Something isn't working label Jan 2, 2023
@morey-tech
Copy link
Contributor

To clarify the example to reproduce, you have spec.source (singular) when it should be spec.sources (plural). Is that the issue or only in the example?

@brandonguigo
Copy link
Author

Oh sorry, it's only in the examples, I have sources inside my real yaml file

@brandonguigo
Copy link
Author

@morey-tech Hey, so just to clarify, it seems that the fix will not be included in the 2.6 release ?
I'm stuck with submodules everytime i want to use secrets for now ?

@morey-tech
Copy link
Contributor

@morey-tech Hey, so just to clarify, it seems that the fix will not be included in the 2.6 release ? I'm stuck with submodules everytime i want to use secrets for now ?

Based on #11966 (comment), unfortunately, it looks that way. As a beta feature, there may be edge cases that aren't supported yet, and Helm Secrets happen to be a victim of that. Unless someone has time to figure out the solution and properly test it before the 2.6 release on 2023-02-06, it'll have to wait for 2.7 around 2023-04-10 or there is "some hope for including it in a patch release"

@aoz-turk
Copy link

Hi,

We want to use helm-secrets with sops backend + multi sources app. From the following documentation, we understand that the limitation lives on Argo CD side.

https://github.com/jkroepke/helm-secrets/wiki/ArgoCD-Integration#multi-source-application-support-beta

Should we wait for 2.7 release?

@jkroepke
Copy link
Contributor

Ping @todaywasawesome, greetings from KubeCon.

@harrywm
Copy link

harrywm commented Apr 25, 2023

Hi there folks,

Is this still on track for 2.7? :)

@jkroepke
Copy link
Contributor

It seems like that this issue can be resolved by #12508

@ishitasequeira Did you see any chance that the PR will be merged into 2.7?

@ishitasequeira
Copy link
Member

@jkroepke Thanks for reaching out.. I have reviewed the PR!! Looking for another eyes from the approvers to get it merged.

@egoist777
Copy link

2.7.0 is released but multi source still doesn't work with helm-secrets. Does anyone have any updates on this ?

@jkroepke
Copy link
Contributor

jkroepke commented May 2, 2023

@egoist777 #12508 is not merged, nothing changed in 2.7.0 in terms of this.

@egoist777
Copy link

@jkroepke I see...
Can you please suggest any workaround for this ? Thanks in advance.

@tol777900
Copy link

@egoist777 #12508 is not merged, nothing changed in 2.7.0 in terms of this.

Any updates when this will get merged ?

@harrywm
Copy link

harrywm commented May 15, 2023

Any updates when this will get merged ?

Seems it's just waiting for an extra reviewer, similar to pre-2.7. Any help testing the feature of course helps get things through quicker!

@madispuk
Copy link

Any updates?

@MohammedNoureldin
Copy link

MohammedNoureldin commented Oct 31, 2023

Soon 2.9.0 will be out. Is there any plans for this? Being able to use encrypted values.yaml is curcial IMHO.

I am a bit confused, because helm-secrets people mention here that there is a limitation, and this is shown here in this issue. However, there is an example shown which supposes that it should work.

I am talking about this:

Multi-Source Application Support [BETA]
ArgoCD has limited supported for helm-secrets and Multi-Source application. sops backend is not tested yet.

References:

https://github.com/argoproj/argo-cd/issues/11866
https://github.com/argoproj/argo-cd/pull/11966
On ArgoCD 2.6.x, helm-secrets isn't supported in Multi-Source application, because the source reference, e.g.: $ref needs to be at the beginn of a string. This is in conflict with helm-secrets, since the string needs to beginn with secrets://. On top, ArgoCD do not resolve references in URLs.

HELM_SECRETS_VALUES_ALLOW_ABSOLUTE_PATH must be set to true, since ArgoCD pass value files with absolute file path.

Ensure that the env HELM_SECRETS_WRAPPER_ENABLED=true (default false) and HELM_SECRETS_VALUES_ALLOW_ABSOLUTE_PATH=true is set on the argocd-repo-server. Please ensure you are following the lastest installation instructions (updated on 2023-03-03).

and after this text in the same page they provided an example with multi-source application. Could any one tell me what is going on? Or what is the status of this limitation that I don't really understand.

@jkroepke
Copy link
Contributor

@MohammedNoureldin

PR #12508 is likely something that we are looking for, but its getting staled

@crenshaw-dev
Copy link
Member

@MohammedNoureldin I'm not sure how that workaround is supposed to work or why it might fail... Maybe they have a custom Argo CD image which supports those env vars?

Re: #12508, it's not stale, I just haven't had time to give it a final review and get it merged yet.

@MohammedNoureldin
Copy link

MohammedNoureldin commented Oct 31, 2023

Thank you for your reply, @crenshaw-dev and @jkroepke.

@l-maia
Copy link

l-maia commented Oct 31, 2023

I think that env is for helm-secrets that would by default reject an absolute path, not argo.

Meanwhile 1 week into testing argocd I already hit this limitation too, only because the substitution assumes that the string must start with $ref.

The simplest method for me would be to allow ref substitution (in helm plugin) using both a file path or an URI style reference, without touching the scheme to allow for arbitrary plugins (secrets://).

And limit the substitution to the first match after the scheme.

@jkroepke
Copy link
Contributor

jkroepke commented Nov 1, 2023

As plugin maintainer, i'm open to any reachable solution.

@crenshaw-dev
Copy link
Member

Yeah I wrote up a small proposal on another issue about how we could do ref substitution on secret values. At the time it seemed gnarlier than the solution of sharing arbitrary files, but now I'm thinking that calculus might have been wrong.

@jkroepke
Copy link
Contributor

jkroepke commented Nov 1, 2023

Whats is the issue with allow to substitute the $ref after the protocol handler secrets//$ref/? That would be the best user expericence.

@harrywm
Copy link

harrywm commented Nov 1, 2023

@jkroepke I see... Can you please suggest any workaround for this ? Thanks in advance.

@egoist777 We're currently using a Helm SubChart method as a workaround, and we actually prefer it to the proposed Multi-App solution. Though, of course, won't work for all cases.

Here's a gist that covers our setup.
https://gist.github.com/harrywm/6a80b6dc355f20595b002734559d4a15

@l-maia
Copy link

l-maia commented Nov 1, 2023

Yeah I wrote up a small proposal on another issue about how we could do ref substitution on secret values. At the time it seemed gnarlier than the solution of sharing arbitrary files, but now I'm thinking that calculus might have been wrong.

I also think the best solution is to have that simplest solution, that can easily unblock most use cases. Namely the usage of charts in external helmrepos with value files from git.

If arbitrary files were a problem i would think that the repos should be cloned to a single app folder and a chroot used to prevent references outside the scope (app, generator,etc).

@crenshaw-dev
Copy link
Member

Whats is the issue with allow to substitute the $ref after the protocol handler secrets//$ref/? That would be the best user expericence.

Path traversal, the bane of the repo-server. :-P

Substituting $ref at the beginning of a valuesFile path is safe and easy, because we know exactly how Helm uses that path, and we can scrub the sensitive, not-to-be-leaked path from error messages.

Substituting in an arbitrary position in the path means that we can no longer be confident that we're successfully scrubbing that path from all error messages or other output.

I think values file substitution should be implemented in a different way: #11966 (comment)

Instead of substituting the $ref variable with the file's home repo path, we should copy the file out to a new, randomized path and substitute that new, short-lived path into the value file path. We don't care if the path gets leaked, because it'll be deleted by the time anyone sees it.

@jwitko
Copy link

jwitko commented Feb 27, 2024

@crenshaw-dev Based on the very thoughtful and appreciated response in #12508 (comment)
I'm wondering how that impacts this issue?
Since this is a bug and not a new feature or feature behavior change is it impacted by the linked decision?
Is there an alternative more localized fix path we could take that wouldn't be impacted by the above decision or is it just time to accept that Helm Secrets will not be a part of multi-source approaches?

@crenshaw-dev
Copy link
Member

crenshaw-dev commented Feb 27, 2024

@jwitko I think there's still a path forward for helm-secrets. I think that something like what I've described at the bottom of this comment #11966 (comment) might be viable. I worry about the URL parsing part, but maybe that's not as tricky as it seems in my head.

@jurgenweber
Copy link

Soon 2.9.0 will be out. Is there any plans for this? Being able to use encrypted values.yaml is curcial IMHO.

I am a bit confused, because helm-secrets people mention here that there is a limitation, and this is shown here in this issue. However, there is an example shown which supposes that it should work.

I am talking about this:

Multi-Source Application Support [BETA]
ArgoCD has limited supported for helm-secrets and Multi-Source application. sops backend is not tested yet.

References:

https://github.com/argoproj/argo-cd/issues/11866
https://github.com/argoproj/argo-cd/pull/11966
On ArgoCD 2.6.x, helm-secrets isn't supported in Multi-Source application, because the source reference, e.g.: $ref needs to be at the beginn of a string. This is in conflict with helm-secrets, since the string needs to beginn with secrets://. On top, ArgoCD do not resolve references in URLs.

HELM_SECRETS_VALUES_ALLOW_ABSOLUTE_PATH must be set to true, since ArgoCD pass value files with absolute file path.

Ensure that the env HELM_SECRETS_WRAPPER_ENABLED=true (default false) and HELM_SECRETS_VALUES_ALLOW_ABSOLUTE_PATH=true is set on the argocd-repo-server. Please ensure you are following the lastest installation instructions (updated on 2023-03-03).

and after this text in the same page they provided an example with multi-source application. Could any one tell me what is going on? Or what is the status of this limitation that I don't really understand.

Yeah, this works! Nice find.

@Maurifc
Copy link

Maurifc commented May 22, 2024

Soon 2.9.0 will be out. Is there any plans for this? Being able to use encrypted values.yaml is curcial IMHO.

I am a bit confused, because helm-secrets people mention here that there is a limitation, and this is shown here in this issue. However, there is an example shown which supposes that it should work.

I am talking about this:

Multi-Source Application Support [BETA]
ArgoCD has limited supported for helm-secrets and Multi-Source application. sops backend is not tested yet.

References:

https://github.com/argoproj/argo-cd/issues/11866
https://github.com/argoproj/argo-cd/pull/11966
On ArgoCD 2.6.x, helm-secrets isn't supported in Multi-Source application, because the source reference, e.g.: $ref needs to be at the beginn of a string. This is in conflict with helm-secrets, since the string needs to beginn with secrets://. On top, ArgoCD do not resolve references in URLs.

HELM_SECRETS_VALUES_ALLOW_ABSOLUTE_PATH must be set to true, since ArgoCD pass value files with absolute file path.

Ensure that the env HELM_SECRETS_WRAPPER_ENABLED=true (default false) and HELM_SECRETS_VALUES_ALLOW_ABSOLUTE_PATH=true is set on the argocd-repo-server. Please ensure you are following the lastest installation instructions (updated on 2023-03-03).

and after this text in the same page they provided an example with multi-source application. Could any one tell me what is going on? Or what is the status of this limitation that I don't really understand.

Thanks. It worked for me.

This is what I did

  1. Set the variables on argocd-repo-server deployment
    - name: HELM_SECRETS_VALUES_ALLOW_ABSOLUTE_PATH
      value: "true"
    - name: HELM_SECRETS_WRAPPER_ENABLED
      value: "true"
  1. Set the encrypted values file path without the secrets:// prefix (ArgoCD Application)
    - helm:
        releaseName: my-app
        valueFiles:
          - $values/secrets.yaml`

@jc36
Copy link

jc36 commented Jun 21, 2024

This works for me too. Thank you.
But another problem was discovered.
Encrypted with SOPS multiline string (ssh RSA key) contains a newline at the end.

gituser_rsa: |
    -----BEGIN OPENSSH PRIVATE KEY-----
    ...
    -----END OPENSSH PRIVATE KEY-----

When we use the secrets:// prefix and do not have envvars HELM_SECRETS_VALUES_ALLOW_ABSOLUTE_PATH and HELM_SECRETS_WRAPPER_ENABLED, then newline is passed to the kubernetes secret template (id_rsa: {{ $.Values.gituser_rsa | b64enc }}) and secret is mounted to container as a file so everything works as expected.

If we add

    - name: HELM_SECRETS_VALUES_ALLOW_ABSOLUTE_PATH
      value: "true"
    - name: HELM_SECRETS_WRAPPER_ENABLED
      value: "true"

then the secrets are also decrypted even without the prefix secrets:// , but it turned out that the value of the variable gets into the secret without the last newline symbol.
So we catch an error reading such an rsa key "Load key "/home/gituser/.ssh/id_rsa": error in libcrypto" and it is clearly related to the missing line translation at the end of the file.

Tried to pass a few additional newline characters at the end and use |+ (yaml multiline with keeping all newlines at the end), but all newlines are cut off.

Of course, I have a workaround to add an additional newline in the secret's helm template (id_rsa: {{ printf "%s\n" $.Values.gituser_rsa | b64enc }}), but I would like to find the real reason.

I did not find any mention of such a bug in sops "issues".

@jkroepke
Copy link
Contributor

Use

gituser_rsa: |-

to strip newline at end.

@jc36
Copy link

jc36 commented Jun 21, 2024

We need to save the newline if it exists.
For some reason, adding the env variables above truncates all newline characters at the end of the decoded variables.

@stephaneetje
Copy link

Hello,

I'm still unable to use sops backend with GCP KMS on multi source apps.
Did you all go using GPG to make it work ?

@stephaneetje
Copy link

I finally managed to make it work, i share here what was wrong in my configuration : jkroepke/helm-secrets#475

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.