mirror of
https://github.com/vitorpamplona/amethyst.git
synced 2026-10-05 11:18:24 +00:00
fix(desktop): macOS forced re-login on cold boot — repair ProGuard keep rules for java-keyring
ProGuard in the release DMG (compose-rules.pro) was keeping pt.davidafsilva.apple.** — a library no longer in the dependency graph. The actual macOS-keychain dependency is com.github.javakeyring:java-keyring, which reflection-loads its OS-specific backend (OSXKeychainBackend / SecretServiceBackend / WinCredentialStoreBackend) at Keyring.create() time. The shrinker stripped the backend classes, Keyring.create() threw BackendNotSupportedException on every cold boot, SecureKeyStorage's fallback silently returned null (no password prompt in a GUI cold-boot), and every account whose key lived in the OS keychain (nsec, NIP-46 bunker ephemeral, NWC secret) was forced back to the login screen on each launch of the release DMG. Dev/Gradle runs skip ProGuard, which is why this never surfaced in development. Primary fix: - Replace dead pt.davidafsilva.apple.** keep rules with com.github.javakeyring.** and keep native methods + constructors on internal.** backends. Defense in depth (so a future regression is visible, not silent): - AccountManager._keychainUnavailable: StateFlow<Boolean> mirrors the existing _storageCorruption / _forceLogoutReason channels. - loadInternalAccount / loadBunkerAccount raise the signal when accounts.json.enc points at a key the keychain cannot return. - LoginScreen shows a one-line error banner when the signal is set; cleared on any successful login. Tests: - AccountManagerLoadAccountTest gains four cases: Internal-no-privkey signals, Bunker-no-ephemeral signals, clearKeychainUnavailable resets, happy path does NOT signal. See docs/plans/2026-06-18-fix-desktop-macos-bunker-relogin-plan.md for brainstorm + plan + deferred follow-ups (Linux/Windows DMG verification, signed-DMG smoke test, ProGuard mapping regression guard). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
nrobi144
co-authored by
Claude Opus 4.7
Claude Opus 4.7
parent
f05500792c
commit
123c055828
@@ -82,7 +82,14 @@
|
||||
# - androidx.sqlite (sqlite-bundled): nativeThreadSafeMode() stripped,
|
||||
# so `BundledSQLiteDriver.threadingMode` threw NoSuchMethodError on the
|
||||
# first relay-store query (LocalRelayStore.refreshStats).
|
||||
# - pt.davidafsilva.apple (jkeychain): macOS Keychain JNI for nsec storage.
|
||||
# - com.github.javakeyring (java-keyring): Keyring.create() reflection-loads
|
||||
# the OS-specific backend (OSXKeychainBackend / SecretServiceBackend /
|
||||
# WinCredentialStoreBackend). Without these keeps the macOS backend was
|
||||
# stripped, BackendNotSupportedException fired on every cold boot, the
|
||||
# SecureKeyStorage fallback silently returned null, and any account whose
|
||||
# key lived in the keychain (nsec, NIP-46 bunker ephemeral, NWC secret)
|
||||
# was forced to re-log-in every launch of the release DMG. The dev/Gradle
|
||||
# run path skips ProGuard, which is why it never surfaced in development.
|
||||
#
|
||||
# secp256k1-kmp is already covered by the `-keep class fr.acinq.secp256k1.**`
|
||||
# rule mirrored from mobile above.
|
||||
@@ -91,9 +98,10 @@
|
||||
native <methods>;
|
||||
static <methods>;
|
||||
}
|
||||
-keep class pt.davidafsilva.apple.** { *; }
|
||||
-keepclassmembers class pt.davidafsilva.apple.** {
|
||||
-keep class com.github.javakeyring.** { *; }
|
||||
-keepclassmembers class com.github.javakeyring.internal.** {
|
||||
native <methods>;
|
||||
<init>(...);
|
||||
}
|
||||
|
||||
# kmp-tor — loads native Tor daemon via JNI reflection
|
||||
|
||||
+23
-2
@@ -140,6 +140,18 @@ class AccountManager internal constructor(
|
||||
private val _forceLogoutReason = MutableStateFlow<String?>(null)
|
||||
val forceLogoutReason: StateFlow<String?> = _forceLogoutReason.asStateFlow()
|
||||
|
||||
// Set to true when accounts.json.enc points at an account whose key cannot
|
||||
// be recovered from the OS keychain on cold boot — i.e. the user is forced
|
||||
// back to the login screen because the keychain read returned nothing for
|
||||
// a key we previously persisted. Mirrors [storageCorruption] / [forceLogoutReason]
|
||||
// (a separate diagnostic channel, not embedded in [AccountState]).
|
||||
private val _keychainUnavailable = MutableStateFlow(false)
|
||||
val keychainUnavailable: StateFlow<Boolean> = _keychainUnavailable.asStateFlow()
|
||||
|
||||
fun clearKeychainUnavailable() {
|
||||
_keychainUnavailable.value = false
|
||||
}
|
||||
|
||||
private val _loginProgress = MutableStateFlow<LoginProgress?>(null)
|
||||
val loginProgress: StateFlow<LoginProgress?> = _loginProgress.asStateFlow()
|
||||
|
||||
@@ -284,7 +296,12 @@ class AccountManager internal constructor(
|
||||
return Result.success(state)
|
||||
}
|
||||
|
||||
// No private key — fall back to read-only
|
||||
// accounts.json.enc said this is an Internal (nsec) account but the
|
||||
// keychain returned no key. Hardening (46caa4d79) ensures legitimate
|
||||
// logout removes the AccountInfo too, so reaching here means the read
|
||||
// side of the keychain is broken (e.g. ProGuard-stripped backend in
|
||||
// the macOS release DMG). Flag it so the login screen can explain.
|
||||
_keychainUnavailable.value = true
|
||||
return loadReadOnlyAccount(npub)
|
||||
}
|
||||
|
||||
@@ -297,7 +314,11 @@ class AccountManager internal constructor(
|
||||
val ephemeralPrivKeyHex =
|
||||
perAccountKey?.takeIf { it.isNotEmpty() }
|
||||
?: secureStorage.getPrivateKey(LEGACY_BUNKER_EPHEMERAL_KEY_ALIAS)?.takeIf { it.isNotEmpty() }
|
||||
?: return Result.failure(Exception("Ephemeral key not found"))
|
||||
?: run {
|
||||
// See [loadInternalAccount] for why we flag this here too.
|
||||
_keychainUnavailable.value = true
|
||||
return Result.failure(Exception("Ephemeral key not found"))
|
||||
}
|
||||
|
||||
val ephemeralKeyPair = KeyPair(privKey = ephemeralPrivKeyHex.hexToByteArray())
|
||||
val ephemeralSigner = NostrSignerInternal(ephemeralKeyPair)
|
||||
|
||||
@@ -67,6 +67,7 @@ fun LoginScreen(
|
||||
var generatedAccount by remember { mutableStateOf<AccountState.LoggedIn?>(null) }
|
||||
|
||||
val loginProgress by accountManager.loginProgress.collectAsState()
|
||||
val keychainUnavailable by accountManager.keychainUnavailable.collectAsState()
|
||||
|
||||
Column(
|
||||
modifier = Modifier.fillMaxSize().padding(32.dp),
|
||||
@@ -87,11 +88,22 @@ fun LoginScreen(
|
||||
color = MaterialTheme.colorScheme.onSurfaceVariant,
|
||||
)
|
||||
|
||||
if (keychainUnavailable) {
|
||||
Spacer(Modifier.height(24.dp))
|
||||
Text(
|
||||
"Your saved session couldn't be restored from the OS keychain. Please log in again.",
|
||||
style = MaterialTheme.typography.bodyMedium,
|
||||
color = MaterialTheme.colorScheme.error,
|
||||
modifier = Modifier.widthIn(max = 480.dp),
|
||||
)
|
||||
}
|
||||
|
||||
Spacer(Modifier.height(48.dp))
|
||||
|
||||
LoginCard(
|
||||
onLogin = { keyInput ->
|
||||
accountManager.loginWithKey(keyInput).map {
|
||||
accountManager.clearKeychainUnavailable()
|
||||
onLoginSuccess()
|
||||
}
|
||||
},
|
||||
@@ -101,11 +113,13 @@ fun LoginScreen(
|
||||
},
|
||||
onLoginBunker = { bunkerUri ->
|
||||
accountManager.loginWithBunker(bunkerUri).map {
|
||||
accountManager.clearKeychainUnavailable()
|
||||
onLoginSuccess()
|
||||
}
|
||||
},
|
||||
onLoginNostrConnect = { onUriGenerated ->
|
||||
accountManager.loginWithNostrConnect(onUriGenerated).map {
|
||||
accountManager.clearKeychainUnavailable()
|
||||
onLoginSuccess()
|
||||
}
|
||||
},
|
||||
|
||||
+79
@@ -34,6 +34,7 @@ import kotlin.io.path.createTempDirectory
|
||||
import kotlin.test.AfterTest
|
||||
import kotlin.test.BeforeTest
|
||||
import kotlin.test.Test
|
||||
import kotlin.test.assertFalse
|
||||
import kotlin.test.assertIs
|
||||
import kotlin.test.assertTrue
|
||||
|
||||
@@ -150,6 +151,84 @@ class AccountManagerLoadAccountTest {
|
||||
assertIs<SignerType.Remote>(state.signerType)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun loadSavedAccountInternalNoPrivkeySignalsKeychainUnavailable() =
|
||||
runTest {
|
||||
val keyPair = KeyPair()
|
||||
val npub = keyPair.pubKey.toNpub()
|
||||
|
||||
manager.accountStorage.saveAccount(AccountInfo(npub = npub, signerType = SignerType.Internal))
|
||||
manager.accountStorage.setCurrentAccount(npub)
|
||||
coEvery { storage.getPrivateKey(npub) } returns null
|
||||
|
||||
assertFalse(manager.keychainUnavailable.value, "precondition: signal starts cleared")
|
||||
manager.loadSavedAccount()
|
||||
assertTrue(
|
||||
manager.keychainUnavailable.value,
|
||||
"accounts.json.enc had an Internal account but the keychain returned null — must signal",
|
||||
)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun loadSavedAccountBunkerNoEphemeralSignalsKeychainUnavailable() =
|
||||
runTest {
|
||||
val validHex = "a".repeat(64)
|
||||
val keyPair = KeyPair()
|
||||
val npub = keyPair.pubKey.toNpub()
|
||||
|
||||
manager.accountStorage.saveAccount(
|
||||
AccountInfo(npub = npub, signerType = SignerType.Remote("bunker://$validHex?relay=wss://r.com")),
|
||||
)
|
||||
manager.accountStorage.setCurrentAccount(npub)
|
||||
coEvery {
|
||||
storage.getPrivateKey(AccountManager.bunkerEphemeralKeyAlias(npub))
|
||||
} returns null
|
||||
coEvery {
|
||||
storage.getPrivateKey(AccountManager.LEGACY_BUNKER_EPHEMERAL_KEY_ALIAS)
|
||||
} returns null
|
||||
|
||||
assertFalse(manager.keychainUnavailable.value, "precondition: signal starts cleared")
|
||||
manager.loadSavedAccount()
|
||||
assertTrue(
|
||||
manager.keychainUnavailable.value,
|
||||
"accounts.json.enc had a Remote bunker account but the ephemeral key was missing — must signal",
|
||||
)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun clearKeychainUnavailableResetsSignal() =
|
||||
runTest {
|
||||
val keyPair = KeyPair()
|
||||
val npub = keyPair.pubKey.toNpub()
|
||||
|
||||
manager.accountStorage.saveAccount(AccountInfo(npub = npub, signerType = SignerType.Internal))
|
||||
manager.accountStorage.setCurrentAccount(npub)
|
||||
coEvery { storage.getPrivateKey(npub) } returns null
|
||||
|
||||
manager.loadSavedAccount()
|
||||
assertTrue(manager.keychainUnavailable.value)
|
||||
manager.clearKeychainUnavailable()
|
||||
assertFalse(manager.keychainUnavailable.value)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun loadSavedAccountInternalWithPrivkeyDoesNotSignalKeychainUnavailable() =
|
||||
runTest {
|
||||
val keyPair = KeyPair()
|
||||
val npub = keyPair.pubKey.toNpub()
|
||||
val privKeyHex = keyPair.privKey!!.toHexKey()
|
||||
|
||||
manager.accountStorage.saveAccount(AccountInfo(npub = npub, signerType = SignerType.Internal))
|
||||
manager.accountStorage.setCurrentAccount(npub)
|
||||
coEvery { storage.getPrivateKey(npub) } returns privKeyHex
|
||||
|
||||
manager.loadSavedAccount()
|
||||
assertFalse(
|
||||
manager.keychainUnavailable.value,
|
||||
"happy path must not raise the keychain-unavailable signal",
|
||||
)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun loadSavedAccountDeletesLegacyFiles() =
|
||||
runTest {
|
||||
|
||||
@@ -0,0 +1,252 @@
|
||||
---
|
||||
title: fix(desktop): macOS forced re-login on cold boot — ProGuard strips java-keyring backend
|
||||
type: fix
|
||||
status: shipped-pr1
|
||||
date: 2026-06-18
|
||||
origin: docs/brainstorms/2026-06-18-fix-macos-bunker-relogin-brainstorm.md
|
||||
---
|
||||
|
||||
## Implementation Status (2026-06-18)
|
||||
|
||||
**Landed in PR 1 (Phases 1 + 2 + partial 3):**
|
||||
- ✅ ProGuard keep rules fix (`desktopApp/compose-rules.pro`) — the actual bug fix
|
||||
- ✅ `AccountManager._keychainUnavailable: StateFlow<Boolean>` mirroring the existing `_storageCorruption` / `_forceLogoutReason` diagnostic channels
|
||||
- ✅ `loadInternalAccount` + `loadBunkerAccount` raise the signal when `accounts.json.enc` points at a key the keychain cannot return
|
||||
- ✅ `LoginScreen` observes the signal, renders a single-line error banner above the LoginCard; `clearKeychainUnavailable()` invoked from all successful login paths
|
||||
- ✅ Four new unit tests in `AccountManagerLoadAccountTest`: Internal-no-privkey signals, Bunker-no-ephemeral signals, `clearKeychainUnavailable()` resets, happy-path does NOT signal
|
||||
|
||||
**Deferred to follow-up (verification + cross-platform sweep):**
|
||||
- Phase 0 Track B: visual repro on a signed release DMG — needs a Mac. The ProGuard rule fix is what solves the reported bug; reproducing the broken state requires building + installing the unfixed DMG.
|
||||
- Phase 4: Linux DEB + Windows MSI cold-boot smoke test. Same keep rule covers all `internal.**` backends so should incidentally fix them; needs CI matrix run.
|
||||
- Phase 5: signed + notarized DMG verification — needs Mac + signing cert.
|
||||
- Phase 0 Track A unit test that mocks `Keyring.create()` throwing — required adding a production injection seam for marginal regression value; the actual ProGuard regression is only catchable on a shrunk artifact (Phase 3 #7 smoke test in the plan below), not unit tests.
|
||||
- Phase 3 #7 release-DMG smoke test extension — needs work on the existing smoke-test harness (out of scope for one-shot fix).
|
||||
- ProGuard `mapping.txt` regression guard gradle task — nice-to-have, deferred.
|
||||
|
||||
# 🐛 fix(desktop): macOS forced re-login on cold boot — ProGuard strips java-keyring backend
|
||||
|
||||
## Overview
|
||||
|
||||
User on macOS using the recent Amethyst Desktop release (official GitHub DMG) is forced to re-log-in with their NIP-46 bunker on **every** cold boot — 100 % reproducible. Investigation determined the bug is **not bunker-specific**: every macOS release-DMG user whose account requires a key stored in the OS keychain loses their session on cold boot (bunker, nsec, NWC). Bunker is the loudest symptom because re-pasting a bunker URI is high-friction; nsec users likely re-paste quickly and never report.
|
||||
|
||||
Plan delivers a **reproduce-before-fix** workflow per `CLAUDE.md`'s "Verify, Don't Guess" instruction: a failing test on `main` first, a confirmed local-DMG reproduction second, then the fix, then signed/notarized verification.
|
||||
|
||||
## Problem Statement / Motivation
|
||||
|
||||
### Symptom (reported)
|
||||
- Affected user: macOS, official DMG, "every restart, always."
|
||||
- Cold boot → login screen instead of feed → user must repeat bunker login flow.
|
||||
|
||||
### Code path
|
||||
```
|
||||
loadSavedAccount() [AccountManager.kt:240]
|
||||
→ loadBunkerAccount(bunkerUri, npub) [AccountManager.kt:259, 291]
|
||||
→ secureStorage.getPrivateKey(bunkerEphemeralKeyAlias(npub)) [AccountManager.kt:296]
|
||||
→ SecureKeyStorage.getPrivateKey() [SecureKeyStorage.kt:110]
|
||||
→ Keyring.create() throws BackendNotSupportedException
|
||||
→ catch → keyringAvailable = false (process-wide), getFromFallback()
|
||||
→ SecureKeyStorage.kt:201: fallbackPassword ?: return null // GUI cold boot, never prompted
|
||||
→ null returned
|
||||
→ loadBunkerAccount returns Result.failure(Exception("Ephemeral key not found")) [:300]
|
||||
→ Main.kt translates failure → AccountState.LoggedOut, no diagnostic surfaced to user
|
||||
```
|
||||
|
||||
The same path also serves:
|
||||
- **nsec**: `loadInternalAccount()` → `secureStorage.getPrivateKey(npub)` (`AccountManager.kt:269`)
|
||||
- **NWC**: `nwc_<npub>` keychain alias per memory note + recent hardening (`46caa4d79`).
|
||||
|
||||
So the same failure invalidates all three on macOS release builds.
|
||||
|
||||
### Root cause
|
||||
`desktopApp/compose-rules.pro:94-96` declares ProGuard keep rules for `pt.davidafsilva.apple.**` — a library that is **not** in this project's dependency graph. The actual macOS keychain dependency is `com.github.javakeyring:java-keyring` (`libs.versions.toml:166`).
|
||||
|
||||
- ProGuard was newly wired in v1.09.1 (`compose-rules.pro:76-90` comment).
|
||||
- `Keyring.create()` reflection-loads its OS backend (e.g. `com.github.javakeyring.internal.osx.OSXKeychainBackend`).
|
||||
- The shrink pass (still on; only `-dontoptimize` is set per `compose-rules.pro:127`) removes classes with no static callers — and the macOS backend has none, by design.
|
||||
- Result: `Keyring.create()` always throws `BackendNotSupportedException` in the release DMG. Dev `:desktopApp:run` skips ProGuard, which is why no-one caught it pre-release.
|
||||
|
||||
### Why it surfaced only after the May 15 hardening
|
||||
- Pre-`46caa4d79`: bunker URI was read from `bunker_uri.txt`; the legacy load path tolerated missing keychain state (rebuilt via different fallback).
|
||||
- Post-`46caa4d79`: cold boot routes strictly on `SignerType` from `accounts.json.enc` and requires the keychain ephemeral key. Same keychain failure now manifests as a hard "ephemeral key not found" → silent LoggedOut.
|
||||
|
||||
## Proposed Solution
|
||||
|
||||
Three-layer fix, deliberately small and bounded:
|
||||
|
||||
1. **Primary**: correct the ProGuard keep rules to actually keep `com.github.javakeyring.**` (and JNA-Structure-style native-method-bearing members in its `internal.**` backends). Delete the dead `pt.davidafsilva.apple.**` rules.
|
||||
2. **Defense in depth**: `SecureKeyStorage.getPrivateKey()` should not silently return null when the keychain is configured-but-broken; route a distinct error so the caller can present a diagnosable state instead of an indistinguishable LoggedOut.
|
||||
3. **Regression guard**: extend the existing release-DMG smoke test (already present for #2819 / `c0c055e77`) with a "bunker cold boot survives restart" assertion against a signed+notarized artifact.
|
||||
|
||||
## Technical Considerations
|
||||
|
||||
### Architecture impacts
|
||||
- Single file change in `compose-rules.pro` for the primary fix. No public API change.
|
||||
- `SecureKeyStorage` gains a new exception case (already declares `SecureStorageException`) — internal contract only.
|
||||
- `AccountManager.loadBunkerAccount` / `loadInternalAccount` propagate a typed failure rather than a generic `Exception("Ephemeral key not found")`.
|
||||
- Login screen reads a single new sentinel from the existing `AccountState` flow (no new global state container).
|
||||
|
||||
### Performance implications
|
||||
- None. ProGuard keep rules add a handful of classes to the shipped jar (~few KB).
|
||||
|
||||
### Security considerations
|
||||
- Keep rules are scoped to `com.github.javakeyring.**`; no broader reflection surface introduced.
|
||||
- Fallback file (`~/.amethyst/keys.enc`) behavior unchanged — still password-gated for genuine fallback environments; the fix prevents the *unintended* fallback path on macOS release builds.
|
||||
|
||||
## System-Wide Impact
|
||||
|
||||
- **Interaction graph**: ProGuard config → `Keyring.create()` backend lookup → JNA call to macOS Security framework → `setPassword` / `getPassword` succeeds → `SecureKeyStorage` returns key → `loadBunkerAccount` / `loadInternalAccount` succeed → `AccountState.LoggedIn`. Same chain serves NWC secret.
|
||||
- **Error propagation**: today `getFromFallback` returns null on a process-wide latch (`keyringAvailable = false`) once `BackendNotSupportedException` fires — every subsequent read in the same process also returns null. After the fix this latch is never tripped on macOS release builds; for genuine fallback scenarios (Linux without secret service) behavior is unchanged.
|
||||
- **State lifecycle risks**: pre-fix Keychain entries written under one code-signing identity may not be readable after a re-signed update (macOS ACL); the user will need to log in **once** after upgrading. Document this; do NOT attempt automatic re-write.
|
||||
- **API surface parity**: bunker, nsec, NWC all read via `SecureKeyStorage` — all benefit from one keep-rule fix and one defense-in-depth wrap. iOS uses a different `actual` and is unaffected.
|
||||
- **Integration test scenarios**: covered in Phase 3 below.
|
||||
|
||||
## Phases
|
||||
|
||||
### Phase 0 — Diagnostics & Reproduction (gating, MUST complete before Phase 1)
|
||||
|
||||
Track A (cheap, CI): write deterministic failing test in `commons:jvmTest`.
|
||||
- File: `commons/src/jvmTest/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorageFallbackSilentFailureTest.kt`
|
||||
- Inject a `Keyring` factory that throws `BackendNotSupportedException`.
|
||||
- Save then read → assert that the API surface today returns `null` silently (the failing-on-fix predicate is "no diagnosable signal exists for cold-boot keychain unavailability").
|
||||
- Test stays after fix as a regression guard for the defense-in-depth path.
|
||||
|
||||
Track B (one-machine, manual recipe): local release-DMG reproduction.
|
||||
- Build current `main`: `./gradlew :desktopApp:packageReleaseDmg` (or `createReleaseDistributable` + `packageDmg` — confirm exact task in Phase 0).
|
||||
- Install DMG; log in with a fresh bunker URI; quit; relaunch → expect login screen.
|
||||
- On the same Mac, `./gradlew :desktopApp:run` with the same `~/.amethyst/` directory → expect logged-in feed (proves dev vs release divergence).
|
||||
- Run `security find-generic-password -s amethyst-desktop` → expect entry present (proves write-side works; isolates read-side).
|
||||
- Inspect ProGuard mapping: confirm `com.github.javakeyring.internal.osx.OSXKeychainBackend` is missing/renamed in the release jar.
|
||||
|
||||
Track C (gated on B inconclusive): instrumented build to affected user.
|
||||
- Add `println` around `Keyring.create()`, `getPassword`, and the fallback entry, with the exception class name and stack.
|
||||
- Hand to the affected user; collect cold-boot log.
|
||||
|
||||
**Phase 0 done when:** Track A test is passing on `main` (i.e. silent-null IS the behaviour) AND Track B has visually reproduced "every restart, always" on a locally-built DMG. If A passes but B can't reproduce, escalate to H2 (signing/entitlements) per the brainstorm.
|
||||
|
||||
### Phase 1 — Primary fix: ProGuard keep rules
|
||||
|
||||
- File: `desktopApp/compose-rules.pro`
|
||||
- Replace the dead `pt.davidafsilva.apple.**` block (lines 85-96) with:
|
||||
```proguard
|
||||
# com.github.javakeyring:java-keyring — macOS Keychain / Linux Secret Service /
|
||||
# Windows Credential Manager backends are loaded by reflection from
|
||||
# Keyring.create(); the shrink pass strips them without these keeps.
|
||||
-keep class com.github.javakeyring.** { *; }
|
||||
-keepclassmembers class com.github.javakeyring.internal.** {
|
||||
native <methods>;
|
||||
<init>(...);
|
||||
}
|
||||
```
|
||||
- Update the JNI-keep comment block above to point to the real library.
|
||||
- Audit: do any other reflection-loaded backends in our deps share this hazard? Quick grep for `Class.forName` / `ServiceLoader` in shipped libs; out-of-scope to fix but worth noting.
|
||||
|
||||
### Phase 2 — Defense in depth: diagnosable cold-boot state
|
||||
|
||||
Today `loadSavedAccount` collapses every failure into `Result.failure(Exception(...))` and the UI shows the login screen with no signal. Add **one** sentinel:
|
||||
|
||||
- `commons` (or `desktopApp` if commons would force cross-module churn): new typed failure reason, e.g. `SignerLoadError.KeychainUnavailable(npub: String)`.
|
||||
- `SecureKeyStorage.getPrivateKey()`: distinguish "keychain says no such entry" (legitimate null) from "keychain backend not usable in this process" (latched state) and surface a `SecureStorageException` for the latter.
|
||||
- `AccountManager.loadBunkerAccount` / `loadInternalAccount`: catch `SecureStorageException`, map to the typed failure reason, route through to `AccountState.LoggedOut(reason = ...)` (extend the existing LoggedOut, do **not** add a new state).
|
||||
- `LoginScreen`: when `reason != null`, render a single-line banner: *"Your saved session couldn't be restored. Please log in again."* (no help-link, no telemetry — keep it minimal).
|
||||
- This is the only UX change; **resist** adding error-recovery dialogs, retry buttons, or settings screens. Per `CLAUDE.md` "Don't add features beyond what the task requires."
|
||||
|
||||
### Phase 3 — Tests
|
||||
|
||||
| # | Scenario | Test file | Type |
|
||||
|---|----------|-----------|------|
|
||||
| 1 | `Keyring.create()` returns a working backend in release classpath (post-ProGuard) | `commons/src/jvmTest/.../SecureKeyStorageBackendLoadsTest.kt` | Unit, asserts `Keyring.create()` does not throw on the running JVM |
|
||||
| 2 | Backend throws → `SecureKeyStorage.getPrivateKey()` surfaces `SecureStorageException`, not silent null | `commons/src/jvmTest/.../SecureKeyStorageFallbackSilentFailureTest.kt` | Unit (Track A above, evolves into this) |
|
||||
| 3 | Cold boot with bunker account + simulated keychain failure → LoggedOut with `KeychainUnavailable` reason | `desktopApp/src/jvmTest/.../AccountManagerBunkerColdBootRecoveryTest.kt` | Unit |
|
||||
| 4 | Cold boot with nsec account + simulated keychain failure → same diagnosable LoggedOut | `desktopApp/src/jvmTest/.../AccountManagerInternalColdBootRecoveryTest.kt` | Unit |
|
||||
| 5 | Cold boot with NWC secret + simulated keychain failure → wallet section degrades gracefully (no crash) | `desktopApp/src/jvmTest/.../AccountManagerNwcColdBootRecoveryTest.kt` | Unit |
|
||||
| 6 | Multi-account: 1 internal + 1 bunker, switching account does not corrupt the other's keychain entry | `desktopApp/src/jvmTest/.../AccountManagerMultiAccountSwitchTest.kt` (extend existing) | Unit |
|
||||
| 7 | Release-DMG smoke: log in with bunker → kill app → relaunch → assert still logged in | `desktopApp/.../SmokeTestRelease*.kt` (extend pattern from `c0c055e77`) | Smoke (signed/notarized DMG) |
|
||||
|
||||
ProGuard mapping regression guard (optional, recommended): a tiny gradle check that fails the release build if `mapping.txt` contains a rename for `com.github.javakeyring.internal.osx.OSXKeychainBackend`.
|
||||
|
||||
### Phase 4 — Cross-platform verification (Linux / Windows release builds)
|
||||
|
||||
- Linux release DEB: built-in fallback is Secret Service / kwallet → if backend stripped, falls back to encrypted file but with the same silent-password failure mode in GUI sessions.
|
||||
- Windows release MSI: WinCredential backend; same reflection-load pattern.
|
||||
- Run smoke test (Phase 3 #7) on Linux DEB and Windows MSI release artifacts. Add to existing CI matrix.
|
||||
|
||||
### Phase 5 — Signed/notarized verification + release notes
|
||||
|
||||
- Verify on a **signed and notarized** macOS DMG (not just locally built) — the affected user reported via official release.
|
||||
- Verify on Linux DEB and Windows MSI.
|
||||
- Release notes line: "Fixed an issue where macOS users were forced to re-log-in on every app restart (since v1.09.1)."
|
||||
- **Known caveat documented**: users who logged in under a previous (unfixed) build may have a Keychain entry whose ACL is bound to a stale signing identity. After upgrading they may have to log in one final time; subsequent restarts work. Do NOT auto-clear the entry — let macOS Security re-bind on the next write.
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
### Functional
|
||||
- [ ] Phase 0 Track A test passes (proves "silent null on backend failure" is the current behaviour).
|
||||
- [ ] Phase 0 Track B reproduces "every restart, always" on a locally-built release DMG.
|
||||
- [ ] After Phase 1 ships: same Track B recipe → bunker session persists across cold boot.
|
||||
- [ ] nsec users also persist across cold boot on the release DMG.
|
||||
- [ ] NWC connection persists across cold boot on the release DMG.
|
||||
- [ ] Multi-account: switching does not cause either account's keychain entry to be lost.
|
||||
|
||||
### Non-functional
|
||||
- [ ] No new dependency added.
|
||||
- [ ] No password prompt ever appears in the GUI cold-boot path (only legitimate fallback environments may prompt).
|
||||
- [ ] Release DMG size unchanged within ±100 KB (sanity check on keep-rule scope).
|
||||
- [ ] No new dialog, settings page, or recovery flow added beyond the single banner line in Phase 2.
|
||||
|
||||
### Quality gates
|
||||
- [ ] Tests 1-6 in Phase 3 all green in CI.
|
||||
- [ ] Smoke test (#7) passes on macOS DMG, Linux DEB, Windows MSI release artifacts.
|
||||
- [ ] `./gradlew spotlessApply` clean.
|
||||
- [ ] Manual verification on a **signed + notarized** macOS DMG (Phase 5).
|
||||
|
||||
## Success Metrics
|
||||
|
||||
- Zero `BackendNotSupportedException` log lines from `SecureKeyStorage` on macOS release builds.
|
||||
- Affected user confirms persistence after upgrading (single user-confirmed datapoint is enough — repro is deterministic).
|
||||
- No new "forced re-login on macOS" reports in the next two releases.
|
||||
|
||||
## Dependencies & Risks
|
||||
|
||||
| Risk | Likelihood | Mitigation |
|
||||
|------|------------|-----------|
|
||||
| Keep rules miss a sub-package that gets reflection-loaded | Low | `-keep class com.github.javakeyring.** { *; }` covers all members under the root package; Phase 3 Test 1 asserts `Keyring.create()` works post-shrink |
|
||||
| Keychain entries written by pre-fix builds unreadable post-fix due to signing-identity rebind | Medium (one-time, user-visible) | Documented as a one-time re-login; release notes call it out |
|
||||
| `-dontoptimize` already in place — no interaction with keep rules | Low | Existing config + manual verification |
|
||||
| Linux/Windows release builds need same fix; lab-testing matrix small | Medium | Extend Phase 4 verification to all three artifacts before merging |
|
||||
| Merge conflict with active "Account Security Hardening" branch (`fix/account-security-hardening`) | Low | This plan touches `compose-rules.pro` + minor `SecureKeyStorage`/`AccountManager` deltas; coordinate with `docs/plans/2026-05-14-fix-account-security-hardening-plan.md` owner |
|
||||
| Defense-in-depth scope creep | Medium | Hard-cap on Phase 2 design: one typed failure, one banner line, no recovery UI |
|
||||
|
||||
## Open Questions (carried from brainstorm)
|
||||
|
||||
- ProGuard mapping file from the affected user's release version — needed to confirm the macOS backend is renamed/stripped post-shrink.
|
||||
- Are non-bunker (nsec) macOS users on the same release also affected? Expected yes per code path; need one confirmation.
|
||||
- Is the `~/.amethyst/keys.enc` fallback file ever created on the affected user's system (`ls -la ~/.amethyst/`)? Confirms the fallback was reached.
|
||||
- Does `security find-generic-password -s amethyst-desktop` show the entry after the first login? Confirms write-side works on the affected box.
|
||||
- Linux DEB + Windows MSI smoke test: confirm same bug, same fix. (Phase 4 will answer.)
|
||||
- DMG signing/notarization status of the affected release: hardened-runtime + entitlements file location — for ruling out H2.
|
||||
|
||||
## Sources & References
|
||||
|
||||
### Origin
|
||||
- **Brainstorm:** [docs/brainstorms/2026-06-18-fix-macos-bunker-relogin-brainstorm.md](docs/brainstorms/2026-06-18-fix-macos-bunker-relogin-brainstorm.md) — carries forward: reproduce-before-fix policy; Tracks A/B/C; H1/H2/H3 hypothesis ranking; "open questions" list.
|
||||
|
||||
### Internal references
|
||||
- `desktopApp/compose-rules.pro:85-96` — incorrect keep rule (`pt.davidafsilva.apple.**`)
|
||||
- `desktopApp/compose-rules.pro:148-150` — JNA keep rules (correct, retain as-is)
|
||||
- `gradle/libs.versions.toml:166` — `java-keyring` dependency
|
||||
- `commons/src/jvmMain/kotlin/com/vitorpamplona/amethyst/commons/keystorage/SecureKeyStorage.kt:110-127, 200-218` — failure path (silent null on backend latch)
|
||||
- `desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManager.kt:240-289` — cold-boot routing
|
||||
- `desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/account/AccountManager.kt:291-307` — `loadBunkerAccount`
|
||||
- Related commits: `46caa4d79` (security hardening), `325a1f6f6` (review fixes), `0e431d674` (loading screen), `c0c055e77` (release smoke test pattern for #2819)
|
||||
- Related plan: [docs/plans/2026-05-14-fix-account-security-hardening-plan.md](docs/plans/2026-05-14-fix-account-security-hardening-plan.md) — overlapping `accounts.json.enc` cold-boot work; coordinate before merge.
|
||||
|
||||
### External references
|
||||
- `java-keyring` README: https://github.com/javakeyring/java-keyring — backend selection mechanism
|
||||
- Compose Multiplatform 1.11.0 ProGuard release notes — context for why ProGuard was added in v1.09.1
|
||||
|
||||
## Unanswered Questions
|
||||
|
||||
- Is the existing release-DMG smoke test infrastructure (`c0c055e77`) capable of preserving state between two app launches? Need to read it before extending — may require new harness work.
|
||||
- Should Phase 2 reasonably be deferred to a follow-up plan? Argument for inlining: tiny, prevents future silent-keychain-failure regressions. Argument against: scope creep on what should be a one-line ProGuard fix.
|
||||
- Backport policy: is a point-release expected, or does this ride the next minor? Affects urgency of Phase 5.
|
||||
- Does the `pt.davidafsilva.apple.**` keep rule predate the dep swap, or did somebody copy-paste from another project's config? Git blame on `compose-rules.pro:94` will answer in seconds.
|
||||
Reference in New Issue
Block a user