-
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
[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 |
@@ -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.
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.
This has been fixed
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?.
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.
done
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?
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 have pushed changes addressing this, opting for the flat structure.
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.
Overall looks good to me. I'll defer the final approval from SIG-CLI POV to @soltysh
reason. The user should instead use `credentialPluginPolicy: DisableAll` in | ||
this case. | ||
|
||
Commands that don't create a client, such as `kubectl config view` will not be |
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.
is there any other command that will be exceptional like kubectl config view?
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.
Off the top of my head, there are
kubectl config
(any subcommand)
kubectl version
kubectl plugin list
Should I update the KEP with a full list?
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.
hmm... I think it's going to be hard to enumerate every specific command that won't actually use the client. There's lots of commands that only run locally with specific options (e.g. all kubectl create ... --dry-run=client
commands, or kubectl {label,annotate,set} --local
)
I think the implementation should be structured so that it intercepts attempts to use a non-allowlisted credential plugin and rejects them at time of use, so commands that don't even need the plugin don't start failing for no reason.
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 the implementation should be structured so that it intercepts attempts to use a non-allowlisted credential plugin and rejects them at time of use, so commands that don't even need the plugin don't start failing for no reason.
So that significantly changes the implementation. Maybe we just need to get the list of plugins (allowed, disallowed, etc.) in kubectl/cmd and pass it to client-go (or cli-runtime) to accept/reject this credential exec plugin, when there is a request to API Server?
I think, this PR needs a rebase to remove the invalid-commit label. Otherwise, this PR can't be merged. |
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 <pmengelbert@gmail.com>
- Describe behavior when allowlist is `nil` - Describe behavior when allowlist is empty - Describe future plans for field additions Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>
Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>
Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>
Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>
49a8abd
to
7e85530
Compare
In the credential plugin allowlist, consider nil and empty lists an error. Jordan Liggitt has pointed out that treating them differently is generally avoided in APIs, as it can confuse users. In order to make the user's intention more explicit, an additional field, `credentialPluginPolicy` has been added. Its value must be one of `EnableAll | DisableAll | Allowlist`. `EnableAll` and `DisableAll` are self-explanatory and do not require an allowlist. `Allowlist` will use the allowlist as defined in the KEP. Note that this change considers the following cases to be errors: - a policy of `EnableAll` with an allowlist provided - a policy of `DisableAll` with an allowlist provided - a policy of `Allowlist` with no allowlist provided (i.e. a `nil` allowlist` - a policy of `Allowlist` with an explicitly empty allowlist provided Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>
Signed-off-by: Peter Engelbert <pmengelbert@gmail.com>
7e85530
to
23506b7
Compare
Just rebased, and fixed that commit. |
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
).