From ae3218249a6bd85af3c813b6ca87738aa72d6776 Mon Sep 17 00:00:00 2001 From: m Date: Wed, 29 Jul 2026 08:06:02 +1000 Subject: [PATCH] fix(desktop): cache OS Keyring handle so startup only prompts once MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 --- .../commons/keystorage/SecureKeyStorage.kt | 104 +++++++++++- .../SecureKeyStorageKeyringCacheTest.kt | 152 ++++++++++++++++++ 2 files changed, 250 insertions(+), 6 deletions(-) create mode 100644 commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorageKeyringCacheTest.kt diff --git a/commons/src/jvmMain/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorage.kt b/commons/src/jvmMain/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorage.kt index 72c1985045..85c8b9c95e 100644 --- a/commons/src/jvmMain/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorage.kt +++ b/commons/src/jvmMain/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorage.kt @@ -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) + } +} diff --git a/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorageKeyringCacheTest.kt b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorageKeyringCacheTest.kt new file mode 100644 index 0000000000..f2553eefec --- /dev/null +++ b/commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorageKeyringCacheTest.kt @@ -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, 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(), + ) + } +}