security: HTTPS bootstrap, CA pinning, truthful control outcomes, and a control hook (3.1.0) - #163
Merged
Merged
Conversation
`register_v2` interpolated `response.text` into `SpanPanelAuthError` on 401/403/422. The panel's validation layer answers a bad passphrase with a FastAPI-style 422 that echoes the submitted body back under `detail[].input`, so the secret that was just rejected landed in an exception message that Home Assistant renders in the config-flow UI, writes to the log, and captures in a diagnostics download. The exception now carries the status code only, matching the shape the branch one line below already used. The body is still recorded, at DEBUG and with credentials removed, because a 422's `loc` is the only thing that says which field the panel objected to. Redaction is a recursive walk rather than a top-level key check. The nesting is not incidental -- it is exactly where the echoed passphrase lives, so a scan of the outermost object would redact nothing in the one response most likely to carry a secret. A body that does not parse as JSON has no structure to walk, so its length and content-type are logged and its content never is.
Every v2 REST call was hardcoded `http://`, including `register_v2`, which sends the panel passphrase up and brings the broker password back. Each function now takes an optional `ssl_context`; supplying one moves that call to `https://`. With `ssl_context=None` the behaviour is byte-identical to 3.0.1, which is what keeps this a minor release. Two things had to change shape for this to be real rather than nominal. `_get_client` yielded an injected httpx client untouched. httpx fixes `verify=` at construction, so a context passed alongside an injected client did nothing -- and the consumer this exists for injects a shared client at every bootstrap call site, so the control would have been off for exactly the caller who turned it on. A supplied context now builds a dedicated client and closes it. The cost is the shared pool and the injected client's timeout policy, which is acceptable because every call here is bootstrap, made a handful of times per config entry. `port: int = 80` could not distinguish an omitted port from a deliberate 80, and those need opposite answers once a scheme is in play. It is now `int | None`, resolving to 80 plaintext and 443 with a context. An explicit 80 alongside a context is refused with `SpanPanelValidationError` naming both numbers rather than guessed at -- a consumer that stored a port before it pinned a CA produces precisely that combination, and either reading of it is defensible. `download_ca_cert` stays plaintext by default and says why in its docstring: it fetches the anchor everything else is verified against, so it has nothing to verify itself against, and its result is a candidate until the caller confirms the fingerprint. It takes a context anyway, for the caller that already holds the anchor and is refetching to compare. `create_span_client` and `SpanMqttClient` thread the context through, so the schema fetch a client makes on its own behalf cannot end up on a different transport from the one that bootstrapped it.
`AsyncMqttBridge` fetched the CA from the panel over unauthenticated plaintext HTTP on every connect *and* every rebuild, and built its trust anchor from whatever came back. Automatic recovery from a CA rotation and automatic acceptance of an interception are the same code path, so a panel presenting a chain from a different CA was silently re-anchored to it on the next reconnect. `MqttClientConfig.ca_pem` is the pin. With it set the bridge makes no CA request on any path. Leaving it `None` keeps 3.0.1's behaviour and warns once per bridge that the anchor was obtained unauthenticated -- once per bridge and not per connect, because the fetch happens on every reconnect and a line per reconnect through a long outage is a log nobody reads. The hard part is not the pin, it is refusing to over-read a TLS failure. A handshake failure carries no evidence that the CA changed, and `ssl` exposes no peer chain when verification fails, so the observed fingerprint cannot come from the failed handshake at all. Against a perfectly valid pinned CA, an expired leaf -- a panel whose clock reset after a power cut -- and a hostname mismatch after an address change both raise `SSLCertVerificationError`; a broker restarting mid-handshake raises `SSLEOFError`, which is the ordinary shape of a firmware upgrade. So: - only `ssl.SSLCertVerificationError` starts a diagnosis, never `ssl.SSLError`; - the diagnosis is a separate plaintext fetch of the panel's advertised CA, compared by fingerprint, and its result is never used to re-anchor; - equal fingerprints, a failed fetch, or an answer that is not a certificate all keep retrying. Escalating on missing evidence would turn a four-minute reboot into a permanent outage. Only a confirmed difference raises `SpanPanelCAChangedError`, carrying both fingerprints because the two remedies -- re-pin, or investigate -- are opposite and only the user can choose. Surfacing it needed a channel. `_reconnect_loop` is fire-and-forget, so raising inside it killed the task invisibly and left the consumer watching a bridge that merely looked disconnected. `set_fatal_error_callback` and the stored `fatal_error` are that channel; `ping()` and `get_snapshot()` re-raise it so a consumer that registered nothing still cannot read a dead transport as a healthy one. The initial-connect path runs the same diagnosis, because a CA that rotated while the consumer was shut down fails there and was previously wrapped as a retryable connection error -- a setup-retry loop with nothing to act on. `_build_ssl_context` moves to `_ssl.build_panel_ssl_context`, joined by `ca_fingerprint`. Both are exported: the consumer builds the same context for its own HTTPS calls and stores and compares the same fingerprint string, and two implementations of a fingerprint that must agree byte-for-byte is a defect waiting for a firmware upgrade to find it. It is taken over the DER rather than the PEM text so a reflowed certificate does not read as a rotated one.
Four ways a setter could report nothing useful, three of which were silent successes: - `_bridge is None` -- `close()` clears the bridge and leaves the adapter, so `_require_adapter()` passed and the setter returned `None` having published nothing at all; - `_client is None` in `publish()` -- an `if` with no `else`, same result; - an unacknowledged write was indistinguishable from a confirmed one, because the return type was `None`; - and publishing while the broker was down looked like a discard and was not. The last one is the substantive fix and it runs opposite to the obvious one. paho *queues* a QoS-1 publish across a disconnect: on `MQTT_ERR_NO_CONN` it keeps the message in `_out_messages` with `state = mqtt_ms_publish` -- its own comment reads "remove from inflight messages so it will be send after a connection is made" -- and the reconnect path reuses the same client object. So the message is not lost; it fires whenever the broker returns, which on a firmware upgrade is minutes later. Checking paho's return code and reporting `FAILED` would tell a user their breaker command failed while it was still pending delivery, and a user told that acts on it. `AsyncMqttBridge.publish` therefore checks `is_connected()` *before* handing the message over, and returns `None` -- refused, never handed to the broker -- rather than letting paho queue it. `PublishState` is the vocabulary. `FAILED` is the only state that is a promise about the future, and it is reachable only from a refusal that happens before paho sees the message. `ACCEPTED` exists because paho gives the QoS-1 PUBACK away for free and "the broker took it and the panel did not act" is a different diagnosis from "nothing ever acknowledged it". `UNCONFIRMED` is not an error and does not raise -- it is the expected result of a write whose value was already current. A transport rebuild drops paho's outbound queue, so anything still awaiting a PUBACK is settled rather than left to burn its deadline. It settles as `UNCONFIRMED`, not `FAILED`: the rebuilt client loses the message on this side, but the original may already have reached the broker, and `FAILED` would be the same lie the `is_connected()` gate exists to prevent. Deadlines are per property and injectable, because otherwise every existing setter test blocks on a real one. The test broker now acknowledges QoS-1 publishes, which is both faster and more faithful. The five setters and the four control protocols move from `-> None` to `-> PublishOutcome`. Additive for callers; breaking for anything type-checked against those protocols with `-> None`, which is what test fakes and simulators are. The release notes name it rather than claiming pure additivity.
`CONFIRMED` needs the property that reports a write, and the transport held only an opaque topic string. Parsing the triple back out of it was rejected: it would bake two schemas' topic grammars into the bootstrap layer, which is the one thing this layer is supposed to know nothing about. The adapter is the only component whose job is the wire format, so it now returns a `ControlTarget` -- topic plus `(device_id, node_id, property_id)` -- from one call, so the topic a command goes to and the property watched for its effect cannot come from different resolutions of the same request. The four `set_*_topic` members are **renamed** to `set_*_target` rather than having their return type changed under the old name. Changing it in place would leave an adapter built against the old contract passing discovery on member presence and failing much later, inside a setter, as `AttributeError` on a `str` -- which is exactly the stale-adapter failure `ADAPTER_CONTRACT_VERSION` exists to catch at discovery. Under a new name the old adapter is rejected where the remedy can still be named, and the contract number stays at 1 because the change really is additive plus a removal rather than a redefinition. The flat adapter's `register_property_callback` was handing consumers the accumulator's `(node_id, property_id, new_value, old_value)` where the protocol declares `(device_id, node_id, property_id, value)`. The two agree on arity and on nothing else: the fourth argument was the *previous* value where the protocol wants the current one, and the device was missing entirely, so a consumer written against the protocol read a node id as a device id and an old value as a new one -- and could not have noticed, because both are strings. `SchemaOneAdapter` already adapted the SDK's arguments to the protocol's; the flat adapter now does the same. Verification itself: the transport registers one property observer per adapter, which serves both halves. The pre-write value answers "is this a no-op" without a round trip -- compared in *wire* vocabulary, because `BATTERY` reaches the publish as `OFF_GRID` under v1.0 and comparing the caller's string would never match and would burn a full deadline on every repeated write. The same stream resolves the write's own deadline: a transition to the value written is `CONFIRMED`, a PUBACK without one is `ACCEPTED`, neither is `UNCONFIRMED`. Two things `CONFIRMED` deliberately does not claim. It is not proof this write caused the transition -- the panel coalesces every API client into one `USER` requester -- and it is not retried when it does not arrive, because a relay write is not idempotent in its physical effect and a racing external change may have legitimately reverted it.
…ive setters Every control command now passes through a single optional interceptor. It exists because a consumer's authorisation gate needs a choke point that cannot be bypassed by a control path somebody forgets to route through it, and because an audit assembled from five separate setters drifts the first time a sixth is added. Its docstring says plainly what it is not. This is a boundary against callers of *this library*: anything holding the broker credential publishes to the panel directly and never reaches this code. Presenting it as a boundary around the panel would be the most damaging thing the feature could do, because a user would stop looking for the real one. The edges, each decided rather than left to the implementation: - **Registration is a protocol member.** `ControlInterceptionProtocol`, new rather than a member added to the four control protocols -- which this release has already broken once -- or to `StreamingCapableProtocol`, which has nothing to do with control. - **One interceptor, replaceable.** Several raise ordering questions with no principled answer; a consumer needing several composes them where it knows which wins. - **A veto propagates unchanged.** The library does not translate the interceptor's exception: the consumer raises a framework error carrying a translated message and needs it to reach the user intact. - **`after_publish` fires for a vetoed command**, with `FAILED` and `detail="vetoed"`. An audit that silently omits refusals is worse than no audit. - **`after_publish` is fired as a task, not awaited.** A sink that merely hangs -- a slow event bus, a blocked writer -- would otherwise stall every control call. Ordering across commands is therefore not guaranteed, and the docstring says so. Interception wraps the refusals and the no-op short-circuit as well as the commands that reach the wire, because the authorisation decision has to precede everything this client decides, and an audit missing the refused commands has a hole exactly where the interesting cases are.
The bootstrap and both adapter distributions move together and the floors say so in both directions. schema-0/-1 raise their floor to 3.1.0 because that is the first bootstrap defining `ControlTarget`; the bootstrap's own extras raise theirs to 1.1.0 because `_derive_required_members` makes every protocol member mandatory of every adapter wheel, so a 1.0.0 adapter against this release is rejected at discovery rather than degraded. Changelogs describe the release against 3.0.1: the publish paths are bug fixes rather than features, and the setters' `-> None` becoming `-> PublishOutcome` is named as a breakage for implementers even though it is additive for callers. Two docstrings in schema-1 still named `set_*_topic`. The README gains the control-outcome vocabulary, the interceptor and CA pinning, and stops counting `SchemaAdapter` as the seventh protocol now that there are eight.
…ts remedy Two gaps the release audit turned up. A rebuild settled the publish future but nothing was watching it: the setter waited on the property transition alone, so a relay write sat out its full five seconds against a transport that had already thrown the message away. The verification future now carries which ending it got -- transition or discard -- and the bridge's settlement resolves it through `_discard_verification`. A PUBACK deliberately does not end the wait: the broker taking the message is not the panel acting on it. Resolved rather than cancelled, because a cancellation here would be indistinguishable from the caller cancelling the control call, and folding one into the other would swallow the other. `_resolve_pending_publishes` claimed to save callers from burning a deadline and only made that possible; its docstring now says which. A member-presence rejection listed the absent names and stopped there, which reads as a fault in the adapter and sends someone hunting a bug. It is what a mismatched pair of packages looks like, caught cleanly in both directions -- a new bootstrap misses the member an old adapter has not grown, an old one misses what a new adapter has renamed -- and the remedy is the same either way, so the message states it. This is the promise `ADAPTER_CONTRACT_VERSION` made and did not keep, kept without widening the constant past its own criterion of defects presence checking cannot catch.
The shim between the accumulator's `(node_id, property_id, new, old)` and the protocol's `(device_id, node_id, property_id, value)` had nothing holding it in place, and it is the arity-compatible kind of wrong: four strings either way, so a bare delegation type-checks, runs, and quietly hands a consumer the node id as a device id and the previous value as the current one. Flat-panel write-then-verify rests entirely on those four arguments arriving in that order -- both CONFIRMED and the no-op pre-check match on `(device_id, node_id, property_id)` and compare the reported value -- and no publish-outcome test would catch a regression, because they all drive a MagicMock adapter. Two writes rather than one: with a single write the new value and the old are indistinguishable, since the first arrives with no previous value at all. Verified the test fails against a bare delegation before keeping it.
…s rebuilt The bridge empties its outbound queue on a rebuild and on teardown alike, and both endings arrived at the same detail string, which named the rebuild. A close() therefore reported itself as a rebuild -- wrong about the cause, in a field whose only job is to tell a person which of three things happened. The string now says what it observed rather than guessing which of the two caused it. Carrying the reason through would mean threading it from the bridge into a future that is a bare bool; naming neither is both smaller and more honest, since from this layer the two are genuinely the same event. The teardown test grows the assertions that would have caught it, and takes a realistic relay deadline so it also pins that a torn-down transport releases its caller rather than holding them to it.
Two things the changelog got wrong about its own release. `SpanPanelClientProtocol` gained `register_fatal_error_callback`, which breaks implementers exactly as the setters' return type does -- and, because the protocol is runtime_checkable, breaks them at runtime too: a consumer that asks isinstance before offering a feature silently stops offering it. Only the setters were named. Both are now. "Refused instead of queued" was stronger than the mechanism. The gate is only as fresh as paho's disconnect detection -- a socket close, or the keepalive -- so a broker that stops answering without closing its socket leaves roughly a keepalive and a half in which a publish is handed over and queued after all, and re-sent as DUP on the next reconnect of the same client. Nothing lies as a result: that caller is told UNCONFIRMED, which promises nothing either way, and FAILED's promise is untouched because this path never produces it. But the sentence described a detected disconnect and claimed every disconnect, so it now says which, and `publish()`'s docstring carries the same bound next to the check it qualifies. Closing the window means rebuilding rather than reconnecting while un-PUBACKed publishes are in flight, which is a larger change than a release note.
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.
Hardens the credential path and makes control commands say what actually happened to them. Bootstrap 3.1.0, both adapters 1.1.0.
Built to a reviewed spec; the four load-bearing behaviours below were each verified against real source rather than against their descriptions.
What changes
The credential no longer travels or rests in the clear. The auth-failure exception carried the panel's response body — which on a 422 can echo the submitted passphrase — into Home Assistant's log; it now reports the status code, and the body is DEBUG-logged through a recursive redaction walk that reaches the nested
detail[].inputcase. Every bootstrap REST call takes an optional SSL context and speaks HTTPS when given one.The trust anchor is pinned.
MqttClientConfigacceptsca_pem, and a pinned bridge never refetches the CA as part of establishing trust. Previously it refetched on every connect and every rebuild, so a panel presenting a certificate from a different CA was silently re-anchored to it — the substitution the pin exists to stop. A confirmed change raisesSpanPanelCAChangedErrorthrough a new typed fatal-error channel rather than dying inside a fire-and-forget reconnect task.Detecting that change is not the same as catching a handshake failure. An expired leaf (panel clock skew after an outage) or a hostname mismatch raises
SSLCertVerificationErroragainst a perfectly valid pin, andsslexposes no peer chain on failure. So onlySSLCertVerificationErroris treated as a candidate — neverssl.SSLError, which also catches theSSLEOFErrorof an ordinary broker restart — and it is confirmed by a diagnostic refetch-and-compare that never escalates on missing evidence.Control commands now report truthfully. Three paths previously returned success for a command that never landed. The important one: paho queues a QoS-1 publish while disconnected and re-sends it as DUP on reconnect, so reading paho's return code and calling it failure would tell a user a breaker command failed while it was still pending delivery. The bridge now refuses before handing the message over, which is what lets
FAILEDmean will never be delivered. Setters returnPublishOutcomewith four states —CONFIRMED(observed on the property topic),ACCEPTED(broker PUBACKed, no transition),UNCONFIRMED,FAILED— andUNCONFIRMEDis not an error: it is the expected result of a no-op write.One place to observe or veto control.
ControlInterceptoris consulted by all five setters. A veto propagates unchanged so a caller's own exception type survives;after_publishfires for every outcome including refusals, as a task so a hanging sink cannot stall a control call.Breaking
PublishOutcomeinstead ofNone, andSpanPanelClientProtocolgainsregister_fatal_error_callback. Additive for callers; anything type-checked against these protocols stops conforming, including underruntime_checkableisinstance.set_*_topicbecomesset_*_target, returning aControlTargetthat names the property reporting the result — write-then-verify needs the topic and that property produced by one call so they cannot disagree. Both adapters move to 1.1.0 in lockstep; floors are raised in both directions so a mismatched pair fails resolution rather than discovery.ADAPTER_CONTRACT_VERSIONstays 1: member-presence checking rejects both bad pairings at discovery with an actionable message, which is what that constant is for the cases presence checking cannot catch.Scope
Nothing here requires a firmware or API change. Authorization is deliberately not attempted at this layer: this is a boundary against callers of the library, not against holders of the broker credential, who reach the broker without passing through any of it. Docstrings say so plainly.
Verification
1029 passed / 11 skipped, mypy clean across 43 source files, all hooks green, 95.72% coverage. No
Any,type: ignore, orcast()added.Tests pin mechanisms rather than outcomes: the disconnected-publish test asserts
paho.publish.assert_not_called(); the pinned-CA tests assertdownload_ca_certis never called; the PUBACK test asserts elapsed time soACCEPTEDcannot regress into firing the instant the broker answers.Not published
This merges unpublished. Do not tag an adapter before the bootstrap — after merge, main declares adapters at 1.1.0 with a floor on a bootstrap version not yet on PyPI.