diff --git a/authz/providers/azure/rbac/checkaccessreqhelper.go b/authz/providers/azure/rbac/checkaccessreqhelper.go index aea5e93c..eb8af1df 100644 --- a/authz/providers/azure/rbac/checkaccessreqhelper.go +++ b/authz/providers/azure/rbac/checkaccessreqhelper.go @@ -49,13 +49,13 @@ const ( NonAADUserNotAllowedVerdict = "Access denied by Azure RBAC for non AAD users. Configure --azure.skip-authz-for-non-aad-users to enable access. If you are an AAD user, please set Extra:oid parameter for impersonated user in the kubeconfig." CheckAccessErrorVerdict = "Access denied due to Azure RBAC check failure. Please retry later." PodsResource = "pods" - ServicesResource = "services" - NodesResource = "nodes" CustomResources = "customresources" - ProxySubresource = "proxy" - AttachSubresource = "attach" - PortForwardSubresource = "portforward" - ExecSubresource = "exec" + StatusSubresource = "status" + ScaleSubresource = "scale" + LogSubresource = "log" + LogsSubresource = "logs" + actionSuffix = "action" + wildcardValue = "*" ReadVerb = "read" WriteVerb = "write" DeleteVerb = "delete" @@ -254,40 +254,52 @@ func getActionName(verb string) string { } } -// securitySensitiveSubresources lists resource/subresource pairs that upstream -// Kubernetes treats as distinct authorization targets. For these, the -// subresource is preserved in the DataAction string -// ("//action") rather than collapsed into the base -// resource action, so the authorization decision keeps the same granularity as -// the upstream Kubernetes RBAC model (see the bootstrappolicy view/edit -// ClusterRoles). +// safeSubresources lists the subresources that upstream Kubernetes treats as +// part of the parent resource's read/write permission: the read-only "view" +// ClusterRole grants them alongside the parent resource (pods/log, pods/status, +// deployments/scale, and the /status and /scale subresources generally). For +// these, the subresource is collapsed into the base resource action +// (/read, /write, ...). // -// The pods exec/attach/portforward/proxy, services/proxy and nodes/proxy -// subresources are granted separately from base read/write in the upstream -// roles, so they are mapped to their own DataAction rather than to -// /read or /write. -var securitySensitiveSubresources = map[string]map[string]struct{}{ - PodsResource: { - ExecSubresource: {}, - AttachSubresource: {}, - PortForwardSubresource: {}, - ProxySubresource: {}, - }, - ServicesResource: { - ProxySubresource: {}, - }, - NodesResource: { - ProxySubresource: {}, - }, +// Every other subresource keeps its own DataAction +// ("//action"). The default is deliberately the distinct +// action rather than the collapsed one: a subresource that has not been +// classified - including subresources added by future Kubernetes versions, by +// CRDs, or by aggregated API servers - must not be silently covered by the +// parent resource's permission. +// +// Special verbs (bind, escalate, use, impersonate, ...) already encode the +// privileged operation in the action name, so getResourceAndAction leaves those +// mappings unchanged. +var safeSubresources = map[string]struct{}{ + StatusSubresource: {}, + ScaleSubresource: {}, + // Kubernetes names this subresource "log"; guard has historically also seen + // the plural spelling, so both are treated as the same read-only capability. + LogSubresource: {}, + LogsSubresource: {}, } 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") - } + action := getActionName(verb) + + // Nothing to preserve when there is no subresource, or when the request is a + // wildcard one (those are expanded from the operations map elsewhere). + if subResource == "" || subResource == wildcardValue || resource == wildcardValue || action == wildcardValue { + return path.Join(resource, action) + } + + // Special verbs already encode the privileged operation in the action name; + // the verb, not the subresource, identifies what is being authorized. + if strings.HasSuffix(action, actionSuffix) { + return path.Join(resource, action) + } + + if _, safe := safeSubresources[subResource]; safe { + return path.Join(resource, action) } - return path.Join(resource, getActionName(verb)) + + return path.Join(resource, subResource, actionSuffix) } func getDataActions(ctx context.Context, subRevReq *authzv1.SubjectAccessReviewSpec, clusterType string, allowCustomResourceTypeCheck bool, allowSubresourceTypeCheck bool) ([]azureutils.AuthorizationActionInfo, error) { diff --git a/authz/providers/azure/rbac/checkaccessreqhelper_test.go b/authz/providers/azure/rbac/checkaccessreqhelper_test.go index 36864b3c..c4dd220d 100644 --- a/authz/providers/azure/rbac/checkaccessreqhelper_test.go +++ b/authz/providers/azure/rbac/checkaccessreqhelper_test.go @@ -373,6 +373,8 @@ func Test_getDataActions(t *testing.T) { []azureutils.AuthorizationActionInfo{{AuthorizationEntity: azureutils.AuthorizationEntity{Id: "arc/batch/cronjobs/write"}, IsDataAction: true}}, }, + // certificatesigningrequests/approvals is not a safe subresource, so it + // no longer collapses into the parent delete action. { "aks6", args{ @@ -381,7 +383,7 @@ func Test_getDataActions(t *testing.T) { ResourceAttributes: &authzv1.ResourceAttributes{Group: "certificates.k8s.io", Resource: "certificatesigningrequests", Subresource: "approvals", Version: "v1", Name: "test", Verb: "deletecollection"}, }, clusterType: aksClusterType, }, - []azureutils.AuthorizationActionInfo{{AuthorizationEntity: azureutils.AuthorizationEntity{Id: "aks/certificates.k8s.io/certificatesigningrequests/delete"}, IsDataAction: true}}, + []azureutils.AuthorizationActionInfo{{AuthorizationEntity: azureutils.AuthorizationEntity{Id: "aks/certificates.k8s.io/certificatesigningrequests/approvals/action"}, IsDataAction: true}}, }, { @@ -392,7 +394,7 @@ func Test_getDataActions(t *testing.T) { ResourceAttributes: &authzv1.ResourceAttributes{Group: "certificates.k8s.io", Resource: "certificatesigningrequests", Subresource: "approvals", Version: "v1", Name: "test", Verb: "deletecollection"}, }, clusterType: "fleet", }, - []azureutils.AuthorizationActionInfo{{AuthorizationEntity: azureutils.AuthorizationEntity{Id: "fleet/certificates.k8s.io/certificatesigningrequests/delete"}, IsDataAction: true}}, + []azureutils.AuthorizationActionInfo{{AuthorizationEntity: azureutils.AuthorizationEntity{Id: "fleet/certificates.k8s.io/certificatesigningrequests/approvals/action"}, IsDataAction: true}}, }, { @@ -501,8 +503,8 @@ func Test_getDataActions(t *testing.T) { []azureutils.AuthorizationActionInfo{{AuthorizationEntity: azureutils.AuthorizationEntity{Id: "aks/nodes/proxy/action"}, IsDataAction: true}}, }, - // A non-sensitive pods subresource (e.g. status) must still collapse to - // the base read action so this fix does not over-restrict legitimate reads. + // status is a safe subresource: it collapses to the parent read action so + // legitimate read-only access is not over-restricted. { "podsStatusSubresourceStillCollapsed", args{ @@ -514,17 +516,55 @@ func Test_getDataActions(t *testing.T) { []azureutils.AuthorizationActionInfo{{AuthorizationEntity: azureutils.AuthorizationEntity{Id: "aks/pods/read"}, IsDataAction: true}}, }, - // CSR with a non-sensitive subresource (e.g. approval, status) - // should still collapse to the base action, not preserve the subresource. + // certificatesigningrequests/approval is not a safe subresource, so it + // gets its own DataAction instead of collapsing into the parent write. { - "csrApprovalSubresourceStillCollapsed", + "csrApprovalSubresourceGetsOwnAction", args{ isWildcardTest: false, subRevReq: &authzv1.SubjectAccessReviewSpec{ ResourceAttributes: &authzv1.ResourceAttributes{Group: "certificates.k8s.io", Resource: "certificatesigningrequests", Subresource: "approval", Version: "v1", Name: "test", Verb: "update"}, }, clusterType: aksClusterType, }, - []azureutils.AuthorizationActionInfo{{AuthorizationEntity: azureutils.AuthorizationEntity{Id: "aks/certificates.k8s.io/certificatesigningrequests/write"}, IsDataAction: true}}, + []azureutils.AuthorizationActionInfo{{AuthorizationEntity: azureutils.AuthorizationEntity{Id: "aks/certificates.k8s.io/certificatesigningrequests/approval/action"}, IsDataAction: true}}, + }, + + // Safe subresources collapse into the parent resource action: upstream's + // read-only view role grants them alongside the parent resource. + { + "podsLogSubresourceCollapses", + args{ + isWildcardTest: false, + subRevReq: &authzv1.SubjectAccessReviewSpec{ + ResourceAttributes: &authzv1.ResourceAttributes{Group: "", Resource: "pods", Subresource: "log", Version: "v1", Name: "test", Verb: "get"}, + }, clusterType: aksClusterType, + }, + []azureutils.AuthorizationActionInfo{{AuthorizationEntity: azureutils.AuthorizationEntity{Id: "aks/pods/read"}, IsDataAction: true}}, + }, + + { + "deploymentsScaleSubresourceCollapses", + args{ + isWildcardTest: false, + subRevReq: &authzv1.SubjectAccessReviewSpec{ + ResourceAttributes: &authzv1.ResourceAttributes{Group: "apps", Resource: "deployments", Subresource: "scale", Version: "v1", Name: "test", Verb: "update"}, + }, clusterType: aksClusterType, + }, + []azureutils.AuthorizationActionInfo{{AuthorizationEntity: azureutils.AuthorizationEntity{Id: "aks/apps/deployments/write"}, IsDataAction: true}}, + }, + + // An unclassified subresource - a new Kubernetes subresource, a CRD or an + // aggregated API - must NOT inherit the parent resource permission. This is + // the point of listing what is safe rather than what is sensitive. + { + "unknownSubresourceIsNotCollapsed", + args{ + isWildcardTest: false, + subRevReq: &authzv1.SubjectAccessReviewSpec{ + ResourceAttributes: &authzv1.ResourceAttributes{Group: "", Resource: "pods", Subresource: "somefuturesubresource", Version: "v1", Name: "test", Verb: "get"}, + }, clusterType: aksClusterType, + }, + []azureutils.AuthorizationActionInfo{{AuthorizationEntity: azureutils.AuthorizationEntity{Id: "aks/pods/somefuturesubresource/action"}, IsDataAction: true}}, }, { @@ -1457,7 +1497,8 @@ func Test_getResultCacheKey(t *testing.T) { }, allowSubresourceTypeCheck: false, }, - "beta@msn.com/-/-/nodes/read", + // scopes is not a safe subresource, so it no longer collapses to nodes/read. + "beta@msn.com/-/-/nodes/scopes/action", }, {