diff --git a/app/src/androidTest/kotlin/io/privkey/keep/nip55/AppSignPolicyOverridesInstrumentedTest.kt b/app/src/androidTest/kotlin/io/privkey/keep/nip55/AppSignPolicyOverridesInstrumentedTest.kt index 3a3ffef0..b259aabe 100644 --- a/app/src/androidTest/kotlin/io/privkey/keep/nip55/AppSignPolicyOverridesInstrumentedTest.kt +++ b/app/src/androidTest/kotlin/io/privkey/keep/nip55/AppSignPolicyOverridesInstrumentedTest.kt @@ -11,6 +11,7 @@ import io.privkey.keep.uniffi.Nip55RequestType import io.privkey.keep.uniffi.SignPolicySelection import io.privkey.keep.uniffi.SignPolicySelectionStorage import io.privkey.keep.uniffi.SignPolicyStore +import io.privkey.keep.uniffi.StorageRead import kotlinx.coroutines.runBlocking import org.junit.After import org.junit.Assert.assertEquals @@ -814,12 +815,11 @@ class AppSignPolicyOverridesInstrumentedTest { } /** - * Why the marker rather than the absence check carries this. A core that reads as - * empty is indistinguishable from one whose value would not decrypt, and by the time - * a second run happens the core may durably hold the stricter Manual an unconfirmed - * write pinned while the row still holds the looser tier the user asked for. Copying - * the row again would then widen the app, so an attempted package is never copied - * twice whatever the core answers. + * Why the marker rather than the absence check carries this. By the time a second run + * happens the core may durably hold the stricter Manual an unconfirmed write pinned + * while the row still holds the looser tier the user asked for. Copying the row again + * would then widen the app, so an attempted package is never copied twice, even when + * the core answers that nothing is stored. */ @Test fun anAttemptedPackageIsNotCopiedWhenTheCoreReadsAsEmpty() = runBlocking { @@ -952,7 +952,8 @@ private class UncommittableStorage : SignPolicySelectionStorage { private val values = HashMap() - override fun load(key: String): String? = values[key] + override fun load(key: String): StorageRead = + values[key]?.let { StorageRead.Found(it) } ?: StorageRead.Absent override fun save(key: String, value: String): Boolean { values[key] = value @@ -976,7 +977,8 @@ private class UnremovableStorage(private val unremovablePackage: String) : SignP private val values = HashMap() - override fun load(key: String): String? = values[key] + override fun load(key: String): StorageRead = + values[key]?.let { StorageRead.Found(it) } ?: StorageRead.Absent override fun save(key: String, value: String): Boolean { values[key] = value @@ -991,16 +993,16 @@ private class UnremovableStorage(private val unremovablePackage: String) : SignP } /** - * A backend whose reads all come back empty while its writes persist: the decrypt fault - * the encrypted-prefs layer reports as an absent value, since it returns the default - * when a value cannot be decrypted. Records what it was asked to store so a test can - * tell a copy that was skipped from one that landed. + * A backend whose reads all come back absent while its writes persist, so a test can + * see what the migration does when the core answers that nothing is stored whatever it + * holds. Records what it was asked to store so a test can tell a copy that was skipped + * from one that landed. */ private class UnreadableStorage : SignPolicySelectionStorage { val saved = mutableListOf>() - override fun load(key: String): String? = null + override fun load(key: String): StorageRead = StorageRead.Absent override fun save(key: String, value: String): Boolean { saved += key to value diff --git a/app/src/androidTest/kotlin/io/privkey/keep/nip55/PermissionStoreIntegrationTest.kt b/app/src/androidTest/kotlin/io/privkey/keep/nip55/PermissionStoreIntegrationTest.kt index dd718da2..726e6fc0 100644 --- a/app/src/androidTest/kotlin/io/privkey/keep/nip55/PermissionStoreIntegrationTest.kt +++ b/app/src/androidTest/kotlin/io/privkey/keep/nip55/PermissionStoreIntegrationTest.kt @@ -629,9 +629,9 @@ class PermissionStoreIntegrationTest { @Test fun flippingAGrantToAskDropsTheLifetimeItWouldHaveExpiredWith() = runBlocking { // Same rule from the other side, and the lifetime here is the grant's own - // rather than the clamp's. An ask that expires stops blocking both the policy - // auto-approve and the whitelisted-relay auto-accept, which is the setting the - // user just switched on, and the flip that set it offered no duration. + // rather than the clamp's. An ask that expires stops blocking the policy + // auto-approve, which is the setting the user just switched on, and the flip + // that set it offered no duration. store.grantPermission("com.test.app", Nip55RequestType.SIGN_EVENT, SENSITIVE_KIND, PermissionDuration.ONE_HOUR) val row = store.getPermissionsForCaller("com.test.app").single() assertEquals("precondition: the grant is bounded", HOUR_MS, row.durationMs) diff --git a/app/src/androidTest/kotlin/io/privkey/keep/nip55/RelayAuthDecisionTest.kt b/app/src/androidTest/kotlin/io/privkey/keep/nip55/RelayAuthDecisionTest.kt new file mode 100644 index 00000000..e953faa5 --- /dev/null +++ b/app/src/androidTest/kotlin/io/privkey/keep/nip55/RelayAuthDecisionTest.kt @@ -0,0 +1,114 @@ +package io.privkey.keep.nip55 + +import android.content.Context +import android.os.SystemClock +import androidx.test.core.app.ApplicationProvider +import androidx.test.ext.junit.runners.AndroidJUnit4 +import io.privkey.keep.storage.RelayAuthWhitelistStore +import io.privkey.keep.uniffi.Nip55DecisionInputs +import io.privkey.keep.uniffi.Nip55Outcome +import io.privkey.keep.uniffi.Nip55RequestType +import io.privkey.keep.uniffi.Nip55StoredPermission +import io.privkey.keep.uniffi.Nip55VelocityCheck +import io.privkey.keep.uniffi.SignPolicySelection +import io.privkey.keep.uniffi.evaluateNip55Request +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith + +/** + * What the background path decides for a relay auth, through the pinned core and the + * same whitelist fold the ContentProvider uses. + * + * The gate tests stop at the gate value. Whether a listed relay is then signed is + * decided below it, in the core, and that is where membership used to authorize an + * app that had never been granted anything. These pin the core this app ships with, + * so a pin that moved back to a core without the fix fails here. + */ +@RunWith(AndroidJUnit4::class) +class RelayAuthDecisionTest { + + private val context: Context = ApplicationProvider.getApplicationContext() + private lateinit var store: RelayAuthWhitelistStore + + @Before + fun setup() { + context.deleteSharedPreferences(RelayAuthWhitelistStore.PREFS_NAME) + store = RelayAuthWhitelistStore(context) + } + + @After + fun teardown() { + context.deleteSharedPreferences(RelayAuthWhitelistStore.PREFS_NAME) + } + + @Test + fun aListedRelayWithNoStandingPermissionPromptsRatherThanSigning() { + store.add("relay.example.com") + val outcome = decide(exact = null) + assertTrue( + "membership must not authorize signing on its own, got $outcome", + outcome is Nip55Outcome.RequireUi + ) + } + + // The control: the same listed relay is signed once the app holds a grant for it, + // so the case above cannot pass on inputs that would prompt for anything. + @Test + fun aListedRelayWithAStandingPermissionIsSigned() { + store.add("relay.example.com") + assertEquals(Nip55Outcome.AutoApprove, decide(exact = allow())) + } + + @Test + fun anUnlistedRelayIsRejectedEvenWithAStandingPermission() { + store.add("other.example.com") + assertEquals(Nip55Outcome.Reject("deny_relay_whitelist"), decide(exact = allow())) + } + + private fun allow() = Nip55StoredPermission( + decision = "allow", + expiresAt = null, + createdAt = System.currentTimeMillis(), + createdAtElapsed = SystemClock.elapsedRealtime(), + durationMs = null + ) + + private fun decide(exact: Nip55StoredPermission?): Nip55Outcome { + val (hosts, readFailed) = relayWhitelistInputs(store) + return evaluateNip55Request( + Nip55DecisionInputs( + packageName = "com.test.app", + callerVerified = true, + signingKilled = false, + isLocked = false, + requestType = Nip55RequestType.SIGN_EVENT, + eventJson = EVENT_22242, + frontDoorWithinLimit = true, + velocityCheck = Nip55VelocityCheck.ALLOWED, + relayWhitelist = hosts, + relayWhitelistReadFailed = readFailed, + policySelection = SignPolicySelection.MANUAL, + isOptedIn = false, + optInRateCheck = null, + hasSignedKindBefore = true, + appAgeMs = null, + currentHour = 12u, + appExpired = false, + permissionLookupOk = true, + storedExactPermission = exact, + storedGenericPermission = null, + nowElapsedMs = SystemClock.elapsedRealtime(), + nowWallMs = System.currentTimeMillis() + ) + ) + } + + private companion object { + const val EVENT_22242 = + """{"kind":22242,"content":"","tags":[["relay","wss://relay.example.com/"],["challenge","abc"]]}""" + } +} diff --git a/app/src/androidTest/kotlin/io/privkey/keep/nip55/RelayAuthGateTest.kt b/app/src/androidTest/kotlin/io/privkey/keep/nip55/RelayAuthGateTest.kt index 8f115eff..1462dfeb 100644 --- a/app/src/androidTest/kotlin/io/privkey/keep/nip55/RelayAuthGateTest.kt +++ b/app/src/androidTest/kotlin/io/privkey/keep/nip55/RelayAuthGateTest.kt @@ -135,13 +135,13 @@ class RelayAuthGateTest { assertEquals(emptyList(), hosts) } - // The same relay, listed and readable, is accepted. Without this the test + // The same relay, listed and readable, is not rejected. Without this the test // above could pass on a gate that rejected everything. @Test - fun whitelistedRelayAutoAccepts() { + fun whitelistedRelayResolvesToWhitelisted() { store.add("relay.example.com") val (gate, host) = evaluateRelayAuthGate(store, event22242) - assertEquals(Nip55RelayAuthGate.AUTO_ACCEPT, gate) + assertEquals(Nip55RelayAuthGate.WHITELISTED, gate) assertEquals("relay.example.com", host) } diff --git a/app/src/androidTest/kotlin/io/privkey/keep/storage/SignPolicySelectionPrefsTest.kt b/app/src/androidTest/kotlin/io/privkey/keep/storage/SignPolicySelectionPrefsTest.kt index 7946686d..e0c1ca32 100644 --- a/app/src/androidTest/kotlin/io/privkey/keep/storage/SignPolicySelectionPrefsTest.kt +++ b/app/src/androidTest/kotlin/io/privkey/keep/storage/SignPolicySelectionPrefsTest.kt @@ -5,9 +5,9 @@ import androidx.test.core.app.ApplicationProvider import androidx.test.ext.junit.runners.AndroidJUnit4 import io.privkey.keep.uniffi.SignPolicySelection import io.privkey.keep.uniffi.SignPolicyStore +import io.privkey.keep.uniffi.StorageRead import org.junit.After import org.junit.Assert.assertEquals -import org.junit.Assert.assertNull import org.junit.Before import org.junit.Test import org.junit.runner.RunWith @@ -69,9 +69,9 @@ class SignPolicySelectionPrefsTest { @Test fun migrationNeverResurrectsTheLegacySelectionAfterTheStoreIsLost() { - // The legacy value is kept on disk, and the encrypted-prefs layer reports an - // undecryptable value as absent, so gating the migration on "new value is - // absent" would re-run it and restore the old, possibly looser, selection. + // The legacy value is kept on disk, and a lost store reads as absent, so gating + // the migration on "new value is absent" alone would re-run it and restore the + // old, possibly looser, selection. // The one-shot marker must prevent that: losing the store falls back to // MANUAL (strictest), never back to the legacy AUTO. writeLegacyOrdinal(SignPolicy.AUTO.ordinal) @@ -86,6 +86,26 @@ class SignPolicySelectionPrefsTest { assertEquals(SignPolicySelection.MANUAL, newStore().globalPolicy()) } + /** + * The migration copies only over a value that is not there. One that is stored but + * will not decrypt resolves to MANUAL, and reading it as absent would put the legacy + * selection, here a looser one, in its place. + */ + @Test + fun migrationNeverCopiesOverAValueThatCannotBeRead() { + val storedName = writeAndFindStoredName(context, SELECTION_PREFS) { + SignPolicySelectionPrefs(context).save(GLOBAL_KEY, SignPolicySelection.BASIC.ordinal.toString()) + } + corruptStoredValue(context, SELECTION_PREFS, storedName) + // As on an install that has not run the migration yet. + context.getSharedPreferences(SignPolicySelectionPrefs.MARKER_PREFS_NAME, Context.MODE_PRIVATE) + .edit().remove(SignPolicySelectionPrefs.MIGRATION_MARKER).commit() + writeLegacyOrdinal(SignPolicy.AUTO.ordinal) + + assertEquals(SignPolicySelection.MANUAL, newStore().globalPolicy()) + forgetWriteProbe(context, SELECTION_PREFS) + } + @Test fun migrationLeavesTheLegacyValueInPlace() { writeLegacyOrdinal(SignPolicy.AUTO.ordinal) @@ -102,7 +122,7 @@ class SignPolicySelectionPrefsTest { fun noLegacyValueLeavesTheStoreUnsetAndDefaultsToManual() { val prefs = SignPolicySelectionPrefs(context) - assertNull(prefs.load(GLOBAL_KEY)) + assertEquals(StorageRead.Absent, prefs.load(GLOBAL_KEY)) assertEquals(SignPolicySelection.MANUAL, SignPolicyStore(prefs).globalPolicy()) } @@ -116,7 +136,7 @@ class SignPolicySelectionPrefsTest { @Test fun unreadableStoredValueFallsBackToManual() { // A value of the wrong type is what the pre-migration prefs file held; reading - // it must yield "unset" rather than throwing into the signing path. + // it must report a fault rather than throwing into the signing path. KeystoreEncryptedPrefs.create(context, SELECTION_PREFS) .edit() .putInt(GLOBAL_KEY, SignPolicy.AUTO.ordinal) @@ -124,7 +144,7 @@ class SignPolicySelectionPrefsTest { val prefs = SignPolicySelectionPrefs(context) - assertNull(prefs.load(GLOBAL_KEY)) + assertEquals(StorageRead.Unavailable, prefs.load(GLOBAL_KEY)) assertEquals(SignPolicySelection.MANUAL, SignPolicyStore(prefs).globalPolicy()) } @@ -152,11 +172,53 @@ class SignPolicySelectionPrefsTest { prefs.remove(GLOBAL_KEY) - assertNull(prefs.load(GLOBAL_KEY)) + assertEquals(StorageRead.Absent, prefs.load(GLOBAL_KEY)) assertEquals(SignPolicySelection.MANUAL, SignPolicyStore(prefs).globalPolicy()) } + /** + * An override that is stored but will not decrypt must not fall back to the global + * selection, which can be looser than the override the owner set. Reported as + * absent, it would: the core reads absent as "no override". Reported as + * unavailable, the core resolves it to MANUAL. + * + * Staged on disk, so this is the production backend's answer for a real + * undecryptable entry rather than a fake's. + */ + @Test + fun anUndecryptableOverrideResolvesToManualRatherThanTheGlobal() { + newStore().setGlobalPolicy(SignPolicySelection.AUTO) + val storedName = writeAndFindStoredName(context, SELECTION_PREFS) { + newStore().setAppOverride(PKG, SignPolicySelection.BASIC) + } + corruptStoredValue(context, SELECTION_PREFS, storedName) + + val store = newStore() + assertEquals(SignPolicySelection.MANUAL, store.appOverride(PKG)) + assertEquals(SignPolicySelection.MANUAL, store.effectivePolicy(PKG)) + forgetWriteProbe(context, SELECTION_PREFS) + } + + // The controls: a readable override wins over the global, and a package with no + // override gets the global, so the case above cannot pass on a store that answers + // MANUAL for everything. + @Test + fun aReadableOverrideWinsOverTheGlobal() { + newStore().setGlobalPolicy(SignPolicySelection.AUTO) + newStore().setAppOverride(PKG, SignPolicySelection.BASIC) + + assertEquals(SignPolicySelection.BASIC, newStore().effectivePolicy(PKG)) + } + + @Test + fun aPackageWithNoOverrideGetsTheGlobal() { + newStore().setGlobalPolicy(SignPolicySelection.AUTO) + + assertEquals(SignPolicySelection.AUTO, newStore().effectivePolicy(PKG)) + } + private companion object { + const val PKG = "com.test.app" const val SELECTION_PREFS = "keep_sign_policy_selection" const val LEGACY_PREFS = "keep_sign_policy" const val GLOBAL_KEY = SignPolicySelectionPrefs.GLOBAL_POLICY_KEY diff --git a/app/src/androidTest/kotlin/io/privkey/keep/storage/UnreadableReadGuardTest.kt b/app/src/androidTest/kotlin/io/privkey/keep/storage/UnreadableReadGuardTest.kt index 89798c4f..a7c5def5 100644 --- a/app/src/androidTest/kotlin/io/privkey/keep/storage/UnreadableReadGuardTest.kt +++ b/app/src/androidTest/kotlin/io/privkey/keep/storage/UnreadableReadGuardTest.kt @@ -166,7 +166,7 @@ class UnreadableReadGuardTest { assertEquals("relay.example.com", repaired.add("wss://relay.example.com")) assertEquals( "and the gate works again afterwards", - Nip55RelayAuthGate.AUTO_ACCEPT, + Nip55RelayAuthGate.WHITELISTED, evaluateRelayAuthGate(repaired, EVENT_22242).first ) forgetWriteProbe(context, WHITELIST_PREFS) diff --git a/app/src/main/kotlin/io/privkey/keep/AccountActions.kt b/app/src/main/kotlin/io/privkey/keep/AccountActions.kt index fcd1a920..36d30645 100644 --- a/app/src/main/kotlin/io/privkey/keep/AccountActions.kt +++ b/app/src/main/kotlin/io/privkey/keep/AccountActions.kt @@ -18,6 +18,7 @@ import io.privkey.keep.uniffi.ShareInfo import kotlinx.coroutines.CancellationException import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.NonCancellable import kotlinx.coroutines.launch import kotlinx.coroutines.sync.Mutex import kotlinx.coroutines.sync.withLock @@ -47,6 +48,15 @@ sealed class CreateGroupState { data class Error(val message: String) : CreateGroupState() } +/** + * Whether storing a share replaced the account that was active before it. There is no + * previous account to clean up after when nothing was stored before; otherwise any change + * of the active pointer is one, and a pointer that cannot be read afterwards counts as a + * change rather than as proof that nothing moved. + */ +internal fun storeSwitchedAccount(hadShare: Boolean, activeBefore: String?, activeAfter: String?): Boolean = + hadShare && (activeAfter == null || activeAfter != activeBefore) + internal class AccountActions( private val keepMobile: KeepMobile, private val storage: AndroidKeystoreStorage, @@ -55,6 +65,7 @@ internal class AccountActions( private val onBiometricRequest: (String, String, Cipher, (Cipher?) -> Unit) -> Unit, private val onAccountSwitched: suspend () -> Unit, private val onAccountDeleted: suspend () -> Unit, + private val onLiveNodeDropped: () -> Unit, private val onStateChanged: (AccountState) -> Unit ) { data class AccountState( @@ -102,12 +113,46 @@ internal class AccountActions( } finally { storage.clearRequestIdContext() } + onLiveNodeDropped() } } finally { storage.clearPendingCipher(switchId) } } + /** + * 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 + * 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 + * reports the core's node drop. + */ + 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() + } + var stored = false + try { + return withContext(Dispatchers.IO) { store().also { stored = true } } + } finally { + withContext(NonCancellable) { + // Here rather than at the caller: a cancellation that lands once the store + // has returned would skip the caller's code, and the core has already + // dropped the node. + if (stored) onLiveNodeDropped() + val (hadShare, activeBefore) = before + val activeAfter = withContext(Dispatchers.IO) { + runCatching { storage.getActiveShareKey() }.getOrNull() + } + if (storeSwitchedAccount(hadShare, activeBefore, activeAfter)) onAccountSwitched() + } + } + } + private suspend fun refreshAccountState() { val result = withContext(Dispatchers.IO) { val hasShare = keepMobile.hasShare() @@ -202,10 +247,24 @@ internal class AccountActions( ) { accountMutex.withLock { val activeAccountKey = withContext(Dispatchers.IO) { storage.getActiveShareKey() } - val wasActive = account.groupPubkeyHex == activeAccountKey + // No pointer is treated as this account: the core builds its node from a + // 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 + // answered, which it may never be if the activity goes away first. + onAccountSwitched() + } try { withContext(Dispatchers.IO) { - keepMobile.deleteShareByKey(account.groupPubkeyHex) + // The core drops its node whatever the delete returns. + try { + keepMobile.deleteShareByKey(account.groupPubkeyHex) + } finally { + onLiveNodeDropped() + } runCatching { keepMobile.deleteRelayConfig(account.groupPubkeyHex) } .onFailure { if (BuildConfig.DEBUG) Log.e("AccountActions", "Relay config cleanup failed: ${it::class.simpleName}") } // Audit the deletion in the current chain. Deleting the active @@ -215,6 +274,11 @@ internal class AccountActions( } } catch (e: Exception) { logAndToast("Delete failed", appContext.getString(R.string.account_delete_failed), e) + // A failed delete may still have removed the share's data, and for the + // active account it cleared the active pointer first, so the screen has + // to be re-read rather than left showing the account as it was. + runCatching { refreshAccountState() } + .onFailure { if (BuildConfig.DEBUG) Log.e("AccountActions", "Refresh after failed delete failed: ${it::class.simpleName}") } onDismiss() return@withLock } @@ -525,7 +589,7 @@ internal class AccountActions( try { storage.setPendingCipher(requestId, cipher, timeoutMs = DKG_CIPHER_TIMEOUT_MS) pendingSet = true - val result = withContext(Dispatchers.IO) { + val result = storingNewActiveShare { storage.setRequestIdContext(requestId) try { keepMobile.frostRunDkg(config, name, passphrase, DKG_ROUND_TIMEOUT_SECS.toULong(), callback) @@ -686,7 +750,7 @@ internal class AccountActions( storage.setPendingCipher(requestId, authedDecrypt, AndroidKeystoreStorage.CipherRole.DECRYPT) pendingSet = true storage.setPendingCipher(requestId, authedEncrypt, AndroidKeystoreStorage.CipherRole.ENCRYPT) - val info = withContext(Dispatchers.IO) { + val info = storingNewActiveShare { storage.setRequestIdContext(requestId) try { // Vault-protected: the passphrase is stashed in the secret. @@ -880,7 +944,7 @@ internal class AccountActions( try { storage.setPendingCipher(importId, cipher) pendingSet = true - val result = withContext(Dispatchers.IO) { + val result = storingNewActiveShare { storage.setRequestIdContext(importId) try { apiCall() diff --git a/app/src/main/kotlin/io/privkey/keep/KeepMobileApp.kt b/app/src/main/kotlin/io/privkey/keep/KeepMobileApp.kt index 38abd977..e306ef06 100644 --- a/app/src/main/kotlin/io/privkey/keep/KeepMobileApp.kt +++ b/app/src/main/kotlin/io/privkey/keep/KeepMobileApp.kt @@ -101,6 +101,7 @@ class KeepMobileApp : Application() { var liveState: KeepLiveState? by mutableStateOf(null) private set + private val liveNode = LiveNodeTracker() @Volatile private var pinMismatch: PinMismatchInfo? = null @@ -412,7 +413,7 @@ class KeepMobileApp : Application() { val mobile = keepMobile ?: throw IllegalStateException("KeepMobile not initialized") initMutex.withLock { - if (liveState != null && mobile.getShareInfo() != null) return + if (isNodeLive() && mobile.getShareInfo() != null) return val relays = getActiveRelays().ifEmpty { listOf("wss://relay.damus.io", "wss://nos.lol", "wss://relay.primal.net") @@ -475,6 +476,7 @@ class KeepMobileApp : Application() { } private suspend fun initializeConnection(mobile: KeepMobile, relays: List) { + val startedAt = liveNode.beginInit() val proxyConfig = runCatching { mobile.getProxyConfig() }.getOrNull() if (BuildConfig.DEBUG) { Log.d(TAG, "Initializing with ${relays.size} relay(s), proxy=${proxyConfig?.enabled == true}") @@ -484,6 +486,7 @@ class KeepMobileApp : Application() { } else { mobile.initialize(relays) } + liveNode.built(startedAt) if (BuildConfig.DEBUG) Log.d(TAG, "Initialize completed, peers: ${mobile.getPeers().size}") } @@ -639,21 +642,46 @@ class KeepMobileApp : Application() { manager.register() } + /** Whether signing can use the node as it stands, rather than having to build one. */ + fun isNodeLive(): Boolean = liveState != null && liveNode.isLive() + + /** + * For the operations after which the core has dropped its live node; see + * [LiveNodeTracker]. No reconnect is attempted from here: building a node loads the + * share, which needs an authenticated cipher this has no way to get, so the next + * request that needs the node prompts for one and builds it. + */ + fun onLiveNodeDropped() { + liveNode.dropped() + mainHandler.post { + // Not over a node an initialize has built since this drop. + if (!liveNode.isLive()) { + liveState = null + // The drop discards the node's pending requests without a state callback. + io.privkey.keep.service.CosignNotifier.update(applicationContext, emptyList()) + } + } + } + fun clearCertificatePin(hostname: String) { runCatching { keepMobile?.clearCertificatePin(hostname) } + .onSuccess { if (it != null) onLiveNodeDropped() } .onFailure { if (BuildConfig.DEBUG) Log.e(TAG, "Failed to clearCertificatePin: ${it::class.simpleName}") } } fun clearAllCertificatePins() { runCatching { keepMobile?.clearCertificatePins() } + .onSuccess { if (it != null) onLiveNodeDropped() } .onFailure { if (BuildConfig.DEBUG) Log.e(TAG, "Failed to clearCertificatePins: ${it::class.simpleName}") } } fun stageCertificatePin(hostname: String, spkiHash: String): Boolean = - keepMobile?.stageCertificatePinCompat(hostname, spkiHash) ?: false + (keepMobile?.stageCertificatePinCompat(hostname, spkiHash) ?: false) + .also { if (it) onLiveNodeDropped() } fun removeCertificatePin(hostname: String, spkiHash: String): CertPinRemoval? = keepMobile?.removeCertificatePinCompat(hostname, spkiHash) + ?.also { onLiveNodeDropped() } fun dismissPinMismatch() { pinMismatch = null diff --git a/app/src/main/kotlin/io/privkey/keep/LiveNodeTracker.kt b/app/src/main/kotlin/io/privkey/keep/LiveNodeTracker.kt new file mode 100644 index 00000000..54a38274 --- /dev/null +++ b/app/src/main/kotlin/io/privkey/keep/LiveNodeTracker.kt @@ -0,0 +1,39 @@ +package io.privkey.keep + +import java.util.concurrent.atomic.AtomicLong + +/** + * Whether the core's live node is still the one the app last initialized. + * + * The core drops its node on an account switch, a share delete, an import or DKG that + * stores a share, and a certificate pin edit, and reports none of them through the state + * callback. A callback the dropped node had already queued can also land after the drop. + * So the last state delivered cannot say whether a node is up. This counts drops + * instead: a node is live only if no drop has happened since the initialize that built + * it began. An initialize that a drop overlapped therefore reads as not live, which costs + * a re-initialize rather than skipping one. + */ +internal class LiveNodeTracker { + + private val drops = AtomicLong() + + @Volatile + private var builtAt = NEVER + + fun dropped() { + drops.incrementAndGet() + } + + /** Taken before an initialize starts, and handed to [built] once it succeeds. */ + fun beginInit(): Long = drops.get() + + fun built(startedAt: Long) { + builtAt = startedAt + } + + fun isLive(): Boolean = builtAt == drops.get() + + private companion object { + const val NEVER = -1L + } +} diff --git a/app/src/main/kotlin/io/privkey/keep/MainActivity.kt b/app/src/main/kotlin/io/privkey/keep/MainActivity.kt index 111a7a46..518e0bed 100644 --- a/app/src/main/kotlin/io/privkey/keep/MainActivity.kt +++ b/app/src/main/kotlin/io/privkey/keep/MainActivity.kt @@ -547,6 +547,7 @@ fun MainScreen( // best-effort audit; a failed append must not block deletion } }, + onLiveNodeDropped = { (appContext.applicationContext as? KeepMobileApp)?.onLiveNodeDropped() }, onStateChanged = { state -> hasShare = state.hasShare shareInfo = state.shareInfo 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 4754916a..49616f94 100644 --- a/app/src/main/kotlin/io/privkey/keep/nip46/BunkerService.kt +++ b/app/src/main/kotlin/io/privkey/keep/nip46/BunkerService.kt @@ -72,7 +72,11 @@ class BunkerService : Service() { private const val APPROVAL_CHANNEL_ID = "keep_bunker_approvals" private const val APPROVAL_NOTIFICATION_ID_BASE = 0x4E46 - private const val APPROVAL_TIMEOUT_MS = 60_000L + // Kept clearly inside the core's own 60 second backstop on this same wait. With + // equal deadlines an approval landing at the boundary would be shown to the user + // as given while the core had already denied it, so this side has to be the one + // that decides. + internal const val APPROVAL_TIMEOUT_MS = 55_000L private const val MAX_TRACKED_CLIENTS = 1000 internal const val MAX_AUTHORIZED_CLIENTS = 100 @@ -430,7 +434,14 @@ class BunkerService : Service() { // connect is not persisted as an authorized client. Authorization is driven // by an explicit assertion rather than the callback merely having fired. if (shouldPersistConnectAuthorization(authorization)) { - authorizeClient(pubkey, safeName, safeRelays) + // A connect the user approved at a prompt lifts an earlier revocation; + // a matched secret alone does not. + authorizeClient( + pubkey, + safeName, + safeRelays, + clearDenylist = authorization == ConnectAuthorization.USER_APPROVED + ) } else { if (BuildConfig.DEBUG) Log.w(TAG, "Bunker: not persisting auto-approved connect ${pubkey.take(8)}") logActivity(EventLogCategory.BUNKER, EventLogLevel.WARN, safeName.ifBlank { pubkey.take(8) }, "connect not persisted (auto-approved)") @@ -634,7 +645,6 @@ class BunkerService : Service() { return REJECTED } - val mobile = keepMobileRef val pk = clientPubkey.lowercase() val denylisted = runCatching { Nip46ClientStore.isDenylisted(this, pk) }.getOrDefault(true) val isAuthorized = isClientAuthorized( @@ -688,13 +698,10 @@ class BunkerService : Service() { return REJECTED } - val result = resultRef.get() ?: REJECTED - if (result.approved && isConnectRequest && mobile != null) { - authorizeClient(clientPubkey, clearDenylist = true) - if (BuildConfig.DEBUG) Log.d(TAG, "Authorized new client: ${truncatePubkey(clientPubkey)}") - } - - return result + // A connect approval is not persisted here: the core can still refuse the connect + // after this returns, at its own deadline or on its own checks. onConnect reports + // the connects it accepted, and persists them. + return resultRef.get() ?: REJECTED } private fun startApprovalActivity(requestId: String, request: BunkerApprovalRequest, isConnectRequest: Boolean) { diff --git a/app/src/main/kotlin/io/privkey/keep/nip55/AppSignPolicyOverrides.kt b/app/src/main/kotlin/io/privkey/keep/nip55/AppSignPolicyOverrides.kt index 2694d8ed..650b7c64 100644 --- a/app/src/main/kotlin/io/privkey/keep/nip55/AppSignPolicyOverrides.kt +++ b/app/src/main/kotlin/io/privkey/keep/nip55/AppSignPolicyOverrides.kt @@ -84,9 +84,7 @@ object AppSignPolicyOverrides { // Neither side knows of one: the app is not pinned, so the global applies. if (fromRow == null && fromCore == null) return@runCatching null // Exactly one side knows of one, so a tier exists but which one is unknown: the - // row has not been migrated, or a write reached one store and not the other, or a - // read faulted (the prefs layer returns the default when a value cannot be - // decrypted, so unreadable is indistinguishable from absent). Resolve the + // row has not been migrated, or a write reached one store and not the other. Resolve the // strictest tier rather than the global, which may be looser than what the user // chose, and rather than the one side that did answer, which may be the stale one. if (fromRow == null || fromCore == null) return@runCatching SignPolicySelection.MANUAL @@ -196,14 +194,13 @@ object AppSignPolicyOverrides { * Copies the row values into the core at startup, which is what moves a pinned app * from resolving Manual to resolving its chosen tier. * - * Gated on persisted markers in [markers] rather than on the core looking empty for a - * package. The encrypted-prefs layer returns the default when a value cannot be - * decrypted, so an unreadable tier is indistinguishable from an absent one, and a - * second run that read it as absent would copy the row's tier over a stricter one the - * core durably holds: `setOverride` pins Manual in the core when its write is - * unconfirmed while the row keeps the tier the user asked for, so core-stricter-than- - * row is a state this code produces deliberately. The absence check stays as the - * second line, but it is not what bounds this. + * Gated on persisted markers in [markers] rather than only on the core looking empty + * for a package. The core reports an unreadable tier as Manual rather than absent, so + * the absence check holds against that fault. The markers are still what bounds this, + * because a copy whose outcome is unknown must not be retried: `setOverride` pins + * Manual in the core when its write is unconfirmed while the row keeps the tier the + * user asked for, so core-stricter-than-row is a state this code produces + * deliberately, and a second copy would undo it. * * Each package's marker is written before its copy is attempted, and a package whose * marker cannot be persisted is skipped rather than copied unrecorded: an attempt that @@ -220,12 +217,6 @@ object AppSignPolicyOverrides { * killed between the marker commit and the write, with no fault involved at all. A * two-phase in-progress marker would not buy anything, because in-progress is precisely * the unknown outcome that must not be retried. - * - * One widening window is left, and the markers do not close it: for a package no run - * has attempted yet, a core that reads as absent because its value would not decrypt - * gets the row's tier copied over whatever it durably holds, which an unconfirmed - * `setOverride` write may have left stricter. Closing that needs a store that can tell - * an absent value from an unreadable one. */ suspend fun migrateLegacyOverrides( core: SignPolicyStore, diff --git a/app/src/main/kotlin/io/privkey/keep/nip55/Nip55Activity.kt b/app/src/main/kotlin/io/privkey/keep/nip55/Nip55Activity.kt index 30e740f5..39765cfc 100644 --- a/app/src/main/kotlin/io/privkey/keep/nip55/Nip55Activity.kt +++ b/app/src/main/kotlin/io/privkey/keep/nip55/Nip55Activity.kt @@ -784,7 +784,7 @@ class Nip55Activity : FragmentActivity() { val mobile = app.getKeepMobile() ?: return "No key is stored in Keep" if (!mobile.hasShare()) return "No key is stored in Keep" - if (app.liveState != null) return null + if (app.isNodeLive()) return null BiometricHelper.requireBiometricReady(this, biometricHelper.checkBiometricStatus()) diff --git a/app/src/main/kotlin/io/privkey/keep/nip55/PermissionDaos.kt b/app/src/main/kotlin/io/privkey/keep/nip55/PermissionDaos.kt index 519d82a8..9d9db073 100644 --- a/app/src/main/kotlin/io/privkey/keep/nip55/PermissionDaos.kt +++ b/app/src/main/kotlin/io/privkey/keep/nip55/PermissionDaos.kt @@ -33,9 +33,8 @@ interface Nip55PermissionDao { // Everything the app's expiry window granted, but neither a refusal nor an explicit // "always ask": both are standing instructions about the app rather than part of the // time-boxed grant, both are written only by the user's own toggle with no expiry of - // their own, and at keep v0.11.0 an ASK row blocks the policy auto-approve and the - // whitelisted-relay auto-accept, so deleting it would hand back an auto-approve the - // user had switched off. LOWER() to match the core's to_ascii_lowercase rather than + // their own, and an ASK row blocks the policy auto-approve, so deleting it would hand + // back an auto-approve the user had switched off. LOWER() to match the core's to_ascii_lowercase rather than // because the column holds mixed case; every writer stores lowercase, since // PermissionDecision.toString() is overridden to name.lowercase(). @Query("DELETE FROM nip55_permissions WHERE callerPackage = :callerPackage AND LOWER(decision) NOT IN ('deny', 'ask')") diff --git a/app/src/main/kotlin/io/privkey/keep/storage/RelayAuthWhitelistStore.kt b/app/src/main/kotlin/io/privkey/keep/storage/RelayAuthWhitelistStore.kt index 1d5d17d9..2ec174d4 100644 --- a/app/src/main/kotlin/io/privkey/keep/storage/RelayAuthWhitelistStore.kt +++ b/app/src/main/kotlin/io/privkey/keep/storage/RelayAuthWhitelistStore.kt @@ -5,9 +5,10 @@ import io.privkey.keep.uniffi.nip55NormalizeRelayHost /** * Global allowlist of relay hosts for NIP-42 (kind 22242) authentication. When the - * list is non-empty it acts as a hard gate: a 22242 request whose relay is listed is - * auto-accepted, one whose relay is not listed is auto-rejected, and an empty list - * defers to normal per-app grant resolution. Entries are stored normalized (via the + * list is non-empty it acts as a hard gate: a 22242 request whose relay is not listed + * is auto-rejected, one whose relay is listed goes on to normal per-app grant + * resolution, and so does every request when the list is empty. Listing a relay never + * authorizes signing on its own. Entries are stored normalized (via the * Rust `nip55_normalize_relay_host`) so they compare consistently with request relays. */ class RelayAuthWhitelistStore(context: Context) { diff --git a/app/src/main/kotlin/io/privkey/keep/storage/SignPolicySelectionPrefs.kt b/app/src/main/kotlin/io/privkey/keep/storage/SignPolicySelectionPrefs.kt index be44e3e4..9a75e4c9 100644 --- a/app/src/main/kotlin/io/privkey/keep/storage/SignPolicySelectionPrefs.kt +++ b/app/src/main/kotlin/io/privkey/keep/storage/SignPolicySelectionPrefs.kt @@ -6,6 +6,7 @@ import android.util.Base64 import android.util.Log import io.privkey.keep.BuildConfig import io.privkey.keep.uniffi.SignPolicySelectionStorage +import io.privkey.keep.uniffi.StorageRead import java.security.MessageDigest private const val TAG = "SignPolicySelectionPrefs" @@ -15,9 +16,11 @@ private const val TAG = "SignPolicySelectionPrefs" * The core owns the selection state and its precedence; Android only persists the * opaque key/value pairs. * - * Every operation fails safe: a read error surfaces as "unset", which the core - * resolves to `Manual`, the strictest tier. A storage fault must never propagate - * into the signing path. + * Every operation fails safe. A value that is there but cannot be read reports + * [StorageRead.Unavailable], never [StorageRead.Absent]: an absent per-app override + * falls back to the global selection, which can be looser than the override the + * owner set, while an unavailable one resolves to `Manual`, the strictest tier. A + * storage fault must never propagate into the signing path. * * Uses its own prefs file. The legacy Kotlin store wrote `global_sign_policy` as an * Int into "keep_sign_policy", and the core reads that key as a String, so sharing @@ -32,10 +35,17 @@ class SignPolicySelectionPrefs(context: Context) : SignPolicySelectionStorage { migrateLegacyGlobalPolicy(context) } - override fun load(key: String): String? = - runCatching { prefs.getString(key, null) } - .onFailure { warn("load failed for $key", it) } - .getOrNull() + override fun load(key: String): StorageRead = + runCatching { + when (val read = prefs.readString(key)) { + is PrefRead.Found -> StorageRead.Found(read.value) + PrefRead.Absent -> StorageRead.Absent + PrefRead.Unreadable -> StorageRead.Unavailable + } + }.getOrElse { + warn("load failed for $key", it) + StorageRead.Unavailable + } // Both writes return the platform's own durable-write result. Reporting // whether the call threw would say nothing, since a commit can return false @@ -59,11 +69,10 @@ class SignPolicySelectionPrefs(context: Context) : SignPolicySelectionStorage { * One-time copy of the global selection the deleted Kotlin store wrote as an Int. * The legacy key is left in place, so this stays non-destructive. * - * Gated on a persisted marker rather than on "is the new value absent". The - * encrypted-prefs layer returns the default when a value cannot be decrypted, so - * an unreadable store is indistinguishable from an unset one; gating on absence - * would re-run this after any such failure and resurrect the legacy selection, - * which may be LOOSER than what the user has chosen since. The marker is set + * Gated on a persisted marker rather than only on "is the new value absent". A store + * that was lost reads as absent, so gating on absence alone would re-run this after + * such a loss and resurrect the legacy selection, which may be LOOSER than what the + * user has chosen since. The marker is set * whether or not the copy succeeds, so this runs at most once: a failed copy * leaves the store unset, which the core resolves to `Manual`, the strictest * tier. Losing a selection that way costs the user a re-pick; resurrecting a @@ -74,7 +83,9 @@ class SignPolicySelectionPrefs(context: Context) : SignPolicySelectionStorage { if (markerPrefs.getBoolean(MIGRATION_MARKER, false)) return runCatching { - if (prefs.getString(GLOBAL_POLICY_KEY, null) == null) { + // Only over a value that is not there. One that is there but unreadable resolves + // to Manual, and copying the legacy selection over it could widen it. + if (prefs.readString(GLOBAL_POLICY_KEY) == PrefRead.Absent) { val legacyPrefs = LegacyPrefsMigration.migrateIfNeeded( context, LEGACY_PREFS_NAME, diff --git a/app/src/main/res/values/strings_connections.xml b/app/src/main/res/values/strings_connections.xml index cc4c30a4..d14e620a 100644 --- a/app/src/main/res/values/strings_connections.xml +++ b/app/src/main/res/values/strings_connections.xml @@ -256,7 +256,7 @@ All relays Relay Auth Whitelist Relay Auth Whitelist - When set, NIP-42 relay authentication (kind 22242) from relays not on the list is automatically rejected, both in the foreground and in background (Content Resolver) requests. Listed relays are auto-approved only for background requests. Leave empty to decide per app. + When set, NIP-42 relay authentication (kind 22242) from relays not on the list is automatically rejected, both in the foreground and in background (Content Resolver) requests. Listing a relay does not approve it: a listed relay still needs the app\'s permission or your approval. Leave empty to decide per app. Relay URL wss://relay.example.com Add diff --git a/app/src/test/kotlin/io/privkey/keep/LiveNodeTrackerTest.kt b/app/src/test/kotlin/io/privkey/keep/LiveNodeTrackerTest.kt new file mode 100644 index 00000000..6b124896 --- /dev/null +++ b/app/src/test/kotlin/io/privkey/keep/LiveNodeTrackerTest.kt @@ -0,0 +1,48 @@ +package io.privkey.keep + +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test + +class LiveNodeTrackerTest { + + @Test + fun nothingIsLiveBeforeTheFirstInitialize() { + assertFalse(LiveNodeTracker().isLive()) + } + + @Test + fun anInitializeThatSucceededIsLive() { + val tracker = LiveNodeTracker() + tracker.built(tracker.beginInit()) + assertTrue(tracker.isLive()) + } + + @Test + fun aDropEndsIt() { + val tracker = LiveNodeTracker() + tracker.built(tracker.beginInit()) + tracker.dropped() + assertFalse(tracker.isLive()) + } + + @Test + fun anInitializeAfterTheDropIsLiveAgain() { + val tracker = LiveNodeTracker() + tracker.built(tracker.beginInit()) + tracker.dropped() + tracker.built(tracker.beginInit()) + assertTrue(tracker.isLive()) + } + + // The case a flag cleared at the end of an initialize would get wrong: the drop lands + // while the initialize is running, so the node it reports may be the dropped one. + @Test + fun anInitializeADropOverlappedIsNotLive() { + val tracker = LiveNodeTracker() + val startedAt = tracker.beginInit() + tracker.dropped() + tracker.built(startedAt) + assertFalse(tracker.isLive()) + } +} diff --git a/app/src/test/kotlin/io/privkey/keep/StoreSwitchedAccountTest.kt b/app/src/test/kotlin/io/privkey/keep/StoreSwitchedAccountTest.kt new file mode 100644 index 00000000..99dfbfe0 --- /dev/null +++ b/app/src/test/kotlin/io/privkey/keep/StoreSwitchedAccountTest.kt @@ -0,0 +1,43 @@ +package io.privkey.keep + +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * When an import or DKG has to be cleaned up after as an account switch. Missing one + * leaves the previous account's grants applying to the new key; the cost of a spurious + * one is only that those grants are revoked. + */ +class StoreSwitchedAccountTest { + + @Test + fun replacingTheActiveAccountIsASwitch() { + assertTrue(storeSwitchedAccount(hadShare = true, activeBefore = A, activeAfter = B)) + } + + @Test + fun aStoreThatLeftTheActiveAccountAloneIsNot() { + assertFalse(storeSwitchedAccount(hadShare = true, activeBefore = A, activeAfter = A)) + } + + @Test + fun theFirstAccountHasNothingToCleanUpAfter() { + assertFalse(storeSwitchedAccount(hadShare = false, activeBefore = null, activeAfter = B)) + } + + @Test + fun aPointerThatCannotBeReadAfterwardsCountsAsAChange() { + assertTrue(storeSwitchedAccount(hadShare = true, activeBefore = A, activeAfter = null)) + } + + @Test + fun anAccountActiveWithoutAPointerIsReplacedByOneThatHasOne() { + assertTrue(storeSwitchedAccount(hadShare = true, activeBefore = null, activeAfter = B)) + } + + private companion object { + const val A = "aa" + const val B = "bb" + } +} diff --git a/app/src/test/kotlin/io/privkey/keep/nip46/ApprovalTimeoutTest.kt b/app/src/test/kotlin/io/privkey/keep/nip46/ApprovalTimeoutTest.kt new file mode 100644 index 00000000..32e252b5 --- /dev/null +++ b/app/src/test/kotlin/io/privkey/keep/nip46/ApprovalTimeoutTest.kt @@ -0,0 +1,26 @@ +package io.privkey.keep.nip46 + +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * 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. + */ +class ApprovalTimeoutTest { + + @Test + fun theWaitEndsWellBeforeTheCoreBackstop() { + assertTrue( + "the margin is what lets this side decide first", + BunkerService.APPROVAL_TIMEOUT_MS <= CORE_APPROVAL_TIMEOUT_MS - 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/keep.version b/keep.version index 294b38c5..a32d51bb 100644 --- a/keep.version +++ b/keep.version @@ -1 +1 @@ -7de93595fc2090d0aa1a936ed87aed4b184c5439 \ No newline at end of file +43841928a2e9e60456de75894aa1105577184ca5