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 cb3731be4c..926135932f 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 @@ -96,7 +96,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 @@ -113,13 +113,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) @@ -137,10 +163,14 @@ 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 { + // 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) } } catch (e: BackendNotSupportedException) { keyringAvailable = false @@ -172,6 +202,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) } @@ -225,10 +265,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 @@ -240,6 +280,375 @@ 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)) + } + + /** + * 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 + val removedFromVault = contents != null && contents.remove(alias) != null + + val removedLegacy = + try { + keyring().deletePassword(SERVICE_NAME, alias) + true + } catch (_: PasswordAccessException) { + 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)) + } + } + + return removedFromVault || removedLegacy + } + + /** + * 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..54a0b62984 --- /dev/null +++ b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorageVaultTest.kt @@ -0,0 +1,474 @@ +/* + * 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)) + } + + // --- 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 88c1a30aff..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 @@ -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 @@ -69,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() @@ -259,12 +261,93 @@ 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: 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] + * 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() { + ensureVaultMetadataKeyMigrated() + try { + 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: CancellationException) { + throw e + } 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) + } + } + 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. 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 val activeNpub = accountStorage.currentAccount() @@ -280,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) @@ -754,6 +837,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() } @@ -924,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) 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 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")) + } + } +}