fix(desktop): stop silent account wipe on keychain errors and upgrade races

Three independent bugs in DesktopAccountStorage / SecureKeyStorage could
turn one ambiguous macOS Keychain reply, one transient read error, or
one Homebrew upgrade race into permanent account-metadata loss on
~/.amethyst/accounts.json.enc.

1. Silent AES metadata-key rotation on ambiguous keychain miss.
   getOrCreateKey() treated null from getPrivateKey("account-metadata-
   key") as "no key exists" and generated a fresh AES key. On macOS
   javakeyring collapses errSecItemNotFound (-25300), errSecAuthFailed
   (-25293), errSecUserCanceled (-128), and errSecInteractionNotAllowed
   (-25308) into the same PasswordAccessException; getFromKeyring turns
   them all into null. A single Deny click on the OS Keychain dialog
   silently rotated the AES key and destroyed the ability to decrypt
   the existing accounts.json.enc.

   Fix: add a new strict SecureKeyStorage.getPrivateKeyOrThrow(npub)
   on the common expect. On JVM/macOS it wraps /usr/bin/security
   find-generic-password whose exit codes (0 = found, 44 = not found,
   others = ambiguous) are documented and unambiguous. On JVM
   Windows/Linux it uses javakeyring but throws on any
   PasswordAccessException from the strict path. On Android it uses
   EncryptedSharedPreferences.contains(). On iOS it mirrors the
   existing "pending (iOS Phase 4)" stub. getOrCreateKey now calls
   getPrivateKeyOrThrow and propagates SecureStorageException without
   ever rotating the key. The permissive getPrivateKey(npub) is
   unchanged; its callers (per-account nsec, ephemeral bunker keys)
   still tolerate null on any error.

2. Any read failure resets the file. readMetadataFromDisk() used to
   rename to accounts.json.enc.corrupt.<ts> and return empty
   AccountMetadata() on any exception, including transient IO and
   the newly-throwing keychain path from bug 1.

   Fix: distinguish exception types.
   - AEADBadTagException / BadPaddingException: back up to
     .corrupt.<ts>, reset, fire StorageCorruption.FileCorrupted
     (unchanged).
   - JacksonException: back up but to .jsonerror.<ts> so it is
     distinguishable from ciphertext corruption; fire
     StorageCorruption.JsonMalformed.
   - Anything else (IO error, OOM, thrown keychain path): do NOT
     rename; rethrow to caller and fire a new
     StorageCorruption.TransientError(cause) subtype. The file stays
     untouched. AccountManager.loadSavedAccount already wraps in
     try/catch and turns the throw into Result.failure.
   - Truncated file (size < GCM IV size): still backup + reset,
     genuinely unusable.

3. No cross-process advisory lock. Homebrew replacing the .app while
   the old process is mid-save, or an accidental double-launch of
   Compose Desktop (no built-in single-instance guard), could produce
   a truncated file that trips bug 2.

   Fix: withAccountsFileLock helper (mirrors SecureKeyStorage.
   withFileLock) wraps read + write in a
   RandomAccessFile(lockFile, "rw").channel.lock() on
   ~/.amethyst/accounts.json.enc.lock (0600). Because FileChannel.lock
   is per-JVM, an in-process Mutex is held before acquiring the
   channel lock. A separate stateMutex guards the read-modify-write
   cycle in saveAccount / deleteAccount / setCurrentAccount so two
   concurrent writers cannot each read the same base metadata and
   each rewrite it.

Backward compatibility: existing keychain items are read unchanged;
no schema migration for accounts.json.enc; the file lock adds a
.lock sidecar older builds ignore.

Tests: new SecureKeyStorageOrThrowTest (pure exit-code parser,
mac lookup Found/NotFound/Ambiguous, non-mac keyring hit and
throw-on-PasswordAccessException). DesktopAccountStorageTest gains
five cases: getOrCreateKey ambiguous-error preserves file and does
not rotate; getOrCreateKey definitive-not-found happy path;
readMetadataFromDisk transient-IO preserves file with no backup
sibling; GCM tag mismatch keeps .corrupt.<ts> backup; JSON malformed
uses new .jsonerror.<ts> suffix; eight concurrent saveAccount calls
serialize under the file lock with no lost updates. All existing
AccountManager* MockK setups extended to also stub
getPrivateKeyOrThrow.

Local verify: :desktopApp:test + :commons:jvmTest, 2490 tests, all
pass. Spotless clean.
This commit is contained in:
mstrofnone
2026-09-12 17:28:09 -04:00
committed by Vitor Pamplona
parent 08a3bab605
commit 285a51e98f
15 changed files with 777 additions and 41 deletions
@@ -107,6 +107,26 @@ actual class SecureKeyStorage private actual constructor() {
}
}
/**
* Android backend: EncryptedSharedPreferences.contains + getString has no
* ambiguous-error state comparable to macOS Keychain user-cancel/deny, so
* "key not present" and "key present" are the only two null outcomes.
* Any thrown exception is a genuine failure and propagates.
*/
actual suspend fun getPrivateKeyOrThrow(npub: String): String? =
withContext(Dispatchers.IO) {
try {
val key = KEY_PREFIX + npub
if (!encryptedPrefs.contains(key)) {
null
} else {
encryptedPrefs.getString(key, null)
}
} catch (e: Exception) {
throw SecureStorageException("Failed to retrieve private key", e)
}
}
actual suspend fun deletePrivateKey(npub: String): Boolean =
withContext(Dispatchers.IO) {
try {
@@ -76,12 +76,34 @@ expect class SecureKeyStorage private constructor() {
* **Security Warning:** The returned String cannot be securely zeroed from memory (JVM limitation).
* Dereference the returned value immediately after use to minimize exposure time.
*
* Callers that MUST distinguish "key does not exist" from "backend refused/locked/failed"
* (for example, before generating a replacement key on disk) should use
* [getPrivateKeyOrThrow] instead. This method returns null on any error and cannot
* safely be used as an "is this the first launch?" probe.
*
* @param npub The public key in npub (Bech32) format
* @return The private key in hexadecimal format, or null if not found
* @throws SecureStorageException if retrieval operation fails
*/
suspend fun getPrivateKey(npub: String): String?
/**
* Retrieves a private key for the given npub, distinguishing "definitively absent"
* from any other failure mode.
*
* On success, returns the key. When the backend confirms the item does not exist,
* returns null. Any other outcome (backend unavailable, user denied the OS prompt,
* keychain locked, I/O error) throws [SecureStorageException]. This is the safe
* primitive for compare-and-swap style flows where a null must not be interpreted
* as permission to generate and persist a replacement.
*
* @param npub The public key in npub (Bech32) format
* @return The private key in hexadecimal format, or null only when the backend
* confirms the item does not exist
* @throws SecureStorageException on any ambiguous or transient failure
*/
suspend fun getPrivateKeyOrThrow(npub: String): String?
/**
* Deletes a private key for the given npub.
*
@@ -34,6 +34,8 @@ actual class SecureKeyStorage private actual constructor() {
actual suspend fun getPrivateKey(npub: String): String? = throw SecureStorageException("Keychain Services binding pending (iOS Phase 4)")
actual suspend fun getPrivateKeyOrThrow(npub: String): String? = throw SecureStorageException("Keychain Services binding pending (iOS Phase 4)")
actual suspend fun deletePrivateKey(npub: String): Boolean = throw SecureStorageException("Keychain Services binding pending (iOS Phase 4)")
actual suspend fun hasPrivateKey(npub: String): Boolean = throw SecureStorageException("Keychain Services binding pending (iOS Phase 4)")
@@ -149,6 +149,75 @@ actual class SecureKeyStorage private actual constructor() {
}
}
/**
* Strict variant that distinguishes "backend confirms item not found" from every
* other outcome. This matters on macOS: `javakeyring` collapses `errSecItemNotFound`
* (-25300), `errSecAuthFailed` (-25293), `errSecUserCanceled` (-128), and
* `errSecInteractionNotAllowed` (-25308) into the same `PasswordAccessException`.
* A caller that mistook "user clicked Deny" for "first launch, generate a fresh
* key" would silently rotate the metadata AES key and permanently destroy the
* accounts.json.enc it was supposed to unlock.
*
* On macOS this shells out to `/usr/bin/security find-generic-password`, whose
* exit codes are documented and unambiguous (44 = not found, 128 = user cancel /
* dialog dismissed, others = backend failure). On Windows / Linux, javakeyring
* has no such ambiguity for the equivalent flows in practice, but we still treat
* any `PasswordAccessException` here as ambiguous (throw) to keep the contract
* strict on the getOrCreate path.
*/
actual suspend fun getPrivateKeyOrThrow(npub: String): String? =
withContext(Dispatchers.IO) {
try {
if (!keyringAvailable) {
return@withContext getFromFallback(npub)
}
if (isMacOs()) {
return@withContext getFromMacSecurityCli(SERVICE_NAME, npub)
}
try {
keyring().getPassword(SERVICE_NAME, npub)
} catch (e: PasswordAccessException) {
// Non-mac backends: keep the strict contract by refusing to
// treat this as "definitively absent". A caller that needs a
// permissive lookup should use getPrivateKey() instead.
throw SecureStorageException(
"Keyring backend refused access or returned ambiguous not-found",
e,
)
}
} catch (e: SecureStorageException) {
throw e
} catch (e: BackendNotSupportedException) {
keyringAvailable = false
println("OS keyring not available, using fallback encrypted storage")
getFromFallback(npub)
} catch (e: Exception) {
throw SecureStorageException("Failed to retrieve private key (strict)", e)
}
}
/**
* Test seam: overridable strategy for the strict macOS lookup. Production wires
* to [defaultMacSecurityLookup] which spawns `/usr/bin/security`. Tests replace
* this with a stub so unit tests run hermetically on any OS.
*/
internal var macSecurityLookup: (String, String) -> MacSecurityResult =
::defaultMacSecurityLookup
private fun getFromMacSecurityCli(
service: String,
account: String,
): String? {
val result = macSecurityLookup(service, account)
return when (result) {
is MacSecurityResult.Found -> result.password
is MacSecurityResult.NotFound -> null
is MacSecurityResult.Ambiguous -> throw SecureStorageException(
"macOS Keychain access failed (${result.reason}, exit=${result.exitCode})",
)
}
}
actual suspend fun deletePrivateKey(npub: String): Boolean =
withContext(Dispatchers.IO) {
try {
@@ -466,6 +535,98 @@ internal interface KeyringHandle {
)
}
/**
* Outcome of a strict macOS `/usr/bin/security find-generic-password` lookup.
* Kept as a sealed hierarchy so [SecureKeyStorage.getPrivateKeyOrThrow] can
* cleanly translate to `null` versus `SecureStorageException`.
*/
internal sealed class MacSecurityResult {
data class Found(
val password: String,
) : MacSecurityResult()
object NotFound : MacSecurityResult()
/**
* Any exit code other than 0 (found) or 44 (item not found). Reason is a short
* human string derived from stderr / documented codes:
* 128 = user cancelled or dismissed the Keychain Access dialog
* -25293 (errSecAuthFailed) surfaces as exit 51 in practice
* -25308 (errSecInteractionNotAllowed) surfaces when Keychain is locked
*/
data class Ambiguous(
val exitCode: Int,
val reason: String,
) : MacSecurityResult()
}
/**
* Pure parser split out for testability on non-macOS CI runners. Maps the
* documented exit code contract of `/usr/bin/security find-generic-password`
* to a [MacSecurityResult]. `stdout` is the raw password body (`-w` prints it
* followed by a newline; strip the trailing newline only). `stderr` is used
* as a hint for the ambiguous [MacSecurityResult.Ambiguous.reason] string.
*/
internal fun parseMacSecurityFindResult(
exitCode: Int,
stdout: String,
stderr: String,
): MacSecurityResult =
when (exitCode) {
0 -> MacSecurityResult.Found(stdout.trimEnd('\n', '\r'))
44 -> MacSecurityResult.NotFound
else -> {
val reason =
when {
exitCode == 128 -> "user cancelled Keychain dialog"
stderr.contains("-25293") -> "errSecAuthFailed"
stderr.contains("-25308") -> "errSecInteractionNotAllowed"
stderr.contains("-128") -> "user cancelled Keychain dialog"
stderr.isNotBlank() ->
stderr
.lineSequence()
.first()
.trim()
.take(120)
else -> "unknown"
}
MacSecurityResult.Ambiguous(exitCode, reason)
}
}
private fun isMacOs(): Boolean = System.getProperty("os.name").orEmpty().startsWith("Mac")
/**
* Production implementation: spawn `/usr/bin/security` and read exit code + streams.
* Kept package-private so tests can also reach it if they want to run the real path
* on a mac host, but production always goes through the [SecureKeyStorage.macSecurityLookup]
* indirection.
*/
internal fun defaultMacSecurityLookup(
service: String,
account: String,
): MacSecurityResult {
val process =
try {
ProcessBuilder(
"/usr/bin/security",
"find-generic-password",
"-s",
service,
"-a",
account,
"-w",
).redirectErrorStream(false).start()
} catch (e: Exception) {
return MacSecurityResult.Ambiguous(-1, "failed to spawn /usr/bin/security: ${e.message ?: e::class.simpleName ?: "unknown"}")
}
process.outputStream.close()
val stdout = process.inputStream.bufferedReader().use { it.readText() }
val stderr = process.errorStream.bufferedReader().use { it.readText() }
val exitCode = process.waitFor()
return parseMacSecurityFindResult(exitCode, stdout, stderr)
}
internal class RealKeyringHandle(
private val keyring: Keyring,
) : KeyringHandle {
@@ -0,0 +1,212 @@
/*
* 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.assertNull
import org.junit.Assert.assertTrue
import org.junit.Assert.fail
import org.junit.Test
/**
* Unit tests for the strict `getPrivateKeyOrThrow` lookup and its pure macOS
* `/usr/bin/security` exit-code parser.
*
* These tests must never touch the OS keychain and must run on any host, so:
* - the macOS integration paths are gated behind [MacSecurityResult] stubs
* injected via `SecureKeyStorage.macSecurityLookup`;
* - the parser test operates on captured stdout/stderr/exitCode triples;
* - non-mac backends are exercised via the `KeyringHandle` test seam already
* used by [SecureKeyStorageKeyringCacheTest].
*/
class SecureKeyStorageOrThrowTest {
private class ExplodingKeyring(
private val onGet: () -> Nothing,
) : KeyringHandle {
override fun getPassword(
service: String,
account: String,
): String = onGet()
override fun setPassword(
service: String,
account: String,
password: String,
) {
// unused in these tests
}
override fun deletePassword(
service: String,
account: String,
) {
// unused in these tests
}
}
private class StaticKeyring(
private val map: Map<Pair<String, String>, String>,
) : KeyringHandle {
override fun getPassword(
service: String,
account: String,
): String = map[service to account] ?: throw PasswordAccessException("no entry")
override fun setPassword(
service: String,
account: String,
password: String,
) {}
override fun deletePassword(
service: String,
account: String,
) {}
}
// --- macOS security(1) parser ---
@Test
fun `parser exit 0 returns Found with trimmed password`() {
val result = parseMacSecurityFindResult(0, "hunter2\n", "")
assertTrue(result is MacSecurityResult.Found)
assertEquals("hunter2", (result as MacSecurityResult.Found).password)
}
@Test
fun `parser exit 0 preserves internal newlines and only strips trailing`() {
val result = parseMacSecurityFindResult(0, "line1\nline2\n", "")
assertEquals("line1\nline2", (result as MacSecurityResult.Found).password)
}
@Test
fun `parser exit 44 returns NotFound`() {
val result =
parseMacSecurityFindResult(
44,
"",
"security: SecKeychainSearchCopyNext: The specified item could not be found in the keychain.\n",
)
assertTrue(result is MacSecurityResult.NotFound)
}
@Test
fun `parser exit 128 flagged as user-cancelled`() {
val result = parseMacSecurityFindResult(128, "", "security: dismissed\n")
assertTrue(result is MacSecurityResult.Ambiguous)
val ambig = result as MacSecurityResult.Ambiguous
assertEquals(128, ambig.exitCode)
assertTrue(ambig.reason.contains("cancel"))
}
@Test
fun `parser stderr -25293 mapped to errSecAuthFailed`() {
val result = parseMacSecurityFindResult(51, "", "security: SecKeychainItemCopyContent (-25293)\n")
val ambig = result as MacSecurityResult.Ambiguous
assertEquals("errSecAuthFailed", ambig.reason)
}
@Test
fun `parser unknown exit falls back to first stderr line`() {
val result = parseMacSecurityFindResult(9999, "", "security: mystery: line 1\nline 2\n")
val ambig = result as MacSecurityResult.Ambiguous
assertEquals("security: mystery: line 1", ambig.reason)
}
// --- getPrivateKeyOrThrow: strict semantics via injected macOS lookup ---
// (Enabled unconditionally: the macSecurityLookup indirection is exercised
// via a stub, so no `security` binary is invoked. The `isMacOs()` check
// means this test only takes the mac path on macOS runners; on Linux it
// takes the javakeyring path, which we validate separately below.)
private fun newStorage(): SecureKeyStorage = SecureKeyStorage.create()
@Test
fun `mac lookup Found returns password without ambiguity`() =
runBlocking {
if (!System.getProperty("os.name").orEmpty().startsWith("Mac")) return@runBlocking
val storage = newStorage()
storage.macSecurityLookup = { _, _ -> MacSecurityResult.Found("secretval") }
assertEquals("secretval", storage.getPrivateKeyOrThrow("account-metadata-key"))
}
@Test
fun `mac lookup NotFound returns null`() =
runBlocking {
if (!System.getProperty("os.name").orEmpty().startsWith("Mac")) return@runBlocking
val storage = newStorage()
storage.macSecurityLookup = { _, _ -> MacSecurityResult.NotFound }
assertNull(storage.getPrivateKeyOrThrow("account-metadata-key"))
}
@Test
fun `mac lookup Ambiguous throws SecureStorageException with reason`() =
runBlocking {
if (!System.getProperty("os.name").orEmpty().startsWith("Mac")) return@runBlocking
val storage = newStorage()
storage.macSecurityLookup = { _, _ ->
MacSecurityResult.Ambiguous(128, "user cancelled Keychain dialog")
}
try {
storage.getPrivateKeyOrThrow("account-metadata-key")
fail("Expected SecureStorageException")
} catch (e: SecureStorageException) {
assertTrue(e.message?.contains("user cancelled") == true)
assertTrue(e.message?.contains("128") == true)
}
}
// --- non-mac backend: PasswordAccessException must throw, never null ---
@Test
fun `non-mac keyring PasswordAccessException throws not returns null`() =
runBlocking {
if (System.getProperty("os.name").orEmpty().startsWith("Mac")) return@runBlocking
val storage = newStorage()
storage.keyringFactory = { ExplodingKeyring { throw PasswordAccessException("locked") } }
try {
storage.getPrivateKeyOrThrow("account-metadata-key")
fail("Expected SecureStorageException")
} catch (e: SecureStorageException) {
assertTrue(
e.message?.contains("ambiguous", ignoreCase = true) == true ||
e.message?.contains("refused", ignoreCase = true) == true,
)
}
}
@Test
fun `non-mac keyring hit returns password`() =
runBlocking {
if (System.getProperty("os.name").orEmpty().startsWith("Mac")) return@runBlocking
val storage = newStorage()
storage.keyringFactory = {
StaticKeyring(
mapOf(
("amethyst-desktop" to "account-metadata-key") to "abc123",
),
)
}
assertEquals("abc123", storage.getPrivateKeyOrThrow("account-metadata-key"))
}
}
@@ -28,7 +28,10 @@ import com.vitorpamplona.amethyst.commons.model.account.AccountStorage
import com.vitorpamplona.amethyst.commons.model.account.SignerType
import com.vitorpamplona.amethyst.commons.util.deleteOrWarn
import com.vitorpamplona.quartz.utils.Log
import kotlinx.coroutines.sync.Mutex
import kotlinx.coroutines.sync.withLock
import java.io.File
import java.io.RandomAccessFile
import java.nio.file.Files
import java.nio.file.StandardCopyOption
import java.nio.file.attribute.PosixFilePermission
@@ -58,6 +61,16 @@ sealed class StorageCorruption(
class JsonMalformed(
backupPath: String?,
) : StorageCorruption(backupPath)
/**
* A transient failure surfaced from the read path (I/O error, keychain refused
* or otherwise ambiguous access, OOM, etc). No backup was written and the
* on-disk file is untouched. Callers should retry or surface an error UI rather
* than treating this as data loss. See [DesktopAccountStorage.readMetadataFromDisk].
*/
class TransientError(
val cause: Throwable,
) : StorageCorruption(backupPath = null)
}
class DesktopAccountStorage(
@@ -68,6 +81,7 @@ class DesktopAccountStorage(
companion object {
private 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
private const val GCM_IV_SIZE = 12
private const val GCM_TAG_BITS = 128
@@ -76,38 +90,51 @@ class DesktopAccountStorage(
private val mapper = jacksonObjectMapper()
private val amethystDir by lazy { File(homeDir, ".amethyst") }
// In-memory cache — read from disk once, then serve from memory
// In-memory cache: read from disk once, then serve from memory
private var cachedMetadata: AccountMetadata? = null
// In-process mutex around the cross-process file lock. Two callers inside
// the same JVM would otherwise fail with OverlappingFileLockException from
// FileChannel.lock(), since JVM file locks are per-JVM not per-thread.
private val fileLockMutex = Mutex()
// Guards read-modify-write cycles on [cachedMetadata]. Distinct from
// [fileLockMutex] so we can hold it across a full read + mutate + write
// sequence (the file lock is taken and released inside each disk op).
private val stateMutex = Mutex()
// --- AccountStorage interface ---
override suspend fun loadAccounts(): List<AccountInfo> = getCachedMetadata().accounts.map { it.toAccountInfo() }
override suspend fun saveAccount(info: AccountInfo) {
val metadata = getCachedMetadata()
val dto = AccountInfoDto.from(info)
val updated = metadata.accounts.filter { it.npub != info.npub } + dto
writeCachedMetadata(metadata.copy(accounts = updated))
}
override suspend fun saveAccount(info: AccountInfo) =
stateMutex.withLock {
val metadata = getCachedMetadata()
val dto = AccountInfoDto.from(info)
val updated = metadata.accounts.filter { it.npub != info.npub } + dto
writeCachedMetadata(metadata.copy(accounts = updated))
}
override suspend fun deleteAccount(npub: String) {
val metadata = getCachedMetadata()
val updated = metadata.accounts.filter { it.npub != npub }
val newActive =
if (metadata.activeNpub == npub) {
updated.firstOrNull()?.npub
} else {
metadata.activeNpub
}
writeCachedMetadata(metadata.copy(accounts = updated, activeNpub = newActive))
}
override suspend fun deleteAccount(npub: String) =
stateMutex.withLock {
val metadata = getCachedMetadata()
val updated = metadata.accounts.filter { it.npub != npub }
val newActive =
if (metadata.activeNpub == npub) {
updated.firstOrNull()?.npub
} else {
metadata.activeNpub
}
writeCachedMetadata(metadata.copy(accounts = updated, activeNpub = newActive))
}
override suspend fun currentAccount(): String? = getCachedMetadata().activeNpub
override suspend fun setCurrentAccount(npub: String) {
val metadata = getCachedMetadata()
writeCachedMetadata(metadata.copy(activeNpub = npub))
}
override suspend fun setCurrentAccount(npub: String) =
stateMutex.withLock {
val metadata = getCachedMetadata()
writeCachedMetadata(metadata.copy(activeNpub = npub))
}
// --- Cached I/O ---
@@ -129,9 +156,17 @@ class DesktopAccountStorage(
val file = getAccountsFile()
if (!file.exists()) return AccountMetadata()
ensureDir()
return withAccountsFileLock {
readMetadataFromDiskLocked(file)
}
}
private suspend fun readMetadataFromDiskLocked(file: File): AccountMetadata {
val encrypted = file.readBytes()
if (encrypted.size < GCM_IV_SIZE) {
val backup = backupCorruptFile(file)
// Genuinely unusable: not enough bytes for the IV. Back up and reset.
val backup = backupCorruptFile(file, ".corrupt")
onCorruption(StorageCorruption.FileCorrupted(backup))
return AccountMetadata()
}
@@ -140,31 +175,43 @@ class DesktopAccountStorage(
val decrypted = decrypt(encrypted)
mapper.readValue<AccountMetadata>(decrypted)
} catch (e: javax.crypto.AEADBadTagException) {
Log.e("DesktopAccountStorage", "GCM auth tag mismatch — file corrupted or key lost", e)
val backup = backupCorruptFile(file)
// Genuine ciphertext corruption or lost/rotated AES key.
Log.e("DesktopAccountStorage", "GCM auth tag mismatch, file corrupted or key lost", e)
val backup = backupCorruptFile(file, ".corrupt")
onCorruption(StorageCorruption.FileCorrupted(backup))
AccountMetadata()
} catch (e: javax.crypto.BadPaddingException) {
Log.e("DesktopAccountStorage", "Decryption failed — file corrupted", e)
val backup = backupCorruptFile(file)
// Genuine ciphertext corruption.
Log.e("DesktopAccountStorage", "Decryption failed, file corrupted", e)
val backup = backupCorruptFile(file, ".corrupt")
onCorruption(StorageCorruption.FileCorrupted(backup))
AccountMetadata()
} catch (e: com.fasterxml.jackson.core.JacksonException) {
// Schema mismatch: decrypted cleanly but the JSON does not fit our shape.
// Distinct suffix so operators can tell it apart from ciphertext corruption.
Log.e("DesktopAccountStorage", "JSON malformed after decryption", e)
val backup = backupCorruptFile(file)
val backup = backupCorruptFile(file, ".jsonerror")
onCorruption(StorageCorruption.JsonMalformed(backup))
AccountMetadata()
} catch (e: kotlin.coroutines.cancellation.CancellationException) {
throw e
} catch (e: Exception) {
Log.e("DesktopAccountStorage", "Failed to read accounts metadata", e)
val backup = backupCorruptFile(file)
onCorruption(StorageCorruption.FileCorrupted(backup))
AccountMetadata()
// Transient failure: I/O error, keychain refused / ambiguous, OOM, etc.
// DO NOT rename the on-disk file; the ciphertext is intact and the next
// launch may succeed (for example after the user re-approves the
// Keychain Access prompt). Surface up for the caller to decide.
Log.e("DesktopAccountStorage", "Transient error reading accounts metadata; file preserved", e)
onCorruption(StorageCorruption.TransientError(e))
throw e
}
}
private fun backupCorruptFile(file: File): String? =
private fun backupCorruptFile(
file: File,
suffix: String,
): String? =
try {
val backup = File(file.parent, "accounts.json.enc.corrupt.${System.currentTimeMillis()}")
val backup = File(file.parent, "${file.name}$suffix.${System.currentTimeMillis()}")
java.nio.file.Files
.copy(file.toPath(), backup.toPath())
file.deleteOrWarn("DesktopAccountStorage", "corrupt accounts file")
@@ -178,31 +225,78 @@ class DesktopAccountStorage(
val json = mapper.writeValueAsBytes(metadata)
val encrypted = encrypt(json)
// Atomic write via temp file
val file = getAccountsFile()
val temp = File(amethystDir, "${ACCOUNTS_FILE}.tmp")
temp.writeBytes(encrypted)
Files.move(temp.toPath(), file.toPath(), StandardCopyOption.REPLACE_EXISTING)
setFilePermissions(file)
withAccountsFileLock {
// Atomic write via temp file, under the cross-process lock so two
// Amethyst instances (Homebrew upgrade race, accidental double-launch)
// cannot interleave writes and truncate the file.
val temp = File(amethystDir, "$ACCOUNTS_FILE.tmp")
temp.writeBytes(encrypted)
Files.move(
temp.toPath(),
file.toPath(),
StandardCopyOption.REPLACE_EXISTING,
StandardCopyOption.ATOMIC_MOVE,
)
setFilePermissions(file)
}
}
/**
* Cross-process advisory lock + in-process mutex around the accounts.json.enc
* read/write critical section. The mutex is required because JVM
* `FileChannel.lock()` is a per-JVM lock and would throw
* `OverlappingFileLockException` on the second acquire from the same JVM.
* The channel lock is required to keep two Amethyst processes serial (upgrade
* race, accidental double-launch, cron-style relaunch).
*
* Mirrors the pattern used in SecureKeyStorage.withFileLock; kept private
* to this class so the two lock lifecycles stay independent.
*/
private suspend inline fun <T> withAccountsFileLock(crossinline block: suspend () -> T): T =
fileLockMutex.withLock {
val lockFile = File(amethystDir, ACCOUNTS_LOCK_FILE)
if (!lockFile.exists()) {
lockFile.createNewFile()
setFilePermissions(lockFile)
}
RandomAccessFile(lockFile, "rw").use { raf ->
raf.channel.lock().use { _ ->
block()
}
}
}
private fun getAccountsFile() = File(amethystDir, ACCOUNTS_FILE)
// --- AES-256-GCM encryption ---
private var cachedKey: ByteArray? = null
/**
* Reads (or creates on first launch) the metadata AES key.
*
* Distinguishes:
* - key exists in keychain: use it
* - keychain confirms definitively absent: generate + persist a fresh key
* - any other outcome (user cancelled/denied prompt, keychain locked,
* backend transient error): propagate the exception, do NOT rotate.
*
* Rotating the AES key on an ambiguous miss silently destroys the ability
* to decrypt the existing accounts.json.enc, wiping the logged-in accounts
* on next launch. That is the bug this method exists to prevent.
*/
private suspend fun getOrCreateKey(): ByteArray {
cachedKey?.let { return it }
val existing = secureStorage.getPrivateKey(METADATA_KEY_ALIAS)
val existing = secureStorage.getPrivateKeyOrThrow(METADATA_KEY_ALIAS)
if (existing != null) {
val key = Base64.getDecoder().decode(existing)
cachedKey = key
return key
}
// Definitively absent: safe to create and persist a fresh key.
val key = ByteArray(AES_KEY_SIZE).also { SecureRandom().nextBytes(it) }
secureStorage.savePrivateKey(METADATA_KEY_ALIAS, Base64.getEncoder().encodeToString(key))
cachedKey = key
@@ -58,6 +58,7 @@ class AccountManagerKeyLoginTest {
val keySlot = slot<String>()
val valueSlot = slot<String>()
coEvery { storage.getPrivateKey(capture(keySlot)) } answers { keyStore[keySlot.captured] }
coEvery { storage.getPrivateKeyOrThrow(capture(keySlot)) } answers { keyStore[keySlot.captured] }
coEvery { storage.savePrivateKey(capture(keySlot), capture(valueSlot)) } answers {
keyStore[keySlot.captured] = valueSlot.captured
}
@@ -49,6 +49,7 @@ class AccountManagerLoadAccountTest {
storage = mockk(relaxed = true)
// Return null so DesktopAccountStorage generates a fresh AES key
coEvery { storage.getPrivateKey("account-metadata-key") } returns null
coEvery { storage.getPrivateKeyOrThrow("account-metadata-key") } returns null
tempDir = createTempDirectory("acctmgr-load-test").toFile()
amethystDir = File(tempDir, ".amethyst")
amethystDir.mkdirs()
@@ -63,6 +63,7 @@ class AccountManagerLoadStateTransitionsTest {
fun setup() {
storage = mockk(relaxed = true)
coEvery { storage.getPrivateKey("account-metadata-key") } returns null
coEvery { storage.getPrivateKeyOrThrow("account-metadata-key") } returns null
tempDir = createTempDirectory("acctmgr-load-state").toFile()
File(tempDir, ".amethyst").mkdirs()
manager = AccountManager(storage, tempDir)
@@ -46,6 +46,7 @@ class AccountManagerLogoutTest {
fun setup() {
storage = mockk(relaxed = true)
coEvery { storage.getPrivateKey("account-metadata-key") } returns null
coEvery { storage.getPrivateKeyOrThrow("account-metadata-key") } returns null
tempDir = createTempDirectory("acctmgr-logout-test").toFile()
manager = AccountManager(storage, tempDir)
}
@@ -56,6 +56,7 @@ class AccountManagerNip46IsolationTest {
fun setup() {
storage = mockk(relaxed = true)
coEvery { storage.getPrivateKey("account-metadata-key") } returns null
coEvery { storage.getPrivateKeyOrThrow("account-metadata-key") } returns null
tempDir = createTempDirectory("acctmgr-nip46-iso-test").toFile()
amethystDir = File(tempDir, ".amethyst")
amethystDir.mkdirs()
@@ -56,6 +56,7 @@ class AccountManagerStateTransitionTest {
fun setup() {
storage = mockk(relaxed = true)
coEvery { storage.getPrivateKey("account-metadata-key") } returns null
coEvery { storage.getPrivateKeyOrThrow("account-metadata-key") } returns null
tempDir = createTempDirectory("acctmgr-state-test").toFile()
amethystDir = File(tempDir, ".amethyst")
amethystDir.mkdirs()
@@ -21,18 +21,28 @@
package com.vitorpamplona.amethyst.desktop.account
import com.vitorpamplona.amethyst.commons.keystorage.SecureKeyStorage
import com.vitorpamplona.amethyst.commons.keystorage.SecureStorageException
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.mockk
import io.mockk.slot
import kotlinx.coroutines.asCoroutineDispatcher
import kotlinx.coroutines.async
import kotlinx.coroutines.awaitAll
import kotlinx.coroutines.runBlocking
import kotlinx.coroutines.test.runTest
import kotlinx.coroutines.withContext
import java.io.File
import java.util.concurrent.Executors
import kotlin.test.AfterTest
import kotlin.test.BeforeTest
import kotlin.test.Test
import kotlin.test.assertEquals
import kotlin.test.assertFails
import kotlin.test.assertFalse
import kotlin.test.assertNotNull
import kotlin.test.assertNull
import kotlin.test.assertTrue
@@ -56,6 +66,9 @@ class DesktopAccountStorageTest {
coEvery { secureStorage.getPrivateKey(capture(keySlot)) } answers {
keyStore[keySlot.captured]
}
coEvery { secureStorage.getPrivateKeyOrThrow(capture(keySlot)) } answers {
keyStore[keySlot.captured]
}
coEvery { secureStorage.savePrivateKey(capture(keySlot), capture(valueSlot)) } answers {
keyStore[keySlot.captured] = valueSlot.captured
}
@@ -197,4 +210,208 @@ class DesktopAccountStorageTest {
// Encrypted content should NOT contain the npub in plaintext
assertTrue(!content.contains("npub1secret"))
}
// --- Bug 1: silent AES key rotation ---
@Test
fun `getOrCreateKey keyring throws ambiguous error does not rotate key or touch file`() =
runTest {
// First launch: seed a real metadata key + an existing accounts.json.enc
storage.saveAccount(AccountInfo("npub1existing", SignerType.Internal))
val file = File(File(tempDir, ".amethyst"), "accounts.json.enc")
val originalBytes = file.readBytes()
val originalMetadataKey = keyStore["account-metadata-key"]
assertNotNull(originalMetadataKey)
// Fresh storage instance simulating a relaunch: the keychain now
// returns an ambiguous error (user cancelled the Keychain dialog).
val throwingStorage: SecureKeyStorage = mockk()
coEvery { throwingStorage.getPrivateKeyOrThrow("account-metadata-key") } throws
SecureStorageException("user cancelled Keychain dialog")
// Legacy permissive read should still return the key; production
// must not fall back to it on the getOrCreate path.
coEvery { throwingStorage.getPrivateKey(any()) } answers { keyStore[firstArg()] }
coEvery { throwingStorage.hasPrivateKey(any()) } answers { keyStore.containsKey(firstArg()) }
val relaunched = DesktopAccountStorage(throwingStorage, tempDir)
// Any operation that needs the metadata key must fail loudly, not
// rotate the key or write a fresh empty file.
assertFails { runBlocking { relaunched.loadAccounts() } }
// Bug 1 invariant: no new savePrivateKey call for the metadata key.
coVerify(exactly = 0) {
throwingStorage.savePrivateKey("account-metadata-key", any())
}
// Bug 1 + Bug 2 invariant: on-disk ciphertext untouched.
assertTrue(file.exists())
assertContentEquals(originalBytes, file.readBytes())
// Bug 1 invariant: keyStore metadata key unchanged.
assertEquals(originalMetadataKey, keyStore["account-metadata-key"])
}
@Test
fun `getOrCreateKey keyring returns definitive not-found creates and persists new key`() =
runTest {
// Happy path first launch: getPrivateKeyOrThrow returns null,
// storage generates + persists a fresh AES key exactly once.
assertNull(keyStore["account-metadata-key"])
storage.saveAccount(AccountInfo("npub1first", SignerType.Internal))
assertNotNull(keyStore["account-metadata-key"])
coVerify(exactly = 1) {
secureStorage.savePrivateKey("account-metadata-key", any())
}
}
// --- Bug 2: read failure must not silently reset the file ---
@Test
fun `readMetadataFromDisk transient IO error does not backup file`() =
runTest {
// Seed a real file we can inspect.
storage.saveAccount(AccountInfo("npub1existing", SignerType.Internal))
val amethystDir = File(tempDir, ".amethyst")
val file = File(amethystDir, "accounts.json.enc")
val originalBytes = file.readBytes()
val originalName = file.name
// Fresh storage that surfaces a transient error from the keychain.
val throwingStorage: SecureKeyStorage = mockk()
coEvery { throwingStorage.getPrivateKeyOrThrow("account-metadata-key") } throws
SecureStorageException("transient keychain error")
coEvery { throwingStorage.getPrivateKey(any()) } returns null
coEvery { throwingStorage.hasPrivateKey(any()) } returns false
val corruptions = mutableListOf<StorageCorruption>()
val relaunched =
DesktopAccountStorage(throwingStorage, tempDir, onCorruption = { corruptions += it })
assertFails { runBlocking { relaunched.loadAccounts() } }
// File preserved, no .corrupt.* or .jsonerror.* sibling created.
assertTrue(file.exists())
assertContentEquals(originalBytes, file.readBytes())
val siblings = amethystDir.listFiles().orEmpty().map { it.name }
assertFalse(siblings.any { it != originalName && it.startsWith("accounts.json.enc") && (it.contains(".corrupt.") || it.contains(".jsonerror.")) })
// The callback fired with the transient subtype so the app can retry.
assertTrue(corruptions.any { it is StorageCorruption.TransientError })
}
@Test
fun `readMetadataFromDisk gcm tag mismatch backs up and resets`() =
runTest {
// Seed a valid file so we have a real metadata key in the mock keystore.
storage.saveAccount(AccountInfo("npub1a", SignerType.Internal))
val amethystDir = File(tempDir, ".amethyst")
val file = File(amethystDir, "accounts.json.enc")
assertTrue(file.exists())
// Overwrite with random bytes that pass the length check but fail
// GCM auth tag verification. Prefix with a fresh IV, then garbage.
val garbage = ByteArray(64) { it.toByte() }
file.writeBytes(garbage)
val corruptions = mutableListOf<StorageCorruption>()
val relaunched =
DesktopAccountStorage(secureStorage, tempDir, onCorruption = { corruptions += it })
val loaded = relaunched.loadAccounts()
assertTrue(loaded.isEmpty())
// Backup exists with the .corrupt.<ts> suffix; original file was
// removed (and will be re-created on next save).
val siblings = amethystDir.listFiles().orEmpty().map { it.name }
assertTrue(siblings.any { it.startsWith("accounts.json.enc.corrupt.") })
assertTrue(corruptions.any { it is StorageCorruption.FileCorrupted })
}
@Test
fun `readMetadataFromDisk json malformed uses jsonerror suffix`() =
runTest {
// Build an accounts.json.enc whose plaintext decrypts fine but is
// not the expected AccountMetadata shape. Easiest path: reuse the
// production encrypt via a lightweight helper storage that lets us
// control the plaintext.
storage.saveAccount(AccountInfo("npub1a", SignerType.Internal))
val amethystDir = File(tempDir, ".amethyst")
val file = File(amethystDir, "accounts.json.enc")
// Encrypt an unrelated JSON payload with the same AES key the mock
// keystore holds so decryption succeeds but Jackson rejects the shape.
val key =
java.util.Base64
.getDecoder()
.decode(keyStore["account-metadata-key"]!!)
val iv = ByteArray(12) { 7 }
val cipher = javax.crypto.Cipher.getInstance("AES/GCM/NoPadding")
cipher.init(
javax.crypto.Cipher.ENCRYPT_MODE,
javax.crypto.spec.SecretKeySpec(key, "AES"),
javax.crypto.spec.GCMParameterSpec(128, iv),
)
val badPayload = cipher.doFinal("\"not an object\"".toByteArray())
file.writeBytes(iv + badPayload)
val corruptions = mutableListOf<StorageCorruption>()
val relaunched =
DesktopAccountStorage(secureStorage, tempDir, onCorruption = { corruptions += it })
val loaded = relaunched.loadAccounts()
assertTrue(loaded.isEmpty())
val siblings = amethystDir.listFiles().orEmpty().map { it.name }
assertTrue(siblings.any { it.startsWith("accounts.json.enc.jsonerror.") })
assertTrue(corruptions.any { it is StorageCorruption.JsonMalformed })
}
// --- Bug 3: cross-process file lock ---
@Test
fun `writeMetadataToDisk concurrent saves serialize under file lock`() {
val executor = Executors.newFixedThreadPool(4)
try {
runBlocking {
withContext(executor.asCoroutineDispatcher()) {
val jobs =
(1..8).map { idx ->
async {
storage.saveAccount(
AccountInfo(
npub = "npub1parallel$idx",
signerType = SignerType.Internal,
),
)
}
}
jobs.awaitAll()
}
}
// All eight accounts present, file not truncated.
val loaded = runBlocking { storage.loadAccounts() }
assertEquals(8, loaded.size)
val npubs = loaded.map { it.npub }.toSet()
assertEquals((1..8).map { "npub1parallel$it" }.toSet(), npubs)
// Lock sidecar exists and is respected.
val lockFile = File(File(tempDir, ".amethyst"), "accounts.json.enc.lock")
assertTrue(lockFile.exists())
} finally {
executor.shutdownNow()
}
}
private fun assertContentEquals(
expected: ByteArray,
actual: ByteArray,
) {
assertEquals(expected.size, actual.size, "byte size mismatch")
for (i in expected.indices) {
if (expected[i] != actual[i]) {
throw AssertionError("byte differs at index $i: expected=${expected[i]} actual=${actual[i]}")
}
}
}
}
@@ -95,6 +95,7 @@ object LaunchScenario {
val tempHome = createTempDirectory("launch-scenario").toFile()
val storage = mockk<SecureKeyStorage>(relaxed = true)
coEvery { storage.getPrivateKey(any()) } returns null
coEvery { storage.getPrivateKeyOrThrow(any()) } returns null
File(tempHome, ".amethyst").mkdirs()
val account = AccountManager(storage, tempHome)
@@ -89,6 +89,7 @@ class AppStateMachineTest {
File(tempDir, ".amethyst").mkdirs()
storage = mockk(relaxed = true)
coEvery { storage.getPrivateKey(any()) } returns null
coEvery { storage.getPrivateKeyOrThrow(any()) } returns null
harnessScope = CoroutineScope(Dispatchers.Default + SupervisorJob())
relay = LaunchFixtureRelay.open(LaunchFixture.build(noteCount = 0).events)
}