fix(auth): allow anonymous callers to GetJWKS and GetOpenIDConfiguration - #5064
fix(auth): allow anonymous callers to GetJWKS and GetOpenIDConfiguration#5064sdachepally wants to merge 2 commits into
Conversation
Signed-off-by: sdachepally <sdachepally@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan includes up to 12 reviews per rolling hour; 10 remain after this review. Summary by CodeRabbit
WalkthroughAnonymous access now succeeds when a rule includes ChangesAnonymous access authorization
Estimated code review effort: 1 (Trivial) | ~5 minutes Merge Risk: ⚪ Minimal · up to The change allows certificate-less callers to access the two intended public discovery APIs while continuing to reject rules that do not include anonymous access; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
| // No proper cert presented, but we will allow stuff that allows just Anonymous | ||
| return perm_info.principals.as_slice() == [Principal::Anonymous]; | ||
| // No proper cert presented, but we allow any rule that lists Anonymous. | ||
| return perm_info.principals.contains(&Principal::Anonymous); |
There was a problem hiding this comment.
Could we add a regression test for this case? The existing anonymous test covers a rule containing only Anonymous (DiscoverMachine), while this fix is specifically for rules where Anonymous appears alongside other principals. An assertion that certless requests can access GetJWKS and GetOpenIDConfiguration would prevent this from regressing.
…iguration. Signed-off-by: sdachepally <sdachepally@nvidia.com>
b5fac75 to
07405bd
Compare
Related issues
nvbug 6504386
Description
Two public Core gRPC calls,
GetJWKSandGetOpenIDConfiguration, were wrongly returning 403 to callers with no client certificate, even though they're supposed to allow anonymous access.Root cause: in
InternalRBACRules::allowed, the "no client certificate" branch allowed a request only when the rule's principal set was exactly[Anonymous]. The affected rules are[Anonymous, Agent, ForgeAdminCLI, SiteAgent], so the exact-match check failed and certless callers were denied. The fix changes that check from exact equality to membership, so any rule that listsAnonymousallows certless callers, while rules withoutAnonymousstill reject them.Type of Change
Breaking Changes
Testing
Ran Core locally with permission checks turned on and called the two RPCs with no client certificate:
Additional Notes
One-line change in
InternalRBACRules::allowed: check whether the rule contains Anonymous instead of checking that the rule equals exactly[Anonymous].