NodeRestriction admission plugin #45929

liggitt · 2017-05-17T02:43:23Z

Adds an optional NodeRestriction admission plugin that limits identifiable kubelets to mutating their own Node object, and Pod objects bound to their node.

This is the admission portion of https://github.com/kubernetes/community/blob/master/contributors/design-proposals/kubelet-authorizer.md and kubernetes/enhancements#279

The `NodeRestriction` admission plugin limits the `Node` and `Pod` objects a kubelet can modify. In order to be limited by this admission plugin, kubelets must use credentials in the `system:nodes` group, with a username in the form `system:node:<nodeName>`. Such kubelets will only be allowed to modify their own `Node` API object, and only modify `Pod` API objects that are bound to their node.

liggitt · 2017-05-17T03:15:34Z

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

liggitt · 2017-05-17T04:06:37Z

@k8s-bot verify test this

liggitt · 2017-05-17T17:49:17Z

@ericchiang @timstclair @deads2k PTAL

ericchiang

I don't think this was covered in the proposal, but what's the general strategy for testing this feature? e.g. how do we know this doesn't prevent some edge-case kubelet feature?

ericchiang · 2017-05-17T17:52:48Z

plugin/pkg/admission/noderestriction/admission.go

+		case "status":
+			return c.admitPodStatus(nodeName, a)
+		default:
+			return admission.NewForbidden(a, fmt.Errorf("unexpected pod subresource"))


nit: include the subresource in the error message?

ericchiang · 2017-05-17T17:54:46Z

plugin/pkg/admission/noderestriction/admission.go

+		// get the existing pod
+		existingPod, err := c.podsGetter.Pods(a.GetNamespace()).Get(a.GetName(), v1.GetOptions{ResourceVersion: "0"})
+		if err != nil {
+			return admission.NewForbidden(a, err)


nit: This could be an internal error like "doesn't exist". By including the error in the response, couldn't we be leaking information about what other nodes exist?

this is actually an error about looking up the pod... for now, I'd rather surface the error (remember nodes can still list/watch all pods, and even if they couldn't, they could see if a specific pod already existed by trying to create a mirror pod with the same name and seeing if they get an AlreadyExists error)

timstclair

Just a couple small comments.

timstclair · 2017-05-17T18:43:06Z

pkg/api/pod/util.go

@@ -25,7 +25,7 @@ import (
 // referenced by the pod spec. If visitor returns false, visiting is short-circuited.
 // Transitive references (e.g. pod -> pvc -> pv -> secret) are not visited.
 // Returns true if visiting completed, false if visiting was short-circuited.
-func VisitPodSecretNames(pod *api.Pod, visitor func(string) bool) bool {
+func VisitPodSecretNames(pod *api.Pod, visitor func(string) (shouldContinue bool)) bool {


nit: declare a type for the visitor?

type Visitor func(name string) (shouldContinue bool)

timstclair · 2017-05-17T19:30:39Z

staging/src/k8s.io/apiserver/pkg/authentication/node/default.go

+
+	userName := u.GetName()
+	nodeName := ""
+	if trimmed := strings.TrimPrefix(userName, "system:node:"); len(trimmed) != len(userName) {


&& len(trimmed) > 0

&& len(trimmed) > 0

so we don't overwrite "" with ""? :)

deads2k · 2017-05-18T18:13:42Z

staging/src/k8s.io/apiserver/pkg/authentication/node/default.go

+// NewDefaultNodeIdentifier returns a default NodeIdentifier implementation,
+// which returns isNode=true if the user groups contain the system:nodes group,
+// and populates nodeName if isNode is true, and the user name is in the format system:node:<nodeName>
+func NewDefaultNodeIdentifier() NodeIdentifier {


Why is this going into the generic API server? Realistically, this is specific to kube-apiserver since the kubelet can only be coded against core resources, right?

I don't feel strongly... NodesGroup = "system:nodes" is in a peer package under k8s.io/apiserver but I can move this if you have an alternate preferred location

moved out of k8s.io/apiserver back under the main package

deads2k · 2017-05-18T18:16:08Z

question about just how generic this the node identification is. lgtm otherwise.

timstclair · 2017-05-18T18:33:04Z

LGTM from me

liggitt · 2017-05-18T19:54:14Z

what's the general strategy for testing this feature? e.g. how do we know this doesn't prevent some edge-case kubelet feature?

This is starting from the pod, pod/status, node, and node/status permissions granted in the system:node RBAC role, which is tested in CI. There are no known cases of nodes updating pods for other nodes (or unbound pods), or updating other node objects.

I'm willing to say that if we find a case like that, we would consider it a kubelet action we would want to prevent, and anyone blocked until the usage is fixed can stop using this admission plugin in the meantime (since the whole point of the plugin is to prevent cross-node/pod mutations)

ericchiang · 2017-05-18T20:08:03Z

/approve

timstclair · 2017-05-18T20:14:37Z

/lgtm

liggitt · 2017-05-18T20:45:18Z

fixed lint error, retagging

smarterclayton · 2017-05-18T21:07:48Z

/approve

based on approval of owners of the relevant areas

k8s-github-robot · 2017-05-18T21:07:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ericchiang, liggitt, smarterclayton, timstclair

Needs approval from an approver in each of these OWNERS Files:

~~cmd/kube-apiserver/OWNERS~~ [liggitt,smarterclayton]
~~hack/OWNERS~~ [smarterclayton]
~~pkg/OWNERS~~ [smarterclayton]
~~plugin/OWNERS~~ [smarterclayton]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-05-19T02:58:11Z

Automatic merge from submit-queue (batch tested with PRs 41535, 45985, 45929, 45948, 46056)

Add doc for this: kubernetes/kubernetes#45929

k8s-github-robot assigned smarterclayton and davidopp May 17, 2017

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 17, 2017

k8s-github-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label May 17, 2017

liggitt assigned ericchiang, timstclair and deads2k and unassigned smarterclayton and davidopp May 17, 2017

k8s-github-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label May 17, 2017

liggitt force-pushed the node-admission branch 2 times, most recently from 9b95102 to d38c74b Compare May 17, 2017 03:00

k8s-ci-robot added sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/node Categorizes an issue or PR as relevant to SIG Node. labels May 17, 2017

liggitt added the area/security label May 17, 2017

liggitt added this to the v1.7 milestone May 17, 2017

liggitt force-pushed the node-admission branch from d38c74b to d109bb3 Compare May 17, 2017 03:26

liggitt mentioned this pull request May 17, 2017

Node admission/authorization openshift/origin#14227

Merged

ericchiang reviewed May 17, 2017

View reviewed changes

timstclair reviewed May 17, 2017

View reviewed changes

liggitt force-pushed the node-admission branch from d109bb3 to 5249d99 Compare May 18, 2017 05:57

deads2k reviewed May 18, 2017

View reviewed changes

Add pod util for extracting referenced configmaps

d609f4e

liggitt force-pushed the node-admission branch from 5249d99 to 66b9c51 Compare May 18, 2017 19:39

k8s-github-robot removed the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label May 18, 2017

k8s-github-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 18, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 18, 2017

liggitt added 2 commits May 18, 2017 16:43

Add NodeIdentifier interface and default implementation

0c516c3

Add NodeRestriction admission plugin

6fd3679

liggitt force-pushed the node-admission branch from 66b9c51 to 6fd3679 Compare May 18, 2017 20:43

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 18, 2017

liggitt added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 18, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 18, 2017

k8s-github-robot merged commit a9fbeef into kubernetes:master May 19, 2017

liggitt deleted the node-admission branch May 19, 2017 04:09

xilabao mentioned this pull request May 19, 2017

add NodeRestriction to admission-controllers kubernetes/website#3842

Merged

CaoShuFeng added a commit to CaoShuFeng/kubernetes.github.io that referenced this pull request Jun 19, 2017

add doc for NodeRestriction admission controller

a6275ad

Add doc for this: kubernetes/kubernetes#45929

CaoShuFeng mentioned this pull request Jun 19, 2017

add doc for NodeRestriction admission controller kubernetes/website#4131

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NodeRestriction admission plugin #45929

NodeRestriction admission plugin #45929

liggitt commented May 17, 2017 •

edited

Loading

liggitt commented May 17, 2017

liggitt commented May 17, 2017

liggitt commented May 17, 2017 •

edited

Loading

ericchiang left a comment

ericchiang May 17, 2017

ericchiang May 17, 2017

liggitt May 18, 2017 •

edited

Loading

timstclair left a comment

timstclair May 17, 2017

liggitt May 18, 2017

timstclair May 17, 2017

liggitt May 18, 2017

deads2k May 18, 2017

liggitt May 18, 2017

liggitt May 18, 2017

deads2k commented May 18, 2017

timstclair commented May 18, 2017

liggitt commented May 18, 2017 •

edited

Loading

ericchiang commented May 18, 2017

timstclair commented May 18, 2017

liggitt commented May 18, 2017

smarterclayton commented May 18, 2017

k8s-github-robot commented May 18, 2017

k8s-github-robot commented May 19, 2017

NodeRestriction admission plugin #45929

NodeRestriction admission plugin #45929

Conversation

liggitt commented May 17, 2017 • edited Loading

liggitt commented May 17, 2017

liggitt commented May 17, 2017

liggitt commented May 17, 2017 • edited Loading

ericchiang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt May 18, 2017 • edited Loading

Choose a reason for hiding this comment

timstclair left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented May 18, 2017

timstclair commented May 18, 2017

liggitt commented May 18, 2017 • edited Loading

ericchiang commented May 18, 2017

timstclair commented May 18, 2017

liggitt commented May 18, 2017

smarterclayton commented May 18, 2017

k8s-github-robot commented May 18, 2017

k8s-github-robot commented May 19, 2017

liggitt commented May 17, 2017 •

edited

Loading

liggitt commented May 17, 2017 •

edited

Loading

liggitt May 18, 2017 •

edited

Loading

liggitt commented May 18, 2017 •

edited

Loading