Repository navigation
Keep pairing keys through transient Keystore errors - #135
Merged
Merged
Conversation
PairingKeyStore deleted a car's key whenever a Keystore decrypt failed, and the app decrypted private keys just to check that a key exists or to show its ID: at startup on the main thread, in Settings composition, and several times per connection. A transient Keystore or Binder error could therefore cost a re-pairing and leave the old key enrolled in the car. PairingKeyStore: - Add hasKey(), publicKeyRaw() and keyId(), which read only the stored public key (key ID = SHA-1(public)[:4], as in TeslaKeyPair.keyId). - Delete a key only on AEADBadTagException, UnrecoverableKeyException or KeyPermanentlyInvalidatedException; any other failure returns null and keeps the key. A decrypt no longer creates the Keystore AES key: a failed lookup used to mint a new key under the same alias, replacing the one that wraps every stored private key. A missing Keystore key is reported as UnrecoverableKeyException (material from another install). - Write keys with commit(); generate() returns null when the key could not be stored, so addKey is never sent for a key the app may lose. - Serialize private-key access across instances (controller, Settings). Callers: - TeslaBleController uses the public-key accessors everywhere except handleSessionInfo, which needs the private key for the ECDH step. The startup stale-slot check uses hasKey(). - Pairing stops with an error when a fresh key cannot be saved. - When the store drops a key this phone can never decrypt (for example a device-only key restored from another phone), handleSessionInfo forgets its cached slot so the car shows unpaired and can pair again; the startup check no longer decrypts, so it cannot catch that case. - Settings shows key IDs from the public keys and runs setBackupEnabled on Dispatchers.IO; the switch then shows the stored mode. VehicleStore keeps a list it cannot parse under vehicles_unreadable (committed before any save() can replace the list) and logs the failure without the list contents, since the parser message quotes VINs. Part of #116. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0129RzhbEE2cboMcYyZMuxqX
On Android 12+ data_extraction_rules.xml does not exclude pairing_key.xml, so a backup can contain device-only keys. They stay wrapped by a Keystore key that never leaves the phone, and PairingKeyStore drops them on load anywhere else. Android 11 and below exclude the file (backup_rules.xml). Say that a backup can't restore device-only keys on another phone, rather than that they stay out of backups, and that usable copies already in Android backup may remain after backup is turned off. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0129RzhbEE2cboMcYyZMuxqX
The "Include vehicle keys in Android backup?" dialog said each device-only key "never leaves" the device. On Android 12+ a backup can hold a wrapped copy, which no other phone can decrypt. Say instead that only this phone can use the key, and that turning backup on lets Android backup restore the keys on a new phone, matching the status line and the "Stop backup" dialog. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0129RzhbEE2cboMcYyZMuxqX
On Android 11 and below, backup_rules.xml always leaves pairing_key.xml out of backups, so turning the switch on there only stored the keys without Keystore encryption and made nothing restorable. Below Android 12, show the switch off and disabled with a line saying key backup needs Android 12 or newer, and never open the enable dialog. A setting already turned on there keeps its key storage (pre-1.0: no migration; reinstall to reset), and its status text no longer claims a restore. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0129RzhbEE2cboMcYyZMuxqX
Contributor
ScreenshotsCaptured from the demo build at 5fe0410. |
Collaborator
Author
Good suggestions |
2 of 13 tasks
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Summary
Part of #116.
A transient Keystore error could delete a car's pairing key. The app also decrypted private keys just to check that one exists or to show its ID: at startup on the main thread, inside Settings composition, and several times per connection.
PairingKeyStorehasKey(),publicKeyRaw()andkeyId()read only the stored public key. The key ID is SHA-1(public)[:4], the same asTeslaKeyPair.keyId. Onlyload()touches the private key.AEADBadTagException,UnrecoverableKeyExceptionorKeyPermanentlyInvalidatedException. Any other failure returns null and keeps the key. The failure goes to Logcat, without IDs.UnrecoverableKeyException, because the material came from another install. Restored device-only keys are still dropped, as before.commit(). If the key can't be stored,generate()returns null and pairing stops with an error, soaddKeyis never sent for a key the app could lose.setBackupEnablednow runs on a background thread, so private-key access is serialized across the two instances (controller and Settings).Callers
TeslaBleControlleruses the public-key accessors everywhere excepthandleSessionInfo, which needs the private key forTeslaSession.import. The startup stale-slot check useshasKey().handleSessionInfoforgets the cached slot and any stale "paired" state. The car then shows as unpaired and offers pairing, as it did before.setBackupEnabledthroughrememberCoroutineScope+Dispatchers.IO, and the switch then shows the mode that was actually stored.VehicleStorevehicles_unreadablewithcommit(), before anysave()can replace the list, and loading starts from an empty list. The warning leaves out the list and the parser message, because that message quotes the input, VINs included.Settings backup wording (second commit)
data_extraction_rules.xmldoesn't excludepairing_key.xml, so a backup can contain device-only keys. They stay wrapped by a Keystore key that never leaves the phone, andPairingKeyStoredrops them on any other install. Android 11 and below exclude the file (backup_rules.xml).backup_rules.xmlalways excludespairing_key.xml, the switch shows off and disabled with a line saying key backup needs Android 12 or newer, and the enable dialog never opens; a setting already on there keeps its storage (no migration; reinstall to reset) and its status text no longer claims a restore.Not in this PR (hence "Part of"):
generate()off the main thread when pairing starts from the UI.Type
Testing
The app module can't be built where this was written (Google Maven is blocked). Verified there:
ktlint and detekt, using the repo config, on the four changed files: clean.
The changed
PairingKeyStore,VehicleStoreandTeslaBleControllercompile against the realcoreplus minimal Android stubs. A replica of the changed Settings logic compiles against kotlinx-coroutines.SettingsScreen.ktitself was not compiled, because it needs Compose.A throwaway JVM harness (not committed) ran the real
PairingKeyStoreandVehicleStoreagainst in-memory SharedPreferences and a fakeAndroidKeyStoreprovider doing real AES-GCM. Its 17 cases cover:Flipping the definite-loss check in either direction makes the harness fail.
No new tests in the repo. The app module has no Robolectric, and these classes call Android framework APIs directly.
Needs a real-car check:
pairing a car with a fresh key (card tap, then a session);
the session after an app restart (no re-pairing, sessions come up);
Settings shows each car's key ID, and the backup toggle still converts keys both ways.
./gradlew :core:test(core untouched)./gradlew :app:testDebugUnitTest(left to CI)./gradlew :app:lintDebug(left to CI)Real car (see above)
Checklist
expected.txtregenerated, not hand-edited (n/a: no protocol change)SPDX-License-Identifier: AGPL-3.0-only(n/a: no new files)Notes for review
UnrecoverableKeyExceptionfrom the key lookup can also wrap a transient failure ("Failed to obtain information about key"). Following the issue, it still counts as definite. A follow-up could narrow it, for example by checking the cause'sisTransientFailure()on API 33+.🤖 Generated with Claude Code
https://claude.ai/code/session_0129RzhbEE2cboMcYyZMuxqX
Generated by Claude Code