From bfdeaa1ace16578d9b716f78523462d83f231c98 Mon Sep 17 00:00:00 2001 From: mstrofnone Date: Sat, 12 Sep 2026 14:37:08 +1000 Subject: [PATCH 1/5] fix(commons): consolidate desktop keychain items into a single vault-v1 item Follow-up to ae3218249a. Caching the Keyring handle collapsed Keyring.create() calls but did not fix the double prompt on macOS, because macOS Keychain gates access per item, not per session. Amethyst's cold-boot path reads at minimum two distinct items, account-metadata-key (the DesktopAccountStorage AES key) and the active account's nsec, so the OS still surfaces two Keychain Access prompts unless the user explicitly picked "Always Allow" on every single item, which many users don't. Fix: consolidate all Amethyst-managed keychain items into a single vault-v1 item, so the OS sees exactly one item to gate. SecureKeyStorage.enableConsolidatedVault(candidateAliases): 1. If vault-v1 already exists, load it and mark the vault active. Legacy per-alias items in candidateAliases are still folded in on this pass so a crash-during-migration leaves nothing stranded. Aliases not in candidateAliases are ignored, so pre-existing items from unrelated accounts do not get pulled in and re-prompt. 2. If vault-v1 is absent, batch-read each candidate alias in the legacy per-item layout (paying the migration prompt once), pack the recovered entries into vault-v1, and delete the originals. The vault item is written first, legacy items are deleted only after that write succeeds, so a crash mid-migration leaves the legacy items in place and the next run retries cleanly. 3. Fresh installs write an empty vault so future savePrivateKey calls stay inside it. Migration is idempotent and cheap when the vault already exists (one keychain read plus a JSON parse). Safe to call on every cold boot, and safe to call twice per process for the two-phase bootstrap pattern (see the AccountManager change in the next commit). Once the vault is active, savePrivateKey / getPrivateKey / deletePrivateKey / hasPrivateKey read and write the in-memory LinkedHashMap and persist the whole map back to the vault item on mutation. Fresh SecureKeyStorage instances that have not opted into the vault continue to use the legacy per-alias layout, so callers that never call enableConsolidatedVault keep working unchanged. The vault contents are stored as a JSON envelope of the form {"schemaVersion":1,"entries":{alias: base64(secret), ...}}. schemaVersion reserves room for future migrations. Values are base64-encoded so alias / secret contents that contain quotes, backslashes, control chars, or non-ASCII round-trip cleanly through the hand-rolled JSON codec (kept hand-rolled so the keystorage module does not need Jackson; Jackson lives in desktopApp and quartz). Amethyst does not layer additional crypto over the OS keyring for legacy per-item storage; the OS keychain is the trust boundary. The vault follows the same policy. Tests (SecureKeyStorageVaultTest, hermetic CountingKeyring KeyringHandle fake, no real OS keychain): - fresh install writes an empty vault item - legacy items are migrated into one vault-v1 item and originals deleted - an existing vault is loaded without re-probing aliases it already contains - legacy leftovers from an interrupted earlier migration get absorbed on the next boot when still named in the candidate list - savePrivateKey after vault enabled persists to the vault - getPrivateKey after vault enabled reads from in-memory contents without further keychain traffic - enableConsolidatedVault is idempotent across repeated calls - two-phase migration folds in aliases discovered after phase 1 - partial legacy migration survives a relaunch (two SecureKeyStorage instances against one backing store) - delete removes from the vault and unlinks the whole item when the last alias goes - vault round-trips keys with quotes, newlines, backslashes, and Unicode All existing SecureKeyStorageKeyringCacheTest cases still pass; the changes are additive and back-compatible when enableConsolidatedVault is never called. --- .../commons/keystorage/SecureKeyStorage.kt | 398 +++++++++++++++++- .../keystorage/SecureKeyStorageVaultTest.kt | 365 ++++++++++++++++ 2 files changed, 753 insertions(+), 10 deletions(-) create mode 100644 commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorageVaultTest.kt diff --git a/commons/src/jvmMain/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorage.kt b/commons/src/jvmMain/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorage.kt index 31a27b98fb..7218f15129 100644 --- a/commons/src/jvmMain/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorage.kt +++ b/commons/src/jvmMain/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorage.kt @@ -92,7 +92,7 @@ actual class SecureKeyStorage private actual constructor() { * some OSes (notably macOS and locked GNOME/KWallet sessions), triggers a * user-visible unlock prompt every time. Callers hit the storage at least * twice on cold start (metadata AES key, then active account nsec), so a - * per-call [Keyring.create] would prompt the user twice on startup — the + * per-call [Keyring.create] would prompt the user twice on startup, the * exact bug this cache fixes. * * Guarded by [keyringLock] so probing/opening the backend happens exactly @@ -109,13 +109,39 @@ actual class SecureKeyStorage private actual constructor() { */ internal var keyringFactory: () -> KeyringHandle = { RealKeyringHandle(Keyring.create()) } + /** + * Consolidated single-item vault name. When [enableConsolidatedVault] has + * run, every alias Amethyst owns is packed as one JSON blob under this + * account name, so the OS keychain sees exactly one item to gate. + * + * macOS Keychain gates access per item, not per session, so caching the + * [Keyring] handle alone cannot collapse the cold-boot double prompt + * (metadata AES key plus the active account's nsec). One item, one ACL, + * one prompt is the durable fix. + */ + private val vaultAlias: String = "vault-v1" + private val vaultMutex = Mutex() + + @Volatile + private var vaultActive: Boolean = false + + /** + * In-memory copy of the vault contents. Non-null iff [vaultActive] is + * true. Guarded by [vaultMutex] for mutations; reads are lock-free via + * the volatile reference plus a defensive copy inside [vaultGet]. + */ + @Volatile + private var vaultContents: MutableMap? = null + actual suspend fun savePrivateKey( npub: String, privKeyHex: String, ) { withContext(Dispatchers.IO) { try { - if (keyringAvailable) { + if (vaultActive) { + vaultPut(npub, privKeyHex) + } else if (keyringAvailable) { saveToKeyring(npub, privKeyHex) } else { saveToFallback(npub, privKeyHex) @@ -133,10 +159,10 @@ actual class SecureKeyStorage private actual constructor() { actual suspend fun getPrivateKey(npub: String): String? = withContext(Dispatchers.IO) { try { - if (keyringAvailable) { - getFromKeyring(npub) - } else { - getFromFallback(npub) + when { + vaultActive -> vaultGet(npub) + keyringAvailable -> getFromKeyring(npub) + else -> getFromFallback(npub) } } catch (e: BackendNotSupportedException) { keyringAvailable = false @@ -221,10 +247,10 @@ actual class SecureKeyStorage private actual constructor() { actual suspend fun deletePrivateKey(npub: String): Boolean = withContext(Dispatchers.IO) { try { - if (keyringAvailable) { - deleteFromKeyring(npub) - } else { - deleteFromFallback(npub) + when { + vaultActive -> vaultDelete(npub) + keyringAvailable -> deleteFromKeyring(npub) + else -> deleteFromFallback(npub) } } catch (e: BackendNotSupportedException) { keyringAvailable = false @@ -236,6 +262,358 @@ actual class SecureKeyStorage private actual constructor() { actual suspend fun hasPrivateKey(npub: String): Boolean = getPrivateKey(npub) != null + // --- Consolidated vault (see [vaultAlias] docs) --- + + /** + * Consolidates [candidateAliases] into a single OS keychain item named + * [vaultAlias]. On the next cold boot only that one item is read, so the + * OS surfaces at most one Keychain Access prompt regardless of how many + * secrets Amethyst manages. + * + * Semantics: + * + * 1. If [vaultAlias] already exists, load it into memory and mark + * [vaultActive]. Legacy per-alias items are not touched, zero extra + * prompts on that path. + * 2. If [vaultAlias] is absent, batch-read each candidate alias in the + * legacy per-item layout (paying the migration prompt once), pack the + * recovered entries into the vault item, and delete the legacy items. + * 3. If neither the vault nor any legacy alias exists (fresh install), + * the vault becomes an empty active map; subsequent writes go + * straight into it. + * + * Migration is idempotent and cheap when the vault already exists (one + * keychain read plus a JSON parse). Safe to call on every cold boot and + * safe to call multiple times per process: additional calls extend the + * vault with any newly-discovered legacy aliases and never rewrite the + * item if the delta is empty. + * + * Legacy items are deleted only after the vault write succeeds, so a + * crash mid-migration leaves the legacy items in place and the next run + * retries cleanly. No data loss window. + * + * The fallback (no-keyring) storage path is not migrated: it already + * uses a single encrypted file, so it does not have the per-item ACL + * problem the vault exists to solve. + */ + suspend fun enableConsolidatedVault(candidateAliases: List) { + withContext(Dispatchers.IO) { + vaultMutex.withLock { + if (!keyringAvailable) return@withLock // fallback path doesn't need the vault + try { + val handle = keyring() + if (!vaultActive) { + val existing = + try { + handle.getPassword(SERVICE_NAME, vaultAlias) + } catch (_: PasswordAccessException) { + null + } + if (existing != null) { + vaultContents = decodeVault(existing).toMutableMap() + vaultActive = true + // Fall through to the fold-in pass so any legacy per-alias + // items left behind by a partial earlier migration get + // absorbed on this cold boot. + } else { + // Fresh migration path: batch-read every candidate alias. + val collected = LinkedHashMap() + for (alias in candidateAliases) { + try { + collected[alias] = handle.getPassword(SERVICE_NAME, alias) + } catch (_: PasswordAccessException) { + // absent, skip + } + } + // Write the vault first, delete legacy items only after the write + // succeeded. Empty vaults are still written so a fresh install ends + // up in vault mode (subsequent savePrivateKey calls populate it). + handle.setPassword(SERVICE_NAME, vaultAlias, encodeVault(collected)) + for (alias in collected.keys) { + try { + handle.deletePassword(SERVICE_NAME, alias) + } catch (_: PasswordAccessException) { + // already gone, fine + } + } + vaultContents = collected + vaultActive = true + return@withLock + } + } + + // Fold-in pass. Runs on: + // - a second `enableConsolidatedVault(fullList)` call after the + // phase-1 metadata-key-only bootstrap, and + // - a cold boot that finds `vault-v1` alongside legacy per-alias + // items from an interrupted earlier migration. + val current = vaultContents ?: LinkedHashMap() + val additions = LinkedHashMap() + for (alias in candidateAliases) { + if (alias in current) continue + val legacy = + try { + handle.getPassword(SERVICE_NAME, alias) + } catch (_: PasswordAccessException) { + null + } ?: continue + additions[alias] = legacy + } + if (additions.isNotEmpty()) { + val merged = LinkedHashMap(current).also { it.putAll(additions) } + handle.setPassword(SERVICE_NAME, vaultAlias, encodeVault(merged)) + for (alias in additions.keys) { + try { + handle.deletePassword(SERVICE_NAME, alias) + } catch (_: PasswordAccessException) { + // already gone, fine + } + } + vaultContents = merged + } + } catch (e: BackendNotSupportedException) { + keyringAvailable = false + println("OS keyring not available during vault migration, keeping fallback storage") + } catch (e: Exception) { + // Migration is best-effort: if the OS keychain is misbehaving we leave + // the legacy per-alias items in place and continue in legacy mode. + println("enableConsolidatedVault: aborting migration: ${e.message}") + } + } + } + } + + /** Returns true iff the consolidated vault has been loaded or migrated. */ + fun isVaultActive(): Boolean = vaultActive + + /** + * Test-only accessor for the current in-memory alias set. Kept internal + * so tests in the same module can assert vault contents without exposing + * secrets to app code. + */ + internal fun snapshotCacheKeys(): Set = vaultContents?.keys?.toSet() ?: emptySet() + + private fun vaultGet(alias: String): String? = vaultContents?.get(alias) + + private fun vaultPut( + alias: String, + value: String, + ) { + val contents = vaultContents ?: LinkedHashMap().also { vaultContents = it } + contents[alias] = value + keyring().setPassword(SERVICE_NAME, vaultAlias, encodeVault(contents)) + } + + private fun vaultDelete(alias: String): Boolean { + val contents = vaultContents ?: return false + val removed = contents.remove(alias) != null + if (!removed) return false + if (contents.isEmpty()) { + return try { + keyring().deletePassword(SERVICE_NAME, vaultAlias) + true + } catch (_: PasswordAccessException) { + true // already gone, still removed from our POV + } + } + keyring().setPassword(SERVICE_NAME, vaultAlias, encodeVault(contents)) + return true + } + + /** + * Envelope: `{"schemaVersion":1,"entries":{alias: base64(secret), …}}`. + * + * The [schemaVersion] field reserves room for future migrations. Values + * are base64-encoded so alias/secret contents that contain quotes, + * backslashes, control chars, or non-ASCII round-trip cleanly through the + * hand-rolled JSON codec (kept hand-rolled to avoid pulling Jackson into + * the keystorage module; Jackson lives in desktopApp / quartz). + */ + private fun encodeVault(map: Map): String { + val sb = StringBuilder("{\"schemaVersion\":1,\"entries\":{") + var first = true + for ((k, v) in map) { + if (!first) sb.append(',') + first = false + sb + .append('"') + .append(jsonEscape(k)) + .append('"') + .append(':') + .append('"') + .append(Base64.getEncoder().encodeToString(v.toByteArray(Charsets.UTF_8))) + .append('"') + } + sb.append("}}") + return sb.toString() + } + + private fun decodeVault(raw: String): Map { + val trimmed = raw.trim() + if (trimmed.length < 2 || trimmed.first() != '{' || trimmed.last() != '}') return emptyMap() + val body = trimmed.substring(1, trimmed.length - 1) + if (body.isBlank()) return emptyMap() + + // Minimal object parser: expect a mix of "key":number and "key":"string" + // pairs at the top level, plus a nested "entries":{ ... } object holding + // base64-encoded alias values. Bare-map layouts written by a hypothetical + // earlier vault format are still accepted (fallback path). + var i = 0 + var entriesRaw: String? = null + while (i < body.length) { + if (body[i] != '"') return decodeBareEntries(body) + val keyEnd = findUnescapedQuote(body, i + 1) + if (keyEnd < 0) return emptyMap() + val key = jsonUnescape(body.substring(i + 1, keyEnd)) + i = keyEnd + 1 + if (i >= body.length || body[i] != ':') return emptyMap() + i += 1 + if (i >= body.length) return emptyMap() + when (body[i]) { + '"' -> { + val valEnd = findUnescapedQuote(body, i + 1) + if (valEnd < 0) return emptyMap() + // String-valued top-level fields are ignored except for legacy + // format detection handled by decodeBareEntries. + i = valEnd + 1 + } + '{' -> { + val objEnd = findMatchingBrace(body, i) + if (objEnd < 0) return emptyMap() + if (key == "entries") { + entriesRaw = body.substring(i + 1, objEnd) + } + i = objEnd + 1 + } + else -> { + // Skip a bare token (schemaVersion number, boolean, null). + while (i < body.length && body[i] != ',' && body[i] != '}') i += 1 + } + } + if (i < body.length) { + if (body[i] != ',') return emptyMap() + i += 1 + } + } + return entriesRaw?.let { decodeBareEntries(it) } ?: emptyMap() + } + + /** Parses a `"k":"b64","k2":"b64"` body into `{k: decoded, k2: decoded}`. */ + private fun decodeBareEntries(body: String): Map { + val out = LinkedHashMap() + val trimmed = body.trim() + if (trimmed.isEmpty()) return out + var i = 0 + while (i < trimmed.length) { + if (trimmed[i] != '"') return emptyMap() + val keyEnd = findUnescapedQuote(trimmed, i + 1) + if (keyEnd < 0) return emptyMap() + val key = jsonUnescape(trimmed.substring(i + 1, keyEnd)) + i = keyEnd + 1 + if (i >= trimmed.length || trimmed[i] != ':') return emptyMap() + i += 1 + if (i >= trimmed.length || trimmed[i] != '"') return emptyMap() + val valEnd = findUnescapedQuote(trimmed, i + 1) + if (valEnd < 0) return emptyMap() + val b64 = trimmed.substring(i + 1, valEnd) + val value = + try { + String(Base64.getDecoder().decode(b64), Charsets.UTF_8) + } catch (_: IllegalArgumentException) { + return emptyMap() + } + out[key] = value + i = valEnd + 1 + if (i < trimmed.length) { + if (trimmed[i] != ',') return emptyMap() + i += 1 + } + } + return out + } + + private fun findUnescapedQuote( + s: String, + from: Int, + ): Int { + var i = from + while (i < s.length) { + when (s[i]) { + '\\' -> i += 2 + '"' -> return i + else -> i += 1 + } + } + return -1 + } + + private fun findMatchingBrace( + s: String, + openAt: Int, + ): Int { + var depth = 0 + var i = openAt + while (i < s.length) { + when (s[i]) { + '"' -> { + val end = findUnescapedQuote(s, i + 1) + if (end < 0) return -1 + i = end + 1 + } + '{' -> { + depth += 1 + i += 1 + } + '}' -> { + depth -= 1 + if (depth == 0) return i + i += 1 + } + else -> i += 1 + } + } + return -1 + } + + private fun jsonEscape(s: String): String { + val sb = StringBuilder(s.length) + for (c in s) { + when (c) { + '\\' -> sb.append("\\\\") + '"' -> sb.append("\\\"") + '\n' -> sb.append("\\n") + '\r' -> sb.append("\\r") + '\t' -> sb.append("\\t") + else -> sb.append(c) + } + } + return sb.toString() + } + + private fun jsonUnescape(s: String): String { + if (!s.contains('\\')) return s + val sb = StringBuilder(s.length) + var i = 0 + while (i < s.length) { + val c = s[i] + if (c == '\\' && i + 1 < s.length) { + when (s[i + 1]) { + '\\' -> sb.append('\\') + '"' -> sb.append('"') + 'n' -> sb.append('\n') + 'r' -> sb.append('\r') + 't' -> sb.append('\t') + else -> sb.append(s[i + 1]) + } + i += 2 + } else { + sb.append(c) + i += 1 + } + } + return sb.toString() + } + // Keyring-based storage /** diff --git a/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorageVaultTest.kt b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorageVaultTest.kt new file mode 100644 index 0000000000..9093f97af3 --- /dev/null +++ b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorageVaultTest.kt @@ -0,0 +1,365 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.amethyst.commons.keystorage + +import com.github.javakeyring.PasswordAccessException +import kotlinx.coroutines.runBlocking +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Test +import java.util.Base64 +import java.util.concurrent.ConcurrentHashMap +import java.util.concurrent.atomic.AtomicInteger + +/** + * Regression tests for [SecureKeyStorage.enableConsolidatedVault]. + * + * macOS Keychain gates access per item, not per session, so caching the + * [Keyring] handle alone (see [SecureKeyStorageKeyringCacheTest]) cannot + * collapse the cold-boot double prompt (metadata AES key plus the active + * account's nsec). The vault consolidates every alias Amethyst owns into a + * single keychain item so the OS sees one ACL to gate. + * + * Fake [KeyringHandle] backend keeps every case hermetic; the real macOS / + * Windows / Linux backends are only exercised by manual QA. + */ +class SecureKeyStorageVaultTest { + private class CountingKeyring : KeyringHandle { + val store: ConcurrentHashMap, String> = ConcurrentHashMap() + val readsBySlot: ConcurrentHashMap, AtomicInteger> = ConcurrentHashMap() + val writesBySlot: ConcurrentHashMap, AtomicInteger> = ConcurrentHashMap() + + override fun getPassword( + service: String, + account: String, + ): String { + readsBySlot.computeIfAbsent(service to account) { AtomicInteger(0) }.incrementAndGet() + return store[service to account] ?: throw PasswordAccessException("no entry") + } + + override fun setPassword( + service: String, + account: String, + password: String, + ) { + writesBySlot.computeIfAbsent(service to account) { AtomicInteger(0) }.incrementAndGet() + store[service to account] = password + } + + override fun deletePassword( + service: String, + account: String, + ) { + if (store.remove(service to account) == null) { + throw PasswordAccessException("no entry") + } + } + + fun reads(alias: String): Int = readsBySlot[SERVICE to alias]?.get() ?: 0 + + fun writes(alias: String): Int = writesBySlot[SERVICE to alias]?.get() ?: 0 + + fun snapshotAliases(): Set = + store.keys + .filter { it.first == SERVICE } + .map { it.second } + .toSet() + + companion object { + const val SERVICE = "amethyst-desktop" + } + } + + private fun newStorageWith( + opens: AtomicInteger = AtomicInteger(0), + prepopulate: (CountingKeyring) -> Unit = {}, + ): Pair { + val backend = CountingKeyring() + prepopulate(backend) + val storage = SecureKeyStorage.create() + storage.keyringFactory = { + opens.incrementAndGet() + backend + } + return storage to backend + } + + /** Envelope used to seed a pre-existing vault item in tests. */ + private fun seedVault(entries: Map): String { + val body = + entries.entries.joinToString(",") { (k, v) -> + "\"" + k + "\":\"" + Base64.getEncoder().encodeToString(v.toByteArray(Charsets.UTF_8)) + "\"" + } + return "{\"schemaVersion\":1,\"entries\":{$body}}" + } + + @Test + fun `enableConsolidatedVault fresh install writes an empty vault item`() = + runBlocking { + val (storage, backend) = newStorageWith() + storage.enableConsolidatedVault(candidateAliases = emptyList()) + assertTrue(storage.isVaultActive()) + // Fresh install: vault item exists so future writes stay inside it. + assertEquals(setOf("vault-v1"), backend.snapshotAliases()) + assertEquals(1, backend.writes("vault-v1")) + } + + @Test + fun `enableConsolidatedVault migrates legacy items and deletes originals`() = + runBlocking { + val (storage, backend) = + newStorageWith { b -> + b.store[CountingKeyring.SERVICE to "account-metadata-key"] = "AES==" + b.store[CountingKeyring.SERVICE to "npub1alice"] = "aaaa" + b.store[CountingKeyring.SERVICE to "npub1bob"] = "bbbb" + } + + storage.enableConsolidatedVault( + candidateAliases = + listOf( + "account-metadata-key", + "npub1alice", + "npub1bob", + "npub1missing", + "bunker_ephemeral_npub1alice", + ), + ) + + // Consolidation: only vault-v1 remains, legacy items unlinked. + assertEquals(setOf("vault-v1"), backend.snapshotAliases()) + assertTrue(storage.isVaultActive()) + + // Reads still serve the correct secrets from the in-memory vault. + assertEquals("AES==", storage.getPrivateKey("account-metadata-key")) + assertEquals("aaaa", storage.getPrivateKey("npub1alice")) + assertEquals("bbbb", storage.getPrivateKey("npub1bob")) + assertNull(storage.getPrivateKey("npub1missing")) + + // Post-migration savePrivateKey stays inside the vault. + storage.savePrivateKey("npub1carol", "cccc") + assertEquals( + "Post-migration writes must stay inside vault-v1, not create per-alias items", + setOf("vault-v1"), + backend.snapshotAliases(), + ) + assertEquals("cccc", storage.getPrivateKey("npub1carol")) + } + + @Test + fun `enableConsolidatedVault existing vault is loaded without probing aliases it already contains`() = + runBlocking { + val (storage, backend) = + newStorageWith { b -> + b.store[CountingKeyring.SERVICE to "vault-v1"] = + seedVault( + mapOf( + "account-metadata-key" to "SEED", + "npub1alice" to "AAAA", + ), + ) + // A leftover legacy item not in the candidate list on this boot, + // e.g. an npub the user removed from accounts.json.enc. + b.store[CountingKeyring.SERVICE to "npub1stale"] = "STALE" + } + + storage.enableConsolidatedVault( + candidateAliases = listOf("account-metadata-key", "npub1alice"), + ) + + assertTrue(storage.isVaultActive()) + assertEquals(1, backend.reads("vault-v1")) + // Aliases that ARE the candidate set must not be re-probed once the vault has them, + // otherwise the cold-boot prompt count would scale with the account count again. + assertEquals(0, backend.reads("account-metadata-key")) + assertEquals(0, backend.reads("npub1alice")) + // Aliases outside the candidate list are ignored entirely on this boot. + assertEquals(0, backend.reads("npub1stale")) + assertEquals("SEED", storage.getPrivateKey("account-metadata-key")) + assertEquals("AAAA", storage.getPrivateKey("npub1alice")) + } + + @Test + fun `enableConsolidatedVault absorbs legacy leftovers when candidate list still names them`() = + runBlocking { + // Interrupted earlier migration: vault-v1 exists but npub1stale is still + // on disk. If the current AccountManager still has npub1stale in its + // candidate list, cold-boot 2 must absorb it. + val (storage, backend) = + newStorageWith { b -> + b.store[CountingKeyring.SERVICE to "vault-v1"] = + seedVault(mapOf("account-metadata-key" to "SEED")) + b.store[CountingKeyring.SERVICE to "npub1stale"] = "STALE" + } + + storage.enableConsolidatedVault( + candidateAliases = listOf("account-metadata-key", "npub1stale"), + ) + + assertTrue(storage.isVaultActive()) + assertEquals(setOf("vault-v1"), backend.snapshotAliases()) + assertEquals("STALE", storage.getPrivateKey("npub1stale")) + } + + @Test + fun `savePrivateKey after vault enabled persists to vault`() = + runBlocking { + val (storage, backend) = newStorageWith() + storage.enableConsolidatedVault(emptyList()) + + storage.savePrivateKey("npub1new", "1111") + storage.savePrivateKey("npub1other", "2222") + + assertEquals(setOf("vault-v1"), backend.snapshotAliases()) + // Empty-vault seed (1) plus two follow-up writes. + assertEquals(3, backend.writes("vault-v1")) + assertEquals("1111", storage.getPrivateKey("npub1new")) + assertEquals("2222", storage.getPrivateKey("npub1other")) + } + + @Test + fun `getPrivateKey after vault enabled reads from vault contents in memory`() = + runBlocking { + val (storage, backend) = + newStorageWith { b -> + b.store[CountingKeyring.SERVICE to "vault-v1"] = + seedVault(mapOf("npub1alice" to "aaaa")) + } + storage.enableConsolidatedVault(listOf("npub1alice")) + + val readsAfterLoad = backend.reads("vault-v1") + repeat(10) { + assertEquals("aaaa", storage.getPrivateKey("npub1alice")) + assertNull(storage.getPrivateKey("npub1missing")) + } + assertEquals( + "Post-load reads must be served entirely from memory", + readsAfterLoad, + backend.reads("vault-v1"), + ) + } + + @Test + fun `enableConsolidatedVault is idempotent when called repeatedly`() = + runBlocking { + val (storage, backend) = + newStorageWith { b -> + b.store[CountingKeyring.SERVICE to "npub1a"] = "AA" + } + storage.enableConsolidatedVault(listOf("npub1a")) + val writesAfterMigration = backend.writes("vault-v1") + storage.enableConsolidatedVault(listOf("npub1a")) + storage.enableConsolidatedVault(listOf("npub1a")) + // No further writes: idempotent second-phase calls with no new legacy + // aliases are a pure no-op. Migration writes the vault once total. + assertEquals( + "enableConsolidatedVault must be safe to call repeatedly (cold-boot invariant)", + writesAfterMigration, + backend.writes("vault-v1"), + ) + } + + @Test + fun `enableConsolidatedVault two phase migration folds in later aliases`() = + runBlocking { + val (storage, backend) = + newStorageWith { b -> + b.store[CountingKeyring.SERVICE to "account-metadata-key"] = "K" + b.store[CountingKeyring.SERVICE to "npub1alice"] = "aaaa" + b.store[CountingKeyring.SERVICE to "npub1bob"] = "bbbb" + } + // Phase 1: bootstrap with just the metadata key, mimicking + // AccountManager.create() before accounts.json.enc is decrypted. + storage.enableConsolidatedVault(listOf("account-metadata-key")) + assertTrue(storage.isVaultActive()) + assertEquals("K", storage.getPrivateKey("account-metadata-key")) + // Legacy nsecs still on disk because they weren't in phase-1 candidates. + assertTrue("npub1alice" in backend.snapshotAliases()) + + // Phase 2: full list once npubs are known. + storage.enableConsolidatedVault(listOf("account-metadata-key", "npub1alice", "npub1bob")) + assertEquals(setOf("vault-v1"), backend.snapshotAliases()) + assertEquals("aaaa", storage.getPrivateKey("npub1alice")) + assertEquals("bbbb", storage.getPrivateKey("npub1bob")) + } + + @Test + fun `enableConsolidatedVault partial legacy migration survives relaunch`() = + runBlocking { + // Cold boot 1: only metadata key gets migrated; nsec stays on disk. + val opens1 = AtomicInteger(0) + val (storage1, backend) = + newStorageWith(opens1) { b -> + b.store[CountingKeyring.SERVICE to "account-metadata-key"] = "K" + b.store[CountingKeyring.SERVICE to "npub1alice"] = "aaaa" + } + storage1.enableConsolidatedVault(listOf("account-metadata-key")) + assertTrue("npub1alice" in backend.snapshotAliases()) + assertEquals("K", storage1.getPrivateKey("account-metadata-key")) + + // Cold boot 2: fresh SecureKeyStorage against the same backing store. + val opens2 = AtomicInteger(0) + val storage2 = SecureKeyStorage.create() + storage2.keyringFactory = { + opens2.incrementAndGet() + backend + } + storage2.enableConsolidatedVault(listOf("account-metadata-key", "npub1alice")) + assertEquals(setOf("vault-v1"), backend.snapshotAliases()) + assertEquals("K", storage2.getPrivateKey("account-metadata-key")) + assertEquals("aaaa", storage2.getPrivateKey("npub1alice")) + } + + @Test + fun `delete removes from vault and unlinks item when the last key goes`() = + runBlocking { + val (storage, backend) = + newStorageWith { b -> + b.store[CountingKeyring.SERVICE to "account-metadata-key"] = "K" + b.store[CountingKeyring.SERVICE to "npub1x"] = "V" + } + storage.enableConsolidatedVault(listOf("account-metadata-key", "npub1x")) + assertTrue(storage.isVaultActive()) + + assertTrue(storage.deletePrivateKey("npub1x")) + assertTrue("vault-v1" in backend.snapshotAliases()) + + assertTrue(storage.deletePrivateKey("account-metadata-key")) + assertFalse( + "Emptying the vault must unlink the keychain item so a fresh install path can re-migrate cleanly", + "vault-v1" in backend.snapshotAliases(), + ) + } + + @Test + fun `vault survives keys with quotes newlines and unicode`() = + runBlocking { + val weirdAlias = "weird\"key\n\u2603" + val weirdValue = "value with \"quotes\" and \\backslashes and \u2603 snowmen" + val (storage, _) = + newStorageWith { b -> + b.store[CountingKeyring.SERVICE to weirdAlias] = weirdValue + } + storage.enableConsolidatedVault(listOf(weirdAlias)) + assertEquals(weirdValue, storage.getPrivateKey(weirdAlias)) + } +} From e524d38dc58cb4528a28d7af5e7e68b9a8f289bb Mon Sep 17 00:00:00 2001 From: mstrofnone Date: Sat, 12 Sep 2026 14:37:17 +1000 Subject: [PATCH 2/5] fix(desktop): wire two-phase vault bootstrap into AccountManager Runs SecureKeyStorage.enableConsolidatedVault at the top of loadSavedAccount, before any other keychain read on the cold-boot hot path, so migrated setups pay exactly one Keychain Access prompt regardless of how many accounts, per-account bunker-ephemerals, and NWC URIs the user has. Phase 1 migrates only account-metadata-key (the AES key that decrypts accounts.json.enc). Nothing else can be enumerated before that file is readable, so this phase runs against a single-alias candidate list. It is a no-op on fresh installs (no legacy item) and on already-migrated setups (vault-v1 exists). Phase 2 runs after accounts.json.enc has been decrypted and the full npub list is known. For each npub we add the nsec alias itself, the per-account bunker-ephemeral alias, and the NWC alias. The legacy shared bunker-ephemeral alias is included for the pre-per-account migration compatibility branch in loadBunkerAccount. Phase 2 is idempotent so it is safe to run on every startup, including when the vault already covers every alias. DesktopAccountStorage.METADATA_KEY_ALIAS is promoted from private to internal so AccountManager.bootstrapConsolidatedVault can name it without duplicating the alias string. The alias itself, its storage, and its read/write path all still live in DesktopAccountStorage. Bootstrap failures are swallowed: SecureKeyStorage falls back to legacy per-alias reads for anything the vault does not cover, so the worst case is the old two-prompt behaviour. Nothing on the account-load path breaks. No API change on AccountManager (bootstrapConsolidatedVault is private) and no visible behaviour change on the fallback (no-keyring) storage path, which already uses a single encrypted file and does not have the per-item ACL problem the vault exists to solve. --- .../desktop/account/AccountManager.kt | 48 +++++++++++++++++++ .../desktop/account/DesktopAccountStorage.kt | 11 ++++- 2 files changed, 58 insertions(+), 1 deletion(-) diff --git a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManager.kt b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManager.kt index 88c1a30aff..860d529a20 100644 --- a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManager.kt +++ b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManager.kt @@ -259,12 +259,60 @@ class AccountManager internal constructor( // --- Account loading --- + /** + * Two-phase consolidation of every Amethyst-owned keychain item into the + * single `vault-v1` item that [SecureKeyStorage.enableConsolidatedVault] + * manages, so cold-boot triggers at most one macOS Keychain Access prompt + * regardless of how many accounts (each with its own nsec, per-account + * bunker ephemeral, and NWC URI) the user has. + * + * Phase 1 migrates only the `account-metadata-key` (the AES key that + * decrypts `accounts.json.enc`). Nothing else can be enumerated before + * that file is readable, so this phase runs against a single-alias + * candidate list. It is a no-op on fresh installs (no legacy item) and + * on already-migrated setups (vault-v1 exists). + * + * Phase 2 runs after `accounts.json.enc` has been decrypted and the full + * npub list is known. For each npub we add the nsec alias itself, the + * per-account bunker-ephemeral alias, and the NWC alias. The legacy + * shared bunker-ephemeral alias is included for the pre-per-account + * migration compatibility branch in [loadBunkerAccount]. Phase 2 is + * idempotent (see [SecureKeyStorage.enableConsolidatedVault]) so it is + * safe to run on every startup and to include aliases the vault already + * covers. + */ + private suspend fun bootstrapConsolidatedVault() { + try { + secureStorage.enableConsolidatedVault(listOf(DesktopAccountStorage.METADATA_KEY_ALIAS)) + val npubs = accountStorage.loadAccounts().map { it.npub } + val aliases = mutableListOf() + aliases += DesktopAccountStorage.METADATA_KEY_ALIAS + aliases += LEGACY_BUNKER_EPHEMERAL_KEY_ALIAS + for (npub in npubs) { + aliases += npub + aliases += bunkerEphemeralKeyAlias(npub) + aliases += nwcKeyAlias(npub) + } + secureStorage.enableConsolidatedVault(aliases) + } catch (e: kotlin.coroutines.cancellation.CancellationException) { + throw e + } catch (_: Exception) { + // Non-fatal: SecureKeyStorage falls back to legacy per-alias reads + // for anything the vault does not cover. Users see the old two-prompt + // behaviour but nothing breaks. + } + } + suspend fun loadSavedAccount(): Result = try { // Clean up legacy files (one-time) listOf("last_account.txt", "bunker_uri.txt", "nwc_connection.txt") .forEach { File(amethystDir, it).deleteOrWarn("AccountManager", "legacy file") } + // Consolidate keychain items into vault-v1 so macOS prompts once, not per item. + // Runs before any other keychain read on the hot startup path. + bootstrapConsolidatedVault() + // Single source of truth: accounts.json.enc val activeNpub = accountStorage.currentAccount() diff --git a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/DesktopAccountStorage.kt b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/DesktopAccountStorage.kt index e1813ff197..75b1dd7088 100644 --- a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/DesktopAccountStorage.kt +++ b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/DesktopAccountStorage.kt @@ -80,7 +80,16 @@ class DesktopAccountStorage( private val onCorruption: (StorageCorruption) -> Unit = {}, ) : AccountStorage { companion object { - private const val METADATA_KEY_ALIAS = "account-metadata-key" + /** + * Alias under which the AES-256-GCM key that encrypts + * `accounts.json.enc` is stored in [SecureKeyStorage]. + * + * Exposed as `internal` so [AccountManager.bootstrapConsolidatedVault] + * can name it in the phase-1 candidate list. The single source of truth + * for the alias string stays here where the key is actually read and + * written. + */ + internal const val METADATA_KEY_ALIAS = "account-metadata-key" private const val ACCOUNTS_FILE = "accounts.json.enc" private const val ACCOUNTS_LOCK_FILE = "accounts.json.enc.lock" private const val AES_KEY_SIZE = 32 // 256 bits From 919350274897ea8b65bb1a7cb323b13231d72b77 Mon Sep 17 00:00:00 2001 From: Vitor Pamplona Date: Sat, 12 Sep 2026 20:40:24 -0400 Subject: [PATCH 3/5] fix(commons): make the keychain vault authoritative without blinding lookups Three defects found reviewing the vault consolidation against current main. 1. The vault wiped every account on the first cold boot after upgrade. getPrivateKeyOrThrow was the only accessor without a vaultActive branch (savePrivateKey, getPrivateKey and deletePrivateKey all had one). The migration deletes the legacy per-alias items after writing vault-v1, so the strict path probed the OS for an item that no longer existed, read macOS exit 44 / NotFound as "definitively absent", and let DesktopAccountStorage.getOrCreateKey mint a fresh AES key over the one that decrypts accounts.json.enc -- the exact silent wipe proposal 5d31b68e added that method to prevent. It was also unrecoverable: phase 1 preserves the original key inside the vault, but getOrCreateKey then persists the new key over the same alias, destroying the only key that could decrypt the .corrupt backup. And because phase 2 calls loadAccounts(), the wipe happened inside the bootstrap itself, so no nsec was ever folded in. The strict path now consults the vault first. A vault miss still falls through to the strict per-alias probe, so uncovered aliases keep the strict contract. 2. The documented legacy fallback did not exist. getPrivateKey was `vaultActive -> vaultGet(npub)` with no fallback, so once the vault was active any alias it did not cover read as absent. Phase 1 activates it with only the metadata key, and a phase 2 that throws is swallowed, so the real worst case was every nsec reading null rather than the advertised "old two-prompt behaviour". A vault miss now falls back to the legacy per-alias item. 3. deletePrivateKey left orphaned secrets. vaultDelete returned false for an alias outside the vault and never touched the legacy item, so logging out of an account whose nsec had not been folded in left the nsec in the OS keychain indefinitely -- still readable via the fallback in 2. It now unlinks the legacy item as well. Also logs the swallowed vault-bootstrap failure; silently discarding it made a half-migrated keychain impossible to diagnose from a user report. All three are pinned by new tests in SecureKeyStorageVaultTest and mutation-checked: reverting any one fix fails exactly its own test. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VgVDQQXAg4cmzsWHoJj61k --- .../commons/keystorage/SecureKeyStorage.kt | 51 ++++++-- .../keystorage/SecureKeyStorageVaultTest.kt | 109 ++++++++++++++++++ .../desktop/account/AccountManager.kt | 11 +- 3 files changed, 157 insertions(+), 14 deletions(-) diff --git a/commons/src/jvmMain/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorage.kt b/commons/src/jvmMain/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorage.kt index 7218f15129..4f36bd7375 100644 --- a/commons/src/jvmMain/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorage.kt +++ b/commons/src/jvmMain/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorage.kt @@ -160,7 +160,11 @@ actual class SecureKeyStorage private actual constructor() { withContext(Dispatchers.IO) { try { when { - vaultActive -> vaultGet(npub) + // A vault miss is NOT proof of absence: the vault only covers the + // aliases a migration pass was given. Phase 1 activates it with just + // the metadata key, and a failed phase 2 leaves every nsec outside + // it. Fall back to the legacy per-alias item before reporting null. + vaultActive -> vaultGet(npub) ?: getFromKeyring(npub) keyringAvailable -> getFromKeyring(npub) else -> getFromFallback(npub) } @@ -194,6 +198,16 @@ actual class SecureKeyStorage private actual constructor() { actual suspend fun getPrivateKeyOrThrow(npub: String): String? = withContext(Dispatchers.IO) { try { + // The vault is authoritative for every alias it covers. Without this the + // strict path probes the OS for a per-alias item the migration already + // deleted, reads exit 44 / NotFound as "definitively absent", and lets + // DesktopAccountStorage.getOrCreateKey mint a fresh AES key over the one + // that decrypts accounts.json.enc -- the exact silent wipe this method + // exists to prevent. A vault miss still falls through to the strict + // per-alias probe, so uncovered aliases keep the strict contract. + if (vaultActive) { + vaultGet(npub)?.let { return@withContext it } + } if (!keyringAvailable) { return@withContext getFromFallback(npub) } @@ -404,20 +418,37 @@ actual class SecureKeyStorage private actual constructor() { keyring().setPassword(SERVICE_NAME, vaultAlias, encodeVault(contents)) } + /** + * Removes [alias] from the vault *and* unlinks any legacy per-alias item still + * holding it. An alias the vault does not cover (a partial migration, or a + * phase 2 that failed) would otherwise survive a logout as an orphaned secret + * in the OS keychain, since [getPrivateKey] can still read it. + */ private fun vaultDelete(alias: String): Boolean { - val contents = vaultContents ?: return false - val removed = contents.remove(alias) != null - if (!removed) return false - if (contents.isEmpty()) { - return try { - keyring().deletePassword(SERVICE_NAME, vaultAlias) + val contents = vaultContents + val removedFromVault = contents != null && contents.remove(alias) != null + + val removedLegacy = + try { + keyring().deletePassword(SERVICE_NAME, alias) true } catch (_: PasswordAccessException) { - true // already gone, still removed from our POV + false // no legacy item, fine + } + + if (removedFromVault) { + if (contents!!.isEmpty()) { + try { + keyring().deletePassword(SERVICE_NAME, vaultAlias) + } catch (_: PasswordAccessException) { + // already gone, still removed from our POV + } + } else { + keyring().setPassword(SERVICE_NAME, vaultAlias, encodeVault(contents)) } } - keyring().setPassword(SERVICE_NAME, vaultAlias, encodeVault(contents)) - return true + + return removedFromVault || removedLegacy } /** diff --git a/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorageVaultTest.kt b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorageVaultTest.kt index 9093f97af3..54a0b62984 100644 --- a/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorageVaultTest.kt +++ b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorageVaultTest.kt @@ -362,4 +362,113 @@ class SecureKeyStorageVaultTest { storage.enableConsolidatedVault(listOf(weirdAlias)) assertEquals(weirdValue, storage.getPrivateKey(weirdAlias)) } + + // --- Interaction with the strict getOrCreate path (proposal 5d31b68e) --- + + /** + * Wires the strict macOS probe to the fake backend so the test models real + * macOS: `security find-generic-password` sees the OS keychain, returning + * exit 0 (Found) while a per-alias item exists and exit 44 (NotFound) once + * the migration has deleted it. + */ + private fun wireMacProbe( + storage: SecureKeyStorage, + backend: CountingKeyring, + ) { + storage.macSecurityLookup = { service, account -> + backend.store[service to account] + ?.let { MacSecurityResult.Found(it) } + ?: MacSecurityResult.NotFound + } + } + + @Test + fun `strict lookup reads through the vault after migration`() = + runBlocking { + val alias = "account-metadata-key" + val (storage, backend) = + newStorageWith { b -> + b.store[CountingKeyring.SERVICE to alias] = "THE-AES-KEY" + } + wireMacProbe(storage, backend) + + assertEquals("THE-AES-KEY", storage.getPrivateKeyOrThrow(alias)) + + storage.enableConsolidatedVault(listOf(alias)) + + // The migration deleted the per-alias item, so an unvaulted strict probe + // would answer "definitively absent" -- and DesktopAccountStorage would + // mint a fresh AES key over the one that decrypts accounts.json.enc, + // wiping every account. The vault must answer instead. + assertFalse(alias in backend.snapshotAliases()) + assertEquals( + "Strict lookup must read through the vault, not report the migrated alias as absent", + "THE-AES-KEY", + storage.getPrivateKeyOrThrow(alias), + ) + } + + @Test + fun `strict lookup keeps the strict contract for aliases outside the vault`() = + runBlocking { + val (storage, backend) = + newStorageWith { b -> + b.store[CountingKeyring.SERVICE to "account-metadata-key"] = "THE-AES-KEY" + } + storage.enableConsolidatedVault(listOf("account-metadata-key")) + + // An alias the vault never covered still falls through to the strict probe: + // a confirmed miss is null, an ambiguous answer still throws. + wireMacProbe(storage, backend) + assertNull(storage.getPrivateKeyOrThrow("npub1neverseen")) + + storage.macSecurityLookup = { _, _ -> MacSecurityResult.Ambiguous(128, "user cancelled Keychain dialog") } + try { + storage.getPrivateKeyOrThrow("npub1neverseen") + throw AssertionError("Expected SecureStorageException for an ambiguous answer") + } catch (e: SecureStorageException) { + assertTrue(e.message?.contains("cancelled") == true) + } + } + + @Test + fun `getPrivateKey falls back to a legacy item the vault does not cover`() = + runBlocking { + // Phase 1 activates the vault with only the metadata key; a phase 2 that + // never ran (or threw, which AccountManager swallows) leaves every nsec + // outside it. Those must stay readable, not read as absent. + val (storage, _) = + newStorageWith { b -> + b.store[CountingKeyring.SERVICE to "account-metadata-key"] = "THE-AES-KEY" + b.store[CountingKeyring.SERVICE to "npub1someaccount"] = "THE-NSEC" + } + + storage.enableConsolidatedVault(listOf("account-metadata-key")) + + assertTrue(storage.isVaultActive()) + assertEquals("THE-AES-KEY", storage.getPrivateKey("account-metadata-key")) + assertEquals( + "An uncovered alias must fall back to its legacy per-alias item", + "THE-NSEC", + storage.getPrivateKey("npub1someaccount"), + ) + assertTrue(storage.hasPrivateKey("npub1someaccount")) + } + + @Test + fun `delete unlinks a legacy item the vault does not cover`() = + runBlocking { + val (storage, backend) = + newStorageWith { b -> + b.store[CountingKeyring.SERVICE to "account-metadata-key"] = "THE-AES-KEY" + b.store[CountingKeyring.SERVICE to "npub1someaccount"] = "THE-NSEC" + } + storage.enableConsolidatedVault(listOf("account-metadata-key")) + + // Logging out of an account whose nsec never reached the vault must not + // leave the secret orphaned in the OS keychain. + assertTrue(storage.deletePrivateKey("npub1someaccount")) + assertFalse("npub1someaccount" in backend.snapshotAliases()) + assertNull(storage.getPrivateKey("npub1someaccount")) + } } diff --git a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManager.kt b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManager.kt index 860d529a20..ce5c824a23 100644 --- a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManager.kt +++ b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManager.kt @@ -52,6 +52,7 @@ import com.vitorpamplona.quartz.nip19Bech32.toNsec import com.vitorpamplona.quartz.nip46RemoteSigner.BunkerClientMetadata import com.vitorpamplona.quartz.nip46RemoteSigner.signer.NostrSignerRemote import com.vitorpamplona.quartz.nip47WalletConnect.Nip47WalletConnect +import com.vitorpamplona.quartz.utils.Log import com.vitorpamplona.quartz.utils.TimeUtils import kotlinx.collections.immutable.ImmutableList import kotlinx.collections.immutable.persistentListOf @@ -296,10 +297,12 @@ class AccountManager internal constructor( secureStorage.enableConsolidatedVault(aliases) } catch (e: kotlin.coroutines.cancellation.CancellationException) { throw e - } catch (_: Exception) { - // Non-fatal: SecureKeyStorage falls back to legacy per-alias reads - // for anything the vault does not cover. Users see the old two-prompt - // behaviour but nothing breaks. + } catch (e: Exception) { + // Non-fatal: SecureKeyStorage falls back to legacy per-alias reads for + // anything the vault does not cover, so the worst case is the old + // multi-prompt behaviour. Log it -- swallowing this silently made a + // half-migrated keychain impossible to diagnose from a user report. + Log.w("AccountManager", "Consolidated keychain vault bootstrap failed; continuing on legacy per-alias reads", e) } } From 81b9ba853ac2c29429ab9809b635187e2720e512 Mon Sep 17 00:00:00 2001 From: Vitor Pamplona Date: Sat, 12 Sep 2026 21:35:16 -0400 Subject: [PATCH 4/5] fix(desktop): migrate the keychain vault before the first account-store read Found by running the migration against a real macOS Keychain: the first launch migrated and loaded fine, the second launch wiped the account store. bootstrapConsolidatedVault() was called from loadSavedAccount(), and its comment claimed it ran "before any other keychain read on the hot startup path". It did not. Main.kt's startup DisposableEffect calls refreshAccountListOnStartup() first, which reads accounts.json.enc and so needs the metadata AES key. On the first launch that read still found the legacy per-alias item and cached the key, so the migration that followed looked harmless. On the second launch the item was gone -- migrated into vault-v1 and deleted -- and the vault had not been activated yet, so the strict lookup answered "definitively absent", getOrCreateKey minted a fresh AES key, wrote it back as a legacy item, and the decrypt that followed failed with a GCM tag mismatch. Observed exactly that: accounts.json.enc renamed to .corrupt., account-metadata-key resurrected as a per-alias item, and the original key still sitting unused inside vault-v1. Phase 1 is now a run-once, mutex-guarded ensureVaultMetadataKeyMigrated() that every account-store entry point calls, including refreshAccountList(). Phase 2 still runs from loadSavedAccount() once the npub list is known. Verified on macOS against the real Keychain and the real account store: three consecutive launches across the migration boundary all load the account, no corruption events, accounts.json.enc byte-identical throughout. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VgVDQQXAg4cmzsWHoJj61k --- .../desktop/account/AccountManager.kt | 40 ++++++- .../AccountManagerVaultBootstrapOrderTest.kt | 104 ++++++++++++++++++ 2 files changed, 141 insertions(+), 3 deletions(-) create mode 100644 desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManagerVaultBootstrapOrderTest.kt diff --git a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManager.kt b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManager.kt index ce5c824a23..dca6de76dd 100644 --- a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManager.kt +++ b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManager.kt @@ -260,6 +260,36 @@ class AccountManager internal constructor( // --- Account loading --- + /** + * Phase 1 of the vault migration: fold the account-metadata key into `vault-v1`. + * + * This MUST complete before any read that needs that key, because the migration + * deletes the legacy per-alias item. A storage read that gets there first finds + * the item gone and mints a fresh AES key over the one that decrypts + * accounts.json.enc, wiping every account. `refreshAccountListOnStartup()` runs + * before `loadSavedAccount()` on the startup path and does exactly that read, so + * phase 1 is hoisted here and every storage entry point calls it. + * + * Run-once and idempotent: the flag is set inside the lock, so concurrent + * callers serialise and only the first does the work. + */ + private suspend fun ensureVaultMetadataKeyMigrated() { + vaultBootstrapMutex.withLock { + if (vaultMetadataKeyMigrated) return@withLock + vaultMetadataKeyMigrated = true + try { + secureStorage.enableConsolidatedVault(listOf(DesktopAccountStorage.METADATA_KEY_ALIAS)) + } catch (e: kotlin.coroutines.cancellation.CancellationException) { + throw e + } catch (e: Exception) { + Log.w("AccountManager", "Consolidated keychain vault phase 1 failed; continuing on legacy per-alias reads", e) + } + } + } + + private val vaultBootstrapMutex = Mutex() + private var vaultMetadataKeyMigrated = false + /** * Two-phase consolidation of every Amethyst-owned keychain item into the * single `vault-v1` item that [SecureKeyStorage.enableConsolidatedVault] @@ -283,8 +313,8 @@ class AccountManager internal constructor( * covers. */ private suspend fun bootstrapConsolidatedVault() { + ensureVaultMetadataKeyMigrated() try { - secureStorage.enableConsolidatedVault(listOf(DesktopAccountStorage.METADATA_KEY_ALIAS)) val npubs = accountStorage.loadAccounts().map { it.npub } val aliases = mutableListOf() aliases += DesktopAccountStorage.METADATA_KEY_ALIAS @@ -312,8 +342,9 @@ class AccountManager internal constructor( listOf("last_account.txt", "bunker_uri.txt", "nwc_connection.txt") .forEach { File(amethystDir, it).deleteOrWarn("AccountManager", "legacy file") } - // Consolidate keychain items into vault-v1 so macOS prompts once, not per item. - // Runs before any other keychain read on the hot startup path. + // Consolidate keychain items into vault-v1 so macOS prompts once, not per + // item. Phase 1 may already have run via refreshAccountListOnStartup(); + // it is run-once, so this call just adds phase 2. bootstrapConsolidatedVault() // Single source of truth: accounts.json.enc @@ -805,6 +836,9 @@ class AccountManager internal constructor( // --- Multi-account management --- suspend fun refreshAccountList() { + // Reads accounts.json.enc, which needs the metadata key -- so the vault + // migration has to have happened first. See [ensureVaultMetadataKeyMigrated]. + ensureVaultMetadataKeyMigrated() _allAccounts.value = accountStorage.loadAccounts().toImmutableList() } diff --git a/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManagerVaultBootstrapOrderTest.kt b/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManagerVaultBootstrapOrderTest.kt new file mode 100644 index 0000000000..a9c341edce --- /dev/null +++ b/desktopApp/src/jvmTest/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManagerVaultBootstrapOrderTest.kt @@ -0,0 +1,104 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.amethyst.desktop.account + +import com.vitorpamplona.amethyst.commons.keystorage.SecureKeyStorage +import com.vitorpamplona.amethyst.commons.model.account.AccountInfo +import com.vitorpamplona.amethyst.commons.model.account.SignerType +import io.mockk.coEvery +import io.mockk.coVerify +import io.mockk.coVerifyOrder +import io.mockk.mockk +import kotlinx.coroutines.test.runTest +import java.io.File +import kotlin.io.path.createTempDirectory +import kotlin.test.AfterTest +import kotlin.test.BeforeTest +import kotlin.test.Test + +/** + * The vault migration deletes the legacy `account-metadata-key` item. Any read of + * accounts.json.enc that beats the migration therefore finds the key gone, mints a + * fresh AES key over the one that decrypts the file, and wipes every account. + * + * `Main.kt` calls [AccountManager.refreshAccountListOnStartup] *before* + * [AccountManager.loadSavedAccount], so hanging the migration off loadSavedAccount + * alone is too late -- this was reproduced on macOS against a real Keychain: the + * first launch migrated fine, the second launch wiped the account store. + */ +class AccountManagerVaultBootstrapOrderTest { + private lateinit var storage: SecureKeyStorage + private lateinit var tempDir: File + private lateinit var manager: AccountManager + private val keyStore = mutableMapOf() + + @BeforeTest + fun setup() { + storage = mockk(relaxed = true) + coEvery { storage.savePrivateKey(any(), any()) } answers { keyStore[firstArg()] = secondArg() } + coEvery { storage.getPrivateKey(any()) } answers { keyStore[firstArg()] } + coEvery { storage.getPrivateKeyOrThrow(any()) } answers { keyStore[firstArg()] } + tempDir = createTempDirectory("acctmgr-vault-order").toFile() + File(tempDir, ".amethyst").mkdirs() + manager = AccountManager(storage, tempDir) + } + + /** + * An empty store short-circuits before it ever needs the metadata key, so seed a + * real accounts.json.enc -- otherwise the ordering under test is never exercised. + */ + private suspend fun seedAccountStore() { + DesktopAccountStorage(storage, tempDir) + .saveAccount(AccountInfo("npub1seeded", SignerType.Internal)) + } + + @AfterTest + fun teardown() { + tempDir.deleteRecursively() + } + + @Test + fun `startup list refresh migrates the vault before touching the account store`() = + runTest { + seedAccountStore() + + manager.refreshAccountListOnStartup() + + coVerifyOrder { + storage.enableConsolidatedVault(listOf("account-metadata-key")) + storage.getPrivateKeyOrThrow("account-metadata-key") + } + } + + @Test + fun `phase one runs once even across both startup entry points`() = + runTest { + seedAccountStore() + + manager.refreshAccountListOnStartup() + manager.refreshAccountList() + runCatching { manager.loadSavedAccount() } + + coVerify(exactly = 1) { + storage.enableConsolidatedVault(listOf("account-metadata-key")) + } + } +} From 94a229e983e090a37fd7b89354c92a8521b49bc2 Mon Sep 17 00:00:00 2001 From: Vitor Pamplona Date: Sat, 12 Sep 2026 21:52:31 -0400 Subject: [PATCH 5/5] style(desktop): import CancellationException instead of inlining its name The vault bootstrap added two more inline kotlin.coroutines.cancellation.CancellationException references next to two existing ones; CLAUDE.md forbids fully-qualified names in function bodies. One import, no behaviour change. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01WHCtgMDNkAHvRDXnthuXQc --- .../amethyst/desktop/account/AccountManager.kt | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManager.kt b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManager.kt index dca6de76dd..02b582fc95 100644 --- a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManager.kt +++ b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManager.kt @@ -70,6 +70,7 @@ import kotlinx.coroutines.sync.Mutex import kotlinx.coroutines.sync.withLock import kotlinx.coroutines.withTimeout import java.io.File +import kotlin.coroutines.cancellation.CancellationException sealed class AccountState { data object Loading : AccountState() @@ -279,7 +280,7 @@ class AccountManager internal constructor( vaultMetadataKeyMigrated = true try { secureStorage.enableConsolidatedVault(listOf(DesktopAccountStorage.METADATA_KEY_ALIAS)) - } catch (e: kotlin.coroutines.cancellation.CancellationException) { + } catch (e: CancellationException) { throw e } catch (e: Exception) { Log.w("AccountManager", "Consolidated keychain vault phase 1 failed; continuing on legacy per-alias reads", e) @@ -325,7 +326,7 @@ class AccountManager internal constructor( aliases += nwcKeyAlias(npub) } secureStorage.enableConsolidatedVault(aliases) - } catch (e: kotlin.coroutines.cancellation.CancellationException) { + } catch (e: CancellationException) { throw e } catch (e: Exception) { // Non-fatal: SecureKeyStorage falls back to legacy per-alias reads for @@ -362,7 +363,7 @@ class AccountManager internal constructor( is SignerType.Remote -> loadBunkerAccount((info.signerType as SignerType.Remote).bunkerUri, activeNpub) is SignerType.ViewOnly -> loadReadOnlyAccount(activeNpub) } - } catch (e: kotlin.coroutines.cancellation.CancellationException) { + } catch (e: CancellationException) { throw e } catch (e: Exception) { Result.failure(e) @@ -1009,7 +1010,7 @@ class AccountManager internal constructor( _nwcConnection.value = parsed Result.success(parsed) - } catch (e: kotlin.coroutines.cancellation.CancellationException) { + } catch (e: CancellationException) { throw e } catch (e: Exception) { Result.failure(e)