diff --git a/.github/workflows/validate-scaffold.yaml b/.github/workflows/validate-scaffold.yaml index a3350d3..fc85afa 100644 --- a/.github/workflows/validate-scaffold.yaml +++ b/.github/workflows/validate-scaffold.yaml @@ -59,6 +59,12 @@ jobs: # renders the explicit local/prod profiles selected through KSail. run: ./scripts/validate-observability-option.sh + - name: 🔐 Validate SSO deep-link redirects + # Every host behind oauth2-proxy finishes its login on its own host and + # returns the user to the page they asked for: Dex must accept exactly + # those callbacks, with no fixed redirect_url or per-route redirect header. + run: ./scripts/tests/test-oauth2-proxy-sso-redirects.sh + - name: 🏷️ Validate default-off recommended workload labels # Existing instances keep their default paths. Exercise the explicit # profiles and the exact immutable shared-policy contract. diff --git a/k8s/bases/apps/homepage/http-route.yaml b/k8s/bases/apps/homepage/http-route.yaml index e66189a..0a1cbe2 100644 --- a/k8s/bases/apps/homepage/http-route.yaml +++ b/k8s/bases/apps/homepage/http-route.yaml @@ -12,11 +12,6 @@ spec: - ${domain} rules: - filters: - - type: RequestHeaderModifier - requestHeaderModifier: - set: - - name: X-Auth-Request-Redirect - value: https://${domain}/ - type: ResponseHeaderModifier responseHeaderModifier: set: diff --git a/k8s/bases/infrastructure/controllers/cilium/http-route.yaml b/k8s/bases/infrastructure/controllers/cilium/http-route.yaml index bb03afc..765d450 100644 --- a/k8s/bases/infrastructure/controllers/cilium/http-route.yaml +++ b/k8s/bases/infrastructure/controllers/cilium/http-route.yaml @@ -20,11 +20,6 @@ spec: - hubble.${domain} rules: - filters: - - type: RequestHeaderModifier - requestHeaderModifier: - set: - - name: X-Auth-Request-Redirect - value: https://hubble.${domain}/ - type: ResponseHeaderModifier responseHeaderModifier: set: diff --git a/k8s/bases/infrastructure/controllers/coroot/http-route.yaml b/k8s/bases/infrastructure/controllers/coroot/http-route.yaml index 1f6d638..b9be755 100644 --- a/k8s/bases/infrastructure/controllers/coroot/http-route.yaml +++ b/k8s/bases/infrastructure/controllers/coroot/http-route.yaml @@ -24,11 +24,6 @@ spec: - observability.${domain} rules: - filters: - - type: RequestHeaderModifier - requestHeaderModifier: - set: - - name: X-Auth-Request-Redirect - value: https://observability.${domain}/ - type: ResponseHeaderModifier responseHeaderModifier: set: diff --git a/k8s/bases/infrastructure/controllers/dex/helm-release.yaml b/k8s/bases/infrastructure/controllers/dex/helm-release.yaml index ccd0f28..c6c10d2 100644 --- a/k8s/bases/infrastructure/controllers/dex/helm-release.yaml +++ b/k8s/bases/infrastructure/controllers/dex/helm-release.yaml @@ -70,6 +70,20 @@ spec: - "https://budget.${domain}/oidc/callback" - "https://vault.${domain}/ui/vault/auth/oidc/oidc/callback" - "https://grafana.${domain}/login/generic_oauth" + # oauth2-proxy's callback on every host it fronts, so it can finish a + # login on the host the user started from and return them to the page + # they asked for. Dex matches callbacks exactly, so each host needs its + # own entry: scripts/tests/test-oauth2-proxy-sso-redirects.sh fails + # when an HTTPRoute puts a host behind oauth2-proxy without one, or + # when an entry outlives its route in every profile and provider. These + # stay after the entries above: the observability-coroot components + # patch this list by index. + - "https://${domain}/oauth2/callback" + - "https://alertmanager.${domain}/oauth2/callback" + - "https://hubble.${domain}/oauth2/callback" + - "https://observability.${domain}/oauth2/callback" + - "https://opencost.${domain}/oauth2/callback" + - "https://prometheus.${domain}/oauth2/callback" trustedPeers: - kubectl - name: kubectl diff --git a/k8s/bases/infrastructure/controllers/kube-prometheus-stack/http-route-alertmanager.yaml b/k8s/bases/infrastructure/controllers/kube-prometheus-stack/http-route-alertmanager.yaml index 91609e4..7e41b51 100644 --- a/k8s/bases/infrastructure/controllers/kube-prometheus-stack/http-route-alertmanager.yaml +++ b/k8s/bases/infrastructure/controllers/kube-prometheus-stack/http-route-alertmanager.yaml @@ -19,11 +19,6 @@ spec: - alertmanager.${domain} rules: - filters: - - type: RequestHeaderModifier - requestHeaderModifier: - set: - - name: X-Auth-Request-Redirect - value: https://alertmanager.${domain}/ - type: ResponseHeaderModifier responseHeaderModifier: set: diff --git a/k8s/bases/infrastructure/controllers/kube-prometheus-stack/http-route-prometheus.yaml b/k8s/bases/infrastructure/controllers/kube-prometheus-stack/http-route-prometheus.yaml index 4817652..97c8856 100644 --- a/k8s/bases/infrastructure/controllers/kube-prometheus-stack/http-route-prometheus.yaml +++ b/k8s/bases/infrastructure/controllers/kube-prometheus-stack/http-route-prometheus.yaml @@ -19,11 +19,6 @@ spec: - prometheus.${domain} rules: - filters: - - type: RequestHeaderModifier - requestHeaderModifier: - set: - - name: X-Auth-Request-Redirect - value: https://prometheus.${domain}/ - type: ResponseHeaderModifier responseHeaderModifier: set: diff --git a/k8s/bases/infrastructure/controllers/oauth2-proxy/helm-release.yaml b/k8s/bases/infrastructure/controllers/oauth2-proxy/helm-release.yaml index 20d0ac4..39de94a 100644 --- a/k8s/bases/infrastructure/controllers/oauth2-proxy/helm-release.yaml +++ b/k8s/bases/infrastructure/controllers/oauth2-proxy/helm-release.yaml @@ -81,7 +81,15 @@ spec: email_domains = ["*"] cookie_domains=[".${domain}"] whitelist_domains = ["${domain}", ".${domain}"] - redirect_url = "https://oauth2-proxy.${domain}/oauth2/callback" + # No redirect_url, deliberately: oauth2-proxy then finishes each login at + # /oauth2/callback on the host the user started from (https, because + # cookie_secure is set), and returns them to the page they asked for as a + # path on that same host. A fixed callback host would bring every login + # back through one host, where that path belongs to no app. Dex accepts + # these callbacks because every fronted host is registered on its + # public-client, which scripts/tests/test-oauth2-proxy-sso-redirects.sh + # enforces. + cookie_secure = true upstreams = [ "http://auth-proxy.oauth2-proxy.svc.cluster.local:80" ] # upstream_timeout — max time to await upstream response headers # (default 30s). Raised to 90s so KEDA scale-to-zero apps behind this diff --git a/k8s/bases/infrastructure/controllers/opencost/http-route.yaml b/k8s/bases/infrastructure/controllers/opencost/http-route.yaml index 4e98b15..f96a04c 100644 --- a/k8s/bases/infrastructure/controllers/opencost/http-route.yaml +++ b/k8s/bases/infrastructure/controllers/opencost/http-route.yaml @@ -21,11 +21,6 @@ spec: - opencost.${domain} rules: - filters: - - type: RequestHeaderModifier - requestHeaderModifier: - set: - - name: X-Auth-Request-Redirect - value: https://opencost.${domain}/ - type: ResponseHeaderModifier responseHeaderModifier: set: diff --git a/k8s/components/observability-coroot/remove-kps-ui/kustomization.yaml b/k8s/components/observability-coroot/remove-kps-ui/kustomization.yaml index 8b26ab4..5dd1ec4 100644 --- a/k8s/components/observability-coroot/remove-kps-ui/kustomization.yaml +++ b/k8s/components/observability-coroot/remove-kps-ui/kustomization.yaml @@ -79,7 +79,19 @@ patches: kind: HelmRelease name: dex namespace: dex + # Prometheus and Alertmanager oauth2-proxy login callbacks first (highest index + # first, after remove-opencost-references took OpenCost's), then Grafana's own. patch: |- + - op: test + path: /spec/values/config/staticClients/0/redirectURIs/9 + value: "https://prometheus.${domain}/oauth2/callback" + - op: remove + path: /spec/values/config/staticClients/0/redirectURIs/9 + - op: test + path: /spec/values/config/staticClients/0/redirectURIs/6 + value: "https://alertmanager.${domain}/oauth2/callback" + - op: remove + path: /spec/values/config/staticClients/0/redirectURIs/6 - op: test path: /spec/values/config/staticClients/0/redirectURIs/4 value: "https://grafana.${domain}/login/generic_oauth" diff --git a/k8s/components/observability-coroot/remove-opencost-references/kustomization.yaml b/k8s/components/observability-coroot/remove-opencost-references/kustomization.yaml index 1035a74..1e95522 100644 --- a/k8s/components/observability-coroot/remove-opencost-references/kustomization.yaml +++ b/k8s/components/observability-coroot/remove-opencost-references/kustomization.yaml @@ -334,3 +334,17 @@ patches: value: opencost - op: remove path: /spec/match/namespaceSelector/matchExpressions/0/values/14 + # OpenCost's oauth2-proxy login callback on Dex's public-client, which this profile no + # longer routes. remove-kps-ui then removes later entries by index, so it must run after. + - target: + group: helm.toolkit.fluxcd.io + version: v2 + kind: HelmRelease + name: dex + namespace: dex + patch: |- + - op: test + path: /spec/values/config/staticClients/0/redirectURIs/9 + value: "https://opencost.${domain}/oauth2/callback" + - op: remove + path: /spec/values/config/staticClients/0/redirectURIs/9 diff --git a/scripts/tests/test-oauth2-proxy-sso-redirects.sh b/scripts/tests/test-oauth2-proxy-sso-redirects.sh new file mode 100755 index 0000000..9230f2b --- /dev/null +++ b/scripts/tests/test-oauth2-proxy-sso-redirects.sh @@ -0,0 +1,206 @@ +#!/usr/bin/env bash +# Guards the oauth2-proxy login round-trip on every host it fronts. +# +# oauth2-proxy finishes a login at /oauth2/callback on the host the user started +# from, then returns them to the page they asked for as a path on that host. So +# oauth2-proxy must not be given a fixed redirect_url, and no route may replace +# that page with a fixed X-Auth-Request-Redirect. Dex only accepts a +# callback it has registered exactly, so every host an HTTPRoute sends to +# oauth2-proxy needs `https:///oauth2/callback` on Dex's public-client. +# Without it, logging in on that host stops at Dex with "Unregistered +# redirect_uri". The reverse also holds: an entry whose route is gone is a +# callback Dex keeps accepting for no reason, so it must be removed with the +# route. +# +# Hosts are read from the rendered provider overlays, the trees Flux applies, +# not from raw files: a route a provider does not include needs no callback, +# and a route only a provider patch adds still does. +# +# Ported from devantler-tech/platform (#3169), where the fix was verified live. +set -euo pipefail + +script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +repo_root="$(cd "${script_dir}/../.." && pwd)" +readonly repo_root + +readonly providers=(hetzner docker) +# Each observability profile is its own set of layers (scripts/validate-observability-option.sh), +# and the profiles put different hosts behind oauth2-proxy, so every profile is rendered. +readonly profiles=(default coroot) +layers_of() { + case "$1" in + default) echo infrastructure/controllers infrastructure apps ;; + coroot) echo infrastructure-controllers-coroot infrastructure-coroot apps-coroot ;; + esac +} +readonly dex_file='k8s/bases/infrastructure/controllers/dex/helm-release.yaml' + +workdir="$(mktemp -d)" +trap 'rm -rf "${workdir}"' EXIT + +fail() { + printf '::error::%s\n' "$1" >&2 + exit 1 +} + +# Every HTTPRoute that sends traffic to the oauth2-proxy Service. A backendRef +# with no namespace means the route's own namespace, and one with no kind means +# a core Service, per the Gateway API defaults. `$ns` is a yq variable, not a +# shell one. +# shellcheck disable=SC2016 +readonly fronted_routes=' + select(.kind == "HTTPRoute") + | .metadata.namespace as $ns + | select( + [ .spec.rules[]?.backendRefs[]? + | select( + (.group // "") == "" + and (.kind // "Service") == "Service" + and .name == "oauth2-proxy" + and (.namespace // $ns) == "oauth2-proxy") + ] | length > 0)' + +hosts_of() { + yq eval -N "${fronted_routes} | .spec.hostnames[]" "$1" | sort -u +} + +# --- Self-test the route selector ------------------------------------------ +# A selector that matches nothing would make every check below pass over an +# empty set, so prove it picks exactly the routes it should on a fixture first. +cat >"${workdir}/fixture.yaml" <<'EOF' +apiVersion: gateway.networking.k8s.io/v1 +kind: HTTPRoute +metadata: {name: fronted, namespace: app} +spec: + hostnames: [fronted.example] + rules: + - backendRefs: [{name: oauth2-proxy, namespace: oauth2-proxy, port: 80}] +--- +apiVersion: gateway.networking.k8s.io/v1 +kind: HTTPRoute +metadata: {name: same-namespace, namespace: oauth2-proxy} +spec: + hostnames: [same-namespace.example] + rules: + - backendRefs: [{name: oauth2-proxy, port: 80}] +--- +apiVersion: gateway.networking.k8s.io/v1 +kind: HTTPRoute +metadata: {name: direct, namespace: app} +spec: + hostnames: [direct.example] + rules: + - backendRefs: [{name: app, port: 80}] +--- +apiVersion: gateway.networking.k8s.io/v1 +kind: HTTPRoute +metadata: {name: other-namespace, namespace: app} +spec: + hostnames: [other-namespace.example] + rules: + - backendRefs: [{name: oauth2-proxy, port: 80}] +EOF +fixture_hosts="$(hosts_of "${workdir}/fixture.yaml" | tr '\n' ' ')" +[[ "${fixture_hosts}" == 'fronted.example same-namespace.example ' ]] || + fail "the oauth2-proxy route selector picked '${fixture_hosts}' from its fixture instead of 'fronted.example same-namespace.example'; fix the selector before trusting this test" + +# --- Render every layer Flux reconciles -------------------------------------- +all_hosts="${workdir}/all-hosts.txt" +: >"${all_hosts}" + +profile_providers=() +for profile in "${profiles[@]}"; do + for p in "${providers[@]}"; do + profile_providers+=("${p}:${profile}") + done +done + +for profile_provider in "${profile_providers[@]}"; do + profile="${profile_provider#*:}" + provider="${profile_provider%%:*} (${profile} profile)" + rendered="${workdir}/${profile_provider/:/-}.yaml" + : >"${rendered}" + read -ra layers <<<"$(layers_of "${profile}")" + for layer in "${layers[@]}"; do + dir="${repo_root}/k8s/providers/${profile_provider%%:*}/${layer}" + # Check kubectl's own status: in a { kubectl; printf; } group the status is + # printf's, so a layer that failed to build would be read as an empty one. + kubectl kustomize "${dir}" >>"${rendered}" 2>"${workdir}/render.err" || + fail "k8s/providers/${profile_provider%%:*}/${layer} failed to build: $(tail -5 "${workdir}/render.err")" + printf '\n---\n' >>"${rendered}" + done + + # A route that names no hostname inherits the listener's, so this test cannot + # tell which callback it needs. Refuse it rather than skip it. + unnamed="$(yq eval -N "${fronted_routes} | select((.spec.hostnames // []) | length == 0) | .metadata.namespace + \"/\" + .metadata.name" "${rendered}")" + [[ -z "${unnamed}" ]] || + fail "${provider}: these HTTPRoutes send traffic to oauth2-proxy without naming a hostname, so their login callback cannot be checked: ${unnamed//$'\n'/, }" + + # oauth2-proxy prefers an X-Auth-Request-Redirect header over the page the + # user asked for, so a route that sets one sends every login to that one URL. + fixed="$(yq eval -N "${fronted_routes} | select([.spec.rules[]?.filters[]? | select(.type == \"RequestHeaderModifier\") | ((.requestHeaderModifier.set // []) + (.requestHeaderModifier.add // []))[] | select(.name | downcase == \"x-auth-request-redirect\")] | length > 0) | .metadata.namespace + \"/\" + .metadata.name" "${rendered}")" + [[ -z "${fixed}" ]] || + fail "${provider}: these HTTPRoutes send every login through oauth2-proxy to one fixed URL, dropping the page the user asked for. Remove their X-Auth-Request-Redirect header: ${fixed//$'\n'/, }" + + hosts="$(hosts_of "${rendered}")" + if [[ "${profile_provider}" == hetzner:* && -z "${hosts}" ]]; then + fail 'the production overlays put no host behind oauth2-proxy; either the SSO wiring changed shape or this test stopped seeing it' + fi + + # oauth2-proxy's own route is an HTTPRoute in the rendered tree here, so the + # selector above already sees its host. Should the chart render that route + # instead (gatewayApi values), read its hostnames from the HelmRelease too. + own_hosts="$(yq eval -N 'select(.kind == "HelmRelease" and .metadata.name == "oauth2-proxy" and .metadata.namespace == "oauth2-proxy") | .spec.values.gatewayApi.hostnames[]?' "${rendered}")" + grep -qx "oauth2-proxy\.[^.]*.*" <<<"$(printf '%s\n%s\n' "${hosts}" "${own_hosts}")" || + fail "${provider}: no route puts oauth2-proxy's own host behind it; its callback host can no longer be derived" + + # oauth2-proxy must finish each login on the host it started from. A fixed + # redirect_url brings every login back through one host, where the path + # oauth2-proxy returns the user to belongs to no app. The callback it derives + # instead is https — the scheme Dex registers — only while cookie_secure is set. + o2p="$(yq eval -N 'select(.kind == "HelmRelease" and .metadata.name == "oauth2-proxy" and .metadata.namespace == "oauth2-proxy") | .spec.values' "${rendered}")" + o2p_config="$(yq eval -N '.config.configFile // ""' - <<<"${o2p}")" + [[ -n "${o2p_config}" ]] || + fail "${provider}: the oauth2-proxy HelmRelease has no config.configFile; its login settings moved, so this test is checking nothing" + if grep -Eq '^[[:space:]]*redirect_url[[:space:]]*=' <<<"${o2p_config}" || + grep -Eq 'redirect[-_]url' <<<"$(yq eval -N '.extraArgs // ""' - <<<"${o2p}")"; then + fail "${provider}: oauth2-proxy sets a fixed redirect_url, so every login finishes on one host and returns the user to a path on a host that serves no app. Remove it from k8s/bases/infrastructure/controllers/oauth2-proxy/helm-release.yaml; oauth2-proxy then finishes each login on the host it started from" + fi + grep -Eq '^[[:space:]]*cookie_secure[[:space:]]*=[[:space:]]*true[[:space:]]*$' <<<"${o2p_config}" || + fail "${provider}: oauth2-proxy must set cookie_secure = true: it is what makes the login callback it derives https, the only scheme Dex has registered" + + registered="$(yq eval -N 'select(.kind == "HelmRelease" and .metadata.name == "dex" and .metadata.namespace == "dex") | .spec.values.config.staticClients[] | select(.id == "public-client") | .redirectURIs[]' "${rendered}")" + [[ -n "${registered}" ]] || + fail "${provider}: no Dex public-client redirectURIs were rendered; the Dex client moved or was renamed, so this test is checking nothing" + + missing=() + while IFS= read -r host; do + [[ -n "${host}" ]] || continue + grep -qxF -- "https://${host}/oauth2/callback" <<<"${registered}" || + missing+=("https://${host}/oauth2/callback") + done < <(printf '%s\n%s\n' "${hosts}" "${own_hosts}" | sort -u) + + if ((${#missing[@]} > 0)); then + fail "${provider}: these hosts are behind oauth2-proxy but Dex does not accept their login callback, so signing in there fails with 'Unregistered redirect_uri'. Add each to the public-client redirectURIs in ${dex_file}: ${missing[*]}" + fi + + printf '%s\n%s\n' "${hosts}" "${own_hosts}" >>"${all_hosts}" + printf '%s\n' "${registered}" >"${workdir}/${profile_provider/:/-}-registered.txt" +done + +# --- No callback outlives its route ---------------------------------------- +# The Dex client is shared by both providers, so an entry is live while any +# provider still fronts its host. +sort -u -o "${all_hosts}" "${all_hosts}" +stale=() +while IFS= read -r uri; do + [[ "${uri}" =~ ^https://([^/]+)/oauth2/callback$ ]] || continue + grep -qxF -- "${BASH_REMATCH[1]}" "${all_hosts}" || stale+=("${uri}") +done < <(sort -u "${workdir}"/*-registered.txt) + +if ((${#stale[@]} > 0)); then + fail "Dex still accepts an oauth2-proxy login callback on hosts no HTTPRoute sends to oauth2-proxy. Remove each from the public-client redirectURIs in ${dex_file}: ${stale[*]}" +fi + +printf 'PASS: a login through oauth2-proxy returns to the page it started from on all %d hosts it fronts, and Dex accepts no other oauth2-proxy callback\n' \ + "$(grep -c . "${all_hosts}")" diff --git a/scripts/validate-observability-option.sh b/scripts/validate-observability-option.sh index e03acee..64be86b 100755 --- a/scripts/validate-observability-option.sh +++ b/scripts/validate-observability-option.sh @@ -797,10 +797,12 @@ assert_coroot_sso_controller_contract() { select(.spec.hostnames == ["observability.${domain}"]) | select(.spec.rules | length == 1) | select(.spec.rules[0].backendRefs == [{"name":"oauth2-proxy","namespace":"oauth2-proxy","port":80}]) | - select(any(.spec.rules[0].filters[]?; - .type == "RequestHeaderModifier" and - any(.requestHeaderModifier.set[]?; - .name == "X-Auth-Request-Redirect" and .value == "https://observability.${domain}/"))) | + # No fixed X-Auth-Request-Redirect: oauth2-proxy returns the user to the + # page they asked for (scripts/tests/test-oauth2-proxy-sso-redirects.sh). + select(all(.spec.rules[0].filters[]?; + .type != "RequestHeaderModifier" or + ([((.requestHeaderModifier.set // []) + (.requestHeaderModifier.add // []))[] | + select(.name | ascii_downcase == "x-auth-request-redirect")] | length == 0))) | select(any(.spec.rules[0].filters[]?; .type == "ResponseHeaderModifier" and any(.responseHeaderModifier.set[]?;