Repository navigation
fix(github-config): pin a team's GitHub identity for its whole lifecycle - #4509
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>
@coderabbitai full review |
|
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 16 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (53)
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 |
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>
Tried on a real admission controllerTried at First, the hole an independent review found in the first commit was real. With the policy from With the fixed policy:
All twenty matched what was intended. Two rows are worth knowing: a cluster administrator is held to the rule too, so adopting a team under a new identity by hand needs the approved list changed first; and the tenant's routine re-apply of a team whose identity is not yet approved keeps working, which is what stops a newly created team from wedging. Also run: the 24 offline fixture rows pass and the repository's guard confirms every row was really evaluated; 21 deliberately broken versions of the policy were tried against them and 20 are caught (the one not caught there, switching the rule to report-only, is caught by the enforcement ledger check instead). The policy from the first commit fails the new wildcard fixtures. Not exercised: the provider itself against GitHub. The trial covers what the cluster admits, not what the provider then does with it. Follow-ups found on the way and filed: #4511 (a managed team can still be nested under another team), #4517 (the older team rules use the same wildcard-reading comparisons), #4518 (whether a membership or grant can be re-pointed by its stored identity). |
…on case Part of #3144 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: 3db86e1c075397775eaa3906c4e545f24aae2056
- CodeRabbit: rate limited — it refused the request on the first head ("Review rate limited", 08:20Z) and has reviewed nothing on this pull request.
- Codex: usage limit since 2026-10-05T05:05Z.
- Cursor Bugbot: usage limit since 2026-10-05T05:09Z; no review from it on record.
An independent reviewer read the change in two rounds, checking Kyverno's behaviour against its 1.19.1 source rather than from memory.
Round 1, at 464bcd9e, found one P0: the rule compared the identity with Kyverno operators that read * and ? as wildcards, so an identity of * was admitted and any later non-numeric value then counted as unchanged. Reproduced on a real admission controller, then fixed in 8e1d8c40 by evaluating the three accepted cases in one literal comparison, with fixtures for both wildcard routes. The first commit's policy fails those fixtures.
Round 2, at 8e1d8c40, found no P0 or P1 in the new comparison. It tried about thirty hostile values on create and update (variable syntax, quotes, spacing, leading zeros, other number spellings, a 300 kB value); all were refused. A value that makes the comparison itself fail is refused too, because the policy and the chart both fail closed. It found no legitimate flow that is now refused.
Round 2 also raised, and this head or a follow-up answers:
- P2, the older team rules use the same wildcard-reading operators on tenant-written text: outside this change, filed as #4517.
- P2, unverified, the stored identity of memberships and grants is not checked: filed as #4518 with verification as its first step.
- P3 comment and fixture notes (operator wording, a stale plural, an unsourced provider claim, a ragged comment, a case pinning that variable syntax is not expanded): all applied in
3db86e1c, which changes comments and fixtures only.
Round 1 also noted that a managed team can still be nested under another team: filed as #4511.
Known and accepted: the provider exemption matches by name prefix in the Crossplane namespace, so it would also cover another package whose name starts the same way; creating an account there is a platform-level permission. A cluster administrator is held to the rule as well.
Not exercised: the provider against GitHub. The trial on a real admission controller is recorded in a comment on this pull request and was repeated for the key cases with the policy from this head.
Verdict: no P0/P1 findings
Why
The cluster manages two GitHub teams that control who may approve and administer the organization's repositories. The existing guardrails check a team's name, but the team that is actually managed is chosen by a numeric identity stored beside it, and nothing checked that. A faulty or compromised configuration release could therefore create a correctly named team and then re-point it at a different team in the organization.
What
The cluster now refuses any attempt to set or change that identity to anything other than the approved identity of that team, both when a team is first declared and on every later change. Leaving the identity alone still works, so do recovery after a rebuild and the component that records the identity of a team it has just created.
Fixes #3144