From cc41e70d9a04f30363ee44271283cf37742ccec2 Mon Sep 17 00:00:00 2001 From: Kyle Santiago Date: Mon, 19 Oct 2026 22:42:25 -0400 Subject: [PATCH 01/13] Drop the previous account's bunker clients and grants on an account switch --- .../privkey/keep/nip46/BunkerRevokeE2ETest.kt | 94 +++++++++++++++++-- .../kotlin/io/privkey/keep/KeepMobileApp.kt | 26 +++-- .../io/privkey/keep/nip46/BunkerService.kt | 71 ++++++++++++++ testharness/nip46probe/README.md | 5 +- testharness/nip46probe/run-revoke-e2e.sh | 9 +- testharness/nip46probe/src/main.rs | 2 + 6 files changed, 185 insertions(+), 22 deletions(-) diff --git a/app/src/androidTest/kotlin/io/privkey/keep/nip46/BunkerRevokeE2ETest.kt b/app/src/androidTest/kotlin/io/privkey/keep/nip46/BunkerRevokeE2ETest.kt index 3e2a7543..d8f29a45 100644 --- a/app/src/androidTest/kotlin/io/privkey/keep/nip46/BunkerRevokeE2ETest.kt +++ b/app/src/androidTest/kotlin/io/privkey/keep/nip46/BunkerRevokeE2ETest.kt @@ -80,6 +80,7 @@ class BunkerRevokeE2ETest { private val dir by lazy { File(context.filesDir, "bunker-e2e") } private val savedFile by lazy { File(context.filesDir, "bunker-e2e-saved.json") } private val clientsFile by lazy { File(context.filesDir, "bunker-e2e-clients.txt") } + private val testAccounts = mutableSetOf() private val testClients = mutableSetOf() private val approvedClients = mutableSetOf() private var requests = 0 @@ -104,6 +105,7 @@ class BunkerRevokeE2ETest { saved.writeTo(savedFile) var passed = false try { + val secondAccount = importSecondAccount(mobile, storage) importThrowawayAccount(mobile, storage) val bunkerRelay = InstrumentationRegistry.getArguments().getString(RELAY_ARG) ?: FrostSignFixture.RELAY mobile.saveRelayConfig(null, RelayConfigInfo(saved.relays.frostRelays, saved.relays.profileRelays, listOf(bunkerRelay))) @@ -199,6 +201,46 @@ class BunkerRevokeE2ETest { assertNull("the client's stored entry must go", Nip46ClientStore.getClient(context, c.pubkey)) } assertDead(url4, url5) + + // Account switch: the previous account's clients, and their remembered grants, + // must not act as the new account. + run { + val c9 = newClient() + assertOk(settled("connect", c9.secret, url5)) + awaitAuthorized(mobile, c9) + val remembered = probeAsync("sign", c9.secret, url5, "1", "90") + approve(c9.pubkey) + assertOk(awaitProbe(remembered)) + stopService() + switchAccount(mobile, storage, secondAccount) + FrostSignTestSupport.initializeWithDecryptContext(mobile, storage, "bunker-e2e-switch-init") + runBlocking { app.onAccountSwitched() } + val afterSwitch = mobile.getBunkerConfig() + assertFalse("the switch turns the bunker off", afterSwitch.enabled) + assertTrue("the switch drops the previous account's clients", afterSwitch.authorizedClients.isEmpty()) + assertNull(Nip46ClientStore.getClient(context, c9.pubkey)) + // The user turns the bunker back on under the new account. + mobile.saveBunkerConfig(BunkerConfigInfo(true, emptyList())) + BunkerService.start(context) + val url6 = awaitRunning(previous = null) + assertRotated(url5, url6) + val switched = System.currentTimeMillis() + assertRefused( + "a grant remembered under the previous account must not sign as the new one", + settled("sign", c9.secret, url6, "1", "15"), c9.pubkey, switched, reason = "Permission denied", + ) + assertFalse("the mark is cleared once the grants are revoked", markers().getBoolean(BunkerService.KEY_REVOKE_ALL_ON_START, false)) + // And it stays revoked: a later start must not restore it. + stopService() + BunkerService.start(context) + assertEquals(url6, awaitRunning(previous = null)) + val restarted = System.currentTimeMillis() + assertRefused( + "a revoked grant must stay revoked across restarts", + settled("sign", c9.secret, url6, "1", "15"), c9.pubkey, restarted, reason = "Permission denied", + ) + assertDead(url5, url6) + } passed = true } finally { screen = Screen.None @@ -238,7 +280,7 @@ class BunkerRevokeE2ETest { } step("stop the bunker") { stopService() } step("rotate the bunker URL") { runBlocking { BunkerService.rotateBunkerCredentials(mobile) } } - failures += saved.restore(context, mobile, storage, testClients, approvedClients) + failures += saved.restore(context, mobile, storage, testClients, approvedClients, testAccounts) step("delete the request files") { check(!dir.exists() || dir.deleteRecursively()) } if (failures.isEmpty()) step("delete the saved state") { check(savedFile.delete() && (!clientsFile.exists() || clientsFile.delete())) } return failures @@ -259,8 +301,9 @@ class BunkerRevokeE2ETest { val recorded = if (clientsFile.exists()) clientsFile.readLines().mapNotNull { line -> line.split(" ").takeIf { it.size == 2 } } else emptyList() failures += SavedState.readFrom(savedFile).restore( context, mobile, storage, - testClients = recorded.map { it[1] }.toSet(), + testClients = recorded.filter { it[0] != ACCOUNT }.map { it[1] }.toSet(), approvedClients = recorded.filter { it[0] == APPROVED }.map { it[1] }.toSet(), + testAccounts = recorded.filter { it[0] == ACCOUNT }.map { it[1] }.toSet(), ) check(failures.isEmpty()) { "could not restore the state an interrupted run saved (see README): ${failures.joinToString()}" @@ -292,6 +335,23 @@ class BunkerRevokeE2ETest { compose.waitUntil(60_000) { compose.onAllNodesWithText(label).fetchSemanticsNodes().isEmpty() } } + // A second account the test owns, for the account switch. Random, so it is never one of + // the device's own; recorded before use so cleanup and recovery delete it. + private fun importSecondAccount(mobile: KeepMobile, storage: AndroidKeystoreStorage): String { + val secret = ByteArray(32).also { SecureRandom().nextBytes(it) }.joinToString("") { "%02x".format(it) } + val nsec = probe("nsec", secret) + check(nsec.startsWith("nsec1")) { "probe returned no nsec: $nsec" } + // An nsec account's group key is the key's own pubkey, so it can be recorded first. + val expected = probe("pubkey", secret) + clientsFile.appendText("$ACCOUNT $expected\n") + testAccounts += expected + val group = withCipher(storage, "bunker-e2e-import-second", storage.getCipherForEncryption()) { + mobile.importNsec(nsec.toByteArray(), "bunker-e2e-second") + }.groupPubkey + check(group == expected) { "unexpected group key $group for the second account" } + return group + } + private fun importThrowawayAccount(mobile: KeepMobile, storage: AndroidKeystoreStorage) { val info = withCipher(storage, "bunker-e2e-import", storage.getCipherForEncryption()) { mobile.importNsec(THROWAWAY_NSEC.toByteArray(), "bunker-e2e") @@ -418,13 +478,13 @@ class BunkerRevokeE2ETest { // The core does not answer a client that holds no permission, so a refusal can reach the // probe as a TIMEOUT. The bunker's own log entry is what proves the request arrived and failed. - private fun assertRefused(message: String, result: String, pubkey: String, since: Long) { + private fun assertRefused(message: String, result: String, pubkey: String, since: Long, reason: String? = null) { assertFalse("$message; got: $result", result.startsWith("OK ")) val log = EventLogStore.getInstance(Nip55Database.getInstance(context)) awaitTrue("the bunker logs the refused sign from ${pubkey.take(8)}") { runBlocking { log.getRecent(200) }.any { it.timestamp >= since && it.category == EventLogCategory.BUNKER.name && it.level == EventLogLevel.WARN.name && - it.source == pubkey.take(8) && it.message.startsWith("sign_event") + it.source == pubkey.take(8) && it.message.startsWith("sign_event") && (reason == null || reason in it.message) } } } @@ -437,6 +497,7 @@ class BunkerRevokeE2ETest { val bunker: BunkerConfigInfo, val denylist: Set, val marker: Boolean, + val revokeAllMark: Boolean, ) { /** Restores each part on its own, so one failure does not skip the rest. Returns the failures. */ /** @@ -450,6 +511,7 @@ class BunkerRevokeE2ETest { storage: AndroidKeystoreStorage, testClients: Set, approvedClients: Set, + testAccounts: Set, ): List { val failures = mutableListOf() fun step(name: String, block: () -> Unit) = runCatching(block).onFailure { @@ -462,12 +524,18 @@ class BunkerRevokeE2ETest { (testClients - approvedClients - denylist).forEach { Nip46ClientStore.removeFromDenylist(context, it) } Nip46ClientStore.addAllToDenylist(context, denylist + approvedClients) } - step("rotation marker") { + step("bunker markers") { check(context.getSharedPreferences(BunkerService.BUNKER_MARKERS_PREFS, Context.MODE_PRIVATE) - .edit().putBoolean(BunkerService.KEY_ROTATED_FOR_EARLIER_REVOKES, marker).commit()) + .edit() + .putBoolean(BunkerService.KEY_ROTATED_FOR_EARLIER_REVOKES, marker) + .putBoolean(BunkerService.KEY_REVOKE_ALL_ON_START, revokeAllMark) + .commit()) + } + step("active account") { activeKey?.takeIf { it != THROWAWAY_GROUP && it !in testAccounts }?.let { switchAccount(mobile, storage, it) } } + step("throwaway account") { dropThrowawayAccount(mobile, storage, testAccounts) } + testAccounts.forEach { key -> + step("test account ${key.take(8)}") { if (mobile.listShares().any { it.groupPubkey == key }) mobile.deleteShareByKey(key) } } - step("active account") { activeKey?.takeIf { it != THROWAWAY_GROUP }?.let { switchAccount(mobile, storage, it) } } - step("throwaway account") { dropThrowawayAccount(mobile, storage) } return failures } @@ -481,6 +549,7 @@ class BunkerRevokeE2ETest { .put("authorizedClients", JSONArray(bunker.authorizedClients)) .put("denylist", JSONArray(denylist.toList())) .put("marker", marker) + .put("revokeAllMark", revokeAllMark) val tmp = File(file.path + ".tmp") tmp.writeText(json.toString()) check(tmp.renameTo(file)) @@ -494,6 +563,8 @@ class BunkerRevokeE2ETest { denylist = Nip46ClientStore.getDenylist(context), marker = context.getSharedPreferences(BunkerService.BUNKER_MARKERS_PREFS, Context.MODE_PRIVATE) .getBoolean(BunkerService.KEY_ROTATED_FOR_EARLIER_REVOKES, false), + revokeAllMark = context.getSharedPreferences(BunkerService.BUNKER_MARKERS_PREFS, Context.MODE_PRIVATE) + .getBoolean(BunkerService.KEY_REVOKE_ALL_ON_START, false), ) fun readFrom(file: File): SavedState { @@ -505,6 +576,7 @@ class BunkerRevokeE2ETest { bunker = BunkerConfigInfo(json.getBoolean("bunkerEnabled"), list("authorizedClients")), denylist = list("denylist").toSet(), marker = json.getBoolean("marker"), + revokeAllMark = json.optBoolean("revokeAllMark", false), ) } } @@ -517,6 +589,7 @@ class BunkerRevokeE2ETest { // The NIP-19 test vector key: public, used only for this temporary account. const val CLIENT = "client" const val APPROVED = "approved" + const val ACCOUNT = "account" const val THROWAWAY_NSEC = "nsec1vl029mgpspedva04g90vltkh6fvh240zqtv9k0t9af8935ke9laqsnlfe5" const val THROWAWAY_GROUP = "7e7e9c42a91bfef19fa929e5fda1b72e0ebc1a4c1141673e2794234d86addf4e" @@ -526,11 +599,12 @@ class BunkerRevokeE2ETest { } // Matched by its key, so no account of the device's own is ever deleted. - fun dropThrowawayAccount(mobile: KeepMobile, storage: AndroidKeystoreStorage) { + fun dropThrowawayAccount(mobile: KeepMobile, storage: AndroidKeystoreStorage, testAccounts: Set = emptySet()) { val shares = mobile.listShares() if (shares.none { it.groupPubkey == THROWAWAY_GROUP }) return if (storage.getActiveShareKey() == THROWAWAY_GROUP) { - shares.firstOrNull { it.groupPubkey != THROWAWAY_GROUP }?.let { switchAccount(mobile, storage, it.groupPubkey) } + shares.firstOrNull { it.groupPubkey != THROWAWAY_GROUP && it.groupPubkey !in testAccounts } + ?.let { switchAccount(mobile, storage, it.groupPubkey) } } mobile.deleteShareByKey(THROWAWAY_GROUP) } diff --git a/app/src/main/kotlin/io/privkey/keep/KeepMobileApp.kt b/app/src/main/kotlin/io/privkey/keep/KeepMobileApp.kt index e306ef06..9b6874c0 100644 --- a/app/src/main/kotlin/io/privkey/keep/KeepMobileApp.kt +++ b/app/src/main/kotlin/io/privkey/keep/KeepMobileApp.kt @@ -495,15 +495,18 @@ class KeepMobileApp : Application() { reconnectJob?.cancel() pinMismatch = null BunkerService.stop(this) - keepMobile?.let { mobile -> - runCatching { - BunkerConfigStore.update(mobile) { current -> - BunkerConfigInfo(false, current.authorizedClients) - } - } - } DescriptorSessionManager.clearAll() withContext(Dispatchers.IO) { + // The bunker's clients belong to the previous account; also turns the bunker off. + var bunkerResetFailed = false + runAccountSwitchCleanup("reset bunker clients") { + try { + keepMobile?.let { BunkerService.resetForAccountSwitch(this@KeepMobileApp, it) } + } catch (e: Exception) { + if (e !is CancellationException) bunkerResetFailed = true + throw e + } + } // Before the revoke: clearAllAppSettings enumerates permission callers to // find overrides whose mirror row is already gone, and revokeAllPermissions // empties that index. @@ -515,6 +518,15 @@ class KeepMobileApp : Application() { runAccountSwitchCleanup("clear auto-sign rate limits") { signingRateLimiter?.clearAll() } runAccountSwitchCleanup("clear signing audit log") { permissionStore?.clearAuditLog() } runAccountSwitchCleanup("clear activity log") { eventLogStore?.clear() } + // After the wipe, so it survives: the person has to know before turning the bunker on. + if (bunkerResetFailed) { + runAccountSwitchCleanup("record bunker reset failure") { + eventLogStore?.log( + EventLogCategory.BUNKER, EventLogLevel.ERROR, "bunker", + "The bunker could not be fully reset for this account; rotate the bunker URL before turning it on" + ) + } + } // Genesis entry for the new account's fresh audit chain: the per-account wipe // above clears the prior trail, so record the activation itself here to keep // every account switch auditable (a switch is otherwise evidence-free). diff --git a/app/src/main/kotlin/io/privkey/keep/nip46/BunkerService.kt b/app/src/main/kotlin/io/privkey/keep/nip46/BunkerService.kt index 3a88a2d0..415366ad 100644 --- a/app/src/main/kotlin/io/privkey/keep/nip46/BunkerService.kt +++ b/app/src/main/kotlin/io/privkey/keep/nip46/BunkerService.kt @@ -93,6 +93,7 @@ class BunkerService : Service() { internal const val MAX_AUTHORIZED_CLIENTS = 100 internal const val BUNKER_MARKERS_PREFS = "keep_bunker_markers" internal const val KEY_ROTATED_FOR_EARLIER_REVOKES = "rotated_for_earlier_revokes" + internal const val KEY_REVOKE_ALL_ON_START = "revoke_all_on_start" private val HEX_PUBKEY_REGEX = Regex("^[a-fA-F0-9]{64}$") @@ -206,6 +207,58 @@ class BunkerService : Service() { return saved } + /** + * Called on an account switch. The bunker's URL, its authorized clients and their + * remembered grants are all device-wide, so without this the clients paired under + * the previous account would act as the new one: requests reach the user under the + * new identity and remembered kinds sign with no prompt. Drops the clients, turns + * the bunker off, rotates the URL, and marks every remembered grant to be revoked + * on the next start, since revoking needs a running engine. A start that finds the + * mark rotates again first, so a rotation that failed here is retried and nothing + * serves the old URL while the old grants are still loaded. Nothing is denylisted, + * so a client can still be paired to the new account deliberately. + * + * Stopgap. The mark and its start-time revoke go once the pinned core revokes all + * clients with no running bunker (keep a one-shot revoke for a mark left set by this + * version). The turn-off, list clear and rotation go once the core keys the bunker + * config by account. The Nip46ClientStore clear stays until that store is per account. + */ + internal suspend fun resetForAccountSwitch(context: Context, keepMobile: io.privkey.keep.uniffi.KeepMobile) { + // Every step runs even if an earlier one failed, since each closes part of it. The + // mark goes first: a reset cut short after it is finished by the next start. + val failures = mutableListOf() + suspend fun step(block: suspend () -> Unit) { + try { + block() + } catch (e: CancellationException) { + throw e + } catch (e: Exception) { + failures += e + } + } + step { check(markers(context).edit().putBoolean(KEY_REVOKE_ALL_ON_START, true).commit()) { "could not mark the grants for revocation" } } + step { + BunkerConfigStore.update(keepMobile) { + // Inside the lock, so a connect save still queued behind it finds its + // client no longer pending and does not write it back (see authorizeClient). + current()?.let { service -> + service.authorizedClientsCache.clear() + service.pendingAuthSaves.clear() + } + io.privkey.keep.uniffi.BunkerConfigInfo(false, emptyList()) + } + } + step { Nip46ClientStore.getClients(context).keys.forEach { Nip46ClientStore.removeClient(context, it) } } + // A nostrconnect request approved under the previous account must not be answered + // under the new one. + pendingNostrConnectRequests.clear() + // Standalone: the service is being stopped for the switch and must not restart. + step { BunkerHandler(keepMobile).use { it.rotateBunkerCredentials() } } + failures.firstOrNull()?.let { first -> throw first.also { failures.drop(1).forEach(it::addSuppressed) } } + } + + private fun markers(context: Context) = context.getSharedPreferences(BUNKER_MARKERS_PREFS, Context.MODE_PRIVATE) + /** * Rotate the bunker credentials whether or not the service is running. */ @@ -495,6 +548,11 @@ class BunkerService : Service() { _status.value = BunkerStatus.STARTING bunkerHandler?.let { runCatching { it.stopBunker() } } + // After an account switch, before anything serves: retries a rotation the switch + // could not do, and keeps the old URL dead while the old grants are loaded. + if (markers(this).getBoolean(KEY_REVOKE_ALL_ON_START, false)) { + BunkerHandler(keepMobile).use { it.rotateBunkerCredentials() } + } if (rotateOnceForEarlierRevokes(this, keepMobile)) { logActivity(EventLogCategory.BUNKER, EventLogLevel.WARN, "bunker", "URL rotated because a client was revoked before this version; pair clients again") } @@ -571,6 +629,19 @@ class BunkerService : Service() { Nip46ClientStore.getDenylist(this) .filter { HEX_PUBKEY_REGEX.matches(it) } .forEach { handler.revokeClient(it) } + // An account switch dropped the previous account's clients; their remembered + // grants were restored by this start and go here. The revoke is a silent no-op + // once onDestroy has stopped the handler, and onDestroy clears serving first, so + // the mark is kept for the next start unless this one is still serving. Ceiling: + // the pinned core also reports success when it fails to save the emptied grants; + // a core whose revoke returns a result closes that. + val markers = markers(this) + if (markers.getBoolean(KEY_REVOKE_ALL_ON_START, false)) { + handler.revokeAllClients() + if (serving) { + check(markers.edit().remove(KEY_REVOKE_ALL_ON_START).commit()) { "could not clear the revoke-all mark" } + } + } } catch (e: Exception) { runCatching { handler.stopBunker() } throw e diff --git a/testharness/nip46probe/README.md b/testharness/nip46probe/README.md index 6ef6ac20..882c2cc4 100644 --- a/testharness/nip46probe/README.md +++ b/testharness/nip46probe/README.md @@ -2,13 +2,13 @@ Host-side NIP-46 client for the bunker revoke end-to-end test, `BunkerRevokeE2ETest`, which is a **manual-only** instrumented test (skipped by default). -The test runs the real `BunkerService` against a public relay and drives each revoke path through its real screen: Revoke on the Bunker screen with the bunker stopped and running, and Disconnect in App Permissions. After each one it asks this probe, a real NIP-46 client on the host, to connect or sign over the relay and checks the answer: +The test runs the real `BunkerService` against a public relay and drives each revoke path through its real screen: Revoke and Revoke All on the Bunker screen with the bunker stopped and running, Disconnect in App Permissions, and an account switch. After each one it asks this probe, a real NIP-46 client on the host, to connect or sign over the relay and checks the answer: - a client signs with a remembered grant after a plain restart (control), and is refused after a revoke made while the bunker was stopped; - the old bunker URL no longer connects after a revoke, and the new one does over the same relay; - a prompt approved after its client was revoked does not sign. -The probe and the test exchange requests through the app's `files/bunker-e2e` directory over `adb shell run-as`. Each request reaches the probe on stdin, so client secrets and bunker URLs never appear in a process listing. The probe prints one line per request: `OK`, `ERR`, `TIMEOUT`, or `UNSENT` when no relay took the request, so a request that never left cannot pass for a dead URL. +The probe and the test exchange requests through the app's `files/bunker-e2e` directory over `adb shell run-as`. Requests reach the probe, and its replies reach the device, on stdin, so client secrets, test account keys and bunker URLs never appear in a process listing. The probe prints one line per request: `OK`, `ERR`, `TIMEOUT`, or `UNSENT` when no relay took the request, so a request that never left cannot pass for a dead URL. ## Run @@ -23,5 +23,6 @@ Notes: - The test imports a temporary account from a public test-vector key (the fixture share is 2-of-3 and cannot sign alone). It saves the device's accounts, relay and bunker settings, denylist and rotation marker to a file before changing anything, and restores them afterwards, or at the start of the next run if this one was killed. - Every run ends with a fresh bunker URL, and the test clients it approved (two per run) stay on the device's denylist, so no test client keeps a way in. Clients paired with the device's bunker before the run need to pair again. - If a killed run's state can no longer be restored (for example, the account that was active has since been deleted), every later run fails at the start and says so. Set the device up by hand, then remove the saved state with `adb shell run-as io.privkey.keep rm files/bunker-e2e-saved.json files/bunker-e2e-clients.txt`. +- For the account switch it imports a second temporary account from a random key, makes it active, and runs the app's account-switch cleanup, which also clears the device's NIP-55 permissions, the app's NIP-46 client records, and the activity and signing logs, as any switch does. Use a test device. Both temporary accounts are deleted afterwards. - The prompt's Approve button requires a biometric, so the test answers the prompt the way that button does once the biometric succeeds. - `Cargo.lock` is committed so the probe builds against the same `nostr` versions as the core. Build output lands in `target/` and is not committed. diff --git a/testharness/nip46probe/run-revoke-e2e.sh b/testharness/nip46probe/run-revoke-e2e.sh index e5d7d034..f85cfa3d 100755 --- a/testharness/nip46probe/run-revoke-e2e.sh +++ b/testharness/nip46probe/run-revoke-e2e.sh @@ -28,9 +28,12 @@ while kill -0 "$instrumentation" 2>/dev/null; do # A probe that fails still answers, so the test reports it instead of waiting out its timeout. out="$(printf '%s\n' "$req" | NIP46PROBE_DEBUG=1 "$probe" 2>>"$probe_log" | tail -n 1)" || true [ -n "$out" ] || out="ERR probe failed, see $probe_log" - encoded="$(printf '%s' "$out" | base64 -w0)" - if on_device "echo $encoded | base64 -d > files/bunker-e2e/res-$n.tmp && mv files/bunker-e2e/res-$n.tmp files/bunker-e2e/res-$n"; then - echo "request $n: ${req%% *} -> ${out:0:120}" + # The reply goes to the device on stdin too, since some replies (an nsec) are secrets. + if printf '%s' "$out" | adb shell -T "run-as $pkg sh -c 'cat > files/bunker-e2e/res-$n.tmp && mv files/bunker-e2e/res-$n.tmp files/bunker-e2e/res-$n'"; then + case "$req" in + nsec\ *) echo "request $n: nsec -> (secret, not shown)" ;; + *) echo "request $n: ${req%% *} -> ${out:0:120}" ;; + esac n=$((n + 1)) fi else diff --git a/testharness/nip46probe/src/main.rs b/testharness/nip46probe/src/main.rs index 26aa658e..a84823a1 100644 --- a/testharness/nip46probe/src/main.rs +++ b/testharness/nip46probe/src/main.rs @@ -2,6 +2,7 @@ //! so client secrets and bunker URLs never appear on a command line: //! //! pubkey +//! nsec (bech32 form, for importing a test account) //! connect [timeout_secs] //! sign [timeout_secs] //! @@ -24,6 +25,7 @@ async fn main() { let timeout = |i: usize| args.get(i).map(|s| s.parse().expect("timeout")).unwrap_or(25); match args[0].as_str() { "pubkey" => println!("{}", keys.public_key().to_hex()), + "nsec" => println!("{}", keys.secret_key().to_bech32().expect("bech32")), "connect" => { let (signer, relays, secret) = bunker(&args[2]); let mut params = vec![signer.to_hex()]; From 26db5ba8db1a5d7d924d243c7c1371e149250432 Mon Sep 17 00:00:00 2001 From: Kyle Santiago Date: Tue, 20 Oct 2026 09:33:03 -0400 Subject: [PATCH 02/13] Stop an account switch when the bunker reset fails --- .../privkey/keep/nip46/BunkerRevokeE2ETest.kt | 41 ++++++++++++++++++- .../kotlin/io/privkey/keep/AccountActions.kt | 12 ++++-- .../kotlin/io/privkey/keep/KeepMobileApp.kt | 6 ++- .../kotlin/io/privkey/keep/MainActivity.kt | 2 +- .../io/privkey/keep/nip46/BunkerService.kt | 35 +++++++++++++--- 5 files changed, 82 insertions(+), 14 deletions(-) diff --git a/app/src/androidTest/kotlin/io/privkey/keep/nip46/BunkerRevokeE2ETest.kt b/app/src/androidTest/kotlin/io/privkey/keep/nip46/BunkerRevokeE2ETest.kt index d8f29a45..3d47c444 100644 --- a/app/src/androidTest/kotlin/io/privkey/keep/nip46/BunkerRevokeE2ETest.kt +++ b/app/src/androidTest/kotlin/io/privkey/keep/nip46/BunkerRevokeE2ETest.kt @@ -23,7 +23,9 @@ import androidx.test.ext.junit.runners.AndroidJUnit4 import androidx.test.platform.app.InstrumentationRegistry import androidx.test.runner.lifecycle.ActivityLifecycleMonitorRegistry import androidx.test.runner.lifecycle.Stage +import io.privkey.keep.AccountActions import io.privkey.keep.KeepMobileApp +import io.privkey.keep.toAccountInfo import io.privkey.keep.nip55.AppPermissionsScreen import io.privkey.keep.nip55.EventLogCategory import io.privkey.keep.nip55.EventLogLevel @@ -42,7 +44,13 @@ import io.privkey.keep.uniffi.KeepMobile import io.privkey.keep.uniffi.RelayConfigInfo import io.privkey.keep.uniffi.parseBunkerUrl import io.privkey.keep.uniffi.truncateStr +import kotlinx.coroutines.CompletableDeferred +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.cancel import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.withTimeout import org.json.JSONArray import org.json.JSONObject import org.junit.Assert.assertEquals @@ -212,9 +220,10 @@ class BunkerRevokeE2ETest { approve(c9.pubkey) assertOk(awaitProbe(remembered)) stopService() + assertSwitchStopsWhenResetFails(mobile, storage, secondAccount) switchAccount(mobile, storage, secondAccount) FrostSignTestSupport.initializeWithDecryptContext(mobile, storage, "bunker-e2e-switch-init") - runBlocking { app.onAccountSwitched() } + assertTrue("the bunker reset must succeed", runBlocking { app.onAccountSwitched() }) val afterSwitch = mobile.getBunkerConfig() assertFalse("the switch turns the bunker off", afterSwitch.enabled) assertTrue("the switch drops the previous account's clients", afterSwitch.authorizedClients.isEmpty()) @@ -337,6 +346,36 @@ class BunkerRevokeE2ETest { // A second account the test owns, for the account switch. Random, so it is never one of // the device's own; recorded before use so cleanup and recovery delete it. + // Drives the app's own switch with a bunker reset that reports failure: the account in use + // must not change, or the previous account's bunker clients would act as the new one. + private fun assertSwitchStopsWhenResetFails(mobile: KeepMobile, storage: AndroidKeystoreStorage, target: String) { + val activeBefore = requireNotNull(storage.getActiveShareKey()) { "no active account" } + val account = storage.listAllShares().map { it.toAccountInfo() }.single { it.groupPubkeyHex == target } + val scope = CoroutineScope(SupervisorJob() + Dispatchers.Default) + val dismissed = CompletableDeferred() + var resetAsked = false + try { + val actions = AccountActions( + keepMobile = mobile, + storage = storage, + coroutineScope = scope, + appContext = context, + onBiometricRequest = { _, _, cipher, onResult -> onResult(cipher) }, + onAccountSwitched = { resetAsked = true; false }, + onAccountDeleted = {}, + onLiveNodeDropped = {}, + onStateChanged = {}, + ) + actions.setCurrentRelays(mobile.getRelayConfig(activeBefore).frostRelays) + actions.switchAccount(account) { dismissed.complete(Unit) } + runBlocking { withTimeout(30_000) { dismissed.await() } } + } finally { + scope.cancel() + } + assertTrue("the switch must ask for the bunker reset", resetAsked) + assertEquals("a failed bunker reset must stop the switch", activeBefore, storage.getActiveShareKey()) + } + private fun importSecondAccount(mobile: KeepMobile, storage: AndroidKeystoreStorage): String { val secret = ByteArray(32).also { SecureRandom().nextBytes(it) }.joinToString("") { "%02x".format(it) } val nsec = probe("nsec", secret) diff --git a/app/src/main/kotlin/io/privkey/keep/AccountActions.kt b/app/src/main/kotlin/io/privkey/keep/AccountActions.kt index 36d30645..0fa3a596 100644 --- a/app/src/main/kotlin/io/privkey/keep/AccountActions.kt +++ b/app/src/main/kotlin/io/privkey/keep/AccountActions.kt @@ -63,7 +63,7 @@ internal class AccountActions( private val coroutineScope: CoroutineScope, private val appContext: Context, private val onBiometricRequest: (String, String, Cipher, (Cipher?) -> Unit) -> Unit, - private val onAccountSwitched: suspend () -> Unit, + private val onAccountSwitched: suspend () -> Boolean, private val onAccountDeleted: suspend () -> Unit, private val onLiveNodeDropped: () -> Unit, private val onStateChanged: (AccountState) -> Unit @@ -226,7 +226,9 @@ internal class AccountActions( // the transition window fail-safe (no grants + old key -> the // request re-prompts). onAccountSwitched clears state account- // agnostically and does not depend on the new active share. - onAccountSwitched() + // A bunker that could not be reset would serve the previous account's + // clients as this one, so the switch stops here, before activation. + check(onAccountSwitched()) { "bunker reset failed" } activateShare(authedCipher, account.groupPubkeyHex) refreshAccountState() } catch (e: Exception) { @@ -329,8 +331,10 @@ internal class AccountActions( // activating the next account's key, so a signing request // racing this switch cannot resolve an old grant against // the new key (revoke-before-activate; see switchAccount). - onAccountSwitched() - if (switchAuthed != null) { + // Skipped like a failed activation when the bunker reset failed + // (see switchAccount); the person then picks an account again. + val bunkerReset = onAccountSwitched() + if (switchAuthed != null && bunkerReset) { try { activateShare(switchAuthed, nextAccount.groupPubkeyHex) } catch (e: Exception) { diff --git a/app/src/main/kotlin/io/privkey/keep/KeepMobileApp.kt b/app/src/main/kotlin/io/privkey/keep/KeepMobileApp.kt index 9b6874c0..f9ecd59d 100644 --- a/app/src/main/kotlin/io/privkey/keep/KeepMobileApp.kt +++ b/app/src/main/kotlin/io/privkey/keep/KeepMobileApp.kt @@ -490,15 +490,16 @@ class KeepMobileApp : Application() { if (BuildConfig.DEBUG) Log.d(TAG, "Initialize completed, peers: ${mobile.getPeers().size}") } - suspend fun onAccountSwitched() { + /** Returns false when the bunker reset failed: the caller must not activate another account. */ + suspend fun onAccountSwitched(): Boolean { connectionJob?.cancel() reconnectJob?.cancel() pinMismatch = null BunkerService.stop(this) DescriptorSessionManager.clearAll() + var bunkerResetFailed = false withContext(Dispatchers.IO) { // The bunker's clients belong to the previous account; also turns the bunker off. - var bunkerResetFailed = false runAccountSwitchCleanup("reset bunker clients") { try { keepMobile?.let { BunkerService.resetForAccountSwitch(this@KeepMobileApp, it) } @@ -549,6 +550,7 @@ class KeepMobileApp : Application() { ) } } + return !bunkerResetFailed } /** diff --git a/app/src/main/kotlin/io/privkey/keep/MainActivity.kt b/app/src/main/kotlin/io/privkey/keep/MainActivity.kt index 518e0bed..db145470 100644 --- a/app/src/main/kotlin/io/privkey/keep/MainActivity.kt +++ b/app/src/main/kotlin/io/privkey/keep/MainActivity.kt @@ -369,7 +369,7 @@ fun MainScreen( onStageCertificatePin: (String, String) -> Boolean = { _, _ -> false }, onRemoveCertificatePin: (String, String) -> CertPinRemoval? = { _, _ -> null }, onDismissPinMismatch: () -> Unit = {}, - onAccountSwitched: suspend () -> Unit = {} + onAccountSwitched: suspend () -> Boolean = { true } ) { val appContext = LocalContext.current.applicationContext val biometricAvailable = biometricStatus == BiometricHelper.BiometricStatus.AVAILABLE diff --git a/app/src/main/kotlin/io/privkey/keep/nip46/BunkerService.kt b/app/src/main/kotlin/io/privkey/keep/nip46/BunkerService.kt index 415366ad..807b4adf 100644 --- a/app/src/main/kotlin/io/privkey/keep/nip46/BunkerService.kt +++ b/app/src/main/kotlin/io/privkey/keep/nip46/BunkerService.kt @@ -236,7 +236,18 @@ class BunkerService : Service() { failures += e } } - step { check(markers(context).edit().putBoolean(KEY_REVOKE_ALL_ON_START, true).commit()) { "could not mark the grants for revocation" } } + // Held in memory too, so a mark that could not be saved still applies to every + // start in this process. Ceiling: that write failing as well as the rotation, and + // the process ending before a start, leaves the old URL and grants in place. An + // explicit switch stops before activating when this throws (see onAccountSwitched); + // an import, or a delete that leaves one account, cannot. + step { + synchronized(revokeAllLock) { + revokeAllRequests++ + revokeAllPending = true + check(markers(context).edit().putBoolean(KEY_REVOKE_ALL_ON_START, true).commit()) { "could not mark the grants for revocation" } + } + } step { BunkerConfigStore.update(keepMobile) { // Inside the lock, so a connect save still queued behind it finds its @@ -259,6 +270,15 @@ class BunkerService : Service() { private fun markers(context: Context) = context.getSharedPreferences(BUNKER_MARKERS_PREFS, Context.MODE_PRIVATE) + private val revokeAllLock = Any() + @Volatile private var revokeAllPending = false + // Guarded by revokeAllLock. A start clears the mark only if no switch marked it again + // after the start read it, since that switch's grants may postdate the revoke. + @Volatile private var revokeAllRequests = 0 + + private fun revokeAllMarked(context: Context) = + revokeAllPending || markers(context).getBoolean(KEY_REVOKE_ALL_ON_START, false) + /** * Rotate the bunker credentials whether or not the service is running. */ @@ -550,7 +570,7 @@ class BunkerService : Service() { bunkerHandler?.let { runCatching { it.stopBunker() } } // After an account switch, before anything serves: retries a rotation the switch // could not do, and keeps the old URL dead while the old grants are loaded. - if (markers(this).getBoolean(KEY_REVOKE_ALL_ON_START, false)) { + if (revokeAllMarked(this)) { BunkerHandler(keepMobile).use { it.rotateBunkerCredentials() } } if (rotateOnceForEarlierRevokes(this, keepMobile)) { @@ -635,11 +655,14 @@ class BunkerService : Service() { // the mark is kept for the next start unless this one is still serving. Ceiling: // the pinned core also reports success when it fails to save the emptied grants; // a core whose revoke returns a result closes that. - val markers = markers(this) - if (markers.getBoolean(KEY_REVOKE_ALL_ON_START, false)) { + val requestsSeen = revokeAllRequests + if (revokeAllMarked(this)) { handler.revokeAllClients() - if (serving) { - check(markers.edit().remove(KEY_REVOKE_ALL_ON_START).commit()) { "could not clear the revoke-all mark" } + synchronized(revokeAllLock) { + if (serving && revokeAllRequests == requestsSeen) { + check(markers(this).edit().remove(KEY_REVOKE_ALL_ON_START).commit()) { "could not clear the revoke-all mark" } + revokeAllPending = false + } } } } catch (e: Exception) { From a075680e52e6ac25a5bc4d165fb7f0b26fd70e20 Mon Sep 17 00:00:00 2001 From: Kyle Santiago Date: Tue, 20 Oct 2026 09:52:40 -0400 Subject: [PATCH 03/13] Reset the bunker before an account switch or delete changes anything --- .../privkey/keep/nip46/BunkerRevokeE2ETest.kt | 53 +++++++++----- .../privkey/keep/nip46/RevokeAllMarkTest.kt | 72 +++++++++++++++++++ .../kotlin/io/privkey/keep/AccountActions.kt | 25 ++++--- .../kotlin/io/privkey/keep/KeepMobileApp.kt | 31 ++++++-- .../kotlin/io/privkey/keep/MainActivity.kt | 4 +- .../io/privkey/keep/nip46/BunkerService.kt | 52 ++++++-------- .../io/privkey/keep/nip46/RevokeAllMark.kt | 43 +++++++++++ 7 files changed, 216 insertions(+), 64 deletions(-) create mode 100644 app/src/androidTest/kotlin/io/privkey/keep/nip46/RevokeAllMarkTest.kt create mode 100644 app/src/main/kotlin/io/privkey/keep/nip46/RevokeAllMark.kt diff --git a/app/src/androidTest/kotlin/io/privkey/keep/nip46/BunkerRevokeE2ETest.kt b/app/src/androidTest/kotlin/io/privkey/keep/nip46/BunkerRevokeE2ETest.kt index 3d47c444..48da1536 100644 --- a/app/src/androidTest/kotlin/io/privkey/keep/nip46/BunkerRevokeE2ETest.kt +++ b/app/src/androidTest/kotlin/io/privkey/keep/nip46/BunkerRevokeE2ETest.kt @@ -220,10 +220,26 @@ class BunkerRevokeE2ETest { approve(c9.pubkey) assertOk(awaitProbe(remembered)) stopService() - assertSwitchStopsWhenResetFails(mobile, storage, secondAccount) - switchAccount(mobile, storage, secondAccount) + val throwaway = THROWAWAY_GROUP + // The delete below must only ever be tried on the test's own account. + check(storage.getActiveShareKey() == throwaway) { "the throwaway account is not the active one" } + var resetAsked = false + val failingReset: suspend (Boolean) -> Boolean = { stop -> resetAsked = stop; false } + // A delete or a switch must stop when the bunker cannot be reset, wiping nothing. + withAccountActions(mobile, storage, failingReset) { actions, done -> actions.deleteAccount(account(storage, throwaway), done) } + assertTrue("the delete must ask to stop on a failed bunker reset", resetAsked) + assertTrue("a failed bunker reset must stop the delete", storage.listAllShares().any { it.toAccountInfo().groupPubkeyHex == throwaway }) + assertEquals(throwaway, storage.getActiveShareKey()) + resetAsked = false + withAccountActions(mobile, storage, failingReset) { actions, done -> actions.switchAccount(account(storage, secondAccount), done) } + assertTrue("the switch must ask to stop on a failed bunker reset", resetAsked) + assertEquals("a failed bunker reset must stop the switch", throwaway, storage.getActiveShareKey()) + // The app's own switch, in its own order: reset, wipe, then activate. + withAccountActions(mobile, storage, { stop -> app.onAccountSwitched(stop) }) { actions, done -> + actions.switchAccount(account(storage, secondAccount), done) + } + assertEquals("the switch must activate the new account", secondAccount, storage.getActiveShareKey()) FrostSignTestSupport.initializeWithDecryptContext(mobile, storage, "bunker-e2e-switch-init") - assertTrue("the bunker reset must succeed", runBlocking { app.onAccountSwitched() }) val afterSwitch = mobile.getBunkerConfig() assertFalse("the switch turns the bunker off", afterSwitch.enabled) assertTrue("the switch drops the previous account's clients", afterSwitch.authorizedClients.isEmpty()) @@ -344,16 +360,19 @@ class BunkerRevokeE2ETest { compose.waitUntil(60_000) { compose.onAllNodesWithText(label).fetchSemanticsNodes().isEmpty() } } - // A second account the test owns, for the account switch. Random, so it is never one of - // the device's own; recorded before use so cleanup and recovery delete it. - // Drives the app's own switch with a bunker reset that reports failure: the account in use - // must not change, or the previous account's bunker clients would act as the new one. - private fun assertSwitchStopsWhenResetFails(mobile: KeepMobile, storage: AndroidKeystoreStorage, target: String) { - val activeBefore = requireNotNull(storage.getActiveShareKey()) { "no active account" } - val account = storage.listAllShares().map { it.toAccountInfo() }.single { it.groupPubkeyHex == target } + private fun account(storage: AndroidKeystoreStorage, key: String) = + storage.listAllShares().map { it.toAccountInfo() }.single { it.groupPubkeyHex == key } + + // Runs one of the app's account actions, the biometric prompt answered with the cipher it + // was given, and waits for the action to dismiss. + private fun withAccountActions( + mobile: KeepMobile, + storage: AndroidKeystoreStorage, + onAccountSwitched: suspend (Boolean) -> Boolean, + action: (AccountActions, () -> Unit) -> Unit, + ) { val scope = CoroutineScope(SupervisorJob() + Dispatchers.Default) val dismissed = CompletableDeferred() - var resetAsked = false try { val actions = AccountActions( keepMobile = mobile, @@ -361,21 +380,21 @@ class BunkerRevokeE2ETest { coroutineScope = scope, appContext = context, onBiometricRequest = { _, _, cipher, onResult -> onResult(cipher) }, - onAccountSwitched = { resetAsked = true; false }, + onAccountSwitched = onAccountSwitched, onAccountDeleted = {}, onLiveNodeDropped = {}, onStateChanged = {}, ) - actions.setCurrentRelays(mobile.getRelayConfig(activeBefore).frostRelays) - actions.switchAccount(account) { dismissed.complete(Unit) } - runBlocking { withTimeout(30_000) { dismissed.await() } } + storage.getActiveShareKey()?.let { actions.setCurrentRelays(mobile.getRelayConfig(it).frostRelays) } + action(actions) { dismissed.complete(Unit) } + runBlocking { withTimeout(60_000) { dismissed.await() } } } finally { scope.cancel() } - assertTrue("the switch must ask for the bunker reset", resetAsked) - assertEquals("a failed bunker reset must stop the switch", activeBefore, storage.getActiveShareKey()) } + // A second account the test owns, for the account switch. Random, so it is never one of + // the device's own; recorded before use so cleanup and recovery delete it. private fun importSecondAccount(mobile: KeepMobile, storage: AndroidKeystoreStorage): String { val secret = ByteArray(32).also { SecureRandom().nextBytes(it) }.joinToString("") { "%02x".format(it) } val nsec = probe("nsec", secret) diff --git a/app/src/androidTest/kotlin/io/privkey/keep/nip46/RevokeAllMarkTest.kt b/app/src/androidTest/kotlin/io/privkey/keep/nip46/RevokeAllMarkTest.kt new file mode 100644 index 00000000..f132582a --- /dev/null +++ b/app/src/androidTest/kotlin/io/privkey/keep/nip46/RevokeAllMarkTest.kt @@ -0,0 +1,72 @@ +package io.privkey.keep.nip46 + +import android.content.Context +import android.content.SharedPreferences +import androidx.test.ext.junit.runners.AndroidJUnit4 +import androidx.test.platform.app.InstrumentationRegistry +import org.junit.After +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith + +@RunWith(AndroidJUnit4::class) +class RevokeAllMarkTest { + private lateinit var prefs: SharedPreferences + + @Before + fun setUp() { + prefs = InstrumentationRegistry.getInstrumentation().targetContext + .getSharedPreferences("revoke_all_mark_test", Context.MODE_PRIVATE) + prefs.edit().clear().commit() + } + + @After + fun tearDown() { + prefs.edit().clear().commit() + } + + @Test + fun aSavedMarkOutlivesTheProcess() { + RevokeAllMark(prefs).set() + assertTrue(RevokeAllMark(prefs).isSet()) + } + + @Test + fun aMarkThatWasNotSavedStillAppliesInThisProcess() { + val mark = RevokeAllMark(prefs) + mark.set() + prefs.edit().remove(BunkerService.KEY_REVOKE_ALL_ON_START).commit() + assertTrue(mark.isSet()) + assertFalse(RevokeAllMark(prefs).isSet()) + } + + @Test + fun aStartDoesNotClearAMarkSetAfterItLooked() { + val mark = RevokeAllMark(prefs) + mark.set() + val seen = mark.requestsSoFar() + mark.set() + assertFalse(mark.clearIfUnchanged(seen) { true }) + assertTrue(mark.isSet()) + assertTrue(RevokeAllMark(prefs).isSet()) + } + + @Test + fun aStartThatStoppedServingDoesNotClearTheMark() { + val mark = RevokeAllMark(prefs) + mark.set() + assertFalse(mark.clearIfUnchanged(mark.requestsSoFar()) { false }) + assertTrue(mark.isSet()) + } + + @Test + fun aServingStartClearsTheMarkEverywhere() { + val mark = RevokeAllMark(prefs) + mark.set() + assertTrue(mark.clearIfUnchanged(mark.requestsSoFar()) { true }) + assertFalse(mark.isSet()) + assertFalse(RevokeAllMark(prefs).isSet()) + } +} diff --git a/app/src/main/kotlin/io/privkey/keep/AccountActions.kt b/app/src/main/kotlin/io/privkey/keep/AccountActions.kt index 0fa3a596..55b748fa 100644 --- a/app/src/main/kotlin/io/privkey/keep/AccountActions.kt +++ b/app/src/main/kotlin/io/privkey/keep/AccountActions.kt @@ -63,7 +63,7 @@ internal class AccountActions( private val coroutineScope: CoroutineScope, private val appContext: Context, private val onBiometricRequest: (String, String, Cipher, (Cipher?) -> Unit) -> Unit, - private val onAccountSwitched: suspend () -> Boolean, + private val onAccountSwitched: suspend (stopIfBunkerResetFails: Boolean) -> Boolean, private val onAccountDeleted: suspend () -> Unit, private val onLiveNodeDropped: () -> Unit, private val onStateChanged: (AccountState) -> Unit @@ -148,7 +148,7 @@ internal class AccountActions( val activeAfter = withContext(Dispatchers.IO) { runCatching { storage.getActiveShareKey() }.getOrNull() } - if (storeSwitchedAccount(hadShare, activeBefore, activeAfter)) onAccountSwitched() + if (storeSwitchedAccount(hadShare, activeBefore, activeAfter)) onAccountSwitched(false) } } } @@ -228,7 +228,7 @@ internal class AccountActions( // agnostically and does not depend on the new active share. // A bunker that could not be reset would serve the previous account's // clients as this one, so the switch stops here, before activation. - check(onAccountSwitched()) { "bunker reset failed" } + check(onAccountSwitched(true)) { "bunker reset failed" } activateShare(authedCipher, account.groupPubkeyHex) refreshAccountState() } catch (e: Exception) { @@ -256,8 +256,14 @@ internal class AccountActions( // Before the delete, not after the follow-up switch prompt. The core's // teardown leaves a running bunker serving with the deleted share, and // this account's grants would otherwise stand until that prompt is - // answered, which it may never be if the activity goes away first. - onAccountSwitched() + // answered, which it may never be if the activity goes away first. Stops + // the delete when the bunker cannot be reset: the next account would + // otherwise take over its clients, with one left even without a switch. + if (!onAccountSwitched(true)) { + logAndToast("Delete failed", appContext.getString(R.string.account_delete_failed), IllegalStateException("bunker reset failed")) + onDismiss() + return@withLock + } } try { withContext(Dispatchers.IO) { @@ -303,7 +309,7 @@ internal class AccountActions( switchToNextAccountAfterDelete(remainingAccounts.first(), onDismiss) } else if (wasActive) { try { - onAccountSwitched() + onAccountSwitched(false) } finally { refreshAccountState() onDismiss() @@ -332,8 +338,9 @@ internal class AccountActions( // racing this switch cannot resolve an old grant against // the new key (revoke-before-activate; see switchAccount). // Skipped like a failed activation when the bunker reset failed - // (see switchAccount); the person then picks an account again. - val bunkerReset = onAccountSwitched() + // (see switchAccount). The reset before the delete succeeded, so + // with one account left, which is then in use anyway, this is safe. + val bunkerReset = onAccountSwitched(true) if (switchAuthed != null && bunkerReset) { try { activateShare(switchAuthed, nextAccount.groupPubkeyHex) @@ -350,7 +357,7 @@ internal class AccountActions( } } else { try { - onAccountSwitched() + onAccountSwitched(false) refreshAccountState() } finally { onDismiss() diff --git a/app/src/main/kotlin/io/privkey/keep/KeepMobileApp.kt b/app/src/main/kotlin/io/privkey/keep/KeepMobileApp.kt index f9ecd59d..47538627 100644 --- a/app/src/main/kotlin/io/privkey/keep/KeepMobileApp.kt +++ b/app/src/main/kotlin/io/privkey/keep/KeepMobileApp.kt @@ -490,16 +490,16 @@ class KeepMobileApp : Application() { if (BuildConfig.DEBUG) Log.d(TAG, "Initialize completed, peers: ${mobile.getPeers().size}") } - /** Returns false when the bunker reset failed: the caller must not activate another account. */ - suspend fun onAccountSwitched(): Boolean { - connectionJob?.cancel() - reconnectJob?.cancel() - pinMismatch = null + /** + * Clears the previous account's state. Returns false when the bunker could not be reset. + * With [stopIfBunkerResetFails] it then stops before wiping anything, and the caller must + * not change the account: the bunker's clients would act as the new one. + */ + suspend fun onAccountSwitched(stopIfBunkerResetFails: Boolean = false): Boolean { BunkerService.stop(this) - DescriptorSessionManager.clearAll() var bunkerResetFailed = false withContext(Dispatchers.IO) { - // The bunker's clients belong to the previous account; also turns the bunker off. + // First: the bunker's clients belong to the previous account; also turns it off. runAccountSwitchCleanup("reset bunker clients") { try { keepMobile?.let { BunkerService.resetForAccountSwitch(this@KeepMobileApp, it) } @@ -508,6 +508,23 @@ class KeepMobileApp : Application() { throw e } } + } + if (bunkerResetFailed && stopIfBunkerResetFails) { + withContext(Dispatchers.IO) { + runAccountSwitchCleanup("record bunker reset failure") { + eventLogStore?.log( + EventLogCategory.BUNKER, EventLogLevel.ERROR, "bunker", + "The bunker could not be reset, so the account was not changed; try again" + ) + } + } + return false + } + connectionJob?.cancel() + reconnectJob?.cancel() + pinMismatch = null + DescriptorSessionManager.clearAll() + withContext(Dispatchers.IO) { // Before the revoke: clearAllAppSettings enumerates permission callers to // find overrides whose mirror row is already gone, and revokeAllPermissions // empties that index. diff --git a/app/src/main/kotlin/io/privkey/keep/MainActivity.kt b/app/src/main/kotlin/io/privkey/keep/MainActivity.kt index db145470..093a53d9 100644 --- a/app/src/main/kotlin/io/privkey/keep/MainActivity.kt +++ b/app/src/main/kotlin/io/privkey/keep/MainActivity.kt @@ -303,7 +303,7 @@ class MainActivity : FragmentActivity() { app.removeCertificatePin(hostname, spkiHash) }, onDismissPinMismatch = app::dismissPinMismatch, - onAccountSwitched = { app.onAccountSwitched() } + onAccountSwitched = { stop -> app.onAccountSwitched(stop) } ) } else { ErrorScreen(app.getInitError() ?: stringResource(R.string.main_init_app_failed)) @@ -369,7 +369,7 @@ fun MainScreen( onStageCertificatePin: (String, String) -> Boolean = { _, _ -> false }, onRemoveCertificatePin: (String, String) -> CertPinRemoval? = { _, _ -> null }, onDismissPinMismatch: () -> Unit = {}, - onAccountSwitched: suspend () -> Boolean = { true } + onAccountSwitched: suspend (Boolean) -> Boolean = { false } ) { val appContext = LocalContext.current.applicationContext val biometricAvailable = biometricStatus == BiometricHelper.BiometricStatus.AVAILABLE diff --git a/app/src/main/kotlin/io/privkey/keep/nip46/BunkerService.kt b/app/src/main/kotlin/io/privkey/keep/nip46/BunkerService.kt index 807b4adf..397c51d3 100644 --- a/app/src/main/kotlin/io/privkey/keep/nip46/BunkerService.kt +++ b/app/src/main/kotlin/io/privkey/keep/nip46/BunkerService.kt @@ -218,12 +218,18 @@ class BunkerService : Service() { * serves the old URL while the old grants are still loaded. Nothing is denylisted, * so a client can still be paired to the new account deliberately. * + * Throws when any step that keeps the clients out failed; the client display list is + * not one of them, since the bunker authorizes from its config. + * * Stopgap. The mark and its start-time revoke go once the pinned core revokes all * clients with no running bunker (keep a one-shot revoke for a mark left set by this * version). The turn-off, list clear and rotation go once the core keys the bunker * config by account. The Nip46ClientStore clear stays until that store is per account. */ internal suspend fun resetForAccountSwitch(context: Context, keepMobile: io.privkey.keep.uniffi.KeepMobile) { + // The stop the switch asked for is asynchronous: halt a running instance now, so a + // start in flight either finishes before the mark exists or serves nothing. + current()?.haltForAccountSwitch() // Every step runs even if an earlier one failed, since each closes part of it. The // mark goes first: a reset cut short after it is finished by the next start. val failures = mutableListOf() @@ -236,18 +242,7 @@ class BunkerService : Service() { failures += e } } - // Held in memory too, so a mark that could not be saved still applies to every - // start in this process. Ceiling: that write failing as well as the rotation, and - // the process ending before a start, leaves the old URL and grants in place. An - // explicit switch stops before activating when this throws (see onAccountSwitched); - // an import, or a delete that leaves one account, cannot. - step { - synchronized(revokeAllLock) { - revokeAllRequests++ - revokeAllPending = true - check(markers(context).edit().putBoolean(KEY_REVOKE_ALL_ON_START, true).commit()) { "could not mark the grants for revocation" } - } - } + step { revokeAllMark(context).set() } step { BunkerConfigStore.update(keepMobile) { // Inside the lock, so a connect save still queued behind it finds its @@ -259,7 +254,8 @@ class BunkerService : Service() { io.privkey.keep.uniffi.BunkerConfigInfo(false, emptyList()) } } - step { Nip46ClientStore.getClients(context).keys.forEach { Nip46ClientStore.removeClient(context, it) } } + runCatching { Nip46ClientStore.getClients(context).keys.forEach { Nip46ClientStore.removeClient(context, it) } } + .onFailure { if (BuildConfig.DEBUG) Log.w(TAG, "Could not clear the NIP-46 client list: ${it::class.simpleName}") } // A nostrconnect request approved under the previous account must not be answered // under the new one. pendingNostrConnectRequests.clear() @@ -270,14 +266,11 @@ class BunkerService : Service() { private fun markers(context: Context) = context.getSharedPreferences(BUNKER_MARKERS_PREFS, Context.MODE_PRIVATE) - private val revokeAllLock = Any() - @Volatile private var revokeAllPending = false - // Guarded by revokeAllLock. A start clears the mark only if no switch marked it again - // after the start read it, since that switch's grants may postdate the revoke. - @Volatile private var revokeAllRequests = 0 + private var revokeAllMarkInstance: RevokeAllMark? = null - private fun revokeAllMarked(context: Context) = - revokeAllPending || markers(context).getBoolean(KEY_REVOKE_ALL_ON_START, false) + @Synchronized + private fun revokeAllMark(context: Context): RevokeAllMark = + revokeAllMarkInstance ?: RevokeAllMark(markers(context.applicationContext)).also { revokeAllMarkInstance = it } /** * Rotate the bunker credentials whether or not the service is running. @@ -517,6 +510,11 @@ class BunkerService : Service() { } } + private suspend fun haltForAccountSwitch() = startStopMutex.withLock { + serving = false + bunkerHandler?.let { runCatching { it.stopBunker() } } + } + /** * Rotate the bunker credentials so a previously distributed (possibly leaked) * bunker:// URL can no longer pair or sign, then restart the bunker to publish @@ -570,7 +568,7 @@ class BunkerService : Service() { bunkerHandler?.let { runCatching { it.stopBunker() } } // After an account switch, before anything serves: retries a rotation the switch // could not do, and keeps the old URL dead while the old grants are loaded. - if (revokeAllMarked(this)) { + if (revokeAllMark(this).isSet()) { BunkerHandler(keepMobile).use { it.rotateBunkerCredentials() } } if (rotateOnceForEarlierRevokes(this, keepMobile)) { @@ -655,15 +653,11 @@ class BunkerService : Service() { // the mark is kept for the next start unless this one is still serving. Ceiling: // the pinned core also reports success when it fails to save the emptied grants; // a core whose revoke returns a result closes that. - val requestsSeen = revokeAllRequests - if (revokeAllMarked(this)) { + val mark = revokeAllMark(this) + val requestsSeen = mark.requestsSoFar() + if (mark.isSet()) { handler.revokeAllClients() - synchronized(revokeAllLock) { - if (serving && revokeAllRequests == requestsSeen) { - check(markers(this).edit().remove(KEY_REVOKE_ALL_ON_START).commit()) { "could not clear the revoke-all mark" } - revokeAllPending = false - } - } + mark.clearIfUnchanged(requestsSeen) { serving } } } catch (e: Exception) { runCatching { handler.stopBunker() } diff --git a/app/src/main/kotlin/io/privkey/keep/nip46/RevokeAllMark.kt b/app/src/main/kotlin/io/privkey/keep/nip46/RevokeAllMark.kt new file mode 100644 index 00000000..42967205 --- /dev/null +++ b/app/src/main/kotlin/io/privkey/keep/nip46/RevokeAllMark.kt @@ -0,0 +1,43 @@ +package io.privkey.keep.nip46 + +import android.content.SharedPreferences + +/** + * The account-switch mark that makes the next bunker start revoke every remembered grant. + * Held in memory as well as in [prefs], so a mark that could not be saved still applies to + * every start in this process. A start clears it only if no switch set it again after the + * start read it, since that switch's grants may postdate the revoke. + * + * Ceiling: a save that failed, or one a power cut rolled back, is lost when the process + * ends, and the next start then restores the previous account's remembered grants. The + * switch's rotation still keeps those clients out unless it failed too; an explicit switch + * or delete stops on that failure, an import cannot. Goes with the start-time revoke. + */ +internal class RevokeAllMark(private val prefs: SharedPreferences) { + private var pending = false + private var requests = 0 + + @Synchronized + fun set() { + requests++ + pending = true + check(prefs.edit().putBoolean(BunkerService.KEY_REVOKE_ALL_ON_START, true).commit()) { + "could not mark the grants for revocation" + } + } + + @Synchronized + fun isSet(): Boolean = pending || prefs.getBoolean(BunkerService.KEY_REVOKE_ALL_ON_START, false) + + @Synchronized + fun requestsSoFar(): Int = requests + + /** Clears the mark unless it was set again after [seen] was read, or [stillServing] says no. */ + @Synchronized + fun clearIfUnchanged(seen: Int, stillServing: () -> Boolean): Boolean { + if (requests != seen || !stillServing()) return false + check(prefs.edit().remove(BunkerService.KEY_REVOKE_ALL_ON_START).commit()) { "could not clear the revoke-all mark" } + pending = false + return true + } +} From e7cab6218577dc3ceebf3b99d4363e38b35f0171 Mon Sep 17 00:00:00 2001 From: Kyle Santiago Date: Tue, 20 Oct 2026 10:01:43 -0400 Subject: [PATCH 04/13] Stop a switch only when no bunker barrier is left --- .../privkey/keep/nip46/BunkerRevokeE2ETest.kt | 26 +++++++++++++++- .../kotlin/io/privkey/keep/AccountActions.kt | 9 +++--- .../kotlin/io/privkey/keep/KeepMobileApp.kt | 8 +++-- .../io/privkey/keep/nip46/BunkerService.kt | 30 ++++++++++++------- .../io/privkey/keep/nip46/RevokeAllMark.kt | 5 ++-- 5 files changed, 59 insertions(+), 19 deletions(-) diff --git a/app/src/androidTest/kotlin/io/privkey/keep/nip46/BunkerRevokeE2ETest.kt b/app/src/androidTest/kotlin/io/privkey/keep/nip46/BunkerRevokeE2ETest.kt index 48da1536..9fe356f9 100644 --- a/app/src/androidTest/kotlin/io/privkey/keep/nip46/BunkerRevokeE2ETest.kt +++ b/app/src/androidTest/kotlin/io/privkey/keep/nip46/BunkerRevokeE2ETest.kt @@ -219,7 +219,11 @@ class BunkerRevokeE2ETest { val remembered = probeAsync("sign", c9.secret, url5, "1", "90") approve(c9.pubkey) assertOk(awaitProbe(remembered)) - stopService() + // The app's own stop: a reset that fails wipes nothing. It also stops the + // bunker, which then serves again so the switch below has one to halt. + assertStoppedSwitchWipesNothing() + BunkerService.start(context) + assertEquals(url5, awaitRunning(previous = null)) val throwaway = THROWAWAY_GROUP // The delete below must only ever be tried on the test's own account. check(storage.getActiveShareKey() == throwaway) { "the throwaway account is not the active one" } @@ -360,6 +364,26 @@ class BunkerRevokeE2ETest { compose.waitUntil(60_000) { compose.onAllNodesWithText(label).fetchSemanticsNodes().isEmpty() } } + private fun assertStoppedSwitchWipesNothing() { + val log = EventLogStore.getInstance(Nip55Database.getInstance(context)) + val sentinel = "bunker-e2e sentinel ${System.nanoTime()}" + runBlocking { log.log(EventLogCategory.BUNKER, EventLogLevel.INFO, "bunker-e2e", sentinel) } + val realReset = app.resetBunkerForSwitch + app.resetBunkerForSwitch = { _, _ -> throw IllegalStateException("bunker-e2e: the reset fails") } + val result = try { + runBlocking { app.onAccountSwitched(true) } + } finally { + app.resetBunkerForSwitch = realReset + } + assertFalse("a reset that fails must be reported", result) + val entries = runBlocking { log.getRecent(200) } + assertTrue("a stopped switch must wipe nothing", entries.any { it.message == sentinel }) + assertTrue( + "a stopped switch must say so", + entries.any { it.level == EventLogLevel.ERROR.name && it.message.startsWith("The bunker was turned off and could not be reset") }, + ) + } + private fun account(storage: AndroidKeystoreStorage, key: String) = storage.listAllShares().map { it.toAccountInfo() }.single { it.groupPubkeyHex == key } diff --git a/app/src/main/kotlin/io/privkey/keep/AccountActions.kt b/app/src/main/kotlin/io/privkey/keep/AccountActions.kt index 55b748fa..35333d29 100644 --- a/app/src/main/kotlin/io/privkey/keep/AccountActions.kt +++ b/app/src/main/kotlin/io/privkey/keep/AccountActions.kt @@ -337,10 +337,11 @@ internal class AccountActions( // activating the next account's key, so a signing request // racing this switch cannot resolve an old grant against // the new key (revoke-before-activate; see switchAccount). - // Skipped like a failed activation when the bunker reset failed - // (see switchAccount). The reset before the delete succeeded, so - // with one account left, which is then in use anyway, this is safe. - val bunkerReset = onAccountSwitched(true) + // Not a stop: the account is already deleted, so the cleanup + // runs whatever the bunker reset does. Activation still waits on + // the reset (see switchAccount); with one account left, which is + // then in use anyway, the reset before the delete already held. + val bunkerReset = onAccountSwitched(false) if (switchAuthed != null && bunkerReset) { try { activateShare(switchAuthed, nextAccount.groupPubkeyHex) diff --git a/app/src/main/kotlin/io/privkey/keep/KeepMobileApp.kt b/app/src/main/kotlin/io/privkey/keep/KeepMobileApp.kt index 47538627..8c0954e0 100644 --- a/app/src/main/kotlin/io/privkey/keep/KeepMobileApp.kt +++ b/app/src/main/kotlin/io/privkey/keep/KeepMobileApp.kt @@ -490,6 +490,10 @@ class KeepMobileApp : Application() { if (BuildConfig.DEBUG) Log.d(TAG, "Initialize completed, peers: ${mobile.getPeers().size}") } + @androidx.annotation.VisibleForTesting + internal var resetBunkerForSwitch: suspend (Context, KeepMobile) -> Unit = + { context, mobile -> BunkerService.resetForAccountSwitch(context, mobile) } + /** * Clears the previous account's state. Returns false when the bunker could not be reset. * With [stopIfBunkerResetFails] it then stops before wiping anything, and the caller must @@ -502,7 +506,7 @@ class KeepMobileApp : Application() { // First: the bunker's clients belong to the previous account; also turns it off. runAccountSwitchCleanup("reset bunker clients") { try { - keepMobile?.let { BunkerService.resetForAccountSwitch(this@KeepMobileApp, it) } + keepMobile?.let { resetBunkerForSwitch(this@KeepMobileApp, it) } } catch (e: Exception) { if (e !is CancellationException) bunkerResetFailed = true throw e @@ -514,7 +518,7 @@ class KeepMobileApp : Application() { runAccountSwitchCleanup("record bunker reset failure") { eventLogStore?.log( EventLogCategory.BUNKER, EventLogLevel.ERROR, "bunker", - "The bunker could not be reset, so the account was not changed; try again" + "The bunker was turned off and could not be reset, so the account was not changed; try again" ) } } diff --git a/app/src/main/kotlin/io/privkey/keep/nip46/BunkerService.kt b/app/src/main/kotlin/io/privkey/keep/nip46/BunkerService.kt index 397c51d3..aca771e4 100644 --- a/app/src/main/kotlin/io/privkey/keep/nip46/BunkerService.kt +++ b/app/src/main/kotlin/io/privkey/keep/nip46/BunkerService.kt @@ -218,8 +218,11 @@ class BunkerService : Service() { * serves the old URL while the old grants are still loaded. Nothing is denylisted, * so a client can still be paired to the new account deliberately. * - * Throws when any step that keeps the clients out failed; the client display list is - * not one of them, since the bunker authorizes from its config. + * Throws only when neither the saved mark nor the rotation took effect: either one + * keeps the clients out, since a saved mark makes every start rotate first and revoke + * every grant, and a rotated URL is one they do not hold. A reset that cannot finish, + * such as one over an unreadable bunker config, then does not block every switch and + * delete for good. * * Stopgap. The mark and its start-time revoke go once the pinned core revokes all * clients with no running bunker (keep a one-shot revoke for a mark left set by this @@ -233,16 +236,17 @@ class BunkerService : Service() { // Every step runs even if an earlier one failed, since each closes part of it. The // mark goes first: a reset cut short after it is finished by the next start. val failures = mutableListOf() - suspend fun step(block: suspend () -> Unit) { + suspend fun step(block: suspend () -> Unit): Boolean = try { block() + true } catch (e: CancellationException) { throw e } catch (e: Exception) { failures += e + false } - } - step { revokeAllMark(context).set() } + val marked = step { revokeAllMark(context).set() } step { BunkerConfigStore.update(keepMobile) { // Inside the lock, so a connect save still queued behind it finds its @@ -260,8 +264,12 @@ class BunkerService : Service() { // under the new one. pendingNostrConnectRequests.clear() // Standalone: the service is being stopped for the switch and must not restart. - step { BunkerHandler(keepMobile).use { it.rotateBunkerCredentials() } } - failures.firstOrNull()?.let { first -> throw first.also { failures.drop(1).forEach(it::addSuppressed) } } + val rotated = step { BunkerHandler(keepMobile).use { it.rotateBunkerCredentials() } } + // Again, now the config is off: a start asked for since the first halt read it on. + stop(context) + current()?.haltForAccountSwitch() + if (!marked && !rotated) throw failures.first().also { first -> failures.drop(1).forEach(first::addSuppressed) } + if (BuildConfig.DEBUG) failures.forEach { Log.w(TAG, "Account switch reset step failed: ${it::class.simpleName}") } } private fun markers(context: Context) = context.getSharedPreferences(BUNKER_MARKERS_PREFS, Context.MODE_PRIVATE) @@ -510,9 +518,11 @@ class BunkerService : Service() { } } - private suspend fun haltForAccountSwitch() = startStopMutex.withLock { - serving = false - bunkerHandler?.let { runCatching { it.stopBunker() } } + private suspend fun haltForAccountSwitch() { + startStopMutex.withLock { + serving = false + bunkerHandler?.let { runCatching { it.stopBunker() } } + } } /** diff --git a/app/src/main/kotlin/io/privkey/keep/nip46/RevokeAllMark.kt b/app/src/main/kotlin/io/privkey/keep/nip46/RevokeAllMark.kt index 42967205..42e89cd2 100644 --- a/app/src/main/kotlin/io/privkey/keep/nip46/RevokeAllMark.kt +++ b/app/src/main/kotlin/io/privkey/keep/nip46/RevokeAllMark.kt @@ -10,8 +10,9 @@ import android.content.SharedPreferences * * Ceiling: a save that failed, or one a power cut rolled back, is lost when the process * ends, and the next start then restores the previous account's remembered grants. The - * switch's rotation still keeps those clients out unless it failed too; an explicit switch - * or delete stops on that failure, an import cannot. Goes with the start-time revoke. + * switch's rotation still keeps those clients out unless it failed or was rolled back too. + * A switch or delete of the active account stops when the save and the rotation both + * failed; an import cannot. Goes with the start-time revoke. */ internal class RevokeAllMark(private val prefs: SharedPreferences) { private var pending = false From 56cde98b0784f9648df12c5adddc03d35cd2a6e2 Mon Sep 17 00:00:00 2001 From: Kyle Santiago Date: Tue, 20 Oct 2026 10:29:32 -0400 Subject: [PATCH 05/13] Record any stop request in the failing-reset stub --- .../kotlin/io/privkey/keep/nip46/BunkerRevokeE2ETest.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/src/androidTest/kotlin/io/privkey/keep/nip46/BunkerRevokeE2ETest.kt b/app/src/androidTest/kotlin/io/privkey/keep/nip46/BunkerRevokeE2ETest.kt index 9fe356f9..d123e9ad 100644 --- a/app/src/androidTest/kotlin/io/privkey/keep/nip46/BunkerRevokeE2ETest.kt +++ b/app/src/androidTest/kotlin/io/privkey/keep/nip46/BunkerRevokeE2ETest.kt @@ -228,7 +228,7 @@ class BunkerRevokeE2ETest { // The delete below must only ever be tried on the test's own account. check(storage.getActiveShareKey() == throwaway) { "the throwaway account is not the active one" } var resetAsked = false - val failingReset: suspend (Boolean) -> Boolean = { stop -> resetAsked = stop; false } + val failingReset: suspend (Boolean) -> Boolean = { stop -> resetAsked = resetAsked || stop; false } // A delete or a switch must stop when the bunker cannot be reset, wiping nothing. withAccountActions(mobile, storage, failingReset) { actions, done -> actions.deleteAccount(account(storage, throwaway), done) } assertTrue("the delete must ask to stop on a failed bunker reset", resetAsked) From ed46adfa2ab0bb35796ca0ad1c8dbc4ff3ec39f8 Mon Sep 17 00:00:00 2001 From: Kyle Santiago Date: Tue, 20 Oct 2026 10:37:41 -0400 Subject: [PATCH 06/13] Require the saved revoke mark before an account switch goes ahead --- .../privkey/keep/nip46/BunkerRevokeE2ETest.kt | 2 +- .../kotlin/io/privkey/keep/KeepMobileApp.kt | 9 +++++---- .../io/privkey/keep/nip46/BunkerService.kt | 17 +++++++++-------- .../io/privkey/keep/nip46/RevokeAllMark.kt | 10 +++++----- 4 files changed, 20 insertions(+), 18 deletions(-) diff --git a/app/src/androidTest/kotlin/io/privkey/keep/nip46/BunkerRevokeE2ETest.kt b/app/src/androidTest/kotlin/io/privkey/keep/nip46/BunkerRevokeE2ETest.kt index d123e9ad..56234030 100644 --- a/app/src/androidTest/kotlin/io/privkey/keep/nip46/BunkerRevokeE2ETest.kt +++ b/app/src/androidTest/kotlin/io/privkey/keep/nip46/BunkerRevokeE2ETest.kt @@ -380,7 +380,7 @@ class BunkerRevokeE2ETest { assertTrue("a stopped switch must wipe nothing", entries.any { it.message == sentinel }) assertTrue( "a stopped switch must say so", - entries.any { it.level == EventLogLevel.ERROR.name && it.message.startsWith("The bunker was turned off and could not be reset") }, + entries.any { it.level == EventLogLevel.ERROR.name && it.message.startsWith("The bunker was stopped and could not be reset") }, ) } diff --git a/app/src/main/kotlin/io/privkey/keep/KeepMobileApp.kt b/app/src/main/kotlin/io/privkey/keep/KeepMobileApp.kt index 8c0954e0..65775e3e 100644 --- a/app/src/main/kotlin/io/privkey/keep/KeepMobileApp.kt +++ b/app/src/main/kotlin/io/privkey/keep/KeepMobileApp.kt @@ -491,7 +491,7 @@ class KeepMobileApp : Application() { } @androidx.annotation.VisibleForTesting - internal var resetBunkerForSwitch: suspend (Context, KeepMobile) -> Unit = + internal var resetBunkerForSwitch: suspend (Context, KeepMobile) -> Boolean = { context, mobile -> BunkerService.resetForAccountSwitch(context, mobile) } /** @@ -502,11 +502,12 @@ class KeepMobileApp : Application() { suspend fun onAccountSwitched(stopIfBunkerResetFails: Boolean = false): Boolean { BunkerService.stop(this) var bunkerResetFailed = false + var bunkerResetComplete = true withContext(Dispatchers.IO) { // First: the bunker's clients belong to the previous account; also turns it off. runAccountSwitchCleanup("reset bunker clients") { try { - keepMobile?.let { resetBunkerForSwitch(this@KeepMobileApp, it) } + keepMobile?.let { bunkerResetComplete = resetBunkerForSwitch(this@KeepMobileApp, it) } } catch (e: Exception) { if (e !is CancellationException) bunkerResetFailed = true throw e @@ -518,7 +519,7 @@ class KeepMobileApp : Application() { runAccountSwitchCleanup("record bunker reset failure") { eventLogStore?.log( EventLogCategory.BUNKER, EventLogLevel.ERROR, "bunker", - "The bunker was turned off and could not be reset, so the account was not changed; try again" + "The bunker was stopped and could not be reset, so the account was not changed; try again" ) } } @@ -541,7 +542,7 @@ class KeepMobileApp : Application() { runAccountSwitchCleanup("clear signing audit log") { permissionStore?.clearAuditLog() } runAccountSwitchCleanup("clear activity log") { eventLogStore?.clear() } // After the wipe, so it survives: the person has to know before turning the bunker on. - if (bunkerResetFailed) { + if (bunkerResetFailed || !bunkerResetComplete) { runAccountSwitchCleanup("record bunker reset failure") { eventLogStore?.log( EventLogCategory.BUNKER, EventLogLevel.ERROR, "bunker", diff --git a/app/src/main/kotlin/io/privkey/keep/nip46/BunkerService.kt b/app/src/main/kotlin/io/privkey/keep/nip46/BunkerService.kt index aca771e4..21ae714a 100644 --- a/app/src/main/kotlin/io/privkey/keep/nip46/BunkerService.kt +++ b/app/src/main/kotlin/io/privkey/keep/nip46/BunkerService.kt @@ -218,18 +218,18 @@ class BunkerService : Service() { * serves the old URL while the old grants are still loaded. Nothing is denylisted, * so a client can still be paired to the new account deliberately. * - * Throws only when neither the saved mark nor the rotation took effect: either one - * keeps the clients out, since a saved mark makes every start rotate first and revoke - * every grant, and a rotated URL is one they do not hold. A reset that cannot finish, - * such as one over an unreadable bunker config, then does not block every switch and - * delete for good. + * Throws when the mark could not be saved, since only it keeps the clients out for good: + * a start that finds it rotates first and then revokes every remembered grant. A rotated + * URL alone does not, because the bunker key is public and a restored grant needs no + * connect. Returns false when another step failed, which the mark still covers, so a + * reset over an unreadable bunker config does not block every switch and delete. * * Stopgap. The mark and its start-time revoke go once the pinned core revokes all * clients with no running bunker (keep a one-shot revoke for a mark left set by this * version). The turn-off, list clear and rotation go once the core keys the bunker * config by account. The Nip46ClientStore clear stays until that store is per account. */ - internal suspend fun resetForAccountSwitch(context: Context, keepMobile: io.privkey.keep.uniffi.KeepMobile) { + internal suspend fun resetForAccountSwitch(context: Context, keepMobile: io.privkey.keep.uniffi.KeepMobile): Boolean { // The stop the switch asked for is asynchronous: halt a running instance now, so a // start in flight either finishes before the mark exists or serves nothing. current()?.haltForAccountSwitch() @@ -264,12 +264,13 @@ class BunkerService : Service() { // under the new one. pendingNostrConnectRequests.clear() // Standalone: the service is being stopped for the switch and must not restart. - val rotated = step { BunkerHandler(keepMobile).use { it.rotateBunkerCredentials() } } + step { BunkerHandler(keepMobile).use { it.rotateBunkerCredentials() } } // Again, now the config is off: a start asked for since the first halt read it on. stop(context) current()?.haltForAccountSwitch() - if (!marked && !rotated) throw failures.first().also { first -> failures.drop(1).forEach(first::addSuppressed) } + if (!marked) throw failures.first().also { first -> failures.drop(1).forEach(first::addSuppressed) } if (BuildConfig.DEBUG) failures.forEach { Log.w(TAG, "Account switch reset step failed: ${it::class.simpleName}") } + return failures.isEmpty() } private fun markers(context: Context) = context.getSharedPreferences(BUNKER_MARKERS_PREFS, Context.MODE_PRIVATE) diff --git a/app/src/main/kotlin/io/privkey/keep/nip46/RevokeAllMark.kt b/app/src/main/kotlin/io/privkey/keep/nip46/RevokeAllMark.kt index 42e89cd2..a3b8bd90 100644 --- a/app/src/main/kotlin/io/privkey/keep/nip46/RevokeAllMark.kt +++ b/app/src/main/kotlin/io/privkey/keep/nip46/RevokeAllMark.kt @@ -8,11 +8,11 @@ import android.content.SharedPreferences * every start in this process. A start clears it only if no switch set it again after the * start read it, since that switch's grants may postdate the revoke. * - * Ceiling: a save that failed, or one a power cut rolled back, is lost when the process - * ends, and the next start then restores the previous account's remembered grants. The - * switch's rotation still keeps those clients out unless it failed or was rolled back too. - * A switch or delete of the active account stops when the save and the rotation both - * failed; an import cannot. Goes with the start-time revoke. + * A save that fails stops a switch or delete of the active account. Ceiling: an import + * cannot be stopped, so there a failed save holds only until the process ends, and a power + * cut can roll back a save that reported success. The next start then restores the previous + * account's remembered grants, and the switch's rotation does not keep a client holding one + * out, since the bunker key is public. Goes with the start-time revoke. */ internal class RevokeAllMark(private val prefs: SharedPreferences) { private var pending = false From 01e9a3472290dc63119126be59b58ca5daa77735 Mon Sep 17 00:00:00 2001 From: Kyle Santiago Date: Tue, 20 Oct 2026 11:01:57 -0400 Subject: [PATCH 07/13] Give each bunker reset outcome an accurate activity-log entry --- .../kotlin/io/privkey/keep/KeepMobileApp.kt | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/app/src/main/kotlin/io/privkey/keep/KeepMobileApp.kt b/app/src/main/kotlin/io/privkey/keep/KeepMobileApp.kt index 65775e3e..c2a87db3 100644 --- a/app/src/main/kotlin/io/privkey/keep/KeepMobileApp.kt +++ b/app/src/main/kotlin/io/privkey/keep/KeepMobileApp.kt @@ -495,9 +495,9 @@ class KeepMobileApp : Application() { { context, mobile -> BunkerService.resetForAccountSwitch(context, mobile) } /** - * Clears the previous account's state. Returns false when the bunker could not be reset. - * With [stopIfBunkerResetFails] it then stops before wiping anything, and the caller must - * not change the account: the bunker's clients would act as the new one. + * Clears the previous account's state. Returns false when the bunker's revoke-all mark + * could not be saved. With [stopIfBunkerResetFails] it then stops before wiping anything, + * and the caller must not change the account: the bunker's clients would act as the new one. */ suspend fun onAccountSwitched(stopIfBunkerResetFails: Boolean = false): Boolean { BunkerService.stop(this) @@ -519,7 +519,7 @@ class KeepMobileApp : Application() { runAccountSwitchCleanup("record bunker reset failure") { eventLogStore?.log( EventLogCategory.BUNKER, EventLogLevel.ERROR, "bunker", - "The bunker was stopped and could not be reset, so the account was not changed; try again" + "The bunker was stopped and could not be reset, so the account was not changed; its clients may need pairing again. Try again" ) } } @@ -542,11 +542,18 @@ class KeepMobileApp : Application() { runAccountSwitchCleanup("clear signing audit log") { permissionStore?.clearAuditLog() } runAccountSwitchCleanup("clear activity log") { eventLogStore?.clear() } // After the wipe, so it survives: the person has to know before turning the bunker on. - if (bunkerResetFailed || !bunkerResetComplete) { + if (bunkerResetFailed) { runAccountSwitchCleanup("record bunker reset failure") { eventLogStore?.log( EventLogCategory.BUNKER, EventLogLevel.ERROR, "bunker", - "The bunker could not be fully reset for this account; rotate the bunker URL before turning it on" + "The bunker could not be reset for this account and may still allow what was approved under the previous one; leave it off and switch accounts again to retry" + ) + } + } else if (!bunkerResetComplete) { + runAccountSwitchCleanup("record partial bunker reset") { + eventLogStore?.log( + EventLogCategory.BUNKER, EventLogLevel.WARN, "bunker", + "The bunker was not fully reset for this account; the reset finishes when the bunker is next turned on" ) } } From 6daa26f2756de60e0af7c395593ebcd625923265 Mon Sep 17 00:00:00 2001 From: Kyle Santiago Date: Tue, 20 Oct 2026 18:55:39 -0400 Subject: [PATCH 08/13] Save the revoke mark before a store that may replace the active account --- .../privkey/keep/nip46/BunkerRevokeE2ETest.kt | 68 ++++++++++++++++++- .../privkey/keep/nip46/RevokeAllMarkTest.kt | 33 +++++++++ .../kotlin/io/privkey/keep/AccountActions.kt | 16 ++++- .../io/privkey/keep/nip46/BunkerService.kt | 3 + .../io/privkey/keep/nip46/RevokeAllMark.kt | 30 ++++++-- 5 files changed, 140 insertions(+), 10 deletions(-) diff --git a/app/src/androidTest/kotlin/io/privkey/keep/nip46/BunkerRevokeE2ETest.kt b/app/src/androidTest/kotlin/io/privkey/keep/nip46/BunkerRevokeE2ETest.kt index 56234030..346325c9 100644 --- a/app/src/androidTest/kotlin/io/privkey/keep/nip46/BunkerRevokeE2ETest.kt +++ b/app/src/androidTest/kotlin/io/privkey/keep/nip46/BunkerRevokeE2ETest.kt @@ -24,6 +24,7 @@ import androidx.test.platform.app.InstrumentationRegistry import androidx.test.runner.lifecycle.ActivityLifecycleMonitorRegistry import androidx.test.runner.lifecycle.Stage import io.privkey.keep.AccountActions +import io.privkey.keep.ImportState import io.privkey.keep.KeepMobileApp import io.privkey.keep.toAccountInfo import io.privkey.keep.nip55.AppPermissionsScreen @@ -269,6 +270,46 @@ class BunkerRevokeE2ETest { settled("sign", c9.secret, url6, "1", "15"), c9.pubkey, restarted, reason = "Permission denied", ) assertDead(url5, url6) + + // Import over the active account: the core switches as it stores, so the mark + // that revokes the previous account's grants is saved before the store runs. + val c10 = newClient() + assertOk(settled("connect", c10.secret, url6)) + awaitAuthorized(mobile, c10) + val rememberedHere = probeAsync("sign", c10.secret, url6, "1", "90") + approve(c10.pubkey) + assertOk(awaitProbe(rememberedHere)) + val active = storage.getActiveShareKey() + val (thirdNsec, thirdAccount) = newTestAccount() + val realReset: suspend (Boolean) -> Boolean = { stop -> app.onAccountSwitched(stop) } + importThroughActions(mobile, storage, thirdNsec, realReset) { throw IllegalStateException("bunker-e2e: the mark fails") } + assertFalse( + "an import must not run when the mark cannot be saved", + storage.listAllShares().any { it.toAccountInfo().groupPubkeyHex == thirdAccount }, + ) + assertEquals(active, storage.getActiveShareKey()) + // An import that stores nothing withdraws its mark, and the grants stay. + importThroughActions(mobile, storage, "nsec1bunkere2einvalid", realReset) + assertFalse("an import that changed nothing must withdraw its mark", markers().getBoolean(BunkerService.KEY_REVOKE_ALL_ON_START, false)) + stopService() + BunkerService.start(context) + assertEquals("an import that changed nothing must not rotate the bunker", url6, awaitRunning(previous = null)) + assertOk(settled("sign", c10.secret, url6, "1", "15")) + // A switching import whose reset then fails still revokes at the next start. + importThroughActions(mobile, storage, thirdNsec, onAccountSwitched = { false }) + assertEquals("the import must activate the new account", thirdAccount, storage.getActiveShareKey()) + assertTrue("the mark must be saved before the import switches", markers().getBoolean(BunkerService.KEY_REVOKE_ALL_ON_START, false)) + FrostSignTestSupport.initializeWithDecryptContext(mobile, storage, "bunker-e2e-import-init") + stopService() + BunkerService.start(context) + val url7 = awaitRunning(previous = null) + assertRotated(url6, url7) + val imported = System.currentTimeMillis() + assertRefused( + "a grant remembered under the account an import replaced must not sign as the new one", + settled("sign", c10.secret, url7, "1", "15"), c10.pubkey, imported, reason = "Permission denied", + ) + assertFalse("the mark is cleared once the grants are revoked", markers().getBoolean(BunkerService.KEY_REVOKE_ALL_ON_START, false)) } passed = true } finally { @@ -393,6 +434,7 @@ class BunkerRevokeE2ETest { mobile: KeepMobile, storage: AndroidKeystoreStorage, onAccountSwitched: suspend (Boolean) -> Boolean, + markBunkerRevokeAhead: () -> (() -> Unit)? = { BunkerService.markRevokeAllAhead(context) }, action: (AccountActions, () -> Unit) -> Unit, ) { val scope = CoroutineScope(SupervisorJob() + Dispatchers.Default) @@ -408,6 +450,7 @@ class BunkerRevokeE2ETest { onAccountDeleted = {}, onLiveNodeDropped = {}, onStateChanged = {}, + markBunkerRevokeAhead = markBunkerRevokeAhead, ) storage.getActiveShareKey()?.let { actions.setCurrentRelays(mobile.getRelayConfig(it).frostRelays) } action(actions) { dismissed.complete(Unit) } @@ -417,9 +460,22 @@ class BunkerRevokeE2ETest { } } - // A second account the test owns, for the account switch. Random, so it is never one of - // the device's own; recorded before use so cleanup and recovery delete it. - private fun importSecondAccount(mobile: KeepMobile, storage: AndroidKeystoreStorage): String { + // Runs the app's own import, waiting for it to finish either way. + private fun importThroughActions( + mobile: KeepMobile, + storage: AndroidKeystoreStorage, + nsec: String, + onAccountSwitched: suspend (Boolean) -> Boolean, + markBunkerRevokeAhead: () -> (() -> Unit)? = { BunkerService.markRevokeAllAhead(context) }, + ) = withAccountActions(mobile, storage, onAccountSwitched, markBunkerRevokeAhead) { actions, done -> + actions.importNsec(nsec.toByteArray(), "bunker-e2e-import", storage.getCipherForEncryption()) { state -> + if (state is ImportState.Success || state is ImportState.Error) done() + } + } + + // An account the test owns, as its nsec and group key. Random, so it is never one of the + // device's own; recorded before use so cleanup and recovery delete it. + private fun newTestAccount(): Pair { val secret = ByteArray(32).also { SecureRandom().nextBytes(it) }.joinToString("") { "%02x".format(it) } val nsec = probe("nsec", secret) check(nsec.startsWith("nsec1")) { "probe returned no nsec: $nsec" } @@ -427,6 +483,12 @@ class BunkerRevokeE2ETest { val expected = probe("pubkey", secret) clientsFile.appendText("$ACCOUNT $expected\n") testAccounts += expected + return nsec to expected + } + + // A second account, for the account switch. + private fun importSecondAccount(mobile: KeepMobile, storage: AndroidKeystoreStorage): String { + val (nsec, expected) = newTestAccount() val group = withCipher(storage, "bunker-e2e-import-second", storage.getCipherForEncryption()) { mobile.importNsec(nsec.toByteArray(), "bunker-e2e-second") }.groupPubkey diff --git a/app/src/androidTest/kotlin/io/privkey/keep/nip46/RevokeAllMarkTest.kt b/app/src/androidTest/kotlin/io/privkey/keep/nip46/RevokeAllMarkTest.kt index f132582a..0ba6ab08 100644 --- a/app/src/androidTest/kotlin/io/privkey/keep/nip46/RevokeAllMarkTest.kt +++ b/app/src/androidTest/kotlin/io/privkey/keep/nip46/RevokeAllMarkTest.kt @@ -6,6 +6,7 @@ import androidx.test.ext.junit.runners.AndroidJUnit4 import androidx.test.platform.app.InstrumentationRegistry import org.junit.After import org.junit.Assert.assertFalse +import org.junit.Assert.assertNull import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test @@ -69,4 +70,36 @@ class RevokeAllMarkTest { assertFalse(mark.isSet()) assertFalse(RevokeAllMark(prefs).isSet()) } + + @Test + fun aMarkSetAheadIsSavedBeforeTheChange() { + RevokeAllMark(prefs).setAhead() + assertTrue(RevokeAllMark(prefs).isSet()) + } + + @Test + fun aMarkSetAheadOfAChangeThatDidNotHappenIsWithdrawn() { + val mark = RevokeAllMark(prefs) + checkNotNull(mark.setAhead()).invoke() + assertFalse(mark.isSet()) + assertFalse(RevokeAllMark(prefs).isSet()) + } + + @Test + fun aWithdrawLeavesAMarkThatWasAlreadyPending() { + val mark = RevokeAllMark(prefs) + mark.set() + assertNull(mark.setAhead()) + assertTrue(mark.isSet()) + } + + @Test + fun aWithdrawLeavesAMarkSetAgainAfterIt() { + val mark = RevokeAllMark(prefs) + val withdraw = checkNotNull(mark.setAhead()) + mark.set() + withdraw() + assertTrue(mark.isSet()) + assertTrue(RevokeAllMark(prefs).isSet()) + } } diff --git a/app/src/main/kotlin/io/privkey/keep/AccountActions.kt b/app/src/main/kotlin/io/privkey/keep/AccountActions.kt index 35333d29..e4bad02d 100644 --- a/app/src/main/kotlin/io/privkey/keep/AccountActions.kt +++ b/app/src/main/kotlin/io/privkey/keep/AccountActions.kt @@ -7,6 +7,7 @@ import java.util.concurrent.atomic.AtomicBoolean import java.util.concurrent.atomic.AtomicReference import android.util.Log import android.widget.Toast +import io.privkey.keep.nip46.BunkerService import io.privkey.keep.storage.AndroidKeystoreStorage import io.privkey.keep.uniffi.DkgConfig import io.privkey.keep.uniffi.DkgProgressCallback @@ -66,7 +67,8 @@ internal class AccountActions( private val onAccountSwitched: suspend (stopIfBunkerResetFails: Boolean) -> Boolean, private val onAccountDeleted: suspend () -> Unit, private val onLiveNodeDropped: () -> Unit, - private val onStateChanged: (AccountState) -> Unit + private val onStateChanged: (AccountState) -> Unit, + private val markBunkerRevokeAhead: () -> (() -> Unit)? = { BunkerService.markRevokeAllAhead(appContext) } ) { data class AccountState( val hasShare: Boolean, @@ -129,12 +131,17 @@ internal class AccountActions( * failed attempt that changed nothing, or a first account, wipes nothing. A pointer or * share list that cannot be read counts as a change. A store that succeeded also * reports the core's node drop. + * + * The bunker's revoke-all mark is saved before [store] runs, since nothing can stop the + * switch once the core has made it; when the mark cannot be saved, [store] does not run. + * It is withdrawn when nothing switched. */ private suspend fun storingNewActiveShare(store: suspend () -> T): T { val before = withContext(Dispatchers.IO) { val hadShare = runCatching { keepMobile.hasShare() }.getOrDefault(true) hadShare to runCatching { storage.getActiveShareKey() }.getOrNull() } + val withdrawMark = if (before.first) withContext(Dispatchers.IO) { markBunkerRevokeAhead() } else null var stored = false try { return withContext(Dispatchers.IO) { store().also { stored = true } } @@ -148,7 +155,12 @@ internal class AccountActions( val activeAfter = withContext(Dispatchers.IO) { runCatching { storage.getActiveShareKey() }.getOrNull() } - if (storeSwitchedAccount(hadShare, activeBefore, activeAfter)) onAccountSwitched(false) + if (storeSwitchedAccount(hadShare, activeBefore, activeAfter)) { + onAccountSwitched(false) + } else if (withdrawMark != null) { + withContext(Dispatchers.IO) { runCatching(withdrawMark) } + .onFailure { if (BuildConfig.DEBUG) Log.e("AccountActions", "Revoke mark withdraw failed: ${it::class.simpleName}") } + } } } } diff --git a/app/src/main/kotlin/io/privkey/keep/nip46/BunkerService.kt b/app/src/main/kotlin/io/privkey/keep/nip46/BunkerService.kt index 21ae714a..ebe51591 100644 --- a/app/src/main/kotlin/io/privkey/keep/nip46/BunkerService.kt +++ b/app/src/main/kotlin/io/privkey/keep/nip46/BunkerService.kt @@ -281,6 +281,9 @@ class BunkerService : Service() { private fun revokeAllMark(context: Context): RevokeAllMark = revokeAllMarkInstance ?: RevokeAllMark(markers(context.applicationContext)).also { revokeAllMarkInstance = it } + /** Saves the revoke-all mark before a store that may replace the active account. */ + internal fun markRevokeAllAhead(context: Context): (() -> Unit)? = revokeAllMark(context).setAhead() + /** * Rotate the bunker credentials whether or not the service is running. */ diff --git a/app/src/main/kotlin/io/privkey/keep/nip46/RevokeAllMark.kt b/app/src/main/kotlin/io/privkey/keep/nip46/RevokeAllMark.kt index a3b8bd90..630b1d9e 100644 --- a/app/src/main/kotlin/io/privkey/keep/nip46/RevokeAllMark.kt +++ b/app/src/main/kotlin/io/privkey/keep/nip46/RevokeAllMark.kt @@ -8,11 +8,11 @@ import android.content.SharedPreferences * every start in this process. A start clears it only if no switch set it again after the * start read it, since that switch's grants may postdate the revoke. * - * A save that fails stops a switch or delete of the active account. Ceiling: an import - * cannot be stopped, so there a failed save holds only until the process ends, and a power - * cut can roll back a save that reported success. The next start then restores the previous - * account's remembered grants, and the switch's rotation does not keep a client holding one - * out, since the bunker key is public. Goes with the start-time revoke. + * A save that fails stops the change: a switch or delete of the active account, or a store + * that may replace it, which saves the mark before it runs. Ceiling: a power cut can roll + * back a save that reported success. The next start then restores the previous account's + * remembered grants, and the switch's rotation does not keep a client holding one out, since + * the bunker key is public. Goes with the start-time revoke. */ internal class RevokeAllMark(private val prefs: SharedPreferences) { private var pending = false @@ -27,6 +27,26 @@ internal class RevokeAllMark(private val prefs: SharedPreferences) { } } + /** + * Sets the mark ahead of a change that may switch accounts. Returns a withdraw for when it + * did not, or null when a mark was already pending, since only a start may clear that one. + */ + @Synchronized + fun setAhead(): (() -> Unit)? { + val wasSet = isSet() + set() + if (wasSet) return null + val token = requests + return { withdraw(token) } + } + + @Synchronized + private fun withdraw(token: Int) { + if (requests != token) return + check(prefs.edit().remove(BunkerService.KEY_REVOKE_ALL_ON_START).commit()) { "could not withdraw the revoke-all mark" } + pending = false + } + @Synchronized fun isSet(): Boolean = pending || prefs.getBoolean(BunkerService.KEY_REVOKE_ALL_ON_START, false) From fd617e33925330e2e80c5f533f3dc332e9851856 Mon Sep 17 00:00:00 2001 From: Kyle Santiago Date: Tue, 20 Oct 2026 19:05:20 -0400 Subject: [PATCH 09/13] Keep the saved revoke mark while a change that may switch is open --- .../privkey/keep/nip46/BunkerRevokeE2ETest.kt | 10 ++- .../privkey/keep/nip46/RevokeAllMarkTest.kt | 68 ++++++++++++++++--- .../kotlin/io/privkey/keep/AccountActions.kt | 15 ++-- .../io/privkey/keep/nip46/BunkerService.kt | 29 ++++---- .../io/privkey/keep/nip46/RevokeAllMark.kt | 53 ++++++++++----- 5 files changed, 125 insertions(+), 50 deletions(-) diff --git a/app/src/androidTest/kotlin/io/privkey/keep/nip46/BunkerRevokeE2ETest.kt b/app/src/androidTest/kotlin/io/privkey/keep/nip46/BunkerRevokeE2ETest.kt index 346325c9..95f355bd 100644 --- a/app/src/androidTest/kotlin/io/privkey/keep/nip46/BunkerRevokeE2ETest.kt +++ b/app/src/androidTest/kotlin/io/privkey/keep/nip46/BunkerRevokeE2ETest.kt @@ -289,7 +289,11 @@ class BunkerRevokeE2ETest { ) assertEquals(active, storage.getActiveShareKey()) // An import that stores nothing withdraws its mark, and the grants stay. - importThroughActions(mobile, storage, "nsec1bunkere2einvalid", realReset) + var savedAhead = false + importThroughActions(mobile, storage, "nsec1bunkere2einvalid", realReset) { + BunkerService.markRevokeAllAhead(context).also { savedAhead = markers().getBoolean(BunkerService.KEY_REVOKE_ALL_ON_START, false) } + } + assertTrue("the import must save the mark before it runs", savedAhead) assertFalse("an import that changed nothing must withdraw its mark", markers().getBoolean(BunkerService.KEY_REVOKE_ALL_ON_START, false)) stopService() BunkerService.start(context) @@ -434,7 +438,7 @@ class BunkerRevokeE2ETest { mobile: KeepMobile, storage: AndroidKeystoreStorage, onAccountSwitched: suspend (Boolean) -> Boolean, - markBunkerRevokeAhead: () -> (() -> Unit)? = { BunkerService.markRevokeAllAhead(context) }, + markBunkerRevokeAhead: () -> (Boolean) -> Unit = { BunkerService.markRevokeAllAhead(context) }, action: (AccountActions, () -> Unit) -> Unit, ) { val scope = CoroutineScope(SupervisorJob() + Dispatchers.Default) @@ -466,7 +470,7 @@ class BunkerRevokeE2ETest { storage: AndroidKeystoreStorage, nsec: String, onAccountSwitched: suspend (Boolean) -> Boolean, - markBunkerRevokeAhead: () -> (() -> Unit)? = { BunkerService.markRevokeAllAhead(context) }, + markBunkerRevokeAhead: () -> (Boolean) -> Unit = { BunkerService.markRevokeAllAhead(context) }, ) = withAccountActions(mobile, storage, onAccountSwitched, markBunkerRevokeAhead) { actions, done -> actions.importNsec(nsec.toByteArray(), "bunker-e2e-import", storage.getCipherForEncryption()) { state -> if (state is ImportState.Success || state is ImportState.Error) done() diff --git a/app/src/androidTest/kotlin/io/privkey/keep/nip46/RevokeAllMarkTest.kt b/app/src/androidTest/kotlin/io/privkey/keep/nip46/RevokeAllMarkTest.kt index 0ba6ab08..cb186268 100644 --- a/app/src/androidTest/kotlin/io/privkey/keep/nip46/RevokeAllMarkTest.kt +++ b/app/src/androidTest/kotlin/io/privkey/keep/nip46/RevokeAllMarkTest.kt @@ -6,7 +6,6 @@ import androidx.test.ext.junit.runners.AndroidJUnit4 import androidx.test.platform.app.InstrumentationRegistry import org.junit.After import org.junit.Assert.assertFalse -import org.junit.Assert.assertNull import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test @@ -78,27 +77,80 @@ class RevokeAllMarkTest { } @Test - fun aMarkSetAheadOfAChangeThatDidNotHappenIsWithdrawn() { + fun aStartDuringAnOpenChangeStillRevokes() { val mark = RevokeAllMark(prefs) - checkNotNull(mark.setAhead()).invoke() + mark.setAhead() + assertTrue(mark.isSet()) + } + + @Test + fun aChangeThatSwitchedKeepsTheMark() { + val mark = RevokeAllMark(prefs) + val seen = mark.requestsSoFar() + mark.setAhead()(true) + assertTrue(mark.isSet()) + assertTrue(RevokeAllMark(prefs).isSet()) + assertFalse("a start that looked before the switch must not clear it", mark.clearIfUnchanged(seen) { true }) + } + + @Test + fun aChangeThatDidNotSwitchWithdrawsTheMark() { + val mark = RevokeAllMark(prefs) + mark.setAhead()(false) assertFalse(mark.isSet()) assertFalse(RevokeAllMark(prefs).isSet()) } @Test - fun aWithdrawLeavesAMarkThatWasAlreadyPending() { + fun aStartKeepsTheSavedMarkWhileAChangeIsOpen() { val mark = RevokeAllMark(prefs) mark.set() - assertNull(mark.setAhead()) + val finish = mark.setAhead() + assertTrue(mark.clearIfUnchanged(mark.requestsSoFar()) { true }) + assertTrue("the open change still needs the saved mark", RevokeAllMark(prefs).isSet()) + finish(true) assertTrue(mark.isSet()) + assertTrue(RevokeAllMark(prefs).isSet()) } @Test - fun aWithdrawLeavesAMarkSetAgainAfterIt() { + fun aChangeThatDidNotSwitchWithdrawsAMarkAStartAlreadyRevokedFor() { val mark = RevokeAllMark(prefs) - val withdraw = checkNotNull(mark.setAhead()) mark.set() - withdraw() + val finish = mark.setAhead() + assertTrue(mark.clearIfUnchanged(mark.requestsSoFar()) { true }) + finish(false) + assertFalse(mark.isSet()) + assertFalse(RevokeAllMark(prefs).isSet()) + } + + @Test + fun anOpenChangeKeepsTheMarkAnotherOneFinishedWithout() { + val mark = RevokeAllMark(prefs) + val first = mark.setAhead() + mark.setAhead()(false) + assertTrue(RevokeAllMark(prefs).isSet()) + first(false) + assertFalse(mark.isSet()) + assertFalse(RevokeAllMark(prefs).isSet()) + } + + @Test + fun aChangeThatDidNotSwitchLeavesAMarkThatWasAlreadyPending() { + val mark = RevokeAllMark(prefs) + mark.set() + mark.setAhead()(false) + assertTrue(mark.isSet()) + assertTrue(RevokeAllMark(prefs).isSet()) + } + + @Test + fun aChangeThatDidNotSwitchLeavesAMarkSavedByAnEarlierProcess() { + RevokeAllMark(prefs).set() + val mark = RevokeAllMark(prefs) + val finish = mark.setAhead() + assertTrue(mark.isSet()) + finish(false) assertTrue(mark.isSet()) assertTrue(RevokeAllMark(prefs).isSet()) } diff --git a/app/src/main/kotlin/io/privkey/keep/AccountActions.kt b/app/src/main/kotlin/io/privkey/keep/AccountActions.kt index e4bad02d..3a918640 100644 --- a/app/src/main/kotlin/io/privkey/keep/AccountActions.kt +++ b/app/src/main/kotlin/io/privkey/keep/AccountActions.kt @@ -68,7 +68,7 @@ internal class AccountActions( private val onAccountDeleted: suspend () -> Unit, private val onLiveNodeDropped: () -> Unit, private val onStateChanged: (AccountState) -> Unit, - private val markBunkerRevokeAhead: () -> (() -> Unit)? = { BunkerService.markRevokeAllAhead(appContext) } + private val markBunkerRevokeAhead: () -> (switched: Boolean) -> Unit = { BunkerService.markRevokeAllAhead(appContext) } ) { data class AccountState( val hasShare: Boolean, @@ -134,14 +134,15 @@ internal class AccountActions( * * The bunker's revoke-all mark is saved before [store] runs, since nothing can stop the * switch once the core has made it; when the mark cannot be saved, [store] does not run. - * It is withdrawn when nothing switched. + * It becomes the switch's mark when the store switched and is withdrawn otherwise. */ private suspend fun storingNewActiveShare(store: suspend () -> T): T { val before = withContext(Dispatchers.IO) { val hadShare = runCatching { keepMobile.hasShare() }.getOrDefault(true) hadShare to runCatching { storage.getActiveShareKey() }.getOrNull() } - val withdrawMark = if (before.first) withContext(Dispatchers.IO) { markBunkerRevokeAhead() } else null + // Not cancellable: a cancel landing after the save would lose what finishes it. + val finishMark = if (before.first) withContext(NonCancellable + Dispatchers.IO) { markBunkerRevokeAhead() } else null var stored = false try { return withContext(Dispatchers.IO) { store().also { stored = true } } @@ -155,12 +156,12 @@ internal class AccountActions( val activeAfter = withContext(Dispatchers.IO) { runCatching { storage.getActiveShareKey() }.getOrNull() } - if (storeSwitchedAccount(hadShare, activeBefore, activeAfter)) { - onAccountSwitched(false) - } else if (withdrawMark != null) { - withContext(Dispatchers.IO) { runCatching(withdrawMark) } + val switched = storeSwitchedAccount(hadShare, activeBefore, activeAfter) + if (finishMark != null) { + withContext(Dispatchers.IO) { runCatching { finishMark(switched) } } .onFailure { if (BuildConfig.DEBUG) Log.e("AccountActions", "Revoke mark withdraw failed: ${it::class.simpleName}") } } + if (switched) onAccountSwitched(false) } } } diff --git a/app/src/main/kotlin/io/privkey/keep/nip46/BunkerService.kt b/app/src/main/kotlin/io/privkey/keep/nip46/BunkerService.kt index ebe51591..bcd4914d 100644 --- a/app/src/main/kotlin/io/privkey/keep/nip46/BunkerService.kt +++ b/app/src/main/kotlin/io/privkey/keep/nip46/BunkerService.kt @@ -282,7 +282,7 @@ class BunkerService : Service() { revokeAllMarkInstance ?: RevokeAllMark(markers(context.applicationContext)).also { revokeAllMarkInstance = it } /** Saves the revoke-all mark before a store that may replace the active account. */ - internal fun markRevokeAllAhead(context: Context): (() -> Unit)? = revokeAllMark(context).setAhead() + internal fun markRevokeAllAhead(context: Context): (switched: Boolean) -> Unit = revokeAllMark(context).setAhead() /** * Rotate the bunker credentials whether or not the service is running. @@ -652,27 +652,28 @@ class BunkerService : Service() { return false } - // A revoke made while the engine was stopped could not drop the client's - // persisted grants, and this start has just restored them. Ceiling: one - // storage write per denylisted client on every start, and grants live until - // the sweep reaches them. Remove once the pinned core clears persisted grants - // on a revoke without a running bunker and drops them on rotation. try { - Nip46ClientStore.getDenylist(this) - .filter { HEX_PUBKEY_REGEX.matches(it) } - .forEach { handler.revokeClient(it) } // An account switch dropped the previous account's clients; their remembered - // grants were restored by this start and go here. The revoke is a silent no-op - // once onDestroy has stopped the handler, and onDestroy clears serving first, so - // the mark is kept for the next start unless this one is still serving. Ceiling: - // the pinned core also reports success when it fails to save the emptied grants; - // a core whose revoke returns a result closes that. + // grants were restored by this start and go here, first, since they are live + // until then. The revoke is a silent no-op once onDestroy has stopped the + // handler, and onDestroy clears serving first, so the mark is kept for the next + // start unless this one is still serving. Ceiling: the pinned core also reports + // success when it fails to save the emptied grants; a core whose revoke returns + // a result closes that. val mark = revokeAllMark(this) val requestsSeen = mark.requestsSoFar() if (mark.isSet()) { handler.revokeAllClients() mark.clearIfUnchanged(requestsSeen) { serving } } + // A revoke made while the engine was stopped could not drop the client's + // persisted grants, and this start has just restored them. Ceiling: one + // storage write per denylisted client on every start, and grants live until + // the sweep reaches them. Remove once the pinned core clears persisted grants + // on a revoke without a running bunker and drops them on rotation. + Nip46ClientStore.getDenylist(this) + .filter { HEX_PUBKEY_REGEX.matches(it) } + .forEach { handler.revokeClient(it) } } catch (e: Exception) { runCatching { handler.stopBunker() } throw e diff --git a/app/src/main/kotlin/io/privkey/keep/nip46/RevokeAllMark.kt b/app/src/main/kotlin/io/privkey/keep/nip46/RevokeAllMark.kt index 630b1d9e..4e0d2143 100644 --- a/app/src/main/kotlin/io/privkey/keep/nip46/RevokeAllMark.kt +++ b/app/src/main/kotlin/io/privkey/keep/nip46/RevokeAllMark.kt @@ -8,15 +8,22 @@ import android.content.SharedPreferences * every start in this process. A start clears it only if no switch set it again after the * start read it, since that switch's grants may postdate the revoke. * + * A change that may switch accounts saves the mark before it runs ([setAhead]), and then + * makes it the switch's mark or withdraws it. While it is open a start still revokes, since + * the core may already have switched, but leaves the saved mark in place. So a bunker that + * restarts during a change that then fails has its grants revoked and its URL rotated for + * nothing. A process that dies with a change open finds the mark saved. + * * A save that fails stops the change: a switch or delete of the active account, or a store - * that may replace it, which saves the mark before it runs. Ceiling: a power cut can roll - * back a save that reported success. The next start then restores the previous account's - * remembered grants, and the switch's rotation does not keep a client holding one out, since - * the bunker key is public. Goes with the start-time revoke. + * that may replace it. Ceiling: a power cut can roll back a save that reported success. The + * next start then restores the previous account's remembered grants, and the switch's + * rotation does not keep a client holding one out, since the bunker key is public. Goes with + * the start-time revoke. */ internal class RevokeAllMark(private val prefs: SharedPreferences) { private var pending = false private var requests = 0 + private var open = 0 @Synchronized fun set() { @@ -28,23 +35,30 @@ internal class RevokeAllMark(private val prefs: SharedPreferences) { } /** - * Sets the mark ahead of a change that may switch accounts. Returns a withdraw for when it - * did not, or null when a mark was already pending, since only a start may clear that one. + * Saves the mark ahead of a change that may switch accounts. Returns what the change calls + * once done, with whether it switched. Throws, leaving nothing open, when it is not saved. */ @Synchronized - fun setAhead(): (() -> Unit)? { - val wasSet = isSet() - set() - if (wasSet) return null - val token = requests - return { withdraw(token) } + fun setAhead(): (switched: Boolean) -> Unit { + // A mark only open changes saved is not a switch's yet. + if (pending || (open == 0 && prefs.getBoolean(BunkerService.KEY_REVOKE_ALL_ON_START, false))) pending = true + if (!prefs.edit().putBoolean(BunkerService.KEY_REVOKE_ALL_ON_START, true).commit()) { + if (!pending && open == 0) prefs.edit().remove(BunkerService.KEY_REVOKE_ALL_ON_START).commit() + throw IllegalStateException("could not mark the grants for revocation") + } + open++ + return { switched -> finishAhead(switched) } } @Synchronized - private fun withdraw(token: Int) { - if (requests != token) return - check(prefs.edit().remove(BunkerService.KEY_REVOKE_ALL_ON_START).commit()) { "could not withdraw the revoke-all mark" } - pending = false + private fun finishAhead(switched: Boolean) { + open-- + if (switched) { + requests++ + pending = true + } else if (open == 0 && !pending) { + check(prefs.edit().remove(BunkerService.KEY_REVOKE_ALL_ON_START).commit()) { "could not withdraw the revoke-all mark" } + } } @Synchronized @@ -53,11 +67,14 @@ internal class RevokeAllMark(private val prefs: SharedPreferences) { @Synchronized fun requestsSoFar(): Int = requests - /** Clears the mark unless it was set again after [seen] was read, or [stillServing] says no. */ + /** + * Clears the mark unless it was set again after [seen] was read, or [stillServing] says no. + * The saved mark stays while a change is open. + */ @Synchronized fun clearIfUnchanged(seen: Int, stillServing: () -> Boolean): Boolean { if (requests != seen || !stillServing()) return false - check(prefs.edit().remove(BunkerService.KEY_REVOKE_ALL_ON_START).commit()) { "could not clear the revoke-all mark" } + if (open == 0) check(prefs.edit().remove(BunkerService.KEY_REVOKE_ALL_ON_START).commit()) { "could not clear the revoke-all mark" } pending = false return true } From 1a9eac8b592a3e6fefc73e5af2bbb5c395aabea5 Mon Sep 17 00:00:00 2001 From: Kyle Santiago Date: Tue, 20 Oct 2026 19:10:59 -0400 Subject: [PATCH 10/13] Keep the ahead mark's finish through a cancel on the caller's dispatcher --- .../privkey/keep/nip46/RevokeAllMarkTest.kt | 30 ++++++++++++++++++- .../kotlin/io/privkey/keep/AccountActions.kt | 5 ++-- 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/app/src/androidTest/kotlin/io/privkey/keep/nip46/RevokeAllMarkTest.kt b/app/src/androidTest/kotlin/io/privkey/keep/nip46/RevokeAllMarkTest.kt index cb186268..4e47d5d3 100644 --- a/app/src/androidTest/kotlin/io/privkey/keep/nip46/RevokeAllMarkTest.kt +++ b/app/src/androidTest/kotlin/io/privkey/keep/nip46/RevokeAllMarkTest.kt @@ -6,6 +6,7 @@ import androidx.test.ext.junit.runners.AndroidJUnit4 import androidx.test.platform.app.InstrumentationRegistry import org.junit.After import org.junit.Assert.assertFalse +import org.junit.Assert.assertThrows import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test @@ -77,10 +78,25 @@ class RevokeAllMarkTest { } @Test - fun aStartDuringAnOpenChangeStillRevokes() { + fun aStartDuringAnOpenChangeRevokesAndKeepsTheMark() { val mark = RevokeAllMark(prefs) mark.setAhead() + val seen = mark.requestsSoFar() assertTrue(mark.isSet()) + assertTrue(mark.clearIfUnchanged(seen) { true }) + assertTrue("a later start revokes again while the change is open", mark.isSet()) + assertTrue(RevokeAllMark(prefs).isSet()) + } + + @Test + fun aMarkThatCouldNotBeSavedAheadLeavesNothingBehind() { + var failing = true + val mark = RevokeAllMark(failingCommits(prefs) { failing }) + assertThrows(IllegalStateException::class.java) { mark.setAhead() } + assertFalse(mark.isSet()) + failing = false + mark.setAhead()(false) + assertFalse("a failed save must not leave a change open", mark.isSet()) } @Test @@ -154,4 +170,16 @@ class RevokeAllMarkTest { assertTrue(mark.isSet()) assertTrue(RevokeAllMark(prefs).isSet()) } + + // A commit that fails as a disk write does: applied in memory, reported as not saved. + private fun failingCommits(prefs: SharedPreferences, failing: () -> Boolean) = object : SharedPreferences by prefs { + override fun edit(): SharedPreferences.Editor { + val editor = prefs.edit() + return object : SharedPreferences.Editor by editor { + override fun putBoolean(key: String?, value: Boolean): SharedPreferences.Editor = apply { editor.putBoolean(key, value) } + override fun remove(key: String?): SharedPreferences.Editor = apply { editor.remove(key) } + override fun commit(): Boolean = if (failing()) false.also { editor.apply() } else editor.commit() + } + } + } } diff --git a/app/src/main/kotlin/io/privkey/keep/AccountActions.kt b/app/src/main/kotlin/io/privkey/keep/AccountActions.kt index 3a918640..ad182bc7 100644 --- a/app/src/main/kotlin/io/privkey/keep/AccountActions.kt +++ b/app/src/main/kotlin/io/privkey/keep/AccountActions.kt @@ -141,8 +141,9 @@ internal class AccountActions( val hadShare = runCatching { keepMobile.hasShare() }.getOrDefault(true) hadShare to runCatching { storage.getActiveShareKey() }.getOrNull() } - // Not cancellable: a cancel landing after the save would lose what finishes it. - val finishMark = if (before.first) withContext(NonCancellable + Dispatchers.IO) { markBunkerRevokeAhead() } else null + // Not cancellable: a cancel landing after the save would lose what finishes it. Nested, + // since a withContext that also changes dispatcher returns cancellably. + val finishMark = if (before.first) withContext(NonCancellable) { withContext(Dispatchers.IO) { markBunkerRevokeAhead() } } else null var stored = false try { return withContext(Dispatchers.IO) { store().also { stored = true } } From 77eb671749725c7519cdd0009062a82eb8ae434d Mon Sep 17 00:00:00 2001 From: Kyle Santiago Date: Tue, 20 Oct 2026 19:15:22 -0400 Subject: [PATCH 11/13] Let an ahead mark finish only once --- .../io/privkey/keep/nip46/RevokeAllMarkTest.kt | 12 ++++++++++++ .../main/kotlin/io/privkey/keep/AccountActions.kt | 7 ++++--- .../kotlin/io/privkey/keep/nip46/RevokeAllMark.kt | 7 +++++-- 3 files changed, 21 insertions(+), 5 deletions(-) diff --git a/app/src/androidTest/kotlin/io/privkey/keep/nip46/RevokeAllMarkTest.kt b/app/src/androidTest/kotlin/io/privkey/keep/nip46/RevokeAllMarkTest.kt index 4e47d5d3..69549045 100644 --- a/app/src/androidTest/kotlin/io/privkey/keep/nip46/RevokeAllMarkTest.kt +++ b/app/src/androidTest/kotlin/io/privkey/keep/nip46/RevokeAllMarkTest.kt @@ -171,6 +171,18 @@ class RevokeAllMarkTest { assertTrue(RevokeAllMark(prefs).isSet()) } + @Test + fun aChangeFinishesOnlyOnce() { + val mark = RevokeAllMark(prefs) + val first = mark.setAhead() + val second = mark.setAhead() + first(false) + first(false) + assertTrue("a repeated finish must not close another open change", RevokeAllMark(prefs).isSet()) + second(false) + assertFalse(RevokeAllMark(prefs).isSet()) + } + // A commit that fails as a disk write does: applied in memory, reported as not saved. private fun failingCommits(prefs: SharedPreferences, failing: () -> Boolean) = object : SharedPreferences by prefs { override fun edit(): SharedPreferences.Editor { diff --git a/app/src/main/kotlin/io/privkey/keep/AccountActions.kt b/app/src/main/kotlin/io/privkey/keep/AccountActions.kt index ad182bc7..c66f7b18 100644 --- a/app/src/main/kotlin/io/privkey/keep/AccountActions.kt +++ b/app/src/main/kotlin/io/privkey/keep/AccountActions.kt @@ -132,9 +132,10 @@ internal class AccountActions( * share list that cannot be read counts as a change. A store that succeeded also * reports the core's node drop. * - * The bunker's revoke-all mark is saved before [store] runs, since nothing can stop the - * switch once the core has made it; when the mark cannot be saved, [store] does not run. - * It becomes the switch's mark when the store switched and is withdrawn otherwise. + * When a share already exists, or that cannot be read, the bunker's revoke-all mark is + * saved before [store] runs, since nothing can stop the switch once the core has made it; + * when the mark cannot be saved, [store] does not run. It becomes the switch's mark when + * the store switched; otherwise it is withdrawn unless another change or switch needs it. */ private suspend fun storingNewActiveShare(store: suspend () -> T): T { val before = withContext(Dispatchers.IO) { diff --git a/app/src/main/kotlin/io/privkey/keep/nip46/RevokeAllMark.kt b/app/src/main/kotlin/io/privkey/keep/nip46/RevokeAllMark.kt index 4e0d2143..2ff513fb 100644 --- a/app/src/main/kotlin/io/privkey/keep/nip46/RevokeAllMark.kt +++ b/app/src/main/kotlin/io/privkey/keep/nip46/RevokeAllMark.kt @@ -1,6 +1,7 @@ package io.privkey.keep.nip46 import android.content.SharedPreferences +import java.util.concurrent.atomic.AtomicBoolean /** * The account-switch mark that makes the next bunker start revoke every remembered grant. @@ -36,7 +37,8 @@ internal class RevokeAllMark(private val prefs: SharedPreferences) { /** * Saves the mark ahead of a change that may switch accounts. Returns what the change calls - * once done, with whether it switched. Throws, leaving nothing open, when it is not saved. + * once done, with whether it switched; calls after the first do nothing. Throws, leaving + * nothing open, when it is not saved. */ @Synchronized fun setAhead(): (switched: Boolean) -> Unit { @@ -47,7 +49,8 @@ internal class RevokeAllMark(private val prefs: SharedPreferences) { throw IllegalStateException("could not mark the grants for revocation") } open++ - return { switched -> finishAhead(switched) } + val finished = AtomicBoolean(false) + return { switched -> if (finished.compareAndSet(false, true)) finishAhead(switched) } } @Synchronized From 575d4674e93b54208c1acc344ce943d6c1ed0e70 Mon Sep 17 00:00:00 2001 From: Kyle Santiago Date: Tue, 20 Oct 2026 20:13:50 -0400 Subject: [PATCH 12/13] Keep the revoke mark when a storage call fails during the revoke --- .../keep/storage/StorageFaultCountTest.kt | 31 +++++++++++++++++++ .../io/privkey/keep/nip46/BunkerService.kt | 10 +++--- .../io/privkey/keep/nip46/RevokeAllMark.kt | 8 ++--- .../keep/storage/AndroidKeystoreStorage.kt | 12 +++++++ 4 files changed, 53 insertions(+), 8 deletions(-) create mode 100644 app/src/androidTest/kotlin/io/privkey/keep/storage/StorageFaultCountTest.kt diff --git a/app/src/androidTest/kotlin/io/privkey/keep/storage/StorageFaultCountTest.kt b/app/src/androidTest/kotlin/io/privkey/keep/storage/StorageFaultCountTest.kt new file mode 100644 index 00000000..9880e5a3 --- /dev/null +++ b/app/src/androidTest/kotlin/io/privkey/keep/storage/StorageFaultCountTest.kt @@ -0,0 +1,31 @@ +package io.privkey.keep.storage + +import androidx.test.ext.junit.runners.AndroidJUnit4 +import androidx.test.platform.app.InstrumentationRegistry +import io.privkey.keep.uniffi.KeepMobileException +import org.junit.Assert.assertEquals +import org.junit.Assert.assertThrows +import org.junit.Test +import org.junit.runner.RunWith +import java.util.UUID + +@RunWith(AndroidJUnit4::class) +class StorageFaultCountTest { + private val storage = AndroidKeystoreStorage(InstrumentationRegistry.getInstrumentation().targetContext) + + @Test + fun aFailedCallIsCounted() { + val before = storage.faultsSoFar() + assertThrows(KeepMobileException.StorageException::class.java) { storage.loadShareByKey("") } + assertEquals(before + 1, storage.faultsSoFar()) + } + + @Test + fun anAbsentRecordIsNotAFault() { + val before = storage.faultsSoFar() + assertThrows(KeepMobileException.StorageNotFound::class.java) { + storage.loadShareByKey("__keep_fault_count_test_${UUID.randomUUID()}") + } + assertEquals(before, storage.faultsSoFar()) + } +} diff --git a/app/src/main/kotlin/io/privkey/keep/nip46/BunkerService.kt b/app/src/main/kotlin/io/privkey/keep/nip46/BunkerService.kt index bcd4914d..b4ee19fe 100644 --- a/app/src/main/kotlin/io/privkey/keep/nip46/BunkerService.kt +++ b/app/src/main/kotlin/io/privkey/keep/nip46/BunkerService.kt @@ -657,14 +657,16 @@ class BunkerService : Service() { // grants were restored by this start and go here, first, since they are live // until then. The revoke is a silent no-op once onDestroy has stopped the // handler, and onDestroy clears serving first, so the mark is kept for the next - // start unless this one is still serving. Ceiling: the pinned core also reports - // success when it fails to save the emptied grants; a core whose revoke returns - // a result closes that. + // start unless this one is still serving. The pinned core also reports success + // when it could not save the emptied grants, so the mark stays unless no storage + // call failed meanwhile; remove that once the core's revoke returns a result. val mark = revokeAllMark(this) val requestsSeen = mark.requestsSoFar() if (mark.isSet()) { + val storage = (applicationContext as? KeepMobileApp)?.getStorage() + val faultsBefore = storage?.faultsSoFar() handler.revokeAllClients() - mark.clearIfUnchanged(requestsSeen) { serving } + mark.clearIfUnchanged(requestsSeen) { serving && storage != null && storage.faultsSoFar() == faultsBefore } } // A revoke made while the engine was stopped could not drop the client's // persisted grants, and this start has just restored them. Ceiling: one diff --git a/app/src/main/kotlin/io/privkey/keep/nip46/RevokeAllMark.kt b/app/src/main/kotlin/io/privkey/keep/nip46/RevokeAllMark.kt index 2ff513fb..b0e6c677 100644 --- a/app/src/main/kotlin/io/privkey/keep/nip46/RevokeAllMark.kt +++ b/app/src/main/kotlin/io/privkey/keep/nip46/RevokeAllMark.kt @@ -71,12 +71,12 @@ internal class RevokeAllMark(private val prefs: SharedPreferences) { fun requestsSoFar(): Int = requests /** - * Clears the mark unless it was set again after [seen] was read, or [stillServing] says no. - * The saved mark stays while a change is open. + * Clears the mark unless it was set again after [seen] was read, or [revokeHeld] says the + * revoke may not have taken. The saved mark stays while a change is open. */ @Synchronized - fun clearIfUnchanged(seen: Int, stillServing: () -> Boolean): Boolean { - if (requests != seen || !stillServing()) return false + fun clearIfUnchanged(seen: Int, revokeHeld: () -> Boolean): Boolean { + if (requests != seen || !revokeHeld()) return false if (open == 0) check(prefs.edit().remove(BunkerService.KEY_REVOKE_ALL_ON_START).commit()) { "could not clear the revoke-all mark" } pending = false return true diff --git a/app/src/main/kotlin/io/privkey/keep/storage/AndroidKeystoreStorage.kt b/app/src/main/kotlin/io/privkey/keep/storage/AndroidKeystoreStorage.kt index 07bb964f..3dafa338 100644 --- a/app/src/main/kotlin/io/privkey/keep/storage/AndroidKeystoreStorage.kt +++ b/app/src/main/kotlin/io/privkey/keep/storage/AndroidKeystoreStorage.kt @@ -26,6 +26,7 @@ import java.security.ProviderException import java.security.spec.MGF1ParameterSpec import java.security.spec.X509EncodedKeySpec import java.util.concurrent.ConcurrentHashMap +import java.util.concurrent.atomic.AtomicLong import javax.crypto.Cipher import javax.crypto.KeyGenerator import javax.crypto.SecretKey @@ -242,12 +243,23 @@ class AndroidKeystoreStorage( try { block() } catch (e: KeepMobileException) { + if (e !is KeepMobileException.StorageNotFound) faults.incrementAndGet() throw e } catch (e: Exception) { + faults.incrementAndGet() Log.e(TAG, "$what could not be completed: ${e::class.simpleName}") throw KeepMobileException.StorageException("$what failed: ${e::class.simpleName}") } + private val faults = AtomicLong() + + /** + * How many fallible storage calls have failed so far, an absence aside. Two reads that + * match show that none failed in between, which is how a caller tells that a core write + * whose own result it cannot see did not fail. + */ + fun faultsSoFar(): Long = faults.get() + private fun isDkgSecretKey(key: String): Boolean = key == DKG_SECRET_STORAGE_KEY private fun sanitizeKey(key: String): String { From c422f22decb9360e892e842ccbb3dd77b36938e5 Mon Sep 17 00:00:00 2001 From: Kyle Santiago Date: Tue, 20 Oct 2026 20:20:30 -0400 Subject: [PATCH 13/13] Move the start-time revoke into the mark so its fault check is tested --- .../privkey/keep/nip46/RevokeAllMarkTest.kt | 44 +++++++++++++++++++ .../keep/storage/StorageFaultCountTest.kt | 14 ++++-- .../io/privkey/keep/nip46/BunkerService.kt | 20 ++++----- .../io/privkey/keep/nip46/RevokeAllMark.kt | 19 ++++++-- .../keep/storage/AndroidKeystoreStorage.kt | 4 +- 5 files changed, 82 insertions(+), 19 deletions(-) diff --git a/app/src/androidTest/kotlin/io/privkey/keep/nip46/RevokeAllMarkTest.kt b/app/src/androidTest/kotlin/io/privkey/keep/nip46/RevokeAllMarkTest.kt index 69549045..54b446fa 100644 --- a/app/src/androidTest/kotlin/io/privkey/keep/nip46/RevokeAllMarkTest.kt +++ b/app/src/androidTest/kotlin/io/privkey/keep/nip46/RevokeAllMarkTest.kt @@ -183,6 +183,50 @@ class RevokeAllMarkTest { assertFalse(RevokeAllMark(prefs).isSet()) } + @Test + fun aStartThatRevokedCleanlyClearsTheMark() { + val mark = RevokeAllMark(prefs) + mark.set() + var revoked = false + mark.revokeIfSet(revoke = { revoked = true }, serving = { true }, faultsSoFar = { 0L }) + assertTrue(revoked) + assertFalse(mark.isSet()) + assertFalse(RevokeAllMark(prefs).isSet()) + } + + @Test + fun aStorageFaultDuringTheRevokeKeepsTheMark() { + val mark = RevokeAllMark(prefs) + mark.set() + var faults = 0L + mark.revokeIfSet(revoke = { faults++ }, serving = { true }, faultsSoFar = { faults }) + assertTrue(mark.isSet()) + assertTrue(RevokeAllMark(prefs).isSet()) + } + + @Test + fun aStartThatCannotCountFaultsKeepsTheMark() { + val mark = RevokeAllMark(prefs) + mark.set() + mark.revokeIfSet(revoke = {}, serving = { true }, faultsSoFar = { null }) + assertTrue(mark.isSet()) + } + + @Test + fun aStartThatStoppedServingKeepsTheMarkAfterItsRevoke() { + val mark = RevokeAllMark(prefs) + mark.set() + mark.revokeIfSet(revoke = {}, serving = { false }, faultsSoFar = { 0L }) + assertTrue(mark.isSet()) + } + + @Test + fun aStartWithNoMarkRevokesNothing() { + var revoked = false + RevokeAllMark(prefs).revokeIfSet(revoke = { revoked = true }, serving = { true }, faultsSoFar = { 0L }) + assertFalse(revoked) + } + // A commit that fails as a disk write does: applied in memory, reported as not saved. private fun failingCommits(prefs: SharedPreferences, failing: () -> Boolean) = object : SharedPreferences by prefs { override fun edit(): SharedPreferences.Editor { diff --git a/app/src/androidTest/kotlin/io/privkey/keep/storage/StorageFaultCountTest.kt b/app/src/androidTest/kotlin/io/privkey/keep/storage/StorageFaultCountTest.kt index 9880e5a3..ed78d1f8 100644 --- a/app/src/androidTest/kotlin/io/privkey/keep/storage/StorageFaultCountTest.kt +++ b/app/src/androidTest/kotlin/io/privkey/keep/storage/StorageFaultCountTest.kt @@ -7,7 +7,6 @@ import org.junit.Assert.assertEquals import org.junit.Assert.assertThrows import org.junit.Test import org.junit.runner.RunWith -import java.util.UUID @RunWith(AndroidJUnit4::class) class StorageFaultCountTest { @@ -20,12 +19,19 @@ class StorageFaultCountTest { assertEquals(before + 1, storage.faultsSoFar()) } + @Test + fun aStorageErrorIsCounted() { + val before = storage.faultsSoFar() + // A share key outside a request has no cipher to use. + assertThrows(KeepMobileException.StorageException::class.java) { storage.loadShareByKey("fault_count_test") } + assertEquals(before + 1, storage.faultsSoFar()) + } + @Test fun anAbsentRecordIsNotAFault() { val before = storage.faultsSoFar() - assertThrows(KeepMobileException.StorageNotFound::class.java) { - storage.loadShareByKey("__keep_fault_count_test_${UUID.randomUUID()}") - } + // Fixed, since each metadata record name gets its own Keystore key. + assertThrows(KeepMobileException.StorageNotFound::class.java) { storage.loadShareByKey("__keep_fault_count_test") } assertEquals(before, storage.faultsSoFar()) } } diff --git a/app/src/main/kotlin/io/privkey/keep/nip46/BunkerService.kt b/app/src/main/kotlin/io/privkey/keep/nip46/BunkerService.kt index b4ee19fe..5a5ebad8 100644 --- a/app/src/main/kotlin/io/privkey/keep/nip46/BunkerService.kt +++ b/app/src/main/kotlin/io/privkey/keep/nip46/BunkerService.kt @@ -658,16 +658,16 @@ class BunkerService : Service() { // until then. The revoke is a silent no-op once onDestroy has stopped the // handler, and onDestroy clears serving first, so the mark is kept for the next // start unless this one is still serving. The pinned core also reports success - // when it could not save the emptied grants, so the mark stays unless no storage - // call failed meanwhile; remove that once the core's revoke returns a result. - val mark = revokeAllMark(this) - val requestsSeen = mark.requestsSoFar() - if (mark.isSet()) { - val storage = (applicationContext as? KeepMobileApp)?.getStorage() - val faultsBefore = storage?.faultsSoFar() - handler.revokeAllClients() - mark.clearIfUnchanged(requestsSeen) { serving && storage != null && storage.faultsSoFar() == faultsBefore } - } + // when it could not save the emptied grants, so the mark is kept when any storage + // call failed meanwhile. Ceiling: an unrelated storage fault, or a lasting one, + // also keeps it, and the next start rotates the URL again; a power cut can roll + // back the saved grants while the cleared mark stays. Remove that check once the + // core's revoke returns a result. + revokeAllMark(this).revokeIfSet( + revoke = { handler.revokeAllClients() }, + serving = { serving }, + faultsSoFar = { (applicationContext as? KeepMobileApp)?.getStorage()?.faultsSoFar() }, + ) // A revoke made while the engine was stopped could not drop the client's // persisted grants, and this start has just restored them. Ceiling: one // storage write per denylisted client on every start, and grants live until diff --git a/app/src/main/kotlin/io/privkey/keep/nip46/RevokeAllMark.kt b/app/src/main/kotlin/io/privkey/keep/nip46/RevokeAllMark.kt index b0e6c677..5e935014 100644 --- a/app/src/main/kotlin/io/privkey/keep/nip46/RevokeAllMark.kt +++ b/app/src/main/kotlin/io/privkey/keep/nip46/RevokeAllMark.kt @@ -71,12 +71,25 @@ internal class RevokeAllMark(private val prefs: SharedPreferences) { fun requestsSoFar(): Int = requests /** - * Clears the mark unless it was set again after [seen] was read, or [revokeHeld] says the + * A start's revoke: runs [revoke] when the mark is set, then clears it if the start is still + * [serving] and no storage call failed meanwhile, which [faultsSoFar] counts; null when it + * cannot be read. Not under the lock, since [revoke] blocks on the core. + */ + fun revokeIfSet(revoke: () -> Unit, serving: () -> Boolean, faultsSoFar: () -> Long?) { + val seen = requestsSoFar() + if (!isSet()) return + val faultsBefore = faultsSoFar() + revoke() + clearIfUnchanged(seen) { serving() && faultsBefore != null && faultsSoFar() == faultsBefore } + } + + /** + * Clears the mark unless it was set again after [seen] was read, or [revokeTook] says the * revoke may not have taken. The saved mark stays while a change is open. */ @Synchronized - fun clearIfUnchanged(seen: Int, revokeHeld: () -> Boolean): Boolean { - if (requests != seen || !revokeHeld()) return false + fun clearIfUnchanged(seen: Int, revokeTook: () -> Boolean): Boolean { + if (requests != seen || !revokeTook()) return false if (open == 0) check(prefs.edit().remove(BunkerService.KEY_REVOKE_ALL_ON_START).commit()) { "could not clear the revoke-all mark" } pending = false return true diff --git a/app/src/main/kotlin/io/privkey/keep/storage/AndroidKeystoreStorage.kt b/app/src/main/kotlin/io/privkey/keep/storage/AndroidKeystoreStorage.kt index 3dafa338..e8c6b0e6 100644 --- a/app/src/main/kotlin/io/privkey/keep/storage/AndroidKeystoreStorage.kt +++ b/app/src/main/kotlin/io/privkey/keep/storage/AndroidKeystoreStorage.kt @@ -255,8 +255,8 @@ class AndroidKeystoreStorage( /** * How many fallible storage calls have failed so far, an absence aside. Two reads that - * match show that none failed in between, which is how a caller tells that a core write - * whose own result it cannot see did not fail. + * match show that no storage call failed in between, which is how a caller tells that a + * core write whose own result it cannot see did not fail here. */ fun faultsSoFar(): Long = faults.get()