feat(storage): encrypt objects at rest when an operator supplies a key - #109
Merged
Conversation
BryanFRD
enabled auto-merge (squash)
August 16, 2026 18:16
There was a problem hiding this comment.
Reviewed the full diff (codec.rs, crypt.rs, storage/mod.rs, rewrite.rs, chart, config/error wiring, and the new test suites).
The design holds up under adversarial reading:
- oid used in AAD is always the caller's expected/path-derived oid, never read off the file — so the "move one file onto another" attack is correctly caught by an AAD mismatch (
Error::Tampered), not silently swallowed. - Per-frame
(index, last, oid)binding correctly rejects reorder/truncation/move; traced through thelast-flag math (index+1 == frames) and it lines up with whatseal/openwere called with at write time. - Per-object key derivation (master + random 16-byte salt via blake3
derive_key) means the nonce only ever needs to be a frame counter — no birthday-bound nonce reuse across objects, confirmed bytwo_objects_under_one_key_never_share_a_keystream. - Frame-flush logic changed from
>=to>so "is this the last frame" is knowable before sealing — checked the boundary cases (exact multiple of FRAME, empty object) and they still produce the same frame counts as before. - Backward/forward compat (unkeyed objects still read, keyed store still serves both formats side by side, bucket mode explicitly disclaims both compression and encryption) all have direct tests.
Nits (non-blocking):
Keyring/ObjectKeyhold raw key bytes in a plainVec/array with nozeroizeon drop — out of scope per the stated threat model (doesn't protect against anyone with the running server), but worth a follow-up if that threat model ever tightens.- The file-reordering tamper case has a full
Framed-level integration test, but the truncation case is only exercised at theObjectKey::openunit-test level, not through a real truncated file viaFramed::open. Given the AAD math is identical either way this is very unlikely to hide anything, just noting the coverage asymmetry.
No blocking findings. CI is green (SonarQube/Check still running at time of review, nothing failing).
SonarQube — aucune nouvelle issueComparaison entre le projet bac à sable de cette PR et la branche par défaut : SonarQube Community n'analyse pas les PR, ce delta est calculé côté CI. Détail |
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 #44.
LFSX_ENCRYPTION_KEY_FILEpoints at a file of 32-byte keys as hex. Unset, nothing changes. A path rather than the key itself for the reason the issue gives: a key in the environment is in the pod spec, indocker inspect, and in every log that dumps the environment.The server refuses to start if the file is missing or unreadable. A server that quietly wrote plaintext because a Secret failed to mount is the one failure this must never have, since nothing downstream would notice and the objects written in the meantime are exactly the ones an operator believed were covered.
It is version 2 of the format compression already had
That format solved most of this before encryption existed: a magic header so a plain store and a framed one are the same store, the plaintext length in the header, four-megabyte frames decoded independently, and an index. So encryption is not a second layer over the file, it is sealed frames inside the one that was there.
Content-Lengthcomes from the header, which the compression work already made true. The issue calls this the detail most likely to be missed; it was already load-bearing.levelbecameOption<i32>rather than growing a special case, so encryption without compression is a configuration rather than a code path.The crypto, and why each choice
ChaCha20-Poly1305, because ARM servers and a Raspberry Pi are supported targets and it does not lean on AES-NI.
A key per object, derived from the master key and a 16-byte salt stored with it. The alternative was a random nonce prefix per object, which puts a birthday bound on how many objects one key may cover. Deriving instead means the nonce is only ever a frame counter and two objects cannot collide on one, by construction rather than by probability.
A key is identified by a hash of itself, not by a number an operator assigns. An id can then never come to name a different key than the one it was written with, and rotation is appending a line rather than remembering which number is next. Every key in the file reads, the first writes, so the first rotation is not a re-encryption of the store.
Each frame is bound to where it was written: its index, whether it is the last, and the object id are authenticated. That refuses three rearrangements someone with write access to the disk could otherwise make without touching a byte inside any frame — reordering, truncation to a shorter object that still opens, and moving one file on top of another in the shared content store. There is a test per case.
Neither
KeyringnorObjectKeyderivesDebug. The tests format the error rather than the value.Two things that are not in it, deliberately
Bucket mode ignores the key, and says so at startup, exactly as it now ignores compression: both formats are read through the file the codec opens to find its index, and a bucket key is not that. This is the case that most deserves encryption, so it is a gap rather than a decision. Filed as #110 — it is the same seam compression needs, not a second piece of work.
No
migratecommand. Objects written before the key stay plaintext and keep being served; re-pushing converts them. The issue leaves this open; rewriting a store in place is a separate change with its own failure modes, and the format was built so it is not required.Chart
Mounts an existing Secret rather than taking the key as a value, since a Helm value lands in the release secret.
defaultMode: 0440and not0400: a projected secret is owned by root,fsGroupgives the group to 65532, and owner-only read looks tighter while leaving the server unable to open its own key.Tests
209 pass. New ones cover the key file and rotation, the three rearrangements above, ranges over encrypted objects, both kinds readable side by side, an encrypted object refused rather than served as ciphertext by a keyless server, the audit reading encrypted objects through their own bytes, and that the plaintext is genuinely not on the disk.
The README states the threat model in the words the issue asks for, including that this does not protect against anyone who has the running server, and points at LUKS or an encrypted volume first.