Skip to content
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

Changed kubectl config view to redact user token #88985

Merged
merged 1 commit into from Mar 18, 2020

Conversation

@brianpursley
Copy link
Contributor

brianpursley commented Mar 9, 2020

What type of PR is this?
/kind bug

What this PR does / why we need it:
When you run kubectl config view, the user token is not redacted like client-certificate-data and client-key-data. This PR redacts the user token.

Which issue(s) this PR fixes:
xref kubernetes/kubectl#667

Special notes for your reviewer:
None

Does this PR introduce a user-facing change?:

`kubectl config view` now redacts bearer tokens by default, similar to client certificates. The `--raw` flag can still be used to output full content.

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

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Mar 9, 2020

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

@brianpursley

This comment has been minimized.

Copy link
Contributor Author

brianpursley commented Mar 9, 2020

/assign smarterclayton

@enj

This comment has been minimized.

Copy link
Member

enj commented Mar 9, 2020

IIRC the reason for redacting the cert data was that it made the output hard to read (since it is long). You can always cat the config file so not really sure it matters if we redact the token here.

// Flatten redacts raw data entries from the config object for a human-readable view.
func ShortenConfig(config *Config) {

The go doc and function name hint that this is not related to security.

@brianpursley

This comment has been minimized.

Copy link
Contributor Author

brianpursley commented Mar 10, 2020

IIRC the reason for redacting the cert data was that it made the output hard to read (since it is long). You can always cat the config file so not really sure it matters if we redact the token here.

// Flatten redacts raw data entries from the config object for a human-readable view.
func ShortenConfig(config *Config) {

The go doc and function name hint that this is not related to security.

@enj you are correct, it is not related to security.

I found some related discussion that its purpose is to shorten long data, like you said.

For reference, on my machine, the pertinent output without redaction looks like this for an AKS cluster:

  user:
    client-certificate-data: REDACTED
    client-key-data: REDACTED
    token: 35ae444a781d891fa006b509500f5c4f7fd30e79aa092fef98806e0996d6adcc7b3929a1943c2ed1439d501a9c2e888ffc1be1955dd804474e050eff84199e08

I guess it is debatable whether this is long enough for "redaction".

I personally don't have a preference, just trying to close out this issue. So if we want to leave it as-is and close the issue, I'm fine with that too.

@@ -98,6 +98,9 @@ func ShortenConfig(config *Config) {
if len(authInfo.ClientCertificateData) > 0 {
authInfo.ClientCertificateData = redactedBytes
}
if len(authInfo.Token) > 0 {
authInfo.Token = string(redactedBytes)

This comment has been minimized.

Copy link
@lavalamp

lavalamp Mar 10, 2020

Member

I'm surprised this works after being cast to a string.

This comment has been minimized.

Copy link
@brianpursley

brianpursley Mar 10, 2020

Author Contributor

Yeah, It is different than the others which are byte arrays. Token is a string.

This comment has been minimized.

Copy link
@liggitt

liggitt Mar 11, 2020

Member

what does the output of this look like? tokens are strings, and the bytes you're assigning here are base-64 decoded versions of "REDACTED+"... I don't expect that would display well

we also have other potentially sensitive fields in the config (password, auth config); see corresponding sanitization of rest.Config printing in https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/client-go/rest/config.go#L158-L175

This comment has been minimized.

Copy link
@liggitt

liggitt Mar 11, 2020

Member

The goal of ShortenConfig was not to keep confidential data from the output (if you're running this command, you already have read access to the data), but to prevent massively long output from byte-serialized certificates.

This comment has been minimized.

Copy link
@brianpursley

brianpursley Mar 11, 2020

Author Contributor

Agreed that it doesn't provide security.

I think you questioning is right though. There is a problem. The unit test passes, but the output is not "REDACTED" it is garbage. Good catch. I will re-visit this.

This comment has been minimized.

Copy link
@brianpursley

brianpursley Mar 11, 2020

Author Contributor

Sorry @lavalamp, I think your intuition was right, it doesn't work. Not sure what I was thinking... 😕

@lavalamp, @liggitt I pushed a new commit and the output looks like this:

- name: clusterUser_k8s_akstest
  user:
    client-certificate-data: REDACTED
    client-key-data: REDACTED
    token: REDACTED

This comment has been minimized.

Copy link
@lavalamp

lavalamp Mar 11, 2020

Member

Why was the test wrong?

This comment has been minimized.

Copy link
@brianpursley

brianpursley Mar 11, 2020

Author Contributor

Why was the test wrong?

It was a new test case I added that was wrong, so it wasn't a problem in an existing test. I made an assumption that string(redactedBytes) would result in "REDACTED" but that was wrong.

redactedBytes is defined as:

sDec, _ := base64.StdEncoding.DecodeString("REDACTED+")
redactedBytes = []byte(string(sDec))

I didn't look closely enough at how that actually worked.

It is sort of tricky in that it is starting with a "base-64 encoded" value of "REDACTED+" and then decoding it, storing it in bytes, so that later when it is re-encoded, it displays as a human-readable "REDACTED".

When I did string(redactedBytes) I got the base64 decode of "REDACTED+" which is actually "D@�\t1\x03".

My test was wrong in the same way, I was inadvertently checking for "D@�\t1\x03" instead of "REDACTED".

This comment has been minimized.

Copy link
@brianpursley

brianpursley Mar 11, 2020

Author Contributor

There is a comment basically saying how the"trick" works, somehow I glossed right over it.

// trick json encoder into printing a human readable string in the raw data

@lavalamp

This comment has been minimized.

Copy link
Member

lavalamp commented Mar 10, 2020

I think it is fine to redact this for readability. It's not a security thing, though.

/approve
/lgtm

@brianpursley

This comment has been minimized.

Copy link
Contributor Author

brianpursley commented Mar 10, 2020

Looks like I need to update some bazel tests...

/hold

@brianpursley brianpursley force-pushed the brianpursley:kubectl-667 branch from 7c1ad2d to 75e7e78 Mar 11, 2020
@k8s-ci-robot k8s-ci-robot removed the lgtm label Mar 11, 2020
@brianpursley brianpursley force-pushed the brianpursley:kubectl-667 branch from 75e7e78 to fd0eb5d Mar 11, 2020
@brianpursley

This comment has been minimized.

Copy link
Contributor Author

brianpursley commented Mar 11, 2020

/unhold

@lavalamp

This comment has been minimized.

Copy link
Member

lavalamp commented Mar 11, 2020

I was 100% fooled by the test. Why did the test output not match reality? Please fix that?

@brianpursley

This comment has been minimized.

Copy link
Contributor Author

brianpursley commented Mar 11, 2020

@lavalamp as mentioned in my comment above, the wrong test was added by me in this commit, so it wasn't a problem with an existing test. I have updated the test and checked the output.

@lavalamp

This comment has been minimized.

Copy link
Member

lavalamp commented Mar 11, 2020

@k8s-ci-robot k8s-ci-robot added size/M and removed size/XS labels Mar 11, 2020
@brianpursley brianpursley force-pushed the brianpursley:kubectl-667 branch from 6945854 to 2d03d44 Mar 11, 2020
@brianpursley

This comment has been minimized.

Copy link
Contributor Author

brianpursley commented Mar 11, 2020

Thanks for the explanation! Are we not running the "Example" function as a test, then? Is it configured wrong? It sounds like that really should have failed.

to be honest, I don't know what that Example function was doing in the unit test. I just updated the commented out yaml so that it would be consistent with the change I made.

It appears not to be an actual test.

I just committed an update that turned it into a real unit test and checks the output against what was previously in the comment, so hopefully it will serve some purpose in the future.

@lavalamp

This comment has been minimized.

Copy link
Member

lavalamp commented Mar 11, 2020

It's supposed to actually test stuff: https://blog.golang.org/examples

@brianpursley brianpursley force-pushed the brianpursley:kubectl-667 branch from 2d03d44 to fd0eb5d Mar 17, 2020
@k8s-ci-robot k8s-ci-robot added size/XS and removed size/M labels Mar 17, 2020
@brianpursley

This comment has been minimized.

Copy link
Contributor Author

brianpursley commented Mar 17, 2020

It's supposed to actually test stuff: https://blog.golang.org/examples

Thanks, I'm still sort of new to Go, so wasn't familiar with that convention. TIL. 👨‍🎓

I reverted it back to the original "Example" test and confirmed the test DOES fail when running it locally.

--- FAIL: Example_minifyAndShorten (0.00s)
got:
clusters:
  cow-cluster:
    LocationOfOrigin: ""
    certificate-authority-data: DATA+OMITTED
    server: http://cow.org:8080
contexts:
  federal-context:
    LocationOfOrigin: ""
    cluster: cow-cluster
    user: red-user
current-context: federal-context
preferences: {}
users:
  red-user:
    LocationOfOrigin: ""
    client-certificate-data: REDACTED
    client-key-data: REDACTED
    token: "D@�\t1\x03"
want:
clusters:
  cow-cluster:
    LocationOfOrigin: ""
    certificate-authority-data: DATA+OMITTED
    server: http://cow.org:8080
contexts:
  federal-context:
    LocationOfOrigin: ""
    cluster: cow-cluster
    user: red-user
current-context: federal-context
preferences: {}
users:
  red-user:
    LocationOfOrigin: ""
    client-certificate-data: REDACTED
    client-key-data: REDACTED
    token: REDACTED
FAIL

This PR never actually got the needs-ok-to-test label removed, and I wasn't a Kubernetes member yet, so it never ran the tests in the CI or else I suspect it would have failed.

I am a member now, so let me try it...
/ok-to-test

Once the tests pass, I think this PR should be ready for re-review.

@brianpursley brianpursley force-pushed the brianpursley:kubectl-667 branch from fd0eb5d to 6fad4ee Mar 17, 2020
@brianpursley

This comment has been minimized.

Copy link
Contributor Author

brianpursley commented Mar 17, 2020

/retest

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Mar 18, 2020

/priority backlog
/lgtm
/approve
/remove-kind bug
/kind cleanup

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Mar 18, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brianpursley, lavalamp, 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:

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

@k8s-ci-robot k8s-ci-robot merged commit 4bc907f into kubernetes:master Mar 18, 2020
17 checks passed
17 checks passed
cla/linuxfoundation brianpursley authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-kind-ga-only-parallel Job succeeded.
Details
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-kind Job succeeded.
Details
pull-kubernetes-e2e-kind-ipv6 Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
@k8s-ci-robot k8s-ci-robot added this to the v1.19 milestone Mar 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.