diff --git a/changelog.d/tsk-sonaie-identity-honours-token-rotation.md b/changelog.d/tsk-sonaie-identity-honours-token-rotation.md new file mode 100644 index 000000000..7bf0282c3 --- /dev/null +++ b/changelog.d/tsk-sonaie-identity-honours-token-rotation.md @@ -0,0 +1,5 @@ +### Security +- Rotating an agent identity's tokens (`POST /api/agents/registry/{id}/rotate-tokens`) now also kills the surfaces that authenticate by identity alone. `check_agent_identity` skipped the `token_min_iat` cutoff that `check_agent_scope` and `check_agent_scope_for_project` both enforce, so a superseded token still passed on the routes that need no scope grant -- creating a scope request (the one route whose purpose is asking for more privilege), the agent decisions routes, container-provisioning requests and the auth-request flow. A rotated token is now refused there, and on an unrouted path it gets the dead-credential 401 instead of the wrong-URL 404. + +### Internal +- The `token_min_iat` rotation-cutoff check was copy-pasted into three places in `tinyagentos/agent_token_auth.py` (`_verify_agent_scope`, `check_agent_identity`, `check_agent_project_grants`); extracted into a single `_enforce_rotation_cutoff` helper so a future change to the cutoff semantics cannot silently apply to only some of them. diff --git a/docs/agent-coordination.md b/docs/agent-coordination.md index 865698739..1a846b554 100644 --- a/docs/agent-coordination.md +++ b/docs/agent-coordination.md @@ -401,6 +401,16 @@ A registered external agent authenticates with its registry JWT (`Authorization: Bearer`) and reaches exactly the routes its granted SCOPES allow, nothing else: the middleware allowlist is a closed set, no skeleton key. +**A refused request says WHICH thing was wrong, and a dead credential is never +flattered.** Off the allowlist the handler never runs, and the status code +splits three ways: no route matches the path -> 404 (a live credential at a +wrong URL); the route exists but this token is not authorised for it -> 401 +from the session gate (a right URL, an unauthorised credential); the credential +itself is dead -- revoked, or superseded by `rotate-tokens` because its `iat` +predates the identity's `token_min_iat` -> 401 on every path, existing or not. +An anonymous caller gets 401 everywhere, so status codes cannot be used to +enumerate routes. + A SEPARATE credential class exists for the Agent-as-a-Model surface: `GET /v1/models` and `POST /v1/chat/completions` are reachable without a session using a CONSENT KEY (`Authorization: Bearer sk-taosagent-...`, minted @@ -743,7 +753,13 @@ that SAME canonical_id instead: never escalate an existing identity. The middleware allowlist exposes only the create path and the two READ paths below to a registry JWT -- approve/deny are POST with an extra trailing segment and match no pattern, so an agent can - never self-approve; the routes re-check identity == canonical_id. At most + never self-approve; the routes re-check identity == canonical_id. + The bearer must also be LIVE: identity-only auth honours the identity's + `token_min_iat` rotation cutoff, so a token superseded by `rotate-tokens` can + no longer create a request (the route answers the same existence-hiding 404 + it gives any other bad credential). This matters more here than anywhere + else -- it is the one route whose whole purpose is asking for MORE + privilege, and it needs no scope grant to reach. At most `_SCOPE_REQUEST_PENDING_CAP` (10) requests may be pending per canonical_id; further creates are 429. The cap is compared INSIDE the store's INSERT, not counted by the route first, so a burst of concurrent self-requests cannot diff --git a/tests/test_agent_scope_requests.py b/tests/test_agent_scope_requests.py index 371fefc46..fa000c7ea 100644 --- a/tests/test_agent_scope_requests.py +++ b/tests/test_agent_scope_requests.py @@ -10,6 +10,7 @@ from __future__ import annotations import asyncio +import time import pytest import pytest_asyncio @@ -144,6 +145,46 @@ async def test_agent_can_self_request_with_own_token(client, monkeypatch, tmp_pa await env.close() +@pytest.mark.asyncio +async def test_agent_cannot_self_request_with_rotated_token( + client, monkeypatch, tmp_path +): + """Rotating an identity's tokens must kill this route too. + + Self-request is authorised by `check_agent_identity` alone -- no scope + grant is required, because the whole point is asking for a scope you do + not have. So if that check skips the rotation cutoff, a leaked token + survives `rotate-tokens` on the one route that can widen its own + privileges. Before the cutoff was enforced this returned 200 and left a + live `pending` request behind. + + The status is 404, not 401: `_authorize_scope_request_creation` + deliberately folds every bad-credential outcome into the not-found body so + the pair (unknown target 404, existing target 401/403) cannot be used as an + existence oracle. The load-bearing assertion is that nothing was created. + """ + env = await _wire(client, monkeypatch, tmp_path) + try: + cid = await _register_active(env) + token = env.agent_token(cid) + # Owner rotates: every token minted before now is superseded. + await env.registry.bump_token_min_iat(cid, int(time.time()) + 3600) + + app = client._transport.app + async with AsyncClient( + transport=ASGITransport(app=app), base_url="http://test" + ) as bare: + resp = await bare.post( + f"/api/agents/registry/{cid}/scope-requests", + headers={"Authorization": f"Bearer {token}"}, + json={"requested_scopes": ["a2a_send"]}, + ) + assert resp.status_code == 404, resp.text + assert await env.scope_store.count_pending_for(cid) == 0 + finally: + await env.close() + + @pytest.mark.asyncio async def test_concurrent_self_requests_cannot_bypass_pending_cap( client, monkeypatch, tmp_path diff --git a/tests/test_auth_middleware.py b/tests/test_auth_middleware.py index 22d8d336b..a147201f4 100644 --- a/tests/test_auth_middleware.py +++ b/tests/test_auth_middleware.py @@ -1,10 +1,19 @@ """Unit tests for auth_middleware allow/deny logic.""" from __future__ import annotations +import time from unittest.mock import AsyncMock, MagicMock, patch import pytest +from cryptography.hazmat.primitives.asymmetric.ed25519 import Ed25519PrivateKey +from cryptography.hazmat.primitives.serialization import ( + Encoding, + NoEncryption, + PrivateFormat, + PublicFormat, +) from fastapi.responses import JSONResponse +from starlette.datastructures import Headers from starlette.responses import RedirectResponse from tinyagentos.auth_middleware import ( @@ -30,7 +39,12 @@ def _request( req = MagicMock() req.method = method req.url.path = path - req.headers = headers or {} + # A real Request's headers are case-INSENSITIVE. A plain dict here silently + # hid `check_agent_identity` from every arm that did not patch it: the + # middleware reads "authorization" while that function reads + # "Authorization", so on a dict the real credential check saw no header at + # all and returned None instead of raising. + req.headers = Headers(headers or {}) req.cookies = cookies or {} if client_host is None: req.client = None @@ -723,6 +737,35 @@ async def test_agent_path_without_bearer_requires_session(self): call_next.assert_not_awaited() +def _registry_keypair() -> tuple[bytes, bytes]: + """Return (private_pem, public_pem) for a fresh Ed25519 keypair.""" + private = Ed25519PrivateKey.generate() + return ( + private.private_bytes( + encoding=Encoding.PEM, + format=PrivateFormat.PKCS8, + encryption_algorithm=NoEncryption(), + ), + private.public_key().public_bytes( + encoding=Encoding.PEM, + format=PublicFormat.SubjectPublicKeyInfo, + ), + ) + + +def _signed_registry_token(private_pem: bytes) -> str: + """Mint a real registry JWT signed by *private_pem*.""" + from tinyagentos.agent_registry_store import mint_registry_token + + return mint_registry_token( + canonical_id="test-agent", + private_key_pem=private_pem, + user_id="", + framework="test", + project_id=None, + ) + + class TestRegistryJwtRouteResolution: """Acceptance tests for the registry-JWT 404-vs-401 fix. @@ -730,6 +773,8 @@ class TestRegistryJwtRouteResolution: (2) no token + unknown route -> 401 AND no token + real route -> 401 with IDENTICAL bodies (3) valid registry JWT + allowlisted real route -> 2xx (4) valid registry JWT + KNOWN non-allowlisted route -> 401 + (5) ROTATED registry JWT + unknown route -> 401 (a dead credential never + earns the wrong-URL 404), with a live-token control alongside it """ @pytest.mark.asyncio @@ -822,6 +867,81 @@ async def test_valid_registry_jwt_known_non_allowlisted_route_returns_401(self): assert resp.body == b'{"error":"Authentication required"}' call_next.assert_not_awaited() + @pytest.mark.asyncio + async def test_rotated_registry_jwt_unknown_route_returns_401_not_404(self): + """A ROTATED token is dead, so it must not earn the 404 that says + "your credential is fine, your URL is not". + + `check_agent_identity` is deliberately NOT patched here: the whole + point is that the real liveness chain runs, so a signature that still + verifies against a still-"active" record does not by itself buy the + 404. + """ + private_pem, public_pem = _registry_keypair() + token = _signed_registry_token(private_pem) + + middleware = AuthMiddleware(app=MagicMock()) + auth_mgr = _default_auth_mgr() + req = _request( + method="GET", + path="/api/definitely-not-a-route", + headers={"authorization": f"Bearer {token}"}, + auth_mgr=auth_mgr, + routes=[_fake_route("/api/system", {"GET"})], + ) + state = MagicMock() + state.auth = auth_mgr + state.agent_registry_keypair = (private_pem, public_pem) + state.agent_registry = MagicMock() + # Active, but every token minted before the cutoff is superseded. + state.agent_registry.get = AsyncMock( + return_value={ + "status": "active", + "token_min_iat": int(time.time()) + 3600, + } + ) + req.app.state = state + call_next = AsyncMock() + + resp = await middleware.dispatch(req, call_next) + + assert resp.status_code == 401 + assert resp.body == b'{"error":"Authentication required"}' + call_next.assert_not_awaited() + + @pytest.mark.asyncio + async def test_live_registry_jwt_unknown_route_still_returns_404(self): + """Control for the arm above: with the SAME real liveness chain and a + LIVE record, the wrong-URL 404 still fires. Narrowing the 404 to live + credentials must not turn into removing it.""" + private_pem, public_pem = _registry_keypair() + token = _signed_registry_token(private_pem) + + middleware = AuthMiddleware(app=MagicMock()) + auth_mgr = _default_auth_mgr() + req = _request( + method="GET", + path="/api/definitely-not-a-route", + headers={"authorization": f"Bearer {token}"}, + auth_mgr=auth_mgr, + routes=[_fake_route("/api/system", {"GET"})], + ) + state = MagicMock() + state.auth = auth_mgr + state.agent_registry_keypair = (private_pem, public_pem) + state.agent_registry = MagicMock() + state.agent_registry.get = AsyncMock( + return_value={"status": "active", "token_min_iat": 0} + ) + req.app.state = state + call_next = AsyncMock() + + resp = await middleware.dispatch(req, call_next) + + assert resp.status_code == 404 + assert resp.body == b'{"error":"Not Found"}' + call_next.assert_not_awaited() + @pytest.mark.asyncio async def test_stale_non_device_bearer_does_not_shadow_valid_session(self): """Regression for tsk-3hei4g CodeRabbit finding #3: a logged-in user diff --git a/tests/test_token_rotation.py b/tests/test_token_rotation.py index 79318b3fb..eeb5d8c6c 100644 --- a/tests/test_token_rotation.py +++ b/tests/test_token_rotation.py @@ -11,7 +11,7 @@ from httpx import ASGITransport, AsyncClient from tinyagentos.agent_registry_store import mint_registry_token -from tinyagentos.agent_token_auth import check_agent_scope +from tinyagentos.agent_token_auth import check_agent_identity, check_agent_scope from taos_test_csrf import csrf_event_hooks @@ -51,6 +51,32 @@ def __init__(self, app, token: str | None = None): self.headers["Authorization"] = f"Bearer {token}" +async def _rotation_fixture(app, *, scopes=("a2a_receive",)): + """Register one ACTIVE agent with *scopes* and return (canonical_id, priv). + + The caller mints its own tokens so it controls whether they are minted + before or after a ``bump_token_min_iat``. + """ + for attr in ("agent_registry", "agent_grants"): + store = getattr(app.state, attr, None) + if store is not None and store._db is None: + await store.init() + + registry = app.state.agent_registry + grants = app.state.agent_grants + priv, _pub = app.state.agent_registry_keypair + + rec = await registry.register( + framework="test", display_name="TestAgent", + origin="external-selfjoin", handle="@test", + ) + cid = rec["canonical_id"] + await registry.set_status(cid, "active") + for scope in scopes: + await grants.add_grant(cid, scope) + return cid, priv + + async def _register_and_mint(app, *, user_id="u", owner_user_id=None, scopes=("a2a_receive",)): """Register an active agent, add grants, and mint a signed JWT. @@ -205,6 +231,49 @@ async def test_default_zero_keeps_existing_tokens_valid(self, app): assert result == cid +class TestTokenMinIatIdentity: + """`check_agent_identity` must honour the SAME rotation cutoff as + `check_agent_scope` and `check_agent_scope_for_project`. + + It proves only WHO the caller is, so it is the auth for every surface an + agent may reach without holding a scope yet: creating a scope request, + the agent decisions routes, the container-provisioning requests, and the + auth-request flow. If it skips the cutoff then rotation -- the one + mechanism for killing a leaked token without deleting the identity -- + does not actually revoke anything on those routes. + """ + + @pytest.mark.asyncio + async def test_rotated_token_rejected(self, app): + """A token minted before the bump must not prove identity afterwards.""" + cid, priv = await _rotation_fixture(app) + old_token = mint_registry_token(cid, priv, user_id="u", framework="test") + await app.state.agent_registry.bump_token_min_iat(cid, int(time.time()) + 3600) + + with pytest.raises(HTTPException) as exc: + await check_agent_identity(_FakeRequest(app, old_token)) + assert exc.value.status_code == 401 + assert exc.value.detail == "token superseded" + + @pytest.mark.asyncio + async def test_token_minted_after_bump_still_proves_identity(self, app): + """Control: rotation must not lock out the replacement token.""" + cid, priv = await _rotation_fixture(app) + await app.state.agent_registry.bump_token_min_iat(cid, int(time.time())) + new_token = mint_registry_token(cid, priv, user_id="u", framework="test") + + assert await check_agent_identity(_FakeRequest(app, new_token)) == cid + + @pytest.mark.asyncio + async def test_default_zero_cutoff_keeps_identity_valid(self, app): + """Control: the migration default (0) must not lock out live tokens.""" + cid, priv = await _rotation_fixture(app) + assert (await app.state.agent_registry.get(cid))["token_min_iat"] == 0 + token = mint_registry_token(cid, priv, user_id="u", framework="test") + + assert await check_agent_identity(_FakeRequest(app, token)) == cid + + # --------------------------------------------------------------------------- # Route-level tests # --------------------------------------------------------------------------- @@ -294,4 +363,43 @@ async def test_rotate_nonexistent_returns_404(self, agent_app): resp = await agent_app.post( "/api/agents/registry/no-such-agent-20260101-000000/rotate-tokens" ) - assert resp.status_code == 404 \ No newline at end of file + assert resp.status_code == 404 + +class TestEnforceRotationCutoffHelper: + """Unit tests for the shared rotation-cutoff check. + + The same four-line token_min_iat comparison used to be copy-pasted into + _verify_agent_scope, check_agent_identity, and check_agent_project_grants + (kilo-code-bot review on #2799): any future change to the cutoff + semantics had to be applied in three places or one would silently + disagree. It is now a single helper all three call. + """ + + def test_iat_at_or_after_cutoff_passes(self): + from tinyagentos.agent_token_auth import _enforce_rotation_cutoff + + _enforce_rotation_cutoff({"token_min_iat": 100}, {"iat": 100}) + _enforce_rotation_cutoff({"token_min_iat": 100}, {"iat": 200}) + + def test_iat_before_cutoff_raises_401(self): + from tinyagentos.agent_token_auth import _enforce_rotation_cutoff + + with pytest.raises(HTTPException) as exc_info: + _enforce_rotation_cutoff({"token_min_iat": 100}, {"iat": 99}) + assert exc_info.value.status_code == 401 + + def test_missing_token_min_iat_defaults_to_zero(self): + """No rotation has ever happened -- any real iat clears the cutoff.""" + from tinyagentos.agent_token_auth import _enforce_rotation_cutoff + + _enforce_rotation_cutoff({}, {"iat": 1}) + + def test_missing_iat_is_treated_as_ancient_and_rejected_once_rotated(self): + """A token with no iat claim is collapsed to 0 (documented + safe-by-default policy), so it is superseded by ANY cutoff a caller + has ever set.""" + from tinyagentos.agent_token_auth import _enforce_rotation_cutoff + + with pytest.raises(HTTPException) as exc_info: + _enforce_rotation_cutoff({"token_min_iat": 1}, {}) + assert exc_info.value.status_code == 401 diff --git a/tinyagentos/agent_token_auth.py b/tinyagentos/agent_token_auth.py index 1b91ce7c0..56488a069 100644 --- a/tinyagentos/agent_token_auth.py +++ b/tinyagentos/agent_token_auth.py @@ -66,6 +66,30 @@ def _grant_unexpired(expires_at, now: datetime) -> bool: return exp > now +def _enforce_rotation_cutoff(record: dict, payload: dict) -> None: + """Reject a token issued before the identity's ``token_min_iat`` cutoff. + + Shared by ``_verify_agent_scope``, ``check_agent_identity``, and + ``check_agent_project_grants`` (previously copy-pasted into all three, + which let the cutoff semantics silently drift between call sites). + + Both ``token_min_iat`` (absent until an identity has ever been rotated) + and ``iat`` default to 0 when missing -- NOT rejected outright. This is a + deliberate safe-by-default choice: an identity that was never rotated has + ``token_min_iat`` unset, and a token missing ``iat`` is treated as + infinitely old rather than exempt, so it is accepted only until the first + rotation and superseded by any cutoff set thereafter. ``mint_registry_token`` + always sets ``iat``; a missing claim only occurs on a hand-crafted token. + + Raises: + 401 -- ``iat`` is strictly before ``token_min_iat`` (superseded by rotation). + """ + token_min_iat = record.get("token_min_iat") or 0 + token_iat = payload.get("iat") or 0 + if token_iat < token_min_iat: + raise HTTPException(status_code=401, detail="token superseded") + + def _get_keypair(request: Request) -> tuple[bytes, bytes]: kp = getattr(request.app.state, "agent_registry_keypair", None) if kp is None: @@ -113,10 +137,7 @@ async def _verify_agent_scope( raise HTTPException(status_code=403, detail="agent is not active in the registry") # Reject tokens issued before the identity's token_min_iat cutoff (rotation). - token_min_iat = record.get("token_min_iat") or 0 - token_iat = payload.get("iat") or 0 - if token_iat < token_min_iat: - raise HTTPException(status_code=401, detail="token superseded") + _enforce_rotation_cutoff(record, payload) # Must hold an active grant for the required scope. grants_store = _get_grants_store(request) @@ -175,7 +196,8 @@ async def check_agent_identity(request: Request) -> Optional[str]: Raises: 401 -- Authorization header present but the token is malformed, has a bad - signature, or is missing the sub claim. + signature, is missing the sub claim, or was superseded by a token + rotation on the identity. 403 -- Token is valid but the agent is not active in the registry. """ auth_header = request.headers.get("Authorization", "") @@ -199,6 +221,15 @@ async def check_agent_identity(request: Request) -> Optional[str]: if record is None or record.get("status") != "active": raise HTTPException(status_code=403, detail="agent is not active in the registry") + # Reject tokens issued before the identity's token_min_iat cutoff (rotation), + # exactly as check_agent_scope and check_agent_scope_for_project do. Identity + # is the ONLY auth on the surfaces that do not need a grant -- creating a + # scope request, the agent decisions routes, container-provisioning requests, + # the auth-request flow -- so skipping it here would leave rotate-tokens + # unable to kill a leaked token on precisely the route that can widen its own + # privileges. + _enforce_rotation_cutoff(record, payload) + return canonical_id @@ -288,10 +319,7 @@ async def check_agent_project_grants( if record is None or record.get("status") != "active": raise HTTPException(status_code=403, detail="agent is not active in the registry") - token_min_iat = record.get("token_min_iat") or 0 - token_iat = payload.get("iat") or 0 - if token_iat < token_min_iat: - raise HTTPException(status_code=401, detail="token superseded") + _enforce_rotation_cutoff(record, payload) grants_store = _get_grants_store(request) grants = await grants_store.list_grants(canonical_id)