Skip to content
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
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
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,252 @@
# 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 github-config 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 admins 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:
- admins
- 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 "admins"
# may carry forProvider.name: some-other-team, so the reference rules below
# accept teamIdRef.name: admins 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: "{{ to_lower(request.object.spec.forProvider.name || '') }}"
operator: AnyNotIn
value:
- ""
- "{{ to_lower(request.object.metadata.name) }}"
- key: "{{ to_lower(request.object.spec.initProvider.name || '') }}"
operator: AnyNotIn
value:
- ""
- "{{ to_lower(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 set spec.forProvider.teamIdRef.name to admins or maintainers with policy.resolve Always, so the reference re-resolves on every reconcile; selectors are not allowed, and initProvider must not carry a teamId, selector or non-allow-listed reference.
deny:
conditions:
any:
# #3195: requiring the reference to re-resolve on every reconcile is what
# makes a directly-authored numeric ID inert, so no forProvider.teamId deny
# is needed -- and none can work. The provider's own reference resolver
# writes the resolved ID into that field, so a deny on it fires on the
# controller's write and deadlocks the resource. Measured live: all 39
# TeamMembership/TeamRepository resources carry a populated teamId.
- key: "{{ request.object.spec.forProvider.teamIdRef.policy.resolve || '' }}"
operator: NotEquals
value: Always
- key: "{{ request.object.spec.forProvider.teamIdSelector || `{}` }}"
operator: NotEquals
value: {}
- key: "{{ request.object.spec.forProvider.teamIdRef.name || '' }}"
operator: AnyNotIn
value:
- admins
- maintainers
# 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:
- ""
- admins
- 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 admins or maintainers by spec.forProvider.teamIdRef.name with policy.resolve Always, and may grant only pull, triage or push permission, in forProvider and initProvider.
deny:
conditions:
any:
# #3195: requiring the reference to re-resolve on every reconcile is what
# makes a directly-authored numeric ID inert, so no forProvider.teamId deny
# is needed -- and none can work. The provider's own reference resolver
# writes the resolved ID into that field, so a deny on it fires on the
# controller's write and deadlocks the resource. Measured live: all 39
# TeamMembership/TeamRepository resources carry a populated teamId.
- key: "{{ request.object.spec.forProvider.teamIdRef.policy.resolve || '' }}"
operator: NotEquals
value: Always
- key: "{{ request.object.spec.forProvider.teamIdSelector || `{}` }}"
operator: NotEquals
value: {}
- key: "{{ request.object.spec.forProvider.teamIdRef.name || '' }}"
operator: AnyNotIn
value:
- admins
- maintainers
Comment thread
devantler marked this conversation as resolved.
# 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 || '' }}"
operator: AnyNotIn
value:
- ""
- pull
- triage
- push
- maintain
- admin
# 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:
- ""
- admins
- maintainers
- key: "{{ request.object.spec.initProvider.permission || '' }}"
operator: AnyNotIn
value:
- ""
- pull
- triage
- push
- maintain
- admin
# #3208: the privilege ceiling is per-team, not flat. A single cap cannot express the
# intent — capping everything at `push` denies all 37 live grants, while widening it to
# `admin` lets the maintainers team be granted `admin` and makes the rule vacuous.
# Measured live, the correlation is exact and has no exceptions: `admins` holds `admin`
# on 22 repositories and `maintainers` holds `maintain` on 15, with zero
# `maintainers`+`admin` and zero `admins`+non-`admin`. Binding the ceiling to the
# referenced team admits all 37 live grants while still denying the escalation a
# compromised artifact would actually want: `admin` through the lower-privileged team.
# The reference rule above already guarantees the team is one of the two allow-listed
# names, so this precondition is exhaustive for anything that rule admits. The flat
# list above remains the known-roles guard against org-defined custom roles.
- name: teamrepositories-maintainers-capped-below-admin
match:
any:
- resources:
kinds:
- team.github.m.upbound.io/*/TeamRepository
namespaces:
- github-config
preconditions:
all:
- key: "{{ request.object.spec.forProvider.teamIdRef.name || '' }}"
operator: Equals
value: maintainers
Comment thread
coderabbitai[bot] marked this conversation as resolved.
validate:
message: GitHub TeamRepository resources referencing the maintainers team may grant at most maintain; admin is reserved to the admins team.
deny:
conditions:
any:
- key: "{{ request.object.spec.forProvider.permission || '' }}"
operator: AnyNotIn
value:
- ""
- pull
- triage
- push
- maintain
- key: "{{ request.object.spec.initProvider.permission || '' }}"
operator: AnyNotIn
value:
- ""
- pull
- triage
- push
- maintain
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-homepage-service-groups.yaml
- best-practices/restrict-tenant-issuer-refs.yaml
- best-practices/restrict-tenant-route-hostnames.yaml
Expand Down
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/admins
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/admins
kind: Team
result: fail
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: admins 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: admins
namespace: github-config
spec:
forProvider:
name: some-other-team
privacy: closed
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/admins
kind: Team
result: pass
- policy: restrict-github-team-management
rule: teams-bind-provider-identity-to-object-name
resources:
- github-config/admins
kind: Team
result: fail
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: admins
namespace: github-config
spec:
initProvider:
name: some-other-team
Loading
Loading