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

CHANGELOG.md

@@ -9,6 +9,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- **A single trusted reviewer key could approve any catalog change, under any policy (#517, follow-up to #519).** `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: never recomputed over the policy body, never compared to anything. A record declaring `threshold: 1` with arbitrary bytes in `policy_hash` verified on one signature, so the M-of-N property the module advertises was unenforced. 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.
+
+  `expected_policy_hash` and `expected_catalog_id` are now required arguments. A record's `policy_hash` must cover its own policy body, which `compute_policy_hash` defines so that producers and verifiers agree on it, and must equal the policy the verifier was configured with. Neither can be defaulted without falling back to the record's own claim, so the signature change is deliberate; nothing calls this module yet.
+
+  `catalog_id`, `sequence`, and `previous_catalog_hash` were format checked and then unused, so a record for a different catalog verified against this one. `catalog_id` is now always bound, and `expected_sequence` and `expected_previous_catalog_hash` join `expected_previous_record_hash` as optional checkpoints, optional because they must come from an external pin or transparency receipt. Distinctness now rejects repeats as each approval is read rather than counting distinct values afterwards, which had admitted alice, bob, alice at a threshold of two. String and integer fields are validated before use, so a non-hashable `role` raises `CatalogApprovalError` instead of escaping as `TypeError`, boolean timestamps are rejected as the shipped schema already rejects them, and the signature is checked against the base64url alphabet and for a 64 byte decode, with the decode moved out of the `try` block that was reporting its errors as "signature is invalid".
+
+  The distinctness rules did not close the property on their own, because nothing bound approvals to distinct keys. A threshold of three under a policy that does not require distinct principals was satisfied by one key signing the identical approval three times, and a threshold of two under a policy that does require distinct roles was satisfied by one key signing twice as two different roles, since a `TrustedReviewer` carrying no role lets the record assert whichever role it likes. A reviewer key now counts once per record whatever the policy says: a repeated signature is one approval presented N times, not N approvals. The check runs after the principal and role rules so those keep reporting the more specific cause.
+
+  The four tests that shipped with #519 passed whether or not any of these checks existed. The suite is now 31 tests, each guard verified by deleting it and confirming a test fails. One is xfailed on purpose: the schema sets a minimum of zero on the approval timestamps and the verifier does not, which is left failing until the schema-wiring decision lands.
+
 - **The catalog-approval signing input was not the JCS it claims to be (#517).** `canonical_json` is documented as RFC 8785 compatible and serialized with `ensure_ascii=True`, which is the one thing JCS does not do: it emits an ASCII escape where the standard emits UTF-8. Any record carrying a non-ASCII `principal_id`, `issuer`, `role`, `catalog_id`, or `policy_id` was signed over different bytes than a conforming producer signs, so a record produced anywhere but here failed with an invalid-signature error that points nowhere near the encoding. `sort_keys` was the second divergence, ordering members by code point where JCS orders by UTF-16 code unit. The two disagree for any key outside the BMP, since a surrogate pair leads with `0xD800` and sorts below a BMP character above `0xE000`.
 
   Members are now ordered on their UTF-16BE bytes and the output is UTF-8. Values JCS cannot pin down are refused rather than serialized into a signing input that two implementations would read differently: floating point numbers, integers beyond `2**53 - 1`, non-string keys, and unpaired surrogates, each as `CatalogApprovalError` rather than as an escaping `UnicodeEncodeError` or `TypeError`. Approval records carry none of those, so refusing them only closes a door.

docs/spec/catalog-approval-provenance.md

@@ -11,9 +11,33 @@ catalog hashes, a change-set digest, an automated-checks digest, and an
 approval policy. Every approval is an Ed25519 signature over the record body and
 its own principal, issuer, role, validity interval, and key identifier. The
 verifier resolves keys from trusted configuration; no record-embedded key can
-bootstrap trust. It rejects unknown fields, revoked or expired keys, invalid
-signatures, duplicate principals or roles when the policy requires distinctness,
-and records whose `new_catalog_hash` differs from the runtime catalog hash.
+bootstrap trust.
+
+The same rule applies to the policy. A record states which policy it followed,
+but the verifier is given the expected policy hash and catalog identifier from
+its own configuration, and rejects any record citing a different policy. The
+record's `policy_hash` must also cover its own policy body, computed over the
+policy object with the `policy_hash` field removed. Without this pinning a
+single reviewer key could issue a record declaring a threshold of one, so the
+M-of-N property depends on the policy being verifier-supplied rather than
+record-supplied.
+
+The verifier rejects unknown fields, revoked or expired keys, invalid
+signatures, repeated principals or roles when the policy requires distinctness,
+and records whose `new_catalog_hash` differs from the runtime catalog hash. A
+reviewer key counts once regardless of policy: a repeated signature is one
+approval presented N times, not N approvals, and without that rule a threshold
+of N is satisfiable by a single key whenever the policy does not demand
+distinct principals, or demands distinct roles while the key's trusted entry
+pins no role.
+The key identifier is the reviewer key's identity for both of those rules. A
+verifier that registers one key under two identifiers has neither: the record
+can present it twice toward one threshold, and revoking one identifier leaves
+the other usable. `trusted_reviewers` must therefore map distinct identifiers to
+distinct keys.
+`catalog_id` is always checked. `sequence`, `previous_record_hash`, and
+`previous_catalog_hash` are checked when the caller supplies the corresponding
+checkpoint, which it must obtain externally as described below.
 
 The signing input is RFC 8785 (JCS): UTF-8 output, object members ordered by
 their UTF-16 code units, and no escaping beyond what ECMAScript `JSON.stringify`

src/cmcp_runtime/catalog/approval.py

@@ -20,6 +20,8 @@
 # RFC 8785 numbers are IEEE 754 doubles, so integers stay exact only to 2**53 - 1.
 _MAX_EXACT_INT = 2**53 - 1
 
+_B64URL = frozenset("ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_")
+
 
 class CatalogApprovalError(ValueError):
     """The detached approval record is malformed or cannot be trusted."""
@@ -83,17 +85,29 @@ def digest_json(value: Any) -> str:
     return "sha256:" + hashlib.sha256(canonical_json(value)).hexdigest()
 
 
+def compute_policy_hash(policy: dict[str, Any]) -> str:
+    """Digest of the approval policy body, excluding the policy_hash field itself.
+
+    Producers and verifiers must agree on this definition, otherwise the policy a
+    record claims to follow cannot be pinned to the policy the verifier trusts.
+    """
+    return digest_json({k: v for k, v in policy.items() if k != "policy_hash"})
+
+
 def _b64(value: bytes) -> str:
     return base64.urlsafe_b64encode(value).rstrip(b"=").decode("ascii")
 
 
 def _decode(value: Any) -> bytes:
-    if not isinstance(value, str) or not value:
-        raise CatalogApprovalError("signature must be a base64url string")
+    if not isinstance(value, str) or not value or any(c not in _B64URL for c in value):
+        raise CatalogApprovalError("signature must be an unpadded base64url string")
     try:
-        return base64.urlsafe_b64decode(value + "=" * ((4 - len(value) % 4) % 4))
+        raw = base64.urlsafe_b64decode(value + "=" * ((4 - len(value) % 4) % 4))
     except (ValueError, TypeError) as exc:
         raise CatalogApprovalError("signature is not valid base64url") from exc
+    if len(raw) != 64:
+        raise CatalogApprovalError("signature must decode to 64 bytes")
+    return raw
 
 
 def _approval_input(record: dict[str, Any], approval: dict[str, Any]) -> bytes:
@@ -119,16 +133,39 @@ def _require_digest(value: Any, field: str) -> str:
     return value
 
 
+def _require_str(value: Any, field: str) -> str:
+    if not isinstance(value, str) or not value:
+        raise CatalogApprovalError(f"{field} must be a non-empty string")
+    return value
+
+
+def _require_int(value: Any, field: str) -> int:
+    if not isinstance(value, int) or isinstance(value, bool):
+        raise CatalogApprovalError(f"{field} must be an integer")
+    return value
+
+
 def verify_catalog_change(
     record: dict[str, Any],
     trusted_reviewers: dict[str, TrustedReviewer],
     *,
     runtime_catalog_hash: str,
+    expected_policy_hash: str,
+    expected_catalog_id: str,
     now: int | None = None,
     revoked_key_ids: frozenset[str] = frozenset(),
+    expected_sequence: int | None = None,
     expected_previous_record_hash: str | None = None,
+    expected_previous_catalog_hash: str | None = None,
 ) -> dict[str, Any]:
-    """Verify policy, chain, reviewer signatures, freshness, and runtime binding."""
+    """Verify policy, chain, reviewer signatures, freshness, and runtime binding.
+
+    The approval policy is never taken on the record's word: `expected_policy_hash`
+    and `expected_catalog_id` come from verifier-side configuration, so a record
+    cannot declare its own threshold or distinctness rules. The chain checkpoints
+    stay optional because they must come from an external pin or transparency
+    receipt, which the record itself cannot supply.
+    """
     if not isinstance(record, dict) or record.get("profile") != PROFILE:
         raise CatalogApprovalError("unknown or missing catalog approval profile")
     required = {
@@ -138,32 +175,48 @@ def verify_catalog_change(
     }
     if set(record) != {"profile", *required}:
         raise CatalogApprovalError("record contains missing or unknown fields")
-    if not isinstance(record["sequence"], int) or isinstance(record["sequence"], bool) or record["sequence"] < 1:
+    if _require_int(record["sequence"], "sequence") < 1:
         raise CatalogApprovalError("sequence must be a positive integer")
     for field in ("previous_record_hash", "previous_catalog_hash", "new_catalog_hash", "change_set_digest", "automated_checks_digest"):
         _require_digest(record[field], field)
+    if _require_str(record["catalog_id"], "catalog_id") != expected_catalog_id:
+        raise CatalogApprovalMismatch("record does not apply to the expected catalog")
+    if expected_sequence is not None and record["sequence"] != expected_sequence:
+        raise CatalogApprovalMismatch("record is not the expected sequence number")
     if expected_previous_record_hash is not None and record["previous_record_hash"] != expected_previous_record_hash:
         raise CatalogApprovalMismatch("record is not the expected next chain element")
+    if expected_previous_catalog_hash is not None and record["previous_catalog_hash"] != expected_previous_catalog_hash:
+        raise CatalogApprovalMismatch("previous_catalog_hash does not match the expected checkpoint")
     if record["new_catalog_hash"] != runtime_catalog_hash:
         raise CatalogApprovalMismatch("new_catalog_hash does not match runtime catalog hash")
 
     policy = record["approval_policy"]
     if not isinstance(policy, dict) or set(policy) != {"policy_id", "policy_hash", "threshold", "distinct_principals", "distinct_roles"}:
         raise CatalogApprovalError("approval_policy has missing or unknown fields")
+    _require_str(policy["policy_id"], "approval_policy.policy_id")
     _require_digest(policy["policy_hash"], "approval_policy.policy_hash")
+    if not isinstance(policy["distinct_principals"], bool) or not isinstance(policy["distinct_roles"], bool):
+        raise CatalogApprovalError("approval_policy distinctness flags must be booleans")
     threshold = policy["threshold"]
-    if not isinstance(threshold, int) or isinstance(threshold, bool) or threshold < 1:
+    if _require_int(threshold, "approval threshold") < 1:
         raise CatalogApprovalError("approval threshold must be a positive integer")
+    if compute_policy_hash(policy) != policy["policy_hash"]:
+        raise CatalogApprovalError("approval_policy.policy_hash does not cover the policy body")
+    if policy["policy_hash"] != _require_digest(expected_policy_hash, "expected_policy_hash"):
+        raise CatalogApprovalMismatch("record cites a policy the verifier does not trust")
     instant = int(time.time()) if now is None else now
     approvals = record["approvals"]
     if not isinstance(approvals, list) or len(approvals) < threshold:
         raise CatalogApprovalMismatch("approval threshold is not satisfied")
     principals: set[str] = set()
     roles: set[str] = set()
+    keys_used: set[str] = set()
     valid = 0
     for approval in approvals:
         if not isinstance(approval, dict) or set(approval) != {"principal_id", "issuer", "key_id", "role", "approved_at", "expires_at", "signature"}:
             raise CatalogApprovalError("approval has missing or unknown fields")
+        for field in ("principal_id", "issuer", "key_id", "role"):
+            _require_str(approval[field], f"approval.{field}")
         key_id = approval["key_id"]
         reviewer = trusted_reviewers.get(key_id)
         if key_id in revoked_key_ids:
@@ -174,21 +227,24 @@ def verify_catalog_change(
             raise CatalogApprovalMismatch("approval principal or issuer does not match trusted key")
         if reviewer.role is not None and approval["role"] != reviewer.role:
             raise CatalogApprovalMismatch("approval role does not match trusted key")
-        if not isinstance(approval["approved_at"], int) or not isinstance(approval["expires_at"], int) or approval["expires_at"] <= approval["approved_at"]:
+        approved_at = _require_int(approval["approved_at"], "approval.approved_at")
+        if _require_int(approval["expires_at"], "approval.expires_at") <= approved_at:
             raise CatalogApprovalError("approval validity interval is invalid")
         if instant < approval["approved_at"] or instant >= approval["expires_at"]:
             raise CatalogApprovalMismatch("approval is not currently valid")
+        signature = _decode(approval["signature"])
         try:
-            reviewer.key.verify(_decode(approval["signature"]), _approval_input(record, approval))
-        except (InvalidSignature, ValueError) as exc:
+            reviewer.key.verify(signature, _approval_input(record, approval))
+        except InvalidSignature as exc:
             raise CatalogApprovalMismatch("approval signature is invalid") from exc
+        if policy["distinct_principals"] and approval["principal_id"] in principals:
+            raise CatalogApprovalMismatch("approval set repeats a principal under a distinct-principal policy")
+        if policy["distinct_roles"] and approval["role"] in roles:
+            raise CatalogApprovalMismatch("approval set repeats a role under a distinct-role policy")
+        if key_id in keys_used:
+            raise CatalogApprovalMismatch("approval set reuses a reviewer key")
         valid += 1
         principals.add(approval["principal_id"])
         roles.add(approval["role"])
-    if policy["distinct_principals"] and len(principals) < threshold:
-        raise CatalogApprovalMismatch("approval threshold lacks distinct principals")
-    if policy["distinct_roles"] and len(roles) < threshold:
-        raise CatalogApprovalMismatch("approval threshold lacks distinct roles")
-    if valid < threshold:
-        raise CatalogApprovalMismatch("approval threshold is not satisfied")
+        keys_used.add(key_id)
     return {"verified": True, "valid_approvals": valid, "new_catalog_hash": record["new_catalog_hash"]}