Limit kubelets from updating their own labels when NodeRestriction is enabled #68267

liggitt · 2018-09-05T04:13:43Z

Implements phase 1 of https://github.com/kubernetes/community/blob/master/keps/sig-auth/0000-20170814-bounding-self-labeling-kubelets.md#implementation-timeline

Docs PR in kubernetes/website#10944

This PR:

Prevents kubelets from adding/modifying labels prefixed with node-restriction.kubernetes.io on their Node objects
Restricts which kubernetes.io labels kubelets can set when updating their Node objects to an allowed set of labels and label prefixes
Warns on kubelet startup if kubernetes.io labels outside that set are passed via --node-labels (will escalate to an error in v1.15)
Warns in admission if kubernetes.io labels outside that set are passed on Node creation (will escalate to a forbidden error in v1.17)

/sig auth
/sig node
/sig storage

/cc @mikedanese @verult @saad-ali @vishh

kube-apiserver: the `NodeRestriction` admission plugin now prevents kubelets from modifying `Node` labels prefixed with `node-restriction.kubernetes.io/`. The `node-restriction.kubernetes.io/` label prefix is reserved for cluster administrators to use for labeling `Node` objects to target workloads to nodes in a way that kubelets cannot modify or spoof.

kubelet: it is now deprecated to use the `--node-labels` flag to set `kubernetes.io/` and `k8s.io/`-prefixed labels other than the following labels:
* `kubernetes.io/hostname`
* `kubernetes.io/instance-type`
* `kubernetes.io/os`
* `kubernetes.io/arch`
* `beta.kubernetes.io/instance-type`
* `beta.kubernetes.io/os`
* `beta.kubernetes.io/arch`
* `failure-domain.kubernetes.io/zone`
* `failure-domain.kubernetes.io/region`
* `failure-domain.beta.kubernetes.io/zone`
* `failure-domain.beta.kubernetes.io/region`
* `[*.]kubelet.kubernetes.io/*`
* `[*.]node.kubernetes.io/*`
Setting other `kubernetes.io/`- and `k8s.io/`-prefixed labels using the `--node-labels` flag will produce a warning in v1.13, and be disallowed in v1.15. Setting labels that are not prefixed with `kubernetes.io/` or `k8s.io/` is still permitted.

liggitt · 2018-09-05T04:42:16Z

cc @vikaschoudhary16 @derekwaynecarr since the plugin watcher looks to be the most immediate source of expanding kubelet self-labeling.

~~is that attempting to go to beta in 1.12?~~
yes, in #68200

edit: talked with @verult, #67684 is the PR that adds self-labeling to the kubelet for CSI drivers. it is currently behind the PluginWatcher gate, but if that goes to beta, we risk shipping 1.12 with uncontrolled self-updates by kubelets.

can we fence that capability of that PR until we can coordinate label controls for kubelets?

dims · 2018-09-24T23:32:12Z

/assign @yujuhong

vishh · 2018-09-28T19:19:04Z

@liggitt

Use cases like device plugins and CSI drivers intend to use self-added labels to express topology, which is fine, but those use cases need to be coordinated with controls the cluster-admin can use to ensure labels remain useful for partitioning a cluster along security-related boundaries

Can you explain this more? I want to make sure usability isn't compromised a lot. Poking a hole in RBAC isn't safe today for node level daemons.

liggitt · 2018-10-05T15:12:00Z

Can you explain this more? I want to make sure usability isn't compromised a lot. Poking a hole in RBAC isn't safe today for node level daemons.

For things the kubelet is expected to self-report topology for (device drivers and CSI come to mind), one possibility would be to have a registration object for the device class or CSI driver that includes the list of label keys that will be used for topology. For example, CSI has the CSIDriver object which would be a natural place to put that info (and has benefits beyond authorization... today the CSI provisioner has to pick a node at random to determine what the topology keys are).

That registration object would not be kubelet-writeable. The node admission plugin could then allow kubelets to self-set labels that were designated as topology labels by one of those registration objects.

vishh · 2018-10-08T18:36:07Z

On Fri, Oct 5, 2018 at 8:14 AM Jordan Liggitt ***@***.***> wrote: Can you explain this more? I want to make sure usability isn't compromised a lot. Poking a hole in RBAC isn't safe today for node level daemons. For things the kubelet is expected to self-report topology for (device drivers and CSI come to mind), one possibility would be to have a registration object for the device class or CSI driver that includes the list of label keys that will be used for topology. For example, CSI has the CSIDriver object which would be a natural place to put that info (and has benefits beyond authorization... today the CSI provisioner has to pick a node at random to determine what the topology keys are). That registration object would not be kubelet-writeable. The node admission plugin could then allow kubelets to self-set labels that were designated as topology labels by one of those registration objects.

How is auth managed for the plugins that write this secondary object which will then get copied over to the Node Object?

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#68267 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGvIKL0sw3N8iZY5W73XIQLsj41f-3p-ks5uh3dlgaJpZM4WaJ0k> .

liggitt · 2018-10-08T18:42:16Z

How is auth managed for the plugins that write this secondary object which will then get copied over to the Node Object?

Either the cluster admin or something at the control plane level would be expected to set those objects up (it could be part of the manifest or add-on that set up the device driver or CSI plugin)

liggitt · 2018-10-23T14:34:36Z

cc @kubernetes/sig-auth-pr-reviews @kubernetes/sig-node-pr-reviews @kubernetes/sig-storage-pr-reviews

mikedanese

Looks reasonable. Who needs to sign off on this?

plugin/pkg/admission/noderestriction/admission_test.go

plugin/pkg/admission/noderestriction/admission.go

mikedanese · 2018-10-23T21:08:46Z

plugin/pkg/admission/noderestriction/admission_test.go

+		{
+			name:       "allow create of my node with labels",
+			podsGetter: noExistingPods,
+			attributes: admission.NewAttributesRecord(mynodeObjLabelA, nil, nodeKind, mynodeObj.Namespace, "", nodeResource, "", admission.Create, false, mynode),


s/mynodeObj.Namespace/mynodeObjLabelA.Namespace/ here and below? Or just omit it. We are already omitting the name and nodes aren't namespaced. It looks like a number of other test cases are sloppy as well.

it doesn't actually bother me to pass the empty object namespace in, and the name of all the mynode*test fixtures is identical, the point of the different ones is to change other fields in the object. will address separately if desired, trying to keep the test diff readable.

mikedanese · 2018-11-14T04:57:14Z

/lgtm
/approve

liggitt · 2018-11-14T05:28:23Z

/assign derekwaynecarr tallclair
for pkg/kubelet approval

derekwaynecarr · 2018-11-15T15:16:20Z

kubelet changes look good to me.

/lgtm
/approve

k8s-ci-robot · 2018-11-15T15:16:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr, liggitt, mikedanese

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:

~~cmd/kubelet/OWNERS~~ [derekwaynecarr,mikedanese]
~~pkg/kubelet/OWNERS~~ [derekwaynecarr]
~~plugin/pkg/admission/noderestriction/OWNERS~~ [derekwaynecarr,liggitt,mikedanese]

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

liggitt · 2018-11-15T17:41:26Z

/retest

kfox1111 · 2018-12-04T17:15:35Z

Saw this in the release notes. This is very good to see.

One thing I don't understand though. To really solve the issue, all usage of labels that could ever have security ramifications now would be required to be prefixed with kubernetes.io/ or k8s.io/
But most of the time its user workload. So we are encouraging users to come up with their own labels under the kubernetes.io/ or k8s.io/ spaces?

Thanks,
Kevin

liggitt · 2018-12-04T17:18:29Z

all usage of labels that could ever have security ramifications now would be required to be prefixed with kubernetes.io/ or k8s.io/
But most of the time its user workload. So we are encouraging users to come up with their own labels under the kubernetes.io/ or k8s.io/ spaces?

See https://kubernetes.io/docs/reference/access-authn-authz/admission-controllers/#noderestriction for a label prefix specifically reserved for this use:

Prevents kubelets from adding/removing/updating labels with a node-restriction.kubernetes.io/ prefix. This label prefix is reserved for administrators to label their Node objects for workload isolation purposes, and kubelets will not be allowed to modify labels with that prefix.

kfox1111 · 2018-12-04T17:21:11Z

Ah. ok. So, the recommendation is in general, "always prefix your node labels with node-restriction.kubernetes.io/"?

liggitt · 2018-12-04T17:26:49Z

So, the recommendation is in general, "always prefix your node labels with node-restriction.kubernetes.io/"?

For the labels you don't want kubelets setting, if you want to use the built-in protection. You can also add your own admission checks to protect non-kubernetes labels.

kfox1111 · 2018-12-04T17:30:23Z

Ok. Yeah, was just looking for general advice to give to new users to avoid issues. Since its always safe, and doesn't have an extra cost, its "always prefix your node labels with node-restriction.kubernetes.io/" (unless you know what you are doing). Thanks. :)

ravilr · 2019-03-01T01:12:35Z

@liggitt 'node-role.kubernetes.io/master' and 'node-role.kubernetes.io/node' seems to be used across projects. Can you please give some direction on what do you think about whitelisting this as allowed set of labels ?

kubernetes/pkg/controller/service/service_controller.go

Line 66 in 954996e

LabelNodeRoleMaster = "node-role.kubernetes.io/master"

Also across kubeadm, kubespray

pires · 2019-04-16T14:46:32Z

Conformance tests will also be broken if this annotation is not used for control-plane nodes and they're unschedulable, eg with taints. See https://github.com/kubernetes/kubernetes/blob/v1.14.1/test/e2e/framework/util.go#L2755-L2760

liggitt · 2019-04-16T14:52:54Z

that label is not appropriate for use in a conformance test, as it is not official API and is not maintained by kubernetes components

liggitt · 2019-04-16T15:12:57Z

opened #76654 to let the e2e/conformance invoker select which nodes they think should be scheduleable, rather than hard-code handling of a label

liggitt · 2019-04-16T15:24:15Z

Can you please give some direction on what do you think about whitelisting this as allowed set of labels ?

Letting arbitrary nodes self-label as a master node is not reasonable. That label is likely to be used in daemonset nodeSelectors running highly-privileged components.

liggitt · 2019-04-16T15:27:27Z

kubeadm does not set that label using self-labeling, but applies it using the superuser client at cluster startup:

https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/phases/markcontrolplane/markcontrolplane.go#L28-L44

k8s-ci-robot requested review from mikedanese, saad-ali, verult and vishh September 5, 2018 04:13

liggitt mentioned this pull request Sep 5, 2018

CSI Node info registration in kubelet #67684

Merged

liggitt assigned liggitt and unassigned liggitt Sep 22, 2018

k8s-ci-robot assigned yujuhong Sep 24, 2018

liggitt force-pushed the node-label-update branch from e8f478d to ac091f7 Compare October 17, 2018 16:43

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 17, 2018

liggitt force-pushed the node-label-update branch from ac091f7 to b56a53b Compare October 17, 2018 16:59

liggitt added this to the v1.13 milestone Oct 23, 2018

mikedanese reviewed Oct 23, 2018

View reviewed changes

k8s-ci-robot assigned mikedanese Nov 14, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 14, 2018

k8s-ci-robot assigned derekwaynecarr and tallclair Nov 14, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 15, 2018

k8s-ci-robot merged commit 504466c into kubernetes:master Nov 15, 2018

liggitt deleted the node-label-update branch November 16, 2018 21:23

corest mentioned this pull request Dec 8, 2018

Kubernetes 1.13 giantswarm/k8scloudconfig#477

Closed

andrewsykim mentioned this pull request Feb 1, 2019

move pkg/kubelet/apis/well_known_labels.go to staging/src/k8s.io/api/core/v1/ #73616

Merged

danielqsj mentioned this pull request Mar 19, 2019

Incompatibility between kubelet and kubeadm and kubectl on which label prefix to use #75457

Closed

woopstar mentioned this pull request Apr 17, 2019

Revert "Fix for unknown 'kubernetes.io' or 'k8s.io' labels specified with --node-labels" kubernetes-sigs/kubespray#4551

Closed

Miouge1 mentioned this pull request Apr 17, 2019

Revert "Fix for unknown 'kubernetes.io' or 'k8s.io' labels specified … kubernetes-sigs/kubespray#4553

Merged

aermakov-zalando mentioned this pull request Jun 3, 2019

Label nodes with their linux distro zalando-incubator/kubernetes-on-aws#2188

Merged

rphillips mentioned this pull request Nov 15, 2019

add new node role labels openshift/machine-config-operator#1269

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit kubelets from updating their own labels when NodeRestriction is enabled #68267

Limit kubelets from updating their own labels when NodeRestriction is enabled #68267

liggitt commented Sep 5, 2018 •

edited

liggitt commented Sep 5, 2018 •

edited

dims commented Sep 24, 2018

vishh commented Sep 28, 2018

liggitt commented Oct 5, 2018

vishh commented Oct 8, 2018 via email

liggitt commented Oct 8, 2018

liggitt commented Oct 23, 2018

mikedanese left a comment

mikedanese Oct 23, 2018

liggitt Oct 25, 2018

mikedanese commented Nov 14, 2018

liggitt commented Nov 14, 2018

derekwaynecarr commented Nov 15, 2018

k8s-ci-robot commented Nov 15, 2018

liggitt commented Nov 15, 2018

kfox1111 commented Dec 4, 2018

liggitt commented Dec 4, 2018

kfox1111 commented Dec 4, 2018

liggitt commented Dec 4, 2018

kfox1111 commented Dec 4, 2018

ravilr commented Mar 1, 2019

pires commented Apr 16, 2019 •

edited

liggitt commented Apr 16, 2019

liggitt commented Apr 16, 2019

liggitt commented Apr 16, 2019 •

edited

liggitt commented Apr 16, 2019

Limit kubelets from updating their own labels when NodeRestriction is enabled #68267

Limit kubelets from updating their own labels when NodeRestriction is enabled #68267

Conversation

liggitt commented Sep 5, 2018 • edited

liggitt commented Sep 5, 2018 • edited

dims commented Sep 24, 2018

vishh commented Sep 28, 2018

liggitt commented Oct 5, 2018

vishh commented Oct 8, 2018 via email

liggitt commented Oct 8, 2018

liggitt commented Oct 23, 2018

mikedanese left a comment

Choose a reason for hiding this comment

mikedanese Oct 23, 2018

Choose a reason for hiding this comment

liggitt Oct 25, 2018

Choose a reason for hiding this comment

mikedanese commented Nov 14, 2018

liggitt commented Nov 14, 2018

derekwaynecarr commented Nov 15, 2018

k8s-ci-robot commented Nov 15, 2018

liggitt commented Nov 15, 2018

kfox1111 commented Dec 4, 2018

liggitt commented Dec 4, 2018

kfox1111 commented Dec 4, 2018

liggitt commented Dec 4, 2018

kfox1111 commented Dec 4, 2018

ravilr commented Mar 1, 2019

pires commented Apr 16, 2019 • edited

liggitt commented Apr 16, 2019

liggitt commented Apr 16, 2019

liggitt commented Apr 16, 2019 • edited

liggitt commented Apr 16, 2019

liggitt commented Sep 5, 2018 •

edited

liggitt commented Sep 5, 2018 •

edited

pires commented Apr 16, 2019 •

edited

liggitt commented Apr 16, 2019 •

edited