diff --git a/infrastructure/core/coredns-custom.yaml b/infrastructure/core/coredns-custom.yaml index 0f39506f..9c83c009 100644 --- a/infrastructure/core/coredns-custom.yaml +++ b/infrastructure/core/coredns-custom.yaml @@ -19,6 +19,7 @@ data: 192.168.22.9 cd.bstein.dev 192.168.22.9 chat.ai.bstein.dev 192.168.22.9 chat.bstein.dev + 192.168.22.9 chat.hermes.bstein.dev 192.168.22.9 ci.bstein.dev 192.168.22.9 cloud.bstein.dev 192.168.22.9 health.bstein.dev @@ -46,6 +47,7 @@ data: 192.168.22.9 wolf.bstein.dev 192.168.22.9 tasks.bstein.dev 192.168.22.9 triage.bstein.dev + 192.168.22.9 triage.hermes.bstein.dev 192.168.22.9 vault.bstein.dev fallthrough } diff --git a/services/hermes/agent-certificate.yaml b/services/hermes/agent-certificate.yaml index 0733bc83..6630ae75 100644 --- a/services/hermes/agent-certificate.yaml +++ b/services/hermes/agent-certificate.yaml @@ -13,3 +13,7 @@ spec: - agent.hermes.bstein.dev - chat.bstein.dev - triage.bstein.dev + # Legacy hosts stay on the certificate until they are retired on purpose; + # the rename in #34 must not break links or sessions already in flight. + - chat.hermes.bstein.dev + - triage.hermes.bstein.dev diff --git a/services/hermes/agent-ingress.yaml b/services/hermes/agent-ingress.yaml index 8d636d20..775adbce 100644 --- a/services/hermes/agent-ingress.yaml +++ b/services/hermes/agent-ingress.yaml @@ -90,6 +90,8 @@ spec: - agent.hermes.bstein.dev - chat.bstein.dev - triage.bstein.dev + - chat.hermes.bstein.dev + - triage.hermes.bstein.dev secretName: hermes-sites-tls rules: - host: chat.bstein.dev @@ -112,3 +114,26 @@ spec: name: oauth2-proxy-hermes-triage port: name: http + # Legacy hosts serve the same backends so the rename is additive. They are + # kept until an explicit retirement change, not redirected: oauth2-proxy + # cookies are host-bound, so a redirect would silently drop the session. + - host: chat.hermes.bstein.dev + http: + paths: + - path: / + pathType: Prefix + backend: + service: + name: oauth2-proxy-hermes-chat + port: + name: http + - host: triage.hermes.bstein.dev + http: + paths: + - path: / + pathType: Prefix + backend: + service: + name: oauth2-proxy-hermes-triage + port: + name: http diff --git a/services/keycloak/bootstrap-jobs/hermes-access-oidc-client-job.yaml b/services/keycloak/bootstrap-jobs/hermes-access-oidc-client-job.yaml index 7eae3a1e..b2020e8e 100644 --- a/services/keycloak/bootstrap-jobs/hermes-access-oidc-client-job.yaml +++ b/services/keycloak/bootstrap-jobs/hermes-access-oidc-client-job.yaml @@ -3,7 +3,7 @@ apiVersion: batch/v1 kind: Job metadata: - name: hermes-access-oidc-client-ensure-10 + name: hermes-access-oidc-client-ensure-11 namespace: sso spec: backoffLimit: 3 diff --git a/services/keycloak/scripts/hermes_access_oidc_ensure.sh b/services/keycloak/scripts/hermes_access_oidc_ensure.sh index 7fcc4569..2e54603c 100755 --- a/services/keycloak/scripts/hermes_access_oidc_ensure.sh +++ b/services/keycloak/scripts/hermes_access_oidc_ensure.sh @@ -84,10 +84,24 @@ ensure_proxy_client() { client_id="$1" public_url="$2" vault_path="$3" + # Optional legacy host kept registered alongside the canonical one during a + # hostname rename. Keycloak matches redirect_uri exactly, so dropping the old + # entry turns every in-flight login into "Invalid parameter: redirect_uri". + legacy_url="${4:-}" + if [ -n "${legacy_url}" ]; then + origins="$(jq -nc --arg a "${public_url}" --arg b "${legacy_url}" '[$a,$b]')" + else + origins="$(jq -nc --arg a "${public_url}" '[$a]')" + fi + redirect_uris="$(printf '%s' "${origins}" | jq -c 'map(. + "/oauth2/callback")')" + # Keycloak takes post-logout origins as one "##"-delimited string. + post_logout="$(printf '%s' "${origins}" | jq -r 'join("##")')" payload="$(jq -nc \ --arg client_id "${client_id}" \ - --arg redirect_uri "${public_url}/oauth2/callback" \ + --argjson redirect_uris "${redirect_uris}" \ + --argjson web_origins "${origins}" \ --arg web_origin "${public_url}" \ + --arg post_logout "${post_logout}" \ '{ clientId:$client_id, name:$client_id, @@ -98,13 +112,13 @@ ensure_proxy_client() { implicitFlowEnabled:false, directAccessGrantsEnabled:false, serviceAccountsEnabled:false, - redirectUris:[$redirect_uri], - webOrigins:[$web_origin], + redirectUris:$redirect_uris, + webOrigins:$web_origins, rootUrl:$web_origin, baseUrl:"/", attributes:{ "pkce.code.challenge.method":"S256", - "post.logout.redirect.uris":$web_origin, + "post.logout.redirect.uris":$post_logout, "access.token.lifespan":"1200" } }')" @@ -327,8 +341,10 @@ ensure_telegram_config() { } ensure_hermes_owner -ensure_proxy_client "hermes-chat-proxy" "https://chat.bstein.dev" "hermes/chat-oidc" +ensure_proxy_client "hermes-chat-proxy" "https://chat.bstein.dev" "hermes/chat-oidc" \ + "https://chat.hermes.bstein.dev" ensure_proxy_client "hermes-agent-proxy" "https://agent.hermes.bstein.dev" "hermes/agent-oidc" -ensure_proxy_client "hermes-triage-proxy" "https://triage.bstein.dev" "hermes/triage-oidc" +ensure_proxy_client "hermes-triage-proxy" "https://triage.bstein.dev" "hermes/triage-oidc" \ + "https://triage.hermes.bstein.dev" ensure_service_account_client "hermes-automation" "hermes/developer-keycloak" ensure_telegram_config diff --git a/testing/tests/test_hermes_public_host_continuity.py b/testing/tests/test_hermes_public_host_continuity.py new file mode 100644 index 00000000..eb62c696 --- /dev/null +++ b/testing/tests/test_hermes_public_host_continuity.py @@ -0,0 +1,153 @@ +"""Public chat/triage hostnames stay served on every layer during a rename. + +PR #34 renamed the chat and triage public hosts in place: the legacy names left +the certificate SANs, the Ingress rules, the CoreDNS overrides and the Keycloak +ensure script in one change. The legacy hosts began answering 404 with Traefik's +default certificate while the renamed hosts could not finish a login, because +Keycloak matches ``redirect_uri`` exactly and still held the old callback. Chat +and triage were unreachable on every hostname at once. + +These tests pin the contract that makes that outage impossible to reintroduce +silently: a public host is either served by all four layers or by none of them. +""" + +from __future__ import annotations + +import re +from pathlib import Path + +import pytest +import yaml + +REPO = Path(__file__).parents[2] +HERMES = REPO / "services/hermes" +KEYCLOAK = REPO / "services/keycloak" +COREDNS = REPO / "infrastructure/core/coredns-custom.yaml" +ENSURE_SCRIPT = KEYCLOAK / "scripts/hermes_access_oidc_ensure.sh" + +# Every hostname the chat/triage surfaces must answer on, with the oauth2-proxy +# backend that serves it. Both the renamed and the legacy names belong here +# until a deliberate retirement change removes a row from this table. +PUBLIC_HOSTS = { + "chat.bstein.dev": "oauth2-proxy-hermes-chat", + "chat.hermes.bstein.dev": "oauth2-proxy-hermes-chat", + "triage.bstein.dev": "oauth2-proxy-hermes-triage", + "triage.hermes.bstein.dev": "oauth2-proxy-hermes-triage", +} + +# The agent surface was deliberately untouched by the rename. +AGENT_HOST = "agent.hermes.bstein.dev" + + +def _docs(path: Path) -> list[dict]: + return [doc for doc in yaml.safe_load_all(path.read_text()) if doc] + + +def _named(path: Path, kind: str, name: str) -> dict: + for doc in _docs(path): + if doc.get("kind") == kind and doc["metadata"]["name"] == name: + return doc + raise AssertionError(f"{kind}/{name} missing from {path}") + + +@pytest.fixture(scope="module") +def certificate() -> dict: + return _named(HERMES / "agent-certificate.yaml", "Certificate", "hermes-sites-tls") + + +@pytest.fixture(scope="module") +def sites_ingress() -> dict: + return _named(HERMES / "agent-ingress.yaml", "Ingress", "hermes-sites") + + +@pytest.fixture(scope="module") +def coredns_hosts() -> set[str]: + block = yaml.safe_load(COREDNS.read_text())["data"]["bstein-dev.server"] + return { + line.split()[1] + for line in block.splitlines() + if len(line.split()) == 2 and re.fullmatch(r"[\d.]+", line.split()[0]) + } + + +@pytest.fixture(scope="module") +def ensure_script() -> str: + return ENSURE_SCRIPT.read_text() + + +@pytest.mark.parametrize("host", sorted(PUBLIC_HOSTS)) +def test_host_is_on_the_shared_certificate(host: str, certificate: dict): + """A host without a SAN serves Traefik's default self-signed certificate.""" + assert host in certificate["spec"]["dnsNames"] + + +@pytest.mark.parametrize("host", sorted(PUBLIC_HOSTS)) +def test_host_resolves_inside_the_cluster(host: str, coredns_hosts: set[str]): + assert host in coredns_hosts + + +@pytest.mark.parametrize("host", sorted(PUBLIC_HOSTS)) +def test_host_has_an_ingress_rule_and_tls_entry(host: str, sites_ingress: dict): + """A host without a rule answers 404 even though DNS and TLS look healthy.""" + spec = sites_ingress["spec"] + assert host in {name for entry in spec["tls"] for name in entry["hosts"]} + + rule = next((item for item in spec["rules"] if item["host"] == host), None) + assert rule is not None, f"no hermes-sites rule serves {host}" + + backends = { + path["backend"]["service"]["name"] for path in rule["http"]["paths"] + } + assert backends == {PUBLIC_HOSTS[host]} + + +@pytest.mark.parametrize("host", sorted(PUBLIC_HOSTS)) +def test_host_is_registered_with_keycloak(host: str, ensure_script: str): + """Keycloak matches redirect_uri exactly, so every served host needs one.""" + assert f"https://{host}" in ensure_script + + +def test_agent_surface_was_not_touched_by_the_rename( + certificate: dict, coredns_hosts: set[str], ensure_script: str +): + assert AGENT_HOST in certificate["spec"]["dnsNames"] + assert AGENT_HOST in coredns_hosts + assert f"https://{AGENT_HOST}" in ensure_script + + +def test_ensure_script_registers_legacy_and_renamed_origins_together( + ensure_script: str, +): + """The renamed proxies must carry both origins; the agent proxy only one.""" + for client, canonical, legacy in ( + ("hermes-chat-proxy", "chat.bstein.dev", "chat.hermes.bstein.dev"), + ("hermes-triage-proxy", "triage.bstein.dev", "triage.hermes.bstein.dev"), + ): + call = re.search( + rf'ensure_proxy_client "{client}".*?(?=\nensure_)', + ensure_script, + re.DOTALL, + ) + assert call, f"{client} is never ensured" + assert f"https://{canonical}" in call.group(0) + assert f"https://{legacy}" in call.group(0) + + +def test_ensure_job_is_rerun_whenever_the_script_changes(): + """The Job is immutable, so a stale name silently skips the rerun.""" + job = _named( + KEYCLOAK / "bootstrap-jobs/hermes-access-oidc-client-job.yaml", + "Job", + # The suffix moves with every rerun; resolve it from the manifest. + _job_name(), + ) + assert job["spec"]["template"]["spec"]["containers"][0]["command"] == [ + "/scripts/hermes_access_oidc_ensure.sh" + ] + + +def _job_name() -> str: + path = KEYCLOAK / "bootstrap-jobs/hermes-access-oidc-client-job.yaml" + name = yaml.safe_load(path.read_text())["metadata"]["name"] + assert re.fullmatch(r"hermes-access-oidc-client-ensure-\d+", name), name + return name