fix(catalog): judge approvals at the approval instant, not the wall clock (#533) - #547
fix(catalog): judge approvals at the approval instant, not the wall clock (#533)#547zohebk8s wants to merge 2 commits into
Conversation
…ecords schemas/catalog-approval.schema.json shipped in the repository and nowhere else. verify_catalog_change reimplemented structural validation by hand, the two drifted, and the file was absent from the wheel force-include, so an installed distribution did not carry it at all. agentrust-io#531 could only document one divergence, a negative approved_at that the schema rejects and the verifier accepted, as a strict xfail. The record is now validated against the schema before any other check, and verification refuses outright when the schema is missing from the installation, following loader.py rather than falling back to hand-written checks. Duplicated checks are deleted rather than left unreachable: field presence and unknown members on the record, the policy and each approval, digest shapes, integer and boolean types, string emptiness, and the signature alphabet. _require_str and _require_int go with them. What stays is what JSON Schema cannot express: * the runtime catalog hash binding and the caller's chain checkpoints * the policy pin, including that policy_hash covers its own body * reviewer identity, role, revocation and key reuse * the validity interval ordering * the signatures The xfail is now a plain assertion, since the case fails closed. previous_record_hash on the first record in a chain is the all-zero digest, sha256: followed by 64 zeros, written into the schema and the spec doc. It validated against the pattern before and meant nothing. Packaging: the schema is force-included beside catalog-entry.schema.json, and verify_python_distribution.py checks both resolve to a file inside the installed distribution, so a wheel that drops one fails the release smoke test rather than a verifier at runtime. A unit test asserts the force-include mapping, since that line is the only thing putting these files next to the code. loader.py's _CATALOG_ENTRY_SCHEMA_PATH is now CATALOG_ENTRY_SCHEMA_PATH, since the distribution check imports it. Verified by mutation: dropping the schema call fails 7 tests, accepting a missing schema fails 1, dropping the 64-byte signature check fails 1. Coverage of approval.py is 149 statements with none missing. Signed-off-by: Mohammed Zoheb Shaik <zoheb.shaik7@gmail.com>
…lock verify_catalog_change checked approved_at and expires_at against time.time(), so a record that was valid when the catalog was approved stopped verifying once its approvals expired. The chain agentrust-io#517 exists to let an auditor replay could not be replayed, which makes the record an authorization token with a lifetime rather than a provenance record. The interval is now what it says it is: an assertion about when the signature could have been produced. The caller passes validity_instant, a pinned checkpoint or transparency-receipt timestamp where it has one. Where it passes nothing, each approval is judged at its own approved_at, so the record stays verifiable indefinitely. Requiring a pin would mean an auditor cannot verify a record without also holding the pin, which is a worse default than the one it replaces. The `now` parameter goes with the wall clock, and `time` is no longer imported. The rename is deliberate: the semantics changed, and a parameter still called `now` would invite callers to keep passing the verification clock. Revocation is untouched and stays at verification time. The reason is now stated where the check is, not only in the doc: whether a key was valid when it signed and whether it is revoked now are different questions. Tests: a record with approvals valid for [100, 200) verifies with no instant supplied, verifies at 150, and is refused at 10**9; and a revoked key is refused whatever instant the approvals are judged at. Verified by mutation: restoring the wall clock fails 2 tests, dropping the window check fails 3, moving revocation off verification time fails 2. Signed-off-by: Mohammed Zoheb Shaik <zoheb.shaik7@gmail.com>
|
🟡 Contributor Check: MEDIUM
Automated check by AgenTrust Contributor Check. |
github-actions
Bot
added
the
needs-review:MEDIUM
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
One consequence of the clock change is worth stating explicitly, since it is not a defect and I do not want it to arrive as a surprise later. With the interval no longer acting as a lifetime, a record replays indefinitely. That means revocation is now the only mechanism that stops a historical approval from counting. Before this change, an approval signed by a key that later turned out to be compromised aged out on its own once That follows from your decision rather than working against it, and it is the reason revocation stays at verification time. It does mean an operator's revocation list is load-bearing rather than a backstop, which is worth a sentence in the spec doc if you agree. Related, and only as information: |
|
#546 is merged, so this needs a rebase onto Worth saying exactly why, because it is not a conflict you caused. I diffed the two branches against each other before merging: all 139 added lines in #546 appear verbatim in this PR, so #546 was a strict subset. Rebasing should drop the whole schema-wiring half and leave only the validity-clock change, roughly 71 lines. What becomes redundant on the rebase, so nothing gets hand-reconciled twice:
That leaves this PR as the answer to decision 1 in #533, which is the half that needs a maintainer ruling on which instant governs rather than a code review. Splitting it out this way makes that ruling reviewable on its own, which is the point. Your note about revocation becoming the only mechanism that stops a historical approval is the right thing to have raised before it landed, not after. It goes in the spec doc alongside the clock change. |
4 participants
Closes the clock half of #533. Stacked on #546, so review the second commit; the first is that PR unchanged.
Why
verify_catalog_changejudgedapproved_atandexpires_atagainsttime.time(). A record that was valid when the catalog was approved stopped verifying the moment its approvals expired, so the chain #517 exists to let an auditor replay could not be replayed. That makes the record an authorization token with a lifetime rather than a provenance record.What
The interval is an assertion about when the signature could have been produced, not a lifetime on the record. The caller passes
validity_instant, a pinned checkpoint or transparency-receipt timestamp where it has one. Where it passes nothing, each approval is judged at its ownapproved_at, so the record stays verifiable indefinitely.Requiring a pin instead would mean an auditor cannot verify a record without also holding the pin, which is a worse default than the one it replaces.
nowis gone with the wall clock, andtimeis no longer imported. The rename is deliberate: the semantics changed, and a parameter still callednowinvites a caller to keep passing the verification clock into it.Revocation stays at verification time, with the reason stated where the check is rather than only in the doc, since the two read as the same check to whoever touches this next:
The spec doc carries both rules, including that the interval bounds when the signature could have been produced.
Tests
The replay test is the one that would have failed before this change: approvals valid for
[100, 200), verified with no instant supplied, verified at 150, refused at10**9. A second pins that a revoked key is refused whatever instant the approvals are judged at.Verified by mutation:
One thing worth your eye
TrustedReviewercarriesprincipal_id,issuer,keyandrole, and no validity window. So "reviewer key validity is checked against an instant inside that window" has nothing to check against beyond the approval's own interval, and that is what this implements: the instant governs the approval interval, not a key lifetime.