docs(comments): correct three comments that state the opposite of the code (#101) - #136
Merged
Merged
Conversation
… code
CRYPTO-2. `sb_identity_aad`'s doc claims it "binds the sealed body AND
EACH KEY-SLOT'S DEK WRAP to this superblock's plaintext identity". It
binds only the body: the function is passed to `seal_body`/`open_body`
and nowhere else, while every DEK wrap authenticates against
`KeySlot::aad()` — state, kdf_id, Argon2 params, salt, wrap_nonce, none
of which carries superblock identity or generation.
This is the most consequential of the three, because it asserts a
security property whose ABSENCE is the actual weakness: a maintainer
who believes key slots are already pinned to the superblock generation
will not add the binding when it is needed. The doc now states the
scope explicitly and says that binding wraps to the generation means
extending the wrap AAD, not reusing this one.
PAGE-IO-1. page_cache.rs's module header asserts "`next_page_id` is a
monotonic allocator ... Rollback does NOT rewind it (see the note in
`discard`)". `truncate` rewinds it — its own doc says "This is the only
path that legitimately rewinds `next_page_id`" — and `truncate` is
exactly what `rollback` and `rollback_to` call. The `discard` the
header points at is `#[allow(dead_code)]` with no production caller, so
the reader is directed at the one path that is NOT the rollback path.
The consequence is an aliasing trap: allocate page 100, roll back to a
committed total of 90, allocate again, and `new_page` returns 100 for
different content. Anyone trusting the header would skip
cache/spillway/freemap invalidation for the reissued range — precisely
the bug class the header claims is impossible. The header now says
monotonic within a transaction, rewound by `truncate` across one, and
spells out the invalidation obligation.
The old rationale ("two concurrent savepoint rollbacks could hand the
same ID to two different allocations") never applied either: the engine
is deliberately single-threaded and single-client. It is dropped rather
than reworded.
A unit test pins the behaviour — `truncate` rewinds the allocator and
the next `new_page` reissues the id. Verified it fails without the
rewind (left: 6, right: 3) rather than passing vacuously.
PAGE-IO-4. `DataPage::insert` says "the transaction layer calls
PageCache::new_page() for every insert rather than scanning existing
pages for free slots ... this function itself is correct; it's just
underutilized". R1 packing replaced that: `transaction::packing` keeps
an insert cursor and calls `DataPage::insert` on it for every value,
allocating a new page only when the cursor fills or a savepoint
disables packing. Calling the slot-directory append path
"underutilized" invites deleting machinery that now runs for every
non-first insert in a transaction.
Closes #101.
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
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.
Closes #101 (3 findings, DESIGN/docs).
CRYPTO-2 — a comment asserting the security property whose absence is the weakness
sb_identity_aad's doc claims it binds "the sealed body and each key-slot'sDEK wrap" to the superblock's plaintext identity.
It binds only the body. The function is passed to
seal_body/open_bodyandnowhere else; every DEK wrap authenticates against
KeySlot::aad(), whichcontains state, kdf_id, Argon2 params, salt and wrap_nonce — no magic, no
format version, no
txn_counter, nosuperblock_count.This is the most consequential of the three. A maintainer reading it believes
the key-slot table is already cryptographically pinned to the superblock
generation it was written in, and so won't add that binding when it's needed —
which is exactly what makes an old key-slot table spliced into a different
generation still authenticate. The comment asserts the property whose absence
is the real gap.
The doc now states the scope explicitly and notes that binding wraps to the
superblock generation means extending the wrap AAD, not reusing this one.
(Related, and reassuring:
KeySlot::aad()does coverkdf_id, so an attackercannot flip a slot from Argon2id to HKDF on disk to strip the passphrase work
factor — the unwrap would fail authentication. Worth stating since the
corrected comment now draws attention to what the slot AAD does and doesn't
cover.)
PAGE-IO-1 — the header documents an anti-aliasing invariant the code does not provide
page_cache.rs's module header asserts:truncaterewinds it — its own doc says "This is the only path thatlegitimately rewinds
next_page_id" — andtruncateis whatrollbackandrollback_tocall. Thediscardthe header points at is#[allow(dead_code)]with no production caller, so the reader is directed atthe one path that is not the rollback path.
The consequence is an aliasing trap: allocate page 100, roll back to a
committed total of 90, allocate again →
new_pagereturns 100, now fordifferent content. Anyone trusting the header would skip cache / spillway /
freemap invalidation for the reissued range — precisely the bug class the
header claims is impossible.
The old rationale — "two concurrent savepoint rollbacks could hand the same ID
to two different allocations" — never applied either: the engine is
deliberately single-threaded and single-client. Dropped rather than reworded.
New test —
truncate_rewinds_the_allocator_so_ids_are_reissuedpins bothhalves:
truncaterewinds, and the nextnew_pagehands the id back out.Counterfactual, with the rewind removed:
PAGE-IO-4 — "underutilized" machinery that every insert now depends on
DataPage::insertcarries a note that "the transaction layer callsPageCache::new_page() for every insert rather than scanning existing pages for
free slots … this function itself is correct; it's just underutilized."
R1 packing replaced that.
transaction::packingkeeps an insert cursor — a pageallocated earlier in the same transaction that still has room — and calls
DataPage::inserton it for every value, allocating a fresh page only when thecursor fills or a savepoint disables packing.
Calling the slot-directory append path "underutilized" is an invitation to
simplify or delete machinery that now runs for every non-first insert in a
transaction, and it mis-sets expectations for anyone debugging page occupancy —
they'd look for one page per value and not find it.
Verification
cargo testall green,cargo clippy --workspace --all-targets -- -D warningsclean,
cargo fmt --checkclean.