Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 73 additions & 1 deletion keps/sig-cli/3104-introduce-kuberc/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ tags, and then generate with `hack/update-toc.sh`.
- [Story 2](#story-2)
- [Story 3](#story-3)
- [Story 4](#story-4)
- [Story 5](#story-5)
- [Notes/Constraints/Caveats (Optional)](#notesconstraintscaveats-optional)
- [Open Questions](#open-questions)
- [Risks and Mitigations](#risks-and-mitigations)
Expand All @@ -104,6 +105,7 @@ tags, and then generate with `hack/update-toc.sh`.
- [option](#option-1)
- [prependarg](#prependarg)
- [appendarg](#appendarg)
- [Allowlist Design Details](#allowlist-design-details)
- [Test Plan](#test-plan)
- [Prerequisite testing updates](#prerequisite-testing-updates)
- [Unit tests](#unit-tests)
Expand Down Expand Up @@ -273,6 +275,10 @@ As a user I would like to be able to opt out of deprecation warnings.

https://github.com/kubernetes/kubectl/issues/1317

#### Story 5

As a user I would like to be able to prevent the execution of untrusted binaries by the client-go credential plugin system.

### Notes/Constraints/Caveats (Optional)

<!--
Expand Down Expand Up @@ -331,6 +337,11 @@ fields (`apiVersion`, `kind`):

* `aliases` Allows users to declare their own command aliases, including options and values.
* `defaults` Enables users to set default options to be applied to commands.
* `credentialPluginPolicy` (available in **kubectl.config.k8s.io/v1beta1**)
Allows users to deny all, alow all, or allow some client-go credential plugins.
* `credentialPluginAllowlist` (available in **kubectl.config.k8s.io/v1beta1**)
Enables users to specify criteria for trusting binaries to be executed by
the client-go credential plugin system.

`aliases` will not be permitted to override built-in commands but will take
precedence over plugins (builtins -> aliases -> plugins). Any additional options
Expand All @@ -343,6 +354,7 @@ 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.


```
apiVersion: kubectl.config.k8s.io/v1beta1
kind: Preference
Expand All @@ -368,6 +380,10 @@ defaults:
- name: interactive
default: "true"

credentialPluginPolicy: Allowlist
credentialPluginAllowlist:
- command: cloudplatform-credential-helper
- command: custom-credential-script
```

### Kubectl Kuberc Management Command (kubectl kuberc)
Expand Down Expand Up @@ -441,12 +457,68 @@ This gives us the opportunity to standardize kuberc files.

#### prependarg

`--prependarg` is an arbitrary list of strings that accepts anything in a string array format.
`--prependarg` is an arbitrary list of strings that accepts anything in a string array format.

#### appendarg

`--appendarg` is an arbitrary list of strings that accepts anything in string array format.

### Allowlist Design Details

`credentialPluginAllowlist` allows the end-user to provide an array of objects
describing required conditions for executing a credential plugin binary. The
overall result of a check against the allowlist will be the logical OR of the
individual checks against the allowlist's entries. Each allowlist entry MUST
have at least one nonempty field describing the conditions required for the
plugin's execution. If multiple fields are specified within an entry, the
binary in question must meet all of the required conditions in that entry in
order to be executed (i.e. they are combined with logical AND).

Each element in the allowlist is a set of criteria; if the binary in question
meets all of the criteria in at least one **set** of criteria, the plugin will
be allowed to 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.

At the outset, the entry object will have only one field, `command`. The path of
the binary specified in the kubeconfig will be compared against that named in
the `command` field. This field may contain a basename, or the full path of a
plugin. To ensure an exact match, `exec.LookPath` will be called on both the
`command` field and the binary named in the kubeconfig. The resulting absolute
paths must match. The following table illustrates this:


| Scenario | `PATH=` | Allowlist `command` | `exec.LookPath(allowlist.Command)` | kubeconfig `command` | `exec.LookPath(execConfig.Command)` | success? |
|----------|---------|---------------------|------------------------------------|----------------------|-------------------------------------|----------|
| kubeconfig lists full path; `my-binary` is in both `/usr/local/bin` and `/usr/bin` | `PATH=/usr/local/bin:/usr/bin:<...>` | my-binary | /usr/local/bin/my-binary | /usr/bin/my-binary | /usr/bin/my-binary | false |
| kubeconfig lists full path; `my-binary` is only in `/usr/local/bin` | `PATH=/usr/local/bin:/usr/bin:<...>` | my-binary | /usr/local/bin/my-binary | /usr/bin/my-binary | /usr/bin/my-binary | false |
| kubeconfig lists full path; `my-binary` is only in `/usr/bin` | `PATH=/usr/local/bin:/usr/bin:<...>` | my-binary | /usr/bin/my-binary | /usr/bin/my-binary | /usr/bin/my-binary | true |
| kubeconfig lists full path; `my-binary` is only in `/usr/bin` | `PATH=/usr/local/bin:/usr/bin:<...>` | /usr/bin/my-binary | /usr/bin/my-binary | /usr/bin/my-binary | /usr/bin/my-binary | true |
| kuberc lists full path; `my-binary` is only in `/usr/bin` | `PATH=/usr/local/bin:/usr/bin:<...>` | /usr/bin/my-binary | /usr/bin/my-binary | my-binary | /usr/bin/my-binary | true |
| kuberc lists full path; `my-binary` is in `/usr/local/bin` | `PATH=/usr/local/bin:/usr/bin:<...>` | /usr/bin/my-binary | /usr/bin/my-binary | my-binary | /usr/local/bin/my-binary | false |
| neither lists full path; `my-binary` is in `/usr/bin`; equivalent to basename match | `PATH=/usr/local/bin:/usr/bin:<...>` | my-binary | /usr/bin/my-binary | my-binary | /usr/bin/my-binary | true |

If `credentialPluginPolicy` is set to `Allowlist`, but a
`credentialPluginAllowlist` is not provided, it will be considered an
configuration error. Rather than guess at what the user intended, the operation
Copy link
Contributor

Choose a reason for hiding this comment

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

Configuration error should error out during parsing not during execution. We already have a separate validation step which I'd expect to be extended with validation for this new addition.

will be aborted just before the `exec` call. An error describing the
misconfiguration will be returned. This is because the allowlist is a security
control, and it is likely the user has made a mistake. Since the output may be
long, it would be easy for a security warning to be lost at the beginning of
the output. An explicitly empty allowlist (i.e. `credentialPluginAllowlist: []`),
in combination with `credentialPluginPolicy: Allowlist` will be considered an
error for the same 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
affected by the allowlist. Additionally, commands that create but do not *use*
a client (such as commands run with `--dry-run`) will likewise remain
unaffected.

In future updates, other allowlist entry fields MAY be added. Specifically,
fields allowing for verification by digest or public key have been discussed.
The initial design MUST accommodate such future additions.

### Test Plan

<!--
Expand Down
4 changes: 3 additions & 1 deletion keps/sig-cli/3104-introduce-kuberc/kep.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,16 @@ authors:
- "@soltysh"
owning-sig: sig-cli
participating-sigs:
- sig-cli
- sig-auth
- sig-api-machinery
status: implementable
creation-date: 2022-06-13
reviewers:
- "@liggitt"
- "@eddiezane"
- "@soltysh"
- "@mpuckett159"
- "@enj"
approvers:
- "@ardaguclu"

Expand Down