fix(kryphos): reject empty vault passphrases, bind ciphertext to entry identity, serialize mutations

[forkwright]

Closes #287
Closes #283
Closes #214

Provenance — read this first

This branch is salvaged from a crashed session and its verification never ran. The machine
orchestrating the work died mid-wave, before this unit's adversarial review or any gate. The commit
was on disk and unpushed; a slightly older version had reached the remote, and the local copy was the
richer of the two (it carried storage_security_tests.rs additions the pushed one lacked), so the
local content is what is here.

Treat CI as the first independent witness this work has had. Nothing below is a claim that it was
checked — only a description of what it does.

What it does

#287 — empty passphrases. Vault::create accepted an empty passphrase. Rejected at the boundary.

#283 — ciphertext not bound to entry identity. Vault ciphertext could be relocated between
entries and still decrypt, which is a confused-deputy substitution: an attacker who can move a blob
gets it decrypted into a slot it was never sealed for. The entry identity is now bound into the
authenticated data, so a relocated blob fails authentication rather than yielding plaintext in the
wrong place.

#214 — non-atomic entry mutations. The duplicate check and the write were separate steps, so two
concurrent callers both passed the check. Mutations are now serialized.

Touched: crates/akroasis/src/vault/mod.rs, crates/kryphos/src/{crypto,storage,vault}.rs,
crates/kryphos/src/storage_security_tests.rs — 378 insertions, 47 deletions.

What a reviewer should attack

Since no review ran, these are the questions I would have asked it, unanswered:

• Is the binding actually authenticated? Binding an identity into a field that is encrypted but
  not authenticated buys nothing — a relocated blob still decrypts. It has to be in the AEAD's
  associated data (or equivalent), and the failure has to be an authentication failure.
• Is the mutation race closed or merely narrowed? A smaller window between check and write is
  still a window. Does the serialization cover the whole check-then-write, and what happens if the
  holder dies mid-mutation?
• Does the empty-passphrase rejection cover every construction path, or only Vault::create?
• Do the new tests exercise the defect or its neighbourhood? For each: what would have to be true
  for it to pass while the bug is still present?

Verification status

None. No local gate, no adversarial review, no CI at the time of writing. akroasis also had no
build/test workflow at all until #375 (a sibling unit from the same wave, also unreviewed) — so this
repo's CI story is itself in flux. Do not read a green tick here as equivalent to one elsewhere until
that lands and is verified.

[forkwright]

Independent verification of the review findings. Three of four fixed and confirmed. One overclaims its own evidence, and it is the same defect this PR was fixing, one level up.

The three code fixes are real

• Migration path — vault.rs:16-33 adds MIN_SUPPORTED_VAULT_VERSION=1; storage.rs:298-314 widens Vault::open from an exact != VAULT_VERSION to a range check; storage.rs:406-450 branches Vault::get on entry.envelope_version so legacy entries decrypt under the original scheme; rotate() re-stamps on rewrite.
• seal_signing_key/unseal_signing_key carried the same unauthenticated-AEAD shape Bind vault ciphertext to its entry identity #283 fixed for entries. Now domain-separated via signing_key_aad() (vault.rs:230-250, 266-320). The verifier also checked whether this change could reintroduce the migration hazard for identities and found zero non-test callers workspace-wide, so no on-disk state can break.
• checked_len_prefix no longer clamps with unwrap_or(u32::MAX); it returns VaultError::FieldTooLarge. The stale #[expect(clippy::unnecessary_wraps)] tied to the infallible body is gone — load-bearing, since an unfulfilled expect is itself an error under -D warnings.

The overclaim

The fix commit states the previous commit "pins all four fixtures failing (CI-observed)." CI observed one.

Run 32046159362 against pin commit 6768a848 is genuinely red — that much closes the original gap. But nextest fail-fasts by default, so it stopped at the first failure:

Summary [19.981s] 690/1093 tests run: 689 passed, 1 failed

crypto::tests::checked_len_prefix_rejects_one_past_u32_max failed at position 683/1093 and cancelled the rest. The other three pinned tests — legacy_v1_vault_opens_and_decrypts_pre_283_entries, legacy_v1_vault_rotate_opportunistically_upgrades_the_envelope, unseal_rejects_ciphertext_sealed_for_a_different_purpose — are module-ordered after crypto::tests and never executed in that job at all. Their names appear nowhere in the 1578-line log.

This is the finding recurring inside its own fix. The original defect was a claimed-but-unwitnessed verification; the correction asserts a four-way observation the run could not have produced. One fixture is system-witnessed red — the strongest evidence in this batch — and three are traced by code-read, which is weaker and should be labelled as such rather than folded into the same sentence.

This repo's own tooling already names the mechanism: pre-push-verify.sh requires --no-fail-fast precisely because without it "cargo stops at the first failing crate, so the run reports a subset as if it were the set." A red pin run needs the same flag, for the same reason.

Correction rather than a block: the code is right and the fixtures are real. What needs changing is the claim — either re-pin with --no-fail-fast so all four are actually observed, or state plainly that one was witnessed and three were traced.

One residual worth stating on the record

For a migrated legacy entry (envelope_version == 0), #283's identity-binding protection is not active until that entry is rotated — storage.rs:440-441 decrypts it under empty AAD exactly as before #283 existed. The code's own WHY comment discloses this, so it is a disclosed tradeoff rather than a hidden hole, and it is the unavoidable cost of supporting migration at all. But it means "vaults are identity-bound after #283" is true only of rotated entries, and anyone reading the closure should know which half they have.

[forkwright]

Rebased onto current main after #375, #382 and #384 landed. All thirteen required properties survive, verified on the rebased tree — but one architectural decision changed the meaning of a stated requirement, and it should not pass without being named.

MIN_SUPPORTED_VAULT_VERSION is 2, not 1 — and #283's migration clause narrows with it

This PR originally set MIN_SUPPORTED_VAULT_VERSION = 1 on the premise that the header shape was unchanged between v1 and v2 and only the AAD had moved. That premise stopped being true when #382 merged.

Verified against origin/main, not inferred:

• storage.rs:616-619 — the fjall lookup key is now blake3::keyed_hash(subkey, name), not the plaintext name.
• storage.rs:74-76 — StoredEntry is now two ciphertexts, encrypted_secret + encrypted_metadata.

So a genuine pre-#382 v1 vault stores its entries under plaintext-name keys in a flat shape that the merged code would neither find (wrong key derivation) nor deserialize (wrong struct). Accepting its header would let open() succeed and then fail confusingly deep inside get/list — strictly worse than a clean up-front rejection, because a misleading success is harder to diagnose than an honest refusal.

What that costs against #283's Desired correction, which reads: "…provide an explicit migration path for existing entries."

• Delivered: a real migration path for entries written under a v2 header before entry_aad existed — the envelope_version == 0 branch at storage.rs:499-513, with rotate() opportunistically re-stamping at :539. That is the only class of "existing entry" that can exist post-fix(kryphos): zeroize decrypted secrets and encrypt credential metadata at rest #382, and it is genuinely migrated rather than rejected.
• Not delivered: migration of a true pre-fix(kryphos): zeroize decrypted secrets and encrypt credential metadata at rest #382 v1 vault. That would require reading the old plaintext-keyed flat shape and rewriting it into the hashed-key two-ciphertext shape — separate work, and it is fix(kryphos): zeroize decrypted secrets and encrypt credential metadata at rest #382's break rather than this PR's. fix(kryphos): zeroize decrypted secrets and encrypt credential metadata at rest #382's own commit message declares it: "hard break, not a migration point (pre-1.0, open() already hard-rejects any header version mismatch — no silent misread risk)."

I am accepting that rather than opening a follow-up, because a follow-up would be migrating from a shape the project has already declared abandoned, at 0.1.24, pre-1.0. If a real v1 vault turns out to exist, that is a genuine defect and should be filed against the break rather than against this PR. Stating it here so the closure is not read as "v1 vaults migrate," which it does not mean.

#283's Done-when is met in full and independently: a regression test moves a valid stored value beneath a different key and mutates bound type/version fields, and get rejects rather than returning a misidentified credential.

One pre-existing gap, flagged not fixed

encrypted_metadata is not itself AAD-bound — only encrypted_secret is. A relocated StoredEntry's metadata still decrypts successfully (exposing name, type, status, history) even though its paired secret then fails authentication. That is main's existing encrypt_metadata/decrypt_metadata design, not a regression introduced here, and binding it is new scope beyond this PR's thirteen properties. Worth its own issue.

Verification

Local, on the rebased tree: cargo check -p kryphos -p akroasis --all-targets clean, fmt --check clean, clippy --all-targets -D warnings clean, and 150 tests passing, 0 failed. CI is the witness of record and is still building; local green is evidence, not a verdict.