diff --git a/k8s/bases/infrastructure/cluster-policies/best-practices/restrict-github-team-management.yaml b/k8s/bases/infrastructure/cluster-policies/best-practices/restrict-github-team-management.yaml index 6f72fb62ed..0edfab9a2c 100644 --- a/k8s/bases/infrastructure/cluster-policies/best-practices/restrict-github-team-management.yaml +++ b/k8s/bases/infrastructure/cluster-policies/best-practices/restrict-github-team-management.yaml @@ -21,7 +21,8 @@ metadata: 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 + blocks repository admin grants. A Team may not be nested under a parent + team or linked to a directory group. This prevents a compromised or unintended github-config artifact from adding arbitrary users to arbitrary GitHub teams or granting privileged repository access. spec: @@ -91,6 +92,61 @@ spec: # team's numeric identity and exempts the provider's own write. That rule # depends on the writing identity, so it cannot live in this # background-scanned policy. + # + # #4511: a nested team inherits every repository grant of its parent, and + # that inheritance happens on GitHub — it never passes through the + # TeamRepository rules below, which are what cap this delegation's grants. A + # Team that named a parent would hand its members the parent's access with no + # grant for those rules to judge. The directory link is refused for the same + # reason from the other side: it lets a group outside this cluster decide who + # the members are, past the member allow-list. + # + # All four fields the provider exposes are covered, in both blocks. + # parentTeamReadId and parentTeamReadSlug are documented as read-back values, + # but the schema accepts them as input, so they are refused rather than + # trusted to stay inert. An empty string is what the provider reports for + # "none" and is accepted. + # + # Measured live: neither team sets any of them, and the provider reports all + # four empty for both. The provider is NOT exempt. If a team were nested on + # GitHub by hand, the provider would try to record the parent on the admins + # object, be refused, and report the object out of sync — which is the + # signal wanted for a nesting nobody reviewed. + - name: teams-not-nested-or-directory-linked + match: + any: + - resources: + kinds: + - team.github.m.upbound.io/*/Team + namespaces: + - github-config + validate: + failureAction: Enforce + # Kyverno admits an update by default when the object already failed the + # rule before it. No object fails it today, and one that somehow did + # must not become freely editable for that reason. The cost: such an + # object could not be changed at all, its finalizer included, until + # the field is cleared. + allowExistingViolations: false + message: >- + A github-config Team must leave parentTeamId, parentTeamReadId, + parentTeamReadSlug and ldapDn unset or empty, in forProvider and + initProvider. Nesting a team or linking it to a directory group is a + reviewed change to + restrict-github-team-management.yaml. + deny: + conditions: + all: + # ONE comparison evaluated by JMESPath rather than eight Kyverno + # operators: AnyNotIn and Equals read * and ? in an operand as + # wildcards, and these fields are written by the party this rule + # constrains. JMESPath == compares strings literally. not_null() + # replaces only a missing value with the empty string, so false, + # an empty list or an empty object is refused here and not left + # to the provider's schema to reject. + - key: "{{ not_null(request.object.spec.forProvider.parentTeamId, '') == '' && not_null(request.object.spec.forProvider.parentTeamReadId, '') == '' && not_null(request.object.spec.forProvider.parentTeamReadSlug, '') == '' && not_null(request.object.spec.forProvider.ldapDn, '') == '' && not_null(request.object.spec.initProvider.parentTeamId, '') == '' && not_null(request.object.spec.initProvider.parentTeamReadId, '') == '' && not_null(request.object.spec.initProvider.parentTeamReadSlug, '') == '' && not_null(request.object.spec.initProvider.ldapDn, '') == '' }}" + operator: Equals + value: false - name: teammemberships-reference-allow-listed-teams match: any: diff --git a/tests/policy-failure-actions.json b/tests/policy-failure-actions.json index 0c25c94884..bc65a4bbbf 100644 --- a/tests/policy-failure-actions.json +++ b/tests/policy-failure-actions.json @@ -7,6 +7,7 @@ "restrict-github-team-external-identity/teams-external-name-is-an-approved-identity": "Enforce", "restrict-github-team-management/teams-allow-listed": "Enforce", "restrict-github-team-management/teams-bind-provider-identity-to-object-name": "Enforce", + "restrict-github-team-management/teams-not-nested-or-directory-linked": "Enforce", "restrict-github-team-management/teammemberships-reference-allow-listed-teams": "Enforce", "restrict-github-team-management/teammemberships-allow-listed-members": "Enforce", "restrict-github-team-management/teammemberships-known-roles": "Enforce", diff --git a/tests/restrict-github-team-management/kyverno-test.yaml b/tests/restrict-github-team-management/kyverno-test.yaml index 04bd3b4732..baabb5c27f 100644 --- a/tests/restrict-github-team-management/kyverno-test.yaml +++ b/tests/restrict-github-team-management/kyverno-test.yaml @@ -35,6 +35,17 @@ results: - github-config/attacker-team kind: Team result: pass + # None of the paved-road teams names a parent team or a directory group. + # attacker-team is here for the same reason as above: the nesting rule judges + # only those fields, whatever the allow-list says about the object. + - policy: restrict-github-team-management + rule: teams-not-nested-or-directory-linked + resources: + - github-config/admins + - 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 diff --git a/tests/restrict-github-team-management/nested-or-directory-linked/kyverno-test.yaml b/tests/restrict-github-team-management/nested-or-directory-linked/kyverno-test.yaml new file mode 100644 index 0000000000..1321272df6 --- /dev/null +++ b/tests/restrict-github-team-management/nested-or-directory-linked/kyverno-test.yaml @@ -0,0 +1,37 @@ +--- +apiVersion: cli.kyverno.io/v1alpha1 +kind: Test +metadata: + name: restrict-github-team-management-nested-or-directory-linked +policies: + - >- + ../../../k8s/bases/infrastructure/cluster-policies/best-practices/restrict-github-team-management.yaml +resources: + - resources.yaml +results: + - policy: restrict-github-team-management + rule: teams-not-nested-or-directory-linked + resources: + - github-config/forprovider-parentteamid + - github-config/forprovider-parentteamreadid + - github-config/forprovider-parentteamreadslug + - github-config/forprovider-ldapdn + - github-config/initprovider-parentteamid + - github-config/initprovider-parentteamreadid + - github-config/initprovider-parentteamreadslug + - github-config/initprovider-ldapdn + - github-config/wildcard-star + - github-config/wildcard-question + - github-config/admins + - github-config/typed-false + - github-config/typed-empty-list + - github-config/whitespace + kind: Team + result: fail + - policy: restrict-github-team-management + rule: teams-not-nested-or-directory-linked + resources: + - github-config/maintainers + - github-config/no-spec + kind: Team + result: pass diff --git a/tests/restrict-github-team-management/nested-or-directory-linked/resources.yaml b/tests/restrict-github-team-management/nested-or-directory-linked/resources.yaml new file mode 100644 index 0000000000..93e1296cb1 --- /dev/null +++ b/tests/restrict-github-team-management/nested-or-directory-linked/resources.yaml @@ -0,0 +1,167 @@ +--- +# A parent team, a read-back parent field, or a directory link — each one alone, +# in each block the provider reads. Every one of these must be refused. The +# object names are not on the team allow-list on purpose: this fixture asserts +# only the nesting rule, which has to refuse them whatever the name. +apiVersion: team.github.m.upbound.io/v1beta1 +kind: Team +metadata: + name: forprovider-parentteamid + namespace: github-config +spec: + forProvider: + parentTeamId: "1234567" +--- +apiVersion: team.github.m.upbound.io/v1beta1 +kind: Team +metadata: + name: forprovider-parentteamreadid + namespace: github-config +spec: + forProvider: + parentTeamReadId: "1234567" +--- +apiVersion: team.github.m.upbound.io/v1beta1 +kind: Team +metadata: + name: forprovider-parentteamreadslug + namespace: github-config +spec: + forProvider: + parentTeamReadSlug: other-team +--- +apiVersion: team.github.m.upbound.io/v1beta1 +kind: Team +metadata: + name: forprovider-ldapdn + namespace: github-config +spec: + forProvider: + ldapDn: cn=owners,ou=groups,dc=example,dc=com +--- +apiVersion: team.github.m.upbound.io/v1beta1 +kind: Team +metadata: + name: initprovider-parentteamid + namespace: github-config +spec: + initProvider: + parentTeamId: "1234567" +--- +apiVersion: team.github.m.upbound.io/v1beta1 +kind: Team +metadata: + name: initprovider-parentteamreadid + namespace: github-config +spec: + initProvider: + parentTeamReadId: "1234567" +--- +apiVersion: team.github.m.upbound.io/v1beta1 +kind: Team +metadata: + name: initprovider-parentteamreadslug + namespace: github-config +spec: + initProvider: + parentTeamReadSlug: other-team +--- +apiVersion: team.github.m.upbound.io/v1beta1 +kind: Team +metadata: + name: initprovider-ldapdn + namespace: github-config +spec: + initProvider: + ldapDn: cn=owners,ou=groups,dc=example,dc=com +--- +# Kyverno's own operators read * and ? as wildcards, so a parent written as a +# wildcard is the value most likely to slip past a comparison. It is text like +# any other and must be refused. +apiVersion: team.github.m.upbound.io/v1beta1 +kind: Team +metadata: + name: wildcard-star + namespace: github-config +spec: + forProvider: + parentTeamId: "*" +--- +apiVersion: team.github.m.upbound.io/v1beta1 +kind: Team +metadata: + name: wildcard-question + namespace: github-config +spec: + initProvider: + parentTeamReadSlug: "?" +--- +# An approved forProvider block must not front a parent supplied through +# initProvider, which upjet merges into any field forProvider leaves unset. +apiVersion: team.github.m.upbound.io/v1beta1 +kind: Team +metadata: + name: admins + namespace: github-config +spec: + forProvider: + name: Admins + privacy: secret + initProvider: + parentTeamId: "1234567" +--- +# The provider reports "no parent" and "no directory link" as empty strings. +# Writing them out changes nothing on GitHub and must stay admitted. +apiVersion: team.github.m.upbound.io/v1beta1 +kind: Team +metadata: + name: maintainers + namespace: github-config +spec: + forProvider: + name: maintainers + parentTeamId: "" + parentTeamReadId: "" + parentTeamReadSlug: "" + ldapDn: "" + initProvider: + parentTeamId: "" + ldapDn: "" +--- +# Not text at all. The provider's schema declares these fields as strings and +# would reject each of these first, but the rule must not depend on that: a +# value that is merely "empty-looking" is still a value somebody set. +apiVersion: team.github.m.upbound.io/v1beta1 +kind: Team +metadata: + name: typed-false + namespace: github-config +spec: + forProvider: + parentTeamId: false +--- +apiVersion: team.github.m.upbound.io/v1beta1 +kind: Team +metadata: + name: typed-empty-list + namespace: github-config +spec: + initProvider: + ldapDn: [] +--- +# Blank to a reader, not empty to GitHub. +apiVersion: team.github.m.upbound.io/v1beta1 +kind: Team +metadata: + name: whitespace + namespace: github-config +spec: + forProvider: + parentTeamReadSlug: " " +--- +# Nothing under spec at all: every field is missing, none is set. +apiVersion: team.github.m.upbound.io/v1beta1 +kind: Team +metadata: + name: no-spec + namespace: github-config