fix(kryphos): zeroize decrypted secrets and encrypt credential metadata at rest - #382
Open
forkwright wants to merge 2 commits into
Open
fix(kryphos): zeroize decrypted secrets and encrypt credential metadata at rest#382forkwright wants to merge 2 commits into
forkwright wants to merge 2 commits into
Conversation
added 2 commits
August 16, 2026 22:15
…ta at rest DecryptedEntry.secret and the config resolver's intermediate string were bare Vec<u8>/String with no zeroization on drop, so decrypted plaintext credentials lingered in freed heap memory. Wrap at the point of allocation (Zeroizing<Vec<u8>>, Zeroizing<String>) so the guarantee is type-enforced rather than assumed; the same class in unseal_signing_key's ephemeral plaintext buffer is hardened too. StoredEntry also kept credential name (as the plaintext fjall key), type, metadata, status, and history unencrypted, so filesystem access to the vault revealed the operator's entire credential inventory without ever attacking the secret's AEAD. Both are now encrypted: metadata moves into a second ChaCha20-Poly1305 field (encrypted_metadata, kept separate from encrypted_secret so list() never touches secret ciphertext) and the fjall record key becomes a keyed-BLAKE3 hash of the name, mirroring the existing tamper-log chain-key derivation. VAULT_VERSION bumps to 2 (hard break, no migration point pre-1.0) since the on-disk shape changed incompatibly.
seal_signing_key held the raw Ed25519 signing key in an ordinary [u8; 32] before encrypting it -- the encrypt-direction sibling of the buffer unseal_signing_key already wraps in Zeroizing, left uncovered when that fix landed. SigningKey::from_bytes made the same mistake one call-frame further in: TryInto<[u8; N]> copies the caller's bytes into a fresh stack array that ed25519_dalek::SigningKey::from_bytes reads and never scrubs, so unseal_signing_key's own Zeroizing wrap stopped one frame short of the ZeroizeOnDrop-protected key it constructs. Both are now type-enforced the same way the decrypt direction already is: zeroizing_signing_key_bytes (vault.rs) and zeroizing_key_array (key.rs) return Zeroizing<[u8; N]> rather than a bare array, and each has a dispositive by-construction test (assert_zeroizes_on_drop against the actual returned value, mirroring decrypted_secret_is_zeroized_on_drop_by_type in storage_tests.rs) -- [u8; N] alone does not implement ZeroizeOnDrop, so the assertion fails to compile against the pre-fix bare-array return and compiles clean against the wrapped one. Corrects the unseal_signing_key WHY comment, which claimed coverage ended at SigningKey's own ZeroizeOnDrop impl -- that claim skipped the from_bytes copy this fix closes.
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.
Summary
Two related plaintext-at-rest/in-memory defects in
kryphos::Vault:#218 — zeroize decrypted secrets
Desired correction / Done when: "
DecryptedEntry's secret field and the config resolver's intermediate string are zeroized on drop (type-enforced)."DecryptedEntry.secretis nowZeroizing<Vec<u8>>—crates/kryphos/src/storage.rs:105.decrypt's return value is moved straight intoZeroizing::new(...)with no intermediate unwrapped binding —crates/kryphos/src/storage.rs:381-382. There is no unscrubbed copy of the original allocation left behind;Zeroizinghas no public escape hatch to a bareVec<u8>(nointo_inner), so this can't regress into "wraps a copy while the original lives on" without the type itself changing.Zeroizing<String>—crates/kryphos/src/config.rs:97.str::from_utf8borrows the still-wrapped bytes instead of consuming them (config.rs:92), so validation never produces an unwrapped copy either. The one unavoidable copy is the final hand-off tofigment::Value::String, which requires a plain, non-zeroizingStringwe don't control (config.rs:104, documented inline with a NOTE) —secret_stritself still zeroizes on drop immediately after.unseal_signing_key's ephemeral plaintext signing-key buffer —crates/kryphos/src/vault.rs:262-266.Negative fixture:
crates/kryphos/src/storage_tests.rs:631(decrypted_secret_is_zeroized_on_drop_by_type) — a compile-time assertion (fn assert_zeroizes_on_drop<T: zeroize::ZeroizeOnDrop>(_: &T)) called against a realDecryptedEntryfromvault.get(). Watched failing by checking out the pre-fixVec<u8>field with only this test added and runningcargo test -p kryphos decrypted_secret_is_zeroized_on_drop_by_typeon a clean build box (verda-build): it failed to compile,error[E0277]: the trait boundu8: ZeroizeOnDropis not satisfied ... required forVecto implementZeroizeOnDrop``. Against this branch it compiles and passes (978/978 nextest, see below). This is deliberately a type-level check, not a runtime heap scan: it encodes exactly what "type-enforced" in the issue means, and it's dispositive rather than allocator-behavior-dependent.#215 — encrypt metadata + obfuscate the lookup key at rest
Desired correction / Done when: "the on-disk fjall contents no longer reveal credential names, types, or tags in plaintext" (the full-encryption path, not the doc-correction alternative — see rationale below).
StoredEntryis now two independently-nonced ChaCha20-Poly1305 ciphertexts,encrypted_secretandencrypted_metadata—crates/kryphos/src/storage.rs:74-77.encrypted_metadatadecrypts toEntryMetadataRecord(name, credential type, metadata, status, history) —storage.rs:82-90.Vault::lookup_key,storage.rs:615-618), mirroring the existingchain_keydomain-separation pattern — not the plaintext name (storage.rs:346/494/542insert with the derivedkey, notname).list()decrypts onlyencrypted_metadata, neverencrypted_secret(storage.rs:400-417) — the existing "list never touches secret ciphertext" property is preserved.VAULT_VERSIONbumped 1 → 2 (crates/kryphos/src/vault.rs:29): hard break, no migration point (pre-1.0,open()already hard-rejects any header version mismatch — no silent misread risk).docs/fjall-column-encryption.mdupdated to match: it previously said metadata "remain[s] structured" outside encryption — now describes the two-ciphertext shape and clarifies its Non-Goals bullet is about the generic ColumnCodec/ENCRYPTED_FIELDS abstraction, not about leaving Vault's own fields plaintext (that Non-Goal predates and is compatible with this fix; issue feat: column-level encryption codec over fjall store (declarative encrypted-fields map) #132, which owns the generic-codec shape, is unrelated future-store work — seedocs/fjall-column-encryption.md"Non-Goals").Why the full-encryption path, not the doc-correction alternative: README.md's Design Constraints state "Security default. Encrypted by default. Unencrypted is the opt-in" as a project-wide claim, not scoped to secret values only — and this is the credential vault, the component that claim is about first. Conceding the doc to match the weaker implementation would be correcting the wrong side of the gap for a security-primary project's flagship type.
Negative fixture:
crates/kryphos/src/storage_tests.rs:659(on_disk_fjall_contents_do_not_reveal_credential_name) — writes one entry with a 40-byte distinctive name, drops the vault, then recursively reads every file fjall actually wrote underdata/and asserts the name is not a byte-for-byte substring anywhere. Watched failing on a clean build box (verda-build) with only this test added on top of pre-fixmain: it panicked,credential name must not appear in plaintext anywhere under the fjall data directory(the name is the literal fjall key pre-fix, so it's trivially present in the LSM tree's persisted pages). Against this branch it passes.Verification
No build/test CI workflow exists yet in this repo (#262, a sibling unit), so a green PR check here means nothing on its own. Gated for real on the build box instead:
kanon gate --tier fullresult (tip3cf9ca3, this branch merged with currentorigin/main):The one failing step (
kanon lint) is twoOIKOS/private-contenterrors onCONTRIBUTING.md:8/:18(an internalkanon.lanhostname literal) — pre-existing onmain, untouched by this branch, and already tracked separately as #377 (filed ~15 minutes before this gate run, evidently by a concurrent session). It blocks a passing stamp for every branch right now, not just this one.cargo fmt/check/clippy/nextestare all clean, and this branch introduces zero newkanon lintviolations (verified before/after viamcp__kanon__lint_checkoncrates/kryphosandcrates/akroasis: 20→19, the one delta being my own test's wording, fixed inline).Done-when checklist
DecryptedEntry's secret field zeroized on drop, type-enforcedstorage.rs:105(Zeroizing<Vec<u8>>) +storage.rs:381-382(wrapped at allocation)config.rs:92-104(Zeroizing<String>, validated by borrow, one documented unavoidable copy at the figment boundary)storage.rs:74-90(encrypted_metadata) +storage.rs:615-618(lookup_key, keyed-hash fjall key)Adjacent findings (out of scope here, filed as follow-ups)
Found while fixing these two; each is genuinely outside the cited Done-when text, so filed rather than bundled:
tamper.logstill records credential names in plaintext (same class as Credential names, metadata, status, and history are stored at rest in plaintext, contradicting the documented encrypted posture #215, different file, deliberately out of its "fjall contents" scope).String(same class as Decrypted secrets returned without zeroization, leaving plaintext credentials in freed memory #218, input side rather than storage side).kryphos::vault::{VaultEntry, VaultHeader}are dead public types that shadow the realStoredEntry/StoredHeadermodel and confused review of this exact area.Closes #218
Closes #215