diff --git a/.github/workflows/debug.yml b/.github/workflows/debug.yml index 7bc53c7a28..12d8f429d0 100644 --- a/.github/workflows/debug.yml +++ b/.github/workflows/debug.yml @@ -197,6 +197,13 @@ jobs: run: | flox activate -d flox/base -- ./gradlew :plugin-api:apiCheck --no-daemon + # testDebugUnitTest, not testV8DebugUnitTest: :plugin-api declares no product flavors, so the + # v8 task every other module has does not exist here. Named as its own step rather than left + # to jacocoAggregateReport, which is not on the PR path at all. + - name: Run plugin-api unit tests + run: | + flox activate -d flox/base -- ./gradlew :plugin-api:testDebugUnitTest --no-daemon + # Combined with a compile task on purpose: spotlessCheck alone never pulls a # source-tree-writing task into the graph, so the implicit-dependency failure ADFA-5244 fixed # cannot reproduce in a standalone run. Without this step a regression of that exclude is diff --git a/build.gradle.kts b/build.gradle.kts index cd5c3f7120..60208faa6a 100755 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -462,11 +462,24 @@ tasks.named("sonarqube") { tasks.register("jacocoAggregateReport") { val excludedProjects = emptySet() - // Depend only on testV8DebugUnitTest tasks in subprojects + // (variant directory, unit-test task) pairs, v8 first so a flavored module keeps using it. Two + // entries because the module set is not uniform: every flavored module builds `v8Debug`, but a + // flavorless one such as `:plugin-api` only builds `debug`. Collecting the v8 names alone dropped + // such a module from this report silently - `mapNotNull` returned nothing for it, so its tests + // never ran here and its classes counted for nothing. + val coverageVariants = + listOf( + "v8Debug" to "testV8DebugUnitTest", + "debug" to "testDebugUnitTest", + ) + + // One unit-test task per subproject, not one per variant: a module has only ever one of these. dependsOn( subprojects .filterNot { it.name in excludedProjects } - .mapNotNull { it.tasks.findByName("testV8DebugUnitTest") }, + .mapNotNull { subproj -> + coverageVariants.firstNotNullOfOrNull { (_, task) -> subproj.tasks.findByName(task) } + }, ) reports { @@ -483,41 +496,49 @@ tasks.register("jacocoAggregateReport") { "**/*Test*.*", ) - // Collect kotlin and java class directories for v8Debug and v8DebugUnitTest variant + // Kotlin and java class directories, for each variant a module might have built. Absent + // directories contribute nothing, so listing both shapes per module is safe. val classDirs = subprojects .filterNot { it.name in excludedProjects } .flatMap { subproj -> - listOf( - fileTree(subproj.layout.buildDirectory.dir("tmp/kotlin-classes/v8Debug")) { - exclude(fileFilter) - }, - fileTree(subproj.layout.buildDirectory.dir("tmp/kotlin-classes/v8DebugUnitTest")) { - exclude(fileFilter) - }, - fileTree(subproj.layout.buildDirectory.dir("classes/java/v8Debug")) { - exclude(fileFilter) - }, - fileTree(subproj.layout.buildDirectory.dir("intermediates/javac/v8DebugUnitTest/classes")) { - exclude(fileFilter) - }, - ) + coverageVariants.flatMap { (variant, _) -> + listOf( + fileTree(subproj.layout.buildDirectory.dir("tmp/kotlin-classes/$variant")) { + exclude(fileFilter) + }, + fileTree(subproj.layout.buildDirectory.dir("tmp/kotlin-classes/${variant}UnitTest")) { + exclude(fileFilter) + }, + fileTree(subproj.layout.buildDirectory.dir("classes/java/$variant")) { + exclude(fileFilter) + }, + fileTree(subproj.layout.buildDirectory.dir("intermediates/javac/${variant}UnitTest/classes")) { + exclude(fileFilter) + }, + ) + } } - // Collect source directories + // Untouched on purpose, though it names only `src/main/java` while nine modules keep sources under + // `src/main/kotlin`: this property is inert for this task. Emptying it outright still renders full + // Kotlin source in the HTML and leaves the XML byte-identical, so adding the Kotlin roots here + // buys nothing. Measured, not assumed - do not "fix" it without re-measuring. val sourceDirs = subprojects .filterNot { it.name in excludedProjects } .map { it.file("src/main/java") } - // Collect execution data (.exec files) + // Collect execution data (.exec files), for each variant a module might have run. val execFiles = subprojects .filterNot { it.name in excludedProjects } - .map { subproj -> - subproj.layout.buildDirectory.file( - "outputs/unit_test_code_coverage/v8DebugUnitTest/testV8DebugUnitTest.exec", - ) + .flatMap { subproj -> + coverageVariants.map { (variant, task) -> + subproj.layout.buildDirectory.file( + "outputs/unit_test_code_coverage/${variant}UnitTest/$task.exec", + ) + } } classDirectories.setFrom(classDirs) diff --git a/docs/PLUGIN_API_CHANGELOG.md b/docs/PLUGIN_API_CHANGELOG.md index f35d285135..744ea89ed8 100644 --- a/docs/PLUGIN_API_CHANGELOG.md +++ b/docs/PLUGIN_API_CHANGELOG.md @@ -58,6 +58,52 @@ milestone. **[verified]** = read from the checked-in ABI dump. **[reconstructed] against an older `plugin-api` — the latter gets `AbstractMethodError`. Only the host's own implementers benefit, and they are recompiled with it. +- **added — Keystore-backed secret storage for plugins** _(ADFA-5269)_ **[verified]** + A plugin that stores a credential can encrypt it with AES/GCM under a hardware-backed + Android Keystore key instead of carrying its own copy of the cipher. Three AI plugins + had already grown near-identical copies that were starting to diverge (only one zeroed + its plaintext buffers, only one told "no secret stored" apart from "stored but no longer + decryptable"), and the host's own `git-core/CryptoManager` is a fourth. Duplicating the + source into each `.cgp` would not fix that: `compileOnly` against the host means one + implementation in the process, loaded by the host's class loader. + `KeystoreSecretStore(alias)` (`encrypt`, `decrypt`, `write`, `readAndMigrate`) and + `KeystoreSecretStore.Stored` / `.Absent` / `.Value` / `.Unreadable` / `.Unavailable`. The + `enc:v1:` marker it writes is deliberately **not** part of the surface: the on-disk format + is the store's business, both formats are handled for the caller, and keeping it private is + what leaves room for an `enc:v2:` later. + Additions to the **class** are additive, because a plugin instantiates it rather than + implementing it. `Stored` is the exception, and the one part of this entry that cannot grow + quietly: it is a public **sealed** interface, so a fifth case makes every plugin's exhaustive + `when` fail to compile, and a `.cgp` already built against four throws + `NoWhenBranchMatchedException` at runtime. Any new `Stored` case needs a `breaking` row — + including the `enc:v2:` state the paragraph above leaves room for, if it ever surfaces. + `encrypt` throws `GeneralSecurityException` and nothing else: whatever the Keystore actually + raises (`IOException` from `KeyStore.load`, the unchecked `ProviderException` from keygen) is + wrapped in one, so catching the documented type is enough — an unwrapped throwable here + reaches the host's crash handler as an IDE crash, not your plugin's error path. + `readAndMigrate` keeps a lost key (`Unreadable` — ask the user for the secret again) apart + from a Keystore that would not answer (`Unavailable` — retry), so a transient failure does + not cost the user a credential that is still perfectly readable. That split covers the cipher + step as well as key acquisition, and it enumerates the *permanent* failures rather than the + transient ones: a wrong key, an altered payload, a malformed one, an invalidated or + unrecoverable key are `Unreadable`, and anything else the Keystore surfaces — a dead binder, a + busy backend — is `Unavailable`. (Asking the platform is not an option here: `BackendBusyException` + is API 31+ and `KeyStoreException.isTransientFailure()` API 33+, against `minSdk 28`.) + A blank stored value is no credential, and all three entry points agree about the same bytes on + disk: `write` forgets one rather than storing it, `readAndMigrate` purges it and reports + `Absent`, `decrypt` returns null. (`encrypt`/`decrypt` used as a bare codec still round-trip + `""`; the rule is about what is *stored*.) + Every one of the four methods does Keystore binder IPC, so **call them off the main thread** — + `decrypt` and `write` additionally share one alias-scoped lock, and `write` holds it across a + synchronous flush. + The `alias` is a constructor parameter and must stay **distinct per + plugin**: plugins share the host's process, UID and therefore its Keystore, so a shared + alias would let one plugin's invalidated-key recovery (`deleteEntry`) destroy another's + stored secret. It must also stay stable across releases, since a secret encrypted under + one alias cannot be read under another. `readAndMigrate` re-encrypts a legacy plaintext + value in place, so a plugin adopting this keeps working for users who configured a + credential before it existed. + ### 26.33 — 2026-08-12 - **added — Plugin-contributed agent tools** _(ADFA-2592)_ **[verified]** Any `.cgp` can add tools to the AI agent, whose tool set was previously fixed at diff --git a/docs/plugin-api.md b/docs/plugin-api.md index 43c52cd0d1..d558e7f536 100644 --- a/docs/plugin-api.md +++ b/docs/plugin-api.md @@ -13,8 +13,9 @@ The surface a plugin binds to is broader than one module. All of the following a - Extension interfaces plugins **implement**: `UIExtension`, `EditorExtension`, `EditorTabExtension`, `DocumentationExtension`, `BuildActionExtension`, `SnippetExtension`, `ProjectExtension`, `FileOpenExtension`, `SettingsExtension`. - IDE service interfaces plugins **call** (via `ServiceRegistry.get(X::class.java)`): `IdeProjectService`, `IdeEditorService`, `IdeFileService`, `IdeEnvironmentService`, `IdeArchiveService`, `IdeBuildService`, `IdeUIService`, `IdeEditorTabService`, `IdeTooltipService`, `IdeThemeService`, `IdeFeatureFlagService`, `IdeCommandService`, `IdeTemplateService`, `IdeSnippetService`, `IdeSidebarService`. - Cross-plugin service interfaces, where **one plugin implements what another calls** (via `SharedServices`): `LlmInferenceService` — implemented by ai-core, called by every AI plugin — together with the types nested in it that a *backend* plugin implements (`LlmBackend`, `HistoryCapableBackend`, `ToolCallingBackend`, `CancellableBackend`, `ConfigurableBackend`) and the value types either side constructs (`ChatMessage`, `LlmConfig`, `LlmResponse`, `SystemPromptRequest`, `ToolDefinition`, `ToolCallRequest`). Also `ToolSourceRegistry` — implemented by ai-core, called by any plugin contributing tools to the agent — with `ToolSource` and `ToolSpec`, which a *contributing* plugin implements, `ToolInvocation`, which ai-core constructs and passes to `ToolSource.invoke`, and `ToolOutcome`, which the source returns. + - Utility classes plugins **instantiate**: `KeystoreSecretStore` (AES/GCM over the Android Keystore, alias supplied by the caller). Host-side implementation rather than an interface, so plugins share one copy in the process instead of compiling their own. - Data classes plugins **construct** (e.g. `MenuItem`, `TabItem`, `EditorTabItem`, `NavigationItem`, `ToolbarAction`, `FabAction`, `PluginBuildAction`, `SnippetContribution`, `PluginTooltipEntry`, `PluginSettingsEntry`). - - Enums / sealed types plugins **reference**: `PluginPermission`, `ShowAsAction`, `ArchiveFormat`, `BuildActionCategory`, `ToolbarActionIds`, `CommandSpec`, `CommandResult`, `ExtractResult`. + - Enums / sealed types plugins **reference**: `PluginPermission`, `ShowAsAction`, `ArchiveFormat`, `BuildActionCategory`, `ToolbarActionIds`, `CommandSpec`, `CommandResult`, `ExtractResult`, `KeystoreSecretStore.Stored`. Sealed, so a plugin `when`s over the cases exhaustively — adding one is a **breaking** change, not an additive one. - **Wire/format contracts outside the module:** - Manifest `` keys — `plugin.id`, `plugin.name`, `plugin.version`, `plugin.description`, `plugin.author`, `plugin.main_class`, `plugin.min_ide_version`, `plugin.max_ide_version`, `plugin.permissions`, `plugin.sidebar_items`, `plugin.icon_day`, `plugin.icon_night`. Matched **by string** — a rename silently breaks every plugin. - Permission **key strings** (`filesystem.read`, `filesystem.write`, `network.access`, `system.commands`, `ide.settings`, `project.structure`, `native.code`, `ide.environment.write`) — also matched by string. diff --git a/plugin-api/api/plugin-api.api b/plugin-api/api/plugin-api.api index 8fc09ea703..4ec832d1f9 100644 --- a/plugin-api/api/plugin-api.api +++ b/plugin-api/api/plugin-api.api @@ -1213,6 +1213,49 @@ public final class com/itsaky/androidide/plugins/extensions/UIExtension$DefaultI public static fun getToolbarActions (Lcom/itsaky/androidide/plugins/extensions/UIExtension;)Ljava/util/List; } +public final class com/itsaky/androidide/plugins/security/KeystoreSecretStore { + public fun (Ljava/lang/String;)V + public final fun decrypt (Ljava/lang/String;)Ljava/lang/String; + public final fun encrypt (Ljava/lang/String;)Ljava/lang/String; + public final fun readAndMigrate (Landroid/content/SharedPreferences;Ljava/lang/String;)Lcom/itsaky/androidide/plugins/security/KeystoreSecretStore$Stored; + public final fun write (Landroid/content/SharedPreferences;Ljava/lang/String;Ljava/lang/String;)Z +} + +public abstract interface class com/itsaky/androidide/plugins/security/KeystoreSecretStore$Stored { +} + +public final class com/itsaky/androidide/plugins/security/KeystoreSecretStore$Stored$Absent : com/itsaky/androidide/plugins/security/KeystoreSecretStore$Stored { + public static final field INSTANCE Lcom/itsaky/androidide/plugins/security/KeystoreSecretStore$Stored$Absent; + public fun equals (Ljava/lang/Object;)Z + public fun hashCode ()I + public fun toString ()Ljava/lang/String; +} + +public final class com/itsaky/androidide/plugins/security/KeystoreSecretStore$Stored$Unavailable : com/itsaky/androidide/plugins/security/KeystoreSecretStore$Stored { + public static final field INSTANCE Lcom/itsaky/androidide/plugins/security/KeystoreSecretStore$Stored$Unavailable; + public fun equals (Ljava/lang/Object;)Z + public fun hashCode ()I + public fun toString ()Ljava/lang/String; +} + +public final class com/itsaky/androidide/plugins/security/KeystoreSecretStore$Stored$Unreadable : com/itsaky/androidide/plugins/security/KeystoreSecretStore$Stored { + public static final field INSTANCE Lcom/itsaky/androidide/plugins/security/KeystoreSecretStore$Stored$Unreadable; + public fun equals (Ljava/lang/Object;)Z + public fun hashCode ()I + public fun toString ()Ljava/lang/String; +} + +public final class com/itsaky/androidide/plugins/security/KeystoreSecretStore$Stored$Value : com/itsaky/androidide/plugins/security/KeystoreSecretStore$Stored { + public fun (Ljava/lang/String;)V + public final fun component1 ()Ljava/lang/String; + public final fun copy (Ljava/lang/String;)Lcom/itsaky/androidide/plugins/security/KeystoreSecretStore$Stored$Value; + public static synthetic fun copy$default (Lcom/itsaky/androidide/plugins/security/KeystoreSecretStore$Stored$Value;Ljava/lang/String;ILjava/lang/Object;)Lcom/itsaky/androidide/plugins/security/KeystoreSecretStore$Stored$Value; + public fun equals (Ljava/lang/Object;)Z + public final fun getPlain ()Ljava/lang/String; + public fun hashCode ()I + public fun toString ()Ljava/lang/String; +} + public final class com/itsaky/androidide/plugins/services/ArchiveFormat : java/lang/Enum { public static final field GZIP Lcom/itsaky/androidide/plugins/services/ArchiveFormat; public static final field TAR Lcom/itsaky/androidide/plugins/services/ArchiveFormat; diff --git a/plugin-api/build.gradle.kts b/plugin-api/build.gradle.kts index a7943ce23c..ff75c15828 100644 --- a/plugin-api/build.gradle.kts +++ b/plugin-api/build.gradle.kts @@ -42,8 +42,18 @@ dependencies { api("org.jetbrains.kotlinx:kotlinx-coroutines-core:1.9.0") + // Logging goes through SLF4J, not android.util.Log. Not `compileOnly` like the Android artifacts + // above: these classes run in the host's process, which already resolves slf4j-api and the IDE's + // provider via :common -> :logger, so declaring it keeps the compile and runtime views the same. + implementation(libs.tooling.slf4j) + // Test dependencies testImplementation("junit:junit:4.13.2") + + // KeystoreSecretStore reaches for android.util.Base64 and a real SharedPreferences, so its tests + // need the framework on the JVM. + testImplementation(libs.tests.robolectric) + testImplementation(libs.tests.google.truth) } tasks.register("createPluginApiJar") { diff --git a/plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/security/KeystoreSecretStore.kt b/plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/security/KeystoreSecretStore.kt new file mode 100644 index 0000000000..e6cc909513 --- /dev/null +++ b/plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/security/KeystoreSecretStore.kt @@ -0,0 +1,373 @@ +package com.itsaky.androidide.plugins.security + +import android.content.SharedPreferences +import android.security.keystore.KeyPermanentlyInvalidatedException +import android.util.Base64 +import org.slf4j.LoggerFactory +import java.security.GeneralSecurityException +import java.security.UnrecoverableEntryException +import java.util.concurrent.ConcurrentHashMap +import javax.crypto.BadPaddingException +import javax.crypto.Cipher +import javax.crypto.IllegalBlockSizeException +import javax.crypto.SecretKey +import javax.crypto.spec.GCMParameterSpec + +/** + * AES/GCM encryption for a caller's secrets, keyed by a hardware-backed Android Keystore secret. + * Only ciphertext is written to SharedPreferences, so a copied prefs file (root, `adb backup`, + * forensic dump) is useless without this device's Keystore. + * + * Here rather than in each plugin because every plugin that stores a credential was carrying its + * own copy of the same cipher, migration and recovery path, and a fix applied to one copy is a fix + * the others silently do not get. + */ +class KeystoreSecretStore internal constructor( + private val alias: String, + private val keys: SecretKeySource, +) { + /** + * @param alias the Keystore alias holding the caller's key. A parameter, and deliberately + * distinct per caller: plugins run in the host's process and UID and therefore share one + * Keystore, so a shared alias would let one caller's invalidated-key recovery destroy another's + * stored secret. It must also stay stable across releases, since a secret encrypted under one + * alias cannot be read under another. It is also what identifies the caller in this class's log + * lines, since every caller shares one logger here. + */ + constructor(alias: String) : this(alias, AndroidKeystoreSource(alias)) + + /** + * Serializes every key acquisition, cipher operation and prefs write under this alias. + * + * Two compounds here are unsafe without it. [encrypt]'s invalidated-key recovery is + * delete-then-create-then-encrypt: interleaved, one caller deletes the *valid* key another just + * minted, and that other caller's returned ciphertext can never be decrypted. [readAndMigrate] + * is a read-modify-write against SharedPreferences, so a [write] committing in the middle of an + * upgrade is silently reverted to the old secret. + * + * Alias-scoped and process-wide rather than per-instance, for the same reason + * [AndroidKeystoreSource]'s own lock is: the Keystore entry is a process-wide resource and + * nothing stops a caller from building two stores over one alias. Coarser than strictly needed + * (it covers every preference key a store touches, not just the one), which costs nothing here - + * these calls are rare, already doing binder IPC, and documented as off-main-thread. + */ + private val aliasLock: Any = lockFor(alias) + + /** + * What was found under a preference key. + * + * Four outcomes rather than a nullable String: "nothing stored", "stored but no longer readable + * on this device" and "readable, but the Keystore would not answer just now" lead to three + * different pieces of advice, and collapsing them is what tells a user their credential was + * refused when it was never sent, or asks them to retype one that is perfectly intact. + */ + sealed interface Stored { + /** Nothing is stored under the key. */ + data object Absent : Stored + + /** The stored value, decrypted. */ + data class Value( + val plain: String, + ) : Stored { + // Never print the secret: a Stored can reach a log line or a crash breadcrumb. + override fun toString(): String = "Value(plain=)" + } + + /** + * Something is stored, but this device's Keystore can no longer open it: the key is gone + * (restore onto new hardware, an OEM reset, a re-enrolled screen lock) or the value does not + * authenticate. Permanent - ask the user for the secret again. + */ + data object Unreadable : Stored + + /** + * Something is stored and is very likely fine, but the Keystore could not be reached to open + * it - not ready this early in boot, a dead binder, a passing keymaster error. Transient: + * retry rather than telling the user their credential is lost. + */ + data object Unavailable : Stored + } + + private companion object { + /** + * Marks a stored value as ciphertext; anything without it is legacy plaintext. + * + * Deliberately not public: the on-disk format is this class's business, and a caller that + * branches on the marker itself is a caller a `enc:v2:` would break. Plugins get both formats + * handled for them by [decrypt] and [readAndMigrate]. + */ + private const val ENC_PREFIX = "enc:v1:" + + private const val TRANSFORM = "AES/GCM/NoPadding" + private const val IV_LEN = 12 + private const val TAG_BITS = 128 + + private val log = LoggerFactory.getLogger(KeystoreSecretStore::class.java) + + /** + * One monitor per alias, shared by every store over that alias. Entries are never evicted; + * an alias is a per-plugin constant, so the map holds a handful of them for the process's + * life rather than growing. + */ + private val ALIAS_LOCKS = ConcurrentHashMap() + + private fun lockFor(alias: String): Any = ALIAS_LOCKS.computeIfAbsent(alias) { Any() } + } + + /** + * Encrypts [plain] into a self-describing string: `enc:v1:` + base64(iv | ciphertext). + * + * The key is not auth-bound, so a credential change does not invalidate it; an alias an OEM + * Keystore drops anyway is regenerated once before retrying. + * + * Keystore IPC (a binder round trip per call, and a key generation on first use) plus AES/GCM, + * so call this off the main thread. + * + * @param plain the value to encrypt. + * @return the ciphertext to store. + * @throws GeneralSecurityException on any Keystore or cipher failure, so the caller can tell the + * user instead of crashing on Save. This is the only throwable a caller has to handle: a + * failure that is not already a `GeneralSecurityException` is wrapped in one, with the original + * as its cause. + */ + @Throws(GeneralSecurityException::class) + fun encrypt(plain: String): String = + synchronized(aliasLock) { + try { + try { + encryptWith(keys.getOrCreate(), plain) + } catch (e: KeyPermanentlyInvalidatedException) { + log.warn("Keystore key '{}' invalidated; regenerating and retrying encrypt", alias, e) + keys.delete() + encryptWith(keys.getOrCreate(), plain) + } + } catch (e: GeneralSecurityException) { + throw e + } catch (e: Exception) { + // Not every way this fails is a GeneralSecurityException, and the KDoc above promises + // one: KeyStore.load(null) declares IOException, and AndroidKeyStore keygen raises the + // unchecked ProviderException for keymaster failures. Unwrapped, a plugin catching + // exactly what is documented still takes an uncaught throwable - in the host's process, + // so it surfaces as an IDE crash. + throw GeneralSecurityException("Could not encrypt a secret under alias '$alias'", e) + } + } + + /** + * Reads a stored value back, handling both formats: an `enc:v1:` value is decrypted and + * anything else is returned unchanged as legacy plaintext. + * + * Keystore IPC plus AES/GCM for a ciphertext value, so call this off the main thread. It also + * takes the same alias lock [write] holds across its synchronous flush, so a main-thread call + * here can park on a background write's disk I/O. + * + * @param stored the stored string, ciphertext or legacy plaintext. + * @return the plaintext, or null when nothing is stored (null or blank, matching [write], which + * forgets a blank secret rather than storing one) or a ciphertext value could not be + * decrypted. Null collapses "the key is lost" and "the Keystore would not answer"; use + * [readAndMigrate] where that difference decides what the user is told. + */ + fun decrypt(stored: String?): String? = (readStored(stored) as? Stored.Value)?.plain + + /** + * Stores [plain] under [key], encrypted; a blank value removes the entry instead. + * + * Keystore IPC, AES/GCM and a synchronous prefs flush, so call this off the main thread. + * + * @param prefs where to store it. + * @param key the preference key. + * @param plain the secret, or blank to forget it. + * @return true once the value is on disk, false when encryption or the write itself failed. The + * flush is synchronous (`commit`, not `apply`) precisely so this answer is true at the moment + * it is returned: a caller that shows "Saved" on true must not be told that before the bytes + * have landed, or a process death loses a credential the user believes is stored. + */ + fun write( + prefs: SharedPreferences?, + key: String, + plain: String, + ): Boolean { + return synchronized(aliasLock) { + val editor = prefs?.edit() ?: return false + if (plain.isBlank()) { + return editor.remove(key).commit() + } + try { + editor.putString(key, encrypt(plain)).commit() + } catch (e: Exception) { + log.error("Could not encrypt the secret for '{}' under alias '{}'", key, alias, e) + false + } + } + } + + /** + * Reads [key] from [prefs], upgrading a legacy plaintext value to ciphertext in place. + * + * Secrets written before this store existed are still plaintext on disk, and [decrypt] alone + * hands them back unchanged forever, so an install configured earlier would never actually gain + * encryption. Re-encrypting on the first read closes that gap without making the user re-enter + * anything. The value migrates verbatim: trimming it here would rewrite a secret whose leading + * or trailing whitespace is significant, and since the upgrade overwrites the plaintext it was + * read from, that loss is unrecoverable. Trim at the call site if your credential format wants + * it. A blank legacy value is purged and reported [Stored.Absent], matching [write], which + * forgets a blank secret rather than storing one. + * + * Keystore IPC plus AES/GCM, so call this off the main thread. + * + * @param prefs where the value lives. + * @param key the preference key. + * @return what was found: nothing, the plaintext, a value that cannot be decrypted here, or a + * value the Keystore would not open just now. + */ + fun readAndMigrate( + prefs: SharedPreferences?, + key: String, + ): Stored { + return synchronized(aliasLock) { + val stored = prefs?.getString(key, null) ?: return Stored.Absent + if (stored.startsWith(ENC_PREFIX)) { + // A lost Keystore alias (restore onto new hardware, an OEM reset, a re-enrolled screen + // lock) is not the same as an absent secret, and must not be reported as one. + return readStored(stored) + } + if (stored.isBlank()) { + // [write] forgets a blank secret rather than storing one; reporting Value("") here would + // tell a caller a credential is configured when none is. Purge so both paths agree. + prefs.edit().remove(key).apply() + return Stored.Absent + } + try { + // `apply`, unlike [write]'s `commit`: the upgrade is best-effort and self-healing. A flush + // lost to process death leaves the legacy plaintext, which still reads back fine and gets + // re-upgraded on the next call, so blocking a read on disk I/O would buy nothing. + prefs.edit().putString(key, encrypt(stored)).apply() + log.info("Upgraded a legacy plaintext value for '{}' to ciphertext", key) + } catch (e: Exception) { + log.warn("Could not upgrade a legacy plaintext value for '{}' to ciphertext", key, e) + } + Stored.Value(stored) + } + } + + /** + * Classifies a stored string, keeping a value that is really gone apart from one the Keystore + * merely would not open just now. + * + * Both the key-acquisition and the cipher step make that split, and for the same reason: + * reporting a momentarily unavailable Keystore as [Stored.Unreadable] asks a user to retype a + * credential that is still perfectly readable, and reporting a permanent loss as + * [Stored.Unavailable] has a conforming caller retry forever and never re-prompt. + */ + private fun readStored(stored: String?): Stored { + if (stored == null) return Stored.Absent + // The rule [write] and [readAndMigrate] already apply, applied here too so all three agree: a + // blank secret is no secret. Value("") would have [decrypt] call a credential configured over + // the exact bytes [readAndMigrate] purges and reports Absent for. + if (stored.isBlank()) return Stored.Absent + if (!stored.startsWith(ENC_PREFIX)) return Stored.Value(stored) + return synchronized(aliasLock) { + val key = + try { + keys.getOrCreate() + } catch (e: KeyPermanentlyInvalidatedException) { + log.warn("Keystore key '{}' is invalidated; a value stored under it is lost", alias, e) + return@synchronized Stored.Unreadable + } catch (e: UnrecoverableEntryException) { + // The alias is there but its key material cannot be recovered: permanent, exactly as an + // invalidated key is, and not something a retry fixes. One arm covers both shapes - + // KeyStore.getEntry declares this, AndroidKeyStoreSpi.engineGetKey raises the + // UnrecoverableKeyException subclass of it. + log.warn("Keystore key '{}' cannot be recovered; a value stored under it is lost", alias, e) + return@synchronized Stored.Unreadable + } catch (e: Exception) { + log.warn("Could not obtain the Keystore key for alias '{}'", alias, e) + return@synchronized Stored.Unavailable + } + try { + Stored.Value(decryptWith(key, stored)) + } catch (e: Exception) { + val outcome = classifyCipherFailure(e) + log.warn("Could not decrypt a stored secret under alias '{}'; reporting {}", alias, outcome, e) + outcome + } + } + } + + /** + * Whether a cipher-step failure means the stored bytes are gone, or only that the Keystore would + * not answer just now. + * + * Enumerates the *permanent* failures rather than the transient ones, because that is the short + * closed list. Everything else a Keystore-backed `Cipher.init`/`doFinal` can surface -- a dead + * binder, a `BackendBusyException` under load, a passing keymaster error -- is worth a retry, and + * a value that fails one of those is intact: clear the failure and the same ciphertext reads back. + * Asking the platform instead is not available in this module: `BackendBusyException` is API 31+ + * and `KeyStoreException.isTransientFailure()` API 33+, against `minSdk 28`. + */ + private fun classifyCipherFailure(e: Exception): Stored = + when (e) { + // GCM authentication failed -- a wrong key, or a payload altered since it was written. The + // subclass AEADBadTagException is the usual one; a provider may raise the plain type instead. + is BadPaddingException, + // The payload is not a whole number of blocks; it is not base64; or it is too short to hold + // an IV (the `require` in [decryptWith]). Malformed, and no retry makes it well-formed. + is IllegalBlockSizeException, + is IllegalArgumentException, + // Cipher.init raises this as readily as key acquisition does, and it is permanent in both. + is KeyPermanentlyInvalidatedException, + -> Stored.Unreadable + + else -> Stored.Unavailable + } + + private fun decryptWith( + key: SecretKey, + stored: String, + ): String { + val combined = Base64.decode(stored.removePrefix(ENC_PREFIX), Base64.NO_WRAP) + // The split below is at a fixed offset, so say so rather than letting copyOfRange throw: + // a truncated payload is a malformed value, not an unexpected error. + require(combined.size >= IV_LEN) { "Ciphertext is too short to hold a $IV_LEN-byte IV" } + val iv = combined.copyOfRange(0, IV_LEN) + val ciphertext = combined.copyOfRange(IV_LEN, combined.size) + val cipher = Cipher.getInstance(TRANSFORM) + cipher.init(Cipher.DECRYPT_MODE, key, GCMParameterSpec(TAG_BITS, iv)) + // Zeroed once the String is built; see the note in encryptWith about the String. + val plainBytes = cipher.doFinal(ciphertext) + return try { + String(plainBytes, Charsets.UTF_8) + } finally { + plainBytes.fill(0) + } + } + + private fun encryptWith( + key: SecretKey, + plain: String, + ): String { + val cipher = Cipher.getInstance(TRANSFORM) + cipher.init(Cipher.ENCRYPT_MODE, key) + val iv = cipher.iv + // [decrypt] splits the payload at a fixed IV_LEN, so a provider handing back any other IV + // length would have us write values we could never read back. GCM is 12 bytes on every + // runtime we ship to; refuse to store rather than store something unreadable. + if (iv.size != IV_LEN) { + throw GeneralSecurityException("Expected a $IV_LEN-byte GCM IV, got ${iv.size}") + } + // Zeroed straight after the cipher reads it. The String itself cannot be: every API these + // secrets pass through (SharedPreferences, JSONObject, setRequestProperty) takes one, so a + // CharArray here would only move the immutable copy one frame away. + val plainBytes = plain.toByteArray(Charsets.UTF_8) + val ciphertext = + try { + cipher.doFinal(plainBytes) + } finally { + plainBytes.fill(0) + } + val combined = ByteArray(iv.size + ciphertext.size) + System.arraycopy(iv, 0, combined, 0, iv.size) + System.arraycopy(ciphertext, 0, combined, iv.size, ciphertext.size) + return ENC_PREFIX + Base64.encodeToString(combined, Base64.NO_WRAP) + } +} diff --git a/plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/security/SecretKeySource.kt b/plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/security/SecretKeySource.kt new file mode 100644 index 0000000000..5e24788159 --- /dev/null +++ b/plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/security/SecretKeySource.kt @@ -0,0 +1,98 @@ +package com.itsaky.androidide.plugins.security + +import android.security.keystore.KeyGenParameterSpec +import android.security.keystore.KeyProperties +import org.slf4j.LoggerFactory +import java.security.KeyStore +import javax.crypto.KeyGenerator +import javax.crypto.SecretKey + +/** + * Where [KeystoreSecretStore] gets its AES key. + * + * A seam, not an extension point, and deliberately `internal`: the Android Keystore is a binder + * service with no JVM implementation, so `KeyStore.getInstance("AndroidKeyStore")` fails even under + * Robolectric. Without this, the cipher, migration and invalidated-key recovery paths -- the parts + * worth testing -- could only be exercised on a device. + */ +internal interface SecretKeySource { + /** + * The key, minting one on first use. + * + * Implementations must be safe to call concurrently: two callers that each mint a key under the + * same alias leave one of them holding a key the store has already overwritten, and every value + * encrypted under it is then unreadable for good. + */ + fun getOrCreate(): SecretKey + + /** Drops the key, so the next [getOrCreate] mints a fresh one. Never throws. */ + fun delete() +} + +/** + * The real source: a hardware-backed key held under [alias] in the Android Keystore. + * + * @param alias the Keystore alias holding the key. + */ +internal class AndroidKeystoreSource( + private val alias: String, +) : SecretKeySource { + override fun getOrCreate(): SecretKey = + synchronized(KEYSTORE_LOCK) { + val store = KeyStore.getInstance(KEYSTORE).apply { load(null) } + val existing = store.getEntry(alias, null) + (existing as? KeyStore.SecretKeyEntry)?.let { return it.secretKey } + if (existing != null) { + // Falling through replaces whatever is under the alias, so be loud about it: plugins + // share the host's UID and therefore its Keystore, and the entry class is the only clue + // to whose it was. The class, not the entry - a PrivateKeyEntry's toString prints its + // certificate chain. + log.warn( + "Keystore alias '{}' holds a {}, not a secret key; replacing it", + alias, + existing.javaClass.name, + ) + } + val generator = KeyGenerator.getInstance(KeyProperties.KEY_ALGORITHM_AES, KEYSTORE) + generator.init( + KeyGenParameterSpec + .Builder( + alias, + KeyProperties.PURPOSE_ENCRYPT or KeyProperties.PURPOSE_DECRYPT, + ).setBlockModes(KeyProperties.BLOCK_MODE_GCM) + .setEncryptionPaddings(KeyProperties.ENCRYPTION_PADDING_NONE) + .build(), + ) + generator.generateKey() + } + + override fun delete() { + synchronized(KEYSTORE_LOCK) { + try { + KeyStore.getInstance(KEYSTORE).apply { load(null) }.deleteEntry(alias) + } catch (e: Exception) { + // The alias, not the key material: a Keystore secret key is non-exportable, so there is + // nothing here to leak, and naming the alias is what makes the line diagnosable. + log.warn("Failed to delete Keystore alias '{}'", alias, e) + } + } + } + + private companion object { + const val KEYSTORE = "AndroidKeyStore" + + /** + * Guards the read-then-generate in [getOrCreate], which is otherwise a lost-update race: + * two threads both find the alias empty, both generate, and the second `generateKey` replaces + * the first in the Keystore -- so whichever thread encrypted under the losing key wrote a + * value nothing can ever decrypt again. + * + * Process-wide rather than per-instance because the Keystore is a single process-wide + * resource and nothing stops a caller from building two stores over one alias. Contention is + * irrelevant: these calls are rare and already doing binder IPC. + */ + val KEYSTORE_LOCK = Any() + + val log = LoggerFactory.getLogger(AndroidKeystoreSource::class.java) + } +} diff --git a/plugin-api/src/test/kotlin/com/itsaky/androidide/plugins/security/KeystoreSecretStoreTest.kt b/plugin-api/src/test/kotlin/com/itsaky/androidide/plugins/security/KeystoreSecretStoreTest.kt new file mode 100644 index 0000000000..9cc1cab1bd --- /dev/null +++ b/plugin-api/src/test/kotlin/com/itsaky/androidide/plugins/security/KeystoreSecretStoreTest.kt @@ -0,0 +1,651 @@ +package com.itsaky.androidide.plugins.security + +import android.content.Context +import android.content.SharedPreferences +import android.security.keystore.KeyPermanentlyInvalidatedException +import android.util.Base64 +import com.google.common.truth.Truth.assertThat +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import org.robolectric.annotation.Config +import java.io.IOException +import java.security.GeneralSecurityException +import java.security.KeyStoreException +import java.security.ProviderException +import java.security.UnrecoverableKeyException +import java.util.concurrent.CopyOnWriteArrayList +import java.util.concurrent.CyclicBarrier +import java.util.concurrent.TimeUnit +import java.util.concurrent.atomic.AtomicBoolean +import javax.crypto.KeyGenerator +import javax.crypto.SecretKey + +/** + * Robolectric rather than a plain JVM test because the store reaches for `android.util.Base64` and + * stores through a real [SharedPreferences]. The Keystore itself is swapped out via + * [SecretKeySource] -- there is no AndroidKeyStore JCA provider off-device. + */ +@RunWith(RobolectricTestRunner::class) +@Config(sdk = [34]) +class KeystoreSecretStoreTest { + private lateinit var prefs: SharedPreferences + private lateinit var keys: FakeKeySource + private lateinit var store: KeystoreSecretStore + + @Before + fun setUp() { + prefs = + RuntimeEnvironment + .getApplication() + .getSharedPreferences("secrets", Context.MODE_PRIVATE) + prefs.edit().clear().apply() + keys = FakeKeySource() + store = KeystoreSecretStore(ALIAS, keys) + } + + @Test + fun `Given_a_plaintext_secret_When_encrypt_Then_the_output_is_marked_ciphertext_and_omits_the_plaintext`() { + val out = store.encrypt("hunter2") + + assertThat(out).startsWith(ENC_PREFIX) + assertThat(out).doesNotContain("hunter2") + } + + @Test + fun `Given_the_same_secret_encrypted_twice_When_the_results_are_compared_Then_the_ciphertexts_differ`() { + assertThat(store.encrypt("same")).isNotEqualTo(store.encrypt("same")) + } + + @Test + fun `Given_an_encrypted_secret_When_decrypt_Then_the_original_plaintext_is_returned`() { + assertThat(store.decrypt(store.encrypt("hunter2"))).isEqualTo("hunter2") + } + + @Test + fun `Given_non_ASCII_emoji_and_multi_byte_secrets_When_round_tripped_Then_each_is_preserved_exactly`() { + // Both directions name Charsets.UTF_8 explicitly; this pins that they agree. + listOf( + "paßwort-äöü", + "密码令牌", + "🔐🚀", + "مفتاح", + // Escapes, not raw 0x00: two literal NUL bytes trip git's binary heuristic, and this + // whole suite then has no reviewable diff on GitHub. Identical bytes once compiled. + "key\u0000with\u0000nuls", + ).forEach { secret -> + assertThat(store.decrypt(store.encrypt(secret))).isEqualTo(secret) + } + } + + @Test + fun `Given_an_empty_and_a_64KB_secret_When_round_tripped_Then_each_is_preserved_exactly`() { + assertThat(store.decrypt(store.encrypt(""))).isEqualTo("") + val long = "x".repeat(64 * 1024) + assertThat(store.decrypt(store.encrypt(long))).isEqualTo(long) + } + + @Test + fun `Given_an_invalidated_Keystore_key_When_encrypt_Then_the_key_is_regenerated_once_and_the_retry_succeeds`() { + keys.invalidated = true + + val out = store.encrypt("hunter2") + + assertThat(store.decrypt(out)).isEqualTo("hunter2") + assertThat(keys.deleted).isEqualTo(1) + // The fake mints a fresh key on delete, so this pins that the retry used the *new* key + // rather than passing because the old one was handed back unchanged. + assertThat(keys.minted).hasSize(2) + } + + @Test + fun `Given_concurrent_recovery_from_an_invalidated_key_When_they_encrypt_Then_one_regeneration_serves_every_caller`() { + // encrypt's recovery is a delete-then-create-then-encrypt compound. Interleaved, the second + // caller's delete drops the *valid* key the first just minted, and the first caller walks away + // with a ciphertext nothing can ever decrypt. Serialized, whoever gets there second finds the + // fresh key and never deletes at all. + val threads = 4 + val ready = CyclicBarrier(threads) + val out = arrayOfNulls(threads) + keys.invalidated = true + + val workers = + (0 until threads).map { t -> + Thread { + ready.await() + out[t] = store.encrypt("secret-$t") + }.apply { start() } + } + workers.forEach { it.join() } + + assertThat(keys.deleted).isEqualTo(1) + (0 until threads).forEach { t -> + assertThat(store.decrypt(out[t])).isEqualTo("secret-$t") + } + } + + @Test + fun `Given_a_Keystore_failure_that_is_not_a_GeneralSecurityException_When_encrypt_Then_it_is_wrapped_in_one`() { + // encrypt documents GeneralSecurityException, but KeyStore.load(null) declares IOException and + // AndroidKeyStore keygen raises the unchecked ProviderException. Unwrapped, a plugin catching + // exactly what is documented still takes an uncaught throwable - in the host's process. + listOf(IOException("keystore file"), ProviderException("keymaster")).forEach { cause -> + keys.getFailure = cause + + val thrown = + try { + store.encrypt("hunter2") + null + } catch (e: GeneralSecurityException) { + e + } + + assertThat(thrown).isNotNull() + assertThat(thrown!!.cause).isSameInstanceAs(cause) + } + } + + @Test(expected = KeyStoreException::class) + fun `Given_an_unavailable_Keystore_When_encrypt_Then_the_failure_propagates_so_the_caller_can_tell_the_user`() { + keys.getFailure = KeyStoreException("Keystore unavailable") + store.encrypt("hunter2") + } + + @Test + fun `Given_a_null_stored_value_When_decrypt_Then_null_is_returned`() { + assertThat(store.decrypt(null)).isNull() + } + + @Test + fun `Given_an_unprefixed_legacy_plaintext_value_When_decrypt_Then_it_is_returned_unchanged`() { + assertThat(store.decrypt("legacy-plaintext")).isEqualTo("legacy-plaintext") + assertThat(store.decrypt(" padded ")).isEqualTo(" padded ") + } + + @Test + fun `Given_a_blank_stored_value_When_decrypt_and_readAndMigrate_Then_both_report_no_credential`() { + listOf("", " ", "\t\n").forEach { blank -> + // decrypt returning "" while readAndMigrate returns Absent over the identical bytes lets one + // plugin send an empty API key on `decrypt(...) != null` and the same plugin, migrated to + // readAndMigrate, correctly find nothing - with nothing on disk having changed. + assertThat(store.decrypt(blank)).isNull() + + prefs.edit().putString(KEY, blank).apply() + assertThat(store.readAndMigrate(prefs, KEY)).isEqualTo(KeystoreSecretStore.Stored.Absent) + } + } + + @Test + fun `Given_ciphertext_tampered_with_in_place_When_decrypt_Then_null_is_returned_rather_than_garbage`() { + val body = store.encrypt("hunter2").removePrefix(ENC_PREFIX) + val raw = Base64.decode(body, Base64.NO_WRAP) + // Flip a bit past the IV: GCM authenticates, so this must fail rather than yield garbage. + raw[raw.size - 1] = (raw[raw.size - 1].toInt() xor 0x01).toByte() + val tampered = ENC_PREFIX + Base64.encodeToString(raw, Base64.NO_WRAP) + + assertThat(store.decrypt(tampered)).isNull() + } + + @Test + fun `Given_a_truncated_malformed_or_non_base64_payload_When_decrypt_Then_null_is_returned`() { + listOf( + ENC_PREFIX, + ENC_PREFIX + Base64.encodeToString(ByteArray(4), Base64.NO_WRAP), + ENC_PREFIX + Base64.encodeToString(ByteArray(12), Base64.NO_WRAP), + ENC_PREFIX + "!!!not-base64!!!", + ).forEach { assertThat(store.decrypt(it)).isNull() } + } + + @Test + fun `Given_a_key_other_than_the_one_that_encrypted_the_value_When_decrypt_Then_null_is_returned`() { + val out = store.encrypt("hunter2") + + val onNewHardware = KeystoreSecretStore(ALIAS, FakeKeySource()) + assertThat(onNewHardware.decrypt(out)).isNull() + } + + @Test + fun `Given_a_secret_When_write_Then_only_ciphertext_reaches_SharedPreferences`() { + assertThat(store.write(prefs, KEY, "hunter2")).isTrue() + + val raw = prefs.getString(KEY, null) + assertThat(raw).startsWith(ENC_PREFIX) + assertThat(raw).doesNotContain("hunter2") + assertThat(store.decrypt(raw)).isEqualTo("hunter2") + } + + @Test + fun `Given_null_prefs_When_write_Then_false_is_returned_and_nothing_is_stored`() { + assertThat(store.write(null, KEY, "hunter2")).isFalse() + } + + @Test + fun `Given_a_blank_value_When_write_Then_the_entry_is_forgotten_rather_than_stored_empty`() { + store.write(prefs, KEY, "hunter2") + + listOf("", " ", "\t\n").forEach { blank -> + store.write(prefs, KEY, "hunter2") + assertThat(store.write(prefs, KEY, blank)).isTrue() + assertThat(prefs.contains(KEY)).isFalse() + } + } + + @Test + fun `Given_encryption_fails_When_write_Then_false_is_returned_and_the_previous_value_survives`() { + store.write(prefs, KEY, "old") + val before = prefs.getString(KEY, null) + keys.getFailure = KeyStoreException("Keystore unavailable") + + assertThat(store.write(prefs, KEY, "new")).isFalse() + assertThat(prefs.getString(KEY, null)).isEqualTo(before) + } + + @Test + fun `Given_null_prefs_or_an_unset_key_When_readAndMigrate_Then_Absent_is_reported`() { + assertThat(store.readAndMigrate(null, KEY)).isEqualTo(KeystoreSecretStore.Stored.Absent) + assertThat(store.readAndMigrate(prefs, KEY)).isEqualTo(KeystoreSecretStore.Stored.Absent) + } + + @Test + fun `Given_a_value_the_store_wrote_When_readAndMigrate_Then_the_decrypted_value_is_returned`() { + store.write(prefs, KEY, "hunter2") + + assertThat(store.readAndMigrate(prefs, KEY)) + .isEqualTo(KeystoreSecretStore.Stored.Value("hunter2")) + } + + @Test + fun `Given_a_stored_secret_and_a_lost_Keystore_key_When_readAndMigrate_Then_Unreadable_not_Absent_is_reported`() { + store.write(prefs, KEY, "hunter2") + + // The distinction the tri-state exists for: reporting this as Absent is what tells a user + // their credential was refused when it was never sent. + val onNewHardware = KeystoreSecretStore(ALIAS, FakeKeySource()) + assertThat(onNewHardware.readAndMigrate(prefs, KEY)) + .isEqualTo(KeystoreSecretStore.Stored.Unreadable) + } + + @Test + fun `Given_a_stored_secret_and_an_unreachable_Keystore_When_readAndMigrate_Then_Unavailable_not_Unreadable_is_reported`() { + store.write(prefs, KEY, "hunter2") + keys.getFailure = KeyStoreException("Keystore unavailable") + + // Unreadable is the caller's cue to make the user type the credential again. The ciphertext + // here is intact and the very next call succeeds, so saying Unreadable would be a lie that + // costs the user their secret. + assertThat(store.readAndMigrate(prefs, KEY)) + .isEqualTo(KeystoreSecretStore.Stored.Unavailable) + + keys.getFailure = null + assertThat(store.readAndMigrate(prefs, KEY)) + .isEqualTo(KeystoreSecretStore.Stored.Value("hunter2")) + } + + @Test + fun `Given_a_stored_secret_and_an_invalidated_key_When_readAndMigrate_Then_Unreadable_is_reported`() { + store.write(prefs, KEY, "hunter2") + keys.invalidated = true + + // The other side of the split above: this one really is gone, and the read path does not + // regenerate, because regenerating cannot bring the value back. + assertThat(store.readAndMigrate(prefs, KEY)) + .isEqualTo(KeystoreSecretStore.Stored.Unreadable) + } + + @Test + fun `Given_a_stored_secret_and_a_key_whose_material_cannot_be_recovered_When_readAndMigrate_Then_Unreadable_is_reported`() { + store.write(prefs, KEY, "hunter2") + // KeyStore.getEntry declares UnrecoverableEntryException and engineGetKey raises the + // UnrecoverableKeyException subclass; either means the entry is there but its key material is + // not - permanent, so Unavailable here would have a conforming caller retry forever. + keys.getFailure = UnrecoverableKeyException("Key material is gone") + + assertThat(store.readAndMigrate(prefs, KEY)) + .isEqualTo(KeystoreSecretStore.Stored.Unreadable) + } + + @Test + fun `Given_the_cipher_fails_transiently_When_readAndMigrate_Then_Unavailable_is_reported_and_the_ciphertext_survives`() { + store.write(prefs, KEY, "hunter2") + // Key acquisition succeeds and only the cipher fails, which is the dead-binder / busy-backend + // shape. Reporting Unreadable here is the exact lie Stored.Unavailable was added to prevent. + keys.cipherUnreachable = true + + assertThat(store.readAndMigrate(prefs, KEY)) + .isEqualTo(KeystoreSecretStore.Stored.Unavailable) + assertThat(store.decrypt(prefs.getString(KEY, null))).isNull() + + // The proof that Unavailable was the honest answer: the stored bytes were never the problem. + keys.cipherUnreachable = false + assertThat(store.readAndMigrate(prefs, KEY)) + .isEqualTo(KeystoreSecretStore.Stored.Value("hunter2")) + } + + @Test + fun `Given_a_permanently_lost_value_When_readAndMigrate_Then_Unreadable_survives_the_transient_default`() { + // The other side of the inversion: with everything unrecognised now defaulting to Unavailable, + // the failures that really do mean the value is gone must still be recognised. + store.write(prefs, KEY, "hunter2") + val body = prefs.getString(KEY, null)!!.removePrefix(ENC_PREFIX) + val raw = Base64.decode(body, Base64.NO_WRAP) + raw[raw.size - 1] = (raw[raw.size - 1].toInt() xor 0x01).toByte() + + listOf( + // GCM authentication failure: AEADBadTagException. + ENC_PREFIX + Base64.encodeToString(raw, Base64.NO_WRAP), + // Not base64, and too short to hold an IV: IllegalArgumentException. + ENC_PREFIX + "!!!not-base64!!!", + ENC_PREFIX + Base64.encodeToString(ByteArray(4), Base64.NO_WRAP), + ).forEach { stored -> + prefs.edit().putString(KEY, stored).apply() + assertThat(store.readAndMigrate(prefs, KEY)) + .isEqualTo(KeystoreSecretStore.Stored.Unreadable) + } + } + + @Test + fun `Given_a_Stored_Value_When_it_is_printed_Then_the_secret_is_redacted`() { + // The store zeroes its plaintext buffers so plaintext does not linger; a generated toString + // would undo that the first time a caller logs a Stored or one lands in a crash breadcrumb. + val value = KeystoreSecretStore.Stored.Value("hunter2") + + assertThat(value.toString()).doesNotContain("hunter2") + assertThat("$value").doesNotContain("hunter2") + assertThat(listOf(value).toString()).doesNotContain("hunter2") + // Redacting the printout must not cost the equality the tri-state is compared by. + assertThat(value.plain).isEqualTo("hunter2") + } + + @Test + fun `Given_a_legacy_plaintext_value_When_readAndMigrate_Then_it_is_upgraded_to_ciphertext_in_place`() { + prefs.edit().putString(KEY, "legacy-secret").apply() + + assertThat(store.readAndMigrate(prefs, KEY)) + .isEqualTo(KeystoreSecretStore.Stored.Value("legacy-secret")) + + val raw = prefs.getString(KEY, null) + assertThat(raw).startsWith(ENC_PREFIX) + assertThat(raw).doesNotContain("legacy-secret") + // Still readable after the upgrade, and stable across a second read. + assertThat(store.readAndMigrate(prefs, KEY)) + .isEqualTo(KeystoreSecretStore.Stored.Value("legacy-secret")) + } + + @Test + fun `Given_a_legacy_value_padded_with_whitespace_When_readAndMigrate_Then_the_padding_is_preserved`() { + prefs.edit().putString(KEY, " legacy-secret\n").apply() + + // Trimming would rewrite a secret whose padding is significant, and unrecoverably: the + // upgrade overwrites the very plaintext it was read from. write does not trim either. + assertThat(store.readAndMigrate(prefs, KEY)) + .isEqualTo(KeystoreSecretStore.Stored.Value(" legacy-secret\n")) + assertThat(store.decrypt(prefs.getString(KEY, null))).isEqualTo(" legacy-secret\n") + } + + @Test + fun `Given_a_secret_padded_with_whitespace_When_written_and_read_back_Then_both_paths_agree`() { + val padded = " padded-secret\n" + assertThat(store.write(prefs, KEY, padded)).isTrue() + + // The encrypted path and the legacy-migration path must not disagree about the same secret. + assertThat(store.readAndMigrate(prefs, KEY)).isEqualTo(KeystoreSecretStore.Stored.Value(padded)) + } + + @Test + fun `Given_the_upgrade_cannot_encrypt_When_readAndMigrate_Then_the_plaintext_is_still_returned`() { + prefs.edit().putString(KEY, "legacy-secret").apply() + keys.getFailure = KeyStoreException("Keystore unavailable") + + // A Keystore that cannot encrypt must not cost the user a credential that already works. + assertThat(store.readAndMigrate(prefs, KEY)) + .isEqualTo(KeystoreSecretStore.Stored.Value("legacy-secret")) + assertThat(prefs.getString(KEY, null)).isEqualTo("legacy-secret") + } + + @Test + fun `Given_a_write_racing_a_legacy_upgrade_When_both_run_Then_the_newer_secret_survives`() { + prefs.edit().putString(KEY, "legacy-secret").apply() + val writer = Thread { store.write(prefs, KEY, "typed-by-the-user") } + // readAndMigrate reads the legacy value, then re-encrypts it back over the same key. Let the + // write run in exactly that window: unserialized, the upgrade silently reverts the credential + // the user just typed, and nothing is surfaced to say so. + val hooked = + ReadHookedPrefs(prefs) { + writer.start() + awaitBlockedOrDone(writer) + } + + val read = store.readAndMigrate(hooked, KEY) + writer.join() + + assertThat(read).isEqualTo(KeystoreSecretStore.Stored.Value("legacy-secret")) + assertThat(store.decrypt(prefs.getString(KEY, null))).isEqualTo("typed-by-the-user") + } + + @Test + fun `Given_a_blank_legacy_value_When_readAndMigrate_Then_it_is_purged_and_reported_Absent`() { + listOf("", " ", "\t\n").forEach { blank -> + prefs.edit().putString(KEY, blank).apply() + + // Same rule write applies: a blank secret is no secret. Value("") would tell a caller a + // credential is configured when none is. + assertThat(store.readAndMigrate(prefs, KEY)).isEqualTo(KeystoreSecretStore.Stored.Absent) + assertThat(prefs.contains(KEY)).isFalse() + } + } + + @Test + fun `Given_two_stores_with_different_keys_When_each_reads_the_other_s_entry_Then_it_is_Unreadable`() { + val other = KeystoreSecretStore(ALIAS, FakeKeySource()) + store.write(prefs, KEY, "mine") + other.write(prefs, "other_key", "theirs") + + assertThat(store.readAndMigrate(prefs, KEY)) + .isEqualTo(KeystoreSecretStore.Stored.Value("mine")) + assertThat(store.readAndMigrate(prefs, "other_key")) + .isEqualTo(KeystoreSecretStore.Stored.Unreadable) + } + + @Test + fun `Given_the_public_alias_constructor_When_a_store_is_constructed_Then_the_Keystore_is_not_touched`() { + // Plugins construct this on whatever thread builds their settings screen, so construction + // must stay cheap: no Keystore IPC until a secret is actually read or written. Robolectric + // has no AndroidKeyStore provider, so this only passes while that stays true. + val real = KeystoreSecretStore(alias = "com.example.plugin.key") + + // Format handling needs no key at all, so it still works here. + assertThat(real.decrypt(null)).isNull() + assertThat(real.decrypt("legacy-plaintext")).isEqualTo("legacy-plaintext") + } + + @Test + fun `Given_a_payload_too_short_to_hold_an_IV_When_decrypt_Then_it_is_rejected_on_a_length_check`() { + // The IV/ciphertext split is at a fixed offset, so a payload shorter than the IV is rejected + // outright rather than by letting copyOfRange throw and catching that. + (0 until 12).forEach { size -> + val short = + ENC_PREFIX + Base64.encodeToString(ByteArray(size), Base64.NO_WRAP) + assertThat(store.decrypt(short)).isNull() + } + } + + @Test + fun `Given_a_secret_When_write_Then_the_plaintext_never_reaches_the_backing_file`() { + // The class's core claim is that a copied prefs file is useless without this device's + // Keystore, so assert it against the real file rather than an in-memory read. + // (This cannot also prove write's commit-not-apply flush: Robolectric drains apply() + // synchronously, so the two are indistinguishable here.) + val file = + java.io.File(RuntimeEnvironment.getApplication().dataDir, "shared_prefs/secrets.xml") + + assertThat(store.write(prefs, KEY, "hunter2")).isTrue() + + assertThat(file.exists()).isTrue() + val onDisk = file.readText() + assertThat(onDisk).contains(KEY) + assertThat(onDisk).doesNotContain("hunter2") + } + + @Test + fun `Given_many_threads_sharing_one_store_When_they_encrypt_and_decrypt_at_once_Then_every_secret_round_trips`() { + // The store holds no mutable state beyond its alias lock, so concurrent use must be safe. + // (The recovery compound is covered by the barrier test above; AndroidKeystoreSource's own + // read-then-generate race is guarded inside it, and has no JVM stand-in.) + val threads = 8 + val perThread = 40 + val failures = java.util.concurrent.ConcurrentLinkedQueue() + val start = java.util.concurrent.CountDownLatch(1) + val workers = + (0 until threads).map { t -> + Thread { + start.await() + repeat(perThread) { i -> + val secret = "secret-$t-$i" + val back = store.decrypt(store.encrypt(secret)) + if (back != secret) failures += "$secret -> $back" + } + }.apply { start() } + } + + start.countDown() + workers.forEach { it.join() } + + assertThat(failures).isEmpty() + } + + @Test + fun `Given_the_four_Stored_cases_When_they_are_compared_Then_each_stays_distinct`() { + val value = KeystoreSecretStore.Stored.Value("a") + + assertThat(value).isEqualTo(KeystoreSecretStore.Stored.Value("a")) + assertThat(value).isNotEqualTo(KeystoreSecretStore.Stored.Value("b")) + assertThat(value.plain).isEqualTo("a") + assertThat(value.copy(plain = "b")).isEqualTo(KeystoreSecretStore.Stored.Value("b")) + + val cases = + listOf( + KeystoreSecretStore.Stored.Absent, + value, + KeystoreSecretStore.Stored.Unreadable, + KeystoreSecretStore.Stored.Unavailable, + ) + // Unavailable collapsing into any of the others is the bug the case exists to prevent. + assertThat(cases.toSet()).hasSize(4) + } + + /** + * Waits until [t] is parked on a monitor or has finished, so a race test can pin the interleaving + * instead of hoping for it. Serialized, the thread blocks; unserialized, it runs to completion - + * either way the caller may proceed, and the assertions are what tell the two apart. + */ + private fun awaitBlockedOrDone(t: Thread) { + val deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(5) + while (System.nanoTime() < deadline) { + if (t.state == Thread.State.BLOCKED || t.state == Thread.State.TERMINATED) return + Thread.yield() + } + throw AssertionError("The racing thread never reached the store") + } + + /** + * Delegating prefs that run [onFirstRead] once, right after the first `getString` returns - the + * one point inside `readAndMigrate`'s read-modify-write window a test can reach without the store + * exposing a seam for it. Hooking the key acquisition instead would not do: that happens inside + * `encrypt`, which takes the alias lock itself, so the racing thread would block either way and + * the test would pass against the unserialized code. + */ + private class ReadHookedPrefs( + private val delegate: SharedPreferences, + private val onFirstRead: () -> Unit, + ) : SharedPreferences by delegate { + private val fired = AtomicBoolean(false) + + override fun getString( + key: String?, + defValue: String?, + ): String? = + delegate.getString(key, defValue).also { + if (fired.compareAndSet(false, true)) onFirstRead() + } + } + + /** + * A key whose material the provider cannot fetch, which is the shape a dead binder or a busy + * keymaster takes: `Cipher.init` asks a Keystore key for its encoding, the IPC fails, and an + * unchecked [ProviderException] comes out of the cipher rather than out of key acquisition. + */ + private class UnreachableKey( + private val delegate: SecretKey, + ) : SecretKey by delegate { + override fun getEncoded(): ByteArray = throw ProviderException("Keystore backend is busy") + } + + /** An in-memory stand-in for the Android Keystore, which has no JVM provider. */ + private class FakeKeySource : SecretKeySource { + @Volatile + private var key: SecretKey = newKey() + + /** Every key minted so far, oldest first, so a test can prove a regeneration really happened. */ + val minted = CopyOnWriteArrayList(listOf(key)) + + @Volatile + var deleted = 0 + private set + + /** + * Makes [getOrCreate] fail the way an alias an OEM Keystore has dropped does. + * + * A property of the key rather than of the next call, which is what the real thing is: once + * one caller recovers by regenerating, every other caller sees the fresh valid key instead. + * A next-call-only flag would model a failure that cannot happen and would make the recovery + * race untestable. Cleared by [delete]. + */ + @Volatile + var invalidated = false + + /** Fails every [getOrCreate] with this, standing in for a Keystore that will not answer. */ + @Volatile + var getFailure: Exception? = null + + /** + * Hands back a key the *cipher* cannot use, so key acquisition succeeds and only `Cipher.init` + * fails - the one interleaving that tells a transient cipher failure apart from a lost key. + */ + @Volatile + var cipherUnreachable = false + + override fun getOrCreate(): SecretKey { + if (invalidated) throw KeyPermanentlyInvalidatedException() + getFailure?.let { throw it } + return if (cipherUnreachable) UnreachableKey(key) else key + } + + override fun delete() { + // Minting a fresh key, not just counting: handing back the very key delete claims to have + // dropped makes the round-trip assertions pass against broken regeneration too. + synchronized(this) { + deleted++ + invalidated = false + key = newKey() + minted += key + } + } + + private companion object { + fun newKey(): SecretKey = KeyGenerator.getInstance("AES").apply { init(256) }.generateKey() + } + } + + private companion object { + const val ALIAS = "KeystoreSecretStoreTest" + const val KEY = "api_key" + + /** + * Pinned here as a literal rather than read off the class: the marker is the on-disk format, + * so changing it must break a test loudly instead of silently agreeing with itself. + */ + const val ENC_PREFIX = "enc:v1:" + } +}