Skip to content

List safe subresources instead of sensitive ones #25

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

Open
Artem Kolomeetc (arxhive) wants to merge 1 commit into
base: master
Choose a base branch
from
Open

List safe subresources instead of sensitive ones #25

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 47 additions & 35 deletions authz/providers/azure/rbac/checkaccessreqhelper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
// ("<resource>/<subresource>/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
// (<resource>/read, <resource>/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
// <resource>/read or <resource>/write.
var securitySensitiveSubresources = map[string]map[string]struct{}{
PodsResource: {
ExecSubresource: {},
AttachSubresource: {},
PortForwardSubresource: {},
ProxySubresource: {},
},
ServicesResource: {
ProxySubresource: {},
},
NodesResource: {
ProxySubresource: {},
},
// Every other subresource keeps its own DataAction
// ("<resource>/<subresource>/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) {
Expand Down
59 changes: 50 additions & 9 deletions authz/providers/azure/rbac/checkaccessreqhelper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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}},
},

{
Expand All @@ -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}},
},

{
Expand Down Expand Up @@ -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{
Expand All @@ -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}},
},

{
Expand Down Expand Up @@ -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",
},

{
Expand Down
Loading