Repository navigation
auth: make IsService/IsUser/IsMachine see through PrincipalMux - #124
Merged
krockpot merged 1 commit intoApr 29, 2026
Merged
Conversation
The principal predicates auth.IsService and auth.IsUser previously checked only the *default* sub-principal of a knox.PrincipalMux. In multi-provider auth chains, the wrapped principal's default may be a different type than the one the caller is actually trying to detect, so any code path that gates on these predicates after auth (e.g. a custom ServiceKeyCreationAuthorizer doing `if !auth.IsService(principal)`) would incorrectly reject real requests even when a service principal was in the mux. Update the predicates to: 1. Fast-path direct type assertion against the raw principal types, then 2. Walk PrincipalMux.Principals() looking for any sub-principal of the target type. Also: - Add IsMachine for symmetry (it didn't previously exist). - Add knox.PrincipalMux.Principals() accessor so server/auth doesn't need to reach into unexported state. - Update existing TestPrincipalMuxUserOrService, which was encoding the old default-only behavior, to reflect the new "any sub-principal" semantics. Add new tests covering raw and mux'd principals across all three predicates plus the new Principals() accessor. Made-with: Cursor
krockpot
approved these changes
Apr 29, 2026
4 tasks done
krockpot
pushed a commit
that referenced
this pull request
May 6, 2026
…ipalMux (#125) * auth: make IsService/IsUser/IsMachine recursively unwrap nested PrincipalMux PR #124 made auth.IsService, auth.IsUser, and auth.IsMachine walk one level of knox.PrincipalMux, which fixed the common single-level case. In production the principal reaching a handler can be a nested PrincipalMux, however: the outer Authentication decorator (server/decorators.go) always wraps whatever a provider returns in a new PrincipalMux, and a composite provider (e.g. an account-aware provider that itself runs a per-account provider chain) returns a PrincipalMux of its own. The result is outerMux(innerMux(leaf)), and the one-level walk in PR #124 stops at innerMux -- which isn't a leaf -- and returns false. This breaks any code path that gates on the OSS Is* predicates after auth, including the postKeysHandler user-vs-service branch and any ServiceKeyCreationAuthorizer that does `if !auth.IsService(p) { reject }`. This change replaces the direct leaf type assertion inside the mux-walk branch with a recursive call to the predicate, so any depth of nesting resolves correctly. The fast-path leaf assertion is preserved for callers that already pass a raw *service/*user/*machine. Adds tests for two-level and three-level nesting, plus a mixed-leaves case where the outer mux holds a service and a user under different sub-trees. Co-authored-by: Cursor <cursoragent@cursor.com> * auth: add depth gate to nested-PrincipalMux unwrapping Adds maxPrincipalMuxDepth (32) as a defensive cap on the recursion in IsUser/IsService/IsMachine. In production the principal graph is built by trusted server code and is at most ~3 levels deep, so the cap is wildly generous and never trips on well-formed input. It exists purely as a guard against a buggy provider that returns a mux containing itself in its allPrincipals map -- without the cap, that would hang the request handler in unbounded recursion. While here, consolidate the recursion into a single principalMatches helper that takes a leaf-test function, and lift the per-predicate leaf tests (isUserLeaf/isServiceLeaf/isMachineLeaf) to package-level functions so each Is* call passes a stable function value and avoids any per-call closure allocation. Adds TestIsService_DepthGate, which builds a chain of exactly maxPrincipalMuxDepth wrappers and asserts the leaf is still found, then adds one more wrapper and asserts the predicate refuses to recurse past the cap. Co-authored-by: Cursor <cursoragent@cursor.com> * auth: lower maxPrincipalMuxDepth from 32 to 5 Production depth is ~3 (Authentication-decorator mux around a composite- provider mux around a leaf), so 5 leaves a small amount of headroom for additional wrapping while keeping the cap close to the realistic worst case. The cap is purely a defensive guard against pathological mux constructions; well-formed input is unaffected. The boundary test (TestIsService_DepthGate) references the constant by name, so it adapts to the new value automatically. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Vinaykumar Miriyala <vmiriyala@pinterest.com> Co-authored-by: Cursor <cursoragent@cursor.com>
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
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
The principal predicates
auth.IsServiceandauth.IsUserare implemented to check only the default sub-principal of aknox.PrincipalMux. When a multi-provider auth chain wraps the authenticated principal in a mux viaknox.NewPrincipalMux, and the default sub-principal happens to be a different type than the one the caller is trying to detect, these predicates returnfalseeven though a matching sub-principal is present.This silently breaks any code path that gates on these predicates after auth. Concretely, the
ServiceKeyCreationAuthorizerhook added in 8438e97 has the shape:Any custom authorizer of this shape will reject every real request when the principal is a
PrincipalMuxwhose default is a non-service principal — even whenprincipal.GetID()returns the expected SPIFFE id and the access logger reportsauth_type: "service"(that field is derived from the matched provider's name, not fromIsService, so the bug is invisible from the request log alone).Fix
PrincipalMux, walkmux.Principals()and return true if any constituent principal matches the target type (default and non-default alike).IsMachinefor symmetry — it didn't previously exist.Principals()accessor onknox.PrincipalMuxthat returns its constituent principals as[]knox.Principal. The underlying field is unexported and there was no existing accessor that returned the full set;Default()only returns one andRaw()returnsRawPrincipals.Test plan
TestPrincipalMuxUserOrService, which was encoding the old default-only behavior, to reflect the new "any sub-principal matches" semantics. This is a behavior change in the predicates; flagging it explicitly. The old behavior is what caused the bug above, so the assertions had to flip.TestIsServicePrincipalMux,TestIsUserPrincipalMux,TestIsMachinePrincipalMux: each wraps a single principal type in a mux and asserts only the matching predicate returns true.TestIsPredicatesRawPrincipals: asserts the fast path still works on raw (non-mux) principals.TestIsServicePrincipalMuxMixedSubPrincipals: a mux whose default is aUserbut which also contains aService—IsServiceandIsUserboth return true,IsMachinereturns false. This is the exact production failure mode.TestPrincipalMuxPrincipals: verifies the newPrincipals()accessor returns all sub-principals.go test ./...— all packages pass.go vet ./...— clean.Notes on scope
Principal.Type()semantics, handler logic, theServiceKeyCreationAuthorizerinterface, or any authorizer implementation.Principals()accessor onPrincipalMuxis the only API addition outside of the predicate functions themselves.Made with Cursor