fix: Match RoleBindings bound to ClusterRoles #34
Conversation
Codecov Report
@@ Coverage Diff @@
## master #34 +/- ##
===========================================
+ Coverage 65.33% 75.39% +10.05%
===========================================
Files 4 5 +1
Lines 375 382 +7
===========================================
+ Hits 245 288 +43
+ Misses 124 83 -41
- Partials 6 11 +5
Continue to review full report at Codecov.
|
| @@ -395,6 +397,7 @@ func (w *whoCan) GetClusterRolesFor(action Action) (clusterRoles, error) { | |||
|
|
|||
| // GetRoleBindings returns the RoleBindings that refer to the given set of Role names or ClusterRole names. | |||
| func (w *whoCan) GetRoleBindings(roleNames roles, clusterRoleNames clusterRoles) (roleBindings []rbac.RoleBinding, err error) { | |||
| // TODO I'm wondering if GetRoleBindings should be invoked at all when the --all-namespaces flag is specified? | |||
There was a problem hiding this comment.
Interesting! I think this comes down to the semantics of --all-namespaces, which I now think is ambiguous:
- If it means "who can verb resource in any namespace" then yes, I think we should be looking at all the possible rolebindings.
- If it means "who can verb resource in all namespace" then we could limit to just clusterRoles, but then we'd be missing the possibility of someone who has this ability, not through a clusterRole but by being bound to a Role for every namespace
The first of these options seems more useful to me anyway. Either way, we should clarify it in the usage.
There was a problem hiding this comment.
So maybe we should have a dedicated flags --any-namespace and --all-namespaces to make it less ambiguous? Plus what you mentioned a better usage docs. Anyway I'll file an issue for that to consider it carefully.
| Rules: []rbac.PolicyRule{ | ||
| { | ||
| Verbs: []string{"get"}, | ||
| NonResourceURLs: []string{"/logs"}, | ||
| APIGroups: []string{""}, |
There was a problem hiding this comment.
If we wanted to support CRDs, would we need to allow the user to specify the APIGroup?
There was a problem hiding this comment.
That's a good point. We do not currently resolve APIGroup so that's another improvement we might want to put in place.
$ kubectl auth can-i get deployments.extensions
yes
$ kubectl who-can get deployments.extensions
Error: resolving resource: the server doesn't have a resource type "deployments.extensions"
| @@ -49,6 +49,11 @@ NONRESOURCEURL is a partial URL that starts with "/".` | |||
|
|
|||
| # List who can access the URL /logs/ | |||
| kubectl who-can get /logs` | |||
|
|
|||
| // RoleKind is the RoleRef's Kind referencing a Role. | |||
| RoleKind = "Role" | |||
There was a problem hiding this comment.
I'm OK with this as it's unlikely to change, but is there not a definition we could pull in from kubernetes somewhere?
There was a problem hiding this comment.
Unfortunately not. I did search in project's dependencies and it seems that it's common to repeatedly type in "Role" and "CusterRole" literals all over the place ;)
pkg/cmd/list.go
Outdated
| glog.V(1).Info(fmt.Sprintf("Match found: roleRef: %v", roleBinding.RoleRef)) | ||
| for _, roleBinding := range list.Items { | ||
| if _, ok := clusterRoleNames[roleBinding.RoleRef.Name]; ok { | ||
| //if w.clusterRoleBindingMatches(&roleBinding, clusterRoleNames) { |
There was a problem hiding this comment.
I doubt you meant to leave this comment here
…r to further facilitate unit testing
This one is supposed to resolve #31 . I took also the opportunity to refactor and add more unit tests to the functionality of mapping [Cluster]RoleBindings to [Cluster]Roles.
At the end I've added the scenario to our integration test suite which reproduced the actual issue reported by @raesene
I suggest review this PR commit by commit.