Skip to content

feat(provenance): freshness and revocation, with the semantics sign.py documents - #164

Open
lywinged wants to merge 2 commits into
agentrust-io:mainfrom
lywinged:fix/provenance-freshness-and-revocation
Open

feat(provenance): freshness and revocation, with the semantics sign.py documents#164
lywinged wants to merge 2 commits into
agentrust-io:mainfrom
lywinged:fix/provenance-freshness-and-revocation

Conversation

@lywinged

Copy link
Copy Markdown
Contributor

Closes #151.

issued_at has been required since this format existed, and _check_structure says why:

A record with no issue time cannot be aged, so a consumer has no way to reject a stale one.

Nothing aged it. Walking the module for Compare nodes mentioning issued_at gives two — the type check and build_record's default — and the only time.time() call was build_record stamping it.

def verify_record(record, trusted_jwk, *,
                  revocation: RevocationStore | None = None,
                  max_age_seconds: int | None = None,
                  max_future_skew_seconds: int = 300) -> None:

Nothing new was needed. RevocationStore and _check_not_revoked already exist in sign.py and are reused rather than reimplemented.

On revocation, precisely

This is not "one format checks and the other does not" — Trust Records skip revocation by default too, as documented. It is that one format gave the consumer the hook and the other did not.

The obvious answer is that the caller holds trusted_jwk and can consult a CRL themselves. That works less well than it looks, because the rule the library uses to decide what a key is listed under is private:

def _key_identifiers(jwk):          # not exported
    identifiers = [jwk_thumbprint(jwk)]
    kid = jwk.get("kid")
    if isinstance(kid, str) and kid and kid not in identifiers:
        identifiers.append(kid)
    return identifiers

A caller writing the check by hand reaches for the thumbprint and misses every entry listed by kid — which is what kid is for. The substitute exists and diverges, in the direction of accepting a revoked key.

Consulted before the signature check, because a signature made by a revoked key stays cryptographically valid and the verifier is the only place the fact can be applied.

On the two defaults

max_age_seconds defaults to None, not to the 86400 of a Trust Record. A provenance record describes an artifact by immutable digest, much like a package signature, and those are conventionally valid indefinitely — a default bound here would be wrong. The docstring names endpoint identity as the case where that reasoning does not hold, since a URL and an SPKI digest decay.

max_future_skew_seconds is enforced whether or not an age bound is set. Adding max_age_seconds alone would have shipped the defect #155 had just fixed for Trust Records: a far-future issued_at sits inside any later age window until that time arrives.

Tests

Ten, and load-bearing rather than decorative — measured by removing each check and counting what notices:

Removed Tests that fail
the freshness block 3
only the future-skew check 1
the revocation call 3
kid from the revocation identifiers 1

The last row is the point of the whole change: that is the caller-side substitute, and it is why the hook belongs here rather than in the caller.

Verification

Fresh clone in a directory that has never held the repository: 353 pass, 1 skipped. ruff and mypy clean, and ruff was clean on the same files before this change.

Merges with #149 without conflict. That PR makes the revocation half cheaper by bringing jwk_thumbprint into this module, but neither depends on the other and they can land in either order.

…y documents

Closes agentrust-io#151.

issued_at has been required since this format existed, and _check_structure
explains why: "A record with no issue time cannot be aged, so a consumer has no
way to reject a stale one." Nothing aged it. Walking the module for Compare
nodes mentioning issued_at gave two, the type check and build_record's default,
and the only time.time() call was build_record stamping it.

sign.verify_record took revocation; provenance.verify_record took nothing. That
is not "one checks and the other does not" - Trust Records skip revocation by
default too - it is that one format gave the consumer the hook and the other did
not, and the caller-side substitute is not equivalent. _key_identifiers is
private and returns the thumbprint *and* the kid, so a caller writing the check
by hand reaches for the thumbprint and misses every entry listed by kid, which is
what kid is for. It diverges in the direction of accepting a revoked key.

    def verify_record(record, trusted_jwk, *,
                      revocation: RevocationStore | None = None,
                      max_age_seconds: int | None = None,
                      max_future_skew_seconds: int = 300) -> None:

Nothing new was needed: RevocationStore and _check_not_revoked already exist and
are reused rather than reimplemented.

max_age_seconds defaults to None, not to the 86400 of a Trust Record. A
provenance record describes an artifact by immutable digest, like a package
signature, and those are conventionally valid indefinitely; a default bound would
be wrong. The docstring names endpoint identity as the case where that reasoning
does not hold, since a URL and an SPKI digest decay.

max_future_skew_seconds is enforced whether or not an age bound is set. Adding
max_age_seconds alone would have shipped the defect agentrust-io#155 had just fixed for Trust
Records: a far-future issued_at sits inside any later age window until that time
arrives.

Revocation is consulted before the signature, because a signature made by a
revoked key stays cryptographically valid and the verifier is the only place the
fact can be applied.

Ten tests. Load-bearing, measured rather than assumed: removing the freshness
block fails three, removing only the future-skew check fails one, removing the
revocation call fails three, and keying revocation on the thumbprint alone -
the caller-side substitute - fails one. That last is the kid case, and it is why
the hook has to live here rather than in the caller.

353 pass; ruff and mypy clean, with ruff clean on the same files before this
change. Merges with agentrust-io#149 without conflict; that PR makes the revocation half
cheaper by bringing jwk_thumbprint into this module, but neither depends on the
other.

Signed-off-by: Louielunz <48041247+lywinged@users.noreply.github.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Contributor Check: UNKNOWN

Check Result
Profile UNKNOWN
Credential LOW
Overall UNKNOWN

Automated check by AgenTrust Contributor Check.

@imran-siddique imran-siddique left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The freshness and revocation hooks are the right shape, but max_age_seconds lacks the validation already applied to max_future_skew_seconds. Passing max_age_seconds=-1 does not report invalid verifier configuration; it classifies essentially every current record as stale. Please require a non-negative integer (excluding bool) when supplied, do the same strict type check for max_future_skew_seconds, and add boundary tests for negative, boolean, and non-integer values. This is a public security-verifier API, so silently accepting malformed policy inputs is unsafe.

Per the review: max_age_seconds had none of the validation max_future_skew_seconds had, so max_age_seconds=-1 was not reported as an invalid verifier configuration - it classified every record ever issued as stale, uniformly, with no error naming the cause. Both parameters now go through one _check_seconds, which rejects non-integers and negatives and excludes bool explicitly, since bool subclasses int in Python and True would otherwise pass as one second. Boundary tests cover negative, boolean, float, string and object inputs for each parameter, plus the case that motivates keeping them distinct: 0 is the strictest expressible bound and None disables the bound, and a validator that treated 0 as falsy would silently accept everything under the strictest policy a caller can write. Merged main in as well (the branch predated agentrust-io#149 and agentrust-io#154); 424 pass, ruff clean. Removing the max_age check fails 4 tests.
@lywinged

Copy link
Copy Markdown
Contributor Author

Fixed in 724fe9e. You were right that the asymmetry was the whole problem:
max_age_seconds=-1 was not a stricter bound, it classified every record ever
issued as stale, uniformly, and a caller who meant to disable the bound got
a blanket refusal with no error naming the cause.

Both parameters now go through one _check_seconds, so the validation cannot
drift apart again. It rejects non-integers and negatives, and excludes bool
explicitly - bool subclasses int in Python, so max_age_seconds=True
would otherwise have been accepted as a one-second bound.

Boundary tests cover -1, -86400, True, False, 1.5, "300" and a bare object
for max_age_seconds, and the same set plus None for max_future_skew_seconds.
One more test keeps 0 and None distinct: 0 is the strictest bound expressible

  • the record must be issued at this instant - and None disables the bound, so
    a validator treating 0 as falsy would silently accept everything under the
    strictest policy a caller can write.

Load-bearing rather than asserted: removing the max_age check fails 4 tests,
removing the skew check fails 6.

The branch also predated #149 and #154, so main is merged in - the conflicts
were import-level only. 424 passed, 1 skipped, ruff clean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-review:UNKNOWN Contributor check flagged UNKNOWN risk

Projects

None yet

Development

Successfully merging this pull request may close these issues.

provenance: issued_at is required for an aging check the verifier never performs, and there is no hook to add one

2 participants