tsk-5bup3r [OPEN] Registry tokens never expire: taOS mints JWTs with - #2235
tsk-5bup3r [OPEN] Registry tokens never expire: taOS mints JWTs with#2235jaylfc wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughChangesRegistry token lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Client
participant renew_registry_token_route
participant renew_registry_token
participant AgentRegistry
Client->>renew_registry_token_route: POST bearer token
renew_registry_token_route->>renew_registry_token: Renew signed token
renew_registry_token->>renew_registry_token_route: Return replacement token
renew_registry_token_route->>AgentRegistry: Check agent is active
AgentRegistry->>renew_registry_token_route: Return registry status
renew_registry_token_route->>Client: Return replacement token or error
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
|
nemotron-ultra-kilo review VERDICT: The changes implement token expiration (exp claim), renewal endpoint, and migration window — mostly solid but with several correctness and security concerns.
Automated first-pass review by the nemotron-ultra-kilo lane. The lead still reviews before merge. |
PR Summary by QodoAdd exp claim to registry JWTs and a self-service token renewal endpoint
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
Code Review by Qodo
1. Renew bypasses exp migration
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| @router.post("/api/agents/registry/token/renew") | ||
| async def renew_registry_token_route(request: Request): |
There was a problem hiding this comment.
1. /token/renew not allowlisted 📜 Skill insight ⛨ Security
A new agent-facing route POST /api/agents/registry/token/renew was added but is not present in the agent-token allowlist, so registry JWT calls can be blocked by auth middleware and return 401. This violates the requirement to add new agent/JWT-accessible routes to tinyagentos/auth_middleware.py.
Agent Prompt
## Issue description
`POST /api/agents/registry/token/renew` is intended to be called with a registry Bearer JWT, but the auth middleware only passes registry JWTs through for explicitly allowlisted paths.
## Issue Context
The PR introduces a new route at `/api/agents/registry/token/renew` that requires a Bearer token, but `_AGENT_TOKEN_PATHS` in `tinyagentos/auth_middleware.py` does not include this path.
## Fix Focus Areas
- tinyagentos/auth_middleware.py[14-51]
- tinyagentos/routes/agent_registry.py[747-774]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| from pydantic import BaseModel, field_validator | ||
|
|
||
| from tinyagentos.agent_registry_store import mint_registry_token | ||
| from tinyagentos.agent_registry_store import mint_registry_token, renew_registry_token, verify_registry_token |
There was a problem hiding this comment.
2. Route imports agent_registry_store 📜 Skill insight ⌂ Architecture
tinyagentos/routes/agent_registry.py directly imports from tinyagentos.agent_registry_store, violating the rule that routes must access stores via request.app.state rather than direct imports of store modules. This increases coupling and bypasses the intended dependency-injection pattern.
Agent Prompt
## Issue description
Route modules under `tinyagentos/routes/` must not directly import store modules; they should obtain store instances via `request.app.state`.
## Issue Context
This PR expands the direct import from `tinyagentos.agent_registry_store` by adding `renew_registry_token` and `verify_registry_token` to the import list.
## Fix Focus Areas
- tinyagentos/routes/agent_registry.py[32-33]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| _private_pem, public_pem = _get_keypair(request) | ||
| try: | ||
| payload = verify_registry_token(raw_token, public_pem) | ||
| payload = verify_registry_token(raw_token, public_pem, allow_no_exp_until=_get_migration_cutoff(request)) | ||
| except ValueError: |
There was a problem hiding this comment.
4. Legacy tokens break immediately 🐞 Bug ☼ Reliability
check_agent_scope()/check_agent_identity() now enforce exp via verify_registry_token(), but the default registry_token_migration_cutoff_ts=None means any legacy registry token without exp is rejected (401) unless an operator explicitly configures a future cutoff. This is a backward-compatibility/availability regression on upgrade for deployments with pre-exp tokens.
Agent Prompt
### Issue description
Legacy registry tokens that lack an `exp` claim will be rejected immediately after upgrade because `registry_token_migration_cutoff_ts` defaults to `None`, causing `verify_registry_token()` to raise `ValueError("token has no exp claim")`.
### Issue Context
The PR adds a migration-window mechanism (`allow_no_exp_until`) but leaves the default configuration with no window. The existing module documentation indicates tokens previously carried no `exp` claim, so this is likely to break existing agents.
### Fix Focus Areas
- tinyagentos/config.py[52-214]
- tinyagentos/agent_token_auth.py[39-109]
- tinyagentos/agent_registry_store.py[365-386]
### Suggested fix
- In `load_config()`, if `registry_token_migration_cutoff_ts` is absent, set it once to a reasonable future timestamp (e.g., `time.time() + 30*86400`) and call `save_config(cfg, path)` similar to the existing litellm_port pin migration.
- Add/adjust tests to ensure legacy no-`exp` tokens authenticate during the grace window by default on existing installs (or clearly document/enforce the requirement to set the cutoff explicitly).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| payload = _verify_signature_only(token, public_key_pem) | ||
| return mint_registry_token( | ||
| payload["sub"], | ||
| private_key_pem, |
There was a problem hiding this comment.
5. Renew bypasses exp migration 🐞 Bug ⛨ Security
renew_registry_token() re-mints tokens after verifying only the signature, so a legacy token without exp can be exchanged for a fresh exp token even after the migration cutoff would have rejected it. This defeats the cutoff policy intended to retire non-expiring tokens.
Agent Prompt
### Issue description
The renewal path accepts legacy tokens with no `exp` forever because it uses `_verify_signature_only()` and never applies the migration cutoff policy.
### Issue Context
The renewal path is intentionally allowed to accept *expired* tokens, but it should not allow *no-exp* legacy tokens past the configured migration cutoff.
### Fix Focus Areas
- tinyagentos/agent_registry_store.py[283-412]
- tinyagentos/routes/agent_registry.py[747-773]
- tinyagentos/config.py[52-214]
### Suggested fix
- Extend `renew_registry_token()` to accept `allow_no_exp_until: float | None` (or `migration_cutoff_ts`) and:
- verify signature,
- require that the token either has an `exp` claim OR `now < allow_no_exp_until`.
- do **not** require the token to be unexpired.
- In `renew_registry_token_route`, pass `request.app.state.config.registry_token_migration_cutoff_ts` into `renew_registry_token()`.
- Add a test covering: a legacy no-`exp` token cannot be renewed after the cutoff.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| registry_token_lifetime_seconds: int = 86400 | ||
| registry_token_migration_cutoff_ts: float | None = None |
There was a problem hiding this comment.
6. Token lifetime config unused 🐞 Bug ≡ Correctness
registry_token_lifetime_seconds is added and parsed into AppConfig, but minting/renewal call sites never pass it, so configuring token lifetime has no effect. Tokens always use DEFAULT_REGISTRY_TOKEN_LIFETIME unless callers explicitly override lifetime_seconds.
Agent Prompt
### Issue description
The new `registry_token_lifetime_seconds` config knob is dead: minted and renewed tokens ignore it.
### Issue Context
`mint_registry_token()` supports `lifetime_seconds`, but production call sites do not pass a value.
### Fix Focus Areas
- tinyagentos/routes/agent_registry.py[242-285]
- tinyagentos/routes/agent_registry.py[287-349]
- tinyagentos/routes/agent_registry.py[747-773]
- tinyagentos/routes/agent_auth_requests.py[449-455]
- tinyagentos/routes/agent_auth_requests.py[537-543]
- tinyagentos/agent_registry_store.py[310-363]
### Suggested fix
- Read `lifetime = request.app.state.config.registry_token_lifetime_seconds` (with sane validation, e.g. `>= 60`) and pass `lifetime_seconds=lifetime` to:
- `/api/agents/registry/register` mint
- `_mint_internal_identity` mint
- consent/approval mint paths in `agent_auth_requests.py`
- `/api/agents/registry/token/renew` renewal (pass through to `renew_registry_token(..., lifetime_seconds=lifetime)`)
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| registry_token_lifetime_seconds=int(data.get("registry_token_lifetime_seconds", 86400)), | ||
| registry_token_migration_cutoff_ts=float(data["registry_token_migration_cutoff_ts"]) if "registry_token_migration_cutoff_ts" in data else None, |
There was a problem hiding this comment.
7. Config fields not saved 🐞 Bug ⚙ Maintainability
load_config() reads registry_token_lifetime_seconds and registry_token_migration_cutoff_ts, but AppConfig.to_dict() omits them, so any save_config() will silently drop these settings from config.yaml. This can unexpectedly revert a configured migration cutoff (and lifetime) after unrelated config-saving migrations.
Agent Prompt
### Issue description
New registry token config settings are not round-trippable: they are loaded but not serialized.
### Issue Context
`save_config()` writes `yaml.dump(config.to_dict())`, so any omitted fields are lost when config is persisted.
### Fix Focus Areas
- tinyagentos/config.py[69-88]
- tinyagentos/config.py[186-214]
- tinyagentos/config.py[345-349]
### Suggested fix
- Update `AppConfig.to_dict()` to include:
- `registry_token_lifetime_seconds` (at least when != default)
- `registry_token_migration_cutoff_ts` (when not None)
- Add a unit test that loads a config containing these keys, saves it, and asserts the keys remain present with the same values.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tinyagentos/agent_registry_store.py`:
- Around line 389-411: Update renew_registry_token to retrieve the subject claim
with payload.get("sub", "") instead of direct payload["sub"] access, matching
the defensive pattern used by other verification call sites and ensuring
malformed signed payloads raise ValueError through mint_registry_token rather
than KeyError.
- Around line 389-411: Bound renew_registry_token so a signed token cannot be
renewed indefinitely after expiration: enforce a maximum grace period relative
to its exp (using the existing registry lifetime configuration where
appropriate) and reject tokens beyond that window before minting the
replacement. Keep valid unexpired tokens and recently expired tokens eligible,
while preserving the existing claim reuse and minting behavior.
In `@tinyagentos/config.py`:
- Around line 199-200: Update the config loading logic for
registry_token_lifetime_seconds and registry_token_migration_cutoff_ts to catch
invalid numeric values, log a warning, and use their respective defaults instead
of allowing ValueError or TypeError to escape. Follow the existing defensive
parsing pattern in load_config and preserve None when the migration cutoff key
is absent.
- Around line 65-66: Update AppConfig.to_dict() to explicitly serialize
registry_token_lifetime_seconds and registry_token_migration_cutoff_ts, using
the same conditional/default-comparison pattern as memory_url so custom values
persist through save_config() and save_config_locked() while defaults remain
omitted.
In `@tinyagentos/routes/agent_registry.py`:
- Around line 747-775: Update the registry token route call sites to pass
request.app.state.config.registry_token_lifetime_seconds as lifetime_seconds:
use it when minting in the /api/agents/registry/register handler and when
renewing in renew_registry_token_route. Preserve the existing token validation
and response behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 733001c2-5533-4055-81ed-3d7890127dc0
📒 Files selected for processing (6)
tests/test_agent_registry.pytests/test_agent_registry_store.pytinyagentos/agent_registry_store.pytinyagentos/agent_token_auth.pytinyagentos/config.pytinyagentos/routes/agent_registry.py
| def renew_registry_token( | ||
| token: str, | ||
| public_key_pem: bytes, | ||
| private_key_pem: bytes, | ||
| *, | ||
| lifetime_seconds: int | None = None, | ||
| ) -> str: | ||
| """Renew an existing registry token. | ||
|
|
||
| Verifies the EdDSA signature (but does not require the token to be | ||
| unexpired) and returns a freshly-minted token with the same claims and | ||
| a new exp. This is the self-service path for agents whose token has | ||
| expired without human re-minting. | ||
| """ | ||
| payload = _verify_signature_only(token, public_key_pem) | ||
| return mint_registry_token( | ||
| payload["sub"], | ||
| private_key_pem, | ||
| user_id=payload.get("user_id", ""), | ||
| framework=payload.get("framework", ""), | ||
| project_id=payload.get("project_id"), | ||
| lifetime_seconds=lifetime_seconds, | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard against a payload missing the sub claim.
renew_registry_token accesses payload["sub"] directly at Line 405. _verify_signature_only only checks the signature, not the claim shape, so a validly-signed but malformed payload (missing sub) raises KeyError here instead of ValueError. The renewal route in tinyagentos/routes/agent_registry.py only catches ValueError, so this would surface as a 500 instead of a controlled 401. Other verification call sites (agent_token_auth.py) defensively use payload.get("sub", "") before use; apply the same pattern here.
🛡️ Proposed fix
payload = _verify_signature_only(token, public_key_pem)
+ canonical_id = payload.get("sub")
+ if not canonical_id:
+ raise ValueError("token missing sub claim")
return mint_registry_token(
- payload["sub"],
+ canonical_id,
private_key_pem,
user_id=payload.get("user_id", ""),
framework=payload.get("framework", ""),
project_id=payload.get("project_id"),
lifetime_seconds=lifetime_seconds,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def renew_registry_token( | |
| token: str, | |
| public_key_pem: bytes, | |
| private_key_pem: bytes, | |
| *, | |
| lifetime_seconds: int | None = None, | |
| ) -> str: | |
| """Renew an existing registry token. | |
| Verifies the EdDSA signature (but does not require the token to be | |
| unexpired) and returns a freshly-minted token with the same claims and | |
| a new exp. This is the self-service path for agents whose token has | |
| expired without human re-minting. | |
| """ | |
| payload = _verify_signature_only(token, public_key_pem) | |
| return mint_registry_token( | |
| payload["sub"], | |
| private_key_pem, | |
| user_id=payload.get("user_id", ""), | |
| framework=payload.get("framework", ""), | |
| project_id=payload.get("project_id"), | |
| lifetime_seconds=lifetime_seconds, | |
| ) | |
| def renew_registry_token( | |
| token: str, | |
| public_key_pem: bytes, | |
| private_key_pem: bytes, | |
| *, | |
| lifetime_seconds: int | None = None, | |
| ) -> str: | |
| """Renew an existing registry token. | |
| Verifies the EdDSA signature (but does not require the token to be | |
| unexpired) and returns a freshly-minted token with the same claims and | |
| a new exp. This is the self-service path for agents whose token has | |
| expired without human re-minting. | |
| """ | |
| payload = _verify_signature_only(token, public_key_pem) | |
| canonical_id = payload.get("sub") | |
| if not canonical_id: | |
| raise ValueError("token missing sub claim") | |
| return mint_registry_token( | |
| canonical_id, | |
| private_key_pem, | |
| user_id=payload.get("user_id", ""), | |
| framework=payload.get("framework", ""), | |
| project_id=payload.get("project_id"), | |
| lifetime_seconds=lifetime_seconds, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tinyagentos/agent_registry_store.py` around lines 389 - 411, Update
renew_registry_token to retrieve the subject claim with payload.get("sub", "")
instead of direct payload["sub"] access, matching the defensive pattern used by
other verification call sites and ensuring malformed signed payloads raise
ValueError through mint_registry_token rather than KeyError.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate file and relevant symbols"
if [ -f tinyagentos/agent_registry_store.py ]; then
wc -l tinyagentos/agent_registry_store.py
ast-grep outline tinyagentos/agent_registry_store.py --match renew_registry_token --view expanded || true
echo "--- relevant lines 240-430 ---"
sed -n '240,430p' tinyagentos/agent_registry_store.py | nl -ba -v240
else
fd -i 'agent_registry_store.py' .
fi
echo "--- search for token handling symbols ---"
rg -n "TOKEN_(LIFETIME|RENEW_|EXPIRED)?|lifetime_seconds|renew_registry_token|mint_registry_token|active|verify_signature|jti" tinyagentosRepository: jaylfc/taOS
Length of output: 361
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- relevant lines 240-430 ---"
sed -n '240,430p' tinyagentos/agent_registry_store.py | sed 's/^/: /'
echo "--- search token-related symbols ---"
rg -n "TOKEN_(LIFETIME|RENEW_|EXPIRED)?|lifetime_seconds|renew_registry_token|mint_registry_token|active|verify_signature|jti" tinyagentos || true
echo "--- inspect registry state references around relevant functions ---"
rg -n "_verify_signature_only|mint_registry_token|renew_registry_token|def .*registry_token|lookup|active|update|delete|agent" tinyagentos/agent_registry_store.py | sed 's/^/: /'Repository: jaylfc/taOS
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- renew_registry_token_route ---"
sed -n '748,774p' tinyagentos/routes/agent_registry.py | sed 's/^/: /'
echo "--- agent_token_auth relevant lines ---"
sed -n '1,140p' tinyagentos/agent_token_auth.py | sed 's/^/: /'
echo "--- deterministic behavior probe ---"
python3 - <<'PY'
import json
from base64 import urlsafe_b64encode, urlsafe_b64decode
from cryptography.hazmat.primitives.asymmetric.ed25519 import Ed25519PrivateKey
from cryptography.hazmat.primitives.serialization import Encoding, PrivateFormat, PublicFormat, NoEncryption
def b64url(data): return urlsafe_b64encode(data).rstrip(b"=").decode("ascii")
def b64url_decode(s): return urlsafe_b64decode(s + b"=" * (4 - len(s) % 4))
private = Ed25519PrivateKey.generate()
public = private.public_key()
priv_pem = private.private_bytes(Encoding.PEM, PrivateFormat.PKCS8, NoEncryption())
pub_pem = public.public_bytes(Encoding.PEM, PublicFormat.SubjectPublicKeyInfo)
def sign(payload):
header = b64url(json.dumps({"alg":"EdDSA","typ":"JWT"}, separators=(",", ":")).encode())
payload_b64 = b64url(json.dumps(payload).encode())
sig = b64url(private.sign(f"{header}.{payload_b64}".encode()))
return f"{header}.{payload_b64}.{sig}"
for days in [25, 36500]:
token = sign({"sub":"agent-a1b2","iss":"taos-registry","iat":0,"exp":days*86400,"jti":"old","user_id":"u"})
# Minimal implementation matching tinyagentos: signature checked, exp ignored, new exp iat+lifetime
payload = json.loads(b64url_decode(token.split(".")[1]))
new_token = sign({"sub":payload["sub"],"iss":"taos-registry","iat":1,\
"exp":1+86400,"jti":"new","user_id":payload.get("user_id",""),**{k:payload.get(k) for k in ["framework","project_id"]}})
print({
"days": days,
"old_exp": payload["exp"],
"old_expired_at_renewal_time": False,
"new_exp": json.loads(new_token.split(".")[1])["exp"],
"renewal_same_id": payload["sub"] == json.loads(new_token.split(".")[1])["sub"]
})
token = sign({"sub":"agent-a1b2","iss":"taos-registry","iat":100,"exp":200,"jti":"old"})
payload = json.loads(b64url_decode(token.split(".")[1]))
try:
public.verify(b64url_decode(token.split(".")[2]), f"{token.split('.')[0]}.{token.split('.')[1]}".encode())
print("signature_verifies_any_expired_payload", payload["sub"])
except Exception as exc:
print("signature_verification_failed", type(exc).__name__, str(exc))
PYRepository: jaylfc/taOS
Length of output: 7356
Bound self-service token renewal.
renew_registry_token only verifies the EdDSA signature and accepts any matching payload, even an expired one. Because renew_registry_token_route returns the fresh token and normal verification later requires the agent to be active, a captured active bearer token can rotate indefinitely while that agent record stays active. This conflicts with the DEFAULT_REGISTRY_TOKEN_LIFETIME intent that leaked bearer credentials should not be permanent. Add an enforcement rule, such as rejecting renewal past a maximum grace period after exp or invalidating the superseded jti/issuer key scope.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tinyagentos/agent_registry_store.py` around lines 389 - 411, Bound
renew_registry_token so a signed token cannot be renewed indefinitely after
expiration: enforce a maximum grace period relative to its exp (using the
existing registry lifetime configuration where appropriate) and reject tokens
beyond that window before minting the replacement. Keep valid unexpired tokens
and recently expired tokens eligible, while preserving the existing claim reuse
and minting behavior.
| registry_token_lifetime_seconds: int = 86400 | ||
| registry_token_migration_cutoff_ts: float | None = None |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -B2 -A15 'def save_config' tinyagentos/config.py
rg -n 'to_dict\(' tinyagentos/config.pyRepository: jaylfc/taOS
Length of output: 1247
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== config.py outline relevant =="
ast-grep outline tinyagentos/config.py --match AppConfig --view expanded || true
echo "== AppConfig class region =="
sed -n '1,130p' tinyagentos/config.py
echo "== save/load/save_locked regions =="
sed -n '320,355p' tinyagentos/config.pyRepository: jaylfc/taOS
Length of output: 8506
Persist the new registry-token fields when saving config.
save_config() writes config.to_dict() directly, and AppConfig.to_dict() only emits memory_url for scalar optional settings, so custom values for registry_token_lifetime_seconds and registry_token_migration_cutoff_ts can be dropped on any save_config()/save_config_locked() call and will reload as defaults.
Add explicit entries for these fields in to_dict() with the same conditional/default-comparison style used for memory_url.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tinyagentos/config.py` around lines 65 - 66, Update AppConfig.to_dict() to
explicitly serialize registry_token_lifetime_seconds and
registry_token_migration_cutoff_ts, using the same
conditional/default-comparison pattern as memory_url so custom values persist
through save_config() and save_config_locked() while defaults remain omitted.
| registry_token_lifetime_seconds=int(data.get("registry_token_lifetime_seconds", 86400)), | ||
| registry_token_migration_cutoff_ts=float(data["registry_token_migration_cutoff_ts"]) if "registry_token_migration_cutoff_ts" in data else None, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Unguarded numeric coercion can crash config loading.
int(data.get("registry_token_lifetime_seconds", 86400)) and float(data["registry_token_migration_cutoff_ts"]) raise an uncaught ValueError/TypeError if the YAML value is malformed (e.g., a non-numeric string), crashing load_config instead of falling back to the default. Wrap these in a try/except that logs a warning and falls back to the default, matching the defensive style already used for other config fields.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tinyagentos/config.py` around lines 199 - 200, Update the config loading
logic for registry_token_lifetime_seconds and registry_token_migration_cutoff_ts
to catch invalid numeric values, log a warning, and use their respective
defaults instead of allowing ValueError or TypeError to escape. Follow the
existing defensive parsing pattern in load_config and preserve None when the
migration cutoff key is absent.
| @router.post("/api/agents/registry/token/renew") | ||
| async def renew_registry_token_route(request: Request): | ||
| """Renew the caller's registry token. | ||
|
|
||
| Accepts the current Bearer token (even if expired) and returns a new | ||
| token with the same claims and a fresh exp. The agent must still be | ||
| active in the registry. | ||
| """ | ||
| auth_header = request.headers.get("Authorization", "") | ||
| if not auth_header.lower().startswith("bearer "): | ||
| raise HTTPException(status_code=401, detail="missing bearer token") | ||
|
|
||
| raw_token = auth_header[7:].strip() | ||
| private_pem, public_pem = _get_keypair(request) | ||
|
|
||
| try: | ||
| new_token = renew_registry_token(raw_token, public_pem, private_pem) | ||
| except ValueError as exc: | ||
| raise HTTPException(status_code=401, detail=str(exc)) from exc | ||
|
|
||
| store = _get_store(request) | ||
| new_payload = verify_registry_token(new_token, public_pem) | ||
| record = await store.get(new_payload["sub"]) | ||
| if record is None or record.get("status") != "active": | ||
| raise HTTPException(status_code=403, detail="agent is not active in the registry") | ||
|
|
||
| return {"token": new_token} | ||
|
|
||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm registry_token_lifetime_seconds is unused elsewhere in the codebase.
rg -n 'registry_token_lifetime_seconds' --type=py
rg -n -B2 -A8 'mint_registry_token\(' tinyagentos/routes/agent_registry.pyRepository: jaylfc/taOS
Length of output: 149
🏁 Script executed:
#!/bin/bash
set -u
echo "git files around candidate names:"
git ls-files | rg '(^|/)tinyagentos/(routes/agent_registry|agent_registry_store|config)\.py$' || true
echo
echo "search config key:"
rg -n 'registry_token_lifetime_seconds' . || true
echo
echo "search token renewal/minting functions and calls:"
rg -n 'def (renew|m)int_registry_token|renew_registry_token|mint_registry_token|DEFAULT_REGISTRY_TOKEN_LIFETIME|registry_token_lifetime' . || true
echo
echo "line counts for candidate files:"
git ls-files | rg '(^|/)tinyagentos/(routes/agent_registry|agent_registry_store|config)\.py$' | while read -r f; do wc -l "$f"; doneRepository: jaylfc/taOS
Length of output: 7062
🏁 Script executed:
#!/bin/bash
set -u
echo "agent_registry_store token functions:"
sed -n '250,420p' tinyagentos/agent_registry_store.py | cat -n
echo
echo "agent_registry registry route registration:"
sed -n '240,290p' tinyagentos/routes/agent_registry.py | cat -n
echo
echo "agent_registry registry route registration 2:"
sed -n '320,355p' tinyagentos/routes/agent_registry.py | cat -n
echo
echo "agent_registry renew route:"
sed -n '740,780p' tinyagentos/routes/agent_registry.py | cat -n
echo
echo "config field parsing:"
sed -n '55,72p' tinyagentos/config.py | cat -n
sed -n '188,205p' tinyagentos/config.py | cat -n
echo
echo "verify all route call sites pass no lifetime_seconds keyword:"
python3 - <<'PY'
import ast, pathlib
for path in [pathlib.Path('tinyagentos/routes/agent_registry.py')]:
tree = ast.parse(path.read_text())
for node in ast.walk(tree):
if isinstance(node, ast.Call):
if isinstance(node.func, ast.Name) and node.func.id in ('mint_registry_token','renew_registry_token'):
kw = [kw.arg for kw in node.keywords]
print(f'{path}:{node.lineno}: {node.func.id} keywords={kw}')
PYRepository: jaylfc/taOS
Length of output: 15249
Use configured registry_token_lifetime_seconds when minting registry tokens.
mint_registry_token() and renew_registry_token() use DEFAULT_REGISTRY_TOKEN_LIFETIME only when lifetime_seconds is None, but both route call sites pass no value. Pass request.app.state.config.registry_token_lifetime_seconds into mint_registry_token() for /api/agents/registry/register and into renew_registry_token() for /api/agents/registry/token/renew so config.yaml lifetime settings apply.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tinyagentos/routes/agent_registry.py` around lines 747 - 775, Update the
registry token route call sites to pass
request.app.state.config.registry_token_lifetime_seconds as lifetime_seconds:
use it when minting in the /api/agents/registry/register handler and when
renewing in renew_registry_token_route. Preserve the existing token validation
and response behavior.
|
I have converted this to DRAFT so it cannot be merged by accident. As written, deploying it takes the entire fleet offline at once, and the agents cannot recover themselves. Please do not undraft until blockers 1 to 5 are done. Credit where it is due first: the store-level crypto is correct and genuinely red-first. I mutated the expiry enforcement four different ways and each one went red, including deleting the exp block entirely (3 failures) and forcing the migration window permanently closed (1 failure). That is the part the card cared most about and you delivered it properly. BLOCKING 1, the renewal path does not work for agents. POST /api/agents/registry/token/renew is not in _AGENT_TOKEN_PATHS in auth_middleware.py, so the middleware rejects it before the handler runs. Proven with a cookie-less bearer-only client against the real app: 401 Authentication required. Your tests pass because the registry_client fixture carries an ADMIN SESSION COOKIE, so they authenticate as the human admin, not as the agent. Renewal therefore requires exactly the manual human re-mint the card said must not be required. Add the route to the allowlist and add a test with a cookie-less client. BLOCKING 2, the migration window defaults to ZERO. registry_token_migration_cutoff_ts defaults to None, which means allow_no_exp_until is None, which means every legacy token without exp is rejected on the first request after deploy. Every agent on the Pi, every internal driver identity, everything on the A2A bus goes to 401 simultaneously, and per blocker 1 they cannot renew out of it. Ship a non-None effective default (a computed now+30d at first start, or a documented mandatory upgrade step). Not None. BLOCKING 3, the operator escape hatch is not durable. AppConfig.to_dict() does not emit either new field, and save_config writes yaml.dump(to_dict()). There are 11 save_config call sites, so an operator who sets a migration cutoff loses it the next time anything touches settings, mid-migration and silently. Add both fields to to_dict() plus a load-save-load round-trip test. BLOCKING 4, renewal re-creates the eternal credential this PR exists to remove. renew_registry_token does a signature-only check with no time bound at all. Probed: a token that expired TEN YEARS ago renews into a fresh 24h token, and a legacy no-exp token renews regardless of the cutoff. So a leaked bearer is still permanent, the attacker just renews forever. Bound it: reject tokens expired beyond a configurable grace, and pass allow_no_exp_until through. BLOCKING 5, this conflicts with hognek's PR 2208 in both senses. Semantically, 2208 revokes by rejecting tokens whose iat is older than token_min_iat; your renewal mints iat=now from a signature-only check that never consults it, so a rotated-out token can be laundered into a valid post-rotation one, defeating rotation entirely. It does not bite today only because renewal is unreachable, so fixing blocker 1 without this opens it. They must land together. Textually you also conflict in routes/agent_registry.py and tests/test_agent_registry_store.py. SHOULD FIX: registry_token_lifetime_seconds is read by nothing, so the configurable lifetime is inert (probed: config set to 60, minted lifetime still 86400) - wire it into all five mint/renew sites or delete the field. Add clock-skew leeway (60s) to both comparisons; today exp+2s is rejected, which will cause spurious 401s across machines. Add HTTP-layer migration-window tests, because mutating the cutoff getter to infinity currently leaves 222 tests green. Guard the numeric coercions in load_config, since an ISO date string in a field named _ts crashes taOS at startup. Audit-log renewal the way minting already is. Refresh user_id/framework/project_id from the registry record rather than replaying the old token's claims. And three docstrings you edited still say the token carries no exp claim. Also please revert the unrelated comment typo in tests/test_agent_registry_store.py where metacharacters became metachrs. |
|
nemotron-ultra-orB review VERDICT: Code review complete - several correctness and test issues found.
Automated first-pass review by the nemotron-ultra-orB lane. The lead still reviews before merge. |
|
nemotron-super review VERDICT: No blocking issues found. Automated first-pass review by the nemotron-super lane. The lead still reviews before merge. |
|
nemotron-super review VERDICT: No blocking issues found. Automated first-pass review by the nemotron-super lane. The lead still reviews before merge. |
1 participant
Autonomous build of board card tsk-5bup3r.
Files:
tests/test_agent_registry.py | 38 ++++++++++++
tests/test_agent_registry_store.py | 114 ++++++++++++++++++++++++++++++++++-
tinyagentos/agent_registry_store.py | 92 ++++++++++++++++++++++------
tinyagentos/agent_token_auth.py | 11 +++-
tinyagentos/config.py | 4 ++
tinyagentos/routes/agent_registry.py | 31 +++++++++-
6 files changed, 269 insertions(+), 21 deletions(-)
Summary by CodeRabbit
New Features
Bug Fixes