ADFA-5270 | Adopt shared secret store, fix terminal-tool dedup, and cover MCP flows

CLAUDE.md

@@ -46,6 +46,30 @@ There is also **one shared Gradle wrapper at the repo root** (`gradlew` + `gradl
 
 Both jars are referenced via `../libs/*.jar`. **Always use the repo-root `libs/` jars and the repo-root Gradle wrapper — never bundle per-plugin copies.** A plugin that ships its own `libs/plugin-api.jar` / `libs/gradle-plugin.jar` (e.g. copied from another plugin) can drift out of sync with the rest of the repo; point `build.gradle.kts` (`compileOnly`) and `settings.gradle.kts` (buildscript `classpath`) at `../libs/*.jar` and delete any local `libs/`. The root `plugin-api.jar` already carries the full API surface (including `IdeTemplateService`/`CgtTemplateBuilder`), so newer sub-APIs do not justify a local copy. **A plugin folder is not standalone in isolation** — copy the root `libs/` along if you move one elsewhere. When CoGo's API changes, refresh via the script above or the **Update libs from CodeOnTheGo** GitHub Action (which also commits the refreshed jars, cuts a release, and deploys `.cgp` files to the website).
 
+### Credentials: use the host's `KeystoreSecretStore`, never your own crypto
+
+A plugin that stores a credential encrypts it with `com.itsaky.androidide.plugins.security.KeystoreSecretStore`
+from `plugin-api.jar` (**26.35+** — set `plugin.min_ide_version` accordingly). It is `compileOnly`
+like the rest of the API, so there is one implementation in the IDE's process rather than a copy
+compiled into each `.cgp`. Do not re-implement AES/GCM in a plugin; three AI plugins each grew a
+copy that started to diverge, which is what ADFA-5255 removed.
+
+Construct it with **this plugin's own alias** (`KeystoreSecretStore(ALIAS)`) as a single
+top-level `val` in a `SecureApiKeyStore.kt`/`SecureTokenStore.kt` that holds nothing but the alias;
+callers use that instance directly. The store logs under its own name — it takes no log tag, and
+the second constructor parameter is a `SecretKeySource` override that plugins do not pass.
+`ai-agent-mcp`, `ai-agent-gemini` and `ai-agent-openai` are the reference shape. Do **not** wrap
+it in an object of forwarding methods — that is just a second copy of the store's contract to keep
+in step. The alias must be unique per
+plugin (all plugins share the host's UID and Keystore, so a shared alias lets one plugin's
+invalidated-key recovery delete another's secret) and must never change across releases.
+
+`readAndMigrate` returns a three-way `Stored` (`Absent` / `Value` / `Unreadable`) rather than a
+nullable String on purpose: "never saved" and "saved but this device's Keystore can no longer open
+it" need opposite advice, and a plugin that collapses them tells a user their credential was
+refused when it was never sent. Collapse it only where the caller genuinely has one answer for
+both, and say so in a comment.
+
 ### Plugin shape
 
 A plugin is an Android *application* module (despite installing as a library) with:

ai-agent-gemini/README.md

@@ -27,10 +27,14 @@ The key is entered in **AI Core → Agent settings**, not here. It is stored
 encrypted (AES/GCM under a hardware-backed Android Keystore secret) and sent as
 an `x-goog-api-key` **header**, never in a URL query string.
 
-`security/SecureApiKeyStore.kt` is the only copy of the crypto — this plugin owns
-both the write and the read, so there are no constants to keep in sync with
-another plugin. A key written under an earlier plugin id is adopted once by
-`preferences/GeminiPreferences.kt` and re-encrypted here.
+`security/SecureApiKeyStore.kt` holds only this plugin's Keystore alias
+(`cotg_ai_gemini_key_v1`); the AES/GCM itself is the IDE's `KeystoreSecretStore`
+(`plugin-api`, since **26.35** — hence this plugin's `min_ide_version`), so there
+is one implementation in the process rather than a copy per plugin. The alias
+stays per plugin: they all share the host's Keystore, so a shared alias would let
+one plugin's invalidated-key recovery delete another's secret. A key written under
+an earlier plugin id is adopted once by `preferences/GeminiPreferences.kt` and
+re-encrypted here.
 
 ## Installation
 
@@ -57,7 +61,7 @@ root of `com/itsaky/androidide/plugins/aiagentgemini/`.
 - `plugin/GeminiPlugin.kt` — plugin entry point; registers the backend with ai-core
 - `backend/GeminiBackend.kt` — the REST transport, streaming (SSE) and model catalog
 - `errors/GeminiErrorFormatter.kt` — turns an API failure into one translated sentence
-- `security/SecureApiKeyStore.kt` — AES/GCM at rest
+- `security/SecureApiKeyStore.kt` — this plugin's Keystore alias, over the IDE's `KeystoreSecretStore`
 - `preferences/GeminiPreferences.kt` — this plugin's settings store, plus the
   one-time adoption of settings written under earlier plugin ids
 - `prompt/GeminiSystemPrompt.kt` — the system prompt this cloud model is given

ai-agent-gemini/ai-agent-gemini.html

@@ -82,9 +82,9 @@ <h2>Technical architecture</h2>
     <tr><td><code>GeminiErrorFormatter</code></td><td>Classifies a failure
       (retired model, quota, refused key, outage, unreachable) so it can be
       reported as one translated sentence.</td></tr>
-    <tr><td><code>SecureApiKeyStore</code></td><td>AES/GCM encryption of the API
-      key under a hardware-backed Android Keystore secret owned by this
-      plugin.</td></tr>
+    <tr><td><code>SecureApiKeyStore</code></td><td>Binds this plugin's Keystore
+      alias to the IDE's <code>KeystoreSecretStore</code>, which AES/GCM-encrypts
+      the API key under a hardware-backed Android Keystore secret.</td></tr>
     <tr><td><code>GeminiSettingsFragment</code></td><td>The settings pane AI Core
       mounts: key entry and verification, visibility toggle, and the model picker
       driven by the live catalog.</td></tr>

ai-agent-gemini/build.gradle.kts

@@ -71,8 +71,8 @@ dependencies {
     testImplementation("org.json:json:20231013")
 }
 
-// SecureApiKeyStore is no longer duplicated: this plugin holds the only copy, so there is nothing
-// left to drift against. The parity check that guarded the ai-assistant copy went with that plugin.
+// No SecureApiKeyStore parity check any more: the AES/GCM core is the host's KeystoreSecretStore
+// (plugin-api), so there is one implementation in the process rather than copies to keep in step.
 
 // AAR metadata checks are disabled by convention for these application-as-library plugins.
 tasks.matching {

ai-agent-gemini/src/main/AndroidManifest.xml

@@ -33,7 +33,7 @@
              and pairs with ai-core, which requires the same release. -->
         <meta-data
             android:name="plugin.min_ide_version"
-            android:value="26.32" />
+            android:value="26.35" />
 
         <meta-data
             android:name="plugin.max_ide_version"

ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/backend/GeminiBackend.kt

@@ -8,7 +8,8 @@ import com.itsaky.androidide.plugins.aiagentgemini.errors.GeminiErrorFormatter
 import com.itsaky.androidide.plugins.aiagentgemini.errors.GeminiFailure
 import com.itsaky.androidide.plugins.aiagentgemini.preferences.GeminiPreferences
 import com.itsaky.androidide.plugins.aiagentgemini.prompt.GeminiSystemPrompt
-import com.itsaky.androidide.plugins.aiagentgemini.security.SecureApiKeyStore
+import com.itsaky.androidide.plugins.aiagentgemini.security.secureApiKeyStore
+import com.itsaky.androidide.plugins.security.KeystoreSecretStore
 import com.itsaky.androidide.plugins.services.LlmInferenceService.*
 import java.io.IOException
 import java.net.HttpURLConnection
@@ -116,8 +117,28 @@ class GeminiBackend(
      */
     private fun refreshKeyCache(): String? {
         val prefs = agentPrefs()
-        val plain = SecureApiKeyStore.readAndMigrate(prefs, GeminiPreferences.KEY_API_KEY)
-            ?.trim()?.takeIf { it.isNotBlank() }
+        val plain = when (val stored = secureApiKeyStore.readAndMigrate(prefs, GeminiPreferences.KEY_API_KEY)) {
+            is KeystoreSecretStore.Stored.Value -> stored.plain.trim().takeIf { it.isNotBlank() }
+            KeystoreSecretStore.Stored.Absent -> null
+            // Reported here rather than passed on as "no key": generation fails either way, but a
+            // lost Keystore entry needs the key entering again, and the log is all that says so.
+            KeystoreSecretStore.Stored.Unreadable -> {
+                context.logger.warn(
+                    "GeminiBackend: the saved API key cannot be decrypted on this device; " +
+                        "it has to be entered again in settings"
+                )
+                null
+            }
+            // Transient, so it returns without caching: the key is very likely intact, and caching
+            // this answer would freeze "no key" until the stored value itself changed.
+            KeystoreSecretStore.Stored.Unavailable -> {
+                context.logger.warn(
+                    "GeminiBackend: the keystore could not be reached to read the saved API key; " +
+                        "retrying on the next read"
+                )
+                return null
+            }
+        }
         val raw = prefs?.getString(GeminiPreferences.KEY_API_KEY, null)
         keyCache = raw?.let { it to plain }
         return plain

ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/preferences/GeminiPreferences.kt

@@ -59,7 +59,7 @@ internal object GeminiPreferences {
      * Copies this backend's settings out of every store in [LEGACY_FILES], once.
      *
      * The API key moves as ciphertext and stays readable: it is encrypted under a Keystore alias
-     * (see [SecureApiKeyStore]) rather than under anything plugin-specific, and every plugin runs
+     * (see [secureApiKeyStore]) rather than under anything plugin-specific, and every plugin runs
      * in the host's process and UID. Copies rather than moves, so downgrading still finds the old
      * values. Call before anything reads a setting.
      *

ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/security/SecureApiKeyStore.kt

@@ -1,146 +1,17 @@
 package com.itsaky.androidide.plugins.aiagentgemini.security
 
-import android.content.SharedPreferences
-import android.security.keystore.KeyGenParameterSpec
-import android.security.keystore.KeyPermanentlyInvalidatedException
-import android.security.keystore.KeyProperties
-import android.util.Base64
-import android.util.Log
-import com.itsaky.androidide.plugins.aiagentgemini.logging.LOG_PREFIX
-import java.security.GeneralSecurityException
-import java.security.KeyStore
-import javax.crypto.Cipher
-import javax.crypto.KeyGenerator
-import javax.crypto.SecretKey
-import javax.crypto.spec.GCMParameterSpec
+import com.itsaky.androidide.plugins.security.KeystoreSecretStore
+
+/** Unique to this plugin and fixed across releases; see [KeystoreSecretStore] for why both matter. */
+private const val ALIAS = "cotg_ai_gemini_key_v1"
 
 /**
- * AES/GCM encryption for sensitive settings (currently the Gemini API key),
- * 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.
+ * This plugin's binding of [KeystoreSecretStore]: its API key, encrypted under this plugin's own
+ * Keystore alias.
  *
- * The [ALIAS] must stay stable across releases: a key encrypted under one
- * alias cannot be read under another, so changing it silently invalidates
- * every stored key. It is also what lets a key written before the AI plugins
- * were reorganised still decrypt today — every plugin runs in the host app's
- * process and UID, so they all share one Android Keystore.
+ * The store is the IDE's, from plugin-api, and callers use it directly. A forwarding object per
+ * method would only be a second copy of its contract to keep in step — and one that had to pick a
+ * single answer for "absent" and "no longer decryptable", which callers here do not share. The
+ * thing this file owns is the alias.
  */
-object SecureApiKeyStore {
-    private const val TAG = "$LOG_PREFIX.SecureApiKeyStore"
-    private const val KEYSTORE = "AndroidKeyStore"
-    private const val ALIAS = "cotg_ai_gemini_key_v1"
-    private const val TRANSFORM = "AES/GCM/NoPadding"
-    private const val IV_LEN = 12
-    private const val TAG_BITS = 128
-
-    /** Marks a stored value as ciphertext; anything without it is treated as legacy plaintext. */
-    const val ENC_PREFIX = "enc:v1:"
-
-    private fun getOrCreateKey(): SecretKey {
-        val ks = KeyStore.getInstance(KEYSTORE).apply { load(null) }
-        (ks.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()
-        )
-        return generator.generateKey()
-    }
-
-    private fun deleteKey() {
-        try {
-            KeyStore.getInstance(KEYSTORE).apply { load(null) }.deleteEntry(ALIAS)
-        } catch (e: Exception) {
-            Log.w(TAG, "Failed to delete Keystore alias $ALIAS", e)
-        }
-    }
-
-    private fun encryptWith(key: SecretKey, plain: String): String {
-        val cipher = Cipher.getInstance(TRANSFORM)
-        cipher.init(Cipher.ENCRYPT_MODE, key)
-        val iv = cipher.iv
-        val ciphertext = cipher.doFinal(plain.toByteArray(Charsets.UTF_8))
-        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)
-    }
-
-    /**
-     * Encrypt [plain] into a self-describing string: [ENC_PREFIX] + 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
-     * @throws GeneralSecurityException on any other Keystore/cipher failure, so the caller can
-     *   inform the user instead of crashing the IDE on Save
-     */
-    @Throws(GeneralSecurityException::class)
-    fun encrypt(plain: String): String {
-        return try {
-            encryptWith(getOrCreateKey(), plain)
-        } catch (e: KeyPermanentlyInvalidatedException) {
-            Log.w(TAG, "Keystore key invalidated; regenerating and retrying encrypt", e)
-            deleteKey()
-            encryptWith(getOrCreateKey(), plain)
-        }
-    }
-
-    /**
-     * Return the plaintext for a stored value, handling both formats transparently:
-     * an [ENC_PREFIX] value is decrypted; anything else is returned unchanged as
-     * legacy plaintext (use [readAndMigrate] to upgrade it in place). Returns
-     * null if a ciphertext value can't be decrypted — e.g. the Keystore key was
-     * lost or invalidated — in which case the user must re-enter the key.
-     */
-    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)
-            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, getOrCreateKey(), GCMParameterSpec(TAG_BITS, iv))
-            String(cipher.doFinal(ciphertext), Charsets.UTF_8)
-        } catch (e: Exception) {
-            Log.w(TAG, "Failed to decrypt stored API key", e)
-            null
-        }
-    }
-
-    /**
-     * Read [key] from [prefs], upgrading a legacy plaintext value to ciphertext in place.
-     *
-     * Keys written before this store existed are still plaintext on disk, and [decrypt] alone
-     * hands them back unchanged forever — so an install that configured its key earlier would
-     * never actually gain encryption. Re-encrypting on the first read closes that gap without
-     * making the user re-enter the key.
-     *
-     * The value is trimmed on migration, so the stored, displayed and sent forms all agree.
-     *
-     * Keystore IPC + AES/GCM, so call this off the main thread.
-     *
-     * @return the trimmed plaintext value, or null when nothing is stored or decryption failed.
-     */
-    fun readAndMigrate(prefs: SharedPreferences?, key: String): String? {
-        val stored = prefs?.getString(key, null) ?: return null
-        if (stored.startsWith(ENC_PREFIX)) return decrypt(stored)
-        val plain = stored.trim()
-        if (plain.isEmpty()) return plain
-        try {
-            prefs.edit().putString(key, encrypt(plain)).apply()
-            Log.i(TAG, "Upgraded legacy plaintext value for '$key' to ciphertext")
-        } catch (e: Exception) {
-            Log.w(TAG, "Could not upgrade legacy plaintext value for '$key' to ciphertext", e)
-        }
-        return plain
-    }
-}
+val secureApiKeyStore = KeystoreSecretStore(ALIAS)