refactor: single shared verify_cert_fingerprint for mqtt/ftps/camera pin checks - #89
Merged
Merged
Conversation
…pin checks MQTT, FTPS (control + data channels), and the direct camera grab each carried their own hand-written copy of the security-critical TLS fingerprint compare — three fail-open bug surfaces. Extract one shared bambu_cli.tlspin. verify_cert_fingerprint and route all five call sites through it. The shared verifier fails closed on a missing pin, a mismatched pin, or an unobtainable peer certificate, and uses hmac.compare_digest (constant time) on normalized hex (lowercase, colon/space stripped). Callers pass an exc_factory so the raised type matches their contract (camera keeps _CameraPinMismatch to steer its fallback policy; mqtt/ftps keep ssl.SSLError). Behaviour is equivalent at each call site with one hardening: the camera path now fails closed on a missing peer certificate (raises _CameraPinMismatch) instead of the previous None.lower() AttributeError that fell through the broad except into the unpinned Docker streamer. Tests: direct unit suite for the shared function (match, mismatch, case/colon/ space variants, empty/None pin, missing cert, exc_factory); per-transport fail-closed tests retained; sabotage-verified that making the verifier fail-open breaks the mqtt, ftps, and camera suites.
hmac.compare_digest raises TypeError on a non-ASCII str, and normalize_fingerprint only strips ':' and ASCII spaces — so a copy-pasted homoglyph or non-breaking space (or any wrong-length value) survived into the compare. That TypeError is not the caller's error type: in the camera path it escaped _grab_camera_frame_direct into the broad except-Exception in _cmd_snapshot and fell through to the UNPINNED Docker streamer, and in the ftps data channel it bypassed the close-before-raise (FD leak). Both are the fail-open bug class this refactor set out to remove. Validate the normalized pin is exactly 64 lowercase hex chars before comparing; a malformed pin now raises via the caller's exc_factory (ssl.SSLError for mqtt/ftps, _CameraPinMismatch for camera) — fail closed. ``actual`` is a hexdigest() and is always clean, so it needs no guard (noted in a comment). Tests: malformed/non-ASCII/NBSP/wrong-length pin -> exc_factory raise (default SSLError + custom factory); camera-path non-ASCII pin -> _CameraPinMismatch (not TypeError); ftps data-channel malformed pin -> SSLError + socket closed. Sabotage-verified: disabling the guard surfaces the TypeError and fails all three transports' new tests. Docs snapshot bumped 1031 -> 1041.
This was referenced Jul 29, 2026
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
1 participant
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.
What & why (roadmap B.5, the only P0 in docs/test-backlog.md)
MQTT, FTPS (control + data channels), and the direct camera grab each rolled their own copy of the security-critical TLS certificate-fingerprint compare — three hand-written fail-open surfaces. This extracts one shared
verify_cert_fingerprintand routes all five call sites through it (single source of truth).The shared function —
bambu_cli/tlspin.pyhmac.compare_digeston normalized hex.:/space stripped — acceptsAA:BB…,aa bb …, and bare 64-hex-char digests (same rule asconfig._expected_fingerprint/context._normalize_fingerprint).exc_factorylets each caller keep its contract — mqtt/ftps raisessl.SSLError, camera raises_CameraPinMismatch(aBambuError) so its Docker-fallback policy is unchanged.Behavioural differences between the three old copies
The three copies were equivalent except the camera path had no missing-peer-cert guard:
fingerprint_sha256(None)returnedNone, thenNone.lower()raisedAttributeError, which was swallowed by the broadexcept Exceptionand fell through to the unpinned Docker streamer — a latent fail-open. mqtt/ftps already guarded this and raisedssl.SSLError. The shared verifier now makes the camera fail closed on a missing cert (_CameraPinMismatch). This is the one intentional behaviour change; every other call site is byte-for-byte equivalent.Tests
tests/test_tlspin.py: match, mismatch, case/colon/space variants, empty/Nonepin, missing cert,exc_factoryrouting,normalize_fingerprint.fingerprint_sha256, plus a new missing-peer-cert regression test.Gates (all green locally)
pytest
-m "not live"1031 passed, coverage 84.59% (floor 83;tlspin.py100%); ruff check + format; mypy; bandit (0 medium/high); syntax/compat/privacy/ci-workflow/cli-help smokes.Scope
Shared function + five call-site swaps + tests + CHANGELOG/SECURITY/docs snapshot updates. No opportunistic refactoring. Accepted camera residuals (TCP-level fallback, broad post-handshake abort) and
upload_file's trust model untouched.Do not merge — maintainer merges.