feat(ble): allow private_key=False to disable command signing - #137
Merged
Conversation
A passive BLE listener that only decodes broadcasts has no reason to generate or load a key it will never use. Commands.__init__ (and VehicleBluetooth/vehicles.create*'s key argument) now distinguish an omitted key from an explicit None via a sentinel default: omitting it keeps today's fallback-to-parent-or-raise behaviour, while passing None explicitly disables signing. Any operation that needs a signed session raises a clear SigningDisabled instead of failing deep in the crypto path.
…ivate_key is None
…r pair() guard fix
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Member
Author
|
Question, to avoid the ambiguity of |
An explicit `private_key=None` used to fall through to the parent's key and
raise `ValueError("No private key.")` when there was none. The sentinel
design in this PR changed that: `None` became the opt-out, so a caller who
wrote `private_key=None` meaning "I haven't got one" silently got a vehicle
that could not sign instead of the error that told them so. That is a
behaviour change on a published, security-adjacent library, not an additive
one.
Drop `KeyOmitted`/`KEY_OMITTED` and restore `None` as the default. `False` is
now the explicit "signing is disabled" value - a value no current caller can
already be passing, so opting out has to be deliberate, and `None` keeps its
existing meaning for both omitted and explicit-`None` callers.
Because `False` and `None` are both falsy, the constructor branches on
identity (`is False` / `is not None`); a truthiness check would collapse the
two states and reintroduce the bug in a new form.
`Vehicles.createBluetooth` had no `key` parameter at all, so the opt-out was
unreachable from the Fleet-parented factory; it gains one as keyword-only
(no positional shift), with the matching `NotImplementedError` overrides in
Teslemetry/Tessie kept in step with their "parameters match the Fleet API
Bluetooth factory" contract.
The `SigningDisabled` guards at `_handshake` and in `pair()`'s fast path are
unchanged.
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.
Intent
Allow a Bluetooth vehicle to be constructed with signing explicitly disabled, for passive-listening use (a BLE listener that only decodes broadcasts and never sends a signed command). The existing
Commands.__init__behavior - raiseValueError("No private key.")when no key is available - is correct and must stay unchanged. What was missing was a way to say "I am deliberately disabling signing" distinct from "I forgot to supply a key".False, notNone, is that signal. An earlier revision of this PR used aKeyOmitted/KEY_OMITTEDsentinel default so that an explicitprivate_key=Nonebecame the opt-out. That was withdrawn: it is a silent behaviour change, not an additive one.main,private_keydefaults toNoneand the constructor branches onif private_key:, so an explicitNonefalls through to the parent's key and raisesValueError("No private key.")when there is none.Noneconstructed successfully with signing disabled.So a caller who wrote
private_key=Nonemeaning "I haven't got one" would have silently received a vehicle that cannot sign, in place of the error that used to tell them so - on a published, security-adjacent library.Falseavoids this entirely:Nonekeeps its existing meaning for both omitted and explicit-Nonecallers, andFalseis a value no current caller can already be passing, so opting out of signing has to be deliberate.Implementation trap worth naming:
FalseandNoneare both falsy, so a truthiness check (if private_key:) collapses the two states and reintroduces the bug in a new form. The constructor branches on identity (is False,is not None) throughout.Any operation that actually needs to sign fails clearly instead of raising a generic
AttributeError/TypeErrordeep in the crypto code:SigningDisabled(LibraryError)inexceptions.pyis raised at the top ofCommands._handshake- reached by_command(all signed commands) and by_ensure_handshake(signed reads) - and, per the round-1 review finding, at the top ofVehicleBluetooth.pair(), whose fast path builds its whitelist request fromself._public_keyand calls_senddirectly without ever reaching_handshake. A signing-disabled vehicle can still receive broadcasts via the existinglisten_*methods, since those never go through_handshake/_command.Vehicles.createBluetoothturned out to have nokeyparameter at all, so the opt-out was unreachable from the Fleet-parented factory. It gains one as keyword-only, so no existing positional caller shifts; the twoNotImplementedErroroverrides inTeslemetry/Tessieare kept in step with their "parameters match the Fleet API Bluetooth factory" docstring contract. Flagging this as the one piece of surface beyond what the previous revision touched.Deliberately out of scope, per explicit instruction: did not touch the funnel, stream publisher, or any BLE decoding logic; did not change how keys are generated/loaded/stored; kept this to one constructor path (no new listener class or passive-mode subsystem).
What Changed
KeyOmitted/KEY_OMITTED.Commands.__init__'sprivate_key(andkeyonVehicleBluetooth.__init__,Vehicles.createBluetooth,VehiclesBluetooth.create/createBluetooth) defaults to plainNoneagain, restoringmain's signature and behaviour for omitted and explicit-Nonecallers.Falseis the new explicit "signing is disabled" value; branches use identity checks, never truthiness.Commands.private_keyis typedEllipticCurvePrivateKey | None, itsNonemeaning signing-disabled.Vehicles.createBluetoothgains a keyword-onlykeyparameter (it previously had none), with matching updates to theTeslemetry/TessieNotImplementedErroroverrides.SigningDisabled(LibraryError)and both guards -Commands._handshakeandVehicleBluetooth.pair()'s fast path - plus the regression test provingpair()raises before touching the transport.tests/test_ble_null_key.pyaddsExplicitNoneIsUnchangedTests, asserting directly thatkey=Nonestill raisesValueErrorwith no parent key and still falls back when the parent has one - the regression this whole revision exists to prevent. Thekey=Falsecases cover construction with signing disabled (including overriding an available parent key), broadcast dispatch vialisten_vehicle_lock_state, andSigningDisabledfrom a signed command, a bare handshake, andpair().docs/bluetooth_vehicles.md's "Passive listening without a private key" section, and theAGENTS.mdgotcha - all of which described the sentinel design.Risk Assessment
✅ Low:
Noneand omission now behave exactly as they do onmain(verified against both revisions and asserted by test), so there is no behaviour change for any existing caller. The only new reachable state is one a caller must opt into with a literalFalse. The addedkeyparameter onVehicles.createBluetoothis keyword-only, so it shifts no existing positional argument.Testing
Full suite: 735 passed, 30 subtests (733 on the previous head; +2 from the new explicit-
Noneregression tests).uv run ruff check tesla_fleet_api testsanduv run pyright tesla_fleet_apiboth clean (0 errors). Beyond the suite, I exercised all four constructor states directly against the built library - omitted/no parent key →ValueError; explicitNone/no parent key →ValueError; explicitNonewith a parent key → falls back;False→private_key is Noneeven with a parent key available - confirming the explicit-Nonepath matchesmainrather than the withdrawn sentinel behaviour.Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
tesla_fleet_api/tesla/vehicle/bluetooth.py:1529- The user intent states as required behavior: "raise it at the top of Commands._handshake - the single choke point every signed session goes through, reached both by _command (all signed commands) and by _ensure_handshake/pair()" (also asserted verbatim in the AGENTS.md addition: "_handshake (the common choke point for every signed session, both _command and _ensure_handshake/pair() reach it)"). This is false for pair()'s fast path. pair() (bluetooth.py:1505) builds its WhitelistOperation request using self._public_key (line 1532) and calls self._send(msg, ...) directly (line 1549) without ever calling _handshake or checking self.private_key. _handshake is only reached from pair()'s slow path, inside _pair_probe() (line 1587), which only runs after the fast-path one-shot wait times out. For a vehicle constructed with key=None (signing disabled), init sets self._public_key = b"" (commands.py:481-486). Calling .pair() on such a vehicle therefore does NOT raise SigningDisabled up front as documented; instead it proceeds to connect_if_needed() and issues a real GATT write to the vehicle with a WhitelistOperation containing an empty PublicKeyRaw, sending a malformed pairing request to real hardware rather than failing clearly. This contradicts the intent's explicit claim that _handshake is reached by pair(), and undermines the stated goal that any operation needing to sign now fails clearly instead of failing deep in the crypto/transport path - here it doesn't fail at all before touching the transport. tests/test_ble_null_key.py does not cover pair(), only door_lock() and handshakeVehicleSecurity(). Recommended fix: add a self.private_key is None check (raising SigningDisabled) at the top of pair(), mirroring _handshake.🔧 Fix: Raise SigningDisabled up front in pair() when private_key is None
1 info still open:
tests/test_ble_null_key.py- The fix-round commit (2f903f2) adds the SigningDisabled guard to pair() but adds no accompanying test exercising VehicleBluetooth.pair() with private_key=None. Existing tests in tests/test_ble_null_key.py only cover door_lock() and handshakeVehicleSecurity() (per the original round-1 finding), so the newly-fixed pair() path — the exact bug reported — still has zero regression coverage and could silently regress in a future refactor.✅ **Test** - passed
✅ No issues found.
uv run pytest tests/test_ble_null_key.py -v (8 passed, including new test_pair_raises_signing_disabled_without_touching_transport)Manually reverted the pair() guard (removed theif self.private_key is None: raise SigningDisabled()lines) and reran the new test to confirm it fails without the fix (TypeError inside _raise_for_whitelist_reply, proving pair() proceeds to the transport), then restored the guard viagit checkout -- tesla_fleet_api/tesla/vehicle/bluetooth.pyuv run pytest tests/test_ble_null_key.py tests/test_ble_mocked_commands.py tests/test_ble_broadcast_confirmation.py tests/test_ble_broadcast_listeners.py -q (56 passed) as a targeted regression check around the touched BLE pairing/signing code✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.