From caa7343f133bc81a4ff1c8d9903eb2cacb3994ba Mon Sep 17 00:00:00 2001 From: Artem Kolomeetc Date: Mon, 10 Aug 2026 17:55:00 -0700 Subject: [PATCH 1/3] fix(authz): scope non-resource discovery exemption and isolate its cache key (MSRC 132991) Harden how Guard authorizes and caches non-resource (discovery) requests so a non-resource SubjectAccessReview cannot influence the authorization decision cached for an unrelated resource request. Two independent changes, either of which is sufficient on its own: 1. Discovery-path matching (AllowNonResPathDiscoveryAccess) previously used a loose strings.HasPrefix(path, "/api"), which also matched non-discovery paths such as "/apiz...". It now matches only an exact discovery/health root or a proper path-segment boundary (root + "/"), and rejects any path containing a ".." traversal segment, via the new isNonResourceDiscoveryPath helper. 2. Non-resource cache keys (getResultCacheKey) were built with path.Join, whose path.Clean resolves "..", so a non-resource path could normalize onto the key of an unrelated resource request; resource and non-resource requests also shared one key namespace. Non-resource keys are now built in a disjoint namespace with a NUL field separator (which cannot appear in a URL path, verb, or user name) and no longer pass through path.Join/path.Clean, so a decision cached for one request can never be served for a different one. Adds Test_AllowNonResPathDiscoveryAccess (real endpoints vs look-alikes vs traversal) and Test_getResultCacheKey_noResourceNonResourceCollision, and updates the existing non-resource getResultCacheKey expectations to the new key format. Signed-off-by: Artem Kolomeetc --- .../azure/rbac/checkaccessreqhelper.go | 23 +++++++- .../azure/rbac/checkaccessreqhelper_test.go | 51 +++++++++++++++- authz/providers/azure/rbac/rbac.go | 36 +++++++++-- authz/providers/azure/rbac/rbac_test.go | 59 +++++++++++++++++++ 4 files changed, 162 insertions(+), 7 deletions(-) diff --git a/authz/providers/azure/rbac/checkaccessreqhelper.go b/authz/providers/azure/rbac/checkaccessreqhelper.go index aea5e93c8..54a602095 100644 --- a/authz/providers/azure/rbac/checkaccessreqhelper.go +++ b/authz/providers/azure/rbac/checkaccessreqhelper.go @@ -634,6 +634,16 @@ func defaultDir(s string) string { return "-" // invalid for a namespace } +// Cache key construction for non-resource requests. cacheKeyFieldSeparator is a +// NUL byte, which cannot legally appear in a URL path, a Kubernetes verb, or an +// AAD user name, so it unambiguously separates the fields and guarantees a +// non-resource key can never equal a resource key (which is "/"-joined and NUL +// free). nonResourceCacheKeyPrefix names that disjoint namespace. +const ( + nonResourceCacheKeyPrefix = "nonresource" + cacheKeyFieldSeparator = "\x00" +) + func getResultCacheKey(subRevReq *authzv1.SubjectAccessReviewSpec, allowSubresourceTypeCheck bool) string { cacheKey := subRevReq.User @@ -650,7 +660,18 @@ func getResultCacheKey(subRevReq *authzv1.SubjectAccessReviewSpec, allowSubresou } } } else if subRevReq.NonResourceAttributes != nil { - cacheKey = path.Join(cacheKey, subRevReq.NonResourceAttributes.Path, getActionName(subRevReq.NonResourceAttributes.Verb)) + // The non-resource path is fully caller-controlled and may contain ".." + // segments. It must NOT flow through path.Join/path.Clean, which would + // resolve ".." and let a path (e.g. "/apiz/../-/-/secrets") normalize onto + // the cache key of an unrelated resource request, so a decision cached for + // one request could be served for a different one (MSRC 132991). Join the + // fields with a NUL separator into a namespace disjoint from resource keys. + cacheKey = strings.Join([]string{ + nonResourceCacheKeyPrefix, + subRevReq.User, + subRevReq.NonResourceAttributes.Path, + getActionName(subRevReq.NonResourceAttributes.Verb), + }, cacheKeyFieldSeparator) } return cacheKey diff --git a/authz/providers/azure/rbac/checkaccessreqhelper_test.go b/authz/providers/azure/rbac/checkaccessreqhelper_test.go index 36864b3c2..aa300a2ae 100644 --- a/authz/providers/azure/rbac/checkaccessreqhelper_test.go +++ b/authz/providers/azure/rbac/checkaccessreqhelper_test.go @@ -1354,7 +1354,7 @@ func Test_getResultCacheKey(t *testing.T) { }, allowSubresourceTypeCheck: false, }, - "charlie@yahoo.com/apis/v1/read", + strings.Join([]string{nonResourceCacheKeyPrefix, "charlie@yahoo.com", "/apis/v1", "read"}, cacheKeyFieldSeparator), }, { @@ -1366,7 +1366,7 @@ func Test_getResultCacheKey(t *testing.T) { }, allowSubresourceTypeCheck: false, }, - "echo@outlook.com/logs/read", + strings.Join([]string{nonResourceCacheKeyPrefix, "echo@outlook.com", "/logs", "read"}, cacheKeyFieldSeparator), }, { @@ -1499,6 +1499,53 @@ func Test_getResultCacheKey(t *testing.T) { } } +// Test_getResultCacheKey_noResourceNonResourceCollision is the regression test +// for MSRC 132991. A non-resource path whose ".." segments normalize down to a +// resource's path (e.g. "/apiz/../-/-/secrets") must NOT produce the same cache +// key as the corresponding resource request (a cluster-wide list of secrets), so +// a decision cached for one request is never served for a different one. +func Test_getResultCacheKey_noResourceNonResourceCollision(t *testing.T) { + const user = "eve@contoso.com" + + // getActionName maps get/list/watch all to "read", and the object name and + // ResourceRequest flag are not part of the key, so these share one key. + resourceVariants := []*authzv1.SubjectAccessReviewSpec{ + {User: user, ResourceAttributes: &authzv1.ResourceAttributes{Resource: "secrets", Verb: "list"}}, + {User: user, ResourceAttributes: &authzv1.ResourceAttributes{Resource: "secrets", Verb: "get"}}, + {User: user, ResourceAttributes: &authzv1.ResourceAttributes{Resource: "secrets", Verb: "watch"}}, + } + + // Non-resource paths that path.Clean would normalize onto the secrets key. + craftedPaths := []string{ + "/apiz/../-/-/secrets", + "/api/../-/-/secrets", + "/healthzz/../-/-/secrets", + "/openapiz/../-/-/secrets", + } + + for _, allowSubresourceTypeCheck := range []bool{false, true} { + resourceKeys := make(map[string]struct{}) + for _, r := range resourceVariants { + resourceKeys[getResultCacheKey(r, allowSubresourceTypeCheck)] = struct{}{} + } + + for _, p := range craftedPaths { + nonRes := &authzv1.SubjectAccessReviewSpec{ + User: user, + NonResourceAttributes: &authzv1.NonResourceAttributes{Path: p, Verb: "get"}, + } + got := getResultCacheKey(nonRes, allowSubresourceTypeCheck) + if _, collides := resourceKeys[got]; collides { + t.Errorf("non-resource path %q (allowSubresourceTypeCheck=%v) collides with a secrets resource cache key: %q", + p, allowSubresourceTypeCheck, got) + } + if !strings.Contains(got, cacheKeyFieldSeparator) { + t.Errorf("non-resource cache key %q for path %q must use the NUL field separator", got, p) + } + } + } +} + func Test_buildCheckAccessURL(t *testing.T) { mustCreateURL := func(rawURL string) url.URL { t.Helper() diff --git a/authz/providers/azure/rbac/rbac.go b/authz/providers/azure/rbac/rbac.go index 78fd4709d..8ff634bfb 100644 --- a/authz/providers/azure/rbac/rbac.go +++ b/authz/providers/azure/rbac/rbac.go @@ -353,16 +353,44 @@ func (a *AccessInfo) SetResultInCache(ctx context.Context, request *authzv1.Subj return store.Set(key, result) } -func (a *AccessInfo) AllowNonResPathDiscoveryAccess(request *authzv1.SubjectAccessReviewSpec) bool { - if request.NonResourceAttributes != nil && a.allowNonResDiscoveryPathAccess && strings.EqualFold(request.NonResourceAttributes.Verb, "get") { - path := strings.ToLower(request.NonResourceAttributes.Path) - if strings.HasPrefix(path, "/api") || strings.HasPrefix(path, "/openapi") || strings.HasPrefix(path, "/version") || strings.HasPrefix(path, "/healthz") { +// discoveryPathRoots are the non-resource path roots that Guard may authorize +// for any authenticated user without an Azure RBAC check. A request path counts +// as discovery only when it equals one of these roots exactly or is nested +// beneath it on a path-segment boundary (root + "/..."). Loose prefix matching +// (for example "/apiz" or "/healthzz") must never be treated as discovery. +var discoveryPathRoots = []string{"/api", "/apis", "/openapi", "/version", "/healthz"} + +// isNonResourceDiscoveryPath reports whether the lowercased non-resource path is +// one of the well-known Kubernetes discovery or health endpoints. It rejects any +// path containing a ".." traversal segment so a path cannot match on a loose +// prefix and, once path.Clean normalizes it elsewhere, share the cache key of an +// unrelated resource request (see getResultCacheKey). Matching on an exact root +// or a path-segment boundary keeps the discovery exemption scoped to real +// discovery endpoints (MSRC 132991). +func isNonResourceDiscoveryPath(lowerPath string) bool { + if lowerPath == "" { + return false + } + for _, segment := range strings.Split(lowerPath, "/") { + if segment == ".." { + return false + } + } + for _, root := range discoveryPathRoots { + if lowerPath == root || strings.HasPrefix(lowerPath, root+"/") { return true } } return false } +func (a *AccessInfo) AllowNonResPathDiscoveryAccess(request *authzv1.SubjectAccessReviewSpec) bool { + if request.NonResourceAttributes != nil && a.allowNonResDiscoveryPathAccess && strings.EqualFold(request.NonResourceAttributes.Verb, "get") { + return isNonResourceDiscoveryPath(strings.ToLower(request.NonResourceAttributes.Path)) + } + return false +} + func (a *AccessInfo) setReqHeaders(req *http.Request) { a.lock.RLock() defer a.lock.RUnlock() diff --git a/authz/providers/azure/rbac/rbac_test.go b/authz/providers/azure/rbac/rbac_test.go index 8499a7d59..4d04fcd31 100644 --- a/authz/providers/azure/rbac/rbac_test.go +++ b/authz/providers/azure/rbac/rbac_test.go @@ -1003,3 +1003,62 @@ type capturedCheckAccess struct { resourceID string actions []azureutils.AuthorizationActionInfo } + +// Test_AllowNonResPathDiscoveryAccess is the regression test for the discovery +// half of MSRC 132991. The discovery exemption (which returns ALLOW with no +// Azure RBAC check) must apply only to the well-known discovery/health endpoints +// on a path-segment boundary, and must reject loose-prefix look-alikes and any +// path containing a ".." traversal segment. +func Test_AllowNonResPathDiscoveryAccess(t *testing.T) { + tests := []struct { + name string + path string + verb string + allowDiscovery bool + nilNonResource bool + want bool + }{ + // Legitimate discovery / health endpoints - allowed. + {name: "api root", path: "/api", verb: "get", allowDiscovery: true, want: true}, + {name: "apis root", path: "/apis", verb: "get", allowDiscovery: true, want: true}, + {name: "core group version", path: "/api/v1", verb: "get", allowDiscovery: true, want: true}, + {name: "named group version", path: "/apis/apps/v1", verb: "get", allowDiscovery: true, want: true}, + {name: "openapi v2", path: "/openapi/v2", verb: "get", allowDiscovery: true, want: true}, + {name: "openapi v3", path: "/openapi/v3", verb: "get", allowDiscovery: true, want: true}, + {name: "version", path: "/version", verb: "get", allowDiscovery: true, want: true}, + {name: "healthz", path: "/healthz", verb: "get", allowDiscovery: true, want: true}, + {name: "healthz subpath", path: "/healthz/ping", verb: "get", allowDiscovery: true, want: true}, + {name: "uppercase healthz", path: "/HEALTHZ", verb: "GET", allowDiscovery: true, want: true}, + + // Loose-prefix look-alikes - must be rejected (the core defect). + {name: "apiz lookalike", path: "/apiz", verb: "get", allowDiscovery: true, want: false}, + {name: "healthzz lookalike", path: "/healthzz", verb: "get", allowDiscovery: true, want: false}, + {name: "openapiz lookalike", path: "/openapiz", verb: "get", allowDiscovery: true, want: false}, + {name: "versionz lookalike", path: "/versionz", verb: "get", allowDiscovery: true, want: false}, + + // Path-traversal crafts - must be rejected. + {name: "apiz traversal to secrets", path: "/apiz/../-/-/secrets", verb: "get", allowDiscovery: true, want: false}, + {name: "api traversal to secrets", path: "/api/../-/-/secrets", verb: "get", allowDiscovery: true, want: false}, + {name: "healthz traversal", path: "/healthz/../-/-/secrets", verb: "get", allowDiscovery: true, want: false}, + + // Non-discovery resources and other guards. + {name: "unrelated path", path: "/logs", verb: "get", allowDiscovery: true, want: false}, + {name: "empty path", path: "", verb: "get", allowDiscovery: true, want: false}, + {name: "non-get verb", path: "/api", verb: "list", allowDiscovery: true, want: false}, + {name: "discovery disabled", path: "/api", verb: "get", allowDiscovery: false, want: false}, + {name: "nil non-resource attrs", nilNonResource: true, allowDiscovery: true, want: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + a := &AccessInfo{allowNonResDiscoveryPathAccess: tt.allowDiscovery} + req := &authzv1.SubjectAccessReviewSpec{User: "eve@contoso.com"} + if !tt.nilNonResource { + req.NonResourceAttributes = &authzv1.NonResourceAttributes{Path: tt.path, Verb: tt.verb} + } + if got := a.AllowNonResPathDiscoveryAccess(req); got != tt.want { + t.Errorf("AllowNonResPathDiscoveryAccess(path=%q, verb=%q) = %v, want %v", tt.path, tt.verb, got, tt.want) + } + }) + } +} From 1d3e4c170dc5e11d47d0baf289040afa49b6d4d3 Mon Sep 17 00:00:00 2001 From: Artem Kolomeetc Date: Wed, 12 Aug 2026 00:45:02 -0700 Subject: [PATCH 2/3] fix(authz): match discovery paths against the upstream system:discovery rule Resolved comments: - Comment #1 by @enj: prefix matching is only correct for /api/*, /apis/* and /openapi/*; the remaining discovery URLs must match exactly. Changes: - rbac.go: replace the single discoveryPathRoots prefix list with an exact-match set (/api, /apis, /healthz, /livez, /openapi, /readyz, /version, /version/) and a prefix list (/api/, /apis/, /openapi/), reproducing the non-resource URLs of the upstream Kubernetes system:discovery ClusterRole and its "*" semantics from rbacv1.NonResourceURLMatches. - rbac.go: /healthz/... and /version/... are no longer exempt; /livez and /readyz are now exempt, matching upstream. A non-discovery path is not denied, it falls through to the regular Azure RBAC check. - rbac_test.go: cover all 11 upstream entries, the subpath and lookalike negatives, and the traversal rejections. Signed-off-by: Artem Kolomeetc --- authz/providers/azure/rbac/rbac.go | 52 +++++++++++++++++-------- authz/providers/azure/rbac/rbac_test.go | 37 +++++++++++++----- 2 files changed, 64 insertions(+), 25 deletions(-) diff --git a/authz/providers/azure/rbac/rbac.go b/authz/providers/azure/rbac/rbac.go index 8ff634bfb..4873dca3d 100644 --- a/authz/providers/azure/rbac/rbac.go +++ b/authz/providers/azure/rbac/rbac.go @@ -353,20 +353,37 @@ func (a *AccessInfo) SetResultInCache(ctx context.Context, request *authzv1.Subj return store.Set(key, result) } -// discoveryPathRoots are the non-resource path roots that Guard may authorize -// for any authenticated user without an Azure RBAC check. A request path counts -// as discovery only when it equals one of these roots exactly or is nested -// beneath it on a path-segment boundary (root + "/..."). Loose prefix matching -// (for example "/apiz" or "/healthzz") must never be treated as discovery. -var discoveryPathRoots = []string{"/api", "/apis", "/openapi", "/version", "/healthz"} - -// isNonResourceDiscoveryPath reports whether the lowercased non-resource path is -// one of the well-known Kubernetes discovery or health endpoints. It rejects any -// path containing a ".." traversal segment so a path cannot match on a loose -// prefix and, once path.Clean normalizes it elsewhere, share the cache key of an -// unrelated resource request (see getResultCacheKey). Matching on an exact root -// or a path-segment boundary keeps the discovery exemption scoped to real -// discovery endpoints (MSRC 132991). +// discoveryExactPaths and discoveryPrefixPaths together reproduce the non-resource +// URLs of the upstream Kubernetes "system:discovery" ClusterRole +// (plugin/pkg/auth/authorizer/rbac/bootstrappolicy/policy.go), which upstream binds +// to the system:authenticated group. Upstream evaluates a rule URL ending in "*" as +// a prefix match with the "*" trimmed and every other rule URL as an exact string +// match (rbacv1.NonResourceURLMatches), so "/api/*" contributes the "/api/" prefix +// while "/healthz" and "/version" match only themselves. Guard must not exempt +// anything outside this set, so paths such as "/healthz/etcd" or "/apiz" get a +// regular Azure RBAC check instead (MSRC 132991). +var discoveryExactPaths = map[string]struct{}{ + "/api": {}, + "/apis": {}, + "/healthz": {}, + "/livez": {}, + "/openapi": {}, + "/readyz": {}, + "/version": {}, + "/version/": {}, +} + +// discoveryPrefixPaths are the upstream "/api/*", "/apis/*" and "/openapi/*" rules +// with the trailing "*" trimmed, matched as prefixes exactly as upstream does. +var discoveryPrefixPaths = []string{"/api/", "/apis/", "/openapi/"} + +// isNonResourceDiscoveryPath reports whether the lowercased non-resource path is one +// of the discovery endpoints granted by the upstream "system:discovery" ClusterRole. +// Guard is deliberately stricter than upstream on one point: a path containing a ".." +// traversal segment is never treated as discovery, so it cannot match a prefix rule +// here and, once path.Clean normalizes it elsewhere, share the cache key of an +// unrelated resource request (see getResultCacheKey). Reporting false is not a denial; +// the request falls through to the regular Azure RBAC check (MSRC 132991). func isNonResourceDiscoveryPath(lowerPath string) bool { if lowerPath == "" { return false @@ -376,8 +393,11 @@ func isNonResourceDiscoveryPath(lowerPath string) bool { return false } } - for _, root := range discoveryPathRoots { - if lowerPath == root || strings.HasPrefix(lowerPath, root+"/") { + if _, ok := discoveryExactPaths[lowerPath]; ok { + return true + } + for _, prefix := range discoveryPrefixPaths { + if strings.HasPrefix(lowerPath, prefix) { return true } } diff --git a/authz/providers/azure/rbac/rbac_test.go b/authz/providers/azure/rbac/rbac_test.go index 4d04fcd31..8ac5df637 100644 --- a/authz/providers/azure/rbac/rbac_test.go +++ b/authz/providers/azure/rbac/rbac_test.go @@ -1005,10 +1005,13 @@ type capturedCheckAccess struct { } // Test_AllowNonResPathDiscoveryAccess is the regression test for the discovery -// half of MSRC 132991. The discovery exemption (which returns ALLOW with no -// Azure RBAC check) must apply only to the well-known discovery/health endpoints -// on a path-segment boundary, and must reject loose-prefix look-alikes and any -// path containing a ".." traversal segment. +// half of MSRC 132991. The discovery exemption (which returns ALLOW with no Azure +// RBAC check) must cover exactly the non-resource URLs of the upstream Kubernetes +// "system:discovery" ClusterRole - "/api", "/api/*", "/apis", "/apis/*", +// "/healthz", "/livez", "/openapi", "/openapi/*", "/readyz", "/version" and +// "/version/" - where only the "*" entries match by prefix and the rest match +// exactly. Subpaths of the exact-match entries, loose-prefix look-alikes and any +// path containing a ".." traversal segment must not be exempted. func Test_AllowNonResPathDiscoveryAccess(t *testing.T) { tests := []struct { name string @@ -1018,22 +1021,37 @@ func Test_AllowNonResPathDiscoveryAccess(t *testing.T) { nilNonResource bool want bool }{ - // Legitimate discovery / health endpoints - allowed. + // Every entry of the upstream system:discovery rule - allowed. The "/api/*", + // "/apis/*" and "/openapi/*" entries are exercised through real subpaths. {name: "api root", path: "/api", verb: "get", allowDiscovery: true, want: true}, - {name: "apis root", path: "/apis", verb: "get", allowDiscovery: true, want: true}, {name: "core group version", path: "/api/v1", verb: "get", allowDiscovery: true, want: true}, + {name: "apis root", path: "/apis", verb: "get", allowDiscovery: true, want: true}, {name: "named group version", path: "/apis/apps/v1", verb: "get", allowDiscovery: true, want: true}, + {name: "healthz", path: "/healthz", verb: "get", allowDiscovery: true, want: true}, + {name: "livez", path: "/livez", verb: "get", allowDiscovery: true, want: true}, + {name: "openapi root", path: "/openapi", verb: "get", allowDiscovery: true, want: true}, {name: "openapi v2", path: "/openapi/v2", verb: "get", allowDiscovery: true, want: true}, {name: "openapi v3", path: "/openapi/v3", verb: "get", allowDiscovery: true, want: true}, + {name: "readyz", path: "/readyz", verb: "get", allowDiscovery: true, want: true}, {name: "version", path: "/version", verb: "get", allowDiscovery: true, want: true}, - {name: "healthz", path: "/healthz", verb: "get", allowDiscovery: true, want: true}, - {name: "healthz subpath", path: "/healthz/ping", verb: "get", allowDiscovery: true, want: true}, + {name: "version trailing slash", path: "/version/", verb: "get", allowDiscovery: true, want: true}, {name: "uppercase healthz", path: "/HEALTHZ", verb: "GET", allowDiscovery: true, want: true}, - // Loose-prefix look-alikes - must be rejected (the core defect). + // Subpaths of the exact-match entries. Upstream grants only the bare + // endpoint, so these real apiserver subpaths are not discovery. + {name: "healthz etcd subpath", path: "/healthz/etcd", verb: "get", allowDiscovery: true, want: false}, + {name: "healthz ping subpath", path: "/healthz/ping", verb: "get", allowDiscovery: true, want: false}, + {name: "livez poststarthook subpath", path: "/livez/poststarthook/start-apiserver-admission-initializer", verb: "get", allowDiscovery: true, want: false}, + {name: "readyz shutdown subpath", path: "/readyz/shutdown", verb: "get", allowDiscovery: true, want: false}, + {name: "version subpath", path: "/version/foo", verb: "get", allowDiscovery: true, want: false}, + + // Loose-prefix look-alikes - must be rejected. {name: "apiz lookalike", path: "/apiz", verb: "get", allowDiscovery: true, want: false}, {name: "healthzz lookalike", path: "/healthzz", verb: "get", allowDiscovery: true, want: false}, + {name: "livezz lookalike", path: "/livezz", verb: "get", allowDiscovery: true, want: false}, {name: "openapiz lookalike", path: "/openapiz", verb: "get", allowDiscovery: true, want: false}, + {name: "readyzz lookalike", path: "/readyzz", verb: "get", allowDiscovery: true, want: false}, + {name: "versionx lookalike", path: "/versionx", verb: "get", allowDiscovery: true, want: false}, {name: "versionz lookalike", path: "/versionz", verb: "get", allowDiscovery: true, want: false}, // Path-traversal crafts - must be rejected. @@ -1043,6 +1061,7 @@ func Test_AllowNonResPathDiscoveryAccess(t *testing.T) { // Non-discovery resources and other guards. {name: "unrelated path", path: "/logs", verb: "get", allowDiscovery: true, want: false}, + {name: "metrics path", path: "/metrics", verb: "get", allowDiscovery: true, want: false}, {name: "empty path", path: "", verb: "get", allowDiscovery: true, want: false}, {name: "non-get verb", path: "/api", verb: "list", allowDiscovery: true, want: false}, {name: "discovery disabled", path: "/api", verb: "get", allowDiscovery: false, want: false}, From a566a961433ced315f254682a8979d30157de9c4 Mon Sep 17 00:00:00 2001 From: Artem Kolomeetc Date: Wed, 12 Aug 2026 01:02:23 -0700 Subject: [PATCH 3/3] fix(authz): build the result cache key with a length-prefixed hash Resolved comments: - Comment #2 by @enj: follow buildKey from k8s.io/apiserver/pkg/endpoints/filters/impersonation/cache.go. Changes: - checkaccessreqhelper.go: replace the path.Join / separator-joined key with a cacheKeyBuilder that writes a one-byte request-shape discriminator followed by every field under a uint32 big-endian length prefix, hashes the result with SHA-256 and appends the un-hashed user name. No field value can shift a field boundary, so two requests share a key only when every field is equal, and a digest collision is confined to one user's own keys. - checkaccessreqhelper.go: drop defaultDir, nonResourceCacheKeyPrefix and cacheKeyFieldSeparator. The "-" placeholder made namespace "" and namespace "-" collide (same for group); length prefixing makes them distinct. - checkaccessreqhelper.go: the field set is unchanged, so cache hit rates are unaffected; the key still uses the derived action, not the raw verb. - checkaccessreqhelper_test.go: replace literal-key assertions with invariant tests for distinctness, determinism, user namespacing and the preserved get/list/watch sharing. - rbac.go: correct the isNonResourceDiscoveryPath comment, which justified the ".." rejection by a path.Clean cache-key collision that no longer exists. Signed-off-by: Artem Kolomeetc --- .../azure/rbac/checkaccessreqhelper.go | 136 +++++--- .../azure/rbac/checkaccessreqhelper_test.go | 296 +++++++++--------- authz/providers/azure/rbac/rbac.go | 9 +- 3 files changed, 255 insertions(+), 186 deletions(-) diff --git a/authz/providers/azure/rbac/checkaccessreqhelper.go b/authz/providers/azure/rbac/checkaccessreqhelper.go index 54a602095..79169d40d 100644 --- a/authz/providers/azure/rbac/checkaccessreqhelper.go +++ b/authz/providers/azure/rbac/checkaccessreqhelper.go @@ -18,6 +18,9 @@ package rbac import ( "context" + "crypto/sha256" + "encoding/binary" + "encoding/hex" "encoding/json" "errors" "fmt" @@ -627,54 +630,103 @@ func createAuthorizationActionInfoList(filteredOperations azureutils.OperationsM return authInfos, nil } -func defaultDir(s string) string { - if s != "" { - return s - } - return "-" // invalid for a namespace -} +// cacheKeyShape identifies which attribute set a SubjectAccessReviewSpec carries. +// It is encoded as a single fixed-width byte at the head of every cache key, which +// keeps the three shapes in disjoint key spaces no matter how many fields each +// branch happens to encode. Upstream uses a single bool for its two shapes; a bool +// cannot express the third shape, a spec carrying neither attribute set. +type cacheKeyShape byte -// Cache key construction for non-resource requests. cacheKeyFieldSeparator is a -// NUL byte, which cannot legally appear in a URL path, a Kubernetes verb, or an -// AAD user name, so it unambiguously separates the fields and guarantees a -// non-resource key can never equal a resource key (which is "/"-joined and NUL -// free). nonResourceCacheKeyPrefix names that disjoint namespace. const ( - nonResourceCacheKeyPrefix = "nonresource" - cacheKeyFieldSeparator = "\x00" + cacheKeyShapeNeither cacheKeyShape = 0 + cacheKeyShapeResource cacheKeyShape = 1 + cacheKeyShapeNonResource cacheKeyShape = 2 ) +// cacheKeyBuilder encodes the fields that identify a CheckAccess result into an +// unambiguous byte string and hashes it. The layout follows buildKey in upstream +// k8s.io/apiserver/pkg/endpoints/filters/impersonation/cache.go. +// +// Every variable-length field carries a uint32 big-endian length prefix, so no +// field value can move a field boundary: two requests share an encoding only when +// every field is byte-for-byte equal. Joining caller-influenced fields with a +// separator instead leaves the boundaries ambiguous, because a field may contain +// the separator, may be rewritten by normalization (path.Clean resolving ".."), or +// may be replaced by a placeholder that another field can also spell (MSRC 132991). +// +// build appends the requestor's user name to the hash in clear text. The hash is +// always 64 characters, so the suffix boundary is fixed and two keys are equal only +// if their users are equal. The user name is set by the authenticator rather than +// chosen by the caller, so a hash collision is only ever reachable among a single +// user's own keys, where it cannot yield a permission that user does not already +// hold. Hashing also bounds the key length regardless of caller-supplied input. +type cacheKeyBuilder struct { + user string + builder []byte +} + +// newCacheKeyBuilder starts a key for user and shape. It writes the shape byte and +// the user name first so that ordering is identical for every shape. +func newCacheKeyBuilder(user string, shape cacheKeyShape) *cacheKeyBuilder { + c := &cacheKeyBuilder{user: user, builder: make([]byte, 0, 256)} + c.builder = append(c.builder, byte(shape)) + c.addString(user) + return c +} + +// addString appends value with a uint32 big-endian length prefix. +func (c *cacheKeyBuilder) addString(value string) { + var length [4]byte + binary.BigEndian.PutUint32(length[:], uint32(len(value))) + c.builder = append(c.builder, length[:]...) + c.builder = append(c.builder, value...) +} + +// build returns the hex-encoded digest of the accumulated fields, suffixed with +// the un-hashed user name. +func (c *cacheKeyBuilder) build() string { + hashed := sha256.Sum256(c.builder) + return hex.EncodeToString(hashed[:]) + "/" + c.user +} + +// cachedSubresource returns the subresource that takes part in the cache key. Only +// subresources carried as a distinct DataAction attribute are cached separately; +// every other subresource shares the base resource's entry. +func cachedSubresource(attr *authzv1.ResourceAttributes, allowSubresourceTypeCheck bool) string { + if allowSubresourceTypeCheck && shouldHandleSubresource(attr.Resource, attr.Subresource) { + return attr.Subresource + } + return "" +} + +// getResultCacheKey returns the key under which the CheckAccess result for +// subRevReq is cached. See cacheKeyBuilder for why the encoding is length-prefixed +// and hashed rather than joined. +// +// The set of fields is unchanged, so cache hit rates are unaffected: the resource +// branch keys on the derived action from getResourceAndAction, which maps get, list +// and watch onto "read", and the non-resource branch keys on getActionName. func getResultCacheKey(subRevReq *authzv1.SubjectAccessReviewSpec, allowSubresourceTypeCheck bool) string { - cacheKey := subRevReq.User + switch { + case subRevReq.ResourceAttributes != nil: + attr := subRevReq.ResourceAttributes + key := newCacheKeyBuilder(subRevReq.User, cacheKeyShapeResource) + key.addString(attr.Namespace) + key.addString(attr.Group) + key.addString(getResourceAndAction(attr.Resource, attr.Subresource, attr.Verb)) + key.addString(cachedSubresource(attr, allowSubresourceTypeCheck)) + return key.build() + + case subRevReq.NonResourceAttributes != nil: + attr := subRevReq.NonResourceAttributes + key := newCacheKeyBuilder(subRevReq.User, cacheKeyShapeNonResource) + key.addString(attr.Path) + key.addString(getActionName(attr.Verb)) + return key.build() - if subRevReq.ResourceAttributes != nil { - cacheKey = path.Join(cacheKey, defaultDir(subRevReq.ResourceAttributes.Namespace)) - cacheKey = path.Join(cacheKey, defaultDir(subRevReq.ResourceAttributes.Group)) - action := getResourceAndAction(subRevReq.ResourceAttributes.Resource, subRevReq.ResourceAttributes.Subresource, subRevReq.ResourceAttributes.Verb) - cacheKey = path.Join(cacheKey, action) - - // Cache results for subresources of interest separately - if allowSubresourceTypeCheck { - if shouldHandleSubresource(subRevReq.ResourceAttributes.Resource, subRevReq.ResourceAttributes.Subresource) { - cacheKey = path.Join(cacheKey, subRevReq.ResourceAttributes.Subresource) - } - } - } else if subRevReq.NonResourceAttributes != nil { - // The non-resource path is fully caller-controlled and may contain ".." - // segments. It must NOT flow through path.Join/path.Clean, which would - // resolve ".." and let a path (e.g. "/apiz/../-/-/secrets") normalize onto - // the cache key of an unrelated resource request, so a decision cached for - // one request could be served for a different one (MSRC 132991). Join the - // fields with a NUL separator into a namespace disjoint from resource keys. - cacheKey = strings.Join([]string{ - nonResourceCacheKeyPrefix, - subRevReq.User, - subRevReq.NonResourceAttributes.Path, - getActionName(subRevReq.NonResourceAttributes.Verb), - }, cacheKeyFieldSeparator) - } - - return cacheKey + default: + return newCacheKeyBuilder(subRevReq.User, cacheKeyShapeNeither).build() + } } func prepareCheckAccessRequestBody(ctx context.Context, req *authzv1.SubjectAccessReviewSpec, clusterType string, resourceId string, useNamespaceResourceScopeFormat bool, allowCustomResourceTypeCheck bool, allowSubresourceTypeCheck bool) ([]*CheckAccessRequest, error) { diff --git a/authz/providers/azure/rbac/checkaccessreqhelper_test.go b/authz/providers/azure/rbac/checkaccessreqhelper_test.go index aa300a2ae..dcefa6734 100644 --- a/authz/providers/azure/rbac/checkaccessreqhelper_test.go +++ b/authz/providers/azure/rbac/checkaccessreqhelper_test.go @@ -23,6 +23,7 @@ import ( "net/http" "net/url" "reflect" + "regexp" "strings" "testing" @@ -1335,170 +1336,181 @@ func Test_prepareCheckAccessRequestBodyWithSubresourceDisabled(t *testing.T) { } } -func Test_getResultCacheKey(t *testing.T) { - type args struct { - subRevReq *authzv1.SubjectAccessReviewSpec - allowSubresourceTypeCheck bool - } - tests := []struct { - name string - args args - want string - }{ - { - aksClusterType, - args{ - subRevReq: &authzv1.SubjectAccessReviewSpec{ - User: "charlie@yahoo.com", - NonResourceAttributes: &authzv1.NonResourceAttributes{Path: "/apis/v1", Verb: "list"}, - }, - allowSubresourceTypeCheck: false, - }, - strings.Join([]string{nonResourceCacheKeyPrefix, "charlie@yahoo.com", "/apis/v1", "read"}, cacheKeyFieldSeparator), - }, +// Cache keys are SHA-256 digests, so the tests below assert invariants rather +// than literal key strings: asserting a hardcoded digest would only restate the +// implementation. - { - aksClusterType, - args{ - subRevReq: &authzv1.SubjectAccessReviewSpec{ - User: "echo@outlook.com", - NonResourceAttributes: &authzv1.NonResourceAttributes{Path: "/logs", Verb: "get"}, - }, - allowSubresourceTypeCheck: false, - }, - strings.Join([]string{nonResourceCacheKeyPrefix, "echo@outlook.com", "/logs", "read"}, cacheKeyFieldSeparator), - }, +const ( + cacheKeyTestUser = "alpha@bing.com" + cacheKeyTestGroup = "apps" +) - { - aksClusterType, - args{ - subRevReq: &authzv1.SubjectAccessReviewSpec{ - User: "alpha@bing.com", - ResourceAttributes: &authzv1.ResourceAttributes{ - Namespace: "dev", Group: "", Resource: "pods", - Subresource: "status", Version: "v1", Name: "test", Verb: "delete", - }, - }, - allowSubresourceTypeCheck: false, - }, - "alpha@bing.com/dev/-/pods/delete", - }, +// cacheKeyDigestPattern matches the hashed portion of a cache key. +var cacheKeyDigestPattern = regexp.MustCompile(`^[0-9a-f]{64}$`) - { - aksClusterType, - args{ - subRevReq: &authzv1.SubjectAccessReviewSpec{ - User: "alpha@bing.com", - ResourceAttributes: &authzv1.ResourceAttributes{ - Namespace: "dev", Group: "", Resource: "pods", - Subresource: "status", Version: "v1", Name: "test", Verb: "delete", - }, - }, - allowSubresourceTypeCheck: true, - }, - "alpha@bing.com/dev/-/pods/delete", - }, +// cacheKeyCase is one input to getResultCacheKey. +type cacheKeyCase struct { + name string + subRevReq *authzv1.SubjectAccessReviewSpec + allowSubresourceTypeCheck bool +} - { - aksClusterType, - args{ - subRevReq: &authzv1.SubjectAccessReviewSpec{ - User: "alpha@bing.com", - ResourceAttributes: &authzv1.ResourceAttributes{ - Namespace: "dev", Group: "", Resource: "pods", - Subresource: "logs", Version: "v1", Name: "test", Verb: "get", - }, +// cacheKeyDistinctCases returns requests whose cache keys must all differ. It +// covers the field encodings that a joined key rendered ambiguous: the "-" +// placeholder that an empty namespace or group used to be mapped onto, a user +// name carrying the "/" separator, and a namespace/group pair whose boundary can +// be shifted without changing their concatenation. +func cacheKeyDistinctCases() []cacheKeyCase { + resourceCase := func(name, user, namespace, group string) cacheKeyCase { + return cacheKeyCase{ + name: name, + subRevReq: &authzv1.SubjectAccessReviewSpec{ + User: user, + ResourceAttributes: &authzv1.ResourceAttributes{ + Namespace: namespace, Group: group, Resource: "pods", Verb: "get", }, - allowSubresourceTypeCheck: false, }, - "alpha@bing.com/dev/-/pods/read", - }, + } + } - { - aksClusterType, - args{ - subRevReq: &authzv1.SubjectAccessReviewSpec{ - User: "alpha@bing.com", - ResourceAttributes: &authzv1.ResourceAttributes{ - Namespace: "dev", Group: "", Resource: "pods", - Subresource: "logs", Version: "v1", Name: "test", Verb: "get", - }, + subresourceCase := func(name string, allowSubresourceTypeCheck bool) cacheKeyCase { + return cacheKeyCase{ + name: name, + subRevReq: &authzv1.SubjectAccessReviewSpec{ + User: cacheKeyTestUser, + ResourceAttributes: &authzv1.ResourceAttributes{ + Namespace: "sub", Resource: "pods", Subresource: "logs", Verb: "get", }, - allowSubresourceTypeCheck: true, }, - "alpha@bing.com/dev/-/pods/read/logs", - }, + allowSubresourceTypeCheck: allowSubresourceTypeCheck, + } + } - { - "arc", - args{ - subRevReq: &authzv1.SubjectAccessReviewSpec{ - User: "beta@msn.com", - ResourceAttributes: &authzv1.ResourceAttributes{ - Namespace: "azure-arc", - Group: "authentication.k8s.io", Resource: "userextras", Subresource: "scopes", Version: "v1", - Name: "test", Verb: "impersonate", - }, + return []cacheKeyCase{ + resourceCase("empty namespace", cacheKeyTestUser, "", cacheKeyTestGroup), + resourceCase("namespace equal to the former empty placeholder", cacheKeyTestUser, "-", cacheKeyTestGroup), + resourceCase("empty group", cacheKeyTestUser, "dev", ""), + resourceCase("group equal to the former empty placeholder", cacheKeyTestUser, "dev", "-"), + resourceCase("namespace and group split as ab|c", cacheKeyTestUser, "ab", "c"), + resourceCase("namespace and group split as a|bc", cacheKeyTestUser, "a", "bc"), + resourceCase("user carrying the separator, short namespace", "a/b", "c", cacheKeyTestGroup), + resourceCase("user without the separator, long namespace", "a", "b/c", cacheKeyTestGroup), + subresourceCase("subresource check disabled", false), + subresourceCase("subresource check enabled", true), + { + name: "resource request", + subRevReq: &authzv1.SubjectAccessReviewSpec{ + User: cacheKeyTestUser, + ResourceAttributes: &authzv1.ResourceAttributes{ + Namespace: "shape", Group: cacheKeyTestGroup, Resource: "deployments", Verb: "get", }, - allowSubresourceTypeCheck: false, }, - "beta@msn.com/azure-arc/authentication.k8s.io/userextras/impersonate/action", }, - { - "arc", - args{ - subRevReq: &authzv1.SubjectAccessReviewSpec{ - User: "beta@msn.com", - ResourceAttributes: &authzv1.ResourceAttributes{ - Namespace: "", Group: "", Resource: "nodes", - Subresource: "scopes", Version: "v1", Name: "", Verb: "list", - }, - }, - allowSubresourceTypeCheck: false, + name: "non-resource request", + subRevReq: &authzv1.SubjectAccessReviewSpec{ + User: cacheKeyTestUser, + NonResourceAttributes: &authzv1.NonResourceAttributes{Path: "/healthz", Verb: "get"}, }, - "beta@msn.com/-/-/nodes/read", }, - { - "allStar", - args{ - subRevReq: &authzv1.SubjectAccessReviewSpec{ - User: "beta@msn.com", - ResourceAttributes: &authzv1.ResourceAttributes{ - Namespace: "", Group: "*", Resource: "*", - Subresource: "scopes", Version: "v1", Name: "", Verb: "*", - }, - }, - allowSubresourceTypeCheck: false, - }, - "beta@msn.com/-/*/*/*", + name: "neither resource nor non-resource attributes", + subRevReq: &authzv1.SubjectAccessReviewSpec{User: cacheKeyTestUser}, }, + } +} - { - "allStarNSscope", - args{ - subRevReq: &authzv1.SubjectAccessReviewSpec{ - User: "beta@msn.com", - ResourceAttributes: &authzv1.ResourceAttributes{ - Namespace: "dev", Group: "*", Resource: "*", - Subresource: "scopes", Version: "v1", Name: "", Verb: "*", - }, - }, - allowSubresourceTypeCheck: false, - }, - "beta@msn.com/dev/*/*/*", - }, +// Test_getResultCacheKey_distinctRequestsGetDistinctKeys asserts that requests +// which must not share a cached decision also do not share a cache key. +func Test_getResultCacheKey_distinctRequestsGetDistinctKeys(t *testing.T) { + cases := cacheKeyDistinctCases() + seen := make(map[string]string, len(cases)) + + for _, tt := range cases { + got := getResultCacheKey(tt.subRevReq, tt.allowSubresourceTypeCheck) + if previous, collides := seen[got]; collides { + t.Errorf("%q and %q share cache key %q", previous, tt.name, got) + continue + } + seen[got] = tt.name } - for _, tt := range tests { +} + +// Test_getResultCacheKey_isDeterministic asserts the key depends only on the +// request, so a cached decision stays reachable across calls. +func Test_getResultCacheKey_isDeterministic(t *testing.T) { + for _, tt := range cacheKeyDistinctCases() { + t.Run(tt.name, func(t *testing.T) { + want := getResultCacheKey(tt.subRevReq, tt.allowSubresourceTypeCheck) + for i := 0; i < 3; i++ { + if got := getResultCacheKey(tt.subRevReq, tt.allowSubresourceTypeCheck); got != want { + t.Errorf("getResultCacheKey() repeat %d = %q, want %q", i, got, want) + } + } + }) + } +} + +// Test_getResultCacheKey_isUserNamespaced asserts every key is a hex digest +// suffixed with the un-hashed user name. Because the digest is fixed width, the +// suffix boundary is unambiguous and keys of two different users can never be +// equal, which confines any digest collision to a single user's own keys. +func Test_getResultCacheKey_isUserNamespaced(t *testing.T) { + for _, tt := range cacheKeyDistinctCases() { t.Run(tt.name, func(t *testing.T) { - if got := getResultCacheKey(tt.args.subRevReq, tt.args.allowSubresourceTypeCheck); got != tt.want { - t.Errorf("getResultCacheKey() = %v, want %v", got, tt.want) + got := getResultCacheKey(tt.subRevReq, tt.allowSubresourceTypeCheck) + + suffix := "/" + tt.subRevReq.User + if !strings.HasSuffix(got, suffix) { + t.Fatalf("getResultCacheKey() = %q, want suffix %q", got, suffix) + } + + digest := strings.TrimSuffix(got, suffix) + if !cacheKeyDigestPattern.MatchString(digest) { + t.Errorf("getResultCacheKey() digest = %q, want 64 lowercase hex characters", digest) } }) } } +// Test_getResultCacheKey_readVerbsShareCacheKey pins the cache hit semantics that +// the key must preserve: get, list and watch all map to the "read" action and so +// share one entry. It fails if the key is ever derived from the raw verb. +func Test_getResultCacheKey_readVerbsShareCacheKey(t *testing.T) { + readVerbs := []string{"list", "watch"} + + for _, allowSubresourceTypeCheck := range []bool{false, true} { + resourceKey := func(verb string) string { + return getResultCacheKey(&authzv1.SubjectAccessReviewSpec{ + User: cacheKeyTestUser, + ResourceAttributes: &authzv1.ResourceAttributes{ + Namespace: "dev", Resource: "secrets", Verb: verb, + }, + }, allowSubresourceTypeCheck) + } + nonResourceKey := func(verb string) string { + return getResultCacheKey(&authzv1.SubjectAccessReviewSpec{ + User: cacheKeyTestUser, + NonResourceAttributes: &authzv1.NonResourceAttributes{Path: "/healthz", Verb: verb}, + }, allowSubresourceTypeCheck) + } + + for _, keyFor := range []func(string) string{resourceKey, nonResourceKey} { + want := keyFor("get") + for _, verb := range readVerbs { + if got := keyFor(verb); got != want { + t.Errorf("verb %q (allowSubresourceTypeCheck=%v) key = %q, want the get key %q", + verb, allowSubresourceTypeCheck, got, want) + } + } + if got := keyFor("delete"); got == want { + t.Errorf("verb delete (allowSubresourceTypeCheck=%v) must not share the read key %q", + allowSubresourceTypeCheck, want) + } + } + } +} + // Test_getResultCacheKey_noResourceNonResourceCollision is the regression test // for MSRC 132991. A non-resource path whose ".." segments normalize down to a // resource's path (e.g. "/apiz/../-/-/secrets") must NOT produce the same cache @@ -1529,6 +1541,7 @@ func Test_getResultCacheKey_noResourceNonResourceCollision(t *testing.T) { resourceKeys[getResultCacheKey(r, allowSubresourceTypeCheck)] = struct{}{} } + nonResourceKeys := make(map[string]string, len(craftedPaths)) for _, p := range craftedPaths { nonRes := &authzv1.SubjectAccessReviewSpec{ User: user, @@ -1539,9 +1552,12 @@ func Test_getResultCacheKey_noResourceNonResourceCollision(t *testing.T) { t.Errorf("non-resource path %q (allowSubresourceTypeCheck=%v) collides with a secrets resource cache key: %q", p, allowSubresourceTypeCheck, got) } - if !strings.Contains(got, cacheKeyFieldSeparator) { - t.Errorf("non-resource cache key %q for path %q must use the NUL field separator", got, p) + if previous, collides := nonResourceKeys[got]; collides { + t.Errorf("non-resource paths %q and %q (allowSubresourceTypeCheck=%v) share cache key %q", + previous, p, allowSubresourceTypeCheck, got) + continue } + nonResourceKeys[got] = p } } } diff --git a/authz/providers/azure/rbac/rbac.go b/authz/providers/azure/rbac/rbac.go index 4873dca3d..70dde8972 100644 --- a/authz/providers/azure/rbac/rbac.go +++ b/authz/providers/azure/rbac/rbac.go @@ -380,10 +380,11 @@ var discoveryPrefixPaths = []string{"/api/", "/apis/", "/openapi/"} // isNonResourceDiscoveryPath reports whether the lowercased non-resource path is one // of the discovery endpoints granted by the upstream "system:discovery" ClusterRole. // Guard is deliberately stricter than upstream on one point: a path containing a ".." -// traversal segment is never treated as discovery, so it cannot match a prefix rule -// here and, once path.Clean normalizes it elsewhere, share the cache key of an -// unrelated resource request (see getResultCacheKey). Reporting false is not a denial; -// the request falls through to the regular Azure RBAC check (MSRC 132991). +// traversal segment is never treated as discovery. nonResourceAttributes.path on a +// SelfSubjectAccessReview is fully caller-controlled and is never routed by the API +// server, so without this check a path such as "/api/../.." would match the "/api/" +// prefix rule and be exempted from the Azure RBAC check. Reporting false is not a +// denial; the request falls through to the regular Azure RBAC check (MSRC 132991). func isNonResourceDiscoveryPath(lowerPath string) bool { if lowerPath == "" { return false