address pr comments

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

@@ -43,7 +43,7 @@ Example goal to enable:
 - Alice wants to develop an Application requiring a shared Secret, and to deploy it on Kubernetes using gitops
 - Alice wants her gitops deployment mechanism to pull the Secrets that it deploys from an
   remote source without writing the Secrets as files to local disk.
-- Alice's organization configures the gitops deployment container to run kustomize in the cluster
+- Alice's organization configures the gitops deployment container to run Kustomize in the cluster
   and be capable of pulling Secrets from remote locations
 - Alice writes her kustomization.yaml to use the generation options configured by her organization.
 
@@ -52,16 +52,16 @@ Example exploit to avoid:
 - 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
+  will run a script to generate some Secret
 - Alice runs `kubectl apply -k https://github.com/carl/mysql` and has the credentials
   of all of her Kubernetes clusters sent to Carl when the Secret is generated.
 
 See [692][execRemoval] for more details.
 
 ## Motivation
 
-The ability to create Kubernetes Secrets generated by commands is a commonly requested user.
-This is useful for use cases where the user doesn't want the Secrets to be appear decrypted
+The ability to create Kubernetes Secrets generated by commands is commonly requested by users.
+This is useful for use cases where the user does not want the Secrets to be appear decrypted
 on disk before being applied to the cluster.
 
 Examples:
@@ -83,23 +83,28 @@ invoke the tools they already depend upon.
 
 ### Non-Goals
 
-- Enable an ecosystem of users authoring plugins and sharing them
-- Reduce friction for publicly published whitebox `kustomization.yaml`s to generate Secrets
-
+- Enable an ecosystem of users (i.e. not centralized within a single organization) authoring plugins and sharing them with one another
+- Eliminate friction for publicly published whitebox `kustomization.yaml`s to generate Secrets
 
 ## Proposal
 
-Re-introduce `exec` Secret generators, but require the executables to have a whitelisted prefix
-and require a flag.
+Re-introduce `exec` SecretGenerators, but with the following safeguards in place to address security concerns.
+
+- Exec SecretGeneration must be explicitly enabled with a flag.  If it is not enabled and is used, Kustomize will exit 1 without running exec.
+- Executables are restricted to flag defined paths - defaulted a location that is empty by default.  Users must install / symlink executables to this location.
+- Executables may optionally be restricted to a flag defined name prefix.  - e.g. Allowing only git or kubectl plugins to be used for SecretGeneration.
+
+### New Flags
+
+This KEP recommends that the following flag conventions are adopted for secret generator plugins to which they apply.
+
+Convention: `--sg-<plugin-type>-<plugin-feature>`
 
-- Introduce a flag `--secret-generator-exec-prefix` defaulted to `kustomize-sg-`
-- Introduce a flag `--enable-exec-secret-generator` defaulted to `false`
-- Commands provided to generate Secrets *must* start with the prefix, or kustomize will exit 1
-- The enable flag *must* be provided if an exec Secret Generator is specified, or kustomize will exit 1
-- 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
+|Feature   | Name                                   | Description               | Default                                        |  Type |
+|----------|----------------------------------------|-----------------------|----------------------------------------------------| ------|
+| `enabled`| `--sg-exec-enabled`  | Enable the `exec` SecretGenerator plugin.  If an `exec` SecretGenerator is used and this flag is false, Kustomize will exit 1 without executing any plugins.   | `false` | `bool` |
+| `path`   | `--sg-exec-path`     | List of relative or absolute paths to look for executables.  Relative paths must are relative to the `kustomization.yaml` provided to Kustomize (e.g. not relative to any base `kustomization.yaml`s).  Will never lookup from user's PATH.  | [`"/usr/local/kustomize/sg"`] | `[]string` |
+| `prefix` | `--sg-exec-prefix`   | All SecretGenerator executables must have this prefix or Kustomize will exit 1 without executing any of the plugins.  If set to "", then no prefix is used.   | `""` | `string` |
 
 ## Risks and Mitigations
 
@@ -111,39 +116,23 @@ Required steps to exploit:
 
 - Alice executes (via `-k` or `kustomize`) an untrusted kustomization.yaml containing
   a SecretGenerator
-- Alice opted-in to generation by providing `--enable-exec-secret-generator`
-- Alice "installed" the targeted SecretGenerator locally on their PATH
-  - Added the command starting with `kustomize-sg-` (or whatever they provided via the flag) to her `PATH`
+- Alice opted-in to generation by providing `--sg-exec-enabled`
+- Alice "installed" the targeted SecretGenerator to `/usr/local/kustomize/sg` or a location she provided
+  with `--sg-exec-path`
 - The installed SecretGenerator can be provided arguments in a way that can be exploited
   - e.g. A command like `cat` would not be possible for Carl to exploit in a meaningful way
 
 Analysis:
 
-This is a very low risk profile, with Alice having to install the targeted binary on their
-PATH as a kustomize plugin (using the name-prefix), the binary would have to be one that could
-be exploited by Carl, Alice would need to run kustomize against an untrusted `kustomization.yaml`,
-and Alice would need to provide the flag `--enable-exec-secret-generator`.  If Carl was able
-to get Alice to perform these steps, he would likely be able to get her to run
-`$ curl <site> | bash` which would be a more effective technique.
-
-### Easy of Use
-
-**Risk:** This is too hard or confusing for users to use.
-
-Required steps to use:
-
-- Alice creates a script to generate the Secret called `kustomize-sg-my-cool-script.sh`
-- Alice adds the script to her PATH
-- Alice authors the `kustomization.yaml` with a SecretGenerator invoking the script
-- Alice runs kustomize with `--enable-exec-secret-generator`
-
-Analysis:
+This is a very low risk profile, with Alice having opt-in to executing binaries through several steps.
 
-There are a small number of simple and well defined steps that ensure Alice explicitly configures
-her environment so that kustomize can execute specific commands.
+- Installing / Linking the binary to be executed to a specific location for Kustomize to find it
+- Enabling SecretGeneration as a flag
 
 ## Alternatives Considered
 
+### Plugins
+
 Create a plugin mechanism similar to git or kubectl plugins and exec to plugins only.
 
 - Plugin mechanisms such as those used by git and kubectl provide a better ux experience when
@@ -153,6 +142,12 @@ Create a plugin mechanism similar to git or kubectl plugins and exec to plugins
   existing tools that users already use, rather than developing new tools.
 - Plugin mechanisms don't offer additional security features over what is being proposed here, as
   they use the same mechanism with an implicit UX.
+
+### Support less
+
+Only allow prefix or path.
+
+- Provides less flexibility
 
 ## Graduation Criteria
 
@@ -165,25 +160,28 @@ such is relatively well understood.
 
 Gather user feedback.  Determine if there are addressable gaps.
 
+Consider the following:
+
+#### Environment Variables
+
+Also exposing functionality configured with flags as Environment Variables
+
+#### Audit Command
+
+Add a new command in Kustomize: `kustomize audit secret-generator`
+
+This will print out which commands (including args and flags) will be invoked when Kustomize is run,
+without actually invoking them.  This will generally be helpful for users to view how secrets are generated.
+
 ## Testing and documentation.
 
 ### Testing
 
-- Verify the that if exec is used with a command that doesn't have the prefix, an error is returned
-  without the command being executed.
-  - If multiple commands exist, and some have the prefix and some don't, then none of the commands should be executed
-- Verify that if the user specifies the prefix flag, the new prefix is used.
-  - Commands with the new prefix should work
-  - Commands with the default prefix should now fail
-- Verify that if the enable flag is not provided, kustomize will exit 1 if and exec generators are provided
-  - Even if they match the prefix
-- Verify absolute paths fail
-- Verify relative paths fail
-- Verify the happy case
+TBD
 
 ### Documentation
 
-Update the "Kubectl Book" Secret Generator chapter.
+TBD
 
 ## Implementation History