Repository navigation
fix(github-config): bind a membership's or grant's stored identity to what it declares - #4524
Conversation
A github-config Team could be created compliant and then re-pointed at a foreign GitHub team by changing crossplane.io/external-name, because the provider finds the remote team by that numeric ID and nothing constrained it. Add an admission policy that accepts the annotation only when it is unset, unchanged, or the approved identity of that team, on create and update. The provider's own record of a team it created is exempt, matched by the revision-suffixed ServiceAccount name it runs as. Part of #3144 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Kyverno's Equals and AnyNotIn read * and ? as wildcards on either side, so an identity of "*" matched the approved one and any later value then counted as unchanged. Evaluate the three accepted cases in one JMESPath expression, which compares strings literally, and cover both wildcard routes with fixtures. Part of #3144 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…on case Part of #3144 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… directory group A nested team inherits its parent's repository access on GitHub, outside the grant rules that cap what the github-config release may hand out, and a directory link moves membership outside the member allow-list. The Team rules checked neither. Refuse all four fields the provider exposes, in forProvider and initProvider, with one literal JMESPath comparison. Fixes #4511 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e rule itself
Review finding: `x || ''` also reads false, [] and {} as empty, so only the
provider's schema stood between those values and admission. not_null() replaces
a missing value only. Adds fixtures for a boolean, an empty list, a blank
string and a Team with no spec, and words the message to match what is accepted.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… what it declares A TeamRepository is read, updated and removed by its stored identity alone, and a TeamMembership is removed by it, so an object that satisfies every rule in restrict-github-team-management could act on a different team, repository or user than the one it names (#4518). Established from the released sources of the provider version prod runs. Anyone but the provider may now only leave that identity unset, leave it unchanged together with the team reference and the repository or user, or set it to the approved identity of the referenced team followed by the declared repository or user. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Evaluation at
38 of 38 as expected. What this trial cannot show is the provider itself acting on a mismatched identity; that part rests on the released source, as described in the pull request. |
…repository or user Independent review: the rule read only forProvider, so a repository stated under initProvider could be moved while the identity counted as unchanged. No object on prod states one. Also names the way to re-point an object in the refusal, and covers a lookalike service account and provider updates. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai review |
|
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
devantler
left a comment
devantler
left a comment
There was a problem hiding this comment.
🤖 Generated by the Agentic Engineer
Self-review (fallback — CodeRabbit, Codex and Cursor Bugbot unavailable)
Reviewed commit: c0f4d0236dc9ba3b22f24571cbdf0293a6c8db86
- CodeRabbit: rate limited — it refused the request ("Review rate limited", 10:20Z) and has reviewed nothing on this pull request.
- Codex: usage limit since 2026-10-05T08:40Z.
- Cursor Bugbot: usage limit since 2026-10-05T09:37Z; no review from it on record.
An independent reviewer read the change in two rounds, proving each claim with kyverno 1.19.1 runs on throwaway fixtures and checking the provider claims against its released source.
Round 1, at 42402808: no P0 or P1. One P2: the rule read the repository or user only under forProvider, so one stated only under initProvider could be moved while the identity counted as unchanged. Fixed in cb3c901f: a stored identity may not be combined with a repository or user under initProvider (no object on prod states one). The refusal now also says how to re-point an object, and fixtures cover a lookalike service account and provider updates.
Round 2, at cb3c901f: no P0 or P1, and the round-1 finding is closed. The two rules are identical apart from repository and username, the expression evaluates as written, the routine re-apply passes, and the provider is skipped on create and update. Non-string, empty, templated and whitespace values behave safely.
Round 2 also raised:
- P2, outside this change: neither policy looks at
teamIdRef.namespace. The stored identity here is still the approved pair, so the exposure is in the older reference rules. Filed as #4525, verification first. - P3: the team half of "unchanged" relies on the older policy refusing a direct team ID or an
initProviderreference. Not reachable today because that policy refuses such an object on create. - P3: an object created with only
initProvider.repositoryis admitted and then refused on every re-apply once the provider records its identity. It fails closed, and the message says what to do. - P3 fixtures: one
initProviderrow passes for another reason, the lookalike rows are only caught by the fixture validator, and the provider-update rows do not model an identity change. Not applied; the validator runs in CI and the guard has two other rows that fail without it. - P3 comment wording: applied in
c0f4d023, which changes two comment lines only.
Not exercised: the provider against GitHub. The trial on a real admission controller is in the evaluation comment and was run at 42402808; the initProvider guard added since is covered by fixtures and mutants only.
Verdict: no P0/P1 findings
…into claude/team-literal-compare-4517
… rules Kyverno's list operators read * and ? in either operand as wildcards, so a value of * matched every entry of an allow-list. Measured on v1.19.1, 25 such values were admitted by restrict-github-team-management: a team display name, a referenced team, a member login, a role and a repository permission. Every condition is now one JMESPath boolean compared with `Equals false`, so each comparison is exact. A membership's or grant's team reference may also only name the github-config namespace, because the provider resolves the reference in the namespace it names (#4525). Fixes #4517 Fixes #4525 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…cover an empty one Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Integrated the repaired security foundations at All 59 Kyverno fixture files evaluate every declared rule. The 46-rule enforced-action baseline, race-enabled baseline tests, and all ten manifest-layer builds pass locally. Fresh native CI and current-head review are still required. Land #4519, then #4526, then this PR so the overlapping changes have one ordered delivery path. The earlier admission trial remains evidence only for the earlier tested policy; it is not a new live-provider trial of this merged commit. |
@coderabbitai full review |
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (50)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📜 Recent review details🧰 Additional context used🧠 Learnings (1)📚 Learning: 2026-08-16T03:58:51.588ZApplied to files:
🔇 Additional comments (50)
📝 Walkthrough📝 WalkthroughPriority: ➖ Normal Merge Risk: ⚪ Minimal · up to This change adds enforced admission rules that stop a membership or grant's stored identity from pointing at a different team, repository or user than it declares. The supplied evidence shows no concrete merge-blocking defect. Confirm policy readiness and that the GitHub configuration still syncs after promotion, as the PR description says. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
devantler
left a comment
devantler
left a comment
There was a problem hiding this comment.
🤖 Generated by the Agentic Engineer
Self-review (fallback — CodeRabbit, Codex and Cursor Bugbot unavailable)
Reviewed commit: ada9540c6234b2a15fbc98f40ec8c660357ff982
- CodeRabbit: this current-head request was refused at 2026-10-05T19:22:33Z; the direct provider response was updated at 19:22:44Z. The head-bound summary at 19:22:41Z reports included reviews unavailable for 57 minutes, until 20:19:41Z. No usage-based billing is authorized.
- Codex: account code-review quota exhausted in the direct response, event 2026-10-05T16:59:30Z, freshly read this round. Recovery requires restored account capacity; no paid overage is authorized.
- Cursor Bugbot: user/team usage or spend limit in the direct response, event 2026-10-03T22:35:03Z, freshly read this round. Recovery requires the account administrator; no newer successful servicing evidence was found.
Fresh direct reads covered this PR's conversation, review objects, threads and current-head checks. The older formal review is stale at this head; no current-head substantive external verdict or Bugbot artifact is present. There are no unresolved review threads.
Reviewed the current 48-path change against main 8cb909aeb70f3927a0522f44d4b10ccc2f7a090d, including its integrated nesting and literal-comparison foundations. The external-identity policy itself is byte-for-byte unchanged from c0f4d0236dc9ba3b22f24571cbdf0293a6c8db86. Both stored-identity rules use literal equality, bind the approved team and declared subject, forbid an initProvider subject alongside an identity, and require the previous team and subject as well as the previous identity for the unchanged path. Missing identities remain admissible; invalid existing objects are not grandfathered. The provider exemption remains limited to its revision service accounts in crossplane-system; the lookalike-account case stays denied. The integrated reference rules retain namespace binding, parent/directory restrictions and the question-mark regression. No credential, provider permission or admission exception is expanded.
All 59 fixture files evaluate their named rules; the 46-rule Enforce baseline, race-enabled baseline tests and ten manifest-layer builds pass at this exact commit. Reviewed the create/update, changed identity, changed reference/subject, wildcard, initProvider, provider and lookalike-account fixtures and their oldObject bindings. The previously reported cross-namespace reference issue is handled by the integrated #4526 foundation, not waived.
The earlier 38-request real-admission trial remains evidence only for its earlier tested commit. This review and the fixture results are not a new exact-head admission-controller trial, provider/GitHub exercise or production readback. CI and ordered delivery through #4519, then #4526, remain required; production must subsequently show the policy Ready and the existing resources still synchronized.
Verdict: no P0/P1 findings
Status note (2026-10-06 10:20Z): the blocker this comment recorded, #4526, merged at 09:57Z. |
The GitHub Actions incident on 5 October cancelled this head's code scanning runs, and those runs cannot be retried. No file changes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai full review |
|
…ntity-4518 # Conflicts: # tests/restrict-github-team-management/literal-comparison/kyverno-test.yaml # tests/restrict-github-team-management/literal-comparison/resources.yaml
…ng the object Removing the stored identity was accepted unconditionally, so one write could clear it and point the membership or grant at another team or subject, after which the provider may record the old identity again on an object that now declares something else. Removal is now accepted only while the team reference and subject stay as they were. Adds fixtures that fail without the rule, and one for another service account in the provider's namespace so widening the exemption is caught. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai full review |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
Evaluation at
After merge: the infrastructure layer on prod is currently suspended for the runner recovery, so the policy will apply when that resumes. The checks listed in the description are owed then. |
Delivery hold at unchanged head The review, checks and evaluation recorded above remain valid for this head. Re-queue once #4565 has merged and a production deploy has completed; if |
1 participant
Why
A team membership or repository grant that passed every existing rule could still act on a different team, repository or user than the one it declares, because nothing checked the identity the provider stores for it. For grants that includes changing a permission, not only removing one.
What
Anyone other than the provider may now only leave that stored identity absent, leave it unchanged together with the team and the repository or user, or set it to the approved identity of the declared team and subject (for recovery after a rebuild). Removing it is accepted only while the team and subject stay as they were, so one write cannot clear it and retarget the object. Every current membership and grant on prod already satisfies the rule, so nothing existing is refused.
Fixes #4518
👉 After merge/promotion: confirm the policy is ready, that the GitHub configuration still applies, and that memberships and grants stay in sync.