remove SecretGenerator due to security concern
Liujingfang1 opened this issue · 33 comments
Need to address the security concerns of Kustomize in order to integrate it into Kubectl
kubernetes/enhancements#684
SecretGenerator is one concern since it can trigger arbitrary command that users put in.
The solution is to remove it from Kustomize.
@Liujingfang1 I'm kind of shocked to learn SecretGenerator is going away. Do you intend to replace it with a substitute? My use-case is using sops to load encrypted secrets into Kubernetes.
(I tried to access the security document you linked to in kubernetes/enhancements#684, but could not read it.)
@jcassee The doc is available to kubernetes-sig-cli@googlegroups.com
Okay, I was mostly interested in the context of the decision and whether you have any advice for current users of SecretGenerator. (I thought the plan was to only remove commands from the kubectl integration, but keep it in kustomize. But maybe the plan is to drop kustomize completely?)
The plan is to remove SecretGenerator entirely from Kustomize.
Okay. So what is your advice to users of the feature? I currently rely on it for my Kubernetes deployments.
Hello, we have a similar approach using sops, a nice workaround regarding this issue would be to allow secretGenerator to behave exactly like configMapGenerator (e.g. providing a 'files' list), so that we could leverage out-of-band automation to generate those files.
That would export the reponsability of executing the creation commands out of kustomize, while still being able to use the secretsGenerator.
@oboukili Yes, that could work. The nice thing about commands is that the unencrypted values never hit the disk, so there is nothing to clean up after deployment. I do see the danger with commands (especially in remote bases), but I thought they were only removed from the kubectl build (#683), which I thought was a good compromise. I'm sad to see them go.
@jcassee I think you're planning on exposing those decrypted files onto a kubernetes secret, in that case that's true it does hit the disk.
As a workaround, I thought of having a different deployment workflow that would allow us to store those encrypted files onto a kubernetes secret and use an init container to decrypt and send those to an Hashicorp Vault KV backend on-the-fly (very specific use case I concur, and one that pulls yet another dependency).
I understand that 'command' concern as well however.
It would still be nice to have such a 'files' feature support for secretGenerators to retain using secrets and all the goodies that come with them.
Initially (back at the beginning) the notion was to have kustomize generate a k8s Secret the same way it generates a ConfigMap - by reading data from disk (as @oboukili and @jcassee suggest).
Placing the secret on disk offends certain folks because it raises fundamental questions of security - who has access to the file, how do we know it's deleted, etc. Having kustomize read a command to run to get the secret, rather than demand the secret be on disk, addressed that risk by introducing a different one.
The secret eventually ends up on disk (in etcd), plain to see to an operator with access to etcd (or to any backups of etcd), or with the ability to run a pod (they can just fire up something to read the secret and print it). If that operator is the person trusted to apply resources to the cluster, one could argue that this person should be able to use text tools to generate k8s objects from disk data and manage cleanups.
I'm (still) in favor of treating them like ConfigMaps, since I see k8s Secrets as the cluster equivalent of a cheap padlock. Don't use them for anything you really care about. As an operator, would you store an access password to a bank database in a k8s Secret?
I don't know how conclude this debate with something everyone agrees is secure (other than feature removal obviously).
I also use the SecretGenerator functionally, but I've been tinkering with extending kustomize to handle SealedSecrets from https://github.com/bitnami-labs/sealed-secrets by making a SealedSecretGenerator.
I'm not using the command options, just using the generator to rename the object from a patch file and all the references, so I get the nice re-deploy when the secret changes feature.
Could the security issue be fixed by removing the command options and leave the generator to create blank secrets to fill in from patches?
@chrishiner So the patches still need to come from somewhere (disk, command, whatever) so seems like the same secret mgmt problem exists. Plus mutating a secret or configmap already in k8s is usually a bad idea. That's why we make new ones with new names (generated from a hash of the contents), then change deployments to point to them. Please feel free to start an issue (or KEP) for how one might use kustomize in a SealedSecret workflow. Sounds good, but this isn't the place for those details.
We decided to use #703 :)
Would you be open to breaking out the secret generation stuff into a separate tool/binary instead? And maybe even add direct integrations with popular tools like sops? We can introduce a new file generator.yaml and then the tool would go through the directory and modify each adjacent kust file with a secretGenerator/configMapGenerator section in the same way that kustomize edit add does.
This would keep kustomize free of any side affects, but, allow users to keep the same workflow.
I am happy to work on this, just wanted to get your thoughts first.
@sethpollack Could a custom transformer create secrets based on commands?
@sethpollack I was thinking the old command/envCommand options could be supported by transformers that are not enabled by default. However, as I think about this, transformers are configured from kustomization.yaml, so it's just as unsafe. (Maybe I am looking for a --i-want-to-be-vulnerable command line option.
I was just getting ready to use kustomize and this ticket has me concerned on how to proceed. How are other sops users working around this in the meantime? Using an older version of kustomize until a workaround comes in?
In most tools you could avoid an intermediate file by using bash process substitution e.g. kustomize build --secret=<(decrypt secrets.yaml) but if I understand correctly the idea of build time flags has been rejected?
If the authors were willing to reconsider build time flags it could be a neat solution to the problem.
@sethpollack other tool is fine and welcome, but then you have a distribution problem
@jcassee Right. The whole thing blew up over the notion of a "hostile" remote kustomization file used as a base. We allow the base to be a git url, but it's up to the user to only use a url they control (e.g. an internal url controlled by devops). Removal of the exec was, as is the case in all these things, an attempt to keep someone from shooting themselves. And of course when convenience is removed for security reasons, people simply invent even less secure workarounds.
@mikesimons We eschew build flags for specific reasons but the value of a secret by definition you might not want to keep in source control (at least not in the clear). Could you flesh that out as a short KEP? Somehow bind the command line arg to the object being generated. Kep PRs go here.
@monopole What about just adding in integrations with popular tools rather than using commands?
@monopole I don't have time to write up anything as formal as a KEP at the minute but if you're interested in the idea as explained here I can see what I can do over the next week or so.
I think the main objection to the removal of the command execution was precisely because folks are either storing their secrets encrypted (e.g. sops, ansible-vault) or storing them in an encrypted value store (such as Vault or AWS Secrets Manager).
Regarding my suggestion; I'd envisage that the kustomize edit commands would be mapped on to cli flags on the build command. Here is a contrived example:
kustomize build \
--add-secret="secret1=file://<(decrypt secret)" \
--add-secret="secret2=envfile:///file" \
--add-label="label1=value1" \
--add-label="label2=value2" \
--add-patch=patch.yaml \
--add-resource=resource.yaml
These params would cause kustomize to mutate the kustomization.yaml just after loading it.
In the case of secret1 kustomize would receive something like "secret1=file:///dev/fd/63" where /dev/fd/63 is a special file attached to stdout of the process executed (thus bypassing writing the secrets in the plain to the disk). This file can be read like a normal file (except seeks). This meets the requirements of not allowing arbitrary executions in kustomization files but allows end-users to decrypt variables just-in-time without writing them to disk.
Regarding the reasons for not using params on build; I saw that but I don't understand the difference between the advocated kustomize edit ... && kustomize build approach and what cli flags would achieve. Both would yield the same effective outcome; the build deviating from the committed config. Maybe I'm missing something but it seems like semantics to me.
As a kustomize/sops user, I'd like to suggest that there are use-cases for having secrets in a base that is used in multiple overlays. For example, an external monitoring solution that requires an API key could be reused in multiple environments. I think command line options as suggested would not cover this use-case like the command options of SecretGenerator did.
Agreed
@jcassee @sethpollack True. The only thing I can think of for secrets working safely in bases (as currently defined) is late-binding of secrets so bases use placeholders and at build time you have to populate a dictionary that defines the replacements. Even still late binding would mean the secrets could not be stored with the base.
What about using golang plugins instead of commands and requiring them to be loaded from outside the kustomize directory? This would also give people the flexibility to build their own plugins.
I like the idea of support for common tools like sops, vault, etc.
I think @sethpollack is onto something with having them live out of tree, though I don't have enough experience with golang plugins to comment on that as an implementation.
@dexhorthy here is a POC, let me know what you think? #760
So, due to the history of this feature (causing a rollback in kubectl), we really need a provisional KEP to proceed.
It doesn't have to be fully fleshed out as an implementable design doc. It's supposed to be a high level thing, where the risks/benefits could be listed.
It would be better historically to have this (very useful) discussion on a KEP, rather than on a closed issue.
Ok will do
Ok here is the KEP kubernetes/enhancements#811
If anyone is interested then I have created a kustomize plugin that transforms Secrets in to SealedSecrets that can be decrypted by Bitnami's SealedSecret controller. https://github.com/devjoes/secret-sealer this is literally my first ever bit of Go code so any feedback would be greatly appreciated.