fix(github-config): constrain team management #2718
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,178 @@ | ||
| # 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 | ||
| 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 }}" | ||
|
devantler marked this conversation as resolved.
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
There was a problem hiding this comment.
When an allowed Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment.
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 The fix, and the measurement that constrains it. The reference rules must exempt the provider identity, exactly as the
With the SA pinned and
So the guard loses nothing against the tenant, which is the threat this policy exists for.
Leaving this thread open: the finding is valid and the fix is not yet pushed. It spans the provider |
||
| - key: "{{ request.object.spec.forProvider.teamIdSelector || `{}` }}" | ||
| operator: NotEquals | ||
| value: {} | ||
| - key: "{{ request.object.spec.forProvider.teamIdRef.name || '' }}" | ||
| operator: AnyNotIn | ||
| value: | ||
| - platform | ||
| - maintainers | ||
|
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 not grant admin permission, in forProvider or 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 | ||
| - key: "{{ request.object.spec.forProvider.permission || '' }}" | ||
|
devantler marked this conversation as resolved.
|
||
| operator: Equals | ||
| value: admin | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
| # 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: Equals | ||
| value: admin | ||
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
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
| 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 |
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
| 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 |
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
| 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 |
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
| 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 |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| --- | ||
| 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 | ||
| 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 | ||
| kind: TeamRepository | ||
| result: fail | ||
| - policy: restrict-github-team-management | ||
| rule: teamrepositories-reference-allow-listed-teams | ||
| resources: | ||
| - github-config/platform-repo | ||
| kind: TeamRepository | ||
| result: pass |
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.