mirror of
https://github.com/vitorpamplona/amethyst.git
synced 2026-08-09 16:14:40 +00:00
fix(desktop): cache OS Keyring handle so startup only prompts once
`SecureKeyStorage`'s three keyring paths (save/get/delete) each called `Keyring.create()` on every invocation. Each call opens a fresh backend session: - macOS: a new Security Framework session against `login.keychain`. Depending on the user's keychain policy (short access window, ACL on the amethyst-desktop item, or first-touch after unlock timeout), this surfaces a Keychain Access prompt every time. - Linux Secret Service / KWallet: a fresh session may re-trigger the wallet-unlock prompt if the daemon closed the previous session. - Windows Credential Manager: less user-visible but still redundant. Amethyst's cold-boot touches the store at least twice — once for `DesktopAccountStorage`'s AES-256-GCM metadata key (`account-metadata-key`), then again for the active account's nsec — so the user was seeing the OS keychain unlock prompt twice in a row before the UI was reachable. Fix: memoise the `Keyring` handle for the lifetime of the process. The `Keyring` object is thread-safe for the three ops we call, so a double-checked lazy singleton behind `keyringLock` is sufficient. The NPE hit path is a proper lazy: any `BackendNotSupportedException` bubbles up on the first call and is caught by the existing outer try/catch, which flips `keyringAvailable=false` and falls back to the encrypted file path (unchanged). Includes a small package-private `KeyringHandle` interface + real delegator so `SecureKeyStorageKeyringCacheTest` can substitute an in-memory handle and count backend-open invocations without touching the OS keychain. Three cases: 1. Cold-boot storm (save/get/delete across metadata + account keys) opens the Keyring exactly once. 2. Repeated `hasPrivateKey` reuses the cache. 3. Concurrent first-touches from 16 threads still open the Keyring exactly once (double-checked locking is race-free). No behavioural change beyond the prompt-count fix. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
m
Claude
+98
-6
@@ -87,6 +87,28 @@ actual class SecureKeyStorage private actual constructor() {
|
||||
private var fallbackPassword: String? = null
|
||||
private val fallbackMutex = Mutex() // Protects concurrent access to fallback file
|
||||
|
||||
/**
|
||||
* Cached Keyring instance. Opening a Keyring session is expensive and, on
|
||||
* 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
|
||||
* exact bug this cache fixes.
|
||||
*
|
||||
* Guarded by [keyringLock] so probing/opening the backend happens exactly
|
||||
* once per process; the [Keyring] itself is thread-safe once obtained.
|
||||
*/
|
||||
@Volatile
|
||||
private var cachedKeyring: KeyringHandle? = null
|
||||
private val keyringLock = Any()
|
||||
|
||||
/**
|
||||
* Package-private factory used by tests to inject a stub Keyring backend
|
||||
* and count backend-open invocations. Production code always defers to
|
||||
* [Keyring.create] through [RealKeyringHandle].
|
||||
*/
|
||||
internal var keyringFactory: () -> KeyringHandle = { RealKeyringHandle(Keyring.create()) }
|
||||
|
||||
actual suspend fun savePrivateKey(
|
||||
npub: String,
|
||||
privKeyHex: String,
|
||||
@@ -146,26 +168,40 @@ actual class SecureKeyStorage private actual constructor() {
|
||||
actual suspend fun hasPrivateKey(npub: String): Boolean = getPrivateKey(npub) != null
|
||||
|
||||
// Keyring-based storage
|
||||
|
||||
/**
|
||||
* Returns the process-wide [Keyring] instance, opening the OS-native
|
||||
* backend on first call. Subsequent calls reuse the same handle so the
|
||||
* user is only prompted (macOS Keychain Access, Secret Service unlock,
|
||||
* KWallet unlock) once per app run.
|
||||
*
|
||||
* Callers must handle [BackendNotSupportedException] — it can escape on
|
||||
* the very first call if no backend is available at all.
|
||||
*/
|
||||
private fun keyring(): KeyringHandle {
|
||||
cachedKeyring?.let { return it }
|
||||
return synchronized(keyringLock) {
|
||||
cachedKeyring ?: keyringFactory().also { cachedKeyring = it }
|
||||
}
|
||||
}
|
||||
|
||||
private fun saveToKeyring(
|
||||
npub: String,
|
||||
privKeyHex: String,
|
||||
) {
|
||||
val keyring = Keyring.create()
|
||||
keyring.setPassword(SERVICE_NAME, npub, privKeyHex)
|
||||
keyring().setPassword(SERVICE_NAME, npub, privKeyHex)
|
||||
}
|
||||
|
||||
private fun getFromKeyring(npub: String): String? =
|
||||
try {
|
||||
val keyring = Keyring.create()
|
||||
keyring.getPassword(SERVICE_NAME, npub)
|
||||
keyring().getPassword(SERVICE_NAME, npub)
|
||||
} catch (e: PasswordAccessException) {
|
||||
null
|
||||
}
|
||||
|
||||
private fun deleteFromKeyring(npub: String): Boolean =
|
||||
try {
|
||||
val keyring = Keyring.create()
|
||||
keyring.deletePassword(SERVICE_NAME, npub)
|
||||
keyring().deletePassword(SERVICE_NAME, npub)
|
||||
true
|
||||
} catch (e: PasswordAccessException) {
|
||||
false
|
||||
@@ -397,3 +433,59 @@ actual class SecureKeyStorage private actual constructor() {
|
||||
return String(decrypted)
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Small package-private abstraction over `com.github.javakeyring.Keyring`,
|
||||
* mirroring the three operations `SecureKeyStorage` actually uses. The real
|
||||
* implementation is a thin delegator; tests substitute an in-memory version
|
||||
* so the desktop unit test suite doesn't touch the OS Keychain (which would
|
||||
* be non-hermetic and slow, and on macOS would surface a user-visible prompt
|
||||
* during test runs).
|
||||
*
|
||||
* Not part of the public API — kept in this file so it stays private to the
|
||||
* keystorage package.
|
||||
*/
|
||||
internal interface KeyringHandle {
|
||||
@Throws(PasswordAccessException::class)
|
||||
fun getPassword(
|
||||
service: String,
|
||||
account: String,
|
||||
): String
|
||||
|
||||
@Throws(PasswordAccessException::class)
|
||||
fun setPassword(
|
||||
service: String,
|
||||
account: String,
|
||||
password: String,
|
||||
)
|
||||
|
||||
@Throws(PasswordAccessException::class)
|
||||
fun deletePassword(
|
||||
service: String,
|
||||
account: String,
|
||||
)
|
||||
}
|
||||
|
||||
internal class RealKeyringHandle(
|
||||
private val keyring: Keyring,
|
||||
) : KeyringHandle {
|
||||
override fun getPassword(
|
||||
service: String,
|
||||
account: String,
|
||||
): String = keyring.getPassword(service, account)
|
||||
|
||||
override fun setPassword(
|
||||
service: String,
|
||||
account: String,
|
||||
password: String,
|
||||
) {
|
||||
keyring.setPassword(service, account, password)
|
||||
}
|
||||
|
||||
override fun deletePassword(
|
||||
service: String,
|
||||
account: String,
|
||||
) {
|
||||
keyring.deletePassword(service, account)
|
||||
}
|
||||
}
|
||||
|
||||
+152
@@ -0,0 +1,152 @@
|
||||
/*
|
||||
* 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.Test
|
||||
import java.util.concurrent.ConcurrentHashMap
|
||||
import java.util.concurrent.atomic.AtomicInteger
|
||||
|
||||
/**
|
||||
* Regression tests for the desktop `SecureKeyStorage` keyring-instance cache.
|
||||
*
|
||||
* Prior to the fix, every `savePrivateKey` / `getPrivateKey` / `deletePrivateKey`
|
||||
* call opened a fresh `Keyring.create()` handle. On macOS, each fresh handle
|
||||
* incurs a Security Framework session open; on GNOME/KWallet a fresh handle can
|
||||
* re-trigger the OS unlock prompt. The Amethyst cold-boot path calls the
|
||||
* storage at least twice (metadata AES key, then the active account nsec), so
|
||||
* the user was seeing the keychain unlock prompt twice on startup.
|
||||
*
|
||||
* These tests pin the invariant that at most one `Keyring` is opened for the
|
||||
* process lifetime of a `SecureKeyStorage` instance, no matter how many
|
||||
* save/get/delete calls happen.
|
||||
*/
|
||||
class SecureKeyStorageKeyringCacheTest {
|
||||
private class InMemoryKeyring : KeyringHandle {
|
||||
private val store: ConcurrentHashMap<Pair<String, String>, String> = ConcurrentHashMap()
|
||||
|
||||
override fun getPassword(
|
||||
service: String,
|
||||
account: String,
|
||||
): String = store[service to account] ?: throw PasswordAccessException("no entry")
|
||||
|
||||
override fun setPassword(
|
||||
service: String,
|
||||
account: String,
|
||||
password: String,
|
||||
) {
|
||||
store[service to account] = password
|
||||
}
|
||||
|
||||
override fun deletePassword(
|
||||
service: String,
|
||||
account: String,
|
||||
) {
|
||||
if (store.remove(service to account) == null) {
|
||||
throw PasswordAccessException("no entry")
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
private fun newStorage(counter: AtomicInteger): SecureKeyStorage {
|
||||
val storage = SecureKeyStorage.create()
|
||||
storage.keyringFactory = {
|
||||
counter.incrementAndGet()
|
||||
InMemoryKeyring()
|
||||
}
|
||||
return storage
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `single instance across many save-get-delete calls (double-prompt regression)`() =
|
||||
runBlocking {
|
||||
val opens = AtomicInteger(0)
|
||||
val storage = newStorage(opens)
|
||||
|
||||
// Simulate the cold-boot storm: metadata key + active-account nsec
|
||||
// + a handful of subsequent NWC / bunker ephemeral key touches.
|
||||
storage.savePrivateKey("account-metadata-key", "aaaa")
|
||||
assertEquals("aaaa", storage.getPrivateKey("account-metadata-key"))
|
||||
storage.savePrivateKey("npub1alice", "bbbb")
|
||||
assertEquals("bbbb", storage.getPrivateKey("npub1alice"))
|
||||
storage.savePrivateKey("bunker-ephemeral-npub1alice", "cccc")
|
||||
assertEquals("cccc", storage.getPrivateKey("bunker-ephemeral-npub1alice"))
|
||||
assertEquals(true, storage.deletePrivateKey("bunker-ephemeral-npub1alice"))
|
||||
assertNull(storage.getPrivateKey("bunker-ephemeral-npub1alice"))
|
||||
|
||||
// The cache must open the keyring exactly once for the process
|
||||
// lifetime; each additional Keyring.create() call would surface as
|
||||
// an OS-level unlock prompt on macOS / GNOME / KWallet.
|
||||
assertEquals(
|
||||
"SecureKeyStorage must open Keyring exactly once per process",
|
||||
1,
|
||||
opens.get(),
|
||||
)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `hasPrivateKey reuses the cached keyring`() =
|
||||
runBlocking {
|
||||
val opens = AtomicInteger(0)
|
||||
val storage = newStorage(opens)
|
||||
|
||||
storage.savePrivateKey("npub1x", "1111")
|
||||
repeat(5) {
|
||||
assertEquals(true, storage.hasPrivateKey("npub1x"))
|
||||
assertEquals(false, storage.hasPrivateKey("npub1missing"))
|
||||
}
|
||||
assertEquals(1, opens.get())
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `concurrent first-touches still open the keyring exactly once`() =
|
||||
runBlocking {
|
||||
val opens = AtomicInteger(0)
|
||||
val storage = newStorage(opens)
|
||||
|
||||
val threads =
|
||||
List(16) { idx ->
|
||||
Thread {
|
||||
runBlocking {
|
||||
// Alternate between get and save so both call paths race to
|
||||
// grab the cached keyring on the first invocation.
|
||||
if (idx % 2 == 0) {
|
||||
storage.getPrivateKey("npub1concurrent-$idx")
|
||||
} else {
|
||||
storage.savePrivateKey("npub1concurrent-$idx", "0x$idx")
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
threads.forEach { it.start() }
|
||||
threads.forEach { it.join() }
|
||||
|
||||
// Race-free single-open guarantee under contention.
|
||||
assertEquals(
|
||||
"Concurrent first-touches must not open the Keyring more than once",
|
||||
1,
|
||||
opens.get(),
|
||||
)
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user