diff --git a/designs/DEVICE_AUTH.md b/designs/DEVICE_AUTH.md index d55ca60403..2006895af7 100644 --- a/designs/DEVICE_AUTH.md +++ b/designs/DEVICE_AUTH.md @@ -18,8 +18,8 @@ > `set_grant_revocation_check`). Wired in `omnigent/server/app.py`, > **opt-in and default-off** via `OMNIGENT_DEVICE_GRANT_ENABLED` (the > `/oauth/*` routes are unmounted unless it is truthy), and then only in -> **accounts** mode (OIDC delegates login to the IdP via the cli-ticket -> flow and never mounts these routes). +> **accounts** and **oidc** modes — the two that own a server-minted +> session cookie. Header mode has no server-mintable identity. > Slack: `integrations/slack/src/omnigent_slack/oauth.py`, > `tokens.py` (Fernet-encrypted `oauth_tokens`), `auth_manager.py`, plus > the bearer/refresh wiring in `omnigent.py` (`ClientAuth`, @@ -38,6 +38,14 @@ > (`app.py`: device auth is `oidc`/`accounts` only), so `start_login` > raises a clear error rather than firing a request the server would 404. > +> **Note.** Mounting the grant under OIDC does not change either +> first-party client: the CLI (`cli.py`) and Slack (`oauth.py`) still +> probe the mode and still take the cli-ticket flow there. The OIDC +> device-grant routes exist for *external* clients — ones that must hold a +> scoped, revocable, refreshable credential rather than the server's own +> session JWT, and that cannot be trusted with account-wide authority. +> Migrating the first-party clients onto it is separate work. +> > Tests: `tests/server/test_device_auth.py`, and the Slack > `test_oauth.py` / `test_tokens.py` / `test_client_auth.py` / > `test_auth_manager.py`. @@ -91,9 +99,11 @@ The device grant builds on existing server primitives: - **Bearer validation** — `UnifiedAuthProvider._check_cookie` accepts `Authorization: Bearer ` and validates the same claim shape (`auth.py`). Delegated access tokens validate through this path unchanged. -- **Browser consent under accounts mode** — the `accounts` provider - establishes the browser identity via its session cookie; the consent page - runs behind it. (This is why the grant mounts in accounts mode only — see +- **Browser consent** — both `accounts` and `oidc` establish the browser + identity through the same HS256 session cookie, and the consent page runs + behind it. Under OIDC the bounce hands off to the IdP, so *how* the user + proves themselves — password, Google, SAML — is never this module's + business. (Header mode mints no session, which is why it is excluded — see the mount restriction below.) - **Open-redirect hardening** — `_sanitize_return_to` (`routes/auth.py`) guards the post-login bounce back to the consent page. @@ -184,10 +194,70 @@ approved it. Mounted in `app.py` only when **`OMNIGENT_DEVICE_GRANT_ENABLED` is truthy** (opt-in, **default-off** — the `/oauth/*` routes are absent otherwise), and -then **only in `accounts` mode** (OIDC delegates login to the IdP via the -cli-ticket flow and never mounts these routes; header mode has no -server-mintable identity — see `create_device_auth_router`, which raises if -constructed for any other source). The `device_grants` table is created +then **only in `accounts` and `oidc` modes**. Header mode has no +server-mintable identity — there is nothing to delegate from and no login to +bounce a consenting browser through — see `create_device_auth_router`, which +raises if constructed for any other source. + +**Forced re-authentication across the two modes.** The consent gate (session +`iat` ≥ grant `created_at`) bounces with `reauth=1`, and each login path has +to honour it in its own way. Accounts holds back the SPA's auto-redirect and +demands a password. OIDC has no form to hold back, so `/auth/login` forwards +`reauth=1` to the IdP as `prompt=login` (OIDC Core 3.1.2.1). Dropping that +forwarding does not break the flow — the IdP satisfies the bounce from its own +session and the callback mints a fresh `iat` that clears the gate — it just +silently removes the deliberate-credential-entry property the gate exists for. +Pinned by `tests/server/test_oidc_reauth_prompt.py`. + +*Every* bounce is forced, including the one for a caller with no Omnigent +session. Under accounts that changes nothing — no session means no credential, +so the form is shown regardless — but under OIDC the caller may still hold a +live IdP session, and the consent page cannot tell the two cases apart. + +**Requested, then verified.** `prompt=login` is only a request, so the bounce +also sends `max_age=0`, which obliges a conforming IdP to report the moment it +authenticated the user in the `auth_time` claim. The callback compares that +claim against the id_token's own `iat` — both the IdP's clock, so skew between +the two servers cancels out and a session established minutes earlier fails the +comparison regardless of whose clock is ahead. A missing `auth_time` fails too: +silence is indistinguishable from a reused session. + +**The proof rides on the session, not on `iat`.** Gating consent on the session +cookie's `iat` was bypassable without ever attacking the gate. `/auth/login` is +a public GET accepting any same-origin `return_to`, so an attacker who starts a +grant can send the victim `/auth/login?return_to=/oauth/device?user_code=…` +with **no** `reauth=1` — no `reauth_at` is signed, nothing is demanded, the IdP +satisfies it from its own session, and the resulting cookie carries an `iat` of +*now* that clears the gate. The forced path was never entered. + +So a successful re-authentication is recorded on the session itself, as an +`auth_time` claim (`mint_session_token`), written **only** where a credential +was actually presented: an accounts password submit, or an IdP-attested +re-authentication. Consent requires `auth_time ≥ grant.created_at`. A login +that skipped the bounce carries no claim, so it bounces and is made to prove +itself — the demand no longer depends on the attacker's link having asked for +it. `reauth_at` remains, signed into the state cookie, as the marker that makes +the callback *refuse* (403, no session minted) rather than merely decline to +stamp the proof. + +An IdP that never emits `auth_time` cannot carry a device grant: consent +bounces once, the forced callback 403s with a clear error, and the loop +terminates rather than spinning. + +**Which providers qualify.** `unsupported_reason` **allowlists** +`provider_type == "oidc"` rather than denying known-bad values. `from_env` +yields only `github` or `oidc` today, so the two are equivalent right now — but +the next OAuth 2.0 dialect modelled under the `oidc` source would otherwise be +admitted by default, behind a gate that cannot hold for it. GitHub is the +present example: `from_env` points it at +`https://github.com/login/oauth/authorize`, which has no `prompt` parameter, no +id_token and no `auth_time`, so there is no way to demand a re-authentication +nor to detect that none happened. + +`app.py` logs every refusal, and computes it **before** the auth-router mount — +that mount is gated on `login_url` being truthy, and header mode's is `None`, +so an operator in the one mode where the grant can never work was previously +the only one who never saw the explanation. The `device_grants` table is created unconditionally by the migration regardless of the flag; only the router mount is gated. This router **owns** `mint_delegated_token` and `DELEGATED_SCOPE`. diff --git a/omnigent/server/app.py b/omnigent/server/app.py index 722315c611..e128471408 100644 --- a/omnigent/server/app.py +++ b/omnigent/server/app.py @@ -2419,6 +2419,30 @@ async def _on_hosts_changed(_host_id: str, owner: str | None) -> None: # /magic/redeem, /users, /users/{id}/reset, /users/me/password). # Must be registered BEFORE the SPA static mount because the SPA's # HTML5-history fallback catches all unmatched extensionless paths. + # Device-grant support is decided BEFORE the auth-router mount below, + # which is gated on `login_url` being truthy — and header mode's is None. + # Computing it inside that block meant the one operator most likely to be + # confused by a missing /oauth/* (header mode, where the grant can never + # work) was the one who never saw the explanation. + from omnigent.server.auth import UnifiedAuthProvider as _UnifiedAuthProvider + from omnigent.server.auth import env_var_is_truthy + from omnigent.server.routes.device_auth import unsupported_reason + + _device_grant_wanted = env_var_is_truthy("OMNIGENT_DEVICE_GRANT_ENABLED", default=False) + _device_grant_blocked = ( + unsupported_reason(auth_provider) + if isinstance(auth_provider, _UnifiedAuthProvider) + else "a custom auth provider cannot mint the session the grant delegates from" + ) + if _device_grant_wanted and _device_grant_blocked is not None: + # Asked for and refused: say so, or the operator sees only the absence + # of /oauth/* and assumes the flag did not take. + _logger.warning( + "device-grant: OMNIGENT_DEVICE_GRANT_ENABLED is set but the " + "/oauth/* routes are NOT mounted — %s. See designs/DEVICE_AUTH.md.", + _device_grant_blocked, + ) + if auth_provider is not None and getattr(auth_provider, "login_url", None): from omnigent.server.auth import UnifiedAuthProvider @@ -2479,17 +2503,16 @@ async def _on_hosts_changed(_host_id: str, owner: str | None) -> None: ) # Device Authorization Grant (RFC 8628): opt-in, default-off via - # OMNIGENT_DEVICE_GRANT_ENABLED, and accounts-mode only. OIDC delegates - # login to the IdP (cli-ticket flow), so it neither needs nor mounts - # these routes. Wires the revocation lookup into the auth provider so - # revoking a grant immediately rejects its delegated access tokens. + # OMNIGENT_DEVICE_GRANT_ENABLED, and only for providers that can be + # made to re-prompt an already signed-in user — see + # `unsupported_reason`, which owns that rule. Wires the revocation + # lookup into the auth provider so revoking a grant immediately + # rejects its delegated access tokens. # See designs/DEVICE_AUTH.md. - from omnigent.server.auth import env_var_is_truthy - if ( - env_var_is_truthy("OMNIGENT_DEVICE_GRANT_ENABLED", default=False) + _device_grant_wanted and isinstance(auth_provider, UnifiedAuthProvider) - and auth_provider._source == "accounts" + and _device_grant_blocked is None and permission_store is not None ): from omnigent.server.device_grant_store import DeviceGrantStore diff --git a/omnigent/server/oidc.py b/omnigent/server/oidc.py index 9b676182eb..d142f253f6 100644 --- a/omnigent/server/oidc.py +++ b/omnigent/server/oidc.py @@ -55,6 +55,8 @@ def mint_session_token( cookie_secret: bytes, ttl_seconds: int, provider: str, + *, + auth_time: int | None = None, ) -> str: """ Mint a signed session JWT with a second-granularity lifetime. @@ -65,21 +67,34 @@ def mint_session_token( same validator (:meth:`UnifiedAuthProvider._check_cookie`) accepts either. + ``auth_time`` records **when this server last saw the user prove who + they are**, on this server's clock. It is deliberately not the same as + ``iat``: minting a session proves only that a login flow completed, + which under OIDC can happen with no user involvement at all when the + IdP reuses its own session. Callers pass it only where a credential + was actually presented — a password submit, or an IdP-attested + re-authentication. Absent otherwise, and consumers must fail closed on + its absence. See :mod:`omnigent.server.routes.device_auth`. + :param user_id: The authenticated user's email, e.g. ``"alice@example.com"``. :param cookie_secret: HMAC key for HS256 signing. :param ttl_seconds: Token lifetime in seconds. :param provider: Identity provider name, e.g. ``"google"`` or ``"accounts"``. Stored as an informational claim. + :param auth_time: Epoch seconds (this server's clock) at which the + user proved their identity, or ``None`` when unproven. :returns: An HS256-signed JWT string. """ now = int(time.time()) - payload = { + payload: dict[str, object] = { "sub": user_id, "iat": now, "exp": now + ttl_seconds, "provider": provider, } + if auth_time is not None: + payload["auth_time"] = auth_time return jwt.encode(payload, cookie_secret, algorithm="HS256") @@ -88,6 +103,8 @@ def mint_session_cookie( cookie_secret: bytes, ttl_hours: int, provider: str, + *, + auth_time: int | None = None, ) -> str: """Mint a signed session cookie JWT. @@ -97,9 +114,12 @@ def mint_session_cookie( :param ttl_hours: Session lifetime in hours. :param provider: Identity provider name, e.g. ``"google"`` or ``"github"``. Stored as an informational claim. + :param auth_time: See :func:`mint_session_token`. :returns: An HS256-signed JWT string. """ - return mint_session_token(user_id, cookie_secret, ttl_hours * 3600, provider) + return mint_session_token( + user_id, cookie_secret, ttl_hours * 3600, provider, auth_time=auth_time + ) def hmac_digest(token: str, secret: bytes) -> str: diff --git a/omnigent/server/routes/accounts_auth.py b/omnigent/server/routes/accounts_auth.py index 75b9834501..224465e7b9 100644 --- a/omnigent/server/routes/accounts_auth.py +++ b/omnigent/server/routes/accounts_auth.py @@ -299,6 +299,9 @@ async def login(body: LoginRequest) -> Response: cookie_secret=config.cookie_secret, ttl_hours=config.session_ttl_hours, provider="accounts", + # The server verified the credential itself just now, so this is + # a local fact rather than a third party's claim about one. + auth_time=int(time.time()), ) user = account_store.get_user(username) @@ -446,6 +449,9 @@ async def register(body: RegisterRequest) -> Response: cookie_secret=config.cookie_secret, ttl_hours=config.session_ttl_hours, provider="accounts", + # The server verified the credential itself just now, so this is + # a local fact rather than a third party's claim about one. + auth_time=int(time.time()), ) resp = JSONResponse( status_code=200, @@ -545,6 +551,9 @@ async def setup(body: SetupRequest) -> Response: cookie_secret=config.cookie_secret, ttl_hours=config.session_ttl_hours, provider="accounts", + # The server verified the credential itself just now, so this is + # a local fact rather than a third party's claim about one. + auth_time=int(time.time()), ) resp = JSONResponse( status_code=200, @@ -631,6 +640,9 @@ async def magic_redeem(request: Request) -> Response: cookie_secret=config.cookie_secret, ttl_hours=config.session_ttl_hours, provider="accounts", + # The server verified the credential itself just now, so this is + # a local fact rather than a third party's claim about one. + auth_time=int(time.time()), ) resp = RedirectResponse(url="/", status_code=302) _set_session_cookie( diff --git a/omnigent/server/routes/auth.py b/omnigent/server/routes/auth.py index 73aeda7b7c..9bb160b081 100644 --- a/omnigent/server/routes/auth.py +++ b/omnigent/server/routes/auth.py @@ -46,6 +46,9 @@ _AUTH_STATE_COOKIE_PLAIN = "ap_auth_state" _AUTH_STATE_TTL_SECONDS = 300 # 5 minutes _CLI_TICKET_TTL_SECONDS = 300 # 5 minutes +# How long the IdP may take between authenticating the user and signing the +# id_token. Both timestamps are the IdP's own, so this is not clock skew. +_REAUTH_PROCESSING_ALLOWANCE_SECONDS = 120 # How long an OIDC invite URL stays redeemable. Matches the accounts # provider's default invite window (72h) — long enough to share # out-of-band, short enough to bound exposure of an unused link. @@ -143,6 +146,11 @@ async def login(request: Request) -> Response: ``state`` parameter. Stores them in a short-lived signed cookie so the callback can verify the response. + ``?reauth=1`` adds ``prompt=login``, forcing the IdP to re-prompt + for credentials even when it already has a session. The + device-grant consent page sets it; see + :mod:`omnigent.server.routes.device_auth`. + :param request: The incoming FastAPI request. :returns: 302 redirect to the IdP with PKCE and state params. @@ -163,6 +171,12 @@ async def login(request: Request) -> Response: # before the callback redeems it. Only meaningful when invites # are enabled; ignored otherwise. invite = request.query_params.get("invite") if _invites_enabled else None + # Forced re-authentication, requested by the device-grant consent page. + # Without it the IdP satisfies the bounce from its own session and the + # consent gate passes on a user who proved nothing. GitHub OAuth has + # no way to demand or report it, so it never gets here — see + # `device_auth.unsupported_reason`. + reauth = request.query_params.get("reauth") == "1" and config.provider_type != "github" # Store state + code_verifier in a short-lived signed cookie. state_payload: dict[str, str | int] = { @@ -175,6 +189,10 @@ async def login(request: Request) -> Response: state_payload["ticket"] = ticket if invite: state_payload["invite"] = invite + if reauth: + # Signed, so the callback's freshness check cannot be removed by + # editing the URL. + state_payload["reauth_at"] = int(time.time()) state_jwt = jwt.encode(state_payload, config.cookie_secret, algorithm="HS256") # Build the authorization URL. @@ -187,6 +205,12 @@ async def login(request: Request) -> Response: "code_challenge": code_challenge, "code_challenge_method": "S256", } + if reauth: + # OIDC Core 3.1.2.1: re-prompt even when the IdP has a session. + # `max_age=0` makes it enforceable — it obliges a conforming IdP + # to return `auth_time`, which the callback then verifies. + params["prompt"] = "login" + params["max_age"] = "0" auth_url = config.authorization_endpoint + "?" + urlencode(params) response = RedirectResponse(url=auth_url, status_code=302) @@ -264,6 +288,11 @@ async def callback(request: Request) -> Response: "code_verifier": code_verifier, } + # When (on our clock) the user proved their identity to the IdP for + # THIS login. Stays None unless the IdP attests to it — GitHub OAuth + # never can, which is why it cannot carry a device grant. + proven_at: int | None = None + async with httpx.AsyncClient() as client: # GitHub requires Accept: application/json to get JSON # response from the token endpoint. @@ -304,6 +333,22 @@ async def callback(request: Request) -> Response: else: email = _resolve_oidc_email(token_json, config) + # Record whether the user actually proved themselves here, rather + # than the IdP silently reusing its own session. Checked on EVERY + # login, not only the forced ones: the device-grant consent gate + # reads this, and a login that skipped the bounce would otherwise + # look identical to one that honoured it. + if _idp_reauthenticated(token_json, config): + proven_at = int(time.time()) + + # This login was demanded by a device-grant consent bounce, so a + # session the IdP simply reused is not good enough. + if state_payload.get("reauth_at") is not None and proven_at is None: + return JSONResponse( + status_code=403, + content={"error": "Re-authentication was required but did not occur"}, + ) + if not email: return JSONResponse( status_code=400, @@ -353,12 +398,16 @@ async def callback(request: Request) -> Response: permission_store.ensure_user(email) promote_if_listed(admin_list, permission_store, email) - # Mint session cookie. + # Mint session cookie. `auth_time` carries the proof forward: under + # OIDC a completed callback says nothing about user involvement, so + # consumers that need a deliberate authentication must read this + # rather than `iat`. session_jwt = mint_session_cookie( user_id=email, cookie_secret=config.cookie_secret, ttl_hours=config.session_ttl_hours, provider=config.provider_type, + auth_time=proven_at, ) # Check if this callback fulfills a CLI login ticket. @@ -767,6 +816,91 @@ def _claim_is_verified_true(value: object) -> bool: return isinstance(value, str) and value.strip().lower() == "true" +def _verified_id_token_claims( + token_json: dict[str, object], + config: OIDCConfig, +) -> dict[str, object] | None: + """Validate the ``id_token`` and return its claims. + + Checks the JWT signature against the IdP's JWKS and verifies ``iss`` + and ``aud``. Shared by every consumer so no caller can read a claim out + of an unverified token. + + :param token_json: The token endpoint response JSON. + :param config: The OIDC configuration with JWKS URI and expected + issuer/audience. + :returns: The verified claims, or ``None`` when the token is missing, + unverifiable, or the config has no JWKS URI. + """ + id_token = token_json.get("id_token") + if not isinstance(id_token, str) or not id_token: + return None + if config.jwks_uri is None: + _logger.warning("Rejecting id_token: OIDC configuration has no JWKS URI") + return None + + try: + jwks_client = jwt.PyJWKClient(config.jwks_uri) + signing_key = jwks_client.get_signing_key_from_jwt(id_token) + claims: dict[str, object] = jwt.decode( + id_token, + signing_key.key, + algorithms=["RS256", "RS384", "RS512", "ES256", "ES384", "ES512"], + audience=config.client_id, + issuer=config.issuer, + ) + except jwt.InvalidTokenError as exc: + _logger.warning("id_token validation failed: %s", exc) + return None + + return claims + + +def _idp_reauthenticated( + token_json: dict[str, object], + config: OIDCConfig, +) -> bool: + """Did the IdP authenticate the user *for this login*, or reuse a session? + + ``prompt=login`` and ``max_age=0`` are requests. This is the check that + they were honoured: a conforming IdP reports when it authenticated the + user via ``auth_time``, and for a genuine re-authentication that moment + coincides with this token's own issuance. + + Both values come from the IdP's clock, so they are compared against each + other rather than against ours — a skew between the two servers cancels + out, and a session the IdP established minutes or hours ago fails the + comparison no matter whose clock is ahead. The allowance covers the + IdP's own processing between authenticating the user and signing the + token, not clock drift. + + Fails closed. A missing ``auth_time`` means the IdP did not answer the + question, which is indistinguishable from it having reused an existing + session. + + :param token_json: The token endpoint response JSON. + :param config: The OIDC configuration. + :returns: True only on a proven fresh authentication. + """ + claims = _verified_id_token_claims(token_json, config) + if claims is None: + return False + + auth_time = claims.get("auth_time") + if not isinstance(auth_time, int): + _logger.warning( + "Re-authentication could not be verified: the id_token has no " + "integer auth_time claim, so the IdP may have reused an existing session" + ) + return False + + issued_at = claims.get("iat") + if not isinstance(issued_at, int): + return False + + return auth_time >= issued_at - _REAUTH_PROCESSING_ALLOWANCE_SECONDS + + def _resolve_oidc_email( token_json: dict[str, object], config: OIDCConfig, @@ -805,25 +939,8 @@ def _resolve_oidc_email( ``email_verified`` is not truthy (and verification is not skipped via config). """ - id_token = token_json.get("id_token") - if not isinstance(id_token, str) or not id_token: - return None - if config.jwks_uri is None: - _logger.warning("Rejecting id_token: OIDC configuration has no JWKS URI") - return None - - try: - jwks_client = jwt.PyJWKClient(config.jwks_uri) - signing_key = jwks_client.get_signing_key_from_jwt(id_token) - claims = jwt.decode( - id_token, - signing_key.key, - algorithms=["RS256", "RS384", "RS512", "ES256", "ES384", "ES512"], - audience=config.client_id, - issuer=config.issuer, - ) - except jwt.InvalidTokenError as exc: - _logger.warning("id_token validation failed: %s", exc) + claims = _verified_id_token_claims(token_json, config) + if claims is None: return None email = claims.get(config.email_claim) diff --git a/omnigent/server/routes/device_auth.py b/omnigent/server/routes/device_auth.py index 5793d9afc4..6317b4c5cc 100644 --- a/omnigent/server/routes/device_auth.py +++ b/omnigent/server/routes/device_auth.py @@ -21,10 +21,17 @@ returns delegated access + refresh tokens. - ``POST /oauth/revoke`` — revoke a grant (backs client logout). -Mounted only in ``accounts`` auth mode (and only when -``OMNIGENT_DEVICE_GRANT_ENABLED`` is set). OIDC deployments delegate login -to the IdP via the cli-ticket flow (``/auth/cli-login``) and never use this -grant; header mode has no server-mintable identity. +Mounted in ``accounts`` and ``oidc`` auth modes (and only when +``OMNIGENT_DEVICE_GRANT_ENABLED`` is set). Header mode has no +server-mintable identity, so there is nothing to delegate from. + +Under OIDC the consent page bounces an unauthenticated (or stale) browser +through ``/auth/login``, which hands off to the IdP — so whether the user +proves themselves with a password, Google, or anything else is the IdP's +business and never this module's. The forced-re-authentication gate below +relies on that bounce carrying ``reauth=1`` through to ``prompt=login``; +without it the IdP would satisfy the bounce from its own session and the +gate would pass without the user having proven anything. See ``designs/DEVICE_AUTH.md`` for the full design + threat model. @@ -53,6 +60,7 @@ import os import secrets import time +from urllib.parse import quote import jwt from fastapi import APIRouter, HTTPException, Request @@ -257,23 +265,72 @@ def allow(self, key: str, now: float) -> bool: return True +def unsupported_reason(auth_provider: UnifiedAuthProvider) -> str | None: + """Why this provider cannot carry a device grant, or ``None`` if it can. + + The consent gate is only as strong as the login path's ability to + re-prompt a user the provider has already authenticated. Where that + cannot be demanded the grant is refused outright rather than issued + behind a gate that silently passes. + + - ``header`` — identity comes from an upstream proxy: no session to + delegate from, and no login to bounce a consenting browser through. + - ``github`` — configured as an OIDC source, but GitHub's OAuth + authorization endpoint is not OIDC and has no ``prompt`` parameter. + It would ignore ``prompt=login``, reuse its session, and return a + callback whose fresh ``iat`` clears the gate. + + :param auth_provider: The active provider. + :returns: A human-readable reason, or ``None`` when supported. + """ + source = auth_provider._source + if source not in ("accounts", "oidc"): + return f"{source!r} auth has no server-minted session to delegate from" + if source == "oidc": + config = auth_provider._oidc_config + if config is None: + return "oidc auth is selected but no OIDC config is present" + # Allowlisted, not denylisted. `provider_type` distinguishes real + # OIDC from OAuth 2.0 dialects modelled under the same source, and + # only the former can be asked to re-authenticate (`prompt=login`) + # and made to attest that it did (`auth_time`). Admitting an + # unaudited value by default would silently issue grants behind a + # gate that cannot hold — which is the failure this predicate exists + # to prevent. + if config.provider_type != "oidc": + return ( + f"{config.provider_type!r} is not full OIDC, so it cannot be asked to " + "re-authenticate an already signed-in user ('prompt=login') nor attest " + "that it did ('auth_time'), and the consent page's gate would not hold" + ) + return None + + def create_device_auth_router( auth_provider: UnifiedAuthProvider, device_grant_store: DeviceGrantStore, ) -> APIRouter: """Build the ``/oauth/*`` device-grant router. - :param auth_provider: The active provider. Must be ``accounts`` mode; - its cookie config supplies the HMAC signing key and public base URL. + :param auth_provider: The active provider. Must be ``accounts`` or + ``oidc`` mode; its cookie config supplies the HMAC signing key and + public base URL. :param device_grant_store: Persistence for device grants. :returns: APIRouter to mount at the app root. """ - if auth_provider._source != "accounts": - raise RuntimeError( - f"create_device_auth_router requires accounts auth (got {auth_provider._source!r})" - ) - cookie_config = auth_provider._accounts_config - assert cookie_config is not None, "accounts mode must have an accounts config" + reason = unsupported_reason(auth_provider) + if reason is not None: + raise RuntimeError(f"create_device_auth_router cannot build: {reason}") + # Both modes sign the same HS256 session cookie and both configs expose + # `cookie_secret`, `session_cookie_name` and `base_url` — see the + # AccountsConfig docstring and UnifiedAuthProvider._check_cookie, which + # picks between them exactly this way. + cookie_config = ( + auth_provider._oidc_config + if auth_provider._source == "oidc" + else auth_provider._accounts_config + ) + assert cookie_config is not None, f"{auth_provider._source} mode must have a cookie config" cookie_secret = cookie_config.cookie_secret base_url = cookie_config.base_url provider_name = auth_provider._source @@ -389,27 +446,43 @@ async def device_authorize(request: Request) -> Response: # ── Browser consent page ────────────────────────────────────── - def _bounce_to_login(user_code: str, *, reauth: bool) -> RedirectResponse: - """302 to the login page, returning to this consent URL afterward. + def _bounce_to_login(user_code: str) -> RedirectResponse: + """302 to the login page with ``reauth=1``, returning here afterward. - ``reauth`` adds ``&reauth=1`` so the login page forces a fresh - password submit instead of auto-bouncing an already-signed-in user - (which would loop back here with the same stale session). + Always forced, on every bounce. "No Omnigent session" does not mean + "no credential" under OIDC — the IdP holds its own, and would satisfy + an unforced bounce silently. The accounts SPA holds back its + auto-redirect and shows the password form; ``/auth/login`` forwards + this to the IdP as ``prompt=login``. """ login_url = auth_provider.login_url or "/login" return_to = f"/oauth/device?user_code={user_code}" if user_code else "/oauth/device" - query = f"return_to={html.escape(return_to, quote=True)}" - if reauth: - query += "&reauth=1" + # Percent-encoded, not HTML-escaped: this is a URL query value, and + # `html.escape` leaves `?`, `=`, `#` and `+` untouched, so a user_code + # carrying one would truncate or corrupt the round-trip. `&` survives + # as `&` today, which is why `reauth=1` still wins — an accident + # of the escaper, not a property worth depending on. + query = f"return_to={quote(return_to, safe='/')}&reauth=1" return RedirectResponse(url=f"{login_url}?{query}", status_code=302) - def _session_iat(request: Request) -> int | None: - """Return the ``iat`` (issue time) of the caller's session JWT. + def _session_auth_time(request: Request) -> int | None: + """When did this session's owner last *prove* who they are? - Read from the session cookie (accounts mode mints a fresh ``iat`` - on every ``/auth/login``, so this is effectively the last-login - time). ``None`` when absent/invalid. Used to enforce that consent - follows a login started FOR this device flow. + Deliberately not ``iat``. Minting a session proves only that a login + flow completed, and under OIDC that can happen with no user + involvement: an attacker who sends a victim a plain + ``/auth/login?return_to=/oauth/device?user_code=…`` link — no + ``reauth=1`` — gets the IdP to satisfy it from its own session, and + the resulting cookie carries an ``iat`` of *now*. Gating on ``iat`` + therefore let a crafted link walk straight past the consent check by + simply never asking for the re-authentication. + + ``auth_time`` is written only where a credential was actually + presented (see :func:`omnigent.server.oidc.mint_session_token`), so + it cannot be manufactured by starting a login flow. + + ``None`` when absent or invalid, which callers must treat as + unproven. """ token = request.cookies.get(cookie_config.session_cookie_name) if not token: @@ -418,8 +491,8 @@ def _session_iat(request: Request) -> int | None: payload = jwt.decode(token, cookie_secret, algorithms=["HS256"]) except jwt.InvalidTokenError: return None - iat = payload.get("iat") - return iat if isinstance(iat, int) else None + auth_time = payload.get("auth_time") + return auth_time if isinstance(auth_time, int) else None @router.get("/oauth/device") async def device_consent_page(request: Request) -> Response: @@ -435,15 +508,20 @@ async def device_consent_page(request: Request) -> Response: device flow began (session ``iat`` ≥ the grant's ``created_at``). A pre-existing session — however recent — is bounced back through the login page with ``reauth=1``, so approving a device grant always - costs a deliberate, fresh password entry. This closes the + costs a deliberate, fresh credential entry. This closes the reflex-approve phishing case: a victim already signed in can't bind - an attacker's grant with one click; they must re-enter their password + an attacker's grant with one click; they must re-authenticate against a screen naming the exact identity and client. + + Every bounce forces it, including the one for a caller with no + Omnigent session at all. Under OIDC that caller may still hold a + live IdP session, which would satisfy an unforced bounce without + their involvement. """ user_id = auth_provider.get_user_id(request) user_code = (request.query_params.get("user_code") or "").strip() if user_id is None: - return _bounce_to_login(user_code, reauth=False) + return _bounce_to_login(user_code) if not user_code: return HTMLResponse(_consent_html(prompt_for_code=True), status_code=200) @@ -456,13 +534,13 @@ async def device_consent_page(request: Request) -> Response: status_code=200, ) - # Force a fresh login when the current session predates this grant: - # only a login started for THIS flow (iat ≥ the grant's created_at) - # may approve. Bounce with reauth=1 so the login page re-prompts - # rather than auto-returning the stale session (which would loop). - session_iat = _session_iat(request) - if session_iat is None or session_iat < grant.created_at: - return _bounce_to_login(user_code, reauth=True) + # Only a credential presented for THIS flow may approve it: the + # proven-authentication time must postdate the grant. Bounce with + # reauth=1 so the login page re-prompts rather than auto-returning + # the existing session (which would loop). + proven_at = _session_auth_time(request) + if proven_at is None or proven_at < grant.created_at: + return _bounce_to_login(user_code) return HTMLResponse( _consent_html( @@ -497,10 +575,10 @@ async def device_approve(request: Request) -> Response: ) # Re-auth gate, enforced here too (not just on the consent GET): a - # stale session must not approve by POSTing directly. Only a login - # started for THIS flow (session iat ≥ the grant's created_at) passes. - session_iat = _session_iat(request) - if session_iat is None or session_iat < grant.created_at: + # stale session must not approve by POSTing directly. Only a + # credential proven for THIS flow passes. + proven_at = _session_auth_time(request) + if proven_at is None or proven_at < grant.created_at: return HTMLResponse( _consent_html( error="Your session is too old to approve this login. " diff --git a/tests/server/test_device_auth.py b/tests/server/test_device_auth.py index 21e6aac239..062e882df2 100644 --- a/tests/server/test_device_auth.py +++ b/tests/server/test_device_auth.py @@ -17,7 +17,7 @@ import logging import secrets -from collections.abc import Iterator +from collections.abc import Callable, Iterator from pathlib import Path import pytest @@ -31,21 +31,87 @@ # ── Router mount guard (unit) ───────────────────────────────────── -@pytest.mark.parametrize("source", ["oidc", "header"]) -def test_router_factory_rejects_non_accounts_mode(source: str, tmp_path: Path) -> None: - """The device grant is accounts-mode only. OIDC delegates login to the IdP - (cli-ticket flow) and never uses these routes; header can't mint identity. - ``create_device_auth_router`` must refuse to build for either.""" +def test_router_factory_rejects_header_mode(tmp_path: Path) -> None: + """Header mode has no server-mintable session: identity is asserted by an + upstream proxy, so there is nothing to delegate FROM and no login to bounce + a consenting browser through. The factory must refuse to build.""" from types import SimpleNamespace from omnigent.server.routes.device_auth import create_device_auth_router - provider = SimpleNamespace(_source=source) + provider = SimpleNamespace(_source="header") store = DeviceGrantStore(f"sqlite:///{tmp_path}/dg.db") - with pytest.raises(RuntimeError, match="accounts"): + with pytest.raises(RuntimeError, match="no server-minted session"): create_device_auth_router(provider, store) # type: ignore[arg-type] +def _oidc_provider(provider_type: str) -> object: + """A minimal OIDC-shaped provider stub for the mount-guard tests.""" + from types import SimpleNamespace + + return SimpleNamespace( + _source="oidc", + _oidc_config=SimpleNamespace( + cookie_secret=b"o" * 32, + base_url="https://omnigent.example.com", + session_cookie_name="__Host-ap_session", + provider_type=provider_type, + ), + _accounts_config=None, + login_url="/auth/login", + ) + + +def test_router_factory_rejects_github_oauth(tmp_path: Path) -> None: + """GitHub is an ``oidc`` source pointed at a NON-OIDC endpoint. + + ``OIDCConfig.from_env`` sets ``provider_type="github"`` and + ``authorization_endpoint=https://github.com/login/oauth/authorize`` — + plain OAuth 2.0, which has no ``prompt`` parameter. It would ignore + ``prompt=login``, reuse its session, and hand back a callback whose fresh + ``iat`` clears the consent gate. Refused outright: a grant issued behind a + gate that cannot hold is worse than no grant, because it looks protected. + """ + from omnigent.server.routes.device_auth import create_device_auth_router + + store = DeviceGrantStore(f"sqlite:///{tmp_path}/dg.db") + with pytest.raises(RuntimeError, match="not full OIDC"): + create_device_auth_router(_oidc_provider("github"), store) # type: ignore[arg-type] + + +def test_unsupported_reason_admits_standard_oidc_and_accounts() -> None: + """The predicate must not over-refuse: real OIDC and accounts both pass.""" + from types import SimpleNamespace + + from omnigent.server.routes.device_auth import unsupported_reason + + assert unsupported_reason(_oidc_provider("oidc")) is None # type: ignore[arg-type] + assert unsupported_reason(SimpleNamespace(_source="accounts")) is None # type: ignore[arg-type] + assert unsupported_reason(_oidc_provider("github")) is not None # type: ignore[arg-type] + assert unsupported_reason(SimpleNamespace(_source="header")) is not None # type: ignore[arg-type] + + +def test_router_factory_builds_in_oidc_mode_from_the_oidc_config(tmp_path: Path) -> None: + """OIDC owns the same HS256 session cookie accounts does, so the grant + works there — the IdP simply decides how the user proves themselves. + + The config must come from ``_oidc_config``: reading ``_accounts_config`` + (None under OIDC) would trip the assert, and reading the wrong secret would + sign device codes and refresh tokens with a key nothing else validates. + """ + from omnigent.server.routes.device_auth import create_device_auth_router + + provider = _oidc_provider("oidc") + store = DeviceGrantStore(f"sqlite:///{tmp_path}/dg.db") + + router = create_device_auth_router(provider, store) # type: ignore[arg-type] + + paths = {route.path for route in router.routes} # type: ignore[attr-defined] + assert "/oauth/device/authorize" in paths + assert "/oauth/token" in paths + assert "/oauth/revoke" in paths + + # ── Store invariants (unit) ─────────────────────────────────────── @@ -275,6 +341,129 @@ def _build_accounts_app( yield client +def _build_oidc_app( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + *, + provider_type: str = "oidc", + provider: object | None = None, +) -> Iterator[TestClient]: + """Build a real app through ``create_app`` with an OIDC auth provider. + + The provider is constructed directly rather than through + ``create_auth_provider`` so no IdP discovery request is made; everything + downstream — including the device-grant mount decision — is the + production path. ``provider`` overrides it outright, for modes that need + no OIDC config at all. + """ + monkeypatch.setenv("OMNIGENT_DEVICE_GRANT_ENABLED", "1") + if provider is None: + monkeypatch.delenv("OMNIGENT_AUTH_PROVIDER", raising=False) + + db_url = f"sqlite:///{tmp_path}/test.db" + from omnigent.db.utils import get_or_create_engine + from omnigent.runtime import init as init_runtime + from omnigent.runtime import telemetry + from omnigent.runtime.agent_cache import AgentCache + from omnigent.runtime.caps import RuntimeCaps + from omnigent.server.app import create_app + from omnigent.server.auth import UnifiedAuthProvider + from omnigent.server.oidc import OIDCConfig + from omnigent.stores.agent_store.sqlalchemy_store import SqlAlchemyAgentStore + from omnigent.stores.artifact_store.local import LocalArtifactStore + from omnigent.stores.comment_store.sqlalchemy_store import SqlAlchemyCommentStore + from omnigent.stores.conversation_store.sqlalchemy_store import SqlAlchemyConversationStore + from omnigent.stores.file_store.sqlalchemy_store import SqlAlchemyFileStore + from omnigent.stores.host_store import HostStore + from omnigent.stores.permission_store.sqlalchemy_store import SqlAlchemyPermissionStore + + get_or_create_engine(db_url) + telemetry.init() + permission_store = SqlAlchemyPermissionStore(db_url) + agent_store = SqlAlchemyAgentStore(db_url) + conversation_store = SqlAlchemyConversationStore(db_url) + file_store = SqlAlchemyFileStore(db_url) + comment_store = SqlAlchemyCommentStore(db_url) + host_store = HostStore(db_url) + artifact_store = LocalArtifactStore(str(tmp_path / "artifacts")) + agent_cache = AgentCache(artifact_store=artifact_store, cache_dir=tmp_path / "cache") + init_runtime( + agent_cache=agent_cache, + caps=RuntimeCaps(), + agent_store=agent_store, + file_store=file_store, + conversation_store=conversation_store, + artifact_store=artifact_store, + comment_store=comment_store, + ) + + config = OIDCConfig( + issuer="https://accounts.google.com", + client_id="cid", + client_secret="secret", + redirect_uri="http://localhost:8000/auth/callback", + cookie_secret=bytes.fromhex("bb" * 32), + scopes="openid email profile", + session_ttl_hours=8, + logout_redirect_uri=None, + allowed_domains=None, + provider_type=provider_type, + authorization_endpoint="https://accounts.google.com/o/oauth2/v2/auth", + token_endpoint="https://oauth2.googleapis.com/token", + jwks_uri="https://www.googleapis.com/oauth2/v3/certs", + userinfo_endpoint=None, + allow_invites=False, + ) + app = create_app( + agent_store=agent_store, + file_store=file_store, + conversation_store=conversation_store, + artifact_store=artifact_store, + agent_cache=agent_cache, + comment_store=comment_store, + permission_store=permission_store, + host_store=host_store, + auth_provider=provider or UnifiedAuthProvider(source="oidc", oidc_config=config), + ) + with TestClient(app) as client: + yield client + + +def test_create_app_mounts_the_grant_under_oidc( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The mount decision must be exercised through ``create_app`` itself. + + Testing the factory directly proves the router builds; it does not prove + the app ever calls it. A typo in the mount condition would silently leave + ``/oauth/*`` absent under OIDC with every other test still green. + """ + for client in _build_oidc_app(tmp_path, monkeypatch): + res = client.post("/oauth/device/authorize", json={"client_id": "polly"}) + assert res.status_code == 200, f"/oauth/device/authorize returned {res.status_code}" + assert res.json()["user_code"] + + +def test_create_app_refuses_the_grant_for_github_oauth( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """GitHub is an ``oidc`` source that cannot honour the re-auth gate. + + The app must boot without it rather than mount routes whose security + property does not hold. + """ + for client in _build_oidc_app(tmp_path, monkeypatch, provider_type="github"): + # Asserted against the route table, not a status code: the SPA + # catch-all answers unmounted paths, so "not 200" would also pass if + # the routes were mounted and merely erroring. + oauth_routes = [ + route.path + for route in client.app.routes # type: ignore[attr-defined] + if getattr(route, "path", "").startswith("/oauth/") + ] + assert oauth_routes == [], f"the grant must not mount for GitHub OAuth: {oauth_routes}" + + @pytest.fixture def app(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Iterator[TestClient]: yield from _build_accounts_app(tmp_path, monkeypatch) @@ -376,7 +565,20 @@ def test_consent_page_requires_login(app: TestClient) -> None: """The consent page bounces an unauthenticated visitor to login.""" r = app.get("/oauth/device?user_code=ABCD-2345", follow_redirects=False) assert r.status_code == 302 - assert "/login" in r.headers["location"] + assert r.headers["location"].startswith("/login?"), r.headers["location"] + + +def test_consent_forces_reauth_even_with_no_session_at_all(app: TestClient) -> None: + """The bounce for a caller with NO session must force re-authentication too. + + Under accounts, "no session" means no credential and the SPA shows the + password form either way. Under OIDC it does not: the caller may still + hold a live IdP session, which would satisfy an unforced bounce silently + and return a fresh ``iat`` that clears the consent gate. The consent page + cannot tell the two apart, so every bounce is forced. + """ + r = app.get("/oauth/device?user_code=ABCD-2345", follow_redirects=False) + assert "reauth=1" in r.headers["location"] def test_consent_forces_reauth_for_stale_session(app: TestClient) -> None: @@ -402,7 +604,8 @@ def test_consent_forces_reauth_for_stale_session(app: TestClient) -> None: r = app.get(f"/oauth/device?user_code={user_code}", follow_redirects=False) assert r.status_code == 302, r.text loc = r.headers["location"] - assert "/login" in loc and "reauth=1" in loc + assert loc.startswith("/login?"), loc + assert "reauth=1" in loc # Approve POST refuses the stale session too (defense in depth — a direct # POST must not bypass the GET's gate). @@ -533,7 +736,7 @@ def test_browser_consent_not_gated_by_client_secret(secret_app: TestClient) -> N r = secret_app.get("/oauth/device?user_code=ABCD-2345", follow_redirects=False) # Bounces to login (unauthenticated), NOT a 401 invalid_client. assert r.status_code == 302 - assert "/login" in r.headers["location"] + assert r.headers["location"].startswith("/login?"), r.headers["location"] def test_no_secret_configured_stays_public(app: TestClient) -> None: @@ -542,8 +745,12 @@ def test_no_secret_configured_stays_public(app: TestClient) -> None: assert r.status_code == 200, r.text -def _capture_app_warnings(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> list[str]: - """Build the accounts app and return WARNING messages from ``server.app``. +def _capture_app_warnings( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + builder: Callable[[], Iterator[TestClient]] | None = None, +) -> list[str]: + """Build an app and return WARNING messages from ``server.app``. Attaches a handler directly to the ``omnigent.server.app`` logger rather than using ``caplog``: the server's telemetry/logging init runs during the @@ -561,7 +768,7 @@ def emit(self, record: logging.LogRecord) -> None: handler = _Collector() logger.addHandler(handler) try: - for _ in _build_accounts_app(tmp_path, monkeypatch): + for _ in builder() if builder else _build_accounts_app(tmp_path, monkeypatch): break # build (and immediately tear down) so the mount runs finally: logger.removeHandler(handler) @@ -588,3 +795,116 @@ def test_startup_silent_when_client_secret_set( monkeypatch.setenv("OMNIGENT_DEVICE_CLIENT_SECRET", _SECRET) warnings = _capture_app_warnings(tmp_path, monkeypatch) assert not any("OMNIGENT_DEVICE_CLIENT_SECRET is not set" in m for m in warnings) + + +def _build_header_app(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Iterator[TestClient]: + """A header/proxy-mode app with the device grant requested.""" + monkeypatch.delenv("OMNIGENT_OIDC_ISSUER", raising=False) + monkeypatch.setenv("OMNIGENT_AUTH_PROVIDER", "header") + monkeypatch.setenv("OMNIGENT_AUTH_ENABLED", "1") + from omnigent.server.auth import UnifiedAuthProvider + + yield from _build_oidc_app( + tmp_path, monkeypatch, provider=UnifiedAuthProvider(source="header") + ) + + +def test_refusal_is_explained_in_header_mode( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Header mode must still learn WHY /oauth/* is missing. + + It is one of the two cases ``unsupported_reason`` exists to explain, and + also the mode where the auth router itself never mounts (``login_url`` is + None). Deciding device-grant support inside that mount block meant the + operator most likely to be confused saw only silence. + """ + warnings = _capture_app_warnings( + tmp_path, monkeypatch, builder=lambda: _build_header_app(tmp_path, monkeypatch) + ) + + assert any( + "OMNIGENT_DEVICE_GRANT_ENABLED is set" in message and "no server-minted session" in message + for message in warnings + ), warnings + + +def test_refusal_is_explained_for_github_oauth( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Same for a GitHub-backed OIDC deployment.""" + warnings = _capture_app_warnings( + tmp_path, + monkeypatch, + builder=lambda: _build_oidc_app(tmp_path, monkeypatch, provider_type="github"), + ) + + assert any( + "OMNIGENT_DEVICE_GRANT_ENABLED is set" in message and "not full OIDC" in message + for message in warnings + ), warnings + + +def test_a_supported_deployment_logs_no_refusal( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The predicate must not over-refuse: real OIDC mounts silently.""" + warnings = _capture_app_warnings( + tmp_path, monkeypatch, builder=lambda: _build_oidc_app(tmp_path, monkeypatch) + ) + + assert not any("routes are NOT mounted" in message for message in warnings), warnings + + +def test_unsupported_reason_refuses_an_unaudited_provider_type() -> None: + """Allowlisted, not denylisted. + + ``from_env`` yields only ``github`` or ``oidc`` today, so denying the one + known-bad value happened to be equivalent. The next OAuth dialect modelled + under this source — or a directly-constructed config — would have been + admitted by default, behind a gate that cannot hold for it. + """ + + from omnigent.server.routes.device_auth import unsupported_reason + + for provider_type in ("github", "gitlab", "oauth2", ""): + reason = unsupported_reason(_oidc_provider(provider_type)) # type: ignore[arg-type] + assert reason is not None, provider_type + assert repr(provider_type) in reason, "the reason must name the value it refused" + + assert unsupported_reason(_oidc_provider("oidc")) is None # type: ignore[arg-type] + + +def test_unsupported_reason_refuses_oidc_with_no_config() -> None: + """A missing config is not a supported deployment.""" + from types import SimpleNamespace + + from omnigent.server.routes.device_auth import unsupported_reason + + provider = SimpleNamespace(_source="oidc", _oidc_config=None, _accounts_config=None) + assert unsupported_reason(provider) is not None # type: ignore[arg-type] + + +def test_bounce_percent_encodes_the_return_to(app: TestClient) -> None: + """The consent URL must survive the bounce whatever the code contains. + + ``html.escape`` is an HTML escaper, not a URL one: it leaves ``?``, + ``=``, ``#`` and ``+`` untouched. A ``#`` therefore ended the URL and + turned everything after it into a fragment, silently truncating the + ``return_to`` the user is meant to come back to. Percent-encoding is the + correct tool for a query value. + + The ``&reauth=1`` that follows survived only because ``html.escape`` + happens to turn ``&`` into ``&`` — an accident of the escaper, not a + property the forced-re-auth invariant should rest on. + """ + from urllib.parse import parse_qs, urlparse + + r = app.get("/oauth/device?user_code=AB%23CD%26reauth=0", follow_redirects=False) + + assert r.status_code == 302 + query = parse_qs(urlparse(r.headers["location"]).query) + assert query["return_to"] == ["/oauth/device?user_code=AB#CD&reauth=0"], ( + "the consent URL must round-trip intact" + ) + assert query["reauth"] == ["1"], "and the forced re-auth must be the only one" diff --git a/tests/server/test_oidc_callback.py b/tests/server/test_oidc_callback.py index e92933b2df..54c0a0d345 100644 --- a/tests/server/test_oidc_callback.py +++ b/tests/server/test_oidc_callback.py @@ -186,7 +186,11 @@ async def _fake_post( yield client, keys -def _do_callback(client: TestClient, id_token: str) -> httpx.Response: +def _do_callback( + client: TestClient, + id_token: str, + extra_state: dict[str, object] | None = None, +) -> httpx.Response: """Drive a full ``/auth/callback`` with a valid state cookie. Crafts the signed state cookie the way ``/auth/login`` would, sets @@ -206,6 +210,7 @@ def _do_callback(client: TestClient, id_token: str) -> httpx.Response: "code_verifier": "verifier", "return_to": "/", "exp": int(time.time()) + 300, + **(extra_state or {}), }, _TEST_SECRET, algorithm="HS256", @@ -478,3 +483,154 @@ def test_callback_accepts_boolean_and_string_true( # Accepted as a verified identity → redirect + session. assert resp.status_code == 302, resp.text assert resp.cookies.get("ap_session") is not None + + +# ── Forced re-authentication is verified, not merely requested ──── + + +def test_reauth_login_accepts_a_proven_fresh_authentication( + callback_client: tuple[TestClient, _IdpKeys], +) -> None: + """An ``auth_time`` after the bounce is what the gate is looking for.""" + client, keys = callback_client + bounced_at = int(time.time()) + token = keys.sign_id_token( + {"email": "alice@example.com", "email_verified": True, "auth_time": bounced_at} + ) + + res = _do_callback(client, token, extra_state={"reauth_at": bounced_at}) + + assert res.status_code == 302, res.text + + +def test_reauth_login_refuses_a_session_the_idp_reused( + callback_client: tuple[TestClient, _IdpKeys], +) -> None: + """The whole point: ``prompt=login`` is a request, this is the check. + + An IdP that ignores it returns a token whose ``auth_time`` predates the + bounce. Accepting that would mint a session with a fresh ``iat``, clear + the device-grant consent gate, and delegate authority to a client the + user never re-authenticated for. + """ + client, keys = callback_client + bounced_at = int(time.time()) + token = keys.sign_id_token( + { + "email": "alice@example.com", + "email_verified": True, + # Signed in an hour before this token was issued, and never + # re-prompted. Both stamps are the IdP's own clock, so the gap is + # real regardless of whose clock is ahead. + "auth_time": bounced_at - 3600, + } + ) + + res = _do_callback(client, token, extra_state={"reauth_at": bounced_at}) + + assert res.status_code == 403 + assert "Re-authentication" in res.json()["error"] + assert "Set-Cookie" not in res.headers, "a refused re-auth must not mint a session" + + +def test_reauth_login_fails_closed_without_auth_time( + callback_client: tuple[TestClient, _IdpKeys], +) -> None: + """No ``auth_time`` is not a pass. + + ``max_age=0`` obliges a conforming IdP to report when it authenticated + the user. Silence is indistinguishable from a reused session, and this + gate is the only thing between a phished consent link and a grant. + """ + client, keys = callback_client + token = keys.sign_id_token({"email": "alice@example.com", "email_verified": True}) + + res = _do_callback(client, token, extra_state={"reauth_at": int(time.time())}) + + assert res.status_code == 403 + + +def test_an_ordinary_login_is_unaffected_by_the_reauth_check( + callback_client: tuple[TestClient, _IdpKeys], +) -> None: + """No ``reauth_at`` in the state ⇒ no ``auth_time`` requirement. + + The check must not leak into normal sign-in, where most IdPs omit the + claim entirely and every login would start failing. + """ + client, keys = callback_client + token = keys.sign_id_token({"email": "alice@example.com", "email_verified": True}) + + assert _do_callback(client, token).status_code == 302 + + +def _session_claims(res: httpx.Response) -> dict[str, object]: + """Decode the session cookie a successful callback set.""" + cookie = res.headers["set-cookie"] + token = cookie.split("ap_session=", 1)[1].split(";", 1)[0] + return jwt.decode(token, _TEST_SECRET, algorithms=["HS256"]) + + +def test_a_proven_login_stamps_auth_time_on_the_session( + callback_client: tuple[TestClient, _IdpKeys], +) -> None: + """The proof has to outlive the callback, or nothing downstream can see it. + + ``iat`` cannot carry it: every completed callback has a fresh one, + including the ones where the IdP reused its own session. + """ + client, keys = callback_client + now = int(time.time()) + token = keys.sign_id_token( + {"email": "alice@example.com", "email_verified": True, "auth_time": now} + ) + + claims = _session_claims(_do_callback(client, token)) + + assert isinstance(claims["auth_time"], int) + assert claims["auth_time"] >= now + + +def test_an_unmarked_login_link_cannot_manufacture_proof( + callback_client: tuple[TestClient, _IdpKeys], +) -> None: + """The bypass: reach /auth/login WITHOUT reauth=1 and the gate is skipped. + + ``/auth/login`` is a public GET that accepts any same-origin + ``return_to``, so an attacker who starts a grant can send the victim + ``/auth/login?return_to=/oauth/device?user_code=XXXX`` — no ``reauth=1``, + therefore no ``reauth_at``, therefore no forced re-authentication. The IdP + satisfies it from its own session and the callback completes normally. + + That must not produce a session the consent gate accepts. No proof, no + ``auth_time`` — the gate then bounces and demands one. + """ + client, keys = callback_client + token = keys.sign_id_token( + { + "email": "alice@example.com", + "email_verified": True, + # The IdP reused a session from an hour before this token. + "auth_time": int(time.time()) - 3600, + } + ) + + res = _do_callback(client, token) # no reauth_at: the crafted-link case + + assert res.status_code == 302, "an ordinary login must still succeed" + assert "auth_time" not in _session_claims(res), ( + "a reused IdP session must not be recorded as proof of identity" + ) + + +def test_an_idp_that_omits_auth_time_proves_nothing( + callback_client: tuple[TestClient, _IdpKeys], +) -> None: + """Silence is not proof, even on an ordinary login.""" + client, keys = callback_client + token = keys.sign_id_token({"email": "alice@example.com", "email_verified": True}) + + res = _do_callback(client, token) + + assert res.status_code == 302 + assert "auth_time" not in _session_claims(res) diff --git a/tests/server/test_oidc_reauth_prompt.py b/tests/server/test_oidc_reauth_prompt.py new file mode 100644 index 0000000000..aee7e9d2d9 --- /dev/null +++ b/tests/server/test_oidc_reauth_prompt.py @@ -0,0 +1,233 @@ +"""Forced re-authentication survives the handoff to an OIDC IdP. + +The device-grant consent page refuses to render for a session that predates +the grant being approved, and bounces through the login page with +``?reauth=1``. That is the anti-phishing gate: approving a device grant must +cost a deliberate credential entry, so a victim handed a one-click link cannot +bind an attacker's grant by reflex. + +In accounts mode the SPA login form enforces it — it holds back its +auto-redirect and demands a password. OIDC has no form to hold back; the IdP +owns the credential. The only way to ask is the OIDC ``prompt`` parameter, so +``/auth/login`` must forward ``reauth=1`` as ``prompt=login``. + +Without that forwarding the flow still *works* — the IdP satisfies the bounce +from its own session, returns a token, and the callback mints a session whose +fresh ``iat`` clears the consent gate. Nothing errors, no test fails, and the +gate silently approves a user who proved nothing. These tests pin the +parameter so that regression cannot pass quietly. +""" + +from __future__ import annotations + +from collections.abc import Iterator +from pathlib import Path +from urllib.parse import parse_qs, urlparse + +import pytest +from fastapi import FastAPI +from fastapi.testclient import TestClient + +from omnigent.server.admin_list import AdminList +from omnigent.server.auth import UnifiedAuthProvider +from omnigent.server.oidc import OIDCConfig +from omnigent.server.routes.auth import create_auth_router +from omnigent.stores.permission_store.sqlalchemy_store import SqlAlchemyPermissionStore + +_TEST_SECRET = bytes.fromhex("aa" * 32) + + +def _oidc_config() -> OIDCConfig: + """An OIDC config over plain HTTP so TestClient handles cookies.""" + return OIDCConfig( + issuer="https://accounts.google.com", + client_id="cid", + client_secret="secret", + redirect_uri="http://localhost:8000/auth/callback", + cookie_secret=_TEST_SECRET, + scopes="openid email profile", + session_ttl_hours=8, + logout_redirect_uri=None, + allowed_domains=None, + provider_type="oidc", + authorization_endpoint="https://accounts.google.com/o/oauth2/v2/auth", + token_endpoint="https://oauth2.googleapis.com/token", + jwks_uri="https://www.googleapis.com/oauth2/v3/certs", + userinfo_endpoint=None, + allow_invites=False, + ) + + +@pytest.fixture +def oidc_client(tmp_path: Path, db_uri: str) -> Iterator[TestClient]: + """An OIDC auth router mounted on a TestClient, redirects not followed.""" + perm_store = SqlAlchemyPermissionStore(db_uri) + admins = tmp_path / "admins" + admins.write_text("") + provider = UnifiedAuthProvider(source="oidc", oidc_config=_oidc_config()) + + app = FastAPI() + app.include_router( + create_auth_router(provider, perm_store, AdminList(admins)), + prefix="/auth", + ) + with TestClient(app) as client: + yield client + + +def _authorize_params(client: TestClient, query: str) -> dict[str, list[str]]: + """Follow ``/auth/login`` one hop and return the IdP authorize params.""" + res = client.get(f"/auth/login{query}", follow_redirects=False) + assert res.status_code == 302, res.status_code + return parse_qs(urlparse(res.headers["location"]).query) + + +def test_reauth_forwards_prompt_login_to_the_idp(oidc_client: TestClient) -> None: + """``?reauth=1`` must reach the IdP as ``prompt=login``. + + This is the whole gate under OIDC. Drop the parameter and the IdP + re-authenticates the user silently from its own session. + """ + params = _authorize_params(oidc_client, "?reauth=1&return_to=/oauth/device") + + assert params.get("prompt") == ["login"] + # `prompt` alone is only a request. `max_age=0` is what obliges a + # conforming IdP to return `auth_time`, which is what the callback then + # verifies — drop it and the gate degrades to asking politely. + assert params.get("max_age") == ["0"] + # The rest of the request must be unchanged — PKCE and state still apply. + assert params["code_challenge_method"] == ["S256"] + assert params["response_type"] == ["code"] + assert params["code_challenge"] and params["state"] + + +def test_an_ordinary_login_does_not_re_prompt(oidc_client: TestClient) -> None: + """No ``reauth`` ⇒ no ``prompt``. + + Sending ``prompt=login`` unconditionally would force a password entry on + every single sign-in, which is how a security control ends up switched + off by whoever finds it annoying. + """ + for query in ("", "?return_to=/sessions"): + params = _authorize_params(oidc_client, query) + assert "prompt" not in params + assert "max_age" not in params + + +@pytest.mark.parametrize("raw", ["0", "true", "yes", "", "1 ", "TRUE"]) +def test_only_an_exact_1_forces_re_authentication(oidc_client: TestClient, raw: str) -> None: + """Anything other than exactly ``1`` is not a re-auth request. + + Matched strictly because the consent page is the only caller and it sends + exactly ``1``; accepting loose truthy spellings would let an unrelated + query param turn on a re-prompt nobody asked for. + """ + assert "prompt" not in _authorize_params(oidc_client, f"?reauth={raw}") + + +def test_re_authentication_still_round_trips_the_return_to(oidc_client: TestClient) -> None: + """The consent URL must survive the re-auth bounce. + + Losing it would land the user on the dashboard after re-entering their + password, with the pending grant abandoned and no way back to it. + """ + import jwt + + # Percent-encoded exactly as `_bounce_to_login` emits it. Decoding the + # stored value before asserting would have passed under either encoding + # and hidden whether the query survived at all. + oidc_client.get( + "/auth/login?reauth=1&return_to=%2Foauth%2Fdevice%3Fuser_code%3DK7M2-QP9X", + follow_redirects=False, + ) + cookie = oidc_client.cookies.get("ap_auth_state") + assert cookie is not None + claims = jwt.decode(cookie, _TEST_SECRET, algorithms=["HS256"]) + + assert claims["return_to"] == "/oauth/device?user_code=K7M2-QP9X" + + +# ── The consent page against a REAL OIDCConfig ──────────────────── + + +def test_consent_page_renders_for_a_real_oidc_session_cookie(tmp_path: Path) -> None: + """A genuine OIDC session must satisfy the consent page's own cookie read. + + ``_session_iat`` reads ``cookie_config.session_cookie_name`` and verifies + with ``cookie_config.cookie_secret``. If either diverged from what + ``/auth/callback`` actually sets, it would return ``None`` on every + request and the consent page would bounce forever — a login loop with no + error and no failing test. A hand-built ``SimpleNamespace`` cannot catch + that; this drives the real config and the real minting helper. + """ + import time + + from fastapi import FastAPI + + from omnigent.server.device_grant_store import DeviceGrantStore + from omnigent.server.oidc import mint_session_cookie + from omnigent.server.routes.device_auth import create_device_auth_router + + config = _oidc_config() + provider = UnifiedAuthProvider(source="oidc", oidc_config=config) + store = DeviceGrantStore(f"sqlite:///{tmp_path}/dg.db") + + app = FastAPI() + app.include_router(create_device_auth_router(provider, store)) + + with TestClient(app) as client: + res = client.post("/oauth/device/authorize", json={"client_id": "polly"}) + assert res.status_code == 200, res.text + user_code = res.json()["user_code"] + + # A session with no proven authentication — the shape an ordinary + # OIDC login produces when the IdP reuses its own session. + unproven = mint_session_cookie("alice@example.com", config.cookie_secret, 8, "oidc") + client.cookies.set(config.session_cookie_name, unproven) + bounced = client.get(f"/oauth/device?user_code={user_code}", follow_redirects=False) + + # An authentication proven AFTER the grant began, exactly as the + # forced bounce produces. + time.sleep(1) + proven = mint_session_cookie( + "alice@example.com", + config.cookie_secret, + 8, + "oidc", + auth_time=int(time.time()), + ) + client.cookies.set(config.session_cookie_name, proven) + page = client.get(f"/oauth/device?user_code={user_code}", follow_redirects=False) + + assert bounced.status_code == 302, "an unproven session must not reach consent" + # `/auth/login`, not the accounts SPA's `/login` — the other half of + # `login_url`, which a `"/login" in location` substring cannot tell apart. + assert bounced.headers["location"].startswith("/auth/login?"), bounced.headers["location"] + assert "reauth=1" in bounced.headers["location"] + + assert page.status_code == 200, f"consent bounced instead of rendering: {page.headers}" + assert "alice@example.com" in page.text, "the consent screen must name the identity" + assert "polly" in page.text, "and the client asking for access" + + +def test_reauth_is_signed_into_the_state_cookie(oidc_client: TestClient) -> None: + """`/auth/login` must record the demand where the callback can trust it. + + The callback refuses a login that failed to re-authenticate only when + the state carries ``reauth_at``. Signed into the cookie rather than read + back off the URL, so it cannot be stripped by editing the redirect. + """ + import jwt + + oidc_client.get("/auth/login?reauth=1&return_to=/oauth/device", follow_redirects=False) + claims = jwt.decode(oidc_client.cookies["ap_auth_state"], _TEST_SECRET, algorithms=["HS256"]) + assert isinstance(claims.get("reauth_at"), int) + + +def test_an_ordinary_login_signs_no_reauth_marker(oidc_client: TestClient) -> None: + """Without it the callback must not demand proof of every sign-in.""" + import jwt + + oidc_client.get("/auth/login?return_to=/sessions", follow_redirects=False) + claims = jwt.decode(oidc_client.cookies["ap_auth_state"], _TEST_SECRET, algorithms=["HS256"]) + assert "reauth_at" not in claims