diff --git a/authz/providers/azure/rbac/checkaccessreqhelper.go b/authz/providers/azure/rbac/checkaccessreqhelper.go index aea5e93c..79169d40 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,33 +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 +const ( + 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 { - cacheKey = path.Join(cacheKey, subRevReq.NonResourceAttributes.Path, getActionName(subRevReq.NonResourceAttributes.Verb)) + default: + return newCacheKeyBuilder(subRevReq.User, cacheKeyShapeNeither).build() } - - return cacheKey } 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 36864b3c..dcefa673 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,232 @@ 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, - }, - "charlie@yahoo.com/apis/v1/read", - }, +// 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, - }, - "echo@outlook.com/logs/read", - }, +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 +// 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{}{} + } + + nonResourceKeys := make(map[string]string, len(craftedPaths)) + 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 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 + } + } +} + 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 78fd4709..70dde897 100644 --- a/authz/providers/azure/rbac/rbac.go +++ b/authz/providers/azure/rbac/rbac.go @@ -353,16 +353,65 @@ 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") { +// 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. 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 + } + for _, segment := range strings.Split(lowerPath, "/") { + if segment == ".." { + return false + } + } + if _, ok := discoveryExactPaths[lowerPath]; ok { + return true + } + for _, prefix := range discoveryPrefixPaths { + if strings.HasPrefix(lowerPath, prefix) { 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 8499a7d5..8ac5df63 100644 --- a/authz/providers/azure/rbac/rbac_test.go +++ b/authz/providers/azure/rbac/rbac_test.go @@ -1003,3 +1003,81 @@ 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 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 + path string + verb string + allowDiscovery bool + nilNonResource bool + want bool + }{ + // 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: "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: "version trailing slash", path: "/version/", verb: "get", allowDiscovery: true, want: true}, + {name: "uppercase healthz", path: "/HEALTHZ", verb: "GET", allowDiscovery: true, want: true}, + + // 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. + {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: "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}, + {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) + } + }) + } +}