auth: allow nodes to create tokens for svcaccts of pods #55019

mikedanese · 2017-11-02T18:16:15Z

running on them. nodes essentially have the power to do this today
but not explicitly. this allows agents using the node identity to
take actions on behalf of local pods.

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

The node authorizer now allows nodes to request service account tokens for the service accounts of pods running on them.

liggitt · 2017-11-02T20:18:57Z

Outlining the use cases for this would be helpful.

liggitt · 2017-11-02T20:21:11Z

plugin/pkg/auth/authorizer/node/graph.go

@@ -211,6 +214,8 @@ func (g *Graph) AddPod(pod *api.Pod) {
 	nodeVertex := g.getOrCreateVertex_locked(nodeVertexType, "", pod.Spec.NodeName)
 	g.graph.SetEdge(newDestinationEdge(podVertex, nodeVertex, nodeVertex))

+	g.graph.SetEdge(newDestinationEdge(g.getOrCreateVertex_locked(serviceAccountVertexType, pod.Namespace, pod.Spec.ServiceAccountName), podVertex, nodeVertex))


if len(pod.Spec.ServiceAccountName) > 0

tallclair · 2017-11-07T00:03:33Z

plugin/pkg/auth/authorizer/node/node_authorizer.go

+func (r *NodeAuthorizer) authorizeImpersonate(nodeName string, startingType vertexType, attrs authorizer.Attributes) (bool, string, error) {
+	if attrs.GetVerb() != "impersonate" || len(attrs.GetName()) == 0 {
+		glog.V(2).Infof("NODE DENY: %s %#v", nodeName, attrs)
+		return false, "can only impersonate individual resources of this type", nil


nit: error message isn't accurate if attrs.GetVerb() != "impersonate"

looks like the other (preexisting) methods make the same mistake, if you feel like cleaning up.

The "individual" makes the error message accurate.

mikedanese · 2017-11-07T22:22:38Z

Use case: I have a node agent that runs and uses the kubelet credentials. I want it to be able to make requests on behalf of pods running on the node to e.g. ask for service account JWTs.

Allowing the node credentials to get serviceaccount tokens indirectly permits impersonation although we don't want to promote that behavior. This makes the permission explicit.

liggitt · 2017-11-10T18:53:11Z

Use case: I have a node agent that runs and uses the kubelet credentials. I want it to be able to make requests on behalf of pods running on the node to e.g. ask for service account JWTs.

Allowing the node credentials to get serviceaccount tokens indirectly permits impersonation although we don't want to promote that behavior. This makes the permission explicit.

I don't think we want impersonation for this... we'd only want a node to be able to request JWTs for a service account that included the node name (and maybe even the pod name/namespace/uid for which the token was requested)

mikedanese · 2018-01-11T03:03:24Z

updated this to work with the new design.

tallclair

lgtm, but I defer to @liggitt and @ericchiang

tallclair · 2018-01-24T23:43:03Z

plugin/pkg/auth/authorizer/node/node_authorizer.go

@@ -165,6 +168,31 @@ func (r *NodeAuthorizer) authorize(nodeName string, startingType vertexType, att
 	return authorizer.DecisionAllow, "", nil
 }

+// authorizeCreateToken authorizes "create" requests to serviceaccounts 'token'
+// subresource of pods running on a nade


s/nade/node

tallclair · 2018-01-24T23:45:52Z

plugin/pkg/auth/authorizer/node/node_authorizer_test.go

+			name:   "deny svcacct token create",
+			attrs:  authorizer.AttributesRecord{User: node0, ResourceRequest: true, Verb: "create", Resource: "serviceaccounts", Subresource: "token", Name: "svcacct0-node1", Namespace: "ns0"},
+			expect: authorizer.DecisionNoOpinion,
+		},


nit: add a case for non-create & non-token subresource

liggitt · 2018-01-26T21:17:22Z

this looks like what I'd expect for this piece. will leave it open until the token request stuff gets in.

related pieces needed as the tokenrequest bits are built out:

update rbac system:nodes role to grant create on serviceaccounts/token for people using just RBAC
the NodeRestriction admission plugin to require token requests from nodes to be bound to a pod (and ensure that pod's nodeName matches the pod and serviceAccountName matches the service account)
integration test exercising authorizer+admission

tallclair · 2018-02-23T23:17:33Z

plugin/pkg/auth/authorizer/node/graph.go

@@ -213,6 +216,10 @@ func (g *Graph) AddPod(pod *api.Pod) {
 	nodeVertex := g.getOrCreateVertex_locked(nodeVertexType, "", pod.Spec.NodeName)
 	g.graph.SetEdge(newDestinationEdge(podVertex, nodeVertex, nodeVertex))

+	if len(pod.Spec.ServiceAccountName) > 0 {


If the pod doesn't mount the service account secrets, should the node still get access to the service account? If it does, then this is actually opening up the node permissions a little bit.

To frame this differently, should the token request be authorized by access to the service account, or the service account secret?

It's a good point that I hadn't thought of. I'm inclined to say no since the motivation of this change was to decouple service account credentials from the secretvolumes. However this might be reasonable once we have a serviceaccountvolumesource but if this is actually desirable we might want to allow pods to run without serviceaccounts at all. Is there any point of a pod running as a service account if it can't get credentials for that service account? @liggitt what do you think.

Is there any point of a pod running as a service account if it can't get credentials for that service account?

For one thing, a pod must always run with a service account (except static pods). By not mounting the credentials, it just gives a bit more defense (e.g. against authz misconfig). Another reason to run as a SA w/o creds is to use a PodSecurityPolicy, which are authorized for the pod's SA.

I think I would still expect the node to have access... @smarterclayton and I have been stewing on how image pulling interacts with the pod identity... thinking through how you could have a node-level image pull credential plugin that made use of service account information.

I'd leave this as is for now, with a TODO to resolve before exiting alpha

tallclair · 2018-02-23T23:24:47Z

plugin/pkg/auth/authorizer/node/node_authorizer.go

+	ok, err := r.hasPathFrom(nodeName, startingType, attrs.GetNamespace(), attrs.GetName())
+	if err != nil {
+		glog.V(2).Infof("NODE DENY: %v", err)
+		return authorizer.DecisionNoOpinion, "no path found to object", nil


nit: shouldn't this be an error return?

No, I don't think this is an error. This gets hit if a node requests a token for a service account when no pods running that service account are scheduled on that node. This should only return an error on server errors, not client errors.

tallclair · 2018-02-23T23:30:27Z

plugin/pkg/admission/noderestriction/admission.go

+		return admission.NewForbidden(a, fmt.Errorf("unexpected type %T", a.GetObject()))
+	}
+	// TokenRequests from a node must have a pod binding. That pod must be
+	// running on the node.


nit: s/running/scheduled/

tallclair · 2018-02-23T23:40:23Z

plugin/pkg/auth/authorizer/rbac/bootstrappolicy/policy.go

+		// Use the Node authorization to limit a node to create tokens for service accounts running on that node
+		// Use the NodeRestriction admission plugin to limit a node to create tokens bound to pods on that node
+		tokenRequestRule := rbac.NewRule("create").Groups(legacyGroup).Resources("serviceaccounts/token").RuleOrDie()
+		nodePolicyRules = append(nodePolicyRules, tokenRequestRule)


Is this just for legacy nodes?

This is for people who haven't started using the node authorizer yet.

liggitt · 2018-02-24T02:16:15Z

plugin/pkg/admission/noderestriction/admission.go

@@ -125,6 +130,12 @@ func (c *nodePlugin) Admit(a admission.Attributes) error {
 			return admission.NewForbidden(a, fmt.Errorf("may only update PVC status"))
 		}

+	case svcacctResource:
+		if c.features.Enabled(features.TokenRequest) {


I'd actually deny altogether if the feature was disabled

Why not leave the current behavior if the feature is turned off? At this point we haven't even checked that the request was to the '/token' subresource. It's a bit weird to be doing denys in an admission controller on the resource attribute alone. Where there is enough information available to the authorizer to deny a request, we should prefer to deny the request there. All the information we would be making this deny decision on is available to the Node authorizer.

Why not leave the current behavior if the feature is turned off?

Agreed. Feature gates should maintain the current behavior when disabled.

liggitt · 2018-02-24T02:20:57Z

plugin/pkg/admission/noderestriction/admission.go

+		if c.features.Enabled(features.TokenRequest) {
+			return c.admitServiceAccount(nodeName, a)
+		}
+		fallthrough


not sure if matters if we deny above if the feature is disabled, but prefer return nil over fallthrough

mikedanese · 2018-02-24T06:08:12Z

/retest

liggitt · 2018-02-26T21:23:14Z

/lgtm

mikedanese · 2018-02-26T21:23:44Z

I need to add some TODOs
/hold

running on them.

nodes should only be able to create TokenRequests if: * token is bound to a pod * binding has uid and name * the pod exists * the pod is running on that node

…counts

k8s-ci-robot · 2018-02-26T21:46:38Z

[APPROVALNOTIFIER] This PR is APPROVED

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

~~plugin/pkg/admission/noderestriction/OWNERS~~ [liggitt]
~~plugin/pkg/auth/OWNERS~~ [liggitt]
~~test/OWNERS~~ [liggitt,mikedanese]

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

mikedanese · 2018-02-26T22:56:29Z

/retest

k8s-github-robot · 2018-02-27T18:50:43Z

Automatic merge from submit-queue (batch tested with PRs 59365, 60446, 60448, 55019, 60431). If you want to cherry-pick this change to another branch, please follow the instructions here.

mikedanese assigned liggitt and timstclair Nov 2, 2017

k8s-github-robot assigned ericchiang Nov 2, 2017

mikedanese force-pushed the svcacct branch from a027d42 to 1056cb5 Compare November 2, 2017 18:17

liggitt reviewed Nov 2, 2017

View reviewed changes

tallclair assigned tallclair and unassigned timstclair Nov 6, 2017

tallclair reviewed Nov 7, 2017

View reviewed changes

mikedanese force-pushed the svcacct branch from 1056cb5 to 1914280 Compare November 20, 2017 17:47

ericchiang mentioned this pull request Dec 11, 2017

proposal: bound service account tokens kubernetes/community#1460

Merged

mikedanese force-pushed the svcacct branch 2 times, most recently from 626d89a to 7f4e09a Compare January 11, 2018 03:02

mikedanese force-pushed the svcacct branch from 7f4e09a to f813539 Compare January 11, 2018 18:43

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 19, 2018

mikedanese force-pushed the svcacct branch from f813539 to d3fa175 Compare January 19, 2018 17:44

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 19, 2018

kubernetes deleted a comment from k8s-github-robot Jan 19, 2018

tallclair reviewed Jan 24, 2018

View reviewed changes

ericchiang changed the title ~~auth: allow nodes to impersonate svcaccts of pods~~ auth: allow nodes to create tokens for svcaccts of pods Jan 25, 2018

mikedanese added this to the v1.10 milestone Feb 22, 2018

mikedanese force-pushed the svcacct branch from d3fa175 to ac1d3ce Compare February 22, 2018 19:49

mikedanese force-pushed the svcacct branch from d3e9517 to 7278b91 Compare February 23, 2018 21:16

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 Feb 23, 2018

tallclair reviewed Feb 23, 2018

View reviewed changes

mikedanese force-pushed the svcacct branch from 7278b91 to 1311458 Compare February 24, 2018 00:32

liggitt reviewed Feb 24, 2018

View reviewed changes

mikedanese force-pushed the svcacct branch from 1311458 to a879abc Compare February 24, 2018 02:58

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 26, 2018

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 26, 2018

mikedanese mentioned this pull request Feb 26, 2018

Implement TokenRequest #58790

Closed

2 tasks

mikedanese added 3 commits February 26, 2018 13:46

auth: allow nodes to create tokones for svcaccts of pods

2cc75f0

running on them.

noderestriction: restrict nodes TokenRequest permission

b43cd73

nodes should only be able to create TokenRequests if: * token is bound to a pod * binding has uid and name * the pod exists * the pod is running on that node

rbac: allow system:node role to make TokenRequests for all service ac…

363e861

…counts

mikedanese force-pushed the svcacct branch from a879abc to 363e861 Compare February 26, 2018 21:46

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 26, 2018

mikedanese added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Feb 26, 2018

k8s-github-robot merged commit 513e67a into kubernetes:master Feb 27, 2018

mikedanese deleted the svcacct branch February 27, 2018 19:09

enj mentioned this pull request Nov 2, 2024

Enforce service account node audience restriction #128077

Merged

auth: allow nodes to create tokens for svcaccts of pods #55019

auth: allow nodes to create tokens for svcaccts of pods #55019

Conversation

mikedanese commented Nov 2, 2017 • edited Loading

liggitt commented Nov 2, 2017

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

mikedanese commented Nov 7, 2017

liggitt commented Nov 10, 2017

mikedanese commented Jan 11, 2018

tallclair 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

liggitt commented Jan 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikedanese Feb 24, 2018 • edited Loading

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

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

mikedanese commented Feb 24, 2018

liggitt commented Feb 26, 2018

mikedanese commented Feb 26, 2018 • edited Loading

k8s-ci-robot commented Feb 26, 2018

mikedanese commented Feb 26, 2018

k8s-github-robot commented Feb 27, 2018

mikedanese commented Nov 2, 2017 •

edited

Loading

mikedanese Feb 24, 2018 •

edited

Loading

mikedanese commented Feb 26, 2018 •

edited

Loading