Presence of bearer token should cancel exec action #91745

anderseknert · 2020-06-03T22:42:00Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
If a bearer token is present in a request/config, the exec credential plugin should accept that as the chosen method of authentication. Judging by an earlier comment in exec.go, this was already the intention. This would however not work since UpdateTransportConfig would set the GetCert callback which would then get called by the transport, triggering the exec plugin action even with a token present in the request. See linked issue for further details.

Which issue(s) this PR fixes:
Fixes #87369

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Fixed ambiguous behavior when bearer token (kubectl --token=..) and an exec credential plugin was configured in the same context - the bearer token now takes precedence.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Signed-off-by: Anders Eknert anders.eknert@bisnode.com

k8s-ci-robot · 2020-06-03T22:42:09Z

Hi @anderseknert. 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.

anderseknert · 2020-06-03T22:50:10Z

/assign @liggitt

liggitt · 2020-06-08T13:54:09Z

/unassign
/assign @enj

fedebongio · 2020-06-09T20:04:21Z

/remove-sig api-machinery

enj

LGTM, but I would like to see a kubectl get --token $TOKEN pods style test to prove that this actually overrides the exec plugin in the way that we expect it to.

staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec.go

enj · 2020-06-11T05:39:21Z

/ok-to-test
/priority important-longterm
/milestone v1.19

enj

Minor comments.

test/cmd/authentication.sh

hack/make-rules/test-cmd.sh

anderseknert · 2020-07-01T21:43:48Z

Added pre-condition check for client certificate authentication capabilities and a test without --token to make sure the plugin is triggered. @enj - let me know if the last commit looks OK and I'll squash it.

anderseknert · 2020-07-01T23:17:14Z

Integration tests not giving the same output on the CI server as on my local machine. Will look into it further tomorrow.

hack/lib/util.sh

If a bearer token is present in a request, the exec credential plugin should accept that as the chosen method of authentication. Judging by an [earlier comment in exec.go](https://github.com/kubernetes/kubernetes/blob/c18bc7e9f7a27d7d197053d98c52896e1dc3bb3e/staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec.go#L217), this was already intended. This would however not work since UpdateTransportConfig would set the GetCert callback which would then get called by the transport, triggering the exec plugin action even with a token present in the request. See linked issue for further details. See kubernetes#87369 for further details. Signed-off-by: Anders Eknert <anders.eknert@bisnode.com>

anderseknert · 2020-07-02T14:38:10Z

OK @enj - now working both locally and on CI server. I've rebased from master and squashed the commits. Quite a process for adding a nil return to a function 😄 But feels good to have this well covered! Thanks for guiding me along the way 👍

enj · 2020-07-02T20:13:37Z

now working both locally and on CI server. I've rebased from master and squashed the commits.

/lgtm

🎉

Quite a process for adding a nil return to a function 😄

I think you just described making literally any change to k/k xD

Thanks for guiding me along the way 👍

My pleasure!

enj · 2020-07-02T20:13:53Z

/retest

enj · 2020-07-02T20:15:03Z

@liggitt this is ready for you now.

liggitt · 2020-07-06T16:15:40Z

test/cmd/authentication.sh

+  ### Without provided --token, the exec credential plugin should be triggered
+  # Pre-condition: Client certificate authentication enabled on the API server - already checked by positive test above
+
+  kube_flags_without_token=('-s' "https://127.0.0.1:${SECURE_API_PORT}" '--insecure-skip-tls-verify=true')


nit: defining this far away from kube_flags_with_token is not ideal... it places assumptions about the secured endpoint/port far away from where we actually start the server

options:

define this in the same place we define kube_flags_with_token (and maybe rename it to make it clear the distinction is that it is pointing at a secured endpoint, e.g. kube_flags_secure_without_token)

use kube_flags_with_token and then append another --token= arg to clear out the token

I have a slight preference for 1

I agree on moving it next to kube_flags_with_token, and I'll do so. However, kube_flags_with_token also uses the secure port, so I'm not sure about including secure in the without_token version. Did you think of kube_flags which is defined next to it, and uses the insecure port?

Or you meant renaming both of them? Anyway, I've moved them so that they're next to each other, with the with_token version simply concatenating the token to the without_token values.

liggitt · 2020-07-06T16:16:30Z

staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec.go

+	// also configured to allow client certificates for authentication. For requests
+	// like "kubectl get --token (token) pods" we should assume the intention is to
+	// use the provided token for authentication.
+	if c.HasTokenAuth() {


if we already have a client cert/key, should that take precedence as well and prevent calling the exec plugin?

I think that makes sense, yes. This PR addressed a real problem we experienced with our exec plugin using tokens, and the client certificate scenario wasn't something I considered - I don't even know what the current behavior there is to be honest, but I could certainly look into supporting that case as well if you want. With that said, I think the single argument of --token makes it a lot more likely to be used ad-hoc than client certificates, which requires something like three(?) file locations to be provided.

I think the current state of the PR is coherent w.r.t. token auth. A follow up issue to discuss/resolve treatment of client cert auth is fine with me

liggitt

/lgtm
/approve
/retest

liggitt · 2020-07-07T18:52:29Z

staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec.go

+	// also configured to allow client certificates for authentication. For requests
+	// like "kubectl get --token (token) pods" we should assume the intention is to
+	// use the provided token for authentication.
+	if c.HasTokenAuth() {


I think the current state of the PR is coherent w.r.t. token auth. A follow up issue to discuss/resolve treatment of client cert auth is fine with me

k8s-ci-robot · 2020-07-07T18:54:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: anderseknert, liggitt

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~~ [liggitt]
~~staging/src/k8s.io/client-go/plugin/pkg/client/auth/OWNERS~~ [liggitt]
~~test/cmd/OWNERS~~ [liggitt]

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

liggitt · 2020-07-08T00:10:55Z

/retest

fejta-bot · 2020-07-08T14:51:49Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

liggitt · 2020-07-08T19:09:12Z

/retest

enj · 2020-07-09T03:06:49Z

/retest

liggitt · 2020-07-09T03:52:16Z

/retest

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 3, 2020

k8s-ci-robot requested review from liggitt and mikedanese June 3, 2020 22:42

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/auth Categorizes an issue or PR as relevant to SIG Auth. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 3, 2020

k8s-ci-robot assigned liggitt Jun 3, 2020

anderseknert force-pushed the gh-87369 branch 2 times, most recently from 4bf8456 to 27803bf Compare June 3, 2020 22:54

k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Jun 3, 2020

k8s-ci-robot assigned enj and unassigned liggitt Jun 8, 2020

k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Jun 9, 2020

enj mentioned this pull request Jun 10, 2020

External client-go credential providers kubernetes/enhancements#541

Closed

enj reviewed Jun 11, 2020

View reviewed changes

staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec.go Show resolved Hide resolved

enj reviewed Jun 11, 2020

View reviewed changes

staging/src/k8s.io/client-go/plugin/pkg/client/auth/exec/exec.go Show resolved Hide resolved

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. labels Jun 11, 2020

enj approved these changes Jun 30, 2020

View reviewed changes

test/cmd/authentication.sh Show resolved Hide resolved

hack/make-rules/test-cmd.sh Show resolved Hide resolved

anderseknert force-pushed the gh-87369 branch from 6333a7d to 5408521 Compare July 1, 2020 21:36

enj reviewed Jul 2, 2020

View reviewed changes

hack/lib/util.sh Show resolved Hide resolved

anderseknert force-pushed the gh-87369 branch from 98975cc to b423216 Compare July 2, 2020 10:12

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 2, 2020

liggitt reviewed Jul 6, 2020

View reviewed changes

Move kube_flags_without_token creation

c595f98

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 6, 2020

liggitt reviewed Jul 7, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 7, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 7, 2020

k8s-ci-robot merged commit fe43b10 into kubernetes:master Jul 9, 2020

anderseknert deleted the gh-87369 branch July 9, 2020 16:40

anderseknert mentioned this pull request Jul 17, 2020

Authentication fails one time after successful 2-FA Bisnode/kubectl-tbac#31

Closed

enj mentioned this pull request May 19, 2021

test/integration/client: test exec calls metric #102152

Merged

ankeesler mentioned this pull request May 20, 2021

exec credential provider: don't run exec plugin with basic auth #102175

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Presence of bearer token should cancel exec action #91745

Presence of bearer token should cancel exec action #91745

anderseknert commented Jun 3, 2020

k8s-ci-robot commented Jun 3, 2020

anderseknert commented Jun 3, 2020

liggitt commented Jun 8, 2020

fedebongio commented Jun 9, 2020

enj left a comment

enj commented Jun 11, 2020

enj left a comment

anderseknert commented Jul 1, 2020

anderseknert commented Jul 1, 2020

anderseknert commented Jul 2, 2020

enj commented Jul 2, 2020

enj commented Jul 2, 2020

enj commented Jul 2, 2020

liggitt Jul 6, 2020

anderseknert Jul 6, 2020

anderseknert Jul 6, 2020

liggitt Jul 6, 2020

anderseknert Jul 6, 2020

liggitt Jul 7, 2020

liggitt left a comment

liggitt Jul 7, 2020

k8s-ci-robot commented Jul 7, 2020

liggitt commented Jul 8, 2020

fejta-bot commented Jul 8, 2020

liggitt commented Jul 8, 2020

enj commented Jul 9, 2020

liggitt commented Jul 9, 2020

Presence of bearer token should cancel exec action #91745

Presence of bearer token should cancel exec action #91745

Conversation

anderseknert commented Jun 3, 2020

k8s-ci-robot commented Jun 3, 2020

anderseknert commented Jun 3, 2020

liggitt commented Jun 8, 2020

fedebongio commented Jun 9, 2020

enj left a comment

Choose a reason for hiding this comment

enj commented Jun 11, 2020

enj left a comment

Choose a reason for hiding this comment

anderseknert commented Jul 1, 2020

anderseknert commented Jul 1, 2020

anderseknert commented Jul 2, 2020

enj commented Jul 2, 2020

enj commented Jul 2, 2020

enj commented Jul 2, 2020

liggitt Jul 6, 2020

Choose a reason for hiding this comment

anderseknert Jul 6, 2020

Choose a reason for hiding this comment

anderseknert Jul 6, 2020

Choose a reason for hiding this comment

liggitt Jul 6, 2020

Choose a reason for hiding this comment

anderseknert Jul 6, 2020

Choose a reason for hiding this comment

liggitt Jul 7, 2020

Choose a reason for hiding this comment

liggitt left a comment

Choose a reason for hiding this comment

liggitt Jul 7, 2020

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 7, 2020

liggitt commented Jul 8, 2020

fejta-bot commented Jul 8, 2020

liggitt commented Jul 8, 2020

enj commented Jul 9, 2020

liggitt commented Jul 9, 2020