From 1314dbbaa5d7ec0dea184b0e5100dca737ee11aa Mon Sep 17 00:00:00 2001 From: Christophe Pettus Date: Wed, 1 Jul 2026 13:12:51 -0700 Subject: [PATCH] docs(comments): add 5 why-comments to the encryption code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Constructive commenting pass (/comment-run) over the on-disk encryption surface merged in #85 — the complement to the wrong-comment fixes in #87. Additive only: ADD/EXTEND, no existing comment text deleted or rewritten. Comments only; full test suite, clippy, and fmt stay green. Each captures a non-obvious "we do X because Y" a first-time reader could not infer from the code: - crypto/mod.rs: Argon2id's Version::V0x13 and 32-byte output are pinned to the on-disk format (like KEK_INFO for HKDF) — changing either silently breaks unwrap of every existing Argon2id slot. - superblock/mod.rs: the four bootstrap fields must stay cleartext because they ARE the AAD, and slot selection + AAD derivation run before any DEK is available (a sealed body would be a chicken-and-egg deadlock). - page_cache.rs: the encrypted cold-load DecryptionFailed is fatal/poisoning, a peer of ChecksumMismatch, not a retryable operational error. - page_io.rs: set_stride swallows a seek error to 0 because its signature is infallible (bootstrap call site) and 0 makes reads fail closed. - spillway.rs: spill writes are deliberately not fsynced — content never crosses a transaction boundary, so durability would be wasted I/O. The pass found 0 wrong comments (fixed in #87) and 0 bugs. Six of the eleven modules reviewed needed nothing added — the encryption code was already well-commented from its per-task review gates. --- src/crypto/mod.rs | 5 +++++ src/page_cache.rs | 6 ++++++ src/page_io.rs | 4 ++++ src/spillway.rs | 5 +++++ src/superblock/mod.rs | 6 ++++++ 5 files changed, 26 insertions(+) diff --git a/src/crypto/mod.rs b/src/crypto/mod.rs index afa6496..b7c5624 100644 --- a/src/crypto/mod.rs +++ b/src/crypto/mod.rs @@ -183,6 +183,11 @@ pub fn derive_kek( KdfId::Argon2id => { let p = Params::new(params.m_cost, params.t_cost, params.p_cost, Some(32)) .map_err(|_| CryptoError::Kdf)?; + // Version::V0x13 and the 32-byte output length (`Some(32)` above) are + // pinned to the on-disk format, exactly like KEK_INFO on the HKDF path: + // changing either re-derives a different KEK, so every existing Argon2id + // slot would stop unwrapping (silent data loss). Bump the format version + // deliberately if this ever changes; the argon2id KAT test pins it. let a2 = Argon2::new(Algorithm::Argon2id, Version::V0x13, p); a2.hash_password_into(ikm, salt, okm.as_mut()) .map_err(|_| CryptoError::Kdf)?; diff --git a/src/page_cache.rs b/src/page_cache.rs index e81a2dd..01f7f97 100644 --- a/src/page_cache.rs +++ b/src/page_cache.rs @@ -955,6 +955,12 @@ impl PageCache { /// A checksum mismatch is a fatal corruption error per ARCHITECTURE.md — /// `ChecksumMismatch` signals the database is broken, not merely that /// the operation failed. + /// + /// The encrypted cold-load path has the same severity: a `DecryptionFailed` + /// from `PageCipher::open` (AEAD tag rejection) is `is_fatal()` — it poisons + /// the manager exactly like `ChecksumMismatch`, not an operational error the + /// caller can retry. The XXH3 checksum and the AEAD tag are peer integrity + /// checks: either failing means the persisted bytes are untrustworthy. fn load_page(&mut self, page_id: u64) -> Result<()> { self.maybe_evict()?; diff --git a/src/page_io.rs b/src/page_io.rs index 33b58a1..9fb7a45 100644 --- a/src/page_io.rs +++ b/src/page_io.rs @@ -215,6 +215,10 @@ impl PageIo { pub fn set_stride(&mut self, stride: usize) { self.stride = stride; let len = match &mut self.backing { + // Infallible by signature (the encrypted-open bootstrap has no Result + // to thread into): a seek failure means a broken fd, already fatal under + // the single-writer flock. Falling back to 0 makes reads fail closed + // (InvalidPageId) instead of computing offsets against a stale count. Backing::File { file } => file.seek(SeekFrom::End(0)).unwrap_or(0), Backing::Memory { bytes } => bytes.len() as u64, }; diff --git a/src/spillway.rs b/src/spillway.rs index a37013c..bcaf261 100644 --- a/src/spillway.rs +++ b/src/spillway.rs @@ -11,6 +11,11 @@ // process and unconditionally discarded. // spill page_id allocates a slot (or overwrites its existing // one), bytes + per-slot checksum are written. +// Writes are deliberately NOT fsynced: spillway content never +// crosses a transaction boundary (rebuilt on demand, discarded at +// `truncate` on commit/rollback and as crash garbage at `open`), so +// a durability barrier would be wasted I/O. The per-slot XXH3 still +// guards a torn write; durability is intentionally omitted. // rehydrate slot is read, checksum verified, bytes returned. // truncate file shrunk to zero, resident-set index cleared. Called // at commit, rollback, and defrag. diff --git a/src/superblock/mod.rs b/src/superblock/mod.rs index 6c99085..9fb117e 100644 --- a/src/superblock/mod.rs +++ b/src/superblock/mod.rs @@ -325,6 +325,12 @@ impl Superblock { /// this superblock's plaintext identity. The four bootstrap fields that stay /// cleartext in both encrypted and plaintext DBs are included; this prevents /// transplanting a sealed body from a different DB or a different txn_counter. + /// + /// These four MUST stay cleartext even in an encrypted DB precisely because + /// they are the AAD: slot selection (`max_by_key` on `txn_counter`) and this + /// AAD derivation both run BEFORE any DEK is available, so the fields cannot + /// live in the DEK-sealed body — the engine must read them to pick the live + /// slot and rebuild the AAD before it can open that body. pub fn sb_identity_aad(&self) -> [u8; 24] { let mut a = [0u8; 24]; a[0..4].copy_from_slice(&self.magic.to_le_bytes());