-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[KEP-3104] Update KEP with credential plugin allowlist #5479
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
base: master
Are you sure you want to change the base?
[KEP-3104] Update KEP with credential plugin allowlist #5479
Conversation
Currently, the kubeconfig may specify arbitrary binaries to run as client-go credential plugins. Due to the fact that kubeconfig files are often generated, and because they contain a lot of noise, there is significant friction in manually inspecting the kubeconfig for suspicious binaries after it is generated. To encourage secure behavior, we want to introduce an allowlist to the kuberc, which will describe the conditions under which a binary plugin may be run. Currently the only condition is the name (absolute path or basename of a binary found in `PATH`). Signed-off-by: Peter Engelbert <[email protected]>
- Describe behavior when allowlist is `nil` - Describe behavior when allowlist is empty - Describe future plans for field additions Signed-off-by: Peter Engelbert <[email protected]>
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pmengelbert The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Welcome @pmengelbert! |
Hi @pmengelbert. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
/sig auth |
This LGTM from a SIG Auth perspective (I added us as a participating SIG). |
/ok-to-test |
Signed-off-by: Peter Engelbert <[email protected]>
Signed-off-by: Peter Engelbert <[email protected]>
@@ -331,6 +337,31 @@ intended behavior and realizing that targeting options effectively addresses the | |||
use cases. During command execution, a merge will be occur, with inline overrides | |||
taking precedence over the defaults. | |||
|
|||
`credentialPluginAllowlist` allows the end-user to provide an array of objects |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, it makes sense to add an example to show how this will be formed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is an example in this same PR, below. Or are you asking for something different?
binary in question must meet all of the required conditions in that entry in | ||
order to be executed. At the outset, the entry object will have only one field, | ||
`name`. The path of the binary specified in the kubeconfig will be compared | ||
against that named in the `name` field. This field may contain a basename, or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this support $PATH/cred_plugin
?. Does full path cover ~/cred_plugin
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually this depends on the behavior of os.LookPath
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reminds me: the function is exec.LookPath
, not os.LookPath
. I made a mistake in this PR and will update it accordingly.
exec.LookPath
doesn't do tilde expansion, but that is POSIX shell behavior and not exec syscall behavior (at least on linux). I believe we should stop short of emulating POSIX substitution rules; the user can add e.g. $HOME/bin
to their PATH if they want.
execute. If no criteria set succeeds after comparing the binary to all sets of | ||
criteria, the operation will be immediately aborted and an error returned. If | ||
`credentialPluginAllowlist` is not provided, or is explicitly made `nil`, all | ||
binaries will be allowed. If `credentialPluginAllowlist`'s value is set to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, we should mention about this list will not be applied for some commands such as kubectl config view
and the reason?.
criteria, the operation will be immediately aborted and an error returned. If | ||
`credentialPluginAllowlist` is not provided, or is explicitly made `nil`, all | ||
binaries will be allowed. If `credentialPluginAllowlist`'s value is set to the | ||
empty list `[]`, *all binaries will be prohibited*. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is API behavior. So I wonder opinions from @liggitt.
credentialPluginAllowlist: nil -- all allowed
credentialPluginAllowlist: [] -- all prohibited
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we try to avoid having an empty list and a nil list behave differently ... it tends to confuse users, and some tools that generate config files will not preserve that distinction
instead, having something explicit in the config to indicate this, either as a fixed special value in the list (e.g. credentialPluginAllowlist: ["none"]
or credentialPluginAllowlist: [""]
) or as a distinct field that works together with the allowlist field (e.g. credentialPluginPolicy: EnableAll | DisableAll | Allowlist
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Of the available options, I prefer the explicit policy. I'd prefer to have these nested under a single top-level grouping, such as
credentialPlugin:
policy: Allowlist
allowlist:
- name: credential-plugin.sh
@ardaguclu @liggitt thoughts on the above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Playing out what it would look like for the three possibly ways they'd want to configure this:
Allow all
absent / default
explicit with nested structure:
credentialPlugin:
policy: AllowAll
explicit with peer structure
credentialPluginPolicy: AllowAll
Allow none
nested structure:
credentialPlugin:
policy: DisableAll
flat structure:
credentialPluginPolicy: DisableAll
Allowlist
nested structure:
credentialPlugin:
policy: Allowlist
allowlist: [...]
flat structure:
credentialPluginPolicy: Allowlist
credentialPluginAllowlist: [...]
The flat structure seems simpler for the existing cases, and avoids the possibility of ~invalid constructions like credentialPlugin: {}
. Are there any other credentialPlugin preferences / options we anticipate wanting to group along with these?
Currently, the kubeconfig may specify arbitrary binaries to run as
client-go credential plugins. Due to the fact that kubeconfig files are
often generated, and because they contain a lot of noise, there is
significant friction in manually inspecting the kubeconfig for
suspicious binaries after it is generated.
To encourage secure behavior, we want to introduce an allowlist to the
kuberc, which will describe the conditions under which a binary plugin
may be run. Currently the only condition is the name (absolute path or
basename of a binary found in
PATH
).