fix(catalog): pin the approval policy verifier-side and bind the chain fields (#517)

[zohebk8s]

Follow-up to #519, which implemented #517. Does not close #517; see Not covered.

Why

The module advertises M-of-N approval and does not enforce it. verify_catalog_change reads threshold, distinct_principals, and distinct_roles out of the record it is verifying, and checks policy_hash for digest shape only: it is never recomputed over the policy body and never compared to anything the verifier was configured with.

A single trusted reviewer key is therefore sufficient. Set threshold: 1, distinct_principals: false, put arbitrary bytes in policy_hash, sign once:

result: {'verified': True, 'valid_approvals': 1, 'new_catalog_hash': 'sha256:3333...'}
policy_hash was garbage and threshold was self-declared as 1

This is the failure the spec doc already forbids for keys, "no record-embedded key can bootstrap trust", applied to the policy instead. M is only meaningful when it comes from verifier-side configuration.

Three smaller defects sit behind the same seam:

• catalog_id, sequence, and previous_catalog_hash were format checked and then never used, so a record for a different catalog verified against this one.
• Distinctness counted distinct values rather than rejecting repeats, so alice, bob, alice satisfied a threshold of two under a distinct-principal policy.
• Field types were unvalidated. A non-hashable role escaped as TypeError rather than CatalogApprovalError, and boolean timestamps were accepted where the shipped JSON Schema rejects them.

What

The policy is pinned verifier-side. expected_policy_hash and expected_catalog_id are new required keyword arguments. A record's policy_hash must cover its own policy body, and must equal the policy the verifier was configured with. compute_policy_hash defines that digest, over the policy object with policy_hash removed, so producers and verifiers agree on it. The spec doc left this undefined.

The chain fields are bound. catalog_id is always checked. expected_sequence and expected_previous_catalog_hash join the existing expected_previous_record_hash as optional checkpoints, optional because #517 is explicit that they must come from an external pin or transparency receipt, which the record cannot supply.

Repeats are rejected as each approval is read, rather than counted after the loop, which also makes the post-loop threshold checks unreachable; they are removed.

A reviewer key counts once, whatever the policy says, which is the part the distinctness rules did not cover. Nothing bound approvals to distinct keys, so both of these verified:

threshold 3, distinct_principals false, one key signing the identical approval 3 times
  -> {'verified': True, 'valid_approvals': 3, ...}

threshold 2, distinct_roles true, one key signing twice as "security" and "owner"
  -> {'verified': True, 'valid_approvals': 2, ...}

The first is arguably what that policy asked for, though a repeated signature is one approval presented three times rather than three approvals. The second is not what the policy asked for at all: it demands two roles and is satisfied by one key, because a TrustedReviewer with role=None lets the record assert whatever role string it likes. The check sits after the principal and role rules so their more specific errors still win. This predates this PR and is on main in the same form, but M-of-N is what this PR claims to make enforceable, so it belongs here.

Types are validated. String and integer fields are checked before use, booleans are rejected where an integer is required, and the signature is checked against the base64url alphabet and for a 64 byte decode. The decode moves out of the try block where its errors were being reported as "signature is invalid".

Tests

The four tests merged with #519 pass whether or not the policy, identity, and validity checks exist, which is how this shipped. The suite is now 31 tests, and every guard was checked by deleting it and confirming a test fails:

drop expected-policy pin     -> 1 failed, 29 passed
drop policy-body digest      -> 1 failed, 29 passed
drop catalog_id bind         -> 1 failed, 29 passed
drop key-reuse rule          -> 2 failed, 28 passed
drop repeated-principal      -> 1 failed, 29 passed
drop distinct-role check     -> 1 failed, 29 passed
drop signature length        -> 1 failed, 29 passed
drop threshold count         -> 2 failed, 28 passed
drop role match              -> 1 failed, 29 passed
drop principal/issuer match  -> 1 failed, 29 passed
drop interval order          -> 1 failed, 29 passed
expires_at made inclusive    -> 1 failed, 29 passed
drop distinctness type guard -> 1 failed, 29 passed

Each row is one guard deleted from approval.py, the suite rerun, and the source restored. The xfailed case below stays xfailed throughout and is never one of the failures.

Added beyond the fixes above: reviewer identity mismatches, validity interval boundaries, a genesis record at sequence: 1, and an agreement test over eleven malformed records asserting that whatever the shipped schema rejects the verifier rejects too. That last one is what would have caught the boolean timestamp divergence.

The last commit closes the remaining error branches, including the four that were uncovered before this PR. One of them is only reachable with a signature whose length is 1 mod 4, since a bad alphabet is rejected before the decode and anything longer decodes and then fails the length check. approval.py is at 100 percent line coverage.

tests/unit/test_catalog_approval.py   30 passed, 1 xfailed
with test_catalog_canonical_json.py    43 passed, 1 xfailed
coverage of approval.py               159 statements, 0 missing, 100%
ruff                                   All checks passed
mypy --strict                          no issues, 1 source file

The xfail is deliberate and strict. The schema sets minimum: 0 on approved_at and expires_at; the verifier applies no lower bound, so a record with a negative approval timestamp verifies. Fixing it belongs with the decision on whether the verifier loads the schema at all, so the test documents the divergence rather than hiding it, and will start failing the moment that lands.

Decisions worth reviewing

The signature change is breaking, and deliberately so. Two new required keyword arguments cannot be defaulted without leaving the hole open: any default for expected_policy_hash means falling back to the record's own claim. Nothing in the tree calls verify_catalog_change yet, so the cost is zero today and rises with every week it waits.

compute_policy_hash excludes policy_hash from its own input. The alternative is hashing the policy as configured, including the field, which is self-referential. Either convention works as long as it is written down; this one is now in the spec doc.

Not covered

• Approvals are still judged against the verification clock, so a record stops verifying once its approvals expire. A provenance chain that cannot be replayed later is a weak provenance chain, and the fix depends on which instant key validity should be judged at, which is a maintainer decision rather than something to guess at here. Tracked in decide: approval validity clock and schema wiring for catalog approval provenance #533.
• schemas/catalog-approval.schema.json is still not loaded by the verifier and still not in [tool.hatch.build.targets.wheel.force-include], so it does not ship in the wheel. Whether the verifier should load it, and what a genesis record puts in previous_record_hash, are the same kind of decision. Also decide: approval validity clock and schema wiring for catalog approval provenance #533.
• canonical_json is not the RFC 8785 it claims to be. That was fix(catalog): make the approval signing input the JCS it claims to be (#517) #532, kept separate as an interop question rather than an authorization one, and it has landed. This branch is rebased on it: the two constants sit side by side, both CHANGELOG entries are kept, and the last commit carries fix(catalog): make the approval signing input the JCS it claims to be (#517) #532's end to end test onto the verifier signature this PR changes, since leaving it would break main the moment this merges.
• The module is still not wired into startup, the CLI, or the proxy, and the automated-checks digest is still opaque to the verifier. Both look like later phases of design: verifiable approval provenance for tool catalogs #517 rather than omissions.

[github-actions]

🟡 Contributor Check: MEDIUM

Check	Result
Profile	MEDIUM
Credential	LOW
Overall	MEDIUM

Automated check by AgenTrust Contributor Check.

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[imran-siddique]

#532 has landed, and it touches the same catalog approval signing path this changes, so this now conflicts in src/cmcp_runtime/catalog/approval.py.

Could you rebase on main? I did not resolve it here: after #532 the two fixes overlap, and picking the resolution inside the signing input is the kind of thing that quietly changes what gets signed. You know which parts of this are still needed once #532 is in; I would only be guessing.

Everything else on the PR is green.

[zohebk8s]

Rebased on main. Three interactions with #532:

• approval.py, two adjacent module constants, _MAX_EXACT_INT and _B64URL. Both kept.
• CHANGELOG.md, both entries at the same anchor. Both kept.
• tests/unit/test_catalog_canonical_json.py merged cleanly and then failed: its end to end test calls verify_catalog_change, which this branch gives two required keyword arguments. The last commit carries it onto the new signature, since leaving it would break main the moment this merges. The record it builds is unchanged apart from computing policy_hash with compute_policy_hash, which is what the verifier now requires the field to cover.

The canonicalization is untouched: canonical_json and _approval_input are byte for byte what #532 merged. Everything else here is still needed, since #532 changed only the signing input encoding, not the policy pinning, the chain binding, the distinctness rule, or the type and signature validation.

After the rebase: 41 passed and 1 xfailed across both catalog test files, approval.py at 155 statements with none missing, ruff and mypy strict clean, and the full unit suite matching main. The xfail is the negative approval timestamp, waiting on #533.

[zohebk8s]

One more commit, and it changes behaviour rather than just moving it, so flagging it rather than letting it ride in after your review.

Going back over the M-of-N property I found that the distinctness rules do not close it, because nothing bound approvals to distinct keys. Both of these verified on the branch as you reviewed it, and on main today:

threshold 3, distinct_principals false, one key signing the identical approval 3 times
  -> {'verified': True, 'valid_approvals': 3, ...}

threshold 2, distinct_roles true, one key signing twice as "security" and "owner"
  -> {'verified': True, 'valid_approvals': 2, ...}

The first is defensible as what that policy asked for, though a repeated signature is one approval presented three times rather than three approvals. The second is not: the policy demands two roles and one key satisfies it, because a TrustedReviewer with role=None lets the record assert whichever role it likes.

A reviewer key now counts once per record whatever the policy says. The check runs after the principal and role rules so those keep reporting the more specific cause, and a legitimate 2-of-2 is unaffected. Two tests, one per case, and a sentence in the spec doc stating the rule.

This predates the PR and I could have filed it separately. It is here because M-of-N being enforceable is what the PR claims, and it was not while a single key could satisfy any threshold. Happy to split it out if you would rather review it on its own.

Branch now: 43 passed and 1 xfailed across both catalog test files, approval.py at 159 statements with none missing, ruff and mypy strict clean. Removing the new guard fails 2 of the 31 tests.

[imran-siddique]

Reviewed the rebased branch. This closes a real authorization bypass, not a hardening nit.

The bypass as it stood: verify_catalog_change read threshold, distinct_principals and distinct_roles out of the record it was verifying, and checked policy_hash for digest shape only. So one trusted reviewer key, threshold: 1, arbitrary bytes in policy_hash, sign once, and the record verifies. M-of-N was advertised and never enforced. Pinning expected_policy_hash and expected_catalog_id verifier-side is the right fix, and compute_policy_hash gives producers and verifiers a shared definition the spec doc had left undefined.

The key-reuse commit you flagged separately is the one I would have asked for if it were not already here. The distinctness rules did not close M-of-N, because nothing bound approvals to distinct keys: threshold 2, distinct_roles true was satisfied by a single key signing twice as "security" and "owner", since a TrustedReviewer with role=None lets the record assert whatever role string it likes. Counting a key once, and placing that check after the principal and role rules so the specific errors still win, is correct.

What I verified rather than took on trust:

• The guard-deletion table. Each row is a guard removed and the suite failing, which is the evidence that the four tests merged with #519 passed with or without the checks existing. That is how this shipped in the first place, and it is the reason the new tests are worth more than their line count.
• The breaking signature change is free today. verify_catalog_change has no callers on main outside its own definition and the two test files, at 3332fef. Two required keyword arguments cannot be defaulted without falling back to the record's own claim, so defaulting them would reopen the hole. Taking the break now is right.
• The test_catalog_canonical_json.py end-to-end test being carried onto the new signature in this branch. Leaving it would have broken main on merge.

Non-blocking, and already tracked in #533: the verification-clock question, the schema not being loaded or shipped in the wheel, and the negative-timestamp divergence held as a strict xfail. Holding that as xfail rather than quietly widening the verifier is the right call, and it will start failing the moment the schema decision lands.

Merging. #517 stays open for the wiring phases.