Connection-freeze hardening: Phases 2-4 (owner token, idle drop, auth-abuse disconnect) - #135
Open
davelee98 wants to merge 10 commits into
Open
Connection-freeze hardening: Phases 2-4 (owner token, idle drop, auth-abuse disconnect)#135davelee98 wants to merge 10 commits into
davelee98 wants to merge 10 commits into
Conversation
davelee98
force-pushed
the
feat/phase4-auth-abuse-disconnect
branch
from
August 1, 2026 21:14
b05c1a1 to
b98b849
Compare
Squashes 16 successive revisions of the freeze-hardening planning docs into their settled state. They were written and re-cut against the tree as the phases were designed, so the intermediate versions record drafting churn -- plan restructurings, threshold decisions later moved to their point of use, and two rounds of external-review discharge -- rather than anything a reader or a bisect would want to land on. CONNECTION_POLICY.md is the normative ruleset the Phase 2-4 commits implement; the PLAN_* documents are the working notes behind it.
…frame identity
Implements Phase 2 of docs/PLAN_FREEZE_HARDENING_2026-07-31.md: the
transport/HAL mechanisms the later phases stand on. No idle timeout and no
reclaim of a held slot — those stay Phase 3.
Mechanisms:
- src/link_owner.{h,cpp}: the owner token as ONE 32-bit atomic word
(transport:2|handle:14|epoch:16), CAS-claimed at the earliest transport hook
so stack callbacks can read it; atomic epoch allocator that never yields the
reserved 0; linkMarkTerminal() returning the displaced identity; the R4
activity clock, whose baseline carries an owner tag so a newly admitted client
can never inherit a prior session's silence.
- Per-handle instance table in both transports. Liveness IS the packed identity
word, so identity and liveness are read in one atomic load.
- Callback-side filtering: per-link write filter, per-link subscribe state, and
handle-targeted notify — the last closes a LIVE LEAK where every response,
authentication traffic included, went to all subscribed clients.
- Frame identity: CommandQueueItem carries its writer's identity word, stamped
before the release-store; serviceBleRx drops frames whose tag is no longer the
owner. This retires the RX-boundary mechanism (bleRxQueueDiscardTo and the
capture that fed it), which could lose its boundary to handle reuse.
- session_guard: abortToKnownState(reason, dropLink, ownerId) with the 11 ordered
steps, transport-dispatched drop, release strictly last; bleDropAndWait()
polling per-handle liveness (not the aggregate count) on a plain delay tick.
- endRefresh() closes the refresh bracket on both paths and re-stamps the clock.
- Deep sleep gates admission with linkMarkTerminal() BEFORE the abort, then does
its own sleep quiescing (panel force-off incl. WARM, buzzer/LED silence). The
abort itself never silences effects and never kills a WARM panel.
- The 15-minute transfer watchdog routes through the abort.
SCOPE ADJUSTMENT — contender refusal is pulled forward from Phase 3, because
Phase 2 is not safely shippable without it. Admission is decided once per
instance and never revisited, so a client reconnecting into a still-held slot
(the ordinary case when loop() was blocked in a refresh) becomes a permanent
contender — and on nRF it occupies the only peripheral link, leaving the device
unreachable until it happens to leave. Releasing the token in the disconnect
callback instead was tried and reverted: it admits a new owner while the departed
session's transfer, crypto and TX ring are still live. LAN accept likewise
refuses rather than evicts, which also removes a path that stranded the token
until reboot and closes an unauthenticated eviction (LAN-TLS bypasses app auth).
Loop order is now R7d-normative: disconnect cleanup and refusal run BEFORE the RX
drain, so a departed session's state is gone before any frame dispatches.
Verification: all 11 PlatformIO envs build; tools/test_link_owner.cpp (70,170
checks) passes under ASan+UBSan and ThreadSanitizer, and kills five mutants
(handle-only identity, epoch-0 allocation, unconditional release, missing
admission baseline, releasable terminal gate). Not verified: any hardware.
Known gaps, stated rather than assumed covered: the notify handle race is
narrowed but not closed (NimBLE can reassign a numeric handle at any time and
notify() takes no epoch); the window between linkClaim's CAS and its baseline
store is not deterministically testable without a firmware-side test hook, so it
rests on publication order plus TSan.
…refusal races Four review rounds against the Phase 2 implementation (dbec776). Each fix below replaced an earlier one that a later round showed to be wrong; the reasoning is recorded at each site so the rejected shapes are not re-tried. - LAN released its token before the deferred abort, on every ordinary exit path (TLS failure, peer close, read error, idle timeout, config restart). A BLE connect could claim the freed slot first, at which point the cleanup saw a live BLE owner and skipped the LAN abort entirely -- leaving the new owner's frames running against the departed LAN session's transfer state. disconnectWiFiServer no longer releases; the abort's final step does, exactly as on BLE. - Contender refusal could disconnect the winner. The connect callback publishes its table entry BEFORE its claim CAS, so an entry seen against a stale owner snapshot may belong to the connection taking the slot. Skipping the scan while unowned (the first attempt) then left a decided loser attached forever -- the original nRF unavailability, reintroduced. Fixed by publishing the claim DISPOSITION: each entry carries the identity word its claim was decided for, so the scan can tell in-flight from decided-and-lost. The identity binding (rather than a bool) is what excludes pairing one entry's identity with another's disposition after a slot is reused. - The scan's three loads are now ordered so a stale read is safe: entry word, then its decided-for word (must match, proving the claim resolved for THIS instance), then the owner word read fresh and last. - disconnect() takes (handle, epoch) and re-validates against the table before asking the stack, so a caller acting on a slightly stale scan cannot drop whoever inherited the numeric handle. - Refusal no longer raises the shared cleanup flag indirectly: serviceBleEvents schedules teardown only when the token's BLE owner has no live table entry (state-based, so it survives coalesced events). - The deep-sleep wake prologue tested aggregate ble.isConnected(), so a contender triggered fullSetupAfterConnection() and closed the wake window. It now tests for a live BLE owner, and reaps contenders in that branch. Residuals, stated rather than implied: notify() and disconnect() are both handle-addressed, so a host-task handle reassignment between validation and the stack call remains possible -- narrowed to a few instructions, and the exposure is a spuriously dropped client that reconnects, never stranded ownership. 11 envs build; host test 70,170 checks passes under ASan+UBSan and TSan.
davelee98
force-pushed
the
feat/phase4-auth-abuse-disconnect
branch
from
August 1, 2026 21:25
f1bed00 to
b650770
Compare
This was referenced Aug 1, 2026
davelee98
changed the title
The Phase 2 work added tools/test_link_owner.cpp but never wired it into CI, so 70k checks covering the owner token, epoch discrimination, the terminal gate and the activity clock were not a repo gate. Run it in the existing host-tests job, twice: ASan+UBSan for the single-threaded semantics and TSan for the concurrent claim/epoch paths, since the two sanitizers cannot be combined and the cross-task claim is exactly what needs the second one. Also reconcile the plan with what shipped: contender refusal is in Phase 2, not Phase 3, because Phase 2 is not safely shippable without it (a client that reconnects into a held slot becomes a permanent contender, and on nRF occupies the only link). The phase table said otherwise, which would have misled the next reader about where that mechanism lives.
…ports Implements Phase 3 of docs/PLAN_FREEZE_HARDENING_2026-07-31.md. Contender refusal already landed in Phase 2 (it was not safely separable), so what remains here is the reclaim path and the LAN half of R4's clock semantics. - serviceIdleTimeout() reclaims a slot whose owner has gone silent (7c). Called LAST in the pass (7d step 4), after BLE RX and handleWiFiServer, so traffic parsed this pass counts. Excludes refresh (7c row 3) and an unowned or terminal slot (7c row 4). Since admission never evicts, this is the ONLY way a held slot is released short of the client leaving. - NO transferActive() gate, per R4. A client that goes silent mid-upload is exactly the case that wedges the device, and a transfer gate would exempt it. The partial transfer is discarded by the abort. - OD_BLE_IDLE_TIMEOUT_MS = 120 s, #ifndef-guarded beside the code that services it, with the client behaviour it assumes recorded on the define. Deliberately generous: R4 inverted the direction of the error, since erring short now costs a legitimate upload rather than a stale session. - LAN's separate activity clock is RETIRED. lastLanActivityMs and its four stamp sites are gone, as is the inline 30 s check inside handleWiFiServer. Both transports now use the shared clock, differing only in their constant (BLE's local 120 s vs LAN's OD_LAN_READ_TIMEOUT_S, which is a client-visible wire-header contract). Two clocks for one rule is how they drift. The TLS handshake completion site stamps the clock, so the idle baseline starts there rather than at TCP accept -- which is why no separate handshake deadline is needed. - Accept/refuse is extracted into admitOrRefuseLanClient() so handleWiFiServer CONTINUES servicing the incumbent after a refusal. Returning early there let a contender starve the incumbent: its handshake never advanced and its frames were never dispatched, so the clock stopped being stamped and the new end-of-pass idle drop killed it with valid commands still unread -- an R3 violation (refusal must be inert) and an R7d one (step 3 before step 4). Found in review. 11 envs build; host tests pass under ASan+UBSan. No hardware: landed, not closed.
…ed command An independent audit of the whole Phase 2+3 stack (fresh review context, not the incremental ones) found eight issues. Fixed here, except two dropped as hostile-DoS only; one of those came BACK in scope on its merits and is the headline fix. - ACTIVITY NOW REQUIRES AN ACCEPTED COMMAND, not merely a recognised one. The old rule let an UNAUTHENTICATED session hold the exclusive slot forever, and not only under abuse: CMD_FIRMWARE_VERSION dispatches before the auth gate, so it never draws RESP_AUTH_REQUIRED and would never have incremented Phase 4's auth-abuse counter either -- the two mechanisms were documented as exhaustive and were not. The benign triggers are a monitoring integration polling firmware version and a client retrying auth with stale credentials. A handshake still cannot race the clock: the window runs from admission, so a client gets the whole 120 s to authenticate; it just may not extend it by retrying. - The deferred cleanup flag carried no identity, so a stale one could run a destructive teardown against whoever held the slot later -- resetting a freshly admitted client's crypto and rings. It now records the owner it was raised for and acts only if the slot still holds exactly that. A cleanup raised while UNOWNED is skipped outright: zero is not a session identity and must not authorise destruction. - A peer-closed LAN socket is now reaped early in the pass (wifiLanReapClosedSession) rather than inside handleWiFiServer. Reaping there was too late: it only raises the deferred cleanup, so the accept a few lines on still tested the corpse's token and refused an ordinary reconnect -- 7d wants release before admission. - BLE event publication is atomic on both targets: payload stored before the flag, flag RELEASE-stored, consumers using __atomic_exchange_n(ACQUIRE). The ESP32 connect site had been missed by the first attempt at this and was still a plain store paired against an atomic consumer. - Comment corrections where the text had become false: the transports' "callbacks only copy and flag" contract (they claim ownership now), command_queue.h's "nRF carries these unused", link_owner.h's "loop-task-only" activity clock, and the LAN baseline description. CONNECTION_POLICY now describes the provisional accept-time window restarted at handshake completion, which is what the code does, and no longer states the supervision timeout as categorically 4-6 s (the central chooses it; the spec permits up to 32 s). - CI/docs said eleven environments. There are twelve: esp32-wrover-e-N4R8 ships via firmware-targets.json but is absent from default_envs, so a bare `pio run` skips it -- and it is a no-WiFi target, the one most likely to catch a broken #ifndef OPENDISPLAY_HAS_WIFI path. Recorded in CLAUDE.md and built explicitly. Out of scope by direction (hostile DoS): a peer that ignores link termination holding controller capacity, which the refusal scan retries every pass anyway. All 12 envs build; host tests pass under ASan+UBSan and TSan. # Conflicts: # .github/workflows/main.yaml
…onfigured The previous fix required an ACCEPTED command, which on a device with encryption enabled correctly stops an unauthenticated session pinning the exclusive slot. But encryption_enabled defaults to 0 -- documented in opendisplay_structs.h as "all commands work unauthenticated" -- and with no gate to pass, "accepted" degrades to "recognised" and the rule becomes a no-op. The hole was therefore still open on the default configuration, which is likely the common one in the field. Split the rule so it behaves the same either way: - CMD_AUTHENTICATE and CMD_FIRMWARE_VERSION never count as activity in ANY configuration. Both dispatch ahead of the auth gate and neither is work the device does for a client, so a session sending only those is idle by any useful definition. This is the half that carries the auth-off case. - Where a gate does exist, the command must also be past it, so an unauthenticated peer's refused commands cannot outlive a working client. Behaviour is unchanged for real clients in both configurations: they send real commands, which stamp. A handshake still cannot race the clock, because the window runs from admission -- 120 s against a one-exchange handshake. 12 envs build.
… optional Two loose ends found while auditing what remains. - takeConnectedEvent/takeDisconnectedEvent claimed the acquire exchange guarantees the payload belongs to the flag just consumed. It does not: release/acquire orders writes PRECEDING the release and nothing freezes the payload afterwards, so a second event landing between the exchange and the load hands us its word. Tolerable only because nothing decides on it -- teardown and connect-side work both derive from the owner token and instance table -- so the comment now says the payload is diagnostic and warns against building a decision on it. - Phase 4's stated premise is stale. It claimed Phase 3 depended on the auth-abuse counter, because the clock stamped any recognised command before the auth gate, so a never-authenticating peer kept it fresh forever. Phase 3's narrower activity rule removed that dependency: handshake/discovery opcodes never stamp, so such a peer now ages normally and the idle timeout drops it. Phase 4 is demoted from correctness requirement to optimisation -- it shortens a 120 s reclaim to about one exchange and gives the client an explicit reason instead of a silent timeout. It should be scheduled on that value, not on the old argument.
…specified as The freeze-hardening plan's Phase 2 step 6 called for "a real primitive replacing the open-coded inline clears in communication.cpp; call it here AND from those sites". Only half landed: the primitive was defined inside session_guard.cpp with no declaration in any header, and the three inline clears stayed. That left the exact drift the step existed to remove. The three sites each cleared a different subset -- one set only `active`, one also the counters, none the totals -- so a config upload aborted down one path kept a stale totalSize and expectedChunks, and the abort's own reset was the only complete one. Move the definition beside the state it owns (config_parser.cpp, declared in config_parser.h) and convert all three sites. abortToKnownState keeps calling it. Also record why the abort's step 2 (client NACK) has no code: every caller either drops the link or runs because the link is already gone, so none can deliver one. Written down so it reads as a decision rather than an omission. 12 envs build.
Drops a BLE link after OD_AUTH_ABUSE_THRESHOLD (10) consecutive commands answered
RESP_AUTH_REQUIRED, so a session that cannot authenticate stops holding the
globally-exclusive slot while it retries.
An OPTIMISATION, not a hole-closer, and built to that standard: Phase 3's narrower
activity rule already means such a peer ages normally and the idle timeout reclaims
at 120 s. This adds speed and a reason -- about one exchange instead of two
minutes, and an explicit final RESP_AUTH_REQUIRED before a deliberate drop rather
than a silent timeout. It may fail safe without reopening anything.
- Counter is BLE-only (the same auth gate is reachable from plaintext LAN, and
counting those would let LAN traffic drop a BLE client), identifies the offender
from the frame's own instance tag rather than "whichever peer the stack lists
first", and is cleared at every session end via the abort and on a successful
handshake.
- Threshold 10 chosen deliberately BELOW py-opendisplay's 16-frame pipe window:
when a session dies mid-upload every in-flight frame is doomed, so dropping at 10
beats waiting out a full window of pointless round trips. An earlier prototype
inherited this number by accident; it is now a decision.
- Best-effort delivery of the final FE: drain TX, then dwell one negotiated
connection interval, both inside a 500 ms hard bound, then abort with
dropLink=true. An empty ring proves stack acceptance of an unacknowledged
notification, not receipt, so a deadline-truncated attempt forfeits it by design.
THE ACTIVITY DECISION MOVED TWICE UNDER REVIEW, and both earlier positions were
wrong the same way -- anything that predicts acceptance is wrong at whatever layer
rejects next:
- gated on isAuthenticated() at the top: an authenticated client sending a
too-short plaintext frame stamped, then got RESP_AUTH_REQUIRED from the length
check below it -- defeating the idle timeout AND pinning the rejection run at
one, so neither mechanism could ever fire.
- just before the dispatch switch: TLS-LAN frames bypass the CCM gate but the
config-write handlers apply their own app-layer auth, so a TLS client repeating
CMD_CONFIG_WRITE stamped on every rejected attempt.
It now sits AFTER the switch and reads an outcome flag set by every rejection site
on every transport. That position has nothing below it.
The servicer also defers one pass while RX is pending, so an authentication frame
that arrived after this pass's drain is dispatched and can cancel the drop.
12 envs build; host tests pass. No hardware: landed, not closed.
davelee98
force-pushed
the
feat/phase4-auth-abuse-disconnect
branch
from
August 2, 2026 12:01
b650770 to
cdc38a7
Compare
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.
Connection-freeze hardening, Phases 2-4. 10 commits.
Split out of this PR since it was first opened, both now independent and reviewable in parallel:
What remains is one dependency chain — Phase 3 needs Phase 2's owner token, Phase 4 needs Phase 3's activity clock — which is why it stays a single PR. The commit boundaries are clean, so it reads phase by phase.
The problem
A single client could hold the globally-exclusive session slot indefinitely, and teardown was scattered across paths that each cleaned up a different subset of state.
What lands
Phase 2 — the owner token. One 32-bit lock-free word,
transport:handle:epoch, claimed by CAS in the connect callback. That is what lets a stack callback decide "is this write from the owner?" with no lock, long before any loop pass. Every queued frame carries its instance tag, so a frame written by a session that ended during a ~16 s refresh cannot dispatch into its successor — the hazard the tag exists for, since handle reuse makes a handle comparison insufficient.Phase 3 — idle drop, one activity clock across BLE and LAN. Handshake and discovery opcodes deliberately do not stamp it. The two realistic pinning triggers were benign — a monitoring integration polling firmware version, and a client retrying authentication with stale credentials — and either used to refresh the clock forever while every other client was refused the slot. A handshake still can't race the clock: the window runs from admission, so a client has the full 120 s against a one-exchange handshake.
Phase 4 — auth-abuse disconnect. Drops a BLE link after 10 consecutive commands answered
RESP_AUTH_REQUIRED, with a bounded best-effort attempt to deliver the final response first. An optimisation over the idle timeout, not a hole-closer: Phase 3 already means such a peer ages normally.abortToKnownState()as the single teardown. Transfer state, chunked config upload, touch suspend counter, crypto session, both rings, then the drop on the owner's transport, then release — strictly in that order, so a slot is never released while its link is still up. Crypto state previously survived a BLE disconnect entirely.Session watchdogs became transport-agnostic in the process, so nRF gains transfer timeouts that were ESP32-only for no reason beyond where they lived.
Verification
nrf52840customandesp32-s3-N16R8build clean. Newtools/test_link_owner.cppcovers the owner token, epoch discrimination, the terminal gate and the activity clock, built locally-Werrorunder ASan+UBSan for the single-threaded semantics and TSan for the concurrent claim/epoch-allocation paths, which are genuinely cross-task in firmware. 70,170 assertions pass.Hardware verification is incomplete, and Phase 4 is unproven. A 7-hour nRF52840 soak against an unauthenticated flooder did not produce the drop — but that device had been running since before the branch was built, so it was not executing this code. A re-test on a correctly flashed image is outstanding. Phases 2-3 have had no dedicated hardware campaign either.
Known gap, left for a follow-up: the auth-abuse counter is fed only by commands refused at the CCM gate. A client that repeatedly fails
CMD_AUTHENTICATEwith a wrong key never increments it — that opcode is dispatched from its own branch before the gate, and every failure path inhandleAuthenticate()returns without touching the counter. Such a peer is still reclaimed at 120 s, just not quickly.No wire-protocol change;
include/opendisplay_protocol.huntouched.