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

fix unsafe JSON construction #81158

Open
wants to merge 1 commit into
base: master
from

Conversation

@zouyee
Copy link
Member

commented Aug 8, 2019

What type of PR is this?

/kind flake

What this PR does / why we need it:

fix unsafe JSON construction
Which issue(s) this PR fixes:

Fixes #81134

Special notes for your reviewer:
Kubernetes uses JavaScript Object Notation (JSON) and similarly structured data sources throughout the codebase. This supports inter-component communications, both internally and externally to the cluster. However, a number of locations within the codebase use unsafe methods of constructing JSON.

Exploit Scenario
Alice runs a Kubernetes cluster in her organization. Bob, a user in Alice’s organization, attempts to add an RBAC permission that he is not entitled to, which causes his entire RBAC construction to be written to logs, and potentially improperly consumed elsewhere.

Does this PR introduce a user-facing change?:

Fix unsafe JSON construction in a number of locations in the codebase

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


@zouyee

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2019

/kind bug
/area kubelet

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: zouyee
To complete the pull request process, please assign thockin
You can assign the PR to them by writing /assign @thockin in a comment when ready.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@zouyee

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2019

/test pull-kubernetes-bazel-test
/test pull-kubernetes-e2e-gce

@neolit123

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

/remove-kind flake
/kind bug
/priority important-longterm
/sig node

@neolit123

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

@neolit123

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

Fixes #81134

as the linked issue outlines there are more instances of this problem (see the comments on the ticket).
could you try solving all of them in this PR?

instead of NONE under Does this PR introduce a user-facing change?: please add:

Fix unsafe JSON construction in a number of locations in the codebase

@zouyee

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2019

Because different components are involved, should they be split into multiple PR?@neolit123

@zouyee

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2019

/test pull-kubernetes-bazel-test

@neolit123

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

please send them all as part of this PR and if they are way to many we can decide to split.
thanks.

@zouyee zouyee force-pushed the zouyee:node-fix branch 2 times, most recently from 15dfb0c to 4a61683 Aug 10, 2019

@k8s-ci-robot k8s-ci-robot removed the size/S label Aug 10, 2019

@zouyee

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2019

/test pull-kubernetes-verify

@neolit123

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

@zouyee
please have a look at the govet failures:

# k8s.io/kubernetes/pkg/controller
pkg/controller/controller_ref_manager.go:219:45: call of ownerRefControllerPatch copies lock value: k8s.io/kubernetes/pkg/controller.BaseControllerRefManager contains sync.Once contains sync.Mutex
pkg/controller/controller_ref_manager.go:344:45: call of ownerRefControllerPatch copies lock value: k8s.io/kubernetes/pkg/controller.BaseControllerRefManager contains sync.Once contains sync.Mutex
pkg/controller/controller_ref_manager.go:482:45: call of ownerRefControllerPatch copies lock value: k8s.io/kubernetes/pkg/controller.BaseControllerRefManager contains sync.Once contains sync.Mutex
pkg/controller/controller_ref_manager.go:534:34: ownerRefControllerPatch passes lock by value: k8s.io/kubernetes/pkg/controller.BaseControllerRefManager contains sync.Once contains sync.Mutex
make[1]: *** [vet] Error 1

https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/81158/pull-kubernetes-verify/1161197154933411840

@zouyee zouyee force-pushed the zouyee:node-fix branch from 34e7dec to 64d271d Aug 14, 2019

@zouyee zouyee force-pushed the zouyee:node-fix branch from 64d271d to 9fc113b Aug 14, 2019

@zouyee

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2019

ping @neolit123

@neolit123

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

/lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.