Registry tokens cannot be invalidated: add per-identity iat cutoff (token rotation blocker) - #2263
Registry tokens cannot be invalidated: add per-identity iat cutoff (token rotation blocker)#2263jaylfc wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
CLOSED - two reasons, neither about the code quality. (1) COLLISION: per-identity iat-cutoff rotation is exactly what open PR #2208 implements (in flight since July, paired with the #2235 token-renewal plan); two independent implementations of the same mechanism means whichever merges first silently decides the design - #2208 keeps the slot. (2) SEQUENCING (standing constraint): rotation must land together with #2235's renewal work or rotation is defeated by renewal - so even #2208 stays unmerged until then. The card is closed as duplicate so it does not redispatch; if #2208 stalls permanently, the card gets re-opened with a STEP-0 supersede note, not rebuilt blind. |
PR Summary by QodoAdd per-identity iat cutoff to enable registry token rotation/invalidation
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Code Review by Qodo
1. token_min_iat in SCHEMA
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| status TEXT NOT NULL DEFAULT 'active', | ||
| token_min_iat INTEGER NOT NULL DEFAULT 0 |
There was a problem hiding this comment.
1. token_min_iat in schema 📜 Skill insight ≡ Correctness
token_min_iat is added to SCHEMA while also being introduced via a migration, which violates the rule that SCHEMA must not reference migration-added columns. This can break first-open/migration ordering guarantees and reintroduce upgrade-path fragility.
Agent Prompt
## Issue description
`token_min_iat` is present in `SCHEMA` even though it is also added via `_migration_v5_add_token_min_iat`. Per compliance, any migration-added column must not appear in `SCHEMA`; it should be retrofitted in `_post_init` via a guarded `ALTER TABLE`.
## Issue Context
This store already follows this pattern for other migration-added elements (e.g., `ACTIVE_HANDLE_INDEX` avoids `SCHEMA` because `status` is added by migration). `token_min_iat` should follow the same invariant.
## Fix Focus Areas
- tinyagentos/agent_registry_store.py[35-51]
- tinyagentos/agent_registry_store.py[209-226]
- tinyagentos/agent_registry_store.py[463-486]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| # Token iat cutoff: reject if iat < token_min_iat (token superseded) | ||
| payload_iat = payload.get("iat") | ||
| token_min_iat = record.get("token_min_iat") | ||
| if payload_iat is not None and token_min_iat is not None and payload_iat < token_min_iat: | ||
| raise HTTPException(status_code=401, detail="token superseded") |
There was a problem hiding this comment.
3. Iat cutoff type mismatch 🐞 Bug ≡ Correctness
token_min_iat is stored as an INTEGER cutoff but rotation writes ISO8601 strings into it, and _verify_agent_scope compares JWT iat (int) to token_min_iat without coercion. This can raise a TypeError (500) during auth for rotated identities, breaking token rotation enforcement and potentially taking down agent-authenticated endpoints for that identity.
Agent Prompt
## Issue description
`token_min_iat` is intended to be compared to the JWT `iat` claim, which is minted as an integer (epoch seconds). The PR writes ISO8601 strings into `token_min_iat` and compares them directly to the integer `iat`, which can trigger a runtime `TypeError` and break authentication.
## Issue Context
- Schema declares `token_min_iat INTEGER`.
- Tokens are minted with `iat = int(time.time())`.
- Rotation writes `datetime.now(...).isoformat()` into `token_min_iat`.
- Auth compares `payload_iat < token_min_iat` without normalizing types.
## Fix Focus Areas
- tinyagentos/agent_registry_store.py[35-50]
- tinyagentos/agent_registry_store.py[299-346]
- tinyagentos/agent_registry_store.py[1040-1065]
- tinyagentos/agent_token_auth.py[107-118]
- tinyagentos/routes/agent_registry.py[489-516]
## Implementation notes
- Change `bump_token_min_iat(..., ts: str)` to accept an `int` cutoff (epoch seconds).
- In `rotate_token`, set `now_iat = int(time.time())` (or equivalent) and store that.
- In `_verify_agent_scope`, coerce both `payload["iat"]` and `record["token_min_iat"]` to `int` (fail closed with 401 if malformed).
- Consider making the DB update monotonic (e.g., only update when new cutoff > existing cutoff) to prevent accidentally lowering the cutoff and re-validating old tokens.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if not await _check_feed_token(request): | ||
| # Fallback to the more basic check_agent_identity which doesn't | ||
| # require a scope grant. | ||
| if await check_agent_identity(request) != canonical_id: |
There was a problem hiding this comment.
4. Rotate-token token mismatch 🐞 Bug ⛨ Security
rotate_token treats any successful _check_feed_token() result as proof the caller holds the target identity’s token, but it never verifies the returned canonical_id matches the path canonical_id. An admin with a feeds_read token for agent A can rotate agent B, contradicting the route’s documented “must hold the token for this identity” constraint.
Agent Prompt
## Issue description
The rotate-token route intends to require the caller to present a valid token for *the same* `canonical_id` being rotated. Today it only checks truthiness of `_check_feed_token()` and skips verifying the token’s `sub` matches the path parameter, enabling cross-identity rotation as long as the caller has *any* valid feeds_read token.
## Issue Context
`_check_feed_token()` returns the canonical_id encoded in whatever Bearer token is provided. `rotate_token()` currently does not compare that returned value to the target `canonical_id`.
## Fix Focus Areas
- tinyagentos/routes/agent_registry.py[200-210]
- tinyagentos/routes/agent_registry.py[489-501]
## Implementation notes
- Capture the returned canonical_id from `_check_feed_token(request)`; if non-None, require it equals the path `canonical_id`.
- Alternatively (simpler), remove `_check_feed_token` from this route and always use `check_agent_identity(request)` and require equality to the path `canonical_id` (since rotate-token should not require any scope grant).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| now = datetime.now(timezone.utc).isoformat() | ||
| updated = await store.bump_token_min_iat(canonical_id, now) |
There was a problem hiding this comment.
5. Rotate-token nameerror 🐞 Bug ≡ Correctness
rotate_token calls datetime.now(timezone.utc) but the module does not import datetime/timezone at top-level, so eligible rotation requests will raise NameError and return 500. This makes the new rotation endpoint unusable on the success path.
Agent Prompt
## Issue description
`rotate_token()` references `datetime` and `timezone`, but `tinyagentos/routes/agent_registry.py` does not import them at module scope. This will crash with `NameError` when the route reaches the cutoff update.
## Issue Context
There is a `from datetime import datetime, timezone` inside a different function, but that does not make the names available in `rotate_token()`.
## Fix Focus Areas
- tinyagentos/routes/agent_registry.py[23-35]
- tinyagentos/routes/agent_registry.py[502-505]
- tinyagentos/routes/agent_registry.py[606-607]
## Implementation notes
- Add `from datetime import datetime, timezone` to the module imports, OR
- Preferably, if switching token_min_iat to epoch seconds, compute `now_iat = int(time.time())` and `import time` instead.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| ) | ||
|
|
||
| before = record.get("token_min_iat") | ||
| now = datetime.now(timezone.utc).isoformat() |
There was a problem hiding this comment.
CRITICAL: token_min_iat column is INTEGER, but rotate_token stores an ISO datetime string. This causes TypeError in _verify_agent_scope when comparing payload_iat (int) with the stored string, returning 500 on every authenticated request after rotation.
| now = datetime.now(timezone.utc).isoformat() | |
| now = int(datetime.now(timezone.utc).timestamp()) |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| # Token rotation helper | ||
| # ------------------------------------------------------------------ | ||
|
|
||
| async def bump_token_min_iat(self, canonical_id: str, ts: str) -> dict: |
There was a problem hiding this comment.
WARNING: bump_token_min_iat type-hints ts: str, but it should accept an integer Unix timestamp to match the INTEGER column and JWT iat semantics.
| async def bump_token_min_iat(self, canonical_id: str, ts: str) -> dict: | |
| async def bump_token_min_iat(self, canonical_id: str, ts: int) -> dict: |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| import tempfile | ||
| import os | ||
|
|
||
| with tempfile.TemporaryDirectory() as tmp: |
| # Since the fixture uses admin, we can't easily test non-admin | ||
| # This test ensures the route has the admin check in place | ||
|
|
||
| # Verify the route exists by checking the router |
| # Rotate the token (bump token_min_iat) | ||
| # Use admin client via direct store method | ||
| store = client._app.state.agent_registry | ||
| now_ts = datetime.now(timezone.utc).isoformat() |
| # an active token with iat >= current token_min_iat before the bump). | ||
| # Use the shared verifier to ensure any token (including the one that will | ||
| # be minted next) would authenticate. | ||
| if not await _check_feed_token(request): |
| if not await _check_feed_token(request): | ||
| # Fallback to the more basic check_agent_identity which doesn't | ||
| # require a scope grant. | ||
| if await check_agent_identity(request) != canonical_id: |
There was a problem hiding this comment.
CRITICAL: check_agent_identity is used here but is not imported at the top of the file. This causes NameError at runtime when the rotate-token route is hit without a registry_feeds_read grant. Add check_agent_identity to the import from tinyagentos.agent_token_auth.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 7 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
Files Reviewed (4 files)
Fix these issues in Kilo Cloud Reviewed by step-3.7-flash · Input: 105.7K · Output: 37.4K · Cached: 1.4M |
1 participant
CARD TITLE (intent, not commit subject): Registry tokens cannot be invalidated: add per-identity iat cutoff (token rotation blocker)
Autonomous build of board card tsk-nslyhc.
Files:
test_token_rotation.py | 203 +++++++++++++++++++++++++++++++++++
tinyagentos/agent_registry_store.py | 61 ++++++++++-
tinyagentos/agent_token_auth.py | 6 ++
tinyagentos/routes/agent_registry.py | 57 ++++++++++
4 files changed, 323 insertions(+), 4 deletions(-)