Skip to content

fix(github-config): constrain team management #2718

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
devantler wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

fix(github-config): constrain team management #2718

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c8a6040
fix(github-config): constrain team management
devantler Jul 18, 2026
6b5eee9
Merge branch 'main' into codex/propose-fix-for-github-team-management…
devantler Jul 19, 2026
73032ca
Merge branch 'main' into codex/propose-fix-for-github-team-management…
devantler Aug 15, 2026
2b57d41
fix(cluster-policies): make the GitHub team guard match, and bind tea…
devantler Aug 15, 2026
e1bd002
fix(cluster-policies): scope the external-name rule with match operat…
devantler Aug 15, 2026
5186097
fix(cluster-policies): close the initProvider bypass; drop the extern…
devantler Aug 15, 2026
426766d
fix(cluster-policies): allow-list team repository permissions
devantler Aug 15, 2026
011e8b4
Merge remote-tracking branch 'origin/main' into codex/propose-fix-for…
devantler Aug 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions k8s/bases/apps/github-config/role.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,11 @@ rules:
- update
- patch
- delete
# Team resources are high-impact because they change real GitHub org
# authorization. RBAC alone cannot express which teams this SA may manage, so
# the restrict-github-team-management Kyverno admission policy constrains the
# allowed team identities, reference style, and repository permissions on top
# of these verbs.
- apiGroups:
- team.github.m.upbound.io
resources:
Expand Down
191 changes: 191 additions & 0 deletions ...bases/infrastructure/cluster-policies/best-practices/restrict-github-team-management.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,191 @@
# Constrains the high-impact GitHub team managed resources delegated to the
# github-config tenant. The tenant's Flux Kustomization applies a separately
# published OCI artifact as the github-config ServiceAccount, and those
# Crossplane resources reconcile with an org-admin GitHub App. Namespace RBAC can
# limit the API group, but it cannot restrict resource spec fields such as the
# team, member, repository, role, or permission. This admission guard keeps that
# delegation scoped to the intended CODEOWNERS teams and blocks direct numeric
# team IDs/selectors that would bypass the allow-list enforced through
# Crossplane references.
apiVersion: kyverno.io/v1
kind: ClusterPolicy
metadata:
name: restrict-github-team-management
annotations:
policies.kyverno.io/title: Restrict GitHub Team Management
policies.kyverno.io/category: Security, GitHub
policies.kyverno.io/severity: high
policies.kyverno.io/subject: Team, TeamMembership, TeamRepository
policies.kyverno.io/minversion: 1.6.0
policies.kyverno.io/description: >-
Restricts provider-upjet-github team resources in the github-config
namespace to the platform CODEOWNERS teams, requires TeamMembership and
TeamRepository resources to reference those Team objects by name, and
blocks repository admin grants. This prevents a compromised or unintended
github-config artifact from adding arbitrary users to arbitrary GitHub
teams or granting privileged repository access.
spec:
validationFailureAction: Enforce
Comment thread
coderabbitai[bot] marked this conversation as resolved.
background: true
rules:
- name: teams-allow-listed
match:
any:
- resources:
kinds:
- team.github.m.upbound.io/*/Team
namespaces:
- github-config
validate:
message: GitHub Team resources managed by github-config must be one of platform or maintainers.
deny:
conditions:
all:
- key: "{{ request.object.metadata.name }}"
Comment thread
devantler marked this conversation as resolved.
Comment thread
devantler marked this conversation as resolved.
operator: AnyNotIn
value:
- platform
- maintainers
# The allow-list above constrains the Kubernetes object name, but the
# provider takes the real GitHub team from spec.forProvider.name (and
# spec.initProvider.name). Without this rule a Team object named "platform"
# may carry forProvider.name: some-other-team, so the reference rules below
# accept teamIdRef.name: platform while the org-admin provider reconciles a
# different remote team. Binding the provider identity to the object name is
# what makes the object name a faithful proxy for the remote team, which is
# the assumption the two reference rules rest on. Both fields are authored
# by the tenant and never rewritten by the provider, so pinning them carries
# no reconciliation risk.
- name: teams-bind-provider-identity-to-object-name
match:
any:
- resources:
kinds:
- team.github.m.upbound.io/*/Team
namespaces:
- github-config
validate:
message: GitHub Team spec.forProvider.name and spec.initProvider.name must be omitted or equal to metadata.name, so the allow-listed object name is the team actually reconciled.
deny:
conditions:
any:
- key: "{{ request.object.spec.forProvider.name || '' }}"
operator: AnyNotIn
value:
- ""
- "{{ request.object.metadata.name }}"
- key: "{{ request.object.spec.initProvider.name || '' }}"
operator: AnyNotIn
value:
- ""
- "{{ request.object.metadata.name }}"
# crossplane.io/external-name is the other way the local name can be
# detached from the remote team, and it is NOT constrained here. A rule
# accepting only an empty annotation or metadata.name looks like the
# obvious counterpart to the binding above, but it breaks legitimate
# re-adoption: after a rebuild, or whenever a Team CR is recreated, the
# existing GitHub team must be adopted by the numeric team ID the provider
# itself writes into this annotation — and refusing that makes Crossplane
# try to create a team that already exists, so github-config cannot recover
# its own teams. Constraining it safely needs a way to tell our teams'
# numeric identities from a foreign one, plus the provider ServiceAccount
# excluded so its post-create write still succeeds. Tracked in #3144 rather
# than half-closed here.
- name: teammemberships-reference-allow-listed-teams
match:
any:
- resources:
kinds:
- team.github.m.upbound.io/*/TeamMembership
namespaces:
- github-config
validate:
message: GitHub TeamMembership resources must use spec.forProvider.teamIdRef.name set to platform or maintainers; direct teamId values and selectors are not allowed, in forProvider or initProvider.
deny:
conditions:
any:
- key: "{{ request.object.spec.forProvider.teamId || '' }}"
operator: NotEquals
value: ""
Comment on lines +107 to +109

@chatgpt-codex-connector chatgpt-codex-connector Bot Aug 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Allow Crossplane to persist resolved team IDs

When an allowed TeamMembership uses teamIdRef, provider-upjet's generated reference resolver persists the referenced Team's external ID into spec.forProvider.teamId before Terraform reconciliation. This condition rejects that controller-authored update; the identical check in the TeamRepository rule does the same there. Consequently the unresolved fixtures pass initial admission, but both intended reference-based resources—and existing resources after their IDs have been resolved—cannot reconcile. Distinguish tenant-supplied IDs from provider resolution rather than universally rejecting a populated resolved field.

Useful? React with 👍 / 👎.

@devantler devantler Aug 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Generated by the Agentic Engineer

P1 confirmed by execution, not by reading — and it is worse than "fixtures pass initial admission": the reference path this policy mandates can never reconcile at all.

Reproduced locally with Kyverno CLI 1.18.2, the exact version CI pins. Adding one fixture — the state provider-upjet leaves behind after it resolves teamIdRef (an allow-listed teamIdRef.name: platform plus the resolved spec.forProvider.teamId) — is denied by teammemberships-reference-allow-listed-teams. So the controller's own write on an approved reference is rejected, and TeamRepository carries the identical condition.

The fix, and the measurement that constrains it. The reference rules must exempt the provider identity, exactly as the crossplane.io/external-name comment in this file already anticipates ("plus the provider ServiceAccount excluded so its post-create write still succeeds"). Two things that are not obvious:

  1. Kyverno does not honour a wildcard here. clusterRoles: [crossplane:provider:provider-upjet-github-*:system] changed nothing — both identities behaved identically. The same exclusion with the exact ClusterRole name fired correctly. Isolated on that single conjunct, so it is the wildcard that fails, not the mechanism.
  2. That makes the dynamic provider identity the real blocker. The SA is provider-upjet-github-<revision-hash> in crossplane-system, and this repo already matches that shape by regex in cluster-security-exceptions. Since neither subjects nor clusterRoles accepts a pattern, the identity has to be pinned — spec.serviceAccountTemplate.metadata.name in the provider's DeploymentRuntimeConfig, which is currently unset.

With the SA pinned and exclude.any[].subjects naming it, both arms come out right:

  • provider identity → reference rules excluded (pass: 5, fail: 1; the single fail is teams-allow-listed on attacker-team, a Team rule that is deliberately not excluded)
  • github-config tenant → all 9 violations still denied, byte-identical to the pre-fix run

So the guard loses nothing against the tenant, which is the threat this policy exists for.

spec.background must also become false: the exclusion matches on request.userInfo, which is absent during background scans — the convention every other tenant-restriction policy in this repo already follows.

Leaving this thread open: the finding is valid and the fix is not yet pushed. It spans the provider DeploymentRuntimeConfig as well as this policy, plus a provider-author/ fixture arm mirroring the existing platform-author/ pattern.

@devantler devantler Aug 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Generated by the Agentic Engineer

Confirmed — this finding is valid, and it is reproducible. I verified it independently
with the Kyverno CLI against this policy rather than accepting it on inspection.

Two TeamMembership resources differing in exactly one field — the resolved ID that
provider-upjet's reference resolver persists into spec.forProvider.teamId — with the same
allow-listed teamIdRef.name: platform:

Applying 4 policy rule(s) to 2 resource(s)...
policy restrict-github-team-management -> resource
  github-config/TeamMembership/subject-after-resolution failed:
1 - teammemberships-reference-allow-listed-teams ...
pass: 1, fail: 1

The control (ref only, teamId absent) passes; the post-resolution shape fails. The same
holds for TeamRepository via teamrepositories-reference-allow-listed-teams.

With validationFailureAction: Enforce and background: true, that means the reference-based
form — the only form these rules permit — is admitted at create and can then never
reconcile, because the controller's own write-back is rejected. So the policy does not merely
over-restrict a corner case; it makes the paved road unusable.

Not fixing it blind, because the two obvious fixes each have a real failure mode:

  1. Exclude the provider ServiceAccount from the two reference rules. This is what the
    crossplane.io/external-name comment at lines 82–93 already contemplates for Team external-name is only pinned at creation, so a tenant UPDATE can still adopt a foreign GitHub team #3144. But
    Crossplane names that SA after the provider revision, so a statically pinned name
    silently stops matching on the next provider upgrade — and this rule failing open is a
    security regression, not an outage.
  2. Allow teamId when an allow-listed teamIdRef is present. This reads like the
    narrow fix and is a bypass: Crossplane's default resolution policy is IfNotPresent, so
    a tenant supplying both a valid ref and a foreign teamId keeps the foreign ID — the
    resolver never overwrites it. That is precisely the attack this policy exists to block.

A correct fix likely has to distinguish the transition (oldObject.teamId empty →
populated, ref allow-listed) rather than the state, or pin the resolution policy. That is a
design decision on the security boundary this PR owns, so I am leaving this thread open
rather than guessing at it.

Also worth noting: the current fixtures cannot catch this — they only cover
pre-resolution shapes, so the suite is green while the deployed behaviour breaks. Whatever
fix lands should add the post-resolution resource as a fixture.

Unrelated to the above, I resolved this PR's merge conflict with main and pushed it
(426766d4 → 011e8b4d); it was DIRTY and is now mergeable-state-clear. Both overlays
validate (577 files each) and naming passes.

@devantler devantler Aug 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Generated by the Agentic Engineer

Confirmed against the live cluster — this finding is correct, and it is broader than the fixtures suggest.

All 39 live TeamMembership/TeamRepository resources in the prod github-config namespace carry a populated spec.forProvider.teamId written by the reference resolver, so this condition rejects the controller's own write on every one of them. kyverno apply against the 41 live resources returns pass: 2, fail: 41.

Leaving this thread open: the fix is not in yet, and it is entangled with three further mismatches between the allow-lists and the deployed tenant (wrong team name, permission allow-list excluding every deployed grant, and a case mismatch in the identity binding). Details and a proposed team-bound permission model are in the PR comment.

@devantler devantler Aug 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Generated by the Agentic Engineer

Confirmed valid — verified against the live cluster, and not yet fixed. Leaving this thread open.

The mechanism is as described. spec.forProvider.teamId is denied whenever populated, but upjet's reference resolver writes the resolved Team external ID into exactly that field once teamIdRef resolves. So an allow-listed resource passes admission at CREATE and is then denied on the controller's own follow-up write — it can never reconcile.

Why the condition cannot simply be dropped. Crossplane's reference resolution policy defaults to IfNotPresent. With the deny removed, a tenant-supplied teamId would already be present, the ref would not be resolved over it, and the allow-listed teamIdRef would become decorative while the org-admin provider reconciled a foreign team. That is precisely the bypass this rule exists to close, so the fix has to distinguish the writer, exactly as you say — not relax the field check.

🔴 The obvious fix is a time-bomb, and this is the part worth recording. The repo already has the right shape for a writer-based carve-out — restrict-tenant-secret-stores.yaml, restrict-tenant-issuer-refs.yaml and restrict-tenant-route-hostnames.yaml all pair exclude.any[].subjects with background: false (userInfo is unavailable to background scans, so that pairing is mandatory, not stylistic). But exclude.subjects matches a ServiceAccount by exact name, and the provider's SA is revision-hashed. Measured on admin@prod just now:

pod=provider-upjet-github-04d509bb9f6f-56bfb8ccc5-6xcfg
sa =provider-upjet-github-04d509bb9f6f

That suffix is the ProviderRevision hash, so it changes on every provider upgrade. An exact-name exclude would work today and silently stop matching after the next bump — reinstating this exact deadlock with no diff, no failing test, and no signal until reconciliation quietly stops.

Durable shape instead: gate the rule on a precondition over request.userInfo.username matched by pattern rather than by exact SA name, e.g. regex_match('^system:serviceaccount:crossplane-system:provider-upjet-github-[a-f0-9]+$', request.userInfo.username), with background: false on that rule per the precedent above. That survives a revision bump. The same treatment applies to the identical condition in teamrepositories-reference-allow-listed-teams.

Per [monorepo memory on Kyverno work], the fix needs verification against the live cluster — kyverno apply over a List renders vacuously — and both states pinned: the provider's resolved-ID write must be admitted, and a tenant-supplied teamId must still be denied.

I have not written that change: it is a behaviour change to an Enforce security policy on this branch, and shipping it without the both-states live verification would risk failing open on the very bypass the rule exists to close. Flagging with the evidence rather than half-closing it.

- key: "{{ request.object.spec.forProvider.teamIdSelector || `{}` }}"
operator: NotEquals
value: {}
- key: "{{ request.object.spec.forProvider.teamIdRef.name || '' }}"
operator: AnyNotIn
value:
- platform
- maintainers
Comment thread
devantler marked this conversation as resolved.
# upjet merges initProvider into any unset forProvider field, so
# every constraint above has to hold there too — otherwise an
# approved forProvider reference fronts a foreign team supplied
# under initProvider.
- key: "{{ request.object.spec.initProvider.teamId || '' }}"
operator: NotEquals
value: ""
- key: "{{ request.object.spec.initProvider.teamIdSelector || `{}` }}"
operator: NotEquals
value: {}
- key: "{{ request.object.spec.initProvider.teamIdRef.name || '' }}"
operator: AnyNotIn
value:
- ""
- platform
- maintainers
- name: teamrepositories-reference-allow-listed-teams
match:
any:
- resources:
kinds:
- team.github.m.upbound.io/*/TeamRepository
namespaces:
- github-config
validate:
message: GitHub TeamRepository resources must reference platform or maintainers by spec.forProvider.teamIdRef.name and may grant only pull, triage or push permission, in forProvider and initProvider.
deny:
conditions:
any:
- key: "{{ request.object.spec.forProvider.teamId || '' }}"
operator: NotEquals
value: ""
- key: "{{ request.object.spec.forProvider.teamIdSelector || `{}` }}"
operator: NotEquals
value: {}
- key: "{{ request.object.spec.forProvider.teamIdRef.name || '' }}"
operator: AnyNotIn
value:
- platform
- maintainers
# An allow-list, not a deny-list on `admin`: GitHub also offers
# `maintain` and organization-defined custom repository roles, so
# denying only `admin` fails open the moment the tenant picks
# another privileged role. An empty value is the provider default
# (`pull`).
- key: "{{ request.object.spec.forProvider.permission || '' }}"
Comment thread
devantler marked this conversation as resolved.
operator: AnyNotIn
value:
- ""
- pull
- triage
- push
# Same initProvider merge as above: an approved forProvider
# reference must not be able to front a foreign team or an admin
# grant supplied under initProvider.
- key: "{{ request.object.spec.initProvider.teamId || '' }}"
operator: NotEquals
value: ""
- key: "{{ request.object.spec.initProvider.teamIdSelector || `{}` }}"
operator: NotEquals
value: {}
- key: "{{ request.object.spec.initProvider.teamIdRef.name || '' }}"
operator: AnyNotIn
value:
- ""
- platform
- maintainers
- key: "{{ request.object.spec.initProvider.permission || '' }}"
operator: AnyNotIn
value:
- ""
- pull
- triage
- push
1 change: 1 addition & 0 deletions k8s/bases/infrastructure/cluster-policies/kustomization.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ resources:
- best-practices/disable-default-sa-automount.yaml
- best-practices/disallow-latest-tag.yaml
- best-practices/propagate-reloader-to-flagger-primary.yaml
- best-practices/restrict-github-team-management.yaml
- best-practices/restrict-tenant-issuer-refs.yaml
- best-practices/restrict-tenant-secret-stores.yaml
- best-practices/validate-host-restrictions.yaml
Expand Down
25 changes: 25 additions & 0 deletions tests/restrict-github-team-management/forprovider-mismatch/kyverno-test.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
---
apiVersion: cli.kyverno.io/v1alpha1
kind: Test
metadata:
name: restrict-github-team-management-forprovider-mismatch
policies:
- >-
../../../k8s/bases/infrastructure/cluster-policies/best-practices/restrict-github-team-management.yaml
resources:
- resources.yaml
results:
# The object name is allow-listed, so that rule cannot catch this.
- policy: restrict-github-team-management
rule: teams-allow-listed
resources:
- github-config/platform
kind: Team
result: pass
# Only the identity binding catches it.
- policy: restrict-github-team-management
rule: teams-bind-provider-identity-to-object-name
resources:
- github-config/platform
kind: Team
result: fail
15 changes: 15 additions & 0 deletions tests/restrict-github-team-management/forprovider-mismatch/resources.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
---
# THE ESCALATION THIS RULE EXISTS TO STOP. metadata.name passes the allow-list,
# so a TeamMembership with teamIdRef.name: platform resolves to this object —
# while the org-admin provider reconciles the unrelated team named here.
# Its own fixture directory because it must reuse the allow-listed object name
# that the paved-road fixture already occupies.
apiVersion: team.github.m.upbound.io/v1beta1
kind: Team
metadata:
name: platform
namespace: github-config
spec:
forProvider:
name: some-other-team
privacy: closed
23 changes: 23 additions & 0 deletions tests/restrict-github-team-management/initprovider-mismatch/kyverno-test.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
---
apiVersion: cli.kyverno.io/v1alpha1
kind: Test
metadata:
name: restrict-github-team-management-initprovider-mismatch
policies:
- >-
../../../k8s/bases/infrastructure/cluster-policies/best-practices/restrict-github-team-management.yaml
resources:
- resources.yaml
results:
- policy: restrict-github-team-management
rule: teams-allow-listed
resources:
- github-config/platform
kind: Team
result: pass
- policy: restrict-github-team-management
rule: teams-bind-provider-identity-to-object-name
resources:
- github-config/platform
kind: Team
result: fail
11 changes: 11 additions & 0 deletions tests/restrict-github-team-management/initprovider-mismatch/resources.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
---
# The same detachment expressed through initProvider, which upjet merges into
# forProvider when the field is unset there.
apiVersion: team.github.m.upbound.io/v1beta1
kind: Team
metadata:
name: platform
namespace: github-config
spec:
initProvider:
name: some-other-team
68 changes: 68 additions & 0 deletions tests/restrict-github-team-management/kyverno-test.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
---
apiVersion: cli.kyverno.io/v1alpha1
kind: Test
metadata:
name: restrict-github-team-management
policies:
- >-
../../k8s/bases/infrastructure/cluster-policies/best-practices/restrict-github-team-management.yaml
resources:
- resources.yaml
results:
# Only the CODEOWNERS teams may be managed through this delegation.
- policy: restrict-github-team-management
rule: teams-allow-listed
resources:
- github-config/attacker-team
kind: Team
result: fail
- policy: restrict-github-team-management
rule: teams-allow-listed
resources:
- github-config/platform
- github-config/maintainers
kind: Team
result: pass
# A self-consistent provider identity passes, including when it is omitted and
# Crossplane derives the remote team from the object name. attacker-team is
# here on purpose: it fails the allow-list above yet must pass this rule, so a
# regression that collapsed the two rules into one would be caught.
- policy: restrict-github-team-management
rule: teams-bind-provider-identity-to-object-name
resources:
- github-config/platform
- github-config/maintainers
- github-config/attacker-team
kind: Team
result: pass
# References must go through the allow-listed Team objects, never a raw ID.
- policy: restrict-github-team-management
rule: teammemberships-reference-allow-listed-teams
resources:
- github-config/direct-team-id
- github-config/initprovider-foreign-team
- github-config/selector-reference
- github-config/non-allow-listed-reference
kind: TeamMembership
result: fail
- policy: restrict-github-team-management
rule: teammemberships-reference-allow-listed-teams
resources:
- github-config/platform-member
kind: TeamMembership
result: pass
# Repository grants are capped below admin.
- policy: restrict-github-team-management
rule: teamrepositories-reference-allow-listed-teams
resources:
- github-config/platform-repo-admin
- github-config/initprovider-admin-grant
- github-config/maintain-grant
kind: TeamRepository
result: fail
- policy: restrict-github-team-management
rule: teamrepositories-reference-allow-listed-teams
resources:
- github-config/platform-repo
kind: TeamRepository
result: pass
Loading
Loading