From cf3f37d716fcbc24b27c5e9258ea3ef98b95071f Mon Sep 17 00:00:00 2001 From: firstmate crewmate Date: Sun, 2 Aug 2026 15:05:32 +1000 Subject: [PATCH 1/2] fix(tesla): make PEM key validation non-blocking, validation/creation optional Reading an existing RSA/EC PEM deserialized it inline in _load_pem_private_key, stalling the event loop by ~200ms for a 4096-bit RSA key. Wrap it in asyncio.to_thread like the generation path already does, add get_rsa_private_key(skip_rsa_key_validation=..., defaults False) to let a caller skip cryptography's RSA consistency check for a trusted externally-supplied key, and always skip it for a PEM the library just generated (it can't be mathematically malformed). async_rsa_key_creation (default True) exposes an opt-out back to the older blocking in-process generator. --- AGENTS.md | 1 + tesla_fleet_api/tesla/tesla.py | 63 +++++++++++++--- tests/test_tesla_private_key.py | 129 ++++++++++++++++++++++++++++++++ 3 files changed, 183 insertions(+), 10 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index f55e5fc..7402f40 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -117,6 +117,7 @@ Keep the `tesla-protocol` floor at `>=0.5.0`; earlier releases have generated `. - **Enums**: Custom `StrEnum`/`IntEnum` in `const.py` (not stdlib). `Region` is a `Literal["na", "eu", "cn"]`, not an enum. - **Seat indexing gotcha**: two distinct seat enums with different conventions. `Seat` is **0-indexed** (`FRONT_LEFT=0`) and is for the manual seat heater/cooler paths (`remote_seat_heater_request`, `remote_seat_cooler_request`). `AutoSeat` is **1-indexed** (`FRONT_LEFT=1`, `FRONT_RIGHT=2`) and is the correct type for `remote_auto_seat_climate_request` on **both** backends — its values equal Tesla's REST wire values and the proto `AutoSeatPosition_*` enum. Don't mix them; passing a `Seat` to the auto-climate command is off-by-one (issue #11). - **Naming**: camelCase for class instance attributes that mirror API structure (`energySites`, `createFleet`). Snake_case for method names that are API endpoints. +- **PEM key handling is non-blocking by default; validation/creation are separately optional**: `Tesla.get_rsa_private_key`/`Tesla.get_private_key` (`tesla/tesla.py`) always deserialize an existing on-disk PEM via `asyncio.to_thread` (RSA validation of a 4096-bit key measured ~200ms - a real event-loop stall if run inline). `get_rsa_private_key`'s `skip_rsa_key_validation` (default `False`) forwards `unsafe_skip_rsa_key_validation` to cryptography for an externally-supplied key you trust; a PEM this library just generated always skips that check unconditionally (it can't be mathematically malformed, only structurally invalid, which still raises). `async_rsa_key_creation` (default `True`) picks between the isolated-subprocess keygen (added in #105, doesn't block the loop) and the older blocking in-process generator. - **BLE discovery gotcha**: a Tesla vehicle advertises no 128-bit service UUID pre-connect — only its VIN-derived local name (`^S[a-f0-9]{16}[CDRP]$`), and only in the scan response, not the `ADV_IND`. `SERVICE_UUID` (`tesla_fleet_api/tesla/vehicle/bluetooth.py`) exists only as a GATT service after connecting. Never pass `service_uuids=[SERVICE_UUID]` as a `BleakScanner` discovery-time filter — it hides the vehicle on a direct BlueZ adapter (an ESPHome proxy doesn't enforce that filter the same way, which can mask the bug in testing). Scan unfiltered with active scanning and match by name; keep `SERVICE_UUID` for post-connect GATT use only. - **BLE domain-routing gotcha**: `Domain` (`tesla_protocol.command.universal_message_pb2`) has more values (`DOMAIN_BROADCAST`, `DOMAIN_AUTHD`, ...) than `VehicleBluetooth._queues` has keys (only `DOMAIN_VEHICLE_SECURITY`/`DOMAIN_INFOTAINMENT`). `_on_message` (`tesla_fleet_api/tesla/vehicle/bluetooth.py`) must look up `_queues` with `.get()` and drop unrecognized domains rather than indexing directly — indexing raises `KeyError` inside the `ReassemblingBuffer` callback, aborting reassembly of any further already-buffered messages in that notification. - **BLE infotainment boot-delay gotcha**: `wake_up()` (VCSEC) returns as soon as the vehicle-security computer acks it, well before the infotainment computer is ready to complete a signed-command handshake. An INFO-domain read/command issued immediately after `wake_up()` can raise `BluetoothTimeout` on the handshake through no fault of the command itself. Live-verified: waiting ~10s after `wake_up()` before the first INFO read is sufficient on the test rig; callers doing INFO work right after waking should retry-with-backoff rather than treat one timeout as failure. diff --git a/tesla_fleet_api/tesla/tesla.py b/tesla_fleet_api/tesla/tesla.py index cc6f7eb..5458d92 100644 --- a/tesla_fleet_api/tesla/tesla.py +++ b/tesla_fleet_api/tesla/tesla.py @@ -119,15 +119,19 @@ async def _generate_rsa_private_key_pem_isolated(key_size: int) -> bytes: async def _deserialize_rsa_pem(pem: bytes) -> rsa.RSAPrivateKey: """Deserialize a freshly generated RSA PEM off the event loop. - A malformed PEM (e.g. from a wrong-but-zero-exit isolated subprocess) - raises `ValueError` here rather than being trusted - the caller treats - that the same as any other isolation failure and falls back. + Skips cryptography's RSA consistency check (`unsafe_skip_rsa_key_validation`) + since this PEM was just generated by us and can't be mathematically + malformed - only structurally invalid (e.g. a wrong-but-zero-exit + isolated subprocess), which still raises `ValueError` here since that + check is independent of PEM parsing itself; the caller treats that the + same as any other isolation failure and falls back. """ value = await asyncio.to_thread( serialization.load_pem_private_key, pem, password=None, backend=default_backend(), + unsafe_skip_rsa_key_validation=True, ) if not isinstance(value, rsa.RSAPrivateKey): raise AssertionError("Generated key is not an RSAPrivateKey") @@ -180,14 +184,29 @@ def _owner_only_opener(file: str, flags: int) -> int: return fd -async def _load_pem_private_key(path: str, retry_invalid: bool = False) -> object: +async def _load_pem_private_key( + path: str, + retry_invalid: bool = False, + unsafe_skip_rsa_key_validation: bool = False, +) -> object: + """Read and deserialize a PEM key file off the event loop. + + Deserialization (in particular RSA's consistency check) runs in a thread + via `asyncio.to_thread` since it can take ~200ms for a 4096-bit key. + `unsafe_skip_rsa_key_validation` is forwarded to cryptography to skip + that check for a caller-trusted RSA PEM; it's a no-op for EC keys. + """ deadline = time.monotonic() + _KEY_READ_RETRY_TIMEOUT while True: async with aiofiles.open(path, "rb") as key_file: key_data = await key_file.read() try: - return serialization.load_pem_private_key( - key_data, password=None, backend=default_backend() + return await asyncio.to_thread( + serialization.load_pem_private_key, + key_data, + password=None, + backend=default_backend(), + unsafe_skip_rsa_key_validation=unsafe_skip_rsa_key_validation, ) except ValueError: if not retry_invalid or time.monotonic() >= deadline: @@ -285,7 +304,11 @@ def public_uncompressed_point(self) -> str: ) async def get_rsa_private_key( - self, path: str = "tedapi_rsa_private.pem", key_size: int = 4096 + self, + path: str = "tedapi_rsa_private.pem", + key_size: int = 4096, + skip_rsa_key_validation: bool = False, + async_rsa_key_creation: bool = True, ) -> rsa.RSAPrivateKey: """Get or create an RSA private key for energy gateway client registration. @@ -294,9 +317,21 @@ async def get_rsa_private_key( PEM file. A newly created key file is opened with O_EXCL so it is born at mode 0o600 with no world-readable window. If another process wins the create race, its file is read instead of raising. + + `skip_rsa_key_validation` skips cryptography's RSA consistency check + when reading a PEM from disk - safe only for a key you trust wasn't + tampered with or corrupted, since it bypasses the check that would + otherwise catch that. `async_rsa_key_creation` (default `True`, + matching prior behavior) generates a new key off the event loop via + an isolated subprocess with an in-process fallback; set `False` to + force the simpler, blocking in-process generator instead. """ if not exists(path): - value, pem = await _generate_rsa_private_key(key_size) + if async_rsa_key_creation: + value, pem = await _generate_rsa_private_key(key_size) + else: + pem = _generate_rsa_private_key_pem(key_size) + value = await _deserialize_rsa_pem(pem) self.rsa_private_key = value try: async with aiofiles.open( @@ -305,13 +340,21 @@ async def get_rsa_private_key( await key_file.write(pem) return self.rsa_private_key except FileExistsError: - value = await _load_pem_private_key(path, retry_invalid=True) + value = await _load_pem_private_key( + path, + retry_invalid=True, + unsafe_skip_rsa_key_validation=skip_rsa_key_validation, + ) if not isinstance(value, rsa.RSAPrivateKey): raise AssertionError("Loaded key is not an RSAPrivateKey") self.rsa_private_key = value return self.rsa_private_key - value = await _load_pem_private_key(path, retry_invalid=True) + value = await _load_pem_private_key( + path, + retry_invalid=True, + unsafe_skip_rsa_key_validation=skip_rsa_key_validation, + ) if not isinstance(value, rsa.RSAPrivateKey): raise AssertionError("Loaded key is not an RSAPrivateKey") self.rsa_private_key = value diff --git a/tests/test_tesla_private_key.py b/tests/test_tesla_private_key.py index b528f77..9546f9e 100644 --- a/tests/test_tesla_private_key.py +++ b/tests/test_tesla_private_key.py @@ -586,3 +586,132 @@ async def fake_create_subprocess_exec( self.assertTrue( any("isolated subprocess" in message for message in logs.output) ) + + +class GetRsaPrivateKeyOptionalValidationTests(IsolatedAsyncioTestCase): + async def test_existing_key_read_validates_by_default(self) -> None: + with tempfile.TemporaryDirectory() as tmp_dir: + path = str(Path(tmp_dir) / "tedapi_rsa_private.pem") + await Tesla().get_rsa_private_key(path, key_size=1024) + + with mock.patch( + "tesla_fleet_api.tesla.tesla.serialization.load_pem_private_key", + wraps=serialization.load_pem_private_key, + ) as load: + await Tesla().get_rsa_private_key(path, key_size=1024) + + load.assert_called_once() + self.assertFalse(load.call_args.kwargs["unsafe_skip_rsa_key_validation"]) + + async def test_skip_rsa_key_validation_forwarded_on_existing_key_read( + self, + ) -> None: + with tempfile.TemporaryDirectory() as tmp_dir: + path = str(Path(tmp_dir) / "tedapi_rsa_private.pem") + created = await Tesla().get_rsa_private_key(path, key_size=1024) + + with mock.patch( + "tesla_fleet_api.tesla.tesla.serialization.load_pem_private_key", + wraps=serialization.load_pem_private_key, + ) as load: + key = await Tesla().get_rsa_private_key( + path, key_size=1024, skip_rsa_key_validation=True + ) + + load.assert_called_once() + self.assertTrue(load.call_args.kwargs["unsafe_skip_rsa_key_validation"]) + self.assertEqual(_rsa_pem(key), _rsa_pem(created)) + + async def test_internally_generated_pem_deserialization_skips_validation( + self, + ) -> None: + with tempfile.TemporaryDirectory() as tmp_dir: + path = str(Path(tmp_dir) / "tedapi_rsa_private.pem") + + with mock.patch( + "tesla_fleet_api.tesla.tesla.serialization.load_pem_private_key", + wraps=serialization.load_pem_private_key, + ) as load: + await Tesla().get_rsa_private_key(path, key_size=1024) + + load.assert_called_once() + self.assertTrue(load.call_args.kwargs["unsafe_skip_rsa_key_validation"]) + + async def test_async_rsa_key_creation_true_uses_isolated_subprocess(self) -> None: + with tempfile.TemporaryDirectory() as tmp_dir: + path = str(Path(tmp_dir) / "tedapi_rsa_private.pem") + + with mock.patch( + "tesla_fleet_api.tesla.tesla.asyncio.create_subprocess_exec", + wraps=asyncio.create_subprocess_exec, + ) as create_subprocess_exec: + key = await Tesla().get_rsa_private_key( + path, key_size=1024, async_rsa_key_creation=True + ) + + create_subprocess_exec.assert_called_once() + self.assertIsInstance(key, rsa.RSAPrivateKey) + + async def test_async_rsa_key_creation_false_skips_isolated_subprocess( + self, + ) -> None: + with tempfile.TemporaryDirectory() as tmp_dir: + path = str(Path(tmp_dir) / "tedapi_rsa_private.pem") + + with mock.patch( + "tesla_fleet_api.tesla.tesla.asyncio.create_subprocess_exec", + ) as create_subprocess_exec: + key = await Tesla().get_rsa_private_key( + path, key_size=1024, async_rsa_key_creation=False + ) + + create_subprocess_exec.assert_not_called() + self.assertIsInstance(key, rsa.RSAPrivateKey) + mode = stat.S_IMODE(Path(path).stat().st_mode) + self.assertEqual(mode, 0o600) + + async def test_existing_key_read_does_not_block_event_loop(self) -> None: + with tempfile.TemporaryDirectory() as tmp_dir: + path = str(Path(tmp_dir) / "tedapi_rsa_private.pem") + real_key = rsa.generate_private_key(public_exponent=65537, key_size=1024) + Path(path).write_bytes(_rsa_pem(real_key)) + os.chmod(path, 0o600) + + def slow_load_pem_private_key( + *args: object, **kwargs: object + ) -> rsa.RSAPrivateKey: + import time as time_module + + time_module.sleep(0.15) + return real_key + + ticks = 0 + + async def heartbeat() -> None: + nonlocal ticks + while True: + await asyncio.sleep(0.01) + ticks += 1 + + with mock.patch( + "tesla_fleet_api.tesla.tesla.serialization.load_pem_private_key", + side_effect=slow_load_pem_private_key, + ): + heart = asyncio.create_task(heartbeat()) + try: + key = await Tesla().get_rsa_private_key(path, key_size=1024) + finally: + heart.cancel() + + self.assertGreater(ticks, 10) + self.assertEqual(_rsa_pem(key), _rsa_pem(real_key)) + + async def test_defaults_unchanged_for_existing_rsa_key_read(self) -> None: + """No new params behaves identically to before this change.""" + with tempfile.TemporaryDirectory() as tmp_dir: + path = str(Path(tmp_dir) / "tedapi_rsa_private.pem") + created = await Tesla().get_rsa_private_key(path, key_size=1024) + + read_back = await Tesla().get_rsa_private_key(path, key_size=1024) + + self.assertEqual(_rsa_pem(read_back), _rsa_pem(created)) From 28b372769f4fc32398fc65e714457521e8a8cabe Mon Sep 17 00:00:00 2001 From: firstmate crewmate Date: Sun, 2 Aug 2026 15:09:41 +1000 Subject: [PATCH 2/2] no-mistakes(document): Document optional non-blocking RSA key handling --- AGENTS.md | 1 - docs/energy_local_control.md | 12 +++++++++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 7402f40..f55e5fc 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -117,7 +117,6 @@ Keep the `tesla-protocol` floor at `>=0.5.0`; earlier releases have generated `. - **Enums**: Custom `StrEnum`/`IntEnum` in `const.py` (not stdlib). `Region` is a `Literal["na", "eu", "cn"]`, not an enum. - **Seat indexing gotcha**: two distinct seat enums with different conventions. `Seat` is **0-indexed** (`FRONT_LEFT=0`) and is for the manual seat heater/cooler paths (`remote_seat_heater_request`, `remote_seat_cooler_request`). `AutoSeat` is **1-indexed** (`FRONT_LEFT=1`, `FRONT_RIGHT=2`) and is the correct type for `remote_auto_seat_climate_request` on **both** backends — its values equal Tesla's REST wire values and the proto `AutoSeatPosition_*` enum. Don't mix them; passing a `Seat` to the auto-climate command is off-by-one (issue #11). - **Naming**: camelCase for class instance attributes that mirror API structure (`energySites`, `createFleet`). Snake_case for method names that are API endpoints. -- **PEM key handling is non-blocking by default; validation/creation are separately optional**: `Tesla.get_rsa_private_key`/`Tesla.get_private_key` (`tesla/tesla.py`) always deserialize an existing on-disk PEM via `asyncio.to_thread` (RSA validation of a 4096-bit key measured ~200ms - a real event-loop stall if run inline). `get_rsa_private_key`'s `skip_rsa_key_validation` (default `False`) forwards `unsafe_skip_rsa_key_validation` to cryptography for an externally-supplied key you trust; a PEM this library just generated always skips that check unconditionally (it can't be mathematically malformed, only structurally invalid, which still raises). `async_rsa_key_creation` (default `True`) picks between the isolated-subprocess keygen (added in #105, doesn't block the loop) and the older blocking in-process generator. - **BLE discovery gotcha**: a Tesla vehicle advertises no 128-bit service UUID pre-connect — only its VIN-derived local name (`^S[a-f0-9]{16}[CDRP]$`), and only in the scan response, not the `ADV_IND`. `SERVICE_UUID` (`tesla_fleet_api/tesla/vehicle/bluetooth.py`) exists only as a GATT service after connecting. Never pass `service_uuids=[SERVICE_UUID]` as a `BleakScanner` discovery-time filter — it hides the vehicle on a direct BlueZ adapter (an ESPHome proxy doesn't enforce that filter the same way, which can mask the bug in testing). Scan unfiltered with active scanning and match by name; keep `SERVICE_UUID` for post-connect GATT use only. - **BLE domain-routing gotcha**: `Domain` (`tesla_protocol.command.universal_message_pb2`) has more values (`DOMAIN_BROADCAST`, `DOMAIN_AUTHD`, ...) than `VehicleBluetooth._queues` has keys (only `DOMAIN_VEHICLE_SECURITY`/`DOMAIN_INFOTAINMENT`). `_on_message` (`tesla_fleet_api/tesla/vehicle/bluetooth.py`) must look up `_queues` with `.get()` and drop unrecognized domains rather than indexing directly — indexing raises `KeyError` inside the `ReassemblingBuffer` callback, aborting reassembly of any further already-buffered messages in that notification. - **BLE infotainment boot-delay gotcha**: `wake_up()` (VCSEC) returns as soon as the vehicle-security computer acks it, well before the infotainment computer is ready to complete a signed-command handshake. An INFO-domain read/command issued immediately after `wake_up()` can raise `BluetoothTimeout` on the handshake through no fault of the command itself. Live-verified: waiting ~10s after `wake_up()` before the first INFO read is sufficient on the test rig; callers doing INFO work right after waking should retry-with-backoff rather than treat one timeout as failure. diff --git a/docs/energy_local_control.md b/docs/energy_local_control.md index ffbcdf9..515500c 100644 --- a/docs/energy_local_control.md +++ b/docs/energy_local_control.md @@ -33,7 +33,17 @@ scripts, REPLs, `python -c`, and notebooks without an entry-point guard. If the subprocess cannot be used (including in a frozen application), exits unsuccessfully, or returns empty or invalid key data, generation falls back to the current process and logs a warning that the asyncio event loop may be -blocked. Loading an existing key file does not start a subprocess. +blocked. Loading and validating an existing key runs in a worker thread and +does not start a subprocess. + +By default, existing keys receive cryptography's full RSA consistency check. +For an existing key that your application already trusts, pass +`skip_rsa_key_validation=True` to skip that potentially expensive check. This +option does not weaken validation when loading the key's PEM structure. Pass +`async_rsa_key_creation=False` only when subprocess spawning is unavailable or +undesired: new-key generation then runs in the current process and blocks the +event loop. Both options affect only RSA key handling; their defaults preserve +the non-blocking and fully validated behavior described above. ## 2. Register the key with the gateway, over the cloud