Argo-cd: Add support for secrets in Application parameters

[niqdev]

It would be nice to have direct support for native secrets in the Application definition.

In the following use case, when referencing a public chart, it would be very handy to have the possibility to reference a secret directly.

# external-dns.yaml
---
apiVersion: argoproj.io/v1alpha1
kind: Application
metadata:
  name: external-dns
spec:
  project: kube-do
  source:
    repoURL: https://github.com/helm/charts.git
    path: stable/external-dns
    helm:
      parameters:
        - name: provider
          value: digitalocean
        - name: digitalocean.apiToken
          value: $(DIGITAL_OCEAN_TOKEN)
        - name: domainFilters
          value: $(EXTERNAL_DNS_DOMAIN)
      # suggested implementation
      secrets:
        - name: secrets-do
  destination:
    server: https://kubernetes.default.svc
    namespace: kube-do

# secrets-do.yaml
---
apiVersion: v1
kind: Secret
metadata:
  name: secrets-do
type: Opaque
stringData:
  config.yaml: |-
    # this could be overridden for example during bootstrap using
    # helm template . --values values.yaml --set digitalOceanToken=abc
    DIGITAL_OCEAN_TOKEN: {{ digitalOceanToken }}
    EXTERNAL_DNS_DOMAIN: {{ externalDnsDomain }}

Resources related to external-dns to have more details

• docs see Deployment env
• helm chart values.yaml
• helm chart DO_TOKEN env declaration

[niqdev]

I would also suggest to add a secretsUpdatePolicy to monitor the secrets e.g. update, rotation, delete ...

• none don't do anything if the secrets are updated
• restart restart the application if the secrets change

[alexec]

Contributing guide if you'd like to raise a PR: https://argoproj.github.io/argo-cd/CONTRIBUTING/

[alexmt]

Argo CD has https://argoproj.github.io/argo-cd/user-guide/auto_sync/ feature which covers secretsUpdatePolicy. If auto-sync is enabled and secret changes when argocd will go ahead push updated manifests.

We would have to make application CRD changes and serval code changes in api server/app controller:

CRD changes

Argo CD supports string parameters for helm, jsonnet (plugins parameters are WIP ). In addition to value we need to support valueFrom:

   # helm specific config
    helm:
      # Extra parameters to set (same as setting through values.yaml, but these take precedence)
      parameters:
      - name: "nginx-ingress.controller.service.annotations.external-dns\\.alpha\\.kubernetes\\.io/hostname"
        valueFrom:
          secretKeyRef:
            name: mysecret
            key: username

    jsonnet:
      # A list of Jsonnet External Variables
      extVars:
      - name: foo
        valueFrom:
          secretKeyRef:
            name: mysecret
            key: username

      # A list of Jsonnet Top-level Arguments
      tlas:
      - code: false
        name: foo
        valueFrom:
          secretKeyRef:
            name: mysecret
            key: username

I think it is ok to add support only for helm first and jsonnet, plugins later.

Code changes:

Manifest generation is performed by argocd-repo-server which does not have permissions to access k8s api for security reasons. So parameter values should be resolved and passed to argocd-repo-server by argocd-api-server and argocd-application-controller.

The argocd-repo-serverand argocd-api-server request manifest generation using GenerateManifest GRPC call. There are only three places in code where we execute GenerateManifest method and pass an instance of ApplicationSource which includes parameters:

https://github.com/argoproj/argo-cd/blob/master/controller/state.go#L113
https://github.com/argoproj/argo-cd/blob/master/server/application/application.go#L193
https://github.com/argoproj/argo-cd/blob/master/util/argo/argo.go#L379

To support valueFrom we would have to add e.g. ResolveValuesFrom method which replaces all valueFrom with actual value in specified ApplicationSource instance and use it before executing GenerateManifest.

[niqdev]

Thanks for the detailed info! I'm not familiar with go and the codebase, but I'll try to look into it and raise a PR

[niqdev]

@alexmt I've finally got a bit of time and started to look into this, but I need a bit of help

Here a bunch of changes, but I have already few doubts

• in manifests/crds/application-crd.yaml there are 6 references to helm, it's enough to add the valueFrom to the spec or should I c&p it also in status, operation, history, ... ? Is the openapi structure correct? I will add the description if that's correct then
• I edited the pkg/apis/application/v1alpha1/types.go, how do I regenerate the models or what is the equivalent of operator-sdk generate k8s? Cos running make codegen overridden all my changes
• repoClient.GenerateManifest take as parameter an ApplicationSource in argo-cd/controller/state.go and server/application/application.go, but doesn't that contains already the unmarshalled ValueFrom type? I don't understand where I have to add/implement ResolveValuesFrom
• Also, in my mind, I was expecting to find in the code something that is creating the pod/deployment/service, or whatever is the resource type declared in the application, where I could pass the parameters and using the native &corev1.Secret{} api to retrieve the secrets and attach it somehow. Am I completely wrong on this? The only place where I found something similar is in pkg/client/clientset/versioned/typed/application/v1alpha1/application.go, but it's code generated and I'm not sure if is the right place. What am I missing?

Do you have an architectural diagram of how client/server/api and all the moving parts are connected? Unfortunately I'm also a golang newbie and I'm having a bit of difficulty to understand how everything works

Thanks in advance for the help :blush:

[alexmt]

Hello @niqdev , thank you for working on it!

• The manifests/crds/application-crd.yaml is auto-generated. You can use make codegen to regenerate everything including CRD yamls. The dependencies are described here: https://argoproj.github.io/argo-cd/CONTRIBUTING/

• Regarding repoClient.GenerateManifest. One of the parameters of this method is ApplicationSource from types.go which includes ApplicationSourceHelm, ApplicationSourceKustomize etc. So once you add ValueFrom fields to data structures the same field will be available in repoClient.GenerateManifest

• The repoClient.GenerateManifest is responsible to pass parameter values to config management tool which produces yaml/json with resource definition. So we just need to make sure that repoClient.GenerateManifest input has resolved values

[patst]

Hey @niqdev
did you make any progress on this? Otherwise I am thinking about giving it a shot. But I am not a Go expert either ;-)

[niqdev]

Hi @patst,
unfortunately I don't have any time to make a concrete progress at the moment, even if I'd really like to.
You are more then welcome to pick it up! So far all the changes that I've made are in the branch linked above. Let me know if you need more info.
Thanks 😄

[dcherman]

Hi,

Just a heads up, I'm starting to tackle this using the same strategy as the helm-operator from WeaveWorks. Both secrets and configmaps would be supported.

My proposal is to introduce something like

valuesFrom:
  - secretKeyRef: ...
  - configMapKeyRef: ....

Since we already have support for passing inline values, the idea is to resolve any configmap/secrets that need to be fetched and merge them with the inline values before passing it off to the repo server. By doing that, it keeps the actual code that needs to change minimal.

Secrets/ConfigMaps would need to exist in the same namespace as ArgoCD which does present a problem; you could potentially reference other people's secrets/configmaps since Argo runs with the same service account.

Working around that seems like it'd require adding another resource type to ArgoCD which could then use argo's RBAC, but I'd like to hear someone's opinion that has more experience here.

Are we OK with the secret/configmap issue for an MVP with an explicit carveout that secrets prefixes with "argocd-" would be disallowed to avoid leaking argo-cd related secrets?

[alexec]

See https://github.com/argoproj/argo-cd/issues/1364.

[jessesuen]

First off, I do understand the motivation why some may feel this feature is necessary. Many Helm charts have a fundamental problem which allows secret values / sensitive information to be held in values.yaml files, essentially making values.yaml files sensitive objects. However, it's not Argo CD's responsibility to solutionize on what I consider a bad pattern or serious problems with the helm charts.

A well written chart should allow existing secrets to be referenced in the chart, rather than containing the sensitive information directly in the values.yaml. Here is a good example of this done properly:
https://github.com/helm/charts/blob/master/stable/postgresql/values.yaml#L113

Retrieving values from configmaps (https://github.com/argoproj/argo-cd/issues/1903) and secrets in the argocd control plane namespace is straying into the territory of config management, which is something we intentionally eschew from Argo CD.

This proposal (and https://github.com/argoproj/argo-cd/issues/1903) is essentially making Argo CD a centralized configuration/secret store, of which all applications can then reference secrets from. At this point, we would additionally need to implement RBAC rules on what applications are allowed to reference secrets.

Are we OK with the secret/configmap issue for an MVP with an explicit carveout that secrets prefixes with "argocd-" would be disallowed to avoid leaking argo-cd related secrets?

This is a perfect illustration of the type of problems we face if we go down this route. It is a slippery slope. Today, we already allow users to use arbitrary secrets in the argocd namespace to hold cluster credentials. These secrets do not necessarily start with the prefix argocd-. With this feature, these secrets would suddenly become available to devlopers who simply could reference the secrets in their application and suddenly obtain cluster/git credentials.

A second issue with these proposals is that it is introducing yet another source of truth of the manifests, which is different than git. Today Argo CD provides two sources of truth which affect the manifests generation:

• The contents of the Git repo.
• Parameters overrides set in the app spec (e.g. helm parameters, inline value contents).

Taking aside from the additional complexity of introducing another lookup as part of manifest generation, being able to reference configmaps and secrets in the argocd namespace, adds yet another source of truth is needs to be managed independently, which is antithetical to the GitOps model.

Lastly, these configmaps and secrets which are stored in the argocd control plane, are only under the control of an argocd operator, and outside the control over an application developer. To reference these secrets/configmaps would require coordination between developers and operators, violating the separation of concerns.

For these reasons, I do not believe this issue or https://github.com/argoproj/argo-cd/issues/1903 are ones that Argo CD should pursue.

[jessesuen]

I'm starting to tackle this using the same strategy as the helm-operator from WeaveWorks. Both secrets and configmaps would be supported.

The reason why this works for Helm Operator, but doesn't for Argo CD, is because these secrets are co-located with the HelmRelease instance, and are protected with the same Kubernetes namespace level RBAC. This means it is multi-tenant by default. Whereas in the case of Argo CD, the secrets are stored in the control plane namespace, not to mention mixed alongside Argo CD specific secrets, with zero RBAC protection and nothing stopping a developer from exposing the control plane secrets.

[dcherman]

@jessesuen So beyond referencing secrets in a chart where I actually agree that a correctly designed chart will allow you to do something like existingSecret, the actual use case that I personally have is generating a partial config during cluster creation via Terraform for use in some charts.

For example, I do not necessarily know the ARN of an IAM Role ahead of time if I'm trying to re-use the same repo to manage multiple clusters and need to reference that role either via kiam, or the new IAM for Service Accounts on EKS. My solution today has been to create a ConfigMap as part of Terraform that specifies just the parts that Terraform has knowledge of; the rest continues to be defined in git with the application.

WDYT?

[niqdev]

@jessesuen thanks for your feedback, I 100% agree with

Many Helm charts have a fundamental problem which allows secret values / sensitive information to be held in values.yaml files, essentially making values.yaml files sensitive objects. However, it's not Argo CD's responsibility to solutionize on what I consider a bad pattern or serious problems with the helm charts.

but even if it's not Argo CD's responsibility, I personally think that like everything there must be a compromise. In work we are using Argo CD managing the secrets with aws-secret-operator and IRSA using our own charts - no issue so far. So I'll give you the use case for why I've opened this issue...

I want to run on my DigitalOcean cluster (which doesn't have any out of the box solution to manage secrets like SSM) a bunch of applications/MVPs/POCs with very low security concerns and unfortunately I've encountered this issue.
I wrote for fun an operator inspired by the former that maps CR to native secrets and I'm using external-dns that force me to override the apiToken. At this point the right solution would be to fork the chart, open a PR and probably risk that all my changes are unaccepted (I had already issues with my PRs not being merged cos they got stale or simply cos the proposed changes in this case would be to invasive). I would then be forced to maintain my own forked chart - which I don't wanna do.

Given this use case and the fact that I wanna still be 100% fully declarative using Argo CD, I would be happy to accept/allow security risks - if that's the concern - and I'm sure that other people would have other reasons or use cases.

WDYT and what would be your solution in situations like this one?

[alexec]

I don't think the discussion is closed on this yet, I think we're monitoring it because there is interest. @niqdev we document solutions here, do you want to add https://github.com/mumoshu/aws-secret-operator?

https://argoproj.github.io/argo-cd/operator-manual/secret-management/

[niqdev]

@alexec I'll create a PR for that

[niqdev]

2598 cc @alexec

[dcherman]

@jessesuen Putting some of the ideas that we were throwing around at KubeCon in here so they don't get lost:

Right now, the problem with the current proposal is that for multi-tenant solutions, it would be horribly insecure since applications would have access to config/secrets that they shouldn't necessarily have access to.

One potential way to solve this could be to enhance the custom tooling/plugins that you can provide to ArgoCD by providing the entirety of the application manifest to it via some file (manifest.json/yaml) which would then allow the custom plugin to combine the values in the manifest itself with values sourced elsewhere (configmaps, secrets, etc..).

That allows you to accomplish the proposal here, but punts on the security aspect by requiring the user to implement this themselves. One potential avenue that comes to mind is signing an app manifest, then using a plugin that verifies the provenance of your application before allowing it access to the requested secrets.

The actual functionality that would be needed for this seems to be minimal on the ArgoCD side of things; basically, just provide the entire app manifest to the plugin either via a file or environment variable that the user can parse and do whatever they want with.

We probably can/should then discuss how we extract some of the core functionality of Helm/Kustomize/etc.. out into plugins that can be iterated on independently of ArgoCD itself.

[CarpathianUA]

Looking forward to have this functionality!
Thanks for your work, guys

[dcherman]

@jessesuen @alexec Just circling back to this - would something like what was proposed above be something that you'd be interested in / would accept into ArgoCD?

Basically, make the entire application manifest as either a file or environment var (not sure which atm) available to custom tools that can then parse it and do whatever they want with those values.

In addition, we would re-order https://github.com/argoproj/argo-cd/blob/57eeaa423157d42997f7147b054d8ee4cbc4a6b4/reposerver/repository/repository.go#L353-L361 such that custom plugins get detected first before the others, that way they could take advantage the existing types/structs for Helm, Kustomize, etc.. in custom plugins. While that is technically a breaking change, I'm not sure how many people actually include data for both something like Helm and a custom plugin in the same manifest since it wouldn't work at all.

That should solve my personal use case since I could easily do something like have my own helm plugin that does something like envsubst with values.yaml inlined into the application manifest. The actual values could still be provided via a configmap, but that becomes my plugin's responsibility to provide all of that, not Argo's.

[edlee2121]

Using a projected serviceAccountToken is one way to inject config/secrets dynamically.
https://kubernetes.io/docs/tasks/configure-pod-container/configure-service-account/#service-account-token-volume-projection

[dcherman]

Another idea for securely doing this:

Each project can have a Kubernetes ServiceAccount assigned to it (or created dynamically using the project name as part of the convention). When ArgoCD attempts to retrieve either ConfigMaps or Secrets, it would do so using the token from the associated service account.

Access to secrets or configmaps in any given namespace can then be governed by plain old Kubernetes RBAC.

Either way, I think we could potentially implement the ConfigMap part of this proposal and punt on the Secret part in order to avoid tackling the security implications right now.

[CuBiC3D]

Any agreement or progress on this topic? I'm seeing a couple of related issues without a solution and I was very surprised to find out that there is no possibility to merge values from a secret. Pulling and customizing charts is a tedious workaround to replace values and create encrypted secrets to push applications to git.