fix(provenance): domain-separated length-prefixed hash; full-field coverage#88
Merged
Merged
Conversation
Closes #27, closes #28, closes #29, closes #30. `ProvenanceEntry::compute_hash` was hashing `previous_hash + entity_id + operation + timestamp` as raw byte concatenation: 1. Three fields — `actor`, `before_snapshot`, `transformation` — were *not* in the preimage. Tampering with any of them left `verify()` returning `true`. The integration test `test_provenance_chain_integrity_multi_step` *codified the hole* (#30 / V-L2-C4): "Actor is not part of hash — tamper to actor alone is invisible". 2. No domain separation. Hash bytes were concatenated without length prefixes or a domain tag, so a future field reorder / addition / removal could silently produce a colliding digest for distinct inputs. No version tag either, so a migration to a different hash algorithm had no way to mark old vs new entries. 3. Timestamp encoded as `to_rfc3339()`. Two valid RFC3339 strings for the same instant (sub-second precision, `Z` vs `+00:00`, etc.) produce two different hashes. This commit lands all four V-L2-C* fixes together because they necessarily ship as one change: - **#27 / V-L2-C1** — new `compute_hash` signature accepts all seven preimage fields and prepends `b"verisim-prov-v1\0"` as the domain tag. Variable-length fields are length-prefixed with `u64_le(len)` for canonical encoding. - **#28 / V-L2-C2** — timestamp encoded as `i64_le(secs) || u32_le(nanos)` via `chrono::DateTime::timestamp()` + `.timestamp_subsec_nanos()`. Different RFC3339 strings for the same instant now produce the same hash. RFC3339 is kept for display surface (JSON, status output) but is no longer part of the hash preimage. - **#29 / V-L2-C3** — five new tests in `abi::tests`: `test_provenance_tamper_actor`, `test_provenance_tamper_before_snapshot`, `test_provenance_tamper_transformation`, `test_provenance_timestamp_canonical_encoding`, `test_provenance_mutation_matrix_breaks_verification` (4-entry chain × 7 fields × every entry, asserts every single mutation breaks `verify()`). - **#30 / V-L2-C4** — the wontfix assertion in `tests/integration_test.rs` is flipped to assert `!tampered.verify()` with the updated comment. API note: `compute_hash` signature changed (4 args → 7 args). The function is `pub`, so this is a breaking change. It's a security fix and the previous signature was wrong by construction; the callers inside the crate are updated. No external callers known. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Lands all four V-L2-C* provenance-hash fixes together (they necessarily ship as one change):
lp(x) = u64_le(len(x)) || x. All seven fields now participate.i64_le(secs) || u32_le(nanos)(12 bytes). Two valid RFC3339 strings for the same instant now produce the same hash. RFC3339 still used for display.abi::tests(actor/before_snapshot/transformation tamper tests + canonical timestamp test + 4-entry × 7-field mutation matrix).tests/integration_test.rs::test_provenance_chain_integrity_multi_stepflipped fromassert!(tampered.verify(), "Actor is not part of hash…")toassert!(!tampered.verify(), "Tampering with actor must break verification").API note:
compute_hashsignature changes (4 args → 7 args). It'spubso technically breaking, but it was wrong by construction and no external callers exist.Closes
Test plan
cargo clippy --all-targets -- -D warningscleancargo test— abi unit tests + tests/integration_test.rs passThe five new tests + the flipped wontfix assertion give a 4×7 mutation matrix covering every (entry, field) combination across the chain.