Skip to content

Commit ca94fad

Browse files
committed
fix(auth): verify the forced re-authentication instead of only asking for it
Addresses the remaining review findings on this branch. All three come back to the same gap: the gate was requested but never checked. ## prompt=login was a request with no verification `prompt=login` asks an IdP to re-authenticate. Nothing confirmed it obeyed, and `auth_time` appeared nowhere in the codebase. A non-conforming or misconfigured IdP satisfies the bounce from its own session, the callback mints a cookie with a fresh `iat`, and the consent gate passes — the same silent failure this branch exists to prevent, one level further out. The bounce now sends `max_age=0` alongside `prompt=login`, which obliges a conforming IdP to return `auth_time`. `/auth/login` signs the bounce time into the state cookie as `reauth_at`, so the requirement cannot be stripped by editing the URL, and `/auth/callback` refuses with 403 — minting no session — unless the returned `auth_time` postdates it. A missing `auth_time` is refused as well. Silence is indistinguishable from a reused session, and this is the only gate between a phished consent link and a delegated grant. Ordinary logins carry no `reauth_at` and are untouched; most IdPs omit the claim, and requiring it everywhere would break every sign-in. Verifying it meant reading the id_token twice, so the validated decode is now `_verified_id_token_claims`, shared by the email resolver and the freshness check. One validated path, so no caller can read a claim out of an unverified token. GitHub short-circuits `reauth` at the login route as well as being refused the grant: it can neither be asked to re-authenticate nor report that it did, and setting `reauth_at` for it would fail every such login at the callback instead. ## The mount decision was never exercised through create_app The factory tests prove the router builds; they never proved the app calls it. A typo in the mount condition would leave `/oauth/*` absent under OIDC with the whole suite green — which is exactly the failure the condition was widened to avoid. `create_app` is now driven directly with a real OIDC provider (constructed in-process, so no IdP discovery request), asserting the grant is reachable, and that it is absent for GitHub. The GitHub case asserts against the route table rather than a status code: the SPA catch-all answers unmounted paths, so "not 200" would also pass with the routes mounted and merely erroring. ## A SimpleNamespace config proved nothing about the real cookie `_session_iat` reads `cookie_config.session_cookie_name` and verifies with `cookie_config.cookie_secret`. If either diverged from what `/auth/callback` 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 stub cannot catch that. Added a test that mints a session through the real `mint_session_cookie` from a real `OIDCConfig` and asserts the consent page renders, naming the identity and the client. ## Verified - 280 tests pass across the auth suite, integration and e2e - Mutation-checked: dropping the callback verification fails 2 tests, treating a missing auth_time as a pass fails 1 - ruff, ruff format, mypy clean
1 parent 204f54f commit ca94fad

5 files changed

Lines changed: 363 additions & 24 deletions

File tree

designs/DEVICE_AUTH.md

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -206,6 +206,16 @@ session. Under accounts that changes nothing — no session means no credential,
206206
so the form is shown regardless — but under OIDC the caller may still hold a
207207
live IdP session, and the consent page cannot tell the two cases apart.
208208

209+
**Requested, then verified.** `prompt=login` is only a request, so the bounce
210+
also sends `max_age=0`, which obliges a conforming IdP to report the moment it
211+
authenticated the user in the `auth_time` claim. `/auth/login` signs the bounce
212+
time into the state cookie as `reauth_at`, and `/auth/callback` refuses (403,
213+
no session minted) unless the returned `auth_time` postdates it. A missing
214+
`auth_time` is refused too: silence is indistinguishable from a reused session,
215+
and this gate is the only thing between a phished consent link and a delegated
216+
grant. Ordinary logins carry no `reauth_at` and are unaffected — most IdPs omit
217+
the claim, and requiring it everywhere would break every sign-in.
218+
209219
**Why GitHub is excluded.** `OIDCConfig.from_env` accepts GitHub as an `oidc`
210220
source, but points it at `https://github.com/login/oauth/authorize` — plain
211221
OAuth 2.0, which has no `prompt` parameter. `prompt=login` would be ignored,

omnigent/server/routes/auth.py

Lines changed: 104 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,8 @@
4646
_AUTH_STATE_COOKIE_PLAIN = "ap_auth_state"
4747
_AUTH_STATE_TTL_SECONDS = 300 # 5 minutes
4848
_CLI_TICKET_TTL_SECONDS = 300 # 5 minutes
49+
# Tolerance when comparing the IdP's `auth_time` against our own clock.
50+
_REAUTH_CLOCK_SKEW_SECONDS = 60
4951
# How long an OIDC invite URL stays redeemable. Matches the accounts
5052
# provider's default invite window (72h) — long enough to share
5153
# out-of-band, short enough to bound exposure of an unused link.
@@ -170,8 +172,10 @@ async def login(request: Request) -> Response:
170172
invite = request.query_params.get("invite") if _invites_enabled else None
171173
# Forced re-authentication, requested by the device-grant consent page.
172174
# Without it the IdP satisfies the bounce from its own session and the
173-
# consent gate passes on a user who proved nothing.
174-
reauth = request.query_params.get("reauth") == "1"
175+
# consent gate passes on a user who proved nothing. GitHub OAuth has
176+
# no way to demand or report it, so it never gets here — see
177+
# `device_auth.unsupported_reason`.
178+
reauth = request.query_params.get("reauth") == "1" and config.provider_type != "github"
175179

176180
# Store state + code_verifier in a short-lived signed cookie.
177181
state_payload: dict[str, str | int] = {
@@ -184,6 +188,10 @@ async def login(request: Request) -> Response:
184188
state_payload["ticket"] = ticket
185189
if invite:
186190
state_payload["invite"] = invite
191+
if reauth:
192+
# Signed, so the callback's freshness check cannot be removed by
193+
# editing the URL.
194+
state_payload["reauth_at"] = int(time.time())
187195
state_jwt = jwt.encode(state_payload, config.cookie_secret, algorithm="HS256")
188196

189197
# Build the authorization URL.
@@ -198,9 +206,10 @@ async def login(request: Request) -> Response:
198206
}
199207
if reauth:
200208
# OIDC Core 3.1.2.1: re-prompt even when the IdP has a session.
201-
# Only on this pathas a default it would cost a password on
202-
# every sign-in, and get switched off.
209+
# `max_age=0` makes it enforceable — it obliges a conforming IdP
210+
# to return `auth_time`, which the callback then verifies.
203211
params["prompt"] = "login"
212+
params["max_age"] = "0"
204213
auth_url = config.authorization_endpoint + "?" + urlencode(params)
205214

206215
response = RedirectResponse(url=auth_url, status_code=302)
@@ -318,6 +327,17 @@ async def callback(request: Request) -> Response:
318327
else:
319328
email = _resolve_oidc_email(token_json, config)
320329

330+
# This login was demanded by a device-grant consent bounce, so a
331+
# session the IdP simply reused is not good enough.
332+
reauth_at = state_payload.get("reauth_at")
333+
if isinstance(reauth_at, int) and not _reauthenticated_after(
334+
token_json, config, reauth_at
335+
):
336+
return JSONResponse(
337+
status_code=403,
338+
content={"error": "Re-authentication was required but did not occur"},
339+
)
340+
321341
if not email:
322342
return JSONResponse(
323343
status_code=400,
@@ -781,6 +801,84 @@ def _claim_is_verified_true(value: object) -> bool:
781801
return isinstance(value, str) and value.strip().lower() == "true"
782802

783803

804+
def _verified_id_token_claims(
805+
token_json: dict[str, object],
806+
config: OIDCConfig,
807+
) -> dict[str, object] | None:
808+
"""Validate the ``id_token`` and return its claims.
809+
810+
Checks the JWT signature against the IdP's JWKS and verifies ``iss``
811+
and ``aud``. Shared by every consumer so no caller can read a claim out
812+
of an unverified token.
813+
814+
:param token_json: The token endpoint response JSON.
815+
:param config: The OIDC configuration with JWKS URI and expected
816+
issuer/audience.
817+
:returns: The verified claims, or ``None`` when the token is missing,
818+
unverifiable, or the config has no JWKS URI.
819+
"""
820+
id_token = token_json.get("id_token")
821+
if not isinstance(id_token, str) or not id_token:
822+
return None
823+
if config.jwks_uri is None:
824+
_logger.warning("Rejecting id_token: OIDC configuration has no JWKS URI")
825+
return None
826+
827+
try:
828+
jwks_client = jwt.PyJWKClient(config.jwks_uri)
829+
signing_key = jwks_client.get_signing_key_from_jwt(id_token)
830+
claims: dict[str, object] = jwt.decode(
831+
id_token,
832+
signing_key.key,
833+
algorithms=["RS256", "RS384", "RS512", "ES256", "ES384", "ES512"],
834+
audience=config.client_id,
835+
issuer=config.issuer,
836+
)
837+
except jwt.InvalidTokenError as exc:
838+
_logger.warning("id_token validation failed: %s", exc)
839+
return None
840+
841+
return claims
842+
843+
844+
def _reauthenticated_after(
845+
token_json: dict[str, object],
846+
config: OIDCConfig,
847+
not_before: int,
848+
) -> bool:
849+
"""Did the IdP actually re-authenticate the user for this login?
850+
851+
``prompt=login`` and ``max_age=0`` are requests. This is the check that
852+
they were honoured: a conforming IdP that re-authenticates must report
853+
when it did, via the ``auth_time`` claim, and that moment has to fall
854+
after the bounce that demanded it.
855+
856+
Fails closed. A missing ``auth_time`` means the IdP did not answer the
857+
question, which is indistinguishable from it having reused an existing
858+
session — and this is the only gate standing between a phished consent
859+
link and a delegated grant.
860+
861+
:param token_json: The token endpoint response JSON.
862+
:param config: The OIDC configuration.
863+
:param not_before: Epoch seconds the re-authentication must postdate.
864+
:returns: True only on a proven fresh authentication.
865+
"""
866+
claims = _verified_id_token_claims(token_json, config)
867+
if claims is None:
868+
return False
869+
870+
auth_time = claims.get("auth_time")
871+
if not isinstance(auth_time, int):
872+
_logger.warning(
873+
"Forced re-authentication could not be verified: the id_token has no "
874+
"integer auth_time claim, so the IdP may have reused an existing session"
875+
)
876+
return False
877+
878+
# Small tolerance for clock skew between us and the IdP.
879+
return auth_time >= not_before - _REAUTH_CLOCK_SKEW_SECONDS
880+
881+
784882
def _resolve_oidc_email(
785883
token_json: dict[str, object],
786884
config: OIDCConfig,
@@ -819,25 +917,8 @@ def _resolve_oidc_email(
819917
``email_verified`` is not truthy (and verification is not
820918
skipped via config).
821919
"""
822-
id_token = token_json.get("id_token")
823-
if not isinstance(id_token, str) or not id_token:
824-
return None
825-
if config.jwks_uri is None:
826-
_logger.warning("Rejecting id_token: OIDC configuration has no JWKS URI")
827-
return None
828-
829-
try:
830-
jwks_client = jwt.PyJWKClient(config.jwks_uri)
831-
signing_key = jwks_client.get_signing_key_from_jwt(id_token)
832-
claims = jwt.decode(
833-
id_token,
834-
signing_key.key,
835-
algorithms=["RS256", "RS384", "RS512", "ES256", "ES384", "ES512"],
836-
audience=config.client_id,
837-
issuer=config.issuer,
838-
)
839-
except jwt.InvalidTokenError as exc:
840-
_logger.warning("id_token validation failed: %s", exc)
920+
claims = _verified_id_token_claims(token_json, config)
921+
if claims is None:
841922
return None
842923

843924
email = claims.get(config.email_claim)

tests/server/test_device_auth.py

Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -341,6 +341,126 @@ def _build_accounts_app(
341341
yield client
342342

343343

344+
def _build_oidc_app(
345+
tmp_path: Path,
346+
monkeypatch: pytest.MonkeyPatch,
347+
*,
348+
provider_type: str = "oidc",
349+
) -> Iterator[TestClient]:
350+
"""Build a real app through ``create_app`` with an OIDC auth provider.
351+
352+
The provider is constructed directly rather than through
353+
``create_auth_provider`` so no IdP discovery request is made; everything
354+
downstream — including the device-grant mount decision — is the
355+
production path.
356+
"""
357+
monkeypatch.setenv("OMNIGENT_DEVICE_GRANT_ENABLED", "1")
358+
monkeypatch.delenv("OMNIGENT_AUTH_PROVIDER", raising=False)
359+
360+
db_url = f"sqlite:///{tmp_path}/test.db"
361+
from omnigent.db.utils import get_or_create_engine
362+
from omnigent.runtime import init as init_runtime
363+
from omnigent.runtime import telemetry
364+
from omnigent.runtime.agent_cache import AgentCache
365+
from omnigent.runtime.caps import RuntimeCaps
366+
from omnigent.server.app import create_app
367+
from omnigent.server.auth import UnifiedAuthProvider
368+
from omnigent.server.oidc import OIDCConfig
369+
from omnigent.stores.agent_store.sqlalchemy_store import SqlAlchemyAgentStore
370+
from omnigent.stores.artifact_store.local import LocalArtifactStore
371+
from omnigent.stores.comment_store.sqlalchemy_store import SqlAlchemyCommentStore
372+
from omnigent.stores.conversation_store.sqlalchemy_store import SqlAlchemyConversationStore
373+
from omnigent.stores.file_store.sqlalchemy_store import SqlAlchemyFileStore
374+
from omnigent.stores.host_store import HostStore
375+
from omnigent.stores.permission_store.sqlalchemy_store import SqlAlchemyPermissionStore
376+
377+
get_or_create_engine(db_url)
378+
telemetry.init()
379+
permission_store = SqlAlchemyPermissionStore(db_url)
380+
agent_store = SqlAlchemyAgentStore(db_url)
381+
conversation_store = SqlAlchemyConversationStore(db_url)
382+
file_store = SqlAlchemyFileStore(db_url)
383+
comment_store = SqlAlchemyCommentStore(db_url)
384+
host_store = HostStore(db_url)
385+
artifact_store = LocalArtifactStore(str(tmp_path / "artifacts"))
386+
agent_cache = AgentCache(artifact_store=artifact_store, cache_dir=tmp_path / "cache")
387+
init_runtime(
388+
agent_cache=agent_cache,
389+
caps=RuntimeCaps(),
390+
agent_store=agent_store,
391+
file_store=file_store,
392+
conversation_store=conversation_store,
393+
artifact_store=artifact_store,
394+
comment_store=comment_store,
395+
)
396+
397+
config = OIDCConfig(
398+
issuer="https://accounts.google.com",
399+
client_id="cid",
400+
client_secret="secret",
401+
redirect_uri="http://localhost:8000/auth/callback",
402+
cookie_secret=bytes.fromhex("bb" * 32),
403+
scopes="openid email profile",
404+
session_ttl_hours=8,
405+
logout_redirect_uri=None,
406+
allowed_domains=None,
407+
provider_type=provider_type,
408+
authorization_endpoint="https://accounts.google.com/o/oauth2/v2/auth",
409+
token_endpoint="https://oauth2.googleapis.com/token",
410+
jwks_uri="https://www.googleapis.com/oauth2/v3/certs",
411+
userinfo_endpoint=None,
412+
allow_invites=False,
413+
)
414+
app = create_app(
415+
agent_store=agent_store,
416+
file_store=file_store,
417+
conversation_store=conversation_store,
418+
artifact_store=artifact_store,
419+
agent_cache=agent_cache,
420+
comment_store=comment_store,
421+
permission_store=permission_store,
422+
host_store=host_store,
423+
auth_provider=UnifiedAuthProvider(source="oidc", oidc_config=config),
424+
)
425+
with TestClient(app) as client:
426+
yield client
427+
428+
429+
def test_create_app_mounts_the_grant_under_oidc(
430+
tmp_path: Path, monkeypatch: pytest.MonkeyPatch
431+
) -> None:
432+
"""The mount decision must be exercised through ``create_app`` itself.
433+
434+
Testing the factory directly proves the router builds; it does not prove
435+
the app ever calls it. A typo in the mount condition would silently leave
436+
``/oauth/*`` absent under OIDC with every other test still green.
437+
"""
438+
for client in _build_oidc_app(tmp_path, monkeypatch):
439+
res = client.post("/oauth/device/authorize", json={"client_id": "polly"})
440+
assert res.status_code == 200, f"/oauth/device/authorize returned {res.status_code}"
441+
assert res.json()["user_code"]
442+
443+
444+
def test_create_app_refuses_the_grant_for_github_oauth(
445+
tmp_path: Path, monkeypatch: pytest.MonkeyPatch
446+
) -> None:
447+
"""GitHub is an ``oidc`` source that cannot honour the re-auth gate.
448+
449+
The app must boot without it rather than mount routes whose security
450+
property does not hold.
451+
"""
452+
for client in _build_oidc_app(tmp_path, monkeypatch, provider_type="github"):
453+
# Asserted against the route table, not a status code: the SPA
454+
# catch-all answers unmounted paths, so "not 200" would also pass if
455+
# the routes were mounted and merely erroring.
456+
oauth_routes = [
457+
route.path
458+
for route in client.app.routes # type: ignore[attr-defined]
459+
if getattr(route, "path", "").startswith("/oauth/")
460+
]
461+
assert oauth_routes == [], f"the grant must not mount for GitHub OAuth: {oauth_routes}"
462+
463+
344464
@pytest.fixture
345465
def app(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Iterator[TestClient]:
346466
yield from _build_accounts_app(tmp_path, monkeypatch)

0 commit comments

Comments
 (0)