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..95f355bd 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,10 @@ 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.ImportState 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 +45,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 @@ -80,6 +89,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 +114,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 +210,111 @@ 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)) + // 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" } + var resetAsked = 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) + 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") + 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) + + // 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. + 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) + 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 { screen = Screen.None @@ -238,7 +354,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 +375,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 +409,97 @@ 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 stopped and could not be reset") }, + ) + } + + 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, + markBunkerRevokeAhead: () -> (Boolean) -> Unit = { BunkerService.markRevokeAllAhead(context) }, + action: (AccountActions, () -> Unit) -> Unit, + ) { + val scope = CoroutineScope(SupervisorJob() + Dispatchers.Default) + val dismissed = CompletableDeferred() + try { + val actions = AccountActions( + keepMobile = mobile, + storage = storage, + coroutineScope = scope, + appContext = context, + onBiometricRequest = { _, _, cipher, onResult -> onResult(cipher) }, + onAccountSwitched = onAccountSwitched, + onAccountDeleted = {}, + onLiveNodeDropped = {}, + onStateChanged = {}, + markBunkerRevokeAhead = markBunkerRevokeAhead, + ) + storage.getActiveShareKey()?.let { actions.setCurrentRelays(mobile.getRelayConfig(it).frostRelays) } + action(actions) { dismissed.complete(Unit) } + runBlocking { withTimeout(60_000) { dismissed.await() } } + } finally { + scope.cancel() + } + } + + // 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: () -> (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() + } + } + + // 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" } + // 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 + 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 + 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 +626,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 +645,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 +659,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 +672,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 +697,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 +711,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 +724,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 +737,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 +747,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/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..54b446fa --- /dev/null +++ b/app/src/androidTest/kotlin/io/privkey/keep/nip46/RevokeAllMarkTest.kt @@ -0,0 +1,241 @@ +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.assertThrows +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()) + } + + @Test + fun aMarkSetAheadIsSavedBeforeTheChange() { + RevokeAllMark(prefs).setAhead() + assertTrue(RevokeAllMark(prefs).isSet()) + } + + @Test + 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 + 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 aStartKeepsTheSavedMarkWhileAChangeIsOpen() { + val mark = RevokeAllMark(prefs) + mark.set() + 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 aChangeThatDidNotSwitchWithdrawsAMarkAStartAlreadyRevokedFor() { + val mark = RevokeAllMark(prefs) + mark.set() + 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()) + } + + @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()) + } + + @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 { + 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/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..ed78d1f8 --- /dev/null +++ b/app/src/androidTest/kotlin/io/privkey/keep/storage/StorageFaultCountTest.kt @@ -0,0 +1,37 @@ +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 + +@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 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() + // 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/AccountActions.kt b/app/src/main/kotlin/io/privkey/keep/AccountActions.kt index 36d30645..c66f7b18 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 @@ -63,10 +64,11 @@ 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 (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: () -> (switched: Boolean) -> Unit = { BunkerService.markRevokeAllAhead(appContext) } ) { data class AccountState( val hasShare: Boolean, @@ -129,12 +131,20 @@ 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. + * + * 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) { 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. 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 } } @@ -148,7 +158,12 @@ internal class AccountActions( val activeAfter = withContext(Dispatchers.IO) { runCatching { storage.getActiveShareKey() }.getOrNull() } - if (storeSwitchedAccount(hadShare, activeBefore, activeAfter)) onAccountSwitched() + 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) } } } @@ -226,7 +241,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(true)) { "bunker reset failed" } activateShare(authedCipher, account.groupPubkeyHex) refreshAccountState() } catch (e: Exception) { @@ -254,8 +271,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) { @@ -301,7 +324,7 @@ internal class AccountActions( switchToNextAccountAfterDelete(remainingAccounts.first(), onDismiss) } else if (wasActive) { try { - onAccountSwitched() + onAccountSwitched(false) } finally { refreshAccountState() onDismiss() @@ -329,8 +352,12 @@ 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) { + // 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) } catch (e: Exception) { @@ -346,7 +373,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 e306ef06..c2a87db3 100644 --- a/app/src/main/kotlin/io/privkey/keep/KeepMobileApp.kt +++ b/app/src/main/kotlin/io/privkey/keep/KeepMobileApp.kt @@ -490,18 +490,44 @@ class KeepMobileApp : Application() { if (BuildConfig.DEBUG) Log.d(TAG, "Initialize completed, peers: ${mobile.getPeers().size}") } - suspend fun onAccountSwitched() { - connectionJob?.cancel() - reconnectJob?.cancel() - pinMismatch = null + @androidx.annotation.VisibleForTesting + internal var resetBunkerForSwitch: suspend (Context, KeepMobile) -> Boolean = + { context, mobile -> BunkerService.resetForAccountSwitch(context, mobile) } + + /** + * 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) - keepMobile?.let { mobile -> - runCatching { - BunkerConfigStore.update(mobile) { current -> - BunkerConfigInfo(false, current.authorizedClients) + 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 { bunkerResetComplete = resetBunkerForSwitch(this@KeepMobileApp, it) } + } catch (e: Exception) { + if (e !is CancellationException) bunkerResetFailed = true + throw e + } + } + } + if (bunkerResetFailed && stopIfBunkerResetFails) { + withContext(Dispatchers.IO) { + 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; its clients may need pairing again. Try again" + ) } } + return false } + connectionJob?.cancel() + reconnectJob?.cancel() + pinMismatch = null DescriptorSessionManager.clearAll() withContext(Dispatchers.IO) { // Before the revoke: clearAllAppSettings enumerates permission callers to @@ -515,6 +541,22 @@ 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 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" + ) + } + } // 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). @@ -537,6 +579,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..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 () -> Unit = {} + 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 3a88a2d0..5a5ebad8 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,83 @@ 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. + * + * 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): 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() + // 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): Boolean = + try { + block() + true + } catch (e: CancellationException) { + throw e + } catch (e: Exception) { + failures += e + false + } + val marked = step { revokeAllMark(context).set() } + 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()) + } + } + 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() + // Standalone: the service is being stopped for the switch and must not restart. + 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) 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) + + private var revokeAllMarkInstance: RevokeAllMark? = null + + @Synchronized + 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): (switched: Boolean) -> Unit = revokeAllMark(context).setAhead() + /** * Rotate the bunker credentials whether or not the service is running. */ @@ -444,6 +522,13 @@ 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 @@ -495,6 +580,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 (revokeAllMark(this).isSet()) { + 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") } @@ -562,12 +652,27 @@ 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 { + // An account switch dropped the previous account's clients; their remembered + // 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. The pinned core also reports success + // 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 + // 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) } 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..5e935014 --- /dev/null +++ b/app/src/main/kotlin/io/privkey/keep/nip46/RevokeAllMark.kt @@ -0,0 +1,97 @@ +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. + * 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. + * + * 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. 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() { + requests++ + pending = true + check(prefs.edit().putBoolean(BunkerService.KEY_REVOKE_ALL_ON_START, true).commit()) { + "could not mark the grants for revocation" + } + } + + /** + * Saves the mark ahead of a change that may switch accounts. Returns what the change calls + * 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 { + // 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++ + val finished = AtomicBoolean(false) + return { switched -> if (finished.compareAndSet(false, true)) finishAhead(switched) } + } + + @Synchronized + 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 + fun isSet(): Boolean = pending || prefs.getBoolean(BunkerService.KEY_REVOKE_ALL_ON_START, false) + + @Synchronized + fun requestsSoFar(): Int = requests + + /** + * 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, 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 07bb964f..e8c6b0e6 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 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() + private fun isDkgSecretKey(key: String): Boolean = key == DKG_SECRET_STORAGE_KEY private fun sanitizeKey(key: String): String { 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()];