From cb7ee5e1ad2d5094280a8dc4e88c8623e217aab8 Mon Sep 17 00:00:00 2001 From: John Trujillo Date: Tue, 25 Aug 2026 12:45:00 -0500 Subject: [PATCH 1/4] feat(plugin-api): add KeystoreSecretStore for plugin credential storage AES/GCM under an alias-parameterized Android Keystore key, so plugins share one implementation instead of each carrying a copy. Additive: no ABI entries removed or changed. --- docs/PLUGIN_API_CHANGELOG.md | 22 ++ docs/plugin-api.md | 1 + plugin-api/api/plugin-api.api | 36 +++ plugin-api/build.gradle.kts | 5 + .../plugins/security/KeystoreSecretStore.kt | 228 ++++++++++++++++++ .../plugins/security/SecretKeySource.kt | 86 +++++++ .../security/KeystoreSecretStoreTest.kt | Bin 0 -> 14061 bytes 7 files changed, 378 insertions(+) create mode 100644 plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/security/KeystoreSecretStore.kt create mode 100644 plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/security/SecretKeySource.kt create mode 100644 plugin-api/src/test/kotlin/com/itsaky/androidide/plugins/security/KeystoreSecretStoreTest.kt diff --git a/docs/PLUGIN_API_CHANGELOG.md b/docs/PLUGIN_API_CHANGELOG.md index f35d285135..0fd49d68aa 100644 --- a/docs/PLUGIN_API_CHANGELOG.md +++ b/docs/PLUGIN_API_CHANGELOG.md @@ -58,6 +58,28 @@ 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, tag)` (`encrypt`, `decrypt`, `write`, `readAndMigrate`) and + `KeystoreSecretStore.Stored` / `.Absent` / `.Value` / `.Unreadable`. 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. + A class a plugin **instantiates**, not an interface it implements, so later additions to + it are additive. 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..2411c5536c 100644 --- a/docs/plugin-api.md +++ b/docs/plugin-api.md @@ -13,6 +13,7 @@ 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`. - **Wire/format contracts outside the module:** diff --git a/plugin-api/api/plugin-api.api b/plugin-api/api/plugin-api.api index 8fc09ea703..2690b2ab19 100644 --- a/plugin-api/api/plugin-api.api +++ b/plugin-api/api/plugin-api.api @@ -1213,6 +1213,42 @@ 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;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$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..90f138e01f 100644 --- a/plugin-api/build.gradle.kts +++ b/plugin-api/build.gradle.kts @@ -44,6 +44,11 @@ dependencies { // Test dependencies testImplementation("junit:junit:4.13.2") + + // KeystoreSecretStore reaches for android.util.Base64/Log 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..0c9ac2a09a --- /dev/null +++ b/plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/security/KeystoreSecretStore.kt @@ -0,0 +1,228 @@ +package com.itsaky.androidide.plugins.security + +import android.content.SharedPreferences +import android.security.keystore.KeyPermanentlyInvalidatedException +import android.util.Base64 +import android.util.Log +import java.security.GeneralSecurityException +import javax.crypto.Cipher +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 tag: 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. + * @param tag the logcat tag to write under, so a line names the caller that emitted it. + */ + constructor(alias: String, tag: String) : this(tag, AndroidKeystoreSource(alias, tag)) + + /** + * What was found under a preference key. + * + * Three outcomes rather than a nullable String: "nothing stored" and "stored but no longer + * readable on this device" lead to opposite advice, and collapsing them is what tells a user + * their credential was refused when it was never sent. + */ + sealed interface Stored { + /** Nothing is stored under the key. */ + data object Absent : Stored + + /** The stored value, decrypted. */ + data class Value( + val plain: String, + ) : Stored + + /** Something is stored, but this device's Keystore can no longer open it. */ + data object Unreadable : 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 + } + + /** + * 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. + * + * @param plain the value to encrypt. + * @return the ciphertext to store. + * @throws GeneralSecurityException on any other Keystore or cipher failure, so the caller can + * tell the user instead of crashing on Save. + */ + @Throws(GeneralSecurityException::class) + fun encrypt(plain: String): String = + try { + encryptWith(keys.getOrCreate(), plain) + } catch (e: KeyPermanentlyInvalidatedException) { + Log.w(tag, "Keystore key invalidated; regenerating and retrying encrypt", e) + keys.delete() + encryptWith(keys.getOrCreate(), plain) + } + + /** + * Reads a stored value back, handling both formats: an `enc:v1:` value is decrypted and + * anything else is returned unchanged as legacy plaintext. + * + * @param stored the stored string, ciphertext or legacy plaintext. + * @return the plaintext, or null when a ciphertext value cannot be decrypted, meaning the + * Keystore key was lost and the user has to enter the secret again. + */ + fun decrypt(stored: String?): String? { + if (stored == null) return null + if (!stored.startsWith(ENC_PREFIX)) return stored + return try { + 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, keys.getOrCreate(), GCMParameterSpec(TAG_BITS, iv)) + // Zeroed once the String is built; see the note in encryptWith about the String. + val plainBytes = cipher.doFinal(ciphertext) + try { + String(plainBytes, Charsets.UTF_8) + } finally { + plainBytes.fill(0) + } + } catch (e: Exception) { + Log.w(tag, "Failed to decrypt a stored secret", e) + null + } + } + + /** + * 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 { + val editor = prefs?.edit() ?: return false + if (plain.isBlank()) { + return editor.remove(key).commit() + } + return try { + editor.putString(key, encrypt(plain)).commit() + } catch (e: Exception) { + Log.e(tag, "Could not encrypt the secret for '$key'", 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, or a value that cannot be decrypted here. + */ + fun readAndMigrate( + prefs: SharedPreferences?, + key: String, + ): Stored { + 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 decrypt(stored)?.let(Stored::Value) ?: Stored.Unreadable + } + 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.i(tag, "Upgraded a legacy plaintext value for '$key' to ciphertext") + } catch (e: Exception) { + Log.w(tag, "Could not upgrade a legacy plaintext value for '$key' to ciphertext", e) + } + return Stored.Value(stored) + } + + 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..10c97b8c6b --- /dev/null +++ b/plugin-api/src/main/kotlin/com/itsaky/androidide/plugins/security/SecretKeySource.kt @@ -0,0 +1,86 @@ +package com.itsaky.androidide.plugins.security + +import android.security.keystore.KeyGenParameterSpec +import android.security.keystore.KeyProperties +import android.util.Log +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. + * @param tag the logcat tag to write under. + */ +internal class AndroidKeystoreSource( + private val alias: String, + private val tag: String, +) : SecretKeySource { + override fun getOrCreate(): SecretKey = + synchronized(KEYSTORE_LOCK) { + val store = KeyStore.getInstance(KEYSTORE).apply { load(null) } + (store.getEntry(alias, null) as? KeyStore.SecretKeyEntry)?.let { return it.secretKey } + 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.w(tag, "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() + } +} 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 0000000000000000000000000000000000000000..59c146b0e26d7f67b51b2a7dde02dfd3a9e83054 GIT binary patch literal 14061 zcmc&*OK%)kcAjnXS6nv?Ot;N0NseL+m}p>1rWB5?M?_jq;4#XVUAMbStgfomgU!xB zKxUBu0kX&Fwv{ROiW?Yu_@OFHX8(&W?5EQVS>-h@|7v(I>qwI@-)5DNgV0QL?_o{ zlUH$;uCGy7aS}Y%rTO?_^NOdMX4y0`pmaXZ(x58pY8IT!zq%|pJ~1u4ewMBD=^EGp>qw9K2rr%w;edlXecS(<+{u zlk_SsvUCnE>>8=lG^;c@LNQF@X*2MxzS6De$j~!#c7uN$b{&k1WnN_*^~``QjiLTJ z?SL~gE=+}C2M6~)_~1Z&puA%#%$u1)RWqF`t@1?2srtp6=c>Ya)rA@Bx-@ueln7ZB zMvrHvRN%GxrqL-+j^9wxxJUQp=h;-=*i}j_SY-8dh8?kBqQ2Ttx32?rj!i+e-4~i} zl8K5-RW5X%8`&ePD|HoXOmB{Omes}Be625j27v^$0)pz=?I0#?xLfHPvyRC-GR6crfHxdAf0iK_89+>R0v; zm2)}4^Ov8Vgs)zozBxWW;Z)f$Fi~6$hQT;7y1@Ssa_{yZA4n^7N1Vc+>_~i`G;q7QxA=EDop+q6HEU&9DF2lJl zE=?4U;~csr6QS&3HivS`$acYR94p}h6_xan)%(D$J?PKCSW|q|AA+>r%Bu}9co>wG zE~=7#X>ju52{?7~^z_fjsP%zSX37^?MZUsp{Z)6Ina3&h9RG+=nzQ0e)dKo&8Nfk> zDeDBMLJ$byhjRvl6-QPKD8ne8AVf3=_9ztTtWL~r`@>-nlj4)VsC9Cl4YrTjDX~^K zAjTJ2gwu3IR>X*cI`Kpd@j(siA~n%YV%<9HWVON)@*CPc8SdbAnx*0K*^|@L(9E;9 zaY$vEgROCRv8);yl$Pmmhj@#iL*HgOO;1a$gLxq8UE#60)?l^)Z?t0sVFWG zuMkb(>%e981hE?iT?SvAKMjAT%rz}F#>78@Q-njz0TA$$uIM9|N>2*|*VpSMaap~b z4DeI0pX=|wT>v+XzWdF0|Nh;7_7COhfB)Ox{O+HA{a^p_o8SHI-?>M>|MnmL{`cSh z<6pU7fBV^TGX(96 zWE*-~1KfPwRhh1$(SFy>cl$Y4kuj|FJO=`eLVCY2PI=rW#O~#JXdyZD02(pX#Ramx z#-wO57g@C2gDBug=Gm2bCCpv}5S(7<1!mi09MU4KrGB%oxV9z&z?M28krf=9Kt|T9;!04@nv3P92K^cTr6XgM9Q6q;Rb|! zg}*@=|D{nQ_26reHQ0Ct#pe%Xloo_Fq=T5Mgq z^(Ja}FqdEj@B_ywcETu>rG~#$KCH##UCw2fucJ=g4BBN>`ob*!L>JKl_!#PkxFM%G z6I64?+}$Q3Yq`r>XwUp*Xp#i8k_yo4+~8O{7-oShA7jyDmL^L-Q*nuVxRIeXYPKN~ zS!^K5*`fOE1o}Y7XYeU}E_?6JEG#d%!B$p2NcdmNro&c0(5%Ton{qnY%||-06mkjEhBg50WJqex)d@-x%|uH=XP0> z%h>cP&g#+$qO6N6TqU%4cjwu&Bx(`=g~D5=v@Kh!@g(KwNkFptYKgIdAc` z!5KYn6xglSJdPF4+S0-evB*2Gy&eB)ryA52*)d!c8CW1WL3i`jb&B)&fC@HLZNc9= zRcqzcwy8&VN6(iJ$(^2uY4@=64RZzWa%rbp=q9_c*|kt9 zMNd-1^EutaBCCi{TX1h*ISq`ck9=K`95PVHN{OQCoX z#e`0!FPNNi21_dL2K}xQzf0t|93W!Yj(I>z=yaHun2g-+h`S>_=d^coHh+Q}+o;g| zpit23?Lg14;^Q_)2M19AvGg_~Q7yN}04>5}Mtc~!7vyE0PN+^xID_J{yespe3jbRV z>YLggzeD)5v{-x0#_I`3c6( zXbeS-AZPs|y?1O-D97C;ljo+?WE%pkIS0yN+y2nGbzZ?I+ zcQ92EA~?26%rJnSyUJ?x_p}#TQ2tl8ql{{Amxf=26(B${Et1Rz-8_UP7j>LO0$z|7 z;X;7f6mSr%$D~pnXqP~E1KY8O!i_p5%KaQ4Clc;CykrBjdLbD zR9gf<%)k~lRO;tBdf03`9(&}VqLr3f?}>e9`D*nQoOp_20Rj?gK2bshnX-~f>M>X0 z1&%EABBew+3oj+1N5RyDI1hS}#`YGOmnQ;4Uby<4??pNzn*h;<<+Y-h!zbSB-65nY za#R_d%?x=pQ@$Cl!FXA8`o^ker})uS`rb+n7FYS`&q+)=1xSNtj4vAp15QDAQGvR* zDz&K+t{w;(x&b~ZsWGyv3o0lqLtC3Ix+tMV@*{M+LH)4~rxnI-YqbXO3hg7as#CqX zSOTDlM#B*D<;%%y79z1mku3zmq3Sc{Ww2-t)n#zsDKBa!e#e7UyQJTN3MCZTW~<(v z$~ml>ZPsG2uPZ_Ex|9ySHOIWr^wZqkXV|}IGOK&z7m6kl}VsQy?AJQ^?q7fX*J%$N?Q6u znJb~hjxHCA;lyZV$xgE)u4mHG9K}-PSr{UTFw|>sPWR@3A@1cf=@fLZLa0X`SdHK; zN0K3`Nm7@yBj5X|sNn9Z$sonml}l8w(HM&R;{?T4DV9Qk7MWu4C{FD))wGVwndItX z*^Dw&6QN#O@}xyH&Vr|4(O|PbZ=*FYKnt$^G%gS!u`uh=!s2^Yyk&4bPjFFRAPdP zzE8aQytX$naseq?fK#GX8%@>~)!jzK3nq6H6HpmfO|VeDY~cbY4=@6u>8NSRfx8-u z(M)SpBwK=ikIy{|%wZ5Tadm_rAai;!&i1x0BziqoLcImabP>t{|2(a&&hL;WN5 z@P4D3wy}jRnxdQ147@rNJDlD5K95thPyT6bYV*hTQt#(&5g)swNqy%MfB~Q4?4Q%g z4%oLNR>2Tx3fHUJ`ddf$FqrEcEoM!3S^ilNTnAQrO0>hO*wwW# zL)^9PS3{Jy_A`3F8oeL$Qh+lu z)F;`S8^Y`5%YlrB8uhu{?Gi=DjLrS`(Cj6b*yMKS+(=9CQ30Hc7fc~j@g(w*0EFMT zpcdJ0@jeYE57>dj=vX*?C(Vr4*{W z8%t2??usBod2o4r+JXK6Kx7=s#w7t?fIB6=5<&{(^)q^V(+$|(!*)mBRTqpd{CL1S z<6!7>UR*8R+XnUaOM_l-;{{l2H=%EXCkMI)u{V5IWF5HpJ?`e{9fF;1zWAi}Gnzra zJ->HgvBsY5HKYnbpyCfZN3{yzy8C-h9X|X}#>(*K0}pAMPkD`-T`;*2B}G&OB&CYt z^8{LVLx<;<_?-eB{Aw(q7su*6<`qrf2^>SvDEunM7fF$1feKoq6%rv77`mNz#0t!Z zM7_kMr23%bbB4lVaD#+?1UGz14M8Hx)-bFOlqxNkqCJ%ZCs^+iTcILTp@F%Mpg6?Z vb{D7;@-{S^+rDy6BzgxX=LbAYjKspd^Lc3xj)|+|qpJr;V8OeCcL)CikfipX literal 0 HcmV?d00001 From 8f739caf20a0563e3b931fe68f4f1e5a1bb75a07 Mon Sep 17 00:00:00 2001 From: John Trujillo Date: Fri, 28 Aug 2026 15:34:02 -0500 Subject: [PATCH 2/4] refactor(plugin-api): log KeystoreSecretStore through SLF4J Drops the unreleased tag constructor param; the per-plugin alias now carries the log context, and slf4j-api is already on the host runtime classpath. --- docs/PLUGIN_API_CHANGELOG.md | 2 +- plugin-api/api/plugin-api.api | 2 +- plugin-api/build.gradle.kts | 9 +++++-- .../plugins/security/KeystoreSecretStore.kt | 22 ++++++++++-------- .../plugins/security/SecretKeySource.kt | 8 +++---- .../security/KeystoreSecretStoreTest.kt | Bin 14061 -> 14025 bytes 6 files changed, 25 insertions(+), 18 deletions(-) diff --git a/docs/PLUGIN_API_CHANGELOG.md b/docs/PLUGIN_API_CHANGELOG.md index 0fd49d68aa..c793ae89f8 100644 --- a/docs/PLUGIN_API_CHANGELOG.md +++ b/docs/PLUGIN_API_CHANGELOG.md @@ -66,7 +66,7 @@ milestone. **[verified]** = read from the checked-in ABI dump. **[reconstructed] 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, tag)` (`encrypt`, `decrypt`, `write`, `readAndMigrate`) and + `KeystoreSecretStore(alias)` (`encrypt`, `decrypt`, `write`, `readAndMigrate`) and `KeystoreSecretStore.Stored` / `.Absent` / `.Value` / `.Unreadable`. 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 diff --git a/plugin-api/api/plugin-api.api b/plugin-api/api/plugin-api.api index 2690b2ab19..a772949a4d 100644 --- a/plugin-api/api/plugin-api.api +++ b/plugin-api/api/plugin-api.api @@ -1214,7 +1214,7 @@ public final class com/itsaky/androidide/plugins/extensions/UIExtension$DefaultI } public final class com/itsaky/androidide/plugins/security/KeystoreSecretStore { - public fun (Ljava/lang/String;Ljava/lang/String;)V + 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; diff --git a/plugin-api/build.gradle.kts b/plugin-api/build.gradle.kts index 90f138e01f..ff75c15828 100644 --- a/plugin-api/build.gradle.kts +++ b/plugin-api/build.gradle.kts @@ -42,11 +42,16 @@ 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/Log and a real SharedPreferences, so its - // tests need the framework on the JVM. + // 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) } 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 index 0c9ac2a09a..c07e75cb3c 100644 --- 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 @@ -3,7 +3,7 @@ package com.itsaky.androidide.plugins.security import android.content.SharedPreferences import android.security.keystore.KeyPermanentlyInvalidatedException import android.util.Base64 -import android.util.Log +import org.slf4j.LoggerFactory import java.security.GeneralSecurityException import javax.crypto.Cipher import javax.crypto.SecretKey @@ -19,7 +19,7 @@ import javax.crypto.spec.GCMParameterSpec * the others silently do not get. */ class KeystoreSecretStore internal constructor( - private val tag: String, + private val alias: String, private val keys: SecretKeySource, ) { /** @@ -27,10 +27,10 @@ class KeystoreSecretStore internal constructor( * 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. - * @param tag the logcat tag to write under, so a line names the caller that emitted it. + * 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, tag: String) : this(tag, AndroidKeystoreSource(alias, tag)) + constructor(alias: String) : this(alias, AndroidKeystoreSource(alias)) /** * What was found under a preference key. @@ -65,6 +65,8 @@ class KeystoreSecretStore internal constructor( 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) } /** @@ -83,7 +85,7 @@ class KeystoreSecretStore internal constructor( try { encryptWith(keys.getOrCreate(), plain) } catch (e: KeyPermanentlyInvalidatedException) { - Log.w(tag, "Keystore key invalidated; regenerating and retrying encrypt", e) + log.warn("Keystore key '{}' invalidated; regenerating and retrying encrypt", alias, e) keys.delete() encryptWith(keys.getOrCreate(), plain) } @@ -116,7 +118,7 @@ class KeystoreSecretStore internal constructor( plainBytes.fill(0) } } catch (e: Exception) { - Log.w(tag, "Failed to decrypt a stored secret", e) + log.warn("Failed to decrypt a stored secret under alias '{}'", alias, e) null } } @@ -146,7 +148,7 @@ class KeystoreSecretStore internal constructor( return try { editor.putString(key, encrypt(plain)).commit() } catch (e: Exception) { - Log.e(tag, "Could not encrypt the secret for '$key'", e) + log.error("Could not encrypt the secret for '{}' under alias '{}'", key, alias, e) false } } @@ -190,9 +192,9 @@ class KeystoreSecretStore internal constructor( // 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.i(tag, "Upgraded a legacy plaintext value for '$key' to ciphertext") + log.info("Upgraded a legacy plaintext value for '{}' to ciphertext", key) } catch (e: Exception) { - Log.w(tag, "Could not upgrade a legacy plaintext value for '$key' to ciphertext", e) + log.warn("Could not upgrade a legacy plaintext value for '{}' to ciphertext", key, e) } return Stored.Value(stored) } 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 index 10c97b8c6b..26ad1d16d0 100644 --- 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 @@ -2,7 +2,7 @@ package com.itsaky.androidide.plugins.security import android.security.keystore.KeyGenParameterSpec import android.security.keystore.KeyProperties -import android.util.Log +import org.slf4j.LoggerFactory import java.security.KeyStore import javax.crypto.KeyGenerator import javax.crypto.SecretKey @@ -33,11 +33,9 @@ internal interface SecretKeySource { * The real source: a hardware-backed key held under [alias] in the Android Keystore. * * @param alias the Keystore alias holding the key. - * @param tag the logcat tag to write under. */ internal class AndroidKeystoreSource( private val alias: String, - private val tag: String, ) : SecretKeySource { override fun getOrCreate(): SecretKey = synchronized(KEYSTORE_LOCK) { @@ -63,7 +61,7 @@ internal class AndroidKeystoreSource( } 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.w(tag, "Failed to delete Keystore alias $alias", e) + log.warn("Failed to delete Keystore alias '{}'", alias, e) } } } @@ -82,5 +80,7 @@ internal class AndroidKeystoreSource( * 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 index 59c146b0e26d7f67b51b2a7dde02dfd3a9e83054..2431c7093d2d47de12c68820b5e94a63ff57f81d 100644 GIT binary patch delta 118 zcmaExdop*!a;C}qm^`@^@=Hq;$}$tV6tp%AF^e#9JNkG!2J0wfr&bnEZj_MT?8TbO z!wMFhd_YBX^Kn5X4g`1daY4z=Qc{b#AVQlDsQh5sd|v$ Date: Mon, 31 Aug 2026 09:42:24 -0500 Subject: [PATCH 3/4] fix(plugin-api): address review findings on KeystoreSecretStore All ten findings from the review at 8f739ca. Both IMPORTANT ones were in the published contract of a class three plugins are about to adopt, so they are fixed rather than documented. encrypt's @Throws(GeneralSecurityException) did not cover every way it can fail: KeyStore.load(null) declares IOException and AndroidKeyStore keygen raises the unchecked ProviderException, so a plugin catching exactly what the KDoc documents still took an uncaught throwable - in the host's process, so it surfaced as an IDE crash. Anything that is not already a GeneralSecurityException is now wrapped in one, cause preserved; a KeyStoreException still passes through unwrapped. The invalidated-key recovery was a delete-then-create-then-encrypt compound with no serialization, so two concurrent callers could leave one holding a ciphertext nothing could ever decrypt. readAndMigrate was a read-modify-write against SharedPreferences with the same gap, so a write committing mid-upgrade was silently reverted. Both now run under one alias-scoped, process-wide monitor shared with write - the same scope AndroidKeystoreSource already uses for its own read-then-generate race, and for the same reason. Also from the review: - Stored.Value overrides toString, so a Stored reaching a log line or a crash breadcrumb can no longer print the decrypted secret. - decrypt no longer collapses "the key is lost" into "the Keystore would not answer". A new Stored.Unavailable reports the transient case, so a keystore that is not ready yet does not tell the user to retype an intact credential. Free to add now: Stored is sealed and unreleased with no consumers, which is exactly why the changelog entry now says a later case needs a breaking row. - AndroidKeystoreSource logs the alias and the unexpected entry class before replacing a non-SecretKeyEntry, instead of silently overwriting whatever a colliding plugin left there. Test harness: - The two raw NUL bytes are now unicode escapes. Identical bytes once compiled, but the file is text to git again, so the suite has a reviewable diff on GitHub (568 added lines vs origin/stage, previously Bin 0 -> 14025 bytes). - FakeKeySource.delete() mints a fresh key instead of only counting, and invalidation is modelled on the key rather than the next call - a next-call-only flag models a failure that cannot happen and is what made the recovery race untestable. - Three new regression tests, each confirmed to fail against the unfixed code for the reason it is named for: concurrent recovery (deleted=2, was 1), a write racing a legacy upgrade (reverted to legacy-secret), and the non-GeneralSecurityException wrap (raw IOException escaped). The Unavailable split and the toString redaction were verified the same way. 36 tests, 0 failures, the two concurrency cases repeated 6x. Whole module: 76 tests, 0 failures. plugin-api.api regenerated: 43 additions, 0 deletions vs origin/stage, apiCheck green. Docs: the 26.36 entry claimed later additions to KeystoreSecretStore are additive. That holds for the class, which plugins instantiate, but not for the sealed Stored the same entry introduces - a new case breaks every plugin's exhaustive when at compile time and throws NoWhenBranchMatchedException in an already-built .cgp. The entry now says a new Stored case needs a breaking row, including the enc:v2: state its own future-proofing note leaves room for, and plugin-api.md lists Stored among the sealed types plugins reference. CI, the last finding: debug.yml ran only :plugin-api:apiCheck, so the suite never ran on a PR at all (analyze.yml is schedule/dispatch only). Adds a step beside apiCheck, running testDebugUnitTest rather than testV8DebugUnitTest because this module declares no product flavors. jacocoAggregateReport had the matching gap: it collected only testV8DebugUnitTest and its class and exec paths were v8Debug-only, so mapNotNull returned nothing for a flavorless module and dropped it from the report entirely. It now carries a (variant, task) pair list, v8 first, and collects both shapes. Verified by diffing the generated report against the old wiring: 286 -> 292 packages (the six com.itsaky.androidide.plugins.* packages were absent altogether), line 1957/79722 -> 2147/80459, branch 312/44837 -> 361/44961, with KeystoreSecretStore itself at 99% line / 93% branch. The task graph gains :plugin-api:testDebugUnitTest, which the old wiring did not contain (0 occurrences in --dry-run before, present after). sourceDirectories is deliberately left naming src/main/java only, though nine modules keep sources under src/main/kotlin: it 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 buys nothing. Measured rather than assumed, and commented in place so the next reader does not "fix" it. --- .github/workflows/debug.yml | 7 + build.gradle.kts | 67 ++++-- docs/PLUGIN_API_CHANGELOG.md | 24 +- docs/plugin-api.md | 2 +- plugin-api/api/plugin-api.api | 7 + .../plugins/security/KeystoreSecretStore.kt | 227 ++++++++++++------ .../plugins/security/SecretKeySource.kt | 14 +- .../security/KeystoreSecretStoreTest.kt | Bin 14025 -> 21602 bytes 8 files changed, 249 insertions(+), 99 deletions(-) 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 c793ae89f8..043f3703cd 100644 --- a/docs/PLUGIN_API_CHANGELOG.md +++ b/docs/PLUGIN_API_CHANGELOG.md @@ -67,12 +67,24 @@ milestone. **[verified]** = read from the checked-in ABI dump. **[reconstructed] 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`. 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. - A class a plugin **instantiates**, not an interface it implements, so later additions to - it are additive. The `alias` is a constructor parameter and must stay **distinct per + `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. + 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 diff --git a/docs/plugin-api.md b/docs/plugin-api.md index 2411c5536c..d558e7f536 100644 --- a/docs/plugin-api.md +++ b/docs/plugin-api.md @@ -15,7 +15,7 @@ The surface a plugin binds to is broader than one module. All of the following a - 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 a772949a4d..4ec832d1f9 100644 --- a/plugin-api/api/plugin-api.api +++ b/plugin-api/api/plugin-api.api @@ -1231,6 +1231,13 @@ public final class com/itsaky/androidide/plugins/security/KeystoreSecretStore$St 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 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 index c07e75cb3c..d19e30f07f 100644 --- 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 @@ -5,6 +5,7 @@ import android.security.keystore.KeyPermanentlyInvalidatedException import android.util.Base64 import org.slf4j.LoggerFactory import java.security.GeneralSecurityException +import java.util.concurrent.ConcurrentHashMap import javax.crypto.Cipher import javax.crypto.SecretKey import javax.crypto.spec.GCMParameterSpec @@ -32,12 +33,30 @@ class KeystoreSecretStore internal constructor( */ 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. * - * Three outcomes rather than a nullable String: "nothing stored" and "stored but no longer - * readable on this device" lead to opposite advice, and collapsing them is what tells a user - * their credential was refused when it was never sent. + * 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. */ @@ -46,10 +65,24 @@ class KeystoreSecretStore internal constructor( /** The stored value, decrypted. */ data class Value( val plain: String, - ) : Stored + ) : 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. */ + /** + * 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 { @@ -67,6 +100,15 @@ class KeystoreSecretStore internal constructor( 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() } } /** @@ -77,17 +119,32 @@ class KeystoreSecretStore internal constructor( * * @param plain the value to encrypt. * @return the ciphertext to store. - * @throws GeneralSecurityException on any other Keystore or cipher failure, so the caller can - * tell the user instead of crashing on Save. + * @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 = - 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) + 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) + } } /** @@ -95,33 +152,11 @@ class KeystoreSecretStore internal constructor( * anything else is returned unchanged as legacy plaintext. * * @param stored the stored string, ciphertext or legacy plaintext. - * @return the plaintext, or null when a ciphertext value cannot be decrypted, meaning the - * Keystore key was lost and the user has to enter the secret again. + * @return the plaintext, or null when 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? { - if (stored == null) return null - if (!stored.startsWith(ENC_PREFIX)) return stored - return try { - 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, keys.getOrCreate(), GCMParameterSpec(TAG_BITS, iv)) - // Zeroed once the String is built; see the note in encryptWith about the String. - val plainBytes = cipher.doFinal(ciphertext) - try { - String(plainBytes, Charsets.UTF_8) - } finally { - plainBytes.fill(0) - } - } catch (e: Exception) { - log.warn("Failed to decrypt a stored secret under alias '{}'", alias, e) - null - } - } + fun decrypt(stored: String?): String? = (readStored(stored) as? Stored.Value)?.plain /** * Stores [plain] under [key], encrypted; a blank value removes the entry instead. @@ -141,15 +176,17 @@ class KeystoreSecretStore internal constructor( key: String, plain: String, ): Boolean { - val editor = prefs?.edit() ?: return false - if (plain.isBlank()) { - return editor.remove(key).commit() - } - return try { - editor.putString(key, encrypt(plain)).commit() - } catch (e: Exception) { - log.error("Could not encrypt the secret for '{}' under alias '{}'", key, alias, e) - false + 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 + } } } @@ -169,34 +206,88 @@ class KeystoreSecretStore internal constructor( * * @param prefs where the value lives. * @param key the preference key. - * @return what was found: nothing, the plaintext, or a value that cannot be decrypted here. + * @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 { - 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 decrypt(stored)?.let(Stored::Value) ?: Stored.Unreadable + 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) } - 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 + } + + /** + * Classifies a stored string, keeping a lost key apart from an unreachable Keystore. + * + * The two failures are caught separately and deliberately: only the ciphertext-side failures + * mean the value is gone, and reporting a momentarily unavailable Keystore the same way is what + * asks a user to retype a credential that is still perfectly readable. + */ + private fun readStored(stored: String?): Stored { + if (stored == null) 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: 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) { + log.warn("Failed to decrypt a stored secret under alias '{}'", alias, e) + Stored.Unreadable + } } - 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) + } + + 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) } - return Stored.Value(stored) } private fun encryptWith( 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 index 26ad1d16d0..5e24788159 100644 --- 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 @@ -40,7 +40,19 @@ internal class AndroidKeystoreSource( override fun getOrCreate(): SecretKey = synchronized(KEYSTORE_LOCK) { val store = KeyStore.getInstance(KEYSTORE).apply { load(null) } - (store.getEntry(alias, null) as? KeyStore.SecretKeyEntry)?.let { return it.secretKey } + 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 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 index 2431c7093d2d47de12c68820b5e94a63ff57f81d..0c90fcc00f0798e6e1c04c01682e53e654f57fe4 100644 GIT binary patch delta 6776 zcmaJ`-)|gO6_!g0-3=8qp{?tr&b8HsUB^3dnnV$9-8ONYHYBk_Y&XzQt?$m<-M!81 z+|8XCZ?>&9;(-T*#LN5vfGQAzmr8y_Jo3OZyi%!1JR(sZDUC z?m6H2&Udc=()-8nrat*>>NRb=>l~*?8D(Yj$;@?CAR=tEGt!J>NuF)2b-(R!x7n5#7ZG>aw+RcvHv8S5FxF zq4uvxYqheYkvGZG$JgG{J+&R_uU=0krl)=HGXHnQm{7_6FQ$HZBs+Unti`_Ut9VgF zCJ|N+#L~m1rDc%}j0j;coFUe?Z;IA1QL#v@?u(92z8i~{j-(xmu7cy@MEi?EMuAAW zI<8hK6)ZgHVjmHwSOl+CB)T%jCc>&+tp>7%kJkd-ZVMBMw{-G$+VaHp0RGZG_H_Hf zhf4US*Vmy6JX(1Ez8d~GUBbVCPP*>5{g0oTdSWWb|4{u!MmukHm1xWRO7wJ;#6pU; zRdH8fChZldP$g(%3URoL*iN1_u z9d$%Y`4Z!KxLFGW?4iT}XT_iZ%e16_U!;+b$BsJ$CMuh3Vapm;jy*C{1$7S&+SDJY z+AH~k=U>Ri0BTz0n_Xpx&9*hYri_|8+LfUWB&Id7MDs5E+k|Puriy$!>?h4FdIL)^ zx5Jnw%r~*`u8NzA7lWHaWp`?I@u53iF&Dwbh8gcDH2KfDtbq-phHn+Nb-x!AI6@|k z0t65zv4GdE3KrQT$Se~DZhf~QmUrukz?wd}9&YC2u%r38t!)+~H&gs@-YMaQ$ZCtX zZY}sZyblWpvIr1?0tWlKk2odjVM4I(QkZ;%jLwbC*+ZV#R93@_ACRd4D}U@L00tIW zl?i7$_KO03=MFK(65%~6mLy2Qjzqwxgj1Xc%fMBbx*{$VZ|Hf5=heylt`+0z#UPU= z7$PqbQ*5-?(=d!*+eG3;o!2LaGIAzxfH{dxRW<+ATixW1Jh(+CHCh+{u~561fnV7m46@k&$S|(EEb;Fjtc7mYtawWj*bA(#3!V> z9n?eVCkVrUhzUtp9tI17Z9||71P8=Lw3AFbU7{~*9IrM6)*~yrCMK@#+sybXjtQ9| z*m|Q~n>bdDIF&;KtcPm8zUXY8G?~ZrWQ^>dN1Vs;cih)_oiIYnc*vk@F2KMVf34!% zi83S1AQCGyxI#Qj#8b)tee`5TZ^-lfY0o)B)YLL!cX;R=YhjjhQ)rjU1hcX?B7)?) zNhph4A{-boH|N31Q?a6Q#m*{GRD$VRX~3SOB5xtu>SvmK$H>yY153T)H3fFAR|PhUEI zh&!?8nGDtdfaAdg4hr>%T>TWa$cP>lKt>`+UnIl6dqT2vjRil+j?|II)pE?2Zh0a= zS{Q{1Ff1y&5}?l?4#9b#RoV{4-S?FWARbUmV>u+Aov>_RQk27%0>(p4+sni)?*o=a zny_(QrEUJ-Q`ozp@4*Jp%Ow6LD_I=N0dS&CZ0N|uj9Hz(Ss;J{Rs>*6CrhL zqX2);RbUJ*#lpBMyV8WwqD36f8>4z4h$I~HpUl0Rug|=kF+l~*cj$|wM8|@COd=J4 z6R-k6i6=fMUPYu4JQYk(Bc0P_`UlFAl^SvAhrj{X%2wKLD;rZZ$1nLb85tPDK4o7; zI15QZBFVVgoj0YmrNR zoAz2>{>PpzM?q03LsP&HD8LQy0<!x^V`S3|nxr(S$H|JTg) zw38UTum#a>vDTn^{Q2|?8D)FWXbnlFHK0ww*<8g*!NR4eUbi&%#$o8uWuh}mu-jO= zp`58kQFjR_PRNofWQW9{N~(LFxT)A*1p;u?4C|&5MY61742A)Olo>3eV8No1@`8z? z@C1@CM7>6Gp+|sxD~N5xSZ)WNK(xW?kdNw;5W!=mr!ycpwQcD$K8cus=u(w+jiIW{ zHRujb|29A0wyGW1`t-9bEY7!Hp@OhC$_ojqE1{vfzG1H-{cCO%OQ$_zEBH@*gS?8M zKKdLeWeA1MnGCq1?;!i~-!$g#Vs%8kMoC|G5F@GXU<@xaiaDY!*KZ0a*@E?6 zoe(-q8=EGze*V|SrElnjoxXqCTYuuoHeT-!mwq}mTX?{iq=VSUMLOaGD*X>;PvqZv z{>U?n{OGVY{<1z(J>tI5z-*!r@LO^pm%dTm#A(7%_PGrMg&7now0MrZna+V*2clN$ zn3W6zDxrb!9xO*dLx+x)C*r`?dl^VZLH~~XvJ0?)f_!#ip;{3O;x5n-1J2N6g0waE z0xb}_&}K@}Gw92S+bp-f=p%I8rDg;4Dxnq8+On&uGeHx|F(4x5{z*p}lpr=icbX8K zaICJuu#SNELbM<4UnnVDA0b*4|M=C%(hT{Foa93jd!b)JKPVc!g+kMWd_oh&HWVD4 z_3+%c5RPOsxS2mXH(@UL8ybP-X`)Dvk_EUlkZ=JxR4kmWR=}>iL^EyB!6C_$EEmM( za*lE*z*$jelvv5cCRC~Fd6A3^U2~unIU~yZ4sUa9bz}YNCJ25V-{aav&aN&PMAQyW zd~LLFk>0v3Y6&GbtXnDh)7zwH@5;@M)f;QsRq@eB;>*Ttt=+kGef{#*+SNLeD@koc zICeDb4Ydjb)|=Eju@V)Rx#@@F)~q$QHqTkf6`>5A>m@+~=p5ke3=ZX_qsD9!c2{wM z(UA#3fe8!qQEG{C@0sWtb;rbO?*W896o=K+8^|_r#W8Z_cn4b674DtF{8%*TI|G7l zF!~uN3av-PifD(pXMpZjh?|3&alC(CLF;7+ap->VemfbQi&yq@QEDi9`+4q zu0*FTdm~sAA=BA18jb=Zr2in*v1Y)Q+2Di*mFvKeHiHAY)D8lc&SKtTrzl@L)ZlCM ztFCFHg~wD$%&Ee5aUle%EP|4`l3IvZN-8mtD98uig4W{Wx^Z+V!K!Sh-e4`vNUgEXCy2VN`3&5d9KWFOmb6Br|3B2vaqDA;pdD_~POX=I=nY z-y!+>$lLnp$(4)J6t8oyL?~U7Ey6}OyY69g<2uI*WigwG>sFRzfGCkR8weGH3z2#q zi7?6v*97k6>Xn~oZ@yzfq)|u>WO?XaX$f;Mie>Z-z;6W&&j7!;at-bmYdG|fq3Q0X zh|t^8E(#L{R69Av3Ys6K0+@7{6!1v-ZxaC{L8zH<*v0K2vnGp(Vrg6!9ddhIE2=6? zt#cekXUR9t*QiHC<+#N+1ose?4ZDhs67?R;M6Dvs64|5fxHP~o8*8`d9)#1A%N1SD zK}B*iL-kRh=FzH`sV4NH&PJ6MMm%ycYBE=WM}1< zr>H=MXu(}GDZ--Xz>Gz{$ix0Ph%E=l3W|eH*0vel_5!^+;2;&8;SK@&$*u-%(J&Ew z-wtI*xaWY#@` zH{#%IL4mA{0*BuTd=o8d{@M9@mC2JF73b9p^Q!~8`XO^<6r(*T@bdT~P@|=$#qCnKQI?Wx~_&3^NA5CpeFC{uHT- mN~K7~Gb;ordP8-cYH~ifIQw*k$gd=c{C}_9-ZvNDo%%l>F9^l} delta 524 zcmZWmJ5L)y5avvHtbrf|gaRWbq_Hs=0g4a>g$X1DU@jr4kc)*Ia|_FzwYT|?3+Macw_AsPIgI90V zVA2)~?YEhaX(!cH!Xy0#HJ$c;ruCtV{t6e;*02Ox)i6z81~Vix&rEsvl06!_Jwv8E zo;)$vlV3ZZ{=aI@XY-Ev`SL>#@#6D7$iDXx)-Z{5sOYA+kSPY93We8d;Z|5vyEp~0 zFLr%1vRq1aXkQ^3v2x~6u-ZqB{3!hZ*3DQ!M5Ry@UwA>E76<5N{j9&LM4fAdI+tK0 z($zbc=1SRpcx?kkpsjddqr&zc>P_`Zp#_(l?U*OP1K3>Mc(8|HomHzM*lPl~6g+}B ztVyo`lQU_?>Rf>ctd#I9&BF=D+#I>?#Egg_5{FfpKrP^PE6;Z%{lEc_ikb2 Date: Mon, 31 Aug 2026 11:22:58 -0500 Subject: [PATCH 4/4] fix(plugin-api): keep permanent and transient secret failures apart Four review findings, all on the read path's classification. Invert the cipher-step catch. Every failure out of decryptWith mapped to Stored.Unreadable, so a transient Keystore failure with intact ciphertext told the caller its credential was permanently lost - the exact outcome Unavailable was added to prevent. Enumerate the permanent failures instead (bad GCM tag, malformed payload, invalidated key) and default everything else to Unavailable. Testing for transience directly is not available here: BackendBusyException is API 31+ and KeyStoreException.isTransientFailure() API 33+, against minSdk 28. Map an unrecoverable key entry to Unreadable. The mirror of the above on the key-acquisition arm: UnrecoverableKeyException/UnrecoverableEntryException mean the alias exists but its material does not, which is permanent, and reporting the transient Unavailable has a conforming caller retry forever and never re-prompt. One catch of UnrecoverableEntryException covers both shapes. Apply the blank rule in readStored. decrypt("") returned "" while readAndMigrate purged the identical bytes and reported Absent, so a plugin testing decrypt(...) != null would send an empty API key where the same plugin on readAndMigrate correctly finds nothing. Both entry points now share the rule write already applied. encrypt/decrypt as a bare codec still round-trip "". Document encrypt and decrypt as off-main-thread, matching write and readAndMigrate. Both route through getOrCreate()'s binder IPC, and decrypt now takes the alias lock that write holds across a synchronous flush. Each fix is pinned by a test confirmed to fail against the unfixed code for the reason it is named for; reverting the inversion alone also fails the two pre-existing Unreadable tests, so a botched inversion cannot pass. apiCheck is unchanged - the classifier is private and no signature moved. --- docs/PLUGIN_API_CHANGELOG.md | 14 ++- .../plugins/security/KeystoreSecretStore.kt | 70 +++++++++++++-- .../security/KeystoreSecretStoreTest.kt | 87 ++++++++++++++++++- 3 files changed, 159 insertions(+), 12 deletions(-) diff --git a/docs/PLUGIN_API_CHANGELOG.md b/docs/PLUGIN_API_CHANGELOG.md index 043f3703cd..744ea89ed8 100644 --- a/docs/PLUGIN_API_CHANGELOG.md +++ b/docs/PLUGIN_API_CHANGELOG.md @@ -83,7 +83,19 @@ milestone. **[verified]** = read from the checked-in ABI dump. **[reconstructed] 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. + 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 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 index d19e30f07f..e6cc909513 100644 --- 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 @@ -5,8 +5,11 @@ 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 @@ -117,6 +120,9 @@ class KeystoreSecretStore internal constructor( * 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 @@ -151,10 +157,15 @@ class KeystoreSecretStore internal constructor( * 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 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. + * @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 @@ -240,14 +251,20 @@ class KeystoreSecretStore internal constructor( } /** - * Classifies a stored string, keeping a lost key apart from an unreachable Keystore. + * Classifies a stored string, keeping a value that is really gone apart from one the Keystore + * merely would not open just now. * - * The two failures are caught separately and deliberately: only the ciphertext-side failures - * mean the value is gone, and reporting a momentarily unavailable Keystore the same way is what - * asks a user to retype a credential that is still perfectly readable. + * 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 = @@ -256,6 +273,13 @@ class KeystoreSecretStore internal constructor( } 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 @@ -263,12 +287,40 @@ class KeystoreSecretStore internal constructor( try { Stored.Value(decryptWith(key, stored)) } catch (e: Exception) { - log.warn("Failed to decrypt a stored secret under alias '{}'", alias, e) - Stored.Unreadable + 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, 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 index 0c90fcc00f..9cc1cab1bd 100644 --- 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 @@ -15,6 +15,7 @@ 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 @@ -160,7 +161,20 @@ class KeystoreSecretStoreTest { @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("")).isEqualTo("") + 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 @@ -280,6 +294,57 @@ class KeystoreSecretStoreTest { .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 @@ -506,6 +571,17 @@ class KeystoreSecretStoreTest { } } + /** + * 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 @@ -533,10 +609,17 @@ class KeystoreSecretStoreTest { @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 key + return if (cipherUnreachable) UnreachableKey(key) else key } override fun delete() {