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

Aggregation ClusterRole circular reference bug #69652

Closed
WanLinghao opened this Issue Oct 11, 2018 · 5 comments

Comments

Projects
None yet
4 participants
@WanLinghao
Contributor

WanLinghao commented Oct 11, 2018

/kind bug

For aggregation clusterrole, in some cases, circular dependency could exist between them, which may cause wired behaviours.
For example, if we create clusterroles as follow:

kind: ClusterRole
apiVersion: rbac.authorization.k8s.io/v1
metadata:
  name: agg1 
  labels:
    rbac.example.com/point: "point1"
aggregationRule:
  clusterRoleSelectors:
  - matchLabels:
      rbac.example.com/point: "point2"
rules: [] 

kind: ClusterRole
apiVersion: rbac.authorization.k8s.io/v1
metadata:
  name: agg2 
  labels:
    rbac.example.com/point: "point2"
aggregationRule:
  clusterRoleSelectors:
  - matchLabels:
      rbac.example.com/point: "point3"
rules: [] 

kind: ClusterRole
apiVersion: rbac.authorization.k8s.io/v1
metadata:
  name: agg3
  labels:
    rbac.example.com/point: "point3"
aggregationRule:
  clusterRoleSelectors:
  - matchLabels:
      rbac.example.com/point: "point1"
rules: [] 

Then we create some non-aggregation clusterrole:

kind: ClusterRole
apiVersion: rbac.authorization.k8s.io/v1
metadata:
  name: child1 
  labels:
    rbac.example.com/point: "point1"
rules:
- apiGroups: [""]
  resources: ["services", "endpoints", "pods"]
  verbs: ["get", "list", "watch"]

All three aggregation clusterroles would look like:


kubectl describe clusterrole agg1
Name:         agg1
Labels:       rbac.example.com/point=point1
Annotations:  <none>
PolicyRule:
  Resources  Non-Resource URLs  Resource Names  Verbs
  ---------  -----------------  --------------  -----
  endpoints  []                 []              [get list watch]
  pods       []                 []              [get list watch]
  services   []                 []              [get list watch]

Then we delete clusterrole child1, all three agg-clusterroles would keep unchanged with the rules from child1. However, the aggregation clusterrole is designed to aggregate rules from other clusterrole, I think it should not keep the rules after the child1 being deleted.
This is just a example which shows circular dependency aggregation clusterroles would cause some wired behaviours. I think it may cause some other bugs or potential security risk. So we need to avoid the circular dependency between agg-clusterroles.

@WanLinghao

This comment has been minimized.

Show comment
Hide comment
@WanLinghao

WanLinghao Oct 11, 2018

Contributor

/sig auth

Contributor

WanLinghao commented Oct 11, 2018

/sig auth

@k8s-ci-robot k8s-ci-robot added sig/auth and removed needs-sig labels Oct 11, 2018

@WanLinghao

This comment has been minimized.

Show comment
Hide comment
@WanLinghao

WanLinghao Oct 11, 2018

Contributor

/assign

Contributor

WanLinghao commented Oct 11, 2018

/assign

@WanLinghao WanLinghao changed the title from ClusterRole AggregationRule circular reference bug to Aggregation ClusterRole circular reference bug Oct 11, 2018

@liggitt

This comment has been minimized.

Show comment
Hide comment
@liggitt
Member

liggitt commented Oct 12, 2018

/cc @deads2k

@deads2k

This comment has been minimized.

Show comment
Hide comment
@deads2k

deads2k Oct 12, 2018

Contributor

The scenario appears to set up a cycle in cluster roles where one role chains to another. Multi-stage chaining is acceptable and even desireable: see usage for admin, edit, and view where coverage is guaranteed this way.

In the scenario described here, a clusterrole is aggregating a clusterrole with permission: X. Because it is a cycle, one of the chains in the cycle has that permission so it ripples through.

There is no security risk here because the rules for aggregation remain the same: aggregate all the roles matching the label selector. In addition, only someone will full RBAC permissions can decide to make an aggregated role and only someone trusted can create new cluster role that can be aggregated.

The current behavior is as expected and is not a security risk.

/close

Contributor

deads2k commented Oct 12, 2018

The scenario appears to set up a cycle in cluster roles where one role chains to another. Multi-stage chaining is acceptable and even desireable: see usage for admin, edit, and view where coverage is guaranteed this way.

In the scenario described here, a clusterrole is aggregating a clusterrole with permission: X. Because it is a cycle, one of the chains in the cycle has that permission so it ripples through.

There is no security risk here because the rules for aggregation remain the same: aggregate all the roles matching the label selector. In addition, only someone will full RBAC permissions can decide to make an aggregated role and only someone trusted can create new cluster role that can be aggregated.

The current behavior is as expected and is not a security risk.

/close

@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Oct 12, 2018

Contributor

@deads2k: Closing this issue.

In response to this:

The scenario appears to set up a cycle in cluster roles where one role chains to another. Multi-stage chaining is acceptable and even desireable: see usage for admin, edit, and view where coverage is guaranteed this way.

In the scenario described here, a clusterrole is aggregating a clusterrole with permission: X. Because it is a cycle, one of the chains in the cycle has that permission so it ripples through.

There is no security risk here because the rules for aggregation remain the same: aggregate all the roles matching the label selector. In addition, only someone will full RBAC permissions can decide to make an aggregated role and only someone trusted can create new cluster role that can be aggregated.

The current behavior is as expected and is not a security risk.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Contributor

k8s-ci-robot commented Oct 12, 2018

@deads2k: Closing this issue.

In response to this:

The scenario appears to set up a cycle in cluster roles where one role chains to another. Multi-stage chaining is acceptable and even desireable: see usage for admin, edit, and view where coverage is guaranteed this way.

In the scenario described here, a clusterrole is aggregating a clusterrole with permission: X. Because it is a cycle, one of the chains in the cycle has that permission so it ripples through.

There is no security risk here because the rules for aggregation remain the same: aggregate all the roles matching the label selector. In addition, only someone will full RBAC permissions can decide to make an aggregated role and only someone trusted can create new cluster role that can be aggregated.

The current behavior is as expected and is not a security risk.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

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