Align discovery path matching with system:discovery and rebuild the result cache key (MSRC 132991) - #24
Open
Artem Kolomeetc (arxhive) wants to merge 3 commits into
Open
Conversation
Mo Khan (enj)
suggested changes
Aug 11, 2026
Artem Kolomeetc (arxhive)
changed the title
added 3 commits
August 12, 2026 01:14
…che 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 <akolomeetc@microsoft.com>
…ry 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 <akolomeetc@microsoft.com>
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 <akolomeetc@microsoft.com>
Artem Kolomeetc (arxhive)
force-pushed
the
fix/msrc-132991-nonres-discovery-cache
branch
from
August 12, 2026 08:17
2e544aa to
a566a96
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Hardens how Guard authorizes and caches non-resource (discovery)
SubjectAccessReviewrequests, so a decision cached for one request cannot be served for a different one. Tracked under MSRC 132991.Both changes now follow the upstream Kubernetes construct they mirror, per review feedback.
Changes
1. Match discovery paths against the upstream
system:discoveryruleAllowNonResPathDiscoveryAccesspreviously matched withstrings.HasPrefix(path, "/api")and the same for/openapi,/version,/healthz, so non-discovery paths such as/apiz...were granted an ALLOW with no Azure RBAC check.Matching now reproduces the non-resource URLs of the upstream
system:discoveryClusterRole (plugin/pkg/auth/authorizer/rbac/bootstrappolicy/policy.go, bound tosystem:authenticated) together with howrbacv1.NonResourceURLMatchesevaluates them: an entry ending in*is a prefix with the*trimmed, every other entry is an exact string match./api,/apis,/healthz,/livez,/openapi,/readyz,/version,/version//api/,/apis/,/openapi/Behaviour changes against the previous revision:
/healthz/<anything>and/version/<anything>are no longer exempt, and/livezand/readyznow are, matching upstream. A path that is not discovery is not denied; it falls through to the regular Azure RBAC check.Guard is deliberately stricter than upstream on one point: a path containing a
..segment is never treated as discovery.nonResourceAttributes.pathon aSelfSubjectAccessReviewis fully caller-controlled and is never routed by the API server, so without the check/api/../..would match the/api/prefix rule.2. Build the result cache key with a length-prefixed hash
getResultCacheKeybuilt the key withpath.Join, whosepath.Cleanresolves.., and joined caller-influenced fields without unambiguous boundaries.It now follows
buildKeyfromk8s.io/apiserver/pkg/endpoints/filters/impersonation/cache.go: a fixed-width request-shape byte, then every field under auint32big-endian length prefix, hashed with SHA-256, with the un-hashed user name appended after the digest. No field value can shift a field boundary, so two requests share a key only when every field is byte equal, and because the digest is fixed width, two keys are equal only if their users are equal. Any digest collision is therefore confined to a single user's own keys, where it cannot yield a permission that user does not already hold.This closes collisions that a separator-based key left in place on the resource branch:
defaultDirmapped""onto"-", so namespace""and namespace"-"produced the same key. Same for group.defaultDiris deleted."a/b"with namespace"c"joined to the same string as user"a"with namespace"b/c".path.Cleanrewrote the key whenever any joined field contained... The key no longer usespath.Joinat all.Two deviations from upstream: no
unsafe(itstoStringsaves one allocation in an apiserver hot path; this runs once perSubjectAccessReview), and a three-state shape byte instead ofaddBool(IsResourceRequest), because a spec can carry neither attribute set.The field set is unchanged, so cache hit rates are unaffected: the key still uses the derived action, so
get,listandwatchcontinue to share one entry.Tests
Test_AllowNonResPathDiscoveryAccess- all 11 upstream entries allowed; subpaths of the exact-match entries (/healthz/etcd,/version/foo,/livez/poststarthook/...,/readyz/shutdown) rejected; loose-prefix look-alikes (/apiz,/healthzz,/livezz,/readyzz,/versionx) rejected;..traversal, verb and flag guards covered.Test_getResultCacheKey_distinctRequestsGetDistinctKeys- requests that must not share a decision do not share a key, including the""vs"-"and boundary-shift cases above.Test_getResultCacheKey_isDeterministic,Test_getResultCacheKey_isUserNamespaced- key depends only on the request, and is a 64 character digest suffixed with the user name.Test_getResultCacheKey_readVerbsShareCacheKey- pins the preservedget/list/watchsharing, so keying on the raw verb fails the build.Test_getResultCacheKey_noResourceNonResourceCollision- non-resource paths that would normalize onto thesecretsresource key never collide with it.Literal-key assertions were replaced with invariant tests, since asserting a hardcoded digest would only restate the implementation. The distinctness table was checked against both a prefix-less encoding and the previous
path.Joinencoding to confirm it discriminates.Validation
go build ./...- OKgo vet ./...- OKgofmt -lon all non-vendor Go files - cleangolangci-lint runinghcr.io/appscode/golang-dev:1.25viamake lint- 0 issuesgo test ./authz/... ./auth/providers/azure/graph/...- all pass