Skip to content

Keys: transient Keystore errors delete pairing keys, and private keys are decrypted on the main thread just to show IDs #116

Closed

Description

@dzid26

Problem

A transient error deletes a valid key

PairingKeyStore.loadInternal deletes the car's key whenever decryptWithKeystore returns null (PairingKeyStore.kt#L67-L91). `decryptWithKeystore` wraps everything in `runCatching` ([L155-L171](https://github.com/dzid26/TeslaBatteryBLE/blob/18f1b464d84c65a0de00261ea02a2a48334532f9/app/src/main/java/com/dzid26/teslable/ble/PairingKeyStore.kt#L155-L171)).`` Any transient failure — a KeyStoreException, a ProviderException from keystore2, a Binder hiccup — is handled like "restored from another device", and the valid key is deleted for good. The user then has to find the NFC card and pair again, and the old key stays enrolled in the car.

The private key is decrypted far more often than needed, and in the wrong places

load() decrypts the private key, and it is called:

  • at startup on the main thread, for every vehicle, only to check whether a key exists (TeslaBleController.kt#L95);``
  • inside Compose composition in Settings, to display key IDs (SettingsScreen.kt#L56-L65),`` although a key ID is just SHA-1(publicKey)[:4] and needs no private key;
  • several times per connection and poll (startSession, sendSessionRequests, handleSessionInfo, requestKeySlot, handleWhitelistInfo, handleWhitelistEntry), on Binder threads (BLE: GATT callbacks mutate shared state from Binder threads, and GATT operations aren't serialized #110).

Each call is a Keystore AES-GCM decrypt and another chance to hit the delete path above. Simply opening Settings can delete a key.

Other storage problems

  • Key writes use SharedPreferences.apply() (L112).`` pairWithFreshKey() generates a key and sends `addKey` immediately. If the process dies before the asynchronous write lands, the car has enrolled a key the app no longer has, and that whitelist slot is orphaned.
  • setBackupEnabled re-encrypts every key on the UI thread, from a Settings click handler.
  • VehicleStore.parse returns emptyList() on any JSON error (VehicleStore.kt#L41-L61).`` The next save() overwrites the stored list, and VINs and key slots are lost without any error.
  • The docs disagree with the code: docs/protocol/session-and-pairing.md says Keystore material "is decrypted and migrated transparently on first load", while the class KDoc says "loading never converts between modes".

Proposal

  • Delete a key only on definite signals: AEADBadTagException, KeyPermanentlyInvalidatedException, UnrecoverableKeyException, or a missing Keystore alias. Surface other errors as retryable.
  • Add publicKey(vehicleId) / keyId(vehicleId) that never touch the private key, cache them per link, and load the private key only for the ECDH step.
  • Persist a fresh key with commit() (or a transactional store) before sending addKey.
  • Move Keystore work off the main thread, and stop calling storage from composables.
  • Make VehicleStore fail loudly on a parse error: keep the raw blob, log it, and don't overwrite it.
  • Add unit tests with a fake Keystore wrapper, via Robolectric or a small interface seam.

Activity

  1. dzid26 commented on Nov 1, 2026

    @dzid26
    CollaboratorAuthor

    Closing: both failures in the title are fixed on main.

    • A transient Keystore error no longer deletes the key. PairingKeyStore.loadInternal removes a key only when isDefiniteKeyLoss() says this install can never read it: AEADBadTagException, KeyPermanentlyInvalidatedException, or an UnrecoverableKeyException whose cause is not a transient KeyStoreException on Android 13+ (PairingKeyStore.kt:109-134, :222-233). Every other failure keeps the key for the next attempt. Landed in Keep pairing keys through transient Keystore errors #135, extended in Keep keys through transient Keystore errors and drop stale paired state #138.
    • No decrypt just to show an ID. hasKey, publicKeyRaw and keyId read the stored public key only, and keyId is SHA-1(publicKey)[:4] (PairingKeyStore.kt:76-85). Settings lists keys through keyId (SettingsScreen.kt:94-100), the startup staleness check uses hasKey (TeslaBleController.kt:212), and only the ECDH step calls load() (TeslaBleController.kt:1232).
    • Key on disk before enrolment. save() uses commit(), and generate() returns null when the write fails, so addKey is never sent for a key the app does not have (PairingKeyStore.kt:99-107, :156-164).
    • Keystore work off the UI thread. setBackupEnabled runs on Dispatchers.IO (SettingsScreen.kt:108-110).
    • VehicleStore keeps the raw list. An unparseable list is copied to vehicles_unreadable with commit() before a save can replace it (VehicleStore.kt:23-36).

    One proposal item is not done: a JVM test for the key-loss classifier. AndroidKeyStore has no unit-test stand-in, so it needs the key-handle seam from #119 (or a fake wrapper). Recorded here; the issue does not need to stay open for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions