mirror of
https://github.com/vitorpamplona/amethyst.git
synced 2026-09-14 00:55:08 +00:00
Merge PR: fix(desktop): consolidate keychain items so cold-boot prompts once
Merges nostr proposal 1c931194 (v4) into main: - fix(commons): consolidate desktop keychain items into a single vault-v1 item - fix(desktop): wire two-phase vault bootstrap into AccountManager - fix(commons): make the keychain vault authoritative without blinding lookups - fix(desktop): migrate the keychain vault before the first account-store read - style(desktop): import CancellationException instead of inlining its name Every nsec, bunker ephemeral and NWC URI now lives in one vault-v1 keychain item behind a single ACL, so cold boot prompts once. One approval releases every secret; this single-ACL model is an accepted maintainer decision. v2-v4 fixed two account-store wipes (strict lookup ignoring the vault; migration running after refreshAccountListOnStartup's first read), a missing legacy fallback, and orphaned nsecs on logout. Verified with 17 mutation-checked tests and three consecutive launches against a real macOS Keychain. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WHCtgMDNkAHvRDXnthuXQc
This commit is contained in:
Vitor Pamplona
Claude Opus 5
+419
-10
@@ -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<String, String>? = 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,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
|
||||
@@ -168,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)
|
||||
}
|
||||
@@ -221,10 +261,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 +276,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<String>) {
|
||||
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<String, String>()
|
||||
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<String, String>()
|
||||
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<String> = vaultContents?.keys?.toSet() ?: emptySet()
|
||||
|
||||
private fun vaultGet(alias: String): String? = vaultContents?.get(alias)
|
||||
|
||||
private fun vaultPut(
|
||||
alias: String,
|
||||
value: String,
|
||||
) {
|
||||
val contents = vaultContents ?: LinkedHashMap<String, String>().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, String>): 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<String, String> {
|
||||
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<String, String> {
|
||||
val out = LinkedHashMap<String, String>()
|
||||
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
|
||||
|
||||
/**
|
||||
|
||||
+474
@@ -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<Pair<String, String>, String> = ConcurrentHashMap()
|
||||
val readsBySlot: ConcurrentHashMap<Pair<String, String>, AtomicInteger> = ConcurrentHashMap()
|
||||
val writesBySlot: ConcurrentHashMap<Pair<String, String>, 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<String> =
|
||||
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<SecureKeyStorage, CountingKeyring> {
|
||||
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, String>): 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"))
|
||||
}
|
||||
}
|
||||
+88
-2
@@ -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<String>()
|
||||
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<AccountState.LoggedIn> =
|
||||
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)
|
||||
|
||||
+10
-1
@@ -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
|
||||
|
||||
+104
@@ -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<String, String>()
|
||||
|
||||
@BeforeTest
|
||||
fun setup() {
|
||||
storage = mockk(relaxed = true)
|
||||
coEvery { storage.savePrivateKey(any(), any()) } answers { keyStore[firstArg()] = secondArg() }
|
||||
coEvery { storage.getPrivateKey(any()) } answers { keyStore[firstArg<String>()] }
|
||||
coEvery { storage.getPrivateKeyOrThrow(any()) } answers { keyStore[firstArg<String>()] }
|
||||
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"))
|
||||
}
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user