Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
6 changes: 6 additions & 0 deletions .github/workflows/validate-scaffold.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
5 changes: 0 additions & 5 deletions k8s/bases/apps/homepage/http-route.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,6 @@ spec:
- ${domain}
rules:
- filters:
- type: RequestHeaderModifier
requestHeaderModifier:
set:
- name: X-Auth-Request-Redirect
value: https://${domain}/
- type: ResponseHeaderModifier
responseHeaderModifier:
set:
Expand Down
5 changes: 0 additions & 5 deletions k8s/bases/infrastructure/controllers/cilium/http-route.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
5 changes: 0 additions & 5 deletions k8s/bases/infrastructure/controllers/coroot/http-route.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
14 changes: 14 additions & 0 deletions k8s/bases/infrastructure/controllers/dex/helm-release.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
206 changes: 206 additions & 0 deletions scripts/tests/test-oauth2-proxy-sso-redirects.sh
Original file line number Diff line number Diff line change
@@ -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://<host>/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}")"
10 changes: 6 additions & 4 deletions scripts/validate-observability-option.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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[]?;
Expand Down
Loading