Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add kep for kustomize secret generator plugins #811

Merged
merged 6 commits into from
Mar 5, 2019

Conversation

sethpollack
Copy link
Contributor

No description provided.

@k8s-ci-robot k8s-ci-robot added do-not-merge/blocked-paths Indicates that a PR should not merge because it touches files in blocked paths. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/pm labels Feb 5, 2019
@k8s-ci-robot k8s-ci-robot requested review from jbeda and seans3 February 5, 2019 01:19
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 5, 2019
@sethpollack
Copy link
Contributor Author

/assign @monopole

Copy link
Contributor

@monopole monopole left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Seth!

* [Graduation Criteria](#graduation-criteria)
* [Implementation History](#implementation-history)

## Summary
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest say what the goal is, and its probably not useful to assume reader has history, e.g.

kustomize wants to generate Secret objects (link to api?), which are general key-value pairs where the value is supposed to be a secret. Reading secret values from local files raises security concerns about file lifetime and access. Reading secret values from the execution of arbitrary commands in a kustomization file introduces concerns in a world where kustomization files can be used directly from the internet. This proposal describes obtaining secret values from specific kustomize plugins...


- Kustomize will not handle plugin installation/managment.

## Proposal
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add an example of what the new secretGenerator stanza would look like in https://github.com/kubernetes-sigs/kustomize/blob/master/docs/kustomization.yaml

## Summary

Kustomize removed the `commands` feature from `SecretGenerator` due to security concerns. This proposal is proposing a safe alternative using golang plugins.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

link to golang plugins, and maybe a sentence or two as to why they are different from any old executable

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the go plugins doc:

Currently plugins are only supported on Linux and macOS.

Shouldn't the solution work on Windows since kustomize ships for Windows?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


### Risks and Mitigations

N/A
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would you frame an attack? If it sounds implausible, all the better.


## Graduation Criteria

Convert the SecretGenerator into a plugin system.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what to put here since we cannot get user data at this point - other than noting the number of people who chimed in on kubernetes-sigs/kustomize#692

@monopole
Copy link
Contributor

monopole commented Feb 7, 2019

PS, just flesh out those points, and we can immediately pull as provisional.

Wait a few days for comments?

Then go to implementable (not just the code - need examples and tests too).

@sethpollack
Copy link
Contributor Author

Ok awesome, I'll get to work on this. Thanks for the feedback!

@sethpollack
Copy link
Contributor Author

@monopole Ok updated. Let me know if it needs anymore changes?

@monopole
Copy link
Contributor

@seans3 pingaroo

Copy link
Contributor

@mattfarina mattfarina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has this been discussed in SIG CLI? If not, I would suggest discussing it there as a next step.

editor: "@sethpollack"
creation-date: 2019-02-04
last-updated: 2019-02-04
status: provisional
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add the related SIGs section to the metadata? Some are listed as labels on the PR. Also, can you add SIG Apps.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has this been discussed in SIG CLI? If not, I would suggest discussing it there as a next step.

Done on Feb 13th

- "@Liujingfang1"
approvers:
- "@monopole"
- "@Liujingfang1"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the approvers be the kustomize admin and/or maintainers? https://github.com/kubernetes-sigs/kustomize/blob/master/OWNERS_ALIASES

Copy link
Contributor

@calebamiles calebamiles Feb 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to answer a question from @lavalamp, it is my intention that approvers and reviewers are the people who did not the people who can which is the problem that the OWNERS mechanism addresses

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@calebamiles Then the fields should be called "reviewed-by" and "approved-by"? And it is weird to have those people added by the author rather than add themselves. I would not be surprised if we have many KEPs created with a different understanding of those fields.

## Summary

Kustomize removed the `commands` feature from `SecretGenerator` due to security concerns. This proposal is proposing a safe alternative using golang plugins.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the go plugins doc:

Currently plugins are only supported on Linux and macOS.

Shouldn't the solution work on Windows since kustomize ships for Windows?


### Non-Goals

- Kustomize will not handle plugin installation/management.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If kustomize is going to add plugins should it try the same model as kubectl for management?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some options for leveraging kubectl plugin model (discussed during/after sig-cli meeting):

  • Require installation of kubectl, and have kustomize execute the secret generation code as a kubectl sub-process, i.e. exec.Command(“kubectl”, “foo”, …) using the plugin via kubectl.

  • Arrange via cli-runtime refactoring to vendor kubectl plugin code into kustomize, to track changes to argument and flag parsing and the notion of plugin subcommands.

  • Vendor no code, and use a simplified kubctl plugin model: given a secret generation command foo bar, just look for a binary called kubectl-foo and run it, passing bar as an argument. This works because the kustomize use case doesn’t have to worry about command collisions, sub-commands, listing plugins, etc.

The downside to all these options is that a plugin intended only for kustomize secret generation would appear in kubectl help, and plugin lists, etc.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above comments assume familiarity with how kubectl plugins work. if interested, see https://kubernetes.io/docs/tasks/extend-kubectl/kubectl-plugins

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like option 3 the best.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are risks of command collisions here. Not right now, because nobody uses kubectl plugins ;) but imagine a world where it is normal to have several unknown kubectl-something commands in $PATH. It is highly likely that these plugins have interesting side effects when invoked, because that's the whole point of plugins.

Given that kustomization.yaml files will come from relatively-untrusted external sources, I would much prefer that this specific category of plugin was cleanly namespaced off from other possible kubectl plugins. This should be as simple as invoking kubectl-kustomize-plugin-foo ..., rather than just kubectl-foo.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do those extensions have to be in $PATH? If users are not meant to invoke them directly, then they shouldn't be.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anguslees I don't want to use kubectl style plugins. Was just listing the options as a thought experiment. Your point taken though; if we did, we could avoid collisions in kubectl with more ugly namespace. When I said the kustomize use case doesn't have to worry about command collisions, i was referring to colliding with other native kustomize commands, which isn't possible since the kustomize plugin wouldn't be used as a cobra-style command. The only trouble would be plugin-plugin collision, and we just error out if the loader detects that.

I'd never use a kustomize.yaml file from an untrusted source. Is there something in the docs that suggests such behavior is recommended?

Copy link
Member

@anguslees anguslees Feb 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd never use a kustomize.yaml file from an untrusted source.

I think the larger kustomize proposal is to have 3rd parties publish remote manifest repositories that may include kustomize.yaml (since they are in turn derived from manifests coming from another 4th party). That means we are pulling kustomize.yaml from a sort-of-untrusted source.

I agree that actually applying the remote manifests necessarily means we are trusting the remote source to give us code/config that will be executed in our local cluster. There are differences between what (and when) the remote source is able to execute in the cluster vs on the local machine, however. In particular, we need to be able to do "--dry-run" -style kubectl operations on remote yaml without endangering the local user.

(See also: the reasons the previous arbitrary-command-based kustomize secret generator was rolled back)


## Proposal

In the proposed design `SecretGenerator` would load golang plugins from `~/.kustomization/plugins`. This would limit the scope of what kustomize can execute to the plugins on the users machine. Also using golang plugins forces a strict interface with well-defined responsibilities.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can I suggest you look at the XDG spec for directory naming? https://standards.freedesktop.org/basedir-spec/basedir-spec-latest.html

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that spirit, would be fine with ~/.config/kustomization_plugins


### Risks and Mitigations

No windows support for golang plugins.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I remember correctly, plugins also have the issue where the path needs to be the same, from the root of the filesystem, for them across systems and developers. This may be fixed but it has stopped others from using them in the past. Has this been looked at?

This is a developer experience risk and I'm curious if it's no longer an issue or if there is a mitigation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you have a reference?

@mattfarina
Copy link
Contributor

It appears that windows support for go plugins is unplanned (issue here).

Helm plugins might provide a style worth looking at. Basic plugins are similar to what kubectl does. There are downloader plugins that can hook into some situations. Docs are at https://docs.helm.sh/using_helm/#downloader-plugins. Just another idea to consider. Happy to share more if anyone is interested.

@monopole
Copy link
Contributor

yes, that's a general model. we looked at it back when thinking about kubectl plugins v1.
the use case here is far more constrained (just generate a secret). we can start with Golang plugins, and allow different kinds later as the need arises.


```secretGenerator:
- name: env-example
plugins:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggest goplugins

i.e. tie the keyword to golang shared object file plugins, leaving the door open to other kinds of plugins later.

@mattfarina
Copy link
Contributor

@monopole Right now kubectl is built for Windows and has install instructions. Instead of improving test coverage or fixing some bugs for windows, the most popular developer operating system, the suggestion is to bake in a feature that doesn't work on Windows and has no plan for windows support.

Is this correct?

@akutz
Copy link
Member

akutz commented Feb 15, 2019

Hi All,

So Go plug-ins. Where do I even begin. @mattfarina is correct, there's no planned support for Windows as far as I'm aware, but more than that, Go plug-ins themselves are fraught with overestimated expectations that run quickly into some unfortunate barriers of entry due to the way Go's typing system works.

A few notes:

When to use Go plug-ins

Go plug-ins make sense in three scenarios:

  1. Plug-ins are built at the same time as the host program in order to provide a base program and additional functionality that can be enabled at runtime by loading plug-ins. Building the plug-ins at the same time as the host program, using the exact same sources, can ensure the exact same dependency graph and ensure that the plug-ins will load into the host program and use shared types to share data.
  2. Shared types are not used to share data unless:
    1. The types belong to stdlib.
    2. The types are interface{} references that can be asserted as Go interfaces defined in both the host program and in the plug-in.
  3. Information is shared by marshaling/unmarshaling data via gRPC/JSON.

Portable Go plug-ins

The approach described at https://github.com/akutz/gpds illustrates using gRPC to build stand-alone binaries that may also be compiled as Go plug-ins. A host program either loads the archives as plug-ins and controls the lifecycle of the plug-in's built-in, memory backed gRPC server, or a plug-in runs as a stand-alone process, exposing the same gRPC endpoint.

Either way, a host program simply accesses plug-ins a well-defined gRPC endpoints via a well-defined object model and API.

Problems with plug-ins

  • Go plug-ins currently only work on Linux and macOS. As far as I'm aware, there are no current plans to get plug-ins working on Windows.
  • People want to think of Go plug-ins as the same type of loadable-at-runtime feature that exists in .NET and Java, but this simply isn't true. It's not trivial for @mattfarina to build a program that accepts Go plug-ins and for me to come along a year later and create a plug-in and expect it to work. There are too many gotchas. Without a full understanding of the constraints involved or an adoption of data marshaling as the means to share information, Go plug-ins may be more trouble than they're worth.

Conclusion

I personally think Go plug-ins are really cool, and I'd love to see them in kubectl. However, because kubectl is a short-lived program, I think external binaries at a well-defined location, ex. ${HOME}/.kubectl/plugins, similar to how Terraform works, is probably a more sensible solution. However, even in this approach, I still think the gRPC pattern outlined at https://github.com/akutz/gpds lays the foundation for building extensible support as independent binaries or as Go plug-ins loaded into the kubectl host process.

@sethpollack
Copy link
Contributor Author

@monopole @pwittrock @soltysh Should we just use the terraform approach? It's a nice middle ground between go plugins and kubectl plugins.

  1. Binaries are prefixed and stored in a dedicated directory.
  2. They use grpc for a strict interface with well-defined responsibilities
  3. Works on all platforms and plugins can be written in any language.

@anguslees
Copy link
Member

anguslees commented Feb 18, 2019

  1. Works on all platforms and plugins can be written in any language.

This. I'm slightly confused where we stand on this wrt kustomize. I think we're proposing (in other KEPs) making kustomization.yaml a standard part of the "Kubernetes Experience". If we make golang plugins a user-visible part of kustomize, then we're also saying that all tools that read "kubernetes yaml" files must be implemented in golang (or call out to an external tool implemented in golang).

Everything else in the k8s public API is language-neutral (with some minor leakage around the quirks of golang's encoding semantics). We (as a k8s project) should be super-clear on whether we're ok binding all our users/implementers to a single language or not. If we want choice of programming environment then exposing a golang plugin API is obviously a non-starter, and it would be great if we stated that out loud so future (sig-cli most likely) KEPs can avoid rediscovering this objection.

For this use-case in particular, we want the freedom to choose languages, because the whole point is to integrate with some existing secrets-management system. It is quite likely that an existing system has APIs that suggest some languages over others - eg: pkcs11 modules are defined as C ABIs, which are particularly difficult to use from golang; macos credentials stores might be easier to access in a proprietary Apple language.


Personally, I would much prefer we just use simple exec-based plugins here. grpc is huge overkill for something that just takes string arguments, and returns key/value pairs(*) - in particular requiring grpc prevents me from just writing my plugin as a simple bit of shell glue around an existing command (which is what almost all of these secret plugins will be).

(*) Eg: Return value could be base64-encoded values in json to stdout

return nil, err
}

symbol, err := p.Lookup(name)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we must go with golang plugins, please don't just allow the kustomization.yaml file to invoke any symbol that happens to be exported.

A better way to do this is to have each plugin export a well-known symbol (perhaps KustomizationSecretGenerators) that is a map (or array for simplicity) of name => function. That way only explicitly marked functions will be accessible.

As a bonus, it allows the same plugin .so to be used to provide other sets of entry points (with different function signatures) in the future. This includes versioning the well-known symbol so we can cleanly evolve the ABI over time.

Copy link
Contributor

@monopole monopole Feb 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anguslees Policy around exposed vars sounds reasonable.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 27, 2019
@pwittrock
Copy link
Member

pwittrock commented Feb 27, 2019

@sethpollack yes the doc should reflect the design we plan to implement. which are the new design details you are referring to? I think we had reached agreement to move forward with a go-plugin approach for iteration 1, and leave exec for iteration 2.

@monopole
Copy link
Contributor

monopole commented Feb 27, 2019

@pwittrock We were only discussing field names, i.e. type vs pluginType.

That's a level of nitpicking best left to the implementation PR. This KEP is done (modulo spelling?)

@monopole
Copy link
Contributor

/lgtm

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

given that most of the above comments are against doing go-plugins, and given that we seem to instead be going with go-plugins, can we at least provide a reference plugin that calls exec and wraps so those of us with existing tools aren't broken?

@sethpollack
Copy link
Contributor Author

@donbowman existing tools are already broken, the exec functionality was removed a while ago. We do plan on adding other plugin types like exec as well.

@mattfarina
Copy link
Contributor

@sethpollack This KEP should capture the plan for plugins. With exec plugins are not listed in the plans. If they are in the plan shouldn't that be listed here? Maybe as part of a multi-phase approach.

@monopole
Copy link
Contributor

monopole commented Mar 4, 2019

@sethpollack Please address above comments by making the following changes:

  1. Update The proposed API would look like this: with what we agreed on above:
secretGenerator:
- name: app-tls
  kvSources:
  - pluginType: builtin  // builtin is the default value of pluginType
    name: files
    args:
    - secret/tls.cert
    - secret/tls.key
- name: env_file_secret
  kvSources:
  - name: env  // this is a builtin
    args:
    - env.txt
- name: myJavaServerEnvVars
  kvSources:
  - name: literals    // this is a builtin
    args:
    - JAVA_HOME=/opt/java/jdk
    - JAVA_TOOL_OPTIONS=-agentlib:hprof
- name: secretFromPlugins
  kvSources:
  - pluginType: go      // described by this KEP
    name: someLocalGoPlugin
    args:
    - someArg
    - someOtherArg
  - pluginType: kubectl      // some future KEP can write this
    name: someKubectlPlugin
    args:
    - anotherArg
    - yetAnotherArg
  1. Update Risks and Mitigations

Risks and Mitigations

Several people want an exec-style plugin

exec-style means execute arbitrary code from some file.

Most the lines of code written to implement this KEP
accomodate the notion of KV generation via a generic
notion of a plugin, and a generic interface, in the
kustomization file and associated structs and code.

This code recharactizes the three existing KV
generation methods as pluginType: builtin
(literals, env and files), introduces the new
pluginType: Go, and leaves room for someone to easily
add, say pluginType: kubectl to look for a kubectl
plugin, and pluginType: whatever to handles some
completely new style, e.g. look for the plugin name as
an executable in some hardcoded location.

Actual implementation of these other kinds of plugins
are out of scope for this KEP, however this KEP's
implementation will make it much easier to create other
kinds of plugins.

Go Plugins that become popular have a clear
path to becoming a builtin - so if someone writes, say,
a general exec-style plugin, we can easily promote it to a
builtin (by virtue of the fact that it's written in Go,
and because of the choices made in this KEP for
describing a plugin in the kustomization file).

No current windows support for golang plugins.

This may be implemented by someone later in which
case goplugins will work on windows. Go itself was
not supported on windows when first introduced.

Also, as noted above, someone can write an exec style plugin
which will work on Windows.

General symbol exection from the plugin

A Go plugin will be cast to a particular hard-coded interface, e.g.

type KvSourcePlugin {
  Get []kvPairs
}

and accessed exclusively through that interface.

Existing KV generators continue as an option

We leave in place the existing three mechanisms
(literals, env and files) for generating KV pairs, but
additionally allow these mechanisms to be expressed as
builtin plugins (see example above).

If plugins - both Go and other styles - prove unpopular
or problematic, we can remove them per API change
policies.

If they do prove popular/useful, we can deprecate
the legacy form for (literals, env and files),
and help people convert to the new builtin form.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 4, 2019
@monopole
Copy link
Contributor

monopole commented Mar 4, 2019

Ok, that addresses the final concerns from @donbowman and @mattfarina, thanks!

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 4, 2019
@monopole
Copy link
Contributor

monopole commented Mar 4, 2019

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 4, 2019
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 4, 2019
@monopole
Copy link
Contributor

monopole commented Mar 4, 2019

/lgtm

thanks for the spelling fixes

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

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

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

@monopole
Copy link
Contributor

monopole commented Mar 4, 2019

/retest

@monopole
Copy link
Contributor

monopole commented Mar 4, 2019

/test pull-enhancements-verify

@donbowman
Copy link

the kep is closed and merged, but... Here is a sample https://www.agilicus.com/safely-secure-secrets-a-sops-plugin-for-kustomize/ to show how to use it with sops https://github.com/mozilla/sops (since this is the top result in search and i didn't find one)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/cli Categorizes an issue or PR as relevant to SIG CLI. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.