diff --git a/app/build.gradle.kts b/app/build.gradle.kts index 93e38b3f..da82b0f3 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -65,8 +65,8 @@ android { applicationId = "io.privkey.keep" minSdk = 33 targetSdk = 36 - versionCode = 30 - versionName = "1.3.1" + versionCode = 31 + versionName = "1.3.2" testInstrumentationRunner = "androidx.test.runner.AndroidJUnitRunner" // Reset the persisted kill switch to its disengaged baseline before every diff --git a/app/src/test/kotlin/io/privkey/keep/nip46/ApprovalTimeoutTest.kt b/app/src/androidTest/kotlin/io/privkey/keep/nip46/ApprovalTimeoutTest.kt similarity index 65% rename from app/src/test/kotlin/io/privkey/keep/nip46/ApprovalTimeoutTest.kt rename to app/src/androidTest/kotlin/io/privkey/keep/nip46/ApprovalTimeoutTest.kt index 32e252b5..8f7623fd 100644 --- a/app/src/test/kotlin/io/privkey/keep/nip46/ApprovalTimeoutTest.kt +++ b/app/src/androidTest/kotlin/io/privkey/keep/nip46/ApprovalTimeoutTest.kt @@ -1,26 +1,29 @@ package io.privkey.keep.nip46 +import androidx.test.ext.junit.runners.AndroidJUnit4 +import io.privkey.keep.uniffi.nip46ApprovalTimeoutSecs import org.junit.Assert.assertTrue import org.junit.Test +import org.junit.runner.RunWith /** * The approval wait has to end before the core's own backstop on the same wait. If it * did not, the core could deny a request at its deadline while the user, a moment later, * saw their approval accepted. */ +@RunWith(AndroidJUnit4::class) class ApprovalTimeoutTest { @Test fun theWaitEndsWellBeforeTheCoreBackstop() { + val coreMs = nip46ApprovalTimeoutSecs().toLong() * 1000L assertTrue( "the margin is what lets this side decide first", - BunkerService.APPROVAL_TIMEOUT_MS <= CORE_APPROVAL_TIMEOUT_MS - MIN_MARGIN_MS + BunkerService.APPROVAL_TIMEOUT_MS <= coreMs - MIN_MARGIN_MS ) } private companion object { - // APPROVAL_TIMEOUT in the core's nip46.rs, which is not exported. - const val CORE_APPROVAL_TIMEOUT_MS = 60_000L const val MIN_MARGIN_MS = 5_000L } } 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 95f355bd..ee8a7466 100644 --- a/app/src/androidTest/kotlin/io/privkey/keep/nip46/BunkerRevokeE2ETest.kt +++ b/app/src/androidTest/kotlin/io/privkey/keep/nip46/BunkerRevokeE2ETest.kt @@ -148,8 +148,8 @@ class BunkerRevokeE2ETest { BunkerService.start(context) val url2 = awaitRunning(previous = null) assertRotated(url1, url2) - val swept = System.currentTimeMillis() - assertRefused("the start must revoke the restored grant of a revoked client", settled("sign", c1.secret, url2, "1", "15"), c1.pubkey, swept) + val restarted = System.currentTimeMillis() + assertRefused("a grant revoked while the bunker was stopped must not come back on a start", settled("sign", c1.secret, url2, "1", "15"), c1.pubkey, restarted) assertDead(url1, url2) // Revoke from the Bunker screen while the bunker is running. @@ -342,9 +342,9 @@ class BunkerRevokeE2ETest { } /** - * Leaves no test client a way in: a fresh bunker URL, every approved test client - * denylisted so the start sweep drops any grant it kept, and the request files (which - * hold client secrets and URLs) deleted. Each step runs whatever happened before it. + * Leaves no test client a way in: a fresh bunker key, under which the core restores no + * grant made before it, every approved test client denylisted, and the request files + * (which hold client secrets and URLs) deleted. Each step runs whatever happened before it. */ private fun cleanUp(mobile: KeepMobile, storage: AndroidKeystoreStorage, saved: SavedState): List { val failures = mutableListOf() 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 54b446fa..d33a5100 100644 --- a/app/src/androidTest/kotlin/io/privkey/keep/nip46/RevokeAllMarkTest.kt +++ b/app/src/androidTest/kotlin/io/privkey/keep/nip46/RevokeAllMarkTest.kt @@ -184,46 +184,37 @@ class RevokeAllMarkTest { } @Test - fun aStartThatRevokedCleanlyClearsTheMark() { + fun aStartThatRevokedClearsTheMark() { val mark = RevokeAllMark(prefs) mark.set() var revoked = false - mark.revokeIfSet(revoke = { revoked = true }, serving = { true }, faultsSoFar = { 0L }) + mark.revokeIfSet { revoked = true } assertTrue(revoked) assertFalse(mark.isSet()) assertFalse(RevokeAllMark(prefs).isSet()) } @Test - fun aStorageFaultDuringTheRevokeKeepsTheMark() { + fun aRevokeThatFailsKeepsTheMark() { val mark = RevokeAllMark(prefs) mark.set() - var faults = 0L - mark.revokeIfSet(revoke = { faults++ }, serving = { true }, faultsSoFar = { faults }) + assertThrows(IllegalStateException::class.java) { mark.revokeIfSet { throw IllegalStateException("the grants were not saved") } } assertTrue(mark.isSet()) assertTrue(RevokeAllMark(prefs).isSet()) } @Test - fun aStartThatCannotCountFaultsKeepsTheMark() { + fun aStartDoesNotClearAMarkSetWhileItRevoked() { 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 }) + mark.revokeIfSet { mark.set() } assertTrue(mark.isSet()) } @Test fun aStartWithNoMarkRevokesNothing() { var revoked = false - RevokeAllMark(prefs).revokeIfSet(revoke = { revoked = true }, serving = { true }, faultsSoFar = { 0L }) + RevokeAllMark(prefs).revokeIfSet { revoked = true } assertFalse(revoked) } diff --git a/app/src/androidTest/kotlin/io/privkey/keep/storage/InfallibleStorageReadTest.kt b/app/src/androidTest/kotlin/io/privkey/keep/storage/InfallibleStorageReadTest.kt index 7c8bf64e..23af2dba 100644 --- a/app/src/androidTest/kotlin/io/privkey/keep/storage/InfallibleStorageReadTest.kt +++ b/app/src/androidTest/kotlin/io/privkey/keep/storage/InfallibleStorageReadTest.kt @@ -5,6 +5,7 @@ import android.util.Base64 import androidx.test.core.app.ApplicationProvider import androidx.test.ext.junit.runners.AndroidJUnit4 import io.privkey.keep.uniffi.KeepMobileException +import io.privkey.keep.uniffi.StorageRead import org.junit.After import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse @@ -249,6 +250,53 @@ class InfallibleStorageReadTest { ) } + @Test + fun the_core_read_returns_the_stored_pointer_as_it_is() { + // Even one naming a share that will not read, which getActiveShareKey keeps too, + // and one naming a share that is no longer in the registry, which it would not. + seedTwoSharesWithTheFirstUnreadable(activeKey = GROUP_HEX) + assertEquals(StorageRead.Found(GROUP_HEX), storage().readActiveShareKey()) + + KeystoreEncryptedPrefs.create(context, MULTI_PREFS + SUFFIX).edit() + .putStringSet("all_share_keys", setOf(SECOND_GROUP_HEX)) + .commit() + assertEquals(StorageRead.Found(GROUP_HEX), storage().readActiveShareKey()) + } + + @Test + fun the_core_read_infers_and_writes_nothing_when_no_pointer_is_stored() { + // getActiveShareKey answers this with the readable share; the core's read must + // report the absence, since a caller deciding whether to set the pointer acts on it. + seedTwoSharesWithTheFirstUnreadable(activeKey = null) + assertEquals(SECOND_GROUP_HEX, storage().getActiveShareKey()) + + assertEquals(StorageRead.Absent, storage().readActiveShareKey()) + assertNull( + "the read must not store a pointer", + KeystoreEncryptedPrefs.create(context, MULTI_PREFS + SUFFIX).getString("active_share_key", null) + ) + } + + @Test + fun an_unreadable_pointer_is_unavailable_to_the_core_rather_than_absent() { + seedReadableStore() + KeystoreEncryptedPrefs.create(context, MULTI_PREFS + SUFFIX).edit().remove("active_share_key").commit() + val stored = writeAndFindStoredName(context, MULTI_PREFS + SUFFIX) { + KeystoreEncryptedPrefs.create(context, MULTI_PREFS + SUFFIX).edit().putString("active_share_key", GROUP_HEX).commit() + } + corruptStoredValue(context, MULTI_PREFS + SUFFIX, stored) + + assertEquals(StorageRead.Unavailable, storage().readActiveShareKey()) + } + + @Test + fun a_pointer_lookup_that_cannot_complete_is_unavailable_to_the_core() { + seedReadableStore() + breakNameResolution(MULTI_PREFS + SUFFIX) + + assertEquals(StorageRead.Unavailable, storage().readActiveShareKey()) + } + @Test fun an_unreadable_active_share_does_not_report_an_empty_store() { // The same fault reached through hasShare, which answers a different diff --git a/app/src/androidTest/kotlin/io/privkey/keep/storage/StorageFaultCountTest.kt b/app/src/androidTest/kotlin/io/privkey/keep/storage/StorageFaultCountTest.kt deleted file mode 100644 index ed78d1f8..00000000 --- a/app/src/androidTest/kotlin/io/privkey/keep/storage/StorageFaultCountTest.kt +++ /dev/null @@ -1,37 +0,0 @@ -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 c66f7b18..82680d9c 100644 --- a/app/src/main/kotlin/io/privkey/keep/AccountActions.kt +++ b/app/src/main/kotlin/io/privkey/keep/AccountActions.kt @@ -125,8 +125,8 @@ internal class AccountActions( /** * Runs [store], a core call that stores a share and makes it the active one, on the IO * dispatcher, and treats it as an account switch when it replaced another account. - * Without the switch cleanup the previous account's grants would apply to the new key - * and a running bunker would keep serving the previous one. Compared rather than + * Without the switch cleanup the previous account's grants and bunker clients would + * carry over to the new key. Compared rather than * assumed, so a call that switched and then failed is still cleaned up, while a * 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 @@ -268,9 +268,8 @@ internal class AccountActions( // share it resolves when none is set, so it may be this one. val wasActive = activeAccountKey == null || account.groupPubkeyHex == activeAccountKey if (wasActive) { - // 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 + // Before the delete, not after the follow-up switch prompt: this account's + // grants and bunker clients would otherwise stand until that prompt is // 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. diff --git a/app/src/main/kotlin/io/privkey/keep/KeepMobileApp.kt b/app/src/main/kotlin/io/privkey/keep/KeepMobileApp.kt index c2a87db3..875d5c4f 100644 --- a/app/src/main/kotlin/io/privkey/keep/KeepMobileApp.kt +++ b/app/src/main/kotlin/io/privkey/keep/KeepMobileApp.kt @@ -487,6 +487,7 @@ class KeepMobileApp : Application() { mobile.initialize(relays) } liveNode.built(startedAt) + io.privkey.keep.nip46.BunkerService.startAfterNodeBuilt(this, mobile) if (BuildConfig.DEBUG) Log.d(TAG, "Initialize completed, peers: ${mobile.getPeers().size}") } @@ -696,6 +697,7 @@ class KeepMobileApp : Application() { */ fun onLiveNodeDropped() { liveNode.dropped() + io.privkey.keep.nip46.BunkerService.restartAfterNodeDrop() mainHandler.post { // Not over a node an initialize has built since this drop. if (!liveNode.isLive()) { 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 5a5ebad8..281ad5b6 100644 --- a/app/src/main/kotlin/io/privkey/keep/nip46/BunkerService.kt +++ b/app/src/main/kotlin/io/privkey/keep/nip46/BunkerService.kt @@ -94,6 +94,10 @@ class BunkerService : Service() { 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" + // The core's message for a start that a stop cancelled. + private const val STOPPED_WHILE_STARTING = "The bunker was stopped while it was starting" + private const val START_SETTLE_TIMEOUT_MS = 5_000L + private const val START_SETTLE_POLL_MS = 25L private val HEX_PUBKEY_REGEX = Regex("^[a-fA-F0-9]{64}$") @@ -141,22 +145,23 @@ class BunkerService : Service() { current()?.authorizedClientsCache?.remove(pubkey.lowercase()) } - internal fun revokeClientInEngine(pubkey: String) { + // Through a handler of its own: the core revoke reaches the running engine from any + // handler, drops persisted grants with no bunker running, and stops whatever bunker + // is running, which the caller restarts. + private fun revokeClientInEngine(keepMobile: io.privkey.keep.uniffi.KeepMobile, pubkey: String) { current()?.authorizedClientsCache?.remove(pubkey.lowercase()) - val handler = current()?.bunkerHandler ?: return try { - handler.revokeClient(pubkey) + BunkerHandler(keepMobile).use { it.revokeClient(pubkey) } } catch (e: Exception) { if (BuildConfig.DEBUG) Log.e(TAG, "Engine client revoke failed: ${e::class.simpleName}") throw e } } - internal fun revokeAllClientsInEngine() { + private fun revokeAllClientsInEngine(keepMobile: io.privkey.keep.uniffi.KeepMobile) { current()?.authorizedClientsCache?.clear() - val handler = current()?.bunkerHandler ?: return try { - handler.revokeAllClients() + BunkerHandler(keepMobile).use { it.revokeAllClients() } } catch (e: Exception) { if (BuildConfig.DEBUG) Log.e(TAG, "Engine revoke-all failed: ${e::class.simpleName}") throw e @@ -165,13 +170,13 @@ class BunkerService : Service() { /** * The one revoke path for NIP-46 clients: [pubkeys], or every client when null. - * Denylists them, drops them from the app's lists and the engine, then rotates the - * bunker URL, since the connect secret in it is reusable and a revoked client that - * kept it could connect again under a new key. Returns the saved bunker config. + * Denylists them, drops them from the app's lists and the core, which also replaces + * the connect secret, then rotates the bunker URL and restarts a running bunker. + * Returns the saved bunker config. * - * Stopgap until the core owns revocation: a single core revoke that records the - * revocation, drops live and persisted grants and invalidates the connect secret - * replaces this sequence, and the Kotlin denylist with it. + * The rotation keeps revoke as it was before the core replaced the connect secret + * itself: every client pairs again. It can go once the copy says only the revoked + * client is affected; the Kotlin denylist goes once the core records revocations. */ internal suspend fun revokeClients( context: Context, @@ -197,37 +202,53 @@ class BunkerService : Service() { runCatching { Nip46ClientStore.removeClient(context, pubkey) } .onFailure { if (BuildConfig.DEBUG) Log.e(TAG, "Failed to remove NIP-46 client: ${it::class.simpleName}") } } + val revokeFailure = try { + if (pubkeys == null) revokeAllClientsInEngine(keepMobile) else revoked.forEach { revokeClientInEngine(keepMobile, it) } + null + } catch (e: CancellationException) { + throw e + } catch (e: Exception) { + e + } + // The secret must change even if the core revoke failed, and the core revoke + // stopped a running bunker, which the rotation restarts. try { - if (pubkeys == null) revokeAllClientsInEngine() else revoked.forEach { revokeClientInEngine(it) } - } finally { - // The secret must change even if the engine revoke failed; the next start's - // sweep revokes any grant the engine still holds. rotateBunkerCredentials(keepMobile) + } catch (e: CancellationException) { + if (revokeFailure != null) runCatching { revokeAllMark(context).set() } + throw e + } catch (e: Exception) { + // Neither took, so the client's saved grants still match the bunker key and + // its connect secret still works: every client is revoked before the next start + // serves, which a restart reaches now. + val markFailure = if (revokeFailure != null) runCatching { revokeAllMark(context).set() }.exceptionOrNull() else null + current()?.restartIfServing() + throw (revokeFailure ?: e).also { thrown -> + if (thrown !== e) thrown.addSuppressed(e) + markFailure?.let(thrown::addSuppressed) + } } + revokeFailure?.let { throw it } 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. + * Called on an account switch. The bunker's URL and its authorized clients are + * device-wide, so without this the clients paired under the previous account would + * reach the user under the new identity. Drops the clients, turns the bunker off, + * rotates the URL and revokes every remembered grant, and marks the grants to be + * revoked again before the next start serves, so a step that failed here is retried + * there. Nothing is denylisted, so a client can still be paired to the new account + * deliberately. The core already keeps the previous account's grants from loading, + * since it scopes them to the bunker key and the account; this does not rely on that. * - * 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. + * Throws when the mark could not be saved, since only it carries the revoke to the + * next start when this one failed. 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. + * 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 @@ -265,6 +286,9 @@ class BunkerService : Service() { pendingNostrConnectRequests.clear() // Standalone: the service is being stopped for the switch and must not restart. step { BunkerHandler(keepMobile).use { it.rotateBunkerCredentials() } } + // Now rather than only at the next start, since the core revokes with no bunker + // running; the mark retries it if this fails. + step { BunkerHandler(keepMobile).use { it.revokeAllClients() } } // Again, now the config is off: a start asked for since the first halt read it on. stop(context) current()?.haltForAccountSwitch() @@ -290,8 +314,8 @@ class BunkerService : Service() { internal suspend fun rotateBunkerCredentials(keepMobile: io.privkey.keep.uniffi.KeepMobile) { current()?.takeIf { it.serving }?.let { return it.rotateBunkerUrl() } BunkerHandler(keepMobile).use { it.rotateBunkerCredentials() } - // A service that began starting during the rotation may have loaded the old keys. - // Remove once the pinned core's rotation stops a bunker started from any handler. + // A service that began starting meanwhile was stopped by the rotation; this rotates + // once more and starts it again. current()?.takeIf { it.serving }?.rotateBunkerUrl() } @@ -309,6 +333,25 @@ class BunkerService : Service() { return rotate } + /** + * The core stops the bunker whenever it drops its live node, without telling this + * service, so a service still meant to serve starts it again. The start retries while + * the node is missing (about a minute) and then gives up; [startAfterNodeBuilt] starts + * it once a node is built. + */ + internal fun restartAfterNodeDrop() { + current()?.restartIfServing() + } + + /** Starts an enabled bunker that is not running once the core has a node to serve. */ + internal fun startAfterNodeBuilt(context: Context, keepMobile: io.privkey.keep.uniffi.KeepMobile) { + // A live service's own start, and its retries, own the bunker. + if (current()?.serving == true) return + if (!runCatching { keepMobile.getBunkerConfig().enabled }.getOrDefault(false)) return + runCatching { start(context) } + .onFailure { if (BuildConfig.DEBUG) Log.w(TAG, "Could not start the bunker after a node build: ${it::class.simpleName}") } + } + fun queueNostrConnectRequest(request: NostrConnectRequest): Boolean { return pendingNostrConnectRequests.offer(request) } @@ -522,6 +565,21 @@ class BunkerService : Service() { } } + private fun restartIfServing() { + val keepMobile = keepMobileRef ?: return + serviceScope.launch { + val relays = runCatching { keepMobile.getRelayConfig(null).bunkerRelays }.getOrDefault(emptyList()) + val restart = startStopMutex.withLock { + if (serving && relays.isNotEmpty()) { + _bunkerUrl.value = null + _status.value = BunkerStatus.STARTING + } + serving && relays.isNotEmpty() + } + if (restart) startBunker(keepMobile, relays) + } + } + private suspend fun haltForAccountSwitch() { startStopMutex.withLock { serving = false @@ -585,6 +643,13 @@ class BunkerService : Service() { if (revokeAllMark(this).isSet()) { BunkerHandler(keepMobile).use { it.rotateBunkerCredentials() } } + // And revokes every remembered grant before this start can restore one. The core + // revokes with no bunker running and reports a save that failed, which keeps the + // mark and fails this start. + revokeAllMark(this).revokeIfSet { + BunkerHandler(keepMobile).use { it.revokeAllClients() } + authorizedClientsCache.clear() + } if (rotateOnceForEarlierRevokes(this, keepMobile)) { logActivity(EventLogCategory.BUNKER, EventLogLevel.WARN, "bunker", "URL rotated because a client was revoked before this version; pair clients again") } @@ -652,38 +717,27 @@ class BunkerService : Service() { return false } - 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) } - } catch (e: Exception) { + // The core returns from the start before its run task marks the bunker running, so + // the status is read once it leaves starting. A start that has not settled on running + // within the timeout is tried again with a fresh handler, since this one still counts + // as starting. Remove once the core's start returns with the bunker already running. + val started = awaitStartSettled(handler) + // onDestroy may have stopped it meanwhile, and reports the stop itself. + if (!serving) { runCatching { handler.stopBunker() } - throw e + return false + } + if (started != BunkerStatus.RUNNING) { + runCatching { handler.stopBunker() } + if (attempt < MAX_START_RETRIES) return true + if (BuildConfig.DEBUG) Log.e(TAG, "Bunker start did not settle: $started") + _status.value = BunkerStatus.ERROR + stopSelf() + return false } - val url = handler.getBunkerUrl() _bunkerUrl.value = url - _status.value = handler.getBunkerStatus() + _status.value = started updateNotification(isActive = true) @@ -694,6 +748,10 @@ class BunkerService : Service() { if (e is io.privkey.keep.uniffi.KeepMobileException.NotInitialized && attempt < MAX_START_RETRIES) { return true } + // onDestroy stopped this handler while it started: a stop, not a failure. + if (e is io.privkey.keep.uniffi.KeepMobileException.InitializationFailed && e.msg == STOPPED_WHILE_STARTING && !serving) { + return false + } if (BuildConfig.DEBUG) Log.e(TAG, "Failed to start bunker: ${e::class.simpleName}") _status.value = BunkerStatus.ERROR stopSelf() @@ -701,6 +759,16 @@ class BunkerService : Service() { } } + private suspend fun awaitStartSettled(handler: BunkerHandler): BunkerStatus = + withTimeoutOrNull(START_SETTLE_TIMEOUT_MS) { + var status = handler.getBunkerStatus() + while (status == BunkerStatus.STARTING) { + delay(START_SETTLE_POLL_MS) + status = handler.getBunkerStatus() + } + status + } ?: BunkerStatus.STARTING + private fun invokeStartBunkerWithProxy( handler: BunkerHandler, relays: List, 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 5e935014..fc82a33a 100644 --- a/app/src/main/kotlin/io/privkey/keep/nip46/RevokeAllMark.kt +++ b/app/src/main/kotlin/io/privkey/keep/nip46/RevokeAllMark.kt @@ -4,7 +4,8 @@ import android.content.SharedPreferences import java.util.concurrent.atomic.AtomicBoolean /** - * The account-switch mark that makes the next bunker start revoke every remembered grant. + * The mark that makes the next bunker start revoke every remembered grant: set by an account + * switch, and by a client revoke whose core revoke and rotation both failed. * 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. @@ -16,10 +17,10 @@ import java.util.concurrent.atomic.AtomicBoolean * 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. + * that may replace it. Ceiling: a power cut can roll back a save that reported success, and + * the next start then skips its revoke. After a switch the core still does not load the + * previous account's grants, since it scopes them to the bunker key and the account; after a + * failed client revoke nothing else covers it. Goes with the start-time revoke. */ internal class RevokeAllMark(private val prefs: SharedPreferences) { private var pending = false @@ -71,16 +72,15 @@ internal class RevokeAllMark(private val prefs: SharedPreferences) { 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. + * A start's revoke: runs [revoke] when the mark is set and clears it once [revoke] returns, + * which the core does only after saving the emptied grants. A [revoke] that throws keeps the + * mark. Not under the lock, since [revoke] blocks on the core. */ - fun revokeIfSet(revoke: () -> Unit, serving: () -> Boolean, faultsSoFar: () -> Long?) { + fun revokeIfSet(revoke: () -> Unit) { val seen = requestsSoFar() if (!isSet()) return - val faultsBefore = faultsSoFar() revoke() - clearIfUnchanged(seen) { serving() && faultsBefore != null && faultsSoFar() == faultsBefore } + clearIfUnchanged(seen) { 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 e8c6b0e6..53db1c5a 100644 --- a/app/src/main/kotlin/io/privkey/keep/storage/AndroidKeystoreStorage.kt +++ b/app/src/main/kotlin/io/privkey/keep/storage/AndroidKeystoreStorage.kt @@ -14,6 +14,7 @@ import io.privkey.keep.BuildConfig import io.privkey.keep.uniffi.KeepMobileException import io.privkey.keep.uniffi.SecureStorage import io.privkey.keep.uniffi.ShareMetadataInfo +import io.privkey.keep.uniffi.StorageRead import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.withContext import java.security.InvalidAlgorithmParameterException @@ -26,7 +27,6 @@ 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 @@ -243,23 +243,12 @@ 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 { @@ -947,6 +936,21 @@ class AndroidKeystoreStorage( override fun getActiveShareKey(): String? = infallibleRead("getActiveShareKey", onFault = null) { activeShareKeyUnguarded() } + /** + * The stored pointer as it is, for the core's decisions to set or clear it: no + * registry check, no inference and no write, unlike [getActiveShareKey]. A pointer + * that will not decrypt, or a lookup that throws, is [StorageRead.Unavailable], so it + * is never taken for an unset pointer. + */ + override fun readActiveShareKey(): StorageRead = + infallibleRead("readActiveShareKey", onFault = StorageRead.Unavailable) { + when (val read = multiSharePrefs.readString(KEY_ACTIVE_SHARE)) { + is PrefRead.Found -> StorageRead.Found(read.value) + PrefRead.Absent -> StorageRead.Absent + PrefRead.Unreadable -> StorageRead.Unavailable + } + } + /** * Whether one share's own file holds share data, with [onFault] deciding what an * unreadable file answers, because the two callers need opposite answers. diff --git a/app/src/main/res/values/strings_connections.xml b/app/src/main/res/values/strings_connections.xml index 41e42726..2cc5495e 100644 --- a/app/src/main/res/values/strings_connections.xml +++ b/app/src/main/res/values/strings_connections.xml @@ -92,15 +92,15 @@ Private/internal relay addresses are not allowed No valid relay URLs found in bunker URL Revoke All Clients - This will disconnect all authorized clients and change the bunker URL, so the old URL can no longer connect. Revoked clients stay blocked until you approve a new connection from them. + This will disconnect all authorized clients, remove the actions they were allowed to remember, and change the bunker URL, so the old URL can no longer connect. Revoked clients stay blocked until you approve a new connection from them. Revoke Client - This client loses access and stays blocked until you approve a new connection from it, and the bunker URL changes so the old URL can no longer connect. Your other clients will need to pair again with the new URL. + This client loses access and stays blocked until you approve a new connection from it, and the bunker URL changes so the old URL can no longer connect. Your other clients will need to pair again with the new URL and approve again the actions they were allowed to remember. Revoke Revoke All Cancel Rotate Bunker URL Rotate Bunker URL - This generates a new bunker URL and invalidates the current one. Anyone holding the old URL (including from a leak) can no longer connect or sign. All existing clients must re-pair with the new URL. + This generates a new bunker URL and invalidates the current one. Anyone holding the old URL (including from a leak) can no longer connect or sign. All existing clients must re-pair with the new URL and approve again the actions they were allowed to remember. Rotate URL Cancel Bunker URL rotated. Re-pair your clients. @@ -174,7 +174,7 @@ Back Disconnect Client? Disconnect App? - This will remove all saved permissions and revoke authorization for %1$s, which stays blocked until you approve a new connection from it. The bunker URL also changes, so your other clients will need to pair again with the new URL. + This will remove all saved permissions and revoke authorization for %1$s, which stays blocked until you approve a new connection from it. The bunker URL also changes, so your other clients will need to pair again with the new URL and approve again the actions they were allowed to remember. This will remove all saved permissions for %1$s. The app will need to request permission again. Disconnect Cancel diff --git a/fastlane/metadata/android/de-DE/changelogs/312.txt b/fastlane/metadata/android/de-DE/changelogs/312.txt new file mode 100644 index 00000000..dd9815d6 --- /dev/null +++ b/fastlane/metadata/android/de-DE/changelogs/312.txt @@ -0,0 +1 @@ +Sicherheitsupdate. Einer Zertifizierungsstelle mit kompromittierten Schlüsseln wird für Relay-Verbindungen nicht mehr vertraut. Beim Wechseln oder Entfernen eines Kontos werden die damit gekoppelten Bunker-Apps getrennt, und gemerkte Freigaben gelten nicht weiter. Ein Widerruf sperrt eine Bunker-App sofort aus, auch bei ausgeschaltetem Bunker. Unlesbare Einstellungen behalten ihre Einschränkungen. Bunker-Apps fragen gemerkte Aktionen einmal neu an. diff --git a/fastlane/metadata/android/de-DE/changelogs/314.txt b/fastlane/metadata/android/de-DE/changelogs/314.txt new file mode 100644 index 00000000..dd9815d6 --- /dev/null +++ b/fastlane/metadata/android/de-DE/changelogs/314.txt @@ -0,0 +1 @@ +Sicherheitsupdate. Einer Zertifizierungsstelle mit kompromittierten Schlüsseln wird für Relay-Verbindungen nicht mehr vertraut. Beim Wechseln oder Entfernen eines Kontos werden die damit gekoppelten Bunker-Apps getrennt, und gemerkte Freigaben gelten nicht weiter. Ein Widerruf sperrt eine Bunker-App sofort aus, auch bei ausgeschaltetem Bunker. Unlesbare Einstellungen behalten ihre Einschränkungen. Bunker-Apps fragen gemerkte Aktionen einmal neu an. diff --git a/fastlane/metadata/android/en-US/changelogs/312.txt b/fastlane/metadata/android/en-US/changelogs/312.txt new file mode 100644 index 00000000..4e3530e7 --- /dev/null +++ b/fastlane/metadata/android/en-US/changelogs/312.txt @@ -0,0 +1 @@ +Security update. A certificate authority whose keys were compromised is no longer trusted for relay connections. Switching or removing an account now disconnects the bunker apps paired under it, and its remembered approvals no longer carry over. Revoking a bunker app cuts it off at once, even while the bunker is off. Stored settings that cannot be read now keep their restrictions instead of resetting. Bunker apps will ask once more for actions you had set to remember. diff --git a/fastlane/metadata/android/en-US/changelogs/314.txt b/fastlane/metadata/android/en-US/changelogs/314.txt new file mode 100644 index 00000000..4e3530e7 --- /dev/null +++ b/fastlane/metadata/android/en-US/changelogs/314.txt @@ -0,0 +1 @@ +Security update. A certificate authority whose keys were compromised is no longer trusted for relay connections. Switching or removing an account now disconnects the bunker apps paired under it, and its remembered approvals no longer carry over. Revoking a bunker app cuts it off at once, even while the bunker is off. Stored settings that cannot be read now keep their restrictions instead of resetting. Bunker apps will ask once more for actions you had set to remember. diff --git a/fastlane/metadata/android/es-ES/changelogs/312.txt b/fastlane/metadata/android/es-ES/changelogs/312.txt new file mode 100644 index 00000000..479eb37b --- /dev/null +++ b/fastlane/metadata/android/es-ES/changelogs/312.txt @@ -0,0 +1 @@ +Actualización de seguridad. Ya no se confía en una autoridad de certificación con claves comprometidas para conectar con relés. Al cambiar o eliminar una cuenta, las apps del bunker vinculadas a ella se desconectan y sus aprobaciones recordadas dejan de valer. Revocar una app del bunker la bloquea al instante, aunque el bunker esté apagado. Los ajustes ilegibles conservan sus restricciones. Las apps del bunker pedirán de nuevo una vez lo que tenía recordado. diff --git a/fastlane/metadata/android/es-ES/changelogs/314.txt b/fastlane/metadata/android/es-ES/changelogs/314.txt new file mode 100644 index 00000000..479eb37b --- /dev/null +++ b/fastlane/metadata/android/es-ES/changelogs/314.txt @@ -0,0 +1 @@ +Actualización de seguridad. Ya no se confía en una autoridad de certificación con claves comprometidas para conectar con relés. Al cambiar o eliminar una cuenta, las apps del bunker vinculadas a ella se desconectan y sus aprobaciones recordadas dejan de valer. Revocar una app del bunker la bloquea al instante, aunque el bunker esté apagado. Los ajustes ilegibles conservan sus restricciones. Las apps del bunker pedirán de nuevo una vez lo que tenía recordado. diff --git a/fastlane/metadata/android/ja-JP/changelogs/312.txt b/fastlane/metadata/android/ja-JP/changelogs/312.txt new file mode 100644 index 00000000..13d7b6d0 --- /dev/null +++ b/fastlane/metadata/android/ja-JP/changelogs/312.txt @@ -0,0 +1 @@ +セキュリティアップデートです。鍵が漏えいした認証局は、リレーへの接続で信頼されなくなりました。アカウントを切り替えたり削除したりすると、そのアカウントで連携したバンカーアプリは切断され、記憶された承認は引き継がれません。バンカーアプリの取り消しは、バンカーが停止中でもすぐに有効になります。読み取れない設定は、リセットされずに制限を保ちます。記憶していた操作は、バンカーアプリから一度だけ改めて確認されます。 diff --git a/fastlane/metadata/android/ja-JP/changelogs/314.txt b/fastlane/metadata/android/ja-JP/changelogs/314.txt new file mode 100644 index 00000000..13d7b6d0 --- /dev/null +++ b/fastlane/metadata/android/ja-JP/changelogs/314.txt @@ -0,0 +1 @@ +セキュリティアップデートです。鍵が漏えいした認証局は、リレーへの接続で信頼されなくなりました。アカウントを切り替えたり削除したりすると、そのアカウントで連携したバンカーアプリは切断され、記憶された承認は引き継がれません。バンカーアプリの取り消しは、バンカーが停止中でもすぐに有効になります。読み取れない設定は、リセットされずに制限を保ちます。記憶していた操作は、バンカーアプリから一度だけ改めて確認されます。 diff --git a/fastlane/metadata/android/pt-BR/changelogs/312.txt b/fastlane/metadata/android/pt-BR/changelogs/312.txt new file mode 100644 index 00000000..63fa6ad8 --- /dev/null +++ b/fastlane/metadata/android/pt-BR/changelogs/312.txt @@ -0,0 +1 @@ +Atualização de segurança. Uma autoridade certificadora com chaves comprometidas não é mais confiável para conexões com relays. Ao trocar ou remover uma conta, os apps do bunker pareados com ela são desconectados e as aprovações lembradas deixam de valer. Revogar um app do bunker o bloqueia na hora, mesmo com o bunker desligado. Configurações ilegíveis mantêm suas restrições. Os apps do bunker vão pedir mais uma vez as ações que você tinha lembrado. diff --git a/fastlane/metadata/android/pt-BR/changelogs/314.txt b/fastlane/metadata/android/pt-BR/changelogs/314.txt new file mode 100644 index 00000000..63fa6ad8 --- /dev/null +++ b/fastlane/metadata/android/pt-BR/changelogs/314.txt @@ -0,0 +1 @@ +Atualização de segurança. Uma autoridade certificadora com chaves comprometidas não é mais confiável para conexões com relays. Ao trocar ou remover uma conta, os apps do bunker pareados com ela são desconectados e as aprovações lembradas deixam de valer. Revogar um app do bunker o bloqueia na hora, mesmo com o bunker desligado. Configurações ilegíveis mantêm suas restrições. Os apps do bunker vão pedir mais uma vez as ações que você tinha lembrado. diff --git a/keep.version b/keep.version index a32d51bb..056b0e94 100644 --- a/keep.version +++ b/keep.version @@ -1 +1 @@ -43841928a2e9e60456de75894aa1105577184ca5 +d90607d589b82a598905126cedc2e49b1677d337