From 10dfdd1172a33479d81ba11253235217e231f17b Mon Sep 17 00:00:00 2001 From: Vitaly Reznik Date: Sat, 19 Sep 2026 22:37:20 +0300 Subject: [PATCH] replay: fail closed when the pack carries no usable binding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `veip-verify replay pack.json` could report PASS on a tampered pack. execution.outcome.result_ref is typed in the schema as {"type": "string", "minLength": 1}, so any string validates. _extract_binding_from_pack() returns None for a string that is not sha256:<64 hex>, verify_integrity_binding() defaulted to require_binding=False, and the CLI only set it True with --require-binding. So overwriting the carrier — the very field that protects the pack — turned tamper detection off: before: $ veip-verify replay pack.json exit=0 PASS replay: no binding present (not required) after: $ veip-verify replay pack.json exit=2 FAIL replay: binding carrier is not a sha256 digest: execution.outcome.result_ref = 'ref://opaque-store/result-1' after: $ veip-verify replay pack.json --allow-missing-binding exit=0 PASS replay: NOT VERIFIED: binding carrier is not a sha256 digest: ... The pack in all three runs is schema-valid, has one field changed (policy.policy_version) and its result_ref rewritten to a plausible opaque reference. Changes, all inside the verifier — the schema and the protocol are untouched: * CLI defaults to requiring the binding; --allow-missing-binding is the explicit opt-out. --require-binding is still accepted as a no-op so existing scripts keep working. * An absent carrier and a malformed one are now reported differently. Reporting a rewritten carrier as "no binding present" hid the difference between a pack that never claimed a binding and one whose binding was overwritten. * When the check is skipped, the reason starts with NOT VERIFIED, because that string is what a reader sees next to PASS. * README documents the default and the opt-out. This makes the code match what the README already states the tool does: "Integrity binding verification (hash verification / tamper detection)". tests/test_binding_carrier.py fails on the unpatched code (3 of its 5 cases) and passes with this change; the existing suite passes unchanged. Co-Authored-By: Claude Opus 5 --- README.md | 3 ++ tests/test_binding_carrier.py | 87 +++++++++++++++++++++++++++++++++++ veip_verifier_core/cli.py | 18 +++++++- veip_verifier_core/replay.py | 50 +++++++++++++++----- 4 files changed, 144 insertions(+), 14 deletions(-) create mode 100644 tests/test_binding_carrier.py diff --git a/README.md b/README.md index dba46c7..41efaf5 100644 --- a/README.md +++ b/README.md @@ -121,6 +121,9 @@ Initial CLI version: `v0.1.0` * `veip-verify replay ` Canonicalize the Evidence Pack and verify integrity bindings. + Fails when the pack carries no usable binding, since a run that checks no + binding detects no tampering. Pass `--allow-missing-binding` to accept such a + pack anyway; the PASS line then says `NOT VERIFIED` and names why. * `veip-verify schema` Print schema path and schema SHA256 fingerprint. diff --git a/tests/test_binding_carrier.py b/tests/test_binding_carrier.py new file mode 100644 index 0000000..0150df9 --- /dev/null +++ b/tests/test_binding_carrier.py @@ -0,0 +1,87 @@ +"""Regression tests: a rewritten binding carrier must not read as a clean PASS. + +The schema types execution.outcome.result_ref as {"type": "string", +"minLength": 1}, so a pack may carry any string there and still validate. +Before this test existed, a pack could be tampered with and its carrier +rewritten to a non-sha256 string, and `veip-verify replay pack.json` printed +"PASS replay: no binding present (not required)" and exited 0. +""" + +from __future__ import annotations + +import json +import subprocess +import sys +from copy import deepcopy +from pathlib import Path + +from veip_verifier_core.replay import verify_integrity_binding +from veip_verifier_core.schema import validate_evidence_pack + +ROOT = Path(__file__).resolve().parents[1] +FIXTURE = ROOT / "tests" / "fixture_pack.json" + + +def run_cmd(*args: str) -> subprocess.CompletedProcess: + return subprocess.run( + [sys.executable, "-m", "veip_verifier_core.cli", *args], + text=True, + capture_output=True, + cwd=str(ROOT), + ) + + +def _tampered_with_rewritten_carrier(tmp_path: Path) -> Path: + """A schema-valid pack: one field changed, carrier overwritten with a + plausible opaque reference instead of the digest.""" + pack = json.loads(FIXTURE.read_text(encoding="utf-8")) + pack["policy"]["policy_version"] = pack["policy"]["policy_version"] + "-tampered" + pack["execution"]["outcome"]["result_ref"] = "ref://opaque-store/result-1" + + validate_evidence_pack(pack) # still schema-valid — that is the point + p = tmp_path / "rewritten_carrier.json" + p.write_text(json.dumps(pack), encoding="utf-8") + return p + + +def test_rewritten_carrier_fails_by_default(tmp_path: Path): + p = run_cmd("replay", str(_tampered_with_rewritten_carrier(tmp_path))) + assert p.returncode == 2, p.stdout + p.stderr + assert "not a sha256 digest" in p.stderr + + +def test_rewritten_carrier_opt_out_does_not_claim_verification(tmp_path: Path): + p = run_cmd( + "replay", + str(_tampered_with_rewritten_carrier(tmp_path)), + "--allow-missing-binding", + ) + assert p.returncode == 0, p.stdout + p.stderr + # PASS is allowed here, but it must not read as "this pack is untampered". + assert "NOT VERIFIED" in p.stdout + + +def test_malformed_carrier_is_distinguished_from_an_absent_one(tmp_path: Path): + pack = json.loads(FIXTURE.read_text(encoding="utf-8")) + pack["execution"]["outcome"]["result_ref"] = "ref://opaque-store/result-1" + malformed = verify_integrity_binding(pack, require_binding=True) + + absent = deepcopy(pack) + del absent["execution"]["outcome"]["result_ref"] + missing = verify_integrity_binding(absent, require_binding=True) + + assert malformed.ok is False and missing.ok is False + assert malformed.reason != missing.reason + assert "not a sha256 digest" in malformed.reason + assert "missing binding" in missing.reason + + +def test_intact_fixture_still_passes_under_the_stricter_default(): + p = run_cmd("replay", str(FIXTURE)) + assert p.returncode == 0, p.stdout + p.stderr + assert "binding matches" in p.stdout + + +def test_require_binding_flag_is_still_accepted(): + p = run_cmd("replay", str(FIXTURE), "--require-binding") + assert p.returncode == 0, p.stdout + p.stderr diff --git a/veip_verifier_core/cli.py b/veip_verifier_core/cli.py index 1c538c6..622d686 100644 --- a/veip_verifier_core/cli.py +++ b/veip_verifier_core/cli.py @@ -133,7 +133,15 @@ def build_parser() -> argparse.ArgumentParser: sp_replay.add_argument( "--require-binding", action="store_true", - help="Fail if the binding carrier is missing (execution.outcome.result_ref sha256:).", + help="No-op: this is now the default. Accepted so existing scripts keep working.", + ) + sp_replay.add_argument( + "--allow-missing-binding", + action="store_true", + help=( + "Do not fail when the pack carries no usable binding. PASS then means " + "'nothing was checked', not 'untampered' — see the printed reason." + ), ) return p @@ -149,7 +157,13 @@ def main(argv: Optional[list[str]] = None) -> int: return cmd_validate(args.path, quiet=bool(args.quiet)) if args.command == "replay": - return cmd_replay(args.path, require_binding=bool(args.require_binding), quiet=bool(args.quiet)) + # Fail closed: a verifier whose default run can silently skip tamper + # detection does not verify. Opt out explicitly with --allow-missing-binding. + return cmd_replay( + args.path, + require_binding=not bool(args.allow_missing_binding), + quiet=bool(args.quiet), + ) eprint("ERROR: unknown command") return EXIT_ERROR diff --git a/veip_verifier_core/replay.py b/veip_verifier_core/replay.py index fffe64e..aa217bd 100644 --- a/veip_verifier_core/replay.py +++ b/veip_verifier_core/replay.py @@ -28,10 +28,15 @@ def sha256_hex(data: bytes) -> str: return hashlib.sha256(data).hexdigest() -def _extract_binding_from_pack(evidence_pack: Dict[str, Any]) -> Optional[str]: +def _raw_binding_carrier(evidence_pack: Dict[str, Any]) -> Optional[str]: """ - Carrier for the binding digest (schema-allowed): - evidence_pack.execution.outcome.result_ref = "sha256:<64-hex>" + The carrier field exactly as the pack carries it, whatever its shape. + None only when the field is absent or is not a string. + + Needed to tell two different situations apart, because the schema types + result_ref as {"type": "string", "minLength": 1} and so admits both: + - the carrier is absent -> nothing claimed to bind + - the carrier is present but is not a sha256 digest """ exe = evidence_pack.get("execution") if not isinstance(exe, dict): @@ -40,7 +45,16 @@ def _extract_binding_from_pack(evidence_pack: Dict[str, Any]) -> Optional[str]: if not isinstance(out, dict): return None rr = out.get("result_ref") - if not isinstance(rr, str): + return rr if isinstance(rr, str) else None + + +def _extract_binding_from_pack(evidence_pack: Dict[str, Any]) -> Optional[str]: + """ + Carrier for the binding digest (schema-allowed): + evidence_pack.execution.outcome.result_ref = "sha256:<64-hex>" + """ + rr = _raw_binding_carrier(evidence_pack) + if rr is None: return None if rr.startswith("sha256:") and len(rr) == len("sha256:") + 64: return rr.split("sha256:", 1)[1] @@ -77,18 +91,30 @@ def compute_integrity_binding(evidence_pack: Dict[str, Any]) -> str: def verify_integrity_binding(evidence_pack: Dict[str, Any], require_binding: bool = False) -> IntegrityResult: """ Verify that the pack's stored binding matches the computed binding. - If require_binding=True and no binding is present, verification FAILS. + If require_binding=True and no usable binding is present, verification FAILS. + + Note for callers: with require_binding=False an ok=True result does NOT mean + the pack was found untampered — it means no binding was checked. The CLI now + passes require_binding=True by default for exactly this reason. """ got = _extract_binding_from_pack(evidence_pack) if got is None: - if require_binding: - return IntegrityResult( - False, - "missing binding (execution.outcome.result_ref sha256:)", - expected=None, - got=None, + raw = _raw_binding_carrier(evidence_pack) + if raw is None: + detail = "missing binding (execution.outcome.result_ref sha256:)" + else: + # Say which of the two it is. Reporting a malformed carrier as simply + # "no binding present" hides the difference between a pack that never + # claimed a binding and a pack whose binding carrier was overwritten. + detail = ( + "binding carrier is not a sha256 digest: " + f"execution.outcome.result_ref = {raw[:64]!r}" ) - return IntegrityResult(True, "no binding present (not required)", expected=None, got=None) + if require_binding: + return IntegrityResult(False, detail, expected=None, got=None) + # ok=True here means only "not required", never "verified" — name that in + # the reason, because this string is what a reader sees next to PASS. + return IntegrityResult(True, f"NOT VERIFIED: {detail}", expected=None, got=None) expected = compute_integrity_binding(evidence_pack) if got == expected: