fix(delegation): bound child scope_grant ttl_seconds by absolute expiry - #353
fix(delegation): bound child scope_grant ttl_seconds by absolute expiry#353rajnisht7 wants to merge 3 commits into
Conversation
1ae802a to
5c13248
Compare
imran-siddique
left a comment
There was a problem hiding this comment.
The bug is real and well found. ttl_seconds is a duration from each hop's own delegated_at, so comparing the two durations in isolation lets a grant outlive the authority it was narrowed from: a one-hour root re-delegating 50 minutes in with another one-hour TTL passes 3600 > 3600 and the child survives its parent by 50 minutes. Comparing absolute expiries is the right closure, and because it holds at every adjacent pair it holds transitively back to the root, so checking pairwise is sufficient rather than needing the root's expiry threaded through.
I also checked the case I expected to be a second hole and it is not: parent_ttl is not None and (child_ttl is None or ...) already refuses an unbounded child under a bounded parent.
One thing to fix before this lands
_parse_delegated_at returns a naive datetime when the timestamp carries no offset, and delegated_at is a required field with no format validation, so a chain can legitimately carry one hop written with Z and one without. Comparing those two raises TypeError, which is not what verify_delegation_chain documents raising.
I ran your _check_scope_narrowing directly across timestamp shapes:
both aware (the bug this PR fixes) -> ValueError (fails closed, correct)
both naive -> ValueError (fails closed, correct)
malformed child ("not-a-date") -> ValueError (fails closed, acceptable)
child +05:30 expressing the same instant -> accepted (correct, offset arithmetic works)
parent aware, child naive -> TypeError: can't compare offset-naive and offset-aware datetimes
parent naive, child aware -> TypeError
The last two matter because of where they land. A relying party wraps chain verification in except ValueError to reject a chain, per the docstring. A TypeError escapes that and surfaces as an unhandled crash in the verifier, on input the schema permits. For a delegation check the failure mode should be refusal, not an exception the caller was never told to catch.
One line:
def _parse_delegated_at(value: str) -> datetime:
"""Parse a hop's ``delegated_at`` ISO 8601 timestamp (accepts a 'Z' suffix).
A timestamp with no offset is treated as UTC rather than left naive: hops in
one chain are written by different parties and need not agree on whether to
include one, and comparing a naive against an aware datetime raises
TypeError, which is not a refusal.
"""
dt = datetime.fromisoformat(value.replace("Z", "+00:00"))
return dt.replace(tzinfo=timezone.utc) if dt.tzinfo is None else dtPlease add a test for a chain whose parent hop carries Z and whose child does not, asserting it still refuses on absolute expiry rather than raising.
Everything else here I would merge as is. Push that and I will re-review straight away.
|
@imran-siddique Thank you for the review, have made the changes, kindly have a look. |
What
The ttl_seconds check only compared the parent and child durations, without considering when each delegation was issued. Since ttl_seconds is measured from each hop’s own delegated_at, a child delegated later could actually expire after its parent, even with a smaller TTL. The check should compare their absolute expiry times instead.
Why
Concrete failure: A root grant valid for 1 hour can delegate again 50 minutes later with another 1-hour TTL. The old check only compared 3600 to 3600, it passed but the child would actually remain valid 50 minutes longer than the root.
Spec impact
None
Test plan
pytest -vpassesmypy src/agent_manifestpassesruff check src/ tests/passesCHANGELOG.mdupdatedDCO
All commits in this PR are signed off (
git commit -s). By submitting this PR I certify the Developer Certificate of Origin.