mirror of
https://github.com/greenart7c3/Amber.git
synced 2026-10-05 19:08:23 +00:00
Fix Applications screen slowness after envelope-encrypting secrets
The GHSA-5fjp-ghh8-wch8 change made every ApplicationDao read decrypt the `secret`/`localKey` columns, and each decryption re-fetched the AMBER_AES_KEY Keystore key: KeyStore.getInstance().load(null) + containsAlias() + getEntry(), three sequential binder IPCs to the keystore daemon, per call. The Applications screen paid 2 x rows of those per page (Paging's initial load is 3 x pageSize rows), fully serialized, plus the TEE cipher operation itself — several seconds on a populated account. Two independent fixes: - SecureCryptoHelper: cache the SecretKey handle (the material never leaves the TEE/StrongBox; the object is a lightweight reference). Fast path is a volatile read; miss path double-checks under a synchronized block. All four public entry points run through withFreshKeyRetry, which retries exactly once with a re-fetched handle on stale-handle errors (InvalidKeyException, KeyStoreException, UnrecoverableKeyException, ProviderException) so concurrent key rotation or the opt-in unlocked-device policy cannot wedge reads. GCM AEADBadTagException (corrupt/wrong-key ciphertext) still propagates immediately, preserving the decryptField failure contract. rotateKey updates the cache in both of its branches. - ApplicationDao: the Applications list now queries a dedicated projection (ApplicationListItem: key, name, relays, icon, lastUsed) that never selects the encrypted columns, so the screen performs zero Keystore operations regardless of row count. The screen renders neither secret nor localKey, so the SELECT * decrypts were pure overhead. DecryptingPagingSource is removed with its only caller. The Pager is also remember(account.hexKey)-ed: it was previously rebuilt on every recomposition (killSwitch / backup-warning state flips), re-running the initial load each time. Also adapts ApplicationsScreen to the NavHostControllerWrapper stability pattern used by the other screens. Verified: ktlintCheck, lint, test (all variants), assembleFreeDebug, assembleOfflineDebug.
This commit is contained in:
@@ -7,7 +7,11 @@ import android.security.keystore.KeyGenParameterSpec
|
||||
import android.security.keystore.KeyProperties
|
||||
import android.util.Base64
|
||||
import java.nio.ByteBuffer
|
||||
import java.security.InvalidKeyException
|
||||
import java.security.KeyStore
|
||||
import java.security.KeyStoreException
|
||||
import java.security.ProviderException
|
||||
import java.security.UnrecoverableKeyException
|
||||
import javax.crypto.Cipher
|
||||
import javax.crypto.KeyGenerator
|
||||
import javax.crypto.SecretKey
|
||||
@@ -24,12 +28,26 @@ object SecureCryptoHelper {
|
||||
private const val TAG_SIZE = 128 // bits
|
||||
private val mutex = Mutex()
|
||||
|
||||
/**
|
||||
* Cached handle to the [KEY_ALIAS] key. An AndroidKeyStore [SecretKey] is
|
||||
* a lightweight reference (the material never leaves the TEE/StrongBox),
|
||||
* but fetching it costs a fresh KeyStore load plus `containsAlias`/
|
||||
* `getEntry` binder round-trips to the keystore daemon — milliseconds
|
||||
* each, sequential. Re-fetching it per cipher operation made every
|
||||
* envelope-encrypted DAO read pay a keystore lookup per field
|
||||
* (see ApplicationEntityCrypto.kt), which dominated load times.
|
||||
* Invalidated by [rotateKey] and by [withFreshKeyRetry] when a
|
||||
* cached handle is found unusable.
|
||||
*/
|
||||
@Volatile
|
||||
private var cachedSecretKey: SecretKey? = null
|
||||
|
||||
suspend fun encrypt(plainText: String): String = mutex.withLock {
|
||||
encryptWithKey(getOrCreateSecretKey(), plainText)
|
||||
withFreshKeyRetry { key -> encryptWithKey(key, plainText) }
|
||||
}
|
||||
|
||||
suspend fun decrypt(encryptedText: String): String = mutex.withLock {
|
||||
decryptWithKey(getOrCreateSecretKey(), encryptedText)
|
||||
withFreshKeyRetry { key -> decryptWithKey(key, encryptedText) }
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -41,7 +59,7 @@ object SecureCryptoHelper {
|
||||
* here so blocking callers do not need a [kotlinx.coroutines.runBlocking]
|
||||
* bridge.
|
||||
*/
|
||||
fun encryptBlocking(plainText: String): String = encryptWithKey(getOrCreateSecretKey(), plainText)
|
||||
fun encryptBlocking(plainText: String): String = withFreshKeyRetry { key -> encryptWithKey(key, plainText) }
|
||||
|
||||
private fun encryptWithKey(key: SecretKey, plainText: String): String {
|
||||
val cipher = Cipher.getInstance(TRANSFORMATION)
|
||||
@@ -61,7 +79,7 @@ object SecureCryptoHelper {
|
||||
* Non-suspending equivalent of [decrypt]. See [encryptBlocking] for the
|
||||
* rationale.
|
||||
*/
|
||||
fun decryptBlocking(encryptedText: String): String = decryptWithKey(getOrCreateSecretKey(), encryptedText)
|
||||
fun decryptBlocking(encryptedText: String): String = withFreshKeyRetry { key -> decryptWithKey(key, encryptedText) }
|
||||
|
||||
private fun decryptWithKey(key: SecretKey, encryptedText: String): String {
|
||||
val data = Base64.decode(encryptedText, Base64.NO_WRAP)
|
||||
@@ -78,7 +96,43 @@ object SecureCryptoHelper {
|
||||
return String(plainBytes, Charsets.UTF_8)
|
||||
}
|
||||
|
||||
/**
|
||||
* Runs [block] with the current key handle, retrying exactly once with a
|
||||
* re-fetched handle when the cached one has gone stale (the entry was
|
||||
* deleted or invalidated underneath us — e.g. a concurrent [rotateKey],
|
||||
* or the opt-in unlocked-device policy refusing key use while locked).
|
||||
* Data errors — chiefly GCM's AEADBadTagException for corrupt or
|
||||
* wrong-key ciphertext — are not stale-handle errors and propagate to the
|
||||
* caller on the first attempt, preserving the decryptField failure
|
||||
* contract in ApplicationEntityCrypto.kt.
|
||||
*/
|
||||
private inline fun <T> withFreshKeyRetry(block: (SecretKey) -> T): T {
|
||||
try {
|
||||
return block(getOrCreateSecretKey())
|
||||
} catch (e: InvalidKeyException) {
|
||||
AmberLog.w(TAG, "Cached Keystore key unusable, re-fetching once", e)
|
||||
} catch (e: KeyStoreException) {
|
||||
AmberLog.w(TAG, "Keystore failure with cached key, re-fetching once", e)
|
||||
} catch (e: UnrecoverableKeyException) {
|
||||
AmberLog.w(TAG, "Cached Keystore key unrecoverable, re-fetching once", e)
|
||||
} catch (e: ProviderException) {
|
||||
AmberLog.w(TAG, "Keystore provider failure, re-fetching key once", e)
|
||||
}
|
||||
cachedSecretKey = null
|
||||
return block(getOrCreateSecretKey())
|
||||
}
|
||||
|
||||
private fun getOrCreateSecretKey(): SecretKey {
|
||||
cachedSecretKey?.let { return it }
|
||||
return synchronized(this) {
|
||||
cachedSecretKey?.let { return it }
|
||||
val key = loadOrCreateSecretKey()
|
||||
cachedSecretKey = key
|
||||
key
|
||||
}
|
||||
}
|
||||
|
||||
private fun loadOrCreateSecretKey(): SecretKey {
|
||||
val keyStore = KeyStore.getInstance(ANDROID_KEYSTORE).apply { load(null) }
|
||||
if (keyStore.containsAlias(KEY_ALIAS)) {
|
||||
val entry = keyStore.getEntry(KEY_ALIAS, null) as? KeyStore.SecretKeyEntry
|
||||
@@ -144,12 +198,15 @@ object SecureCryptoHelper {
|
||||
*
|
||||
* Uses internal non-locking cipher methods ([encryptWithKey]/
|
||||
* [decryptWithKey]) and raw DataStore/DAO helpers to avoid mutex
|
||||
* reentrancy (Kotlin's [Mutex] is not reentrant).
|
||||
* reentrancy (Kotlin's [Mutex] is not reentrant). The cached-key path
|
||||
* ([getOrCreateSecretKey]/[withFreshKeyRetry]) is deliberately not used
|
||||
* here because this function manages the cache itself and must address
|
||||
* the old/new keys explicitly.
|
||||
*/
|
||||
suspend fun rotateKey(context: Context, requireUnlockedDevice: Boolean) = mutex.withLock {
|
||||
val keyStore = getKeyStore()
|
||||
if (!keyStore.containsAlias(KEY_ALIAS)) {
|
||||
generateSecretKey(requireUnlockedDevice)
|
||||
cachedSecretKey = generateSecretKey(requireUnlockedDevice)
|
||||
return@withLock
|
||||
}
|
||||
|
||||
@@ -224,8 +281,11 @@ object SecureCryptoHelper {
|
||||
}
|
||||
|
||||
// 2. Delete the old key and generate a new one with the new policy.
|
||||
// Update the handle cache in the same critical section so concurrent
|
||||
// blocking callers never observe the deleted old key.
|
||||
keyStore.deleteEntry(KEY_ALIAS)
|
||||
val newKey = generateSecretKey(requireUnlockedDevice)
|
||||
cachedSecretKey = newKey
|
||||
|
||||
// 3. Re-encrypt and store all secrets with the new key.
|
||||
for ((npub, keys) in decryptedAccountKeys) {
|
||||
|
||||
@@ -54,14 +54,20 @@ interface ApplicationDao {
|
||||
|
||||
suspend fun getAllNotConnected(): List<ApplicationWithPermissions> = getAllNotConnectedRaw().map { it.decryptFromStorage() }
|
||||
|
||||
@Query("SELECT a.* FROM application a WHERE a.pubKey = :pubKey ORDER BY a.lastUsed DESC")
|
||||
fun getAllPagingRaw(pubKey: String): PagingSource<Int, ApplicationEntity>
|
||||
|
||||
fun getAllPaging(pubKey: String): PagingSource<Int, ApplicationEntity> = DecryptingPagingSource(getAllPagingRaw(pubKey))
|
||||
|
||||
@Query("SELECT DISTINCT relays FROM application")
|
||||
fun getAllRelayLists(): List<RelayListWrapper>
|
||||
|
||||
/**
|
||||
* Paged Applications-list projection. Selects only the rendered columns
|
||||
* ([ApplicationListItem]) and deliberately NOT the envelope-encrypted
|
||||
* `secret`/`localKey` columns: each Keystore decrypt is a binder + TEE
|
||||
* round trip, and the list screen renders neither field, so a `SELECT *`
|
||||
* here made the screen pay 2 × rows cipher operations per page (Paging's
|
||||
* initial load is 3 × pageSize rows) for values it never reads.
|
||||
*/
|
||||
@Query("SELECT `key`, name, relays, icon, lastUsed FROM application WHERE pubKey = :pubKey ORDER BY lastUsed DESC")
|
||||
fun getApplicationListItemsPaging(pubKey: String): PagingSource<Int, ApplicationListItem>
|
||||
|
||||
@Query("SELECT name FROM application WHERE `key` = :key LIMIT 1")
|
||||
suspend fun getAppName(key: String): String?
|
||||
|
||||
|
||||
@@ -114,3 +114,18 @@ data class ApplicationKeyName(
|
||||
val key: String,
|
||||
val name: String,
|
||||
)
|
||||
|
||||
/**
|
||||
* Row projection for the Applications list screen ([ApplicationDao.getApplicationListItemsPaging]):
|
||||
* exactly the columns that screen renders. Deliberately excludes the
|
||||
* envelope-encrypted `secret`/`localKey` columns — every Keystore cipher
|
||||
* operation is a binder + TEE round trip, so a `SELECT *` paged load made
|
||||
* the screen pay 2 × rows decrypts per page for fields it never displays.
|
||||
*/
|
||||
data class ApplicationListItem(
|
||||
val key: String,
|
||||
val name: String,
|
||||
val relays: List<NormalizedRelayUrl>,
|
||||
val icon: String,
|
||||
val lastUsed: Long,
|
||||
)
|
||||
|
||||
@@ -1,7 +1,5 @@
|
||||
package com.greenart7c3.nostrsigner.database
|
||||
|
||||
import androidx.paging.PagingSource
|
||||
import androidx.paging.PagingState
|
||||
import com.greenart7c3.nostrsigner.Amber
|
||||
import com.greenart7c3.nostrsigner.AmberLog
|
||||
import com.greenart7c3.nostrsigner.SecureCryptoHelper
|
||||
@@ -65,28 +63,3 @@ fun ApplicationEntity.decryptFromStorage(): ApplicationEntity = copy(
|
||||
|
||||
/** Decrypts the embedded [ApplicationWithPermissions.application] after a Room read. */
|
||||
fun ApplicationWithPermissions.decryptFromStorage(): ApplicationWithPermissions = copy(application = application.decryptFromStorage())
|
||||
|
||||
/**
|
||||
* Wraps a raw (encrypted-column) [PagingSource] and decrypts every
|
||||
* [ApplicationEntity] emitted by a successful [LoadResult.Page]. Error and
|
||||
* Invalid results are passed through unchanged. Used by
|
||||
* [ApplicationDao.getAllPaging] so the `ApplicationsScreen` Pager sees
|
||||
* plaintext entities exactly like the non-paging DAO reads.
|
||||
*/
|
||||
internal class DecryptingPagingSource(
|
||||
private val delegate: PagingSource<Int, ApplicationEntity>,
|
||||
) : PagingSource<Int, ApplicationEntity>() {
|
||||
override fun getRefreshKey(state: PagingState<Int, ApplicationEntity>): Int? = delegate.getRefreshKey(state)
|
||||
|
||||
override suspend fun load(params: PagingSource.LoadParams<Int>): PagingSource.LoadResult<Int, ApplicationEntity> = when (val result = delegate.load(params)) {
|
||||
is PagingSource.LoadResult.Page -> PagingSource.LoadResult.Page(
|
||||
data = result.data.map { it.decryptFromStorage() },
|
||||
prevKey = result.prevKey,
|
||||
nextKey = result.nextKey,
|
||||
itemsBefore = result.itemsBefore,
|
||||
itemsAfter = result.itemsAfter,
|
||||
)
|
||||
is PagingSource.LoadResult.Error -> result
|
||||
is PagingSource.LoadResult.Invalid -> result
|
||||
}
|
||||
}
|
||||
|
||||
@@ -182,7 +182,7 @@ class CachingApplicationDao(
|
||||
|
||||
override suspend fun getAllNotConnected(): List<ApplicationWithPermissions> = delegate.getAllNotConnected()
|
||||
|
||||
override fun getAllPaging(pubKey: String): PagingSource<Int, ApplicationEntity> = delegate.getAllPaging(pubKey)
|
||||
override fun getApplicationListItemsPaging(pubKey: String): PagingSource<Int, ApplicationListItem> = delegate.getApplicationListItemsPaging(pubKey)
|
||||
|
||||
override fun getAllRelayLists(): List<RelayListWrapper> = delegate.getAllRelayLists()
|
||||
|
||||
@@ -245,8 +245,6 @@ class CachingApplicationDao(
|
||||
|
||||
override suspend fun getAllNotConnectedRaw(): List<ApplicationWithPermissions> = delegate.getAllNotConnectedRaw()
|
||||
|
||||
override fun getAllPagingRaw(pubKey: String): PagingSource<Int, ApplicationEntity> = delegate.getAllPagingRaw(pubKey)
|
||||
|
||||
override suspend fun getByKeyRaw(key: String): ApplicationWithPermissions? = delegate.getByKeyRaw(key)
|
||||
|
||||
override fun getByKeySyncRaw(key: String): ApplicationWithPermissions? = delegate.getByKeySyncRaw(key)
|
||||
|
||||
@@ -35,7 +35,6 @@ import androidx.compose.ui.text.withLink
|
||||
import androidx.compose.ui.unit.dp
|
||||
import androidx.compose.ui.unit.sp
|
||||
import androidx.lifecycle.compose.collectAsStateWithLifecycle
|
||||
import androidx.navigation.NavController
|
||||
import androidx.paging.Pager
|
||||
import androidx.paging.PagingConfig
|
||||
import androidx.paging.compose.collectAsLazyPagingItems
|
||||
@@ -59,16 +58,24 @@ import kotlinx.coroutines.launch
|
||||
fun ApplicationsScreen(
|
||||
modifier: Modifier,
|
||||
account: Account,
|
||||
navController: NavController,
|
||||
navControllerWrapper: NavHostControllerWrapper,
|
||||
) {
|
||||
// remember: without it every recomposition (killSwitch / backup-warning
|
||||
// state flips) builds a new Pager + cold flow and re-runs the initial
|
||||
// load from scratch. Keyed on the account so switching accounts reloads.
|
||||
val pager =
|
||||
Pager(
|
||||
PagingConfig(
|
||||
pageSize = 20,
|
||||
enablePlaceholders = false,
|
||||
),
|
||||
) {
|
||||
Amber.instance.getDatabase(account.npub).dao().getAllPaging(account.hexKey)
|
||||
remember(account.hexKey) {
|
||||
Pager(
|
||||
PagingConfig(
|
||||
pageSize = 20,
|
||||
enablePlaceholders = false,
|
||||
),
|
||||
) {
|
||||
// Projection excludes the envelope-encrypted secret/localKey
|
||||
// columns: the list renders neither, and each Keystore decrypt
|
||||
// is a binder + TEE round trip (2 x rows per page).
|
||||
Amber.instance.getDatabase(account.npub).dao().getApplicationListItemsPaging(account.hexKey)
|
||||
}
|
||||
}
|
||||
|
||||
val lazyPagingItems = pager.flow.collectAsLazyPagingItems()
|
||||
@@ -111,7 +118,7 @@ fun ApplicationsScreen(
|
||||
message = stringResource(R.string.make_backup_message),
|
||||
buttonText = stringResource(R.string.backup),
|
||||
onClick = {
|
||||
navController.navigate(Route.AccountBackup.route)
|
||||
navControllerWrapper.navController.navigate(Route.AccountBackup.route)
|
||||
},
|
||||
)
|
||||
}
|
||||
@@ -163,7 +170,7 @@ fun ApplicationsScreen(
|
||||
AmberButton(
|
||||
modifier = Modifier.padding(top = 20.dp),
|
||||
onClick = {
|
||||
navController.navigate(Route.Activities.route)
|
||||
navControllerWrapper.navController.navigate(Route.Activities.route)
|
||||
},
|
||||
text = stringResource(R.string.activity),
|
||||
)
|
||||
@@ -181,9 +188,9 @@ fun ApplicationsScreen(
|
||||
// tap (or a tap landing while the NavHost graph is being recreated) can
|
||||
// otherwise fire navigate() against a graph that no longer contains the
|
||||
// Permission destination, crashing with IllegalArgumentException.
|
||||
if (navController.currentDestination?.route == Route.Applications.route) {
|
||||
if (navControllerWrapper.navController.currentDestination?.route == Route.Applications.route) {
|
||||
runCatching {
|
||||
navController.navigate("Permission/${applicationWithHistory.key}")
|
||||
navControllerWrapper.navController.navigate("Permission/${applicationWithHistory.key}")
|
||||
}.onFailure {
|
||||
AmberLog.e("ApplicationsScreen", "Failed to open permissions for ${applicationWithHistory.key}", it)
|
||||
}
|
||||
|
||||
@@ -423,7 +423,7 @@ fun MainScreen(
|
||||
.padding(horizontal = verticalPadding)
|
||||
.padding(top = verticalPadding * 1.5f),
|
||||
account = account,
|
||||
navController = navController.navController,
|
||||
navControllerWrapper = navController,
|
||||
)
|
||||
},
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user