From d36bdad015a2f13c3c524233adc42818a638516c Mon Sep 17 00:00:00 2001 From: Christophe Pettus Date: Wed, 1 Jul 2026 12:04:58 -0700 Subject: [PATCH] docs(comments): fix 10 stale/wrong comments on the encryption surface MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An adversarial commenting pass over the on-disk encryption code (merged in #85). Comments only — no behavior change; full test suite, clippy, and fmt stay green. Wrong facts: - crypto/mod.rs: derive_kek's `# Errors` said BadKeyLength fires only for `kdf == Hkdf` with empty raw bytes; the empty-key check is unconditional (before the KDF match) and covers Raw and Passphrase. - superblock/mod.rs (x2): sealed-superblock comments said sensitive scalars 16..52 are zeroed, but page_size is cleartext at 48..52 — the zeroed range is 16..48. - python/src/db.rs: key coercion claimed an empty/bad raw key raises BadKeyLength via to_py_err; every CryptoError maps to InvalidEncryptionKey -> InvalidEncryptionKeyError (there is no BadKeyLength Python error). - transaction/recovery.rs: "Slot 0 was written last" — it is written first (i=0, highest counter superblock_count-1). - transaction/keys.rs (test): comment said txn_counter=3 after fresh_encrypted; it is 2. Stale: - page_io.rs: header still said "Two fsyncs per commit"; it is three (I28 pre-drain + data + superblock) — the header #84 missed. - transaction/commit.rs + lifecycle.rs: CommitCtx described as "ten pieces" of state; the cipher + crypto_header fields make it twelve. - python/src/errors.rs: exception-hierarchy comment omitted DecryptionFailedError from the FatalError tier. Three further candidates could not be adversarially verified (transient rate limit) and were left as-is rather than rewritten on an unverified claim. --- python/src/db.rs | 5 +++-- python/src/errors.rs | 1 + src/crypto/mod.rs | 3 ++- src/page_io.rs | 3 ++- src/superblock/mod.rs | 8 +++++--- src/transaction/commit.rs | 2 +- src/transaction/keys.rs | 4 ++-- src/transaction/lifecycle.rs | 2 +- src/transaction/recovery.rs | 2 +- 9 files changed, 18 insertions(+), 12 deletions(-) diff --git a/python/src/db.rs b/python/src/db.rs index ccb6c5a..15ddc02 100644 --- a/python/src/db.rs +++ b/python/src/db.rs @@ -50,8 +50,9 @@ use chisel::Chisel; use crate::errors::to_py_err; /// Map a Python key argument to a `chisel::Key`. `bytes` → `Key::Raw` (any -/// length; the engine validates the length and raises BadKeyLength via -/// to_py_err if it is wrong). `str` → `Key::Passphrase`. Anything else raises +/// non-empty length is accepted; empty key material is refused deep in the +/// engine and surfaces as InvalidEncryptionKeyError via to_py_err). `str` → +/// `Key::Passphrase`. Anything else raises /// a Python `TypeError`. Key material is wrapped in `Zeroizing` immediately so /// it is scrubbed when the `Key` is dropped; we never log or repr the value. /// diff --git a/python/src/errors.rs b/python/src/errors.rs index 3684faa..7698cf7 100644 --- a/python/src/errors.rs +++ b/python/src/errors.rs @@ -47,6 +47,7 @@ // not match the handle's stored tag) // FatalError (drop-and-reopen recovery only) // IoError (ALSO subclasses builtin OSError — see register) +// DecryptionFailedError (encrypted page/superblock failed AEAD auth) // ChecksumMismatchError // CorruptSuperblockError // FileSizeMismatchError diff --git a/src/crypto/mod.rs b/src/crypto/mod.rs index afa6496..f4c8c92 100644 --- a/src/crypto/mod.rs +++ b/src/crypto/mod.rs @@ -152,7 +152,8 @@ const KEK_INFO: &[u8] = b"chisel-kek-v1"; /// # Errors /// Returns `CryptoError::Kdf` if the KDF primitive rejects its parameters /// (e.g. Argon2id with zero memory cost). Returns `CryptoError::BadKeyLength` -/// if `kdf == Hkdf` and the raw key bytes are empty. +/// if the supplied key material is empty (an empty `Raw` key or empty +/// `Passphrase`), regardless of `kdf`. pub fn derive_kek( key: &Key, kdf: KdfId, diff --git a/src/page_io.rs b/src/page_io.rs index 33b58a1..052eb43 100644 --- a/src/page_io.rs +++ b/src/page_io.rs @@ -10,7 +10,8 @@ // Two backings, one interface: // - `Backing::File` — the durable path. Owns a `File` handle for its entire // lifetime; the advisory flock is tied to that fd and released on drop. -// Two fsyncs per commit; shadow paging guarantees crash consistency. +// Three fsyncs per commit (I28 pre-drain, data pages, superblock); shadow +// paging guarantees crash consistency. // - `Backing::Memory` — the ephemeral path. Pages live in a flat `Vec` // addressed by `page_id * stride`; fsync is a no-op; no flock is taken. // Used for benchmark parity with SQLite `:memory:` — see the in-memory-mode diff --git a/src/superblock/mod.rs b/src/superblock/mod.rs index 6c99085..7fa9f57 100644 --- a/src/superblock/mod.rs +++ b/src/superblock/mod.rs @@ -381,7 +381,8 @@ impl Superblock { /// Serialize an encrypted superblock: bootstrap fields + crypto-header in /// cleartext; sensitive fields sealed under the DEK. The byte ranges that /// would hold sensitive data in a plaintext page are left ZERO so nothing - /// leaks (named_roots at 52..308, root/page-id scalars at 16..52, etc.). + /// leaks (named_roots at 52..308, root/page-id scalars at 16..48, etc.; + /// page_size at 48..52 stays cleartext). /// /// Panics if `self.encryption` is `None` — only call for encrypted DBs. pub fn serialize_encrypted(&self, cipher: &crate::crypto::PageCipher) -> [u8; PAGE_SIZE] { @@ -390,8 +391,9 @@ impl Superblock { .as_ref() .expect("serialize_encrypted requires Superblock.encryption = Some"); let mut buf = [0u8; PAGE_SIZE]; - // Plaintext bootstrap fields only. Sensitive scalar fields (16..52) - // and named_roots (52..308) are intentionally left zero. + // Plaintext bootstrap fields only. Sensitive scalar fields (16..48) + // and named_roots (52..308) are intentionally left zero (page_size at + // 48..52 is a cleartext bootstrap field, written just below). buf[0..4].copy_from_slice(&self.magic.to_le_bytes()); buf[4..8].copy_from_slice(&self.format_version.to_le_bytes()); buf[8..16].copy_from_slice(&self.txn_counter.to_le_bytes()); diff --git a/src/transaction/commit.rs b/src/transaction/commit.rs index fc73dd7..7c151ad 100644 --- a/src/transaction/commit.rs +++ b/src/transaction/commit.rs @@ -7,7 +7,7 @@ use super::*; -/// Borrows of the exactly-ten pieces of `TransactionManager` state the commit +/// Borrows of the exactly-twelve pieces of `TransactionManager` state the commit /// protocol touches, bundled so `commit_inner` can stay a thin caller while the /// load-bearing sequence lives here. All fields are distinct manager fields (plus /// the shared `&RefCell` for the cache), so the borrow checker accepts the diff --git a/src/transaction/keys.rs b/src/transaction/keys.rs index dfa7d0b..908e0d5 100644 --- a/src/transaction/keys.rs +++ b/src/transaction/keys.rs @@ -373,8 +373,8 @@ mod tests { #[test] fn rewrite_alternates_superblock_slots() { let mut db = fresh_encrypted(); - // After fresh_encrypted: one create + one data commit = txn_counter=3 - // (create writes N=2 initial slots + one commit). The next write targets + // After fresh_encrypted: create leaves txn_counter = superblock_count-1 + // = 1 (N=2), then one data commit bumps it to 2. The next write targets // txn_counter % 2. // CryptoHeader is Copy so we can just use the value twice. let hdr: CryptoHeader = db.crypto_header.unwrap(); diff --git a/src/transaction/lifecycle.rs b/src/transaction/lifecycle.rs index ebd9ea6..2b41e3b 100644 --- a/src/transaction/lifecycle.rs +++ b/src/transaction/lifecycle.rs @@ -200,7 +200,7 @@ impl TransactionManager { fn commit_inner(&mut self) -> Result<()> { // The 3-fsync commit protocol lives in `commit::run_commit`, operating - // over the ten pieces of manager state it touches via `CommitCtx`. The + // over the twelve pieces of manager state it touches via `CommitCtx`. The // ordering there is load-bearing (see the step-by-step rationale on // `commit()` above and in `commit.rs`); this stays a thin caller. All // `&mut self.` borrows are distinct fields and `&self.cache` is a diff --git a/src/transaction/recovery.rs b/src/transaction/recovery.rs index 74cf42e..42d43c3 100644 --- a/src/transaction/recovery.rs +++ b/src/transaction/recovery.rs @@ -146,7 +146,7 @@ impl TransactionManager { current_roots: roots, handle_table: HandleTable::new(), membership_index: MembershipIndex::new(), - // Slot 0 was written last in the loop above, at counter + // Slot 0 was written first in the loop above (i=0), at counter // (superblock_count - 1 - 0) = superblock_count - 1. That's // the highest counter and therefore the winner on select(). txn_counter: (superblock_count - 1) as u64,