diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index be0ddb9c8..33f447bde 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -122,6 +122,7 @@ jobs: # either side changing alone is the drift this test exists to catch. - 'scripts/tests/test-restrict-homepage-service-groups.sh' - 'scripts/tests/test-kyverno-umami-mutation-rbac.sh' + - 'scripts/tests/test-tenant-route-hostname-boundary.sh' - 'scripts/tests/test-umami-provisioning-bootstrap.sh' - 'scripts/tests/test-kyverno-admission-vpa.sh' - 'scripts/tests/kyverno-admission-vpa-rules.yaml' @@ -450,6 +451,8 @@ jobs: bash scripts/tests/test-restrict-homepage-service-groups.sh shellcheck scripts/tests/test-kyverno-umami-mutation-rbac.sh bash scripts/tests/test-kyverno-umami-mutation-rbac.sh + shellcheck scripts/tests/test-tenant-route-hostname-boundary.sh + bash scripts/tests/test-tenant-route-hostname-boundary.sh shellcheck scripts/tests/test-actual-budget-auth-route.sh bash scripts/tests/test-actual-budget-auth-route.sh diff --git a/docs/TENANTS.md b/docs/TENANTS.md index 96c167208..cee305fd2 100644 --- a/docs/TENANTS.md +++ b/docs/TENANTS.md @@ -220,8 +220,26 @@ carry itself**: here** — **hostnames**, **`gethomepage.dev/*` dashboard annotations**, routes, and app config: - List all of a tenant's hostnames (local + prod + any custom domains) directly in its - `deploy/httproute.yaml`. The Gateway attaches only the hostnames that match a listener in a - given environment, so listing them all is safe everywhere. + `deploy/httproute.yaml`, and add every approved hostname to the platform-side + Kyverno allow-list in + [`restrict-tenant-route-hostnames.yaml`](../k8s/bases/infrastructure/cluster-policies/best-practices/restrict-tenant-route-hostnames.yaml). + The shared platform Gateway intentionally accepts routes from all namespaces, so the + admission policy is the boundary that prevents a tenant artifact from claiming another + platform hostname. +- **A tenant with no rule of its own is denied by default.** Adding the allow-list entry is + therefore part of onboarding, not an optional hardening step: until a tenant namespace has + a rule naming its approved hostnames, its HTTPRoutes are refused at admission with a + message saying exactly that. The alternative — enumerating only some tenants — is + fail-open, and `doggy-countdown` ran that way, able to claim any hostname because no rule + matched its namespace. +- **`HTTPRoute` is the only route kind a tenant can use.** The hostname allow-list above + matches `HTTPRoute`, so every other Gateway API route kind is closed off rather than left + to reach the shared listener unchecked: the tenant role grants only `httproutes` and + `referencegrants`, and each Gateway listener pins `allowedRoutes.kinds` to `HTTPRoute` + (an HTTPS listener would otherwise accept `GRPCRoute` too). A tenant needing another kind + is a platform change, not a tenant one — extend the hostname policy to cover that kind + and relax both layers together. CI runs the rendered two-layer route-kind boundary test + alongside the canonical Kyverno policy fixtures, so neither half can widen silently. - The platform's `homepage` app discovers `gethomepage.dev/*` annotations on the tenant's HTTPRoute cluster-wide, so the tenant authors them in its own artifact — they are tenant self-presentation, not platform config. diff --git a/k8s/bases/infrastructure/cluster-roles/gateway-tenant-edit.yaml b/k8s/bases/infrastructure/cluster-roles/gateway-tenant-edit.yaml index cbf577aed..dbf17d842 100644 --- a/k8s/bases/infrastructure/cluster-roles/gateway-tenant-edit.yaml +++ b/k8s/bases/infrastructure/cluster-roles/gateway-tenant-edit.yaml @@ -1,7 +1,22 @@ # Aggregate ClusterRole that extends the tenant role (`tenant-edit`) with -# Gateway API verbs so a tenant ServiceAccount can manage HTTPRoute / -# ReferenceGrant resources in its namespace without broadening the built-in -# `edit` role for every namespace. +# Gateway API verbs so a tenant ServiceAccount can manage +# HTTPRoute / ReferenceGrant resources in its namespace. +# +# HTTPRoute is the ONLY route kind granted, and that is a security boundary +# rather than a convenience. `restrict-tenant-route-hostnames` confines a +# tenant to its approved hostnames by matching `HTTPRoute`, so any other route +# kind a tenant could create would reach the shared hostname-less listener +# without that policy ever running — a compromised tenant artifact could then +# claim another service's hostname under the wildcard Gateway certificate. +# Granting `grpcroutes` here previously left exactly that gap open, because an +# HTTPS listener accepts GRPCRoute as well as HTTPRoute by default. +# +# The Gateway listeners pin `allowedRoutes.kinds` to HTTPRoute as the second +# layer, so a route created by some other principal still cannot attach. If a +# route kind is ever genuinely needed, extend the hostname policy to cover it +# and relax BOTH layers together — never this one alone. +# This role deliberately carries only the tenant-specific aggregation label; +# ordinary built-in `edit` bindings must not inherit Gateway API route verbs. --- apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRole @@ -13,10 +28,6 @@ rules: - apiGroups: ["gateway.networking.k8s.io"] resources: - httproutes - - grpcroutes - - tcproutes - - tlsroutes - - udproutes - referencegrants verbs: - get diff --git a/k8s/bases/infrastructure/gateway/gateway.yaml b/k8s/bases/infrastructure/gateway/gateway.yaml index f9cde6485..7399e9f7b 100644 --- a/k8s/bases/infrastructure/gateway/gateway.yaml +++ b/k8s/bases/infrastructure/gateway/gateway.yaml @@ -6,6 +6,14 @@ metadata: spec: gatewayClassName: cilium listeners: + # Every listener pins `allowedRoutes.kinds` to HTTPRoute. Left unset, an + # HTTPS listener accepts GRPCRoute as well (Gateway API's documented + # default), and `restrict-tenant-route-hostnames` matches HTTPRoute + # only — so a GRPCRoute could claim another service's hostname on this + # shared, hostname-less listener without the hostname policy ever running. + # The tenant RBAC (`cluster-roles/gateway-tenant-edit.yaml`) grants only + # HTTPRoute for the same reason; this is the second layer, covering routes + # created by any other principal. Relax both together or neither. - name: https port: 443 protocol: HTTPS @@ -16,9 +24,15 @@ spec: allowedRoutes: namespaces: from: All + kinds: + - group: gateway.networking.k8s.io + kind: HTTPRoute - name: http port: 80 protocol: HTTP allowedRoutes: namespaces: from: All + kinds: + - group: gateway.networking.k8s.io + kind: HTTPRoute diff --git a/k8s/providers/hetzner/infrastructure/patches/add-ascoachingogvaner-dk-listeners.yaml b/k8s/providers/hetzner/infrastructure/patches/add-ascoachingogvaner-dk-listeners.yaml index c232cb25b..ad7ee1188 100644 --- a/k8s/providers/hetzner/infrastructure/patches/add-ascoachingogvaner-dk-listeners.yaml +++ b/k8s/providers/hetzner/infrastructure/patches/add-ascoachingogvaner-dk-listeners.yaml @@ -11,6 +11,10 @@ # Certificate), and the tenant ships a ReferenceGrant authorising this Gateway to # read that Secret. Until the cert issues the listener simply stays # ResolvedRefs=False (Gateway still Programmed) — it never gates reconciliation. +# +# Both listeners pin `allowedRoutes.kinds` to HTTPRoute, matching the base +# Gateway's listeners — see k8s/bases/infrastructure/gateway/gateway.yaml for +# why an unpinned HTTPS listener is a hostname-claiming gap. - op: add path: /spec/listeners/- value: @@ -28,6 +32,9 @@ allowedRoutes: namespaces: from: All + kinds: + - group: gateway.networking.k8s.io + kind: HTTPRoute - op: add path: /spec/listeners/- value: @@ -45,3 +52,6 @@ allowedRoutes: namespaces: from: All + kinds: + - group: gateway.networking.k8s.io + kind: HTTPRoute diff --git a/scripts/tests/test-tenant-route-hostname-boundary.sh b/scripts/tests/test-tenant-route-hostname-boundary.sh new file mode 100755 index 000000000..7711e78f1 --- /dev/null +++ b/scripts/tests/test-tenant-route-hostname-boundary.sh @@ -0,0 +1,82 @@ +#!/usr/bin/env bash +# Negative coverage for the tenant hostname boundary on the shared Gateway. +# +# The boundary is enforced by `restrict-tenant-route-hostnames`, which +# matches HTTPRoute. That leaves a gap unless the other route kinds cannot reach +# the shared listener at all: an HTTPS listener accepts GRPCRoute as well as +# HTTPRoute by default, so a tenant able to create a GRPCRoute could claim +# another service's hostname under the wildcard certificate without the policy +# ever running. +# +# Two layers close it, and this test pins BOTH — either alone is bypassable: +# 1. tenant RBAC grants only `httproutes` (+ `referencegrants`); +# 2. every Gateway listener pins `allowedRoutes.kinds` to HTTPRoute, covering +# routes created by any other principal. +# The policy's allow-list and default-deny behavior have their own real Kyverno +# fixtures under tests/restrict-tenant-route-hostnames; this test owns the +# separate route-kind boundary those fixtures cannot exercise. +set -euo pipefail + +script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +repo_root="$(cd "${script_dir}/../.." && pwd)" + +role_file="${repo_root}/k8s/bases/infrastructure/cluster-roles/gateway-tenant-edit.yaml" +# The prod overlay is what carries every listener: two come from the base +# Gateway and two more are appended by the hetzner JSON6902 patch, which a +# per-file check would never see. +overlay="${repo_root}/k8s/providers/hetzner/infrastructure" + +workdir="$(mktemp -d)" +trap 'rm -rf "${workdir}"' EXIT +render="${workdir}/render.yaml" +gateway="${workdir}/gateway.yaml" + +fail() { + echo "::error::$1" + exit 1 +} + +# --- Layer 1: tenant RBAC grants only HTTPRoute ----------------------------- +# Positive control first: a typo'd path or a renamed role would otherwise make +# every assertion below pass over an empty document set. +role_name="$(yq eval 'select(.kind=="ClusterRole") | .metadata.name' "${role_file}")" +[ "${role_name}" = "gateway-tenant-edit" ] || + fail "expected ClusterRole gateway-tenant-edit in ${role_file}, found '${role_name}'" + +granted="$(yq eval \ + 'select(.kind=="ClusterRole") | .rules[] | select(.apiGroups[] == "gateway.networking.k8s.io") | .resources[]' \ + "${role_file}" | sort | tr '\n' ' ')" +[ -n "${granted// /}" ] || + fail "gateway-tenant-edit grants no gateway.networking.k8s.io resources at all — the rule shape changed, so this test is no longer checking anything" + +expected_grant="httproutes referencegrants " +[ "${granted}" = "${expected_grant}" ] || + fail "gateway-tenant-edit must grant exactly 'httproutes referencegrants'; found '${granted}'. Any other route kind bypasses restrict-tenant-route-hostnames — extend that policy and the listener kinds before widening this." + +# --- Layer 2: every listener pins allowedRoutes.kinds to HTTPRoute ---------- +kubectl kustomize "${overlay}" >"${render}" 2>"${workdir}/render.err" || + fail "prod infrastructure overlay failed to build: $(tail -5 "${workdir}/render.err")" + +yq eval 'select(.kind=="Gateway" and .metadata.name=="platform")' "${render}" >"${gateway}" +listener_count="$(yq eval '.spec.listeners | length' "${gateway}")" +# The live Gateway had 4 listeners when this boundary was written. Requiring at +# least that many keeps an empty or mis-selected render from passing vacuously — +# adding a listener is fine, silently losing them all is not. +case "${listener_count}" in + '' | null | 0) fail "no Gateway/platform listeners found in the rendered overlay — the render or the selector changed, so this test is checking nothing" ;; +esac +[ "${listener_count}" -ge 4 ] || + fail "expected at least 4 Gateway listeners in the prod overlay, found ${listener_count} — did the ascoachingogvaner listener patch stop applying?" + +# Compare the kinds as a JOINED STRING, never as an array. yq's `!=` does not +# do deep array equality — `(… | sort) != ["HTTPRoute"]` evaluates to true even +# for a correctly pinned listener, which reported all four as unpinned when this +# test was written. It failed closed there, but the same shape fails OPEN when +# the sense is reversed, so keep the comparison on strings. +unpinned="$(yq eval \ + '[.spec.listeners[] | select(((.allowedRoutes.kinds // []) | map(.kind) | sort | join(",")) != "HTTPRoute")] | .[].name' \ + "${gateway}" | tr '\n' ' ')" +[ -z "${unpinned// /}" ] || + fail "these Gateway listeners do not pin allowedRoutes.kinds to exactly [HTTPRoute]: ${unpinned}— an unpinned HTTPS listener also accepts GRPCRoute, which restrict-tenant-route-hostnames does not match." + +echo "Tenant route-kind boundary holds: RBAC grants HTTPRoute only and all ${listener_count} listeners pin kinds to HTTPRoute." diff --git a/scripts/validate-eks-ci-role-policy/main.go b/scripts/validate-eks-ci-role-policy/main.go index 7d8afe437..39c3293ca 100644 --- a/scripts/validate-eks-ci-role-policy/main.go +++ b/scripts/validate-eks-ci-role-policy/main.go @@ -622,6 +622,25 @@ const ( // every surviving selected object; #2741 removes only the already-protected // Headlamp PVC identity and retains the authorization-neutral changes above. // +// Measured for #2713 merged with exact main 6ebcb24f. Two independent renderers +// agree on this value: the required CI job on the approved toolchain (run +// 31974280947) and a local render. The branch's own earlier value was rendered +// against main 6d926e42 and never described this merge result. +// +// The reviewed reasoning for the change itself is unaffected by the merge. The +// complete authorization delta changes exactly ONE selected entry: +// +// rbac.authorization.k8s.io/v1 ClusterRole gateway-tenant-edit +// +// Its resources are narrowed from httproutes, grpcroutes, tcproutes, tlsroutes, +// udproutes and referencegrants to httproutes and referencegrants. The removed +// four route kinds bypass the HTTPRoute-only tenant hostname policy on the +// shared Gateway. Identity, labels, verbs, apiGroups, bindings and membership +// are otherwise byte-identical, so this is a strict privilege reduction. Main +// already carries the canonical restrict-tenant-route-hostnames policy and the +// removal of built-in edit aggregation; neither is duplicated by this change. +// Gateway listener-kind pins are outside this authorization projection and are +// covered by scripts/tests/test-tenant-route-hostname-boundary.sh. // Measured for the #2725 Umami provisioning repair merged with exact main // 6ebcb24f. Two independent renderers agree on this value: the required CI job // on the approved toolchain (run 31973534571) and a local render. @@ -646,6 +665,37 @@ const ( // withholds from every other consumer. The apps Flux Kustomization timeout moves // from 20m to 30m in both rendered cluster overlays. The ServiceAccount itself is // unchanged from main in canonical content. +// +// Re-approved after merging main into this branch. Both parents measured against +// the same exact main 6ebcb24f, but each described only its own delta — this +// branch the gateway-tenant-edit route-kind narrowing, main the #2725 Umami +// provisioning grant — so neither value describes the merge result, and the +// conflict could not be resolved by picking a side. Both records above are +// retained because both deltas are present in the merged surface: a strict +// privilege reduction on one selected ClusterRole, and the Lease-scoped Umami +// provisioning grant. They are independent and neither cancels the other, so the +// grant-bearing accounting recorded for each carries over unchanged and only the +// whole-surface digest moves. +// +// Measured against the merge result at c790f999. Two independent renderers agree +// on the value below: the required `🔐 Validate EKS Authorization` job (run +// 32019353067) and a local render on kubectl v1.36.1 / kustomize v5.8.1. +// +// Conservation behind the new digest: the whole-surface fingerprint is the ONLY +// control that moved. Both renderers report zero per-identity mismatches and +// zero missing resources against expectedRenderedHashes, so every individually +// approved entry — including both parents' deltas — is byte-identical to its +// recorded value and only the aggregate over the entry set changed. Both also +// report the same 35 unresolved-substitution notes, which are diagnostic rather +// than a control: a resource carrying `${…}` is forced into the aggregate, so +// its literal text is already covered by this digest. +// +// That symmetry corrects the assumption recorded before the measurement, which +// held that a local render could not approve this because it would be incomplete +// without the CI substitution inputs. The approved toolchain reports the same 35 +// notes and the same digest, so the two renders are equivalent here and the +// local one is a genuine second renderer rather than a degraded copy. +// // Re-approved after merging main d925654e into this branch. Both parents had // re-approved this constant independently — main for the #2725 Umami // provisioning grant recorded directly above, and this branch for the crossview @@ -675,7 +725,43 @@ const ( // surface, which is consistent with its content — documentation plus a Kyverno // policy description annotation, carrying no grant. That is evidence about // what did NOT change; it is not a second rendering of what did. -const expectedRenderedSurfaceSHA = "88667d39d19c923b0b84e3c0b4c548409a3990f360dfe9ddad22778eefdda328" +// +// Re-approved after merging main b9af3892 into this branch, whose own parent is +// 90cf208e. Both parents had re-approved this constant independently — this +// branch for the gateway-tenant-edit route-kind narrowing recorded above, and +// main for the crossview reload annotation recorded directly above — so neither +// parent's value describes the merge result and the conflict could not be +// resolved by picking a side. The merged surface is main's b9af3892 surface with +// this branch's single authorization delta applied on top: the strict privilege +// reduction on ClusterRole gateway-tenant-edit already described above. Main's +// grant-bearing accounting for the #2725 Umami provisioning repair reached this +// branch through the earlier merge and is unchanged by this one, so only the +// whole-surface digest moves. +// +// Two independent renderers agree on the value below, and each was first proved +// against clean main b9af3892 as a matched control: +// +// - the approved toolchain (checksum-verified kubectl v1.36.2 / Kustomize +// v5.8.1) running this validator, which reports the contract passing on +// clean main and this digest on the merge result; +// - a local render on kubectl v1.36.1 / Kustomize v5.8.1 through +// TestValidateAuthorizationAcceptsCommittedPolicy, which passes on clean +// main and reports this same digest on the merge result. +// +// The matched controls are what make the measurement trustworthy rather than a +// guess: both renderers reproduce every already-approved digest in main, so the +// one value they disagree with main about is the one this merge actually moved. +// +// Conservation behind the new digest: the whole-surface fingerprint is the ONLY +// control that moved. The run reports exactly one problem — this aggregate — and +// zero per-identity mismatches and zero missing resources against +// expectedRenderedHashes, so every individually approved entry is byte-identical +// to its recorded value. It also reports the same 35 unresolved-substitution +// notes that clean main reports; those are diagnostic rather than a control, +// emitted only alongside an aggregate mismatch to explain a hash that moved, and +// a resource carrying `${…}` is forced into the aggregate so its literal text is +// already covered by this digest. +const expectedRenderedSurfaceSHA = "b5b394181a8f2bc325bd427a90990fbc3872fb1f41f3d938f1e28f0aac1075f4" // authorizationOverlayPaths lists every independently reconciled production // layer where an object can grant privileges to the aws/aws service account.