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

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

Merged
merged 3 commits into from Feb 27, 2018

Conversation

@mikedanese
Member

mikedanese commented Nov 2, 2017

ref #58790

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

This comment has been minimized.

Member

liggitt commented Nov 2, 2017

Outlining the use cases for this would be helpful.

@@ -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))

This comment has been minimized.

@liggitt

liggitt Nov 2, 2017

Member

if len(pod.Spec.ServiceAccountName) > 0

@tallclair tallclair assigned tallclair and unassigned timstclair Nov 6, 2017

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

This comment has been minimized.

@tallclair

tallclair Nov 7, 2017

Member

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

This comment has been minimized.

@tallclair

tallclair Nov 7, 2017

Member

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

This comment has been minimized.

@mikedanese

mikedanese Nov 7, 2017

Member

The "individual" makes the error message accurate.

@mikedanese

This comment has been minimized.

Member

mikedanese commented Nov 7, 2017

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

This comment has been minimized.

Member

liggitt commented Nov 10, 2017

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

This comment has been minimized.

Member

mikedanese commented Jan 11, 2018

updated this to work with the new design.

@@ -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

This comment has been minimized.

@tallclair

tallclair Jan 24, 2018

Member

s/nade/node

This comment has been minimized.

@mikedanese
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,
},

This comment has been minimized.

@tallclair

tallclair Jan 24, 2018

Member

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

This comment has been minimized.

@mikedanese

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

@liggitt

This comment has been minimized.

Member

liggitt commented Jan 26, 2018

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

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

@mikedanese

This comment has been minimized.

Member

mikedanese commented Feb 22, 2018

@liggitt I added those todos in #58790

I also put this behind the TokenRequest fetaure flag. I'd like to get this into 1.10.

case svcAcctResource:
if utilfeature.DefaultFeatureGate.Enabled(features.TokenRequest) {
return r.authorizeCreateToken(nodeName, serviceAccountVertexType, attrs)
}

This comment has been minimized.

@liggitt

liggitt Feb 22, 2018

Member

if the feature gate is not enabled, return no decision here (just like volume attachment)

This comment has been minimized.

@mikedanese
@@ -50,6 +51,7 @@ func init() {
}
func TestAuthorizer(t *testing.T) {
defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.TokenRequest, true)()

This comment has been minimized.

@liggitt

liggitt Feb 22, 2018

Member

see usage of features field in table test for toggling features during this test... unit tests can run in parallel (as opposed to typical integration tests), so I'd rather not get in the habit of changing global state in them

This comment has been minimized.

@mikedanese

mikedanese Feb 22, 2018

Member

Changed this to match how CSI feature flag is used.

@mikedanese

This comment has been minimized.

Member

mikedanese commented Feb 23, 2018

Addressed all comments.

@liggitt

This comment has been minimized.

Member

liggitt commented Feb 23, 2018

can those TODOs be done in this PR? I'd prefer to not drop this without the admission check

@mikedanese

This comment has been minimized.

Member

mikedanese commented Feb 23, 2018

Sure, I'll work on this today.

@k8s-ci-robot k8s-ci-robot added size/L and removed size/M labels Feb 23, 2018

@@ -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 {

This comment has been minimized.

@tallclair

tallclair Feb 23, 2018

Member

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.

This comment has been minimized.

@tallclair

tallclair Feb 23, 2018

Member

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

This comment has been minimized.

@mikedanese

mikedanese Feb 24, 2018

Member

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.

This comment has been minimized.

@tallclair

tallclair Feb 24, 2018

Member

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.

This comment has been minimized.

@liggitt

liggitt Feb 26, 2018

Member

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

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

This comment has been minimized.

@tallclair

tallclair Feb 23, 2018

Member

nit: shouldn't this be an error return?

This comment has been minimized.

@mikedanese

mikedanese Feb 24, 2018

Member

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.

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.

This comment has been minimized.

@tallclair

tallclair Feb 23, 2018

Member

nit: s/running/scheduled/

This comment has been minimized.

@mikedanese
// 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)

This comment has been minimized.

@tallclair

tallclair Feb 23, 2018

Member

Is this just for legacy nodes?

This comment has been minimized.

@mikedanese

mikedanese Feb 24, 2018

Member

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

@@ -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) {

This comment has been minimized.

@liggitt

liggitt Feb 24, 2018

Member

I'd actually deny altogether if the feature was disabled

This comment has been minimized.

@mikedanese

mikedanese Feb 24, 2018

Member

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.

This comment has been minimized.

@tallclair

tallclair Feb 24, 2018

Member

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

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

This comment has been minimized.

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

This comment has been minimized.

@liggitt

liggitt Feb 24, 2018

Member

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

This comment has been minimized.

@mikedanese
@mikedanese

This comment has been minimized.

Member

mikedanese commented Feb 24, 2018

/retest

@liggitt

This comment has been minimized.

Member

liggitt commented Feb 26, 2018

/lgtm

@mikedanese

This comment has been minimized.

Member

mikedanese commented Feb 26, 2018

I need to add some TODOs
/hold

@mikedanese mikedanese referenced this pull request Feb 26, 2018

Closed

Implement TokenRequest #58790

2 of 2 tasks complete

mikedanese added some commits Nov 2, 2017

noderestriction: restrict nodes TokenRequest permission
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
@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Feb 26, 2018

[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:

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

@k8s-merge-robot k8s-merge-robot removed the lgtm label Feb 26, 2018

@mikedanese

This comment has been minimized.

Member

mikedanese commented Feb 26, 2018

/retest

@mikedanese mikedanese added lgtm and removed do-not-merge/hold labels Feb 26, 2018

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 27, 2018

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.

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

13 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation mikedanese authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@mikedanese mikedanese deleted the mikedanese:svcacct branch Feb 27, 2018

@mikedanese mikedanese added this to Closed in Container Identity Jun 12, 2018

@mikedanese mikedanese moved this from Closed to Done in Container Identity Jun 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment