FairwindsOps / rok8s-scripts

Opinionated scripts for managing application deployment lifecycle in Kubernetes
https://fairwinds.com
Apache License 2.0
298 stars 76 forks source link

Philosophy on secrets creation with Helm charts #245

Closed sc250024 closed 5 years ago

sc250024 commented 5 years ago

Greetings,

I'm happy to have stumbled upon this repository, and I'm extremely grateful that someone is open-sourcing something that almost everyone uses in some form (whether hand-crafted themselves, or through a SaaS provider). Standardizing this type of thing is very useful for the community.

I'm using Sops for secrets encryption, and there's one thing that confuses me about secrets creation with these scripts. Specifically, let's look at this part: https://github.com/reactiveops/rok8s-scripts/blob/master/docs/secrets.md#creating-a-new-secret. The documentation states:

Once your editor opens up you can create a Kubernetes Secret Document. Below is an example of a kubernetes secret YAML.

apiVersion: v1
kind: Secret
metadata:
  name: example-name
stringData:
  EXAMPLE_VAR: "example value"

The format of this secrets file is enforced via the following line when the secrets are deployed: https://github.com/reactiveops/rok8s-scripts/blob/master/bin/k8s-deploy-secrets#L66.

Essentially, it means that you must use the standard Kubernetes API YAML for your secrets. This approach goes against how Helm charts work. If you look at the official repository for Helm charts, for every chart that uses the kind: Secret API, it's almost always a template. For example, let's take the Minio secrets.yaml from here:

{{- if not .Values.existingSecret }}
apiVersion: v1
kind: Secret
metadata:
  name: {{ template "minio.fullname" . }}
  labels:
    app: {{ template "minio.name" . }}
    chart: {{ template "minio.chart" . }}
    release: {{ .Release.Name }}
    heritage: {{ .Release.Service }}
type: Opaque
data:
  accesskey: {{ .Values.accessKey | b64enc }}
  secretkey: {{ .Values.secretKey | b64enc }}
{{- if .Values.gcsgateway.enabled }}
  gcs_key.json: {{ .Values.gcsgateway.gcsKeyJson | b64enc }}
{{- end }}
{{- end }}

This means that control of how and where secrets that are encrypted via Sops are deployed is left up to the template; the only thing that happens on deployment is that the sops binary is called to decrypt the file, and expose those secrets as template variables for the Helm chart.

In order to use secrets.yaml file in the way that this repository requires, I would have to encrypt the template itself. However, this doesn't work because sops cannot verify that it's proper YAML. If I try to encrypt the same Minio secrets.yaml template above, I get the following error:

[CMD]    ERRO[1285] Could not load tree, probably due to invalid syntax. Press a key to return to the editor, or Ctrl+C to exit.  error="Error unmarshaling input YAML: yaml: did not find expected node content"

It seems to me there are a couple of approaches to take here:

Thoughts?

sudermanjr commented 5 years ago

@sc250024 Thanks for the thoughtful and carefully-written description.

If I read this correctly, you are trying to encrypt a helm-template'd secret using SOPS.

We have chosen to keep secret creation entirely out of the hands of helm. This may not be clear from the documentation, but the current implementation suggests that you create the secret as a normal manifest (sops encrypted) and then use k8s-deploy-secrets to deploy them before doing a helm-deploy.

We prefer this way because (At the time of this decision, not 100% this is still true) helm stores all rendered templates in a configmap in the cluster, not a secret.

sc250024 commented 5 years ago

Hey @sudermanjr ! Thanks for the quick reply, and the thoughtful explanation.

You're more or less correct in stating what I'm trying to achieve. Essentially, I'm preferring that the rok8s scripts handle the automated decryption of the Sops encrypted files (whatever they contain), and then I would prefer to pass those secrets with something like the Helm --values option. This would be then processed by a secrets.yaml template in the Helm chart like the Minio example I have above.

This part you mentioned...

We prefer this way because (At the time of this decision, not 100% this is still true) helm stores all rendered templates in a configmap in the cluster, not a secret.

...depends on how the user has initiated Tiller in their Kubernetes cluster. You can check the official documentation here, but the blurb that's important is this part:

By default, tiller stores release information in ConfigMaps in the namespace where it is running. As of Helm 2.7.0, there is now a beta storage backend that uses Secrets for storing release information. This was added for additional security in protecting charts in conjunction with the release of Secret encryption in Kubernetes.

To enable the secrets backend, you’ll need to init Tiller with the following options:

helm init --override 'spec.template.spec.containers[0].command'='{/tiller,--storage=secret}' Currently, if you want to switch from the default backend to the secrets backend, you’ll have to do the migration for this on your own. When this backend graduates from beta, there will be a more official path of migration

So personally, I use secrets as the storage backend in Helm. Looking at the script bin/k8s-deploy-secrets, it appears that the rok8s philosophy on secrets is really baked into the scripts themselves.

How would you suggest changing this functionality for the future, if at all? Would this be a breaking change suitable for the 8.0.0 release?

sudermanjr commented 5 years ago

I will have to think about the possible implementation a bit more about this next week, but in general our philosophy has been that we are open to new concepts. I will also bring this up with the other maintainers of this project.

If we introduce breaking changes or new features, we try to hide them behind a feature flag in order to make them available but not default. If that is not possible we introduce a new version. However, a new version that shifts usage in a fundamental way would be problematic (but not impossible) for us and our various implementations.

A different usage of tiller for us would be a decent amount of effort to migrate all of our clusters, but could possibly be done. This mostly depends on the migration path for our existing implementations.

TL;DR - this is definitely a good thing for us to be thinking about, and bears much further scrutiny and planning.

sc250024 commented 5 years ago

@sudermanjr Maybe something like this? https://github.com/reactiveops/rok8s-scripts/pull/246

That should be considered a WIP, and only to show as an example. We could pass the variable ROK8S_HELM_DEPLOY_EXTRAARGS to the helm-deploy script. As a consequence, the line rm "${DECRYPTED_FILE}" would have to be moved somewhere else.

I was thinking about the secrets removal anyway, because I feel that should be moved to a trap function. I noticed that if there's a configuration error, the unencrypted secrets still exist on the filesystem. Error or not, with a trap function, you get a nice cleanup of all potential leftover files.

What do you think?

sudermanjr commented 5 years ago

The concept seems great. Doesn't break backward compatibility and opens the door to using helm secret backend as well. Trap function seems like a great idea. We typically use ephemeral build machines/containers, so it has been top of mind, but definitely a great addition.

The only thing I would add is that if you go forward with making this fully functional it would be great to see some improvement in the documentation around these settings

endzyme commented 5 years ago

Hi all! Just catching up here. Love the thoughts so far. I've added a comment on poc implementation you linked above. Looking forward to discussing more!

endzyme commented 5 years ago

Sorry this has taken so long for me to circle back on! "tomorrow" shouldn't be 2 weeks :sweat_smile: -- I've got a POC that I wanted to run by you if you'd like @sc250024.

See this commit for the POC.

Intent is to provide a common way to decrypt a set of files with one command and a list of files. This way it can return a list of decrypted files, to do whatever you want. An example use of this new script is in the k8s-deploy-secrets script, and you can get an idea of how you might use this in your own pipelines to populate the ${ROK8S_HELM_DEPLOY_EXTRAARGS} variable. An example below:

# Your Pipeline File Here
#...
ROK8S_HELM_DEPLOY_EXTRAARGS=""
#...
decrypted_values=($(sops-decrypt-files my-values.yaml my-other-values.yaml))
for file in ${decrypted_values[@]}; do
  ROK8S_HELM_DEPLOY_EXTRAARGS+=" --values ${file}"
done
#...
helm-deploy -f deploy/<env>.config

Is this too far away from what you were looking for?

endzyme commented 5 years ago

Hi @sc250024. Just wanted to follow up here and see if we should move forward or close this out. Hope all is well!