diff --git a/docs/crypto/key-storage-migration-design.md b/docs/crypto/key-storage-migration-design.md new file mode 100644 index 00000000..5a94de52 --- /dev/null +++ b/docs/crypto/key-storage-migration-design.md @@ -0,0 +1,140 @@ +# Key-storage migration — design (DESIGN ONLY, not yet approved) + +> **Status:** proposal for owner review. **No code ships from this doc** until the owner signs off — +> it touches the E2EE root of trust, where a mistake means permanent, unrecoverable content loss. +> Batch 5.1 of the roadmap (`make-a-plan-for-recursive-spindle.md`). + +## Context / problem + +All on-device secrets go through `data/local/SecurePreferencesFactory`, which uses +**`androidx.security:security-crypto:1.0.0`** (`EncryptedSharedPreferences` + `MasterKeys`). That +library is **deprecated and unmaintained by Google** (no fixes, no target-SDK updates). Six stores +depend on it: + +| Store | Holds | If lost… | +|---|---|---| +| `crypto/CoupleKeyStore` | **the couple AES-256 keyset** (Tink, JSON) + recovery phrase | **all E2EE content unreadable** — the crown jewel | +| `crypto/UserKeyManager` | per-user ECIES P-256 private key | sealed-answer/keybox flows break until re-published | +| `crypto/PendingAnswerKeyStore` | one-time per-answer keys, pre-reveal | a pending answer can't be revealed | +| `data/local/RecoveryPhraseStore` | the recovery phrase | falls back to partner copy | +| `data/local/PendingInviteStore` | inviter's code + phrase during pairing | pairing restart | +| `data/repository/SharedPreferencesLocalAnswerRepository` | local answer drafts | drafts lost | + +### The load-bearing hazard to fix along the way + +`SecurePreferencesFactory.encryptedSharedPreferences()` catches any failure opening the store and +calls **`reset()` — which deletes the file** — then recreates it empty. For draft/pending stores +that's an acceptable "start over." **For `CoupleKeyStore` it is silent, permanent couple-key +destruction**: a transient Keystore hiccup, an OS upgrade that invalidates the master key, or a +botched migration would wipe the couple key with no prompt. The couple is partner-recoverable today +(the partner holds the same key + phrase), so it's not *total* loss — but it turns a recoverable blip +into a full re-pair/restore. **Any migration must remove this auto-wipe for the couple-key store** and +replace it with a fail-closed path that preserves ciphertext and asks the user to recover, never +deletes. + +## Goals + +1. Move off `androidx.security:security-crypto` to a **maintained** primitive. +2. **Never lose the couple key** during or after migration — the overriding constraint. +3. Zero user-visible disruption for the happy path (transparent, lazy migration). +4. Keep the wire/at-rest format of *server-stored* data unchanged (this is purely on-device storage; + Firestore `enc:v1:` ciphertext and the recovery-phrase-wrapped `wrappedCoupleKey` are untouched). + +## Non-goals + +- No change to the E2EE scheme itself (Tink AEAD, Argon2id recovery wrap, ECIES keyboxes) — see + `SECURITY.md` / `docs/Engineering_Reference_Manual.md`. +- No change to server storage or Firestore rules. +- Not couple-key *rotation* / forward secrecy (separate roadmap item). + +## Target design (recommended) + +**Tink's Android-Keystore-backed keyset storage** (`AndroidKeysetManager` + +`AndroidKeystoreKmsClient`), replacing `EncryptedSharedPreferences`: + +- A single app master key in the AndroidKeyStore (`AndroidKeystoreKmsClient`, alias e.g. + `closer_master_key`) wraps each stored Tink keyset; keysets persist in a plain `SharedPreferences` + as Tink-encrypted blobs. This is Tink-native (we already depend on `tink-android`), maintained, and + removes the `androidx.security` dependency. +- Secrets that are raw strings (recovery phrase, invite phrase, local drafts) are wrapped with a + dedicated Tink AEAD whose keyset is itself Keystore-master-wrapped (same mechanism), so nothing is + stored in cleartext. +- **Fail-closed**, not fail-wiped: if a keyset can't be decrypted, surface a recover-this-device flow + (existing partner-assisted restore / recovery-phrase paths) — never delete. + +**Alternative considered:** raw AndroidKeyStore AES-GCM + manual IV/blob management. Rejected — more +bespoke crypto code to get wrong; Tink already gives us the KMS-client wrapper. + +## Migration strategy + +Transparent, lazy, per-store, one direction. For each key on read: + +1. **Dual-read window.** New `SecureStoreV2` tries the new (Tink-Keystore) location first; on miss, + falls back to reading the **old** `EncryptedSharedPreferences` value. +2. **Re-wrap on read.** When a value is found only in the old store, decrypt it via the old lib and + **write it to the new store**, then return it. (Lazy migration — no big-bang pass, no startup + stall.) The old value is **left in place** until the migration is confirmed durable (see below). +3. **Confirm, then clean.** Only after the new value has been successfully read back from the new + store (a verify-read) is the old entry deleted. A crash between steps 2 and 3 is safe: next read + re-does the re-wrap idempotently. +4. **Never-lose-the-couple-key rule.** For `CoupleKeyStore` specifically: the old-store delete in + step 3 is gated on a successful new-store round-trip AND is a no-op if the new value is absent. + The `reset()`/auto-wipe path is removed for this store; a decrypt failure raises a typed + `KeyUnavailable` that routes to recovery UI, not deletion. + +### Failure matrix + +| Situation | Behavior | +|---|---| +| New store has the value | Use it (fast path). | +| Only old store has it | Re-wrap → new store → verify → (later) delete old. | +| Neither has it, key expected | `KeyUnavailable` → recovery flow (partner/phrase). **No wipe.** | +| New-store write fails mid-migration | Keep old value; return decrypted value from old; retry next read. | +| Keystore master invalidated (OS upgrade / biometric change) | Detect, treat as `KeyUnavailable` → recovery; do NOT recreate empty. | + +## Consumer ordering (lowest → highest risk) + +1. `PendingInviteStore`, `SharedPreferencesLocalAnswerRepository`, `PendingAnswerKeyStore` — + ephemeral/rebuildable; a wipe here is tolerable, so migrate first to shake out the mechanism. +2. `UserKeyManager` (ECIES) — recoverable by re-publishing a fresh public key; medium risk. +3. `RecoveryPhraseStore` — partner-recoverable; medium risk. +4. **`CoupleKeyStore` last** — only after the mechanism is proven on the others, and only with the + fail-closed (no-wipe) behavior in place. + +## Staged rollout + telemetry + +- Behind a Remote Config flag `key_storage_v2_enabled` (build the RC wrapper first — see + `Future.md`, currently no wrapper exists). Default off; enable to a small % first. +- **Content-free** telemetry only (respecting the analytics consent toggle): migration attempted / + succeeded / fell-back / `KeyUnavailable` counts per store — never key material. Watch the + `KeyUnavailable`-on-couple-key rate like a hawk; any nonzero blip is a rollback trigger. +- Kill switch: flag off → `SecureStoreV2` reads new-then-old but stops *writing* new (freezes + migration) without breaking either store. + +## Test plan (before any staged rollout) + +- Instrumented (real Keystore, can't be JVM-unit-tested): write via old lib → read via V2 → + re-wrap → new-store round-trip → old entry cleaned only after verify. +- **Fresh-device / process-death**: kill between re-wrap and cleanup; assert idempotent recovery, no + loss. +- **Keystore-invalidation simulation**: force a decrypt failure; assert `KeyUnavailable` + recovery + route, assert the file is NOT deleted. +- Full E2EE round-trip after migration (encrypt/decrypt a message, reveal a daily answer) on a + migrated device — extends the existing QA "content is ciphertext at rest" pass. +- Backward-compat: a device that never migrates (flag off) keeps working on the old store. + +## Open questions for the owner + +1. **Acceptable rollout %/duration** for the couple-key store, given partner-recoverability as the + safety net? +2. Keep the old `androidx.security` dependency for a full release cycle as the dual-read source, then + remove — or remove sooner? +3. Do we take the opportunity to **also** remove the `reset()` auto-wipe from the *other* stores now + (independent, small, strictly-safer change), or bundle it all? + +## Recommendation + +Ship the **`reset()`-auto-wipe removal for `CoupleKeyStore` as a small, standalone, pre-migration +fix** (it's strictly safer and independent of the library swap), then do the Tink-Keystore migration +lazily behind the RC flag in the consumer order above, couple key last. Nothing here is implemented +until this design is approved.