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

Fix RBAC authorizer of ServiceAccount #29071

Merged
merged 1 commit into from
Jul 20, 2016
Merged

Fix RBAC authorizer of ServiceAccount #29071

merged 1 commit into from
Jul 20, 2016

Conversation

albatross0
Copy link
Contributor

RBAC authorizer assigns a role to a wrong service account.

How to reproduce

1.Create role and rolebinding to allow default user in kube-system namespace to read secrets in kube-system namespace.

# kubectl create -f role.yaml
# kubectl create -f binding.yaml

# role.yaml
kind: Role
apiVersion: rbac.authorization.k8s.io/v1alpha1
metadata:
  name: secret-reader
  namespace: kube-system
rules:
  - apiGroups: [""]
    resources: ["secrets"]
    verbs: ["get", "watch", "list"]
    nonResourceURLs: []
# binding.yaml
kind: RoleBinding
apiVersion: rbac.authorization.k8s.io/v1alpha1
metadata:
  name: read-secrets
  namespace: kube-system
subjects:
  - kind: ServiceAccount
    name: default
    namespace: kube-system
roleRef:
  kind: Role
  namespace: kube-system
  name: secret-reader
  apiVersion: rbac.authorization.k8s.io/v1alpha1

2.Set a credential of default user

$ kubectl config set-credentials default_user --token=<token_of_system:serviceaccount:kube-system:default>
$ kubectl config set-context default_user-context --cluster=test-cluster --user=default_user
$ kubectl config use-context default_user-context

3.Try to get secrets as default user in kube-system namespace

$ kubectl --namespace=kube-system get secrets
the server does not allow access to the requested resource (get secrets)

As shown above, default user could not access to secrets.
But if I have kube-system user in default namespace, it is allowed access to secrets.

4.Create a service account and try to get secrets as kube-system user in default namespace

# kubectl --namespace=default create serviceaccount kube-system
serviceaccount "kube-system" created
$ kubectl config set-credentials kube-system_user --token=<token_of_system:serviceaccount:default:kube-system>
$ kubectl config set-context kube-system_user-context --cluster=test-cluster --user=kube-system_user
$ kubectl config use-context kube-system_user-context
$ kubectl --namespace=kube-system get secrets
NAME                  TYPE                                  DATA      AGE
default-token-8pyb3   kubernetes.io/service-account-token   3         4d

@k8s-bot
Copy link

k8s-bot commented Jul 17, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

4 similar comments
@k8s-bot
Copy link

k8s-bot commented Jul 17, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-bot
Copy link

k8s-bot commented Jul 17, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-bot
Copy link

k8s-bot commented Jul 17, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-bot
Copy link

k8s-bot commented Jul 17, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-label-needed labels Jul 17, 2016
@erictune erictune assigned ericchiang and unassigned erictune Jul 18, 2016
@ericchiang
Copy link
Contributor

Yes these are mixed up. Thanks for the fix. lgtm

@ericchiang
Copy link
Contributor

@kubernetes/sig-auth can someone okay to test this? I'd consider it a pretty high priority bug.

@liggitt
Copy link
Member

liggitt commented Jul 20, 2016

@k8s-bot ok to test

@ericchiang ericchiang mentioned this pull request Jul 20, 2016
Closed
@liggitt liggitt added release-note Denotes a PR that will be considered when it comes time to generate release notes. cherrypick-candidate and removed release-note-label-needed labels Jul 20, 2016
@liggitt liggitt added this to the v1.3 milestone Jul 20, 2016
liggitt
liggitt reviewed Jul 20, 2016
View reviewed changes
pkg/apis/rbac/validation/rulevalidation.go
@@ -202,7 +202,7 @@ func appliesToUser(user user.Info, subject rbac.Subject) (bool, error) {
return false, fmt.Errorf("subject of kind service account without specified namespace")
}
// TODO(ericchiang): Is there a better way of matching a service account name?
return "system:serviceaccount:"+subject.Name+":"+subject.Namespace == user.GetName(), nil
Copy link
Member

Choose a reason for hiding this comment

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

Use MakeUsername in pkg/serviceaccount

Copy link
Member

Choose a reason for hiding this comment

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

can remove the TODO now as well :)

@liggitt
Copy link
Member

liggitt commented Jul 20, 2016

and go ahead and squash

@albatross0
Copy link
Contributor Author

Thanks for your review !

@liggitt
Copy link
Member

liggitt commented Jul 20, 2016

squash, then LGTM

@albatross0
Fix RBAC authorizer of ServiceAccount
d1b14e2 
RBAC authorizer assigns a role to a wrong service account.
@albatross0 albatross0 force-pushed the fix_rbac_for_serviceaccounts branch from ef803b4 to d1b14e2 Compare July 20, 2016 16:59
@albatross0
Copy link
Contributor Author

Thanks for you support. I'm a newbie and learning about git.

@liggitt
Copy link
Member

liggitt commented Jul 20, 2016

LGTM

@liggitt liggitt added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 20, 2016
@liggitt
Copy link
Member

liggitt commented Jul 20, 2016

@ericchiang can you cherry-pick this to the release-1.3 branch?

@ericchiang ericchiang mentioned this pull request Jul 20, 2016
Closed
@k8s-bot
Copy link

k8s-bot commented Jul 20, 2016

GCE e2e build/test passed for commit d1b14e2.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Jul 20, 2016

GCE e2e build/test passed for commit d1b14e2.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 8043baf into kubernetes:master Jul 20, 2016
@fabioy fabioy added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Jul 24, 2016
@fabioy
Copy link
Contributor

fabioy commented Jul 24, 2016

@albatross0 Cherrypick approved. Please create a PR for this. Thank you.

@albatross0
Copy link
Contributor Author

PR #29313 already exists. Could you tell me what I should do ?

k8s-github-robot pushed a commit that referenced this pull request Jul 26, 2016
Merge pull request #29313 from ericchiang/automated-cherry-pick-of-#2…
be922c3 
…9071-upstream-release-1.3

Automatic merge from submit-queue

Automated cherry pick of #29071 upstream release 1.3
shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016
Merge pull request kubernetes#29313 from ericchiang/automated-cherry-…
ebebf01 
…pick-of-#29071-upstream-release-1.3

Automatic merge from submit-queue

Automated cherry pick of kubernetes#29071 upstream release 1.3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@ericchiang ericchiang

Labels
cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. 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. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
v1.3
Development

Successfully merging this pull request may close these issues.
8 participants
@albatross0 @k8s-bot @ericchiang @liggitt @k8s-github-robot @fabioy @googlebot @erictune