Repository navigation
test(aead-frame): prove the storage reader's pieces with Kani - #531
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
🧬 Mutation testing (cargo-mutants,
|
coderdan
left a comment
There was a problem hiding this comment.
Code review (high effort): 10 findings, most serious first. Each is inline.
The harnesses are correct for what they assert. The findings are places where they prove less than their docs, the README or the workflow comment say, plus CI trigger and runtime costs. I'll fix them on this branch before pushing.
✅ No CRAP threshold violations1496 function(s) analyzed · threshold 30 |
61f5537 to
87b5fd9
Compare
671889d to
fb0da3c
Compare
87b5fd9 to
a2627c6
Compare
fb0da3c to
8220c06
Compare
… transport changes serde.rs defines the storage profile's SOME and MAP tags and builds body()'s errors, both of which the harnesses check, so kani-storage.yml now triggers on all of packages/aead-frame/src. The mutants exclusion's comment names both Kani workflows. Raised in review on #531. Claude-Session: https://claude.ai/code/session_01EnoQSYdggmUYkqNyahDchE
a2627c6 to
09a2ab3
Compare
freshtonic
left a comment
There was a problem hiding this comment.
Comment only. I reviewed against #524. The header, count and integer-identity harnesses are correct, and the value oracle handles sign extension and the u128 > i128::MAX case. Dropping the whole-body validate harness is reasonable, given the solver times you found. I am not approving yet, because some harnesses claim properties that they do not prove (the first two items below). For a PR whose purpose is the proofs, I think we must fix that before merge.
Trigger paths. 09a2ab3 adds packages/aead-frame/src/**, which covers serde.rs and lib.rs. packages/protected/** is still not in the trigger, and --features serde compiles it into the code under proof (Protected, Zeroizing). Add packages/protected/src/** and packages/protected/Cargo.toml.
Some harnesses pass for an implementation that refuses everything.
reading_an_object_key_stays_in_bounds,reading_a_map_key_admits_only_its_kindsandreading_a_scalar_stays_in_boundsassert only insideif let Ok. Akey()ormap_key()that always returnedErrwould pass all three. Add a check that a valid key or scalar is accepted.- The key harness does not check that the key length equals the u32 at
pos. - The map-key harness asserts
KEYS.contains(..), which is the check thatmap_keymakes. Compare against a tag list taken fromSTORAGE.md. Also, assert that f64 is refused as a key in the wide-scalar harness.
Depth. Only the some call site is shown to pass depth + 1. If the array, object or map site passed depth, recursion would have no bound and every harness would still pass. Concrete checks, each with a container at depth 128 that holds a null and is refused, would close this cheaply. Also, the_depth_bound_is_128 never asserts MAX_DEPTH == 128, so a change to frame::MAX_DEPTH would leave a proof whose name is wrong.
Smaller items
- With a 6-byte bound and a 4-byte length, text has at most 2 payload bytes. So rule 6 (UTF-8) is not checked on 3- and 4-byte sequences. Say so in the doc, or give one harness a larger concrete length.
- About 60 lines of
kani-storage.ymlcopykani.yml, including theKANI_VERSIONpin and the cache key. So a Kani version bump needs edits in two files, and the two workflows share one cache. cfg(kani)is declared in[lints.rust], butprotectedusesbuild.rs. Both are fine, but the PR description says it followsprotected.- Use
HEADER.len()in place of the bare4in the header harness.
…ay in bounds and keys compare as specified
…epth bound from both sides, wide scalars and same-kind keys
…t it compiles from, two harnesses at a time
… transport changes serde.rs defines the storage profile's SOME and MAP tags and builds body()'s errors, both of which the harnesses check, so kani-storage.yml now triggers on all of packages/aead-frame/src. The mutants exclusion's comment names both Kani workflows. Raised in review on #531. Claude-Session: https://claude.ai/code/session_01EnoQSYdggmUYkqNyahDchE
…ses everything, and prove each container's depth Raised in review on #531: - The scalar, object-key and map-key harnesses now also require a plainly valid input to be accepted, so a reader that refused everything would fail them. The key harness requires exactly the key the u32 length and UTF-8 allow. - A four-byte key is checked against RFC 3629's table, written apart from the standard library, to cover three- and four-byte sequences. - The map-key harness checks the key's tag against the tags STORAGE.md allows, written out, rather than the reader's own list. The wide-scalar harness requires a float to be refused as a key. - MAX_DEPTH is asserted to be 128. A some, an array and a map at 128 holding a null are refused, so each passes its children down a level. An object's hash set asks the OS for random bytes, which Kani cannot model, so the corpus pins its depth. - The header harness uses HEADER.len() rather than a bare 4. Claude-Session: https://claude.ai/code/session_01EnoQSYdggmUYkqNyahDchE
The serde feature compiles Protected and Zeroizing into the code under proof. Raised in review on #531. Claude-Session: https://claude.ai/code/session_01EnoQSYdggmUYkqNyahDchE
8220c06 to
6c6e432
Compare
09a2ab3 to
d57c5db
Compare
|
Thanks, James. These are fixed in b29eb3e and d57c5db:
All 23 harnesses verify locally in 607 s, two at a time. |
…e place kani.yml and kani-storage.yml each carried the same cache, toolchain and install steps and their own KANI_VERSION. They now use .github/actions/setup-kani, which holds the pin and the cache key, and both run when it changes. Raised in review on #531. Claude-Session: https://claude.ai/code/session_01EnoQSYdggmUYkqNyahDchE
Closes #524. Stacked on #530.
What
packages/aead-frame/src/storage/proofs.rsadds 23 Kani harnesses about the storage reader, the code that parses bytes that may be corrupt or tampered with. Each property holds for every input within its bound.the_header_is_refused_exactly_as_its_rules_saybodyis refused asNotABlobexactly when rule 1 says, and asUnsupportedVersionexactly when rule 2 says; otherwise it returns the rest of the input itselfa_count_is_never_more_than_the_bytes_leftCursor::countreturns exactly the u32 at the cursor when it fits in the bytes left after it, and refuses otherwisereading_a_scalar_stays_in_boundsCursor::scalarnever panics; it reads exactlybuf[pos..new_pos], at least one byte, or refuses and leaves the cursor where it was; it accepts a plainly valid scalar (a null, a wholeu8)reading_an_object_key_stays_in_boundsCursor::keynever panics, and reads exactly the key the u32 length at the cursor and valid UTF-8 allowa_four_byte_object_key_is_read_exactly_when_it_is_utf8reading_a_map_key_admits_only_its_kindsCursor::map_keynever panics, accepts only a tag STORAGE.md allows (written out, not the reader's list), read exactly at the cursor, and accepts a wholeu8keyreading_a_wide_scalar_takes_exactly_its_widthi64,u64,f64,i128andu128are read whole exactly when the input holds them, and refused otherwise; the integers also as map keys, andf64neverthe_depth_bound_is_128MAX_DEPTHis 128;nodepast 128 refuses without reading a byte; at 128 a null still readsa_some_passes_its_value_down_a_level,an_array_passes_its_elements_down_a_level,a_map_passes_its_values_down_a_levelsomeand the array read at 127). An object is not provable in Kani: its repeat check'sHashSetasks the OS for random bytes, which Kani can't model; the corpus's depth rows pin itinteger_keys_equal_by_value_from_*(10)Identityexactly when their values are equal, where the value is worked out from the bytes without the leaf codectext_and_bytes_keys_equal_by_their_byteskeys_of_different_kinds_are_never_one_keyIdentity, even with the same bytescargo kani -p vitaminc-aead-frame --features serde -j 2 --output-format terseverifies all 23 in 607 s on an M-series Mac.What isn't proved, and why
The issue's third harness,
validateon any short body, did not solve in a usable time:nodeper recursive call site (array, object, map value, some), each with the leaf codec and the object'sHashSet.node(#[kani::recursion],-Z function-contracts) removes the copies, but still didn't finish in 15 minutes for 4-byte bodies.So, following
protected'slocked/proofs.rs, which documents a whole-path harness it couldn't solve, the proofs cover the pieces a body is read with. Whole bodies stay with the property tests (any_bytes_are_read_alike,a_corrupted_blob_is_read_alike), the Go fuzz target, and the Rust fuzzing follow-up. The module doc records this.Production code only gains
#[cfg(kani)] mod proofs;. The reader is unchanged.CI
There's a new workflow,
.github/workflows/kani-storage.yml("📐 Kani (storage reader)"):packages/aead-frame/src/**, the leaf codec (packages/aead/src/**),vitaminc-protected(packages/protected/src/**), those crates' manifests, the rootCargo.tomlorCargo.lockchange.-j 2withtimeout-minutes: 60.kani.yml(protected) is unchanged, so neither crate's change runs the other's proofs.Also
packages/aead-frame/Cargo.tomldeclarescfg(kani)forcheck-cfgin[lints.rust];protecteddoes the same in itsbuild.rs..cargo/mutants.tomlexcludes the proofs file, as forprotected..cargo-crap.tomlexcludessrc/**/proofs.rs: Kani harnesses are built only undercfg(kani), so coverage always reads them as 0% and the pessimistic CRAP score flagged three of them. This also coversprotected'slocked/proofs.rs, which had passed only because its functions are simple. (61f5537)Notes for review
cargo-kanishim has no version set, so I ran the installed binary directly.