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.

Sign up for GitHub

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

Jump to bottom

Add groups to service account user.Info #8607

Merged
merged 1 commit into from
Aug 5, 2015
Merged

Add groups to service account user.Info #8607

merged 1 commit into from
Aug 5, 2015
+181 −30

Conversation

liggitt
Copy link
Member

@liggitt liggitt commented May 21, 2015

  • Adds the system: prefix to service account usernames to follow the pattern for other system-created usernames
  • Adds two groups to the user.Info returned from the JWT authenticator
    • system:serviceaccounts, which is useful for granting authorization to all service accounts
    • system:serviceaccounts:<namespace>, which is useful for granting authorization to all service accounts in a given namespace

@liggitt liggitt force-pushed the serviceaccount_groups branch from 265cfa9 to 5a4b7be Compare May 21, 2015 04:36
@liggitt
Copy link
Member Author

liggitt commented May 21, 2015

/cc @pweil- the UserInfo function is what I envisioned for turning a service account into a user.Info interface for the purposes of determining which security context contraints apply to it.

@liggitt
Copy link
Member Author

liggitt commented May 21, 2015

@erictune PTAL

@liggitt
Copy link
Member Author

liggitt commented May 21, 2015

/cc @deads2k

@liggitt liggitt force-pushed the serviceaccount_groups branch from 5a4b7be to 2d001cc Compare May 21, 2015 07:49
@liggitt liggitt mentioned this pull request May 21, 2015
Merged
10 tasks
@deads2k
Copy link
Contributor

deads2k commented May 21, 2015

I'm not sure I like including group information the token itself. I'm worried that someone would take the next step and allow the specification of group names. Maybe if we forced any custom groups to be prefixed by the NamespaceGroup? It's definitely worthy of a comment or example.

deads2k
deads2k reviewed May 21, 2015
View reviewed changes
pkg/serviceaccount/jwt.go
UID: serviceAccountUID,
Groups: []string{},
}, true, nil
return UserInfo(namespace, serviceAccountName, serviceAccountUID), true, nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is a group claim in the token, shouldn't we be respecting it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I waffled on the claim... removed it until I can think through it more

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will take a look tonight

On Thu, May 21, 2015 at 2:26 PM, Jordan Liggitt notifications@github.com
wrote:

In pkg/serviceaccount/jwt.go
#8607 (comment)
:

@@ -216,11 +205,7 @@ func (j *jwtTokenAuthenticator) AuthenticateToken(token string) (user.Info, bool
}
}

  •   return &user.DefaultInfo{

  •       Name:   sub,

  •       UID:    serviceAccountUID,

  •       Groups: []string{},

  •   }, true, nil

  •   return UserInfo(namespace, serviceAccountName, serviceAccountUID), true, nil

I waffled on the claim... removed it until I can think through it more

—
Reply to this email directly or view it on GitHub
https://github.com/GoogleCloudPlatform/kubernetes/pull/8607/files#r30849532
.

@liggitt liggitt force-pushed the serviceaccount_groups branch from 2d001cc to 2726e25 Compare May 21, 2015 21:25
@liggitt liggitt changed the title Add groups to service account JWT Add groups to service account user.Info May 21, 2015
@k8s-bot
Copy link

k8s-bot commented May 21, 2015

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

If this message is too spammy, please complain @ixdy.

@pweil-
Copy link
Contributor

pweil- commented May 22, 2015

/cc @pweil- the UserInfo function is what I envisioned for turning a service account into a user.Info interface for the purposes of determining which security context contraints apply to it.

@pmorie this is what we'll need for the admission

@liggitt liggitt mentioned this pull request May 23, 2015
Merged
7 tasks
@liggitt liggitt force-pushed the serviceaccount_groups branch from 2726e25 to 7310245 Compare May 23, 2015 03:07
@pweil- pweil- mentioned this pull request May 26, 2015
Closed
9 tasks
@erictune
Copy link
Member

Is this needed for things other than #7893? If so, give use case?

@liggitt
Copy link
Member Author

liggitt commented May 30, 2015

Use cases:

  1. Be able to grant all service accounts in a namespace read access to resources in that namespace (e.g. services, endpoints, etc). Being able to address them as a group (e.g. system:serviceaccounts:mynamespace) means grants don't have to be manually managed as service accounts are added/removed. In OpenShift, we use this to grant read access to our ImageStream resource to allow any service account in a namespace to pull an image in that namespace.
  2. Be able to grant all service accounts access to resources (to extend use case 1, to have a common namespace that all service accounts could list services in). Again, addressing them as a group (e.g. system:serviceaccounts) makes this very simple.

@erictune
Copy link
Member

erictune commented Jun 3, 2015

I think this makes sense, but at this point, I'm going to defer this till after 1.0.

@erictune erictune added this to the v1.0-post milestone Jun 3, 2015
@liggitt liggitt force-pushed the serviceaccount_groups branch 2 times, most recently from 5ce8a9b to 657d9df Compare June 5, 2015 19:11
@liggitt liggitt mentioned this pull request Jun 16, 2015
Merged
@deads2k
Copy link
Contributor

deads2k commented Jun 30, 2015

@liggitt please rebase on 96828f2 and let me know when I can pick it.

@liggitt liggitt force-pushed the serviceaccount_groups branch from 657d9df to 30b2655 Compare June 30, 2015 20:44
@k8s-bot
Copy link

k8s-bot commented Jun 30, 2015

GCE e2e build/test passed for commit 30b2655fa9efda8c65ebe003157770b70354482c.

@liggitt
Copy link
Member Author

liggitt commented Jun 30, 2015

@deads2k rebased

@bgrant0607 bgrant0607 removed this from the v1.0-post milestone Jul 24, 2015
@erictune
Copy link
Member

lgtm

@liggitt
Copy link
Member Author

liggitt commented Aug 3, 2015

@erictune label lgtm, or tag a second reviewer?

@liggitt liggitt force-pushed the serviceaccount_groups branch from 30b2655 to 9bef018 Compare August 4, 2015 14:47
@k8s-bot
Copy link

k8s-bot commented Aug 4, 2015

GCE e2e build/test failed for commit 9bef0188747ad379ba02b94cd5c242f6260710a1.

@liggitt liggitt force-pushed the serviceaccount_groups branch from 9bef018 to 709c2c8 Compare August 4, 2015 17:03
@k8s-bot
Copy link

k8s-bot commented Aug 4, 2015

GCE e2e build/test passed for commit 709c2c8.

@erictune erictune added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 4, 2015
@alex-mohr
Copy link
Contributor

@k8s-bot test this [contrib/submit-queue: candidate for merging]

@k8s-bot
Copy link

k8s-bot commented Aug 5, 2015

GCE e2e build/test passed for commit 709c2c8.

@alex-mohr
Copy link
Contributor

@k8s-bot test this [contrib/submit-queue: candidate for merging]

@k8s-bot
Copy link

k8s-bot commented Aug 5, 2015

GCE e2e build/test failed for commit 709c2c8.

alex-mohr added a commit that referenced this pull request Aug 5, 2015
@alex-mohr
Merge pull request #8607 from liggitt/serviceaccount_groups
c29c841 
Add groups to service account user.Info
@alex-mohr alex-mohr merged commit c29c841 into kubernetes:master Aug 5, 2015
@liggitt liggitt deleted the serviceaccount_groups branch August 5, 2015 12:36
@erictune erictune added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Sep 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@erictune erictune

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@liggitt @deads2k @k8s-bot @pweil- @erictune @alex-mohr @googlebot @bgrant0607