Ensure static certs in kubeconfig override exec plugin #107410

margocrawf · 2022-01-07T17:02:01Z

Check whether static cert is already configured in UpdateTransportConfig

Also update test-cmd.sh to pass a signing ca to the kube controller
manager, so CSRs work properly in integration tests.

/kind bug

Fixed a bug that caused credentials in an exec plugin to override the static certificates set in a kubeconfig.

k8s-ci-robot · 2022-01-07T17:02:11Z

Hi @margocrawf. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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/test-infra repository.

enj · 2022-01-07T17:20:08Z

/triage accepted
/assign
/ok-to-test
/milestone v1.24
/priority important-longterm

neolit123 · 2022-01-07T18:04:29Z

This seems like unexpected behavior because other statically configured credentials (i.e., basic auth, bearer token) take precedent over the stuff returned by the exec plugin.

Dynamic (plugins) taking precedence over static config seems like the more intuative pattern to me. But i am not very familiar with area of the code and it seems that making this alternative switch now can be more breaking.

enj · 2022-03-02T14:40:14Z

hack/testdata/auth/testcert.csr

@@ -0,0 +1,15 @@
+-----BEGIN CERTIFICATE REQUEST-----


For all of these new testdata files, add:

A comment describing how they were generated

A note saying that this is a test-only credential and that no security vulnerabilities should be reported in regards to them being published to a public repo

enj · 2022-03-02T14:43:53Z

test/cmd/authentication.sh

+metadata:
+  name: testcert
+spec:
+  request: $(base64 < hack/testdata/auth/testcert.csr | tr -d '\n')


It is unclear what testcert.csr encodes.

enj · 2022-03-02T14:48:31Z

test/cmd/authentication.sh

+  kubectl config set-credentials testcert --client-certificate="${TMPDIR:-/tmp}"/testcert.crt --client-key="hack/testdata/auth/testcert.key" --exec-api-version=client.authentication.k8s.io/v1beta1 --exec-command=/tmp/invalid_execcredential.sh
+  output6=$(kubectl  "${kube_flags_without_token[@]:?}" --user testcert get namespace kube-system -o name)
+  if [[ "${output6}" =~ "Unauthorized" ]]; then
+    kube::log::status "Unexpected output when providing --client-certificate/--client-key for authentication - exec credential plugin likely triggered. Output: ${output6}"


This error message looks incorrect since this is testing the kubeconfig based cert creds.

enj · 2022-03-02T14:49:57Z

test/cmd/authentication.sh

+  kube::test::get_object_assert 'csr/testcert' '{{range.status.conditions}}{{.type}}{{end}}' ''
+  kubectl certificate approve testcert
+  kube::test::get_object_assert 'csr/testcert' '{{range.status.conditions}}{{.type}}{{end}}' 'Approved'
+  kubectl get csr testcert -o jsonpath='{.status.certificate}' | base64 -d > "${TMPDIR:-/tmp}"/testcert.crt


Use kube::test::wait_object_assert or something equivalent to poll for this, otherwise you are racing the signer and this test will flake in CI.

enj · 2022-03-02T15:01:53Z

This seems like unexpected behavior because other statically configured credentials (i.e., basic auth, bearer token) take precedent over the stuff returned by the exec plugin.

Dynamic (plugins) taking precedence over static config seems like the more intuative pattern to me. But i am not very familiar with area of the code and it seems that making this alternative switch now can be more breaking.

To be consistent with our behavior for tokens, static config must have precedence, i.e. #99603 (comment). In particular, CLI flags are considered static config, and they must override the client-go credential plugin.

In an ideal world, we would detect the difference between CLI flags and kubeconfig data, and make the combination of the latter and a client-go credential plugin result in an error (since it is ambiguous what credentials you want to use). With CLI flags, it is clear what credentials the user is intending to use, and that is what this change addresses.

I am okay with saying that static kubeconfig credentials takes precedence over client-go credential plugins simply because picking either is wrong/ambiguous, but at least then we are consistent across tokens/certs.

- Also update test-cmd.sh to pass a signing ca to the kube controller manager, so CSRs work properly in integration tests. Signed-off-by: Margo Crawford <margaretc@vmware.com>

enj · 2022-03-08T16:45:13Z

/lgtm
/approve

k8s-ci-robot · 2022-03-08T16:45:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enj, margocrawf

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:

~~hack/OWNERS~~ [enj]
~~staging/src/k8s.io/client-go/plugin/pkg/client/auth/OWNERS~~ [enj]
~~test/OWNERS~~ [enj]

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

margocrawf · 2022-03-08T17:57:05Z

/retest

k8s-ci-robot added area/test sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Jan 7, 2022

margocrawf force-pushed the master branch from 9cf2ff0 to 4415511 Compare January 7, 2022 17:03

k8s-ci-robot requested review from KnVerey and neolit123 January 7, 2022 17:03

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jan 7, 2022

k8s-ci-robot assigned enj Jan 7, 2022

k8s-ci-robot added this to the v1.24 milestone Jan 7, 2022

k8s-ci-robot removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jan 7, 2022

margocrawf force-pushed the master branch from 4415511 to 7f1675e Compare January 7, 2022 19:38

enj added this to Needs Triage in SIG Auth Old Jan 10, 2022

enj moved this from Needs Triage to In Review in SIG Auth Old Jan 31, 2022

enj suggested changes Mar 2, 2022

View reviewed changes

SIG Auth Old automation moved this from In Review to Changes Requested Mar 2, 2022

margocrawf force-pushed the master branch 2 times, most recently from c02605c to 16e93d8 Compare March 7, 2022 16:57

Check whether static cert is already configured in UpdateTransportConfig

f015fd6

- Also update test-cmd.sh to pass a signing ca to the kube controller manager, so CSRs work properly in integration tests. Signed-off-by: Margo Crawford <margaretc@vmware.com>

margocrawf force-pushed the master branch from 16e93d8 to f015fd6 Compare March 7, 2022 21:54

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 8, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 8, 2022

k8s-ci-robot merged commit 19935de into kubernetes:master Mar 8, 2022

SIG Auth Old automation moved this from Changes Requested to Closed / Done Mar 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure static certs in kubeconfig override exec plugin #107410

Ensure static certs in kubeconfig override exec plugin #107410

margocrawf commented Jan 7, 2022 •

edited

k8s-ci-robot commented Jan 7, 2022

enj commented Jan 7, 2022

neolit123 commented Jan 7, 2022

enj Mar 2, 2022

enj Mar 2, 2022

enj Mar 2, 2022

enj Mar 2, 2022

enj commented Mar 2, 2022

enj commented Mar 8, 2022

k8s-ci-robot commented Mar 8, 2022

margocrawf commented Mar 8, 2022

Ensure static certs in kubeconfig override exec plugin #107410

Ensure static certs in kubeconfig override exec plugin #107410

Conversation

margocrawf commented Jan 7, 2022 • edited

k8s-ci-robot commented Jan 7, 2022

enj commented Jan 7, 2022

neolit123 commented Jan 7, 2022

enj Mar 2, 2022

Choose a reason for hiding this comment

enj Mar 2, 2022

Choose a reason for hiding this comment

enj Mar 2, 2022

Choose a reason for hiding this comment

enj Mar 2, 2022

Choose a reason for hiding this comment

enj commented Mar 2, 2022

enj commented Mar 8, 2022

k8s-ci-robot commented Mar 8, 2022

margocrawf commented Mar 8, 2022

margocrawf commented Jan 7, 2022 •

edited