diff --git a/docs/federation.md b/docs/federation.md index 1685ed5..34b6e13 100644 --- a/docs/federation.md +++ b/docs/federation.md @@ -31,15 +31,56 @@ Envoy terminates the client mTLS certificate and forwards the verified cert PEM 2. On the **Frigate path** (`host == FRIGATE_HOST`) the existing behavior is preserved: inject `X-Proxy-Secret`. Federation does **not** run there. 3. If the request host is **claimed by a provider** (its `hosts` allowlist, see - `providers.yaml`), the service **federates**: it derives a stable subject from - the cert, runs the `get_bearer` decision ladder, and either injects - `Authorization: Bearer ` upstream, lets the client's own bearer through - unchanged, or denies. + `providers.yaml`), the service derives a stable subject from the cert, then: + - Vikunja's **own OIDC paths** (the frontend callback route and the backend + endpoint that redeems a code) are let through **untouched** — see + [Browser bootstrap](#browser-bootstrap-document-navigations) below. + - A **document navigation** (a real browser address-bar/link hit, not an + API/XHR call) gets the browser-bootstrap `denied_response` instead of the + silent ladder — see below. + - Everything else **federates**: it runs the `get_bearer` decision ladder, + and either injects `Authorization: Bearer ` upstream, lets the + client's own bearer through unchanged, or denies. 4. Any other allowed host passes through with **no header added**. Host scoping is an allowlist on purpose: attaching this ext_authz to a new vhost must not silently start injecting some other backend's bearer over the client's own credential. +## Browser bootstrap (document navigations) + +`get_bearer`'s `Authorization` header is added to the request Envoy forwards +*upstream* — invisible to a browser's own JS. A browser SPA hitting Vikunja +directly therefore never becomes "logged in" from that alone: it still falls +through to Vikunja's own client-side OIDC login, which redirects to this +service's OP `authorization_endpoint` — a route that does not exist (see +`op/routes.py`: codes are minted server-to-server only, by design). + +Reconciling this without any Envoy/Contour changes: `envoy_authz.federator. +browser_bootstrap` detects a top-level navigation (`Sec-Fetch-Dest: document`, +falling back to `Accept: text/html`) to a federated host and, instead of the +silent ladder, denies with an HTTP **200** whose body is a small page that +does exactly what Vikunja's own "Login with ``" button would have: +stores a `state` in `localStorage`, then navigates to Vikunja's callback route +with a code this service already minted (`store.create_authorization_code`, +the same call `federate()` uses). `DeniedHttpResponse.status`/`headers`/`body` +are fully caller-controlled — Envoy sends them straight to the browser without +ever proxying that hit to Vikunja — so this replicates Vikunja's real, +CSRF-checked front-channel flow (`OpenIdAuth.vue` hard-fails on a `state` +mismatch, so a bare server-side redirect that skipped setting it first would +not work). + +Two paths must be exempted from this (and from the ladder) or the exchange +breaks: `frontend_oidc_path(provider)` (the callback page the bootstrap script +navigates to) and `vikunja.callback_path(provider)` (the API call that page +makes to redeem the code) are always let through untouched. + +There is no way to tell from the server side whether a browser already holds +a valid Vikunja JWT, so this runs on **every** document navigation to a +federated host, not just the first. That is intentional, not a cache miss: +this whole model derives a session from the mTLS cert per request, so a full +page reload legitimately re-deriving a fresh one is consistent with treating +"logout" as meaningless while the cert is presented. + ## The `get_bearer` decision ladder Per request, for a derived `Subject` (`sub` is a 16-char SHA-256 of the cert's @@ -78,9 +119,13 @@ per-request `GET /api/v1/user`. ## Return contract +This ladder only runs for non-navigation requests (see +[Browser bootstrap](#browser-bootstrap-document-navigations) above for how a +real browser page load is handled instead): + | `get_bearer` result | `Check` response | |---|---| -| `str` | `OK` + `headers: Authorization: Bearer ` | +| `str` | `OK` + `headers: Authorization: Bearer ` (upstream request only) | | `None` | `OK` with **no** `Authorization` header (client's bearer allowed through) | | raises `DownstreamError` | deny | diff --git a/envoy_authz/federator/browser_bootstrap.py b/envoy_authz/federator/browser_bootstrap.py new file mode 100644 index 0000000..d0b2f89 --- /dev/null +++ b/envoy_authz/federator/browser_bootstrap.py @@ -0,0 +1,77 @@ +"""Silent front-channel login for a browser hitting a federated host directly. + +The existing get_bearer ladder (session.py) authenticates API/XHR calls by +injecting `Authorization` into the *upstream* request — invisible to a +browser's own JS, so a browser SPA never becomes "logged in" from it alone. +Vikunja's frontend instead expects a real OIDC front-channel redirect (its +`/auth/openid/` route validates a `state` it stored in localStorage +before redirecting out) — but this federator's OP has no `/oauth/authorize` +(codes are minted server-to-server only, see op/routes.py). Reconciling this +without any Envoy/Contour changes: Check() detects a top-level browser +navigation and denies it with an HTTP 200 whose body is a tiny script that +does exactly what Vikunja's own "Login with " button would have +done (store `state`, then navigate to the callback route with a code we +already minted) before Vikunja's real page is ever served. Envoy sends a +`denied_response` straight to the client without proxying anything upstream, +so this never touches Vikunja. +""" + +import html +import secrets +from urllib.parse import urlparse + +from .providers import Provider + + +def is_document_navigation(headers: dict) -> bool: + """True for a top-level browser navigation, not an API/XHR/asset request. + + `Sec-Fetch-Dest` is sent by all current Chromium/Firefox releases and is + unambiguous (`document` only for a real address-bar/link navigation). + Falls back to `Accept: text/html` for clients that omit Sec-Fetch-* + (older browsers, curl, the API test suite), so those keep going through + the existing silent get_bearer ladder rather than getting a page. + """ + dest = headers.get("sec-fetch-dest") + if dest is not None: + return dest == "document" + return "text/html" in headers.get("accept", "") + + +def frontend_oidc_path(provider: Provider) -> str: + """Vikunja's own frontend OIDC-callback route (not the backend API path). + + A request here must be let through untouched: it is the page navigation + the bootstrap script above triggers, and Vikunja's SPA (OpenIdAuth.vue) + needs to actually render and run there. + """ + return urlparse(provider.redirect_url).path + + +def render_bootstrap_html(*, redirect_path: str, code: str, state: str) -> str: + """The whole HTTP response body for the browser-bootstrap denied_response. + + Mirrors Vikunja's frontend `redirectToProvider()` exactly (store `state` + in localStorage, then navigate) so its callback's CSRF check passes as if + the user had clicked its own "Login with " button. `code` and + `state` are our own (itsdangerous URL-safe base64 / secrets.token_urlsafe) + output and cannot contain a quote or ``, but both are still + HTML-escaped before being embedded in a JS string literal — templating + untrusted-shaped values into a script body without escaping is exactly how + an XSS hole gets introduced later if the value ever changes shape. + """ + safe_code = html.escape(code, quote=True) + safe_state = html.escape(state, quote=True) + safe_path = html.escape(redirect_path, quote=True) + return ( + '' + "" + "" + ) + + +def new_state() -> str: + return secrets.token_urlsafe(18) diff --git a/envoy_authz/federator/vikunja.py b/envoy_authz/federator/vikunja.py index c18d22c..dbcf806 100644 --- a/envoy_authz/federator/vikunja.py +++ b/envoy_authz/federator/vikunja.py @@ -24,6 +24,16 @@ _REFRESH_COOKIE_NAME = "vikunja_refresh_token" +def callback_path(provider: Provider) -> str: + """The Vikunja backend path that redeems a federation auth code. + + Shared with the browser-bootstrap path (grpc_service.Check): that path must + let a request to this exact path through untouched rather than treat it as + a plain federated request, or Vikunja's own callback exchange never runs. + """ + return f"/api/v1/auth/openid/{provider.provider_key}/callback" + + class DownstreamError(Exception): """Vikunja returned an error or was unreachable. @@ -124,7 +134,7 @@ def federate(self, subject: Subject) -> VikunjaSession: name=subject.name, nonce=None, ) - callback = f"/api/v1/auth/openid/{self._provider.provider_key}/callback" + callback = callback_path(self._provider) try: resp = self._client.post( callback, diff --git a/envoy_authz/grpc_service.py b/envoy_authz/grpc_service.py index 14e035f..79a8083 100644 --- a/envoy_authz/grpc_service.py +++ b/envoy_authz/grpc_service.py @@ -13,10 +13,17 @@ from envoy_authz.identity import parse_client_identity from .config import Config, verify_client_cert +from .federator.browser_bootstrap import ( + frontend_oidc_path, + is_document_navigation, + new_state, + render_bootstrap_html, +) from .federator.providers import PROVIDERS, get_provider, provider_for_host from .federator.session import SessionCache, get_bearer +from .federator.store import create_authorization_code from .federator.subject import derive_subject -from .federator.vikunja import DownstreamError, VikunjaClient +from .federator.vikunja import DownstreamError, VikunjaClient, callback_path logger = logging.getLogger(__name__) @@ -234,7 +241,7 @@ def Check(self, request, context): _vikunja is not None and _SESSIONS is not None and client_cert is not None - and provider_for_host(host) is not None + and (provider := provider_for_host(host)) is not None ): try: # identity may be None here if the best-effort parse above @@ -245,24 +252,94 @@ def Check(self, request, context): except Exception: logger.exception("Failed to derive subject for federation") return _deny(retryable=False) - incoming_bearer = _extract_bearer(headers) - try: - upstream = get_bearer(subject, incoming_bearer, _vikunja, _SESSIONS) - except DownstreamError as exc: - logger.warning("denied-federation-failure sub=%s", subject.sub) - return _deny(exc.retryable) - if upstream is not None: - return_headers.append( - HeaderValueOption( - header=HeaderValue( - key="Authorization", - value=f"Bearer {upstream}", - ), + + # Envoy's request.path includes the query string (Vikunja's + # callback is always hit as .../broker?code=...&state=...), so + # comparing the raw path against a bare route never matches. + path_no_query = path.split("?", 1)[0] + if path_no_query in ( + frontend_oidc_path(provider), + callback_path(provider), + ): + # Vikunja's own OIDC machinery: the callback page the + # bootstrap script below navigates to, and the backend + # call that page makes to redeem the code. Neither needs + # (or should get) an injected bearer, and intercepting + # either would break the exchange rather than complete it. + logger.info("allowed-through-oidc-path sub=%s", subject.sub) + elif is_document_navigation(headers): + # A real address-bar/link navigation, not an API/XHR call. + # The Authorization header the ladder below injects is only + # ever visible to Vikunja's *backend* — never to the + # browser's own JS — so a browser hitting this host + # directly would still fall through to Vikunja's own + # client-side OIDC login, which this federator's OP has no + # /oauth/authorize to answer (op/routes.py: codes are + # minted server-to-server only). Do what clicking + # Vikunja's "Login" button would have done instead: mint a + # code, then deny with a page that stores `state` and + # navigates to the callback exactly as that button would + # — Envoy sends `denied_response` straight to the browser + # without ever proxying this hit to Vikunja. + if not subject.email: + logger.warning( + "cannot bootstrap sub=%s: client cert has no email " + "(rfc822Name SAN)", + subject.sub, ) + return _deny(retryable=False) + code = create_authorization_code( + client_id=provider.client_id, + redirect_uri=provider.redirect_url, + scope=provider.scope, + user_id=subject.sub, + email=subject.email, + name=subject.name, + nonce=None, + ) + logger.info("bootstrap-redirect sub=%s", subject.sub) + return external_auth_pb2.CheckResponse( + status=status_pb2.Status(code=code_pb2.PERMISSION_DENIED), + denied_response=external_auth_pb2.DeniedHttpResponse( + status=http_status_pb2.HttpStatus( + code=http_status_pb2.StatusCode.OK + ), + headers=[ + HeaderValueOption( + header=HeaderValue( + key="Content-Type", + value="text/html; charset=utf-8", + ), + ), + ], + body=render_bootstrap_html( + redirect_path=frontend_oidc_path(provider), + code=code, + state=new_state(), + ), + ), ) - logger.info("injected-bearer sub=%s", subject.sub) else: - logger.info("allowed-through-client-bearer sub=%s", subject.sub) + incoming_bearer = _extract_bearer(headers) + try: + upstream = get_bearer( + subject, incoming_bearer, _vikunja, _SESSIONS + ) + except DownstreamError as exc: + logger.warning("denied-federation-failure sub=%s", subject.sub) + return _deny(exc.retryable) + if upstream is not None: + return_headers.append( + HeaderValueOption( + header=HeaderValue( + key="Authorization", + value=f"Bearer {upstream}", + ), + ) + ) + logger.info("injected-bearer sub=%s", subject.sub) + else: + logger.info("allowed-through-client-bearer sub=%s", subject.sub) return external_auth_pb2.CheckResponse( status=status_pb2.Status(code=code_pb2.OK), diff --git a/tests/unit/test_browser_bootstrap.py b/tests/unit/test_browser_bootstrap.py new file mode 100644 index 0000000..b8a427d --- /dev/null +++ b/tests/unit/test_browser_bootstrap.py @@ -0,0 +1,71 @@ +"""Unit tests for the browser-bootstrap helpers (pure functions, no gRPC).""" + +from envoy_authz.federator.browser_bootstrap import ( + frontend_oidc_path, + is_document_navigation, + render_bootstrap_html, +) +from envoy_authz.federator.providers import Provider + + +def _provider(**overrides) -> Provider: + defaults = { + "hosts": ["vikunja.example.com"], + "client_id": "vikunja", + "client_secret": "s", + "redirect_url": "https://vikunja.example.com/auth/openid/broker", + "api_base": "http://vikunja:3456", + "provider_key": "broker", + } + defaults.update(overrides) + return Provider(**defaults) + + +def test_is_document_navigation_true_for_sec_fetch_dest_document(): + assert is_document_navigation({"sec-fetch-dest": "document"}) is True + + +def test_is_document_navigation_false_for_sec_fetch_dest_other_values(): + # A real browser sends this for scripts/styles/images/XHR — must not be + # treated as a page load even though the same browser is asking. + for dest in ("script", "style", "image", "empty"): + assert is_document_navigation({"sec-fetch-dest": dest}) is False + + +def test_is_document_navigation_falls_back_to_accept_header(): + # No Sec-Fetch-Dest (older browser / curl / the existing API tests) — + # Accept: text/html is the fallback signal. + assert is_document_navigation({"accept": "text/html,application/xhtml+xml"}) + assert not is_document_navigation({"accept": "application/json"}) + assert not is_document_navigation({}) + + +def test_frontend_oidc_path_is_the_redirect_url_path_component(): + provider = _provider(redirect_url="https://vikunja.example.com/auth/openid/broker") + assert frontend_oidc_path(provider) == "/auth/openid/broker" + + +def test_render_bootstrap_html_sets_state_before_navigating(): + body = render_bootstrap_html( + redirect_path="/auth/openid/broker", code="a.b.c", state="xyz123" + ) + # Order matters: Vikunja's own redirectToProvider() stores state THEN + # navigates — reversing this would fail its CSRF check every time. + set_state_index = body.index("localStorage.setItem('state', 'xyz123')") + navigate_index = body.index("window.location.href") + assert set_state_index < navigate_index + assert "/auth/openid/broker?code=a.b.c&state=xyz123" in body + + +def test_render_bootstrap_html_escapes_html_special_characters(): + # code/state are our own itsdangerous/secrets output and never contain + # these characters in practice, but the render must not silently trust + # that — an unescaped `` or quote would be a real XSS hole the + # moment either value's shape ever changes. + body = render_bootstrap_html( + redirect_path="/auth/openid/broker", + code="", + state="'; alert(2); '", + ) + assert "" not in body + assert "'; alert(2); '" not in body diff --git a/tests/unit/test_check.py b/tests/unit/test_check.py index ae4d9c6..3fbc284 100644 --- a/tests/unit/test_check.py +++ b/tests/unit/test_check.py @@ -105,6 +105,92 @@ def test_check_allows_through_when_get_bearer_returns_none( assert "Authorization" not in added +def test_check_document_navigation_returns_bootstrap_page( + grpc_servicer, email_client_cert_pem +): + """A real browser navigation (Sec-Fetch-Dest: document) to a federated + host gets a denied_response whose HTTP status is 200 and whose body is a + page that stores `state` and navigates to the callback — mirroring + Vikunja's own "Login" button — rather than the silent Authorization + header, which a browser SPA can never see.""" + req = grpc_servicer.check_request( + host="vikunja.example.com", + path="/", + client_cert_pem=email_client_cert_pem, + headers={"sec-fetch-dest": "document"}, + ) + resp = grpc_servicer.servicer.Check(req, None) + assert resp.status.code == code_pb2.PERMISSION_DENIED + assert resp.denied_response.status.code == http_status_pb2.StatusCode.OK + added = {h.header.key: h.header.value for h in resp.denied_response.headers} + assert added["Content-Type"] == "text/html; charset=utf-8" + assert "localStorage.setItem('state'" in resp.denied_response.body + assert "/auth/openid/broker?code=" in resp.denied_response.body + + +def test_check_denies_bootstrap_when_cert_has_no_email( + grpc_servicer, trusted_client_cert_pem +): + # trusted_client_cert_pem carries no rfc822Name SAN — same "cannot + # provision a downstream user" constraint as the existing federate() path. + req = grpc_servicer.check_request( + host="vikunja.example.com", + path="/", + client_cert_pem=trusted_client_cert_pem, + headers={"sec-fetch-dest": "document"}, + ) + resp = grpc_servicer.servicer.Check(req, None) + assert resp.status.code == code_pb2.PERMISSION_DENIED + assert resp.denied_response.status.code == http_status_pb2.StatusCode.Unauthorized + + +def test_check_frontend_oidc_path_passes_through_untouched( + grpc_servicer, email_client_cert_pem, monkeypatch +): + """The callback page the bootstrap script navigates to must render + normally, not get intercepted into another bootstrap page (which would + loop) or federated (get_bearer must not run for it).""" + from envoy_authz import grpc_service + + def _boom(*a, **k): + raise AssertionError("get_bearer must not run on Vikunja's own OIDC path") + + monkeypatch.setattr(grpc_service, "get_bearer", _boom) + # Real Envoy always includes the query string in `path` — this is exactly + # how the bootstrap script's own navigation (?code=...&state=...) arrives. + # A bare-path comparison with no query string would pass vacuously here. + req = grpc_servicer.check_request( + host="vikunja.example.com", + path="/auth/openid/broker?code=abc123&state=xyz789", + client_cert_pem=email_client_cert_pem, + headers={"sec-fetch-dest": "document"}, + ) + resp = grpc_servicer.servicer.Check(req, None) + assert resp.status.code == code_pb2.OK + assert [h.header.key for h in resp.ok_response.headers] == [] + + +def test_check_backend_oidc_callback_path_passes_through_untouched( + grpc_servicer, email_client_cert_pem, monkeypatch +): + """The frontend's own POST redeeming the code must reach Vikunja + untouched too, even though it is not a document navigation.""" + from envoy_authz import grpc_service + + def _boom(*a, **k): + raise AssertionError("get_bearer must not run on Vikunja's own OIDC path") + + monkeypatch.setattr(grpc_service, "get_bearer", _boom) + req = grpc_servicer.check_request( + host="vikunja.example.com", + path="/api/v1/auth/openid/broker/callback", + client_cert_pem=email_client_cert_pem, + ) + resp = grpc_servicer.servicer.Check(req, None) + assert resp.status.code == code_pb2.OK + assert [h.header.key for h in resp.ok_response.headers] == [] + + def test_check_denies_503_on_retryable_federation_failure( grpc_servicer, email_client_cert_pem, monkeypatch ):