Preserve subresource granularity in CheckAccess DataAction mapping - #22
Conversation
Artem Kolomeetc (arxhive)
changed the title
363ad1e to
e3c8339
Compare
Artem Kolomeetc (arxhive)
changed the title
Dong Liu (karataliu)
left a comment
Dong Liu (karataliu)
left a comment
There was a problem hiding this comment.
this would introduce breaks to existing cases, consider updating release note when bumping guard version.
|
how will this be impacted by #23 |
Several Kubernetes subresources are distinct authorization targets in the upstream Kubernetes RBAC model: pods/exec, pods/attach, pods/portforward, pods/proxy, services/proxy and nodes/proxy. The DataAction builder mapped a request on these onto the base resource's action (for example pods/read). Preserve the subresource in the DataAction as "<resource>/<subresource>/action" so the authorization decision keeps the same granularity as the upstream view/edit ClusterRoles, using the same mechanism already applied to pods/exec and certificatesigningrequests/nodeclient. Add the corresponding subresource name constants and unit tests. Signed-off-by: Artem Kolomeetc <akolomeetc@microsoft.com>
e3c8339 to
e12396e
Compare
Rebased onto master after #23 merged. #23 reverted #12, which removed the securitySensitiveSubresources map and restored the inline exec check in getResourceAndAction. This PR reintroduces the map form of getResourceAndAction, but scoped to pod exec/attach/portforward/proxy, services/proxy and nodes/proxy only. It does not re-add the certificatesigningrequests/nodeclient entry that #23 reverted, so the CSR behavior on master is unchanged. pods/exec keeps the same output as the current inline check. Conflict resolved in e12396e; build, lint and unit tests pass. |
Agreed - this is a behavioral change: a GET on pods exec/attach/portforward/proxy, services/proxy and nodes/proxy now maps to //action instead of the base /read. Callers that relied on the collapsed mapping need the corresponding data action granted. docs/CHANGELOG.md is auto-generated (frozen at v0.6.2) and versioning is git-tag driven, so the note belongs in the AKS guard version-bump rather than a hand edit here. I will flag it as a breaking change in that bump. Let me know if you would prefer it captured somewhere in-repo as well. |
| var securitySensitiveSubresources = map[string]map[string]struct{}{ | ||
| PodsResource: { | ||
| ExecSubresource: {}, | ||
| AttachSubresource: {}, | ||
| PortForwardSubresource: {}, | ||
| ProxySubresource: {}, | ||
| }, | ||
| ServicesResource: { | ||
| ProxySubresource: {}, | ||
| }, | ||
| NodesResource: { | ||
| ProxySubresource: {}, | ||
| }, | ||
| } | ||
|
|
||
| if resource == PodsResource && subResource == "exec" { | ||
| action = path.Join(resource, subResource, "action") | ||
| } else { | ||
| action = path.Join(resource, getActionName(verb)) | ||
| func getResourceAndAction(resource string, subResource string, verb string) string { | ||
| if subs, ok := securitySensitiveSubresources[resource]; ok && subResource != "" { | ||
| if _, sensitive := subs[subResource]; sensitive { | ||
| return path.Join(resource, subResource, "action") | ||
| } | ||
| } | ||
|
|
||
| return action | ||
| return path.Join(resource, getActionName(verb)) |
There was a problem hiding this comment.
This is not the right approach. It should be inverted to define what is safe (i.e. the status sub-resource), not what is unsafe.
There was a problem hiding this comment.
Agreed, and the history of this list makes the point - it grew from exec to nodeclient to proxy to attach/portforward, each time after the gap was found rather than before. Inverted in #25.
The safe set is taken from the upstream view ClusterRole (bootstrappolicy.viewRules), which grants exactly these subresource kinds alongside their parent: status, scale, and pods/log. So safeSubresources = {status, scale, log, logs} and everything else gets //action. Wildcard requests and the special verbs (bind/escalate/use/impersonate) keep their current mapping, since there the verb rather than the subresource identifies the operation.
Two consequences worth your eye, both called out in the PR:
- The Microsoft.ContainerService registry currently defines only managedClusters/pods/{read,write,delete} and pods/exec/action, so anything off the safe list matches only wildcard-bearing roles and is denied for roles that enumerate leaf actions. Fail-closed as intended, but a behavioral change.
- It also re-gates the CSR subresources (approval/approvals, and the same rule would cover nodeclient). Revert "fix: CSR subresource action collapse allowing unauthorized kubelet certificate issuance"聽#23 reverted fix: CSR subresource action collapse allowing unauthorized kubelet certificate issuance聽#12 which had gated nodeclient, so this partially re-applies what that revert removed. If CSR should stay collapsed until those DataActions exist, I will add them to the safe set explicitly.
5 participants
Summary
Several Kubernetes subresources are distinct authorization targets in the upstream Kubernetes RBAC model:
pods/exec,pods/attach,pods/portforward,pods/proxy,services/proxyandnodes/proxy. The CheckAccessDataActionbuilder mapped a request on these onto the base resource's action (for examplepods/read).This change preserves the subresource in the
DataActionas<resource>/<subresource>/action, using the same mechanism already applied topods/execandcertificatesigningrequests/nodeclient, so the authorization decision keeps the same granularity as the upstream view/edit ClusterRoles.Changes
podsexec/attach/portforward/proxy,services/proxyandnodes/proxyto distinct<resource>/<subresource>/actionDataActions.Testing
go build ./...- OKgofmt -l- cleangolangci-lint run ./authz/providers/azure/rbac/inghcr.io/appscode/golang-dev:1.25- 0 issuesgo test ./authz/providers/azure/rbac/... ./auth/providers/azure/graph/...- all pass