feat: remote-announced operator/stop-only role over the pstop aux channel - #107
Conversation
Introduce common/pstop_aux_channel.h, a non-certified wrapper header that defines a small bidirectional data channel over the pstop_c v2 message padding (padding1/padding2 from #104). pstop_c itself is untouched. - Uplink (padding1): remote-announced role (unspecified/stop_only/operator), versioned, with fail-safe decode (unknown version -> unspecified, bad role byte -> stop_only). - Downlink (padding2): reserved + plumbed for future machine->remote feedback. - Wire the shared include path into host, machn+firmware (via dcs_support), and the ROS 2 machine build. - docs/PSTOP_AUX_CHANNEL_DESIGN.md documents the layout, role semantics (AND-rule, fail-safe, rebond-on-change), and rollout. - host test_aux_channel covers round-trip + fail-safe paths.
The remote now owns its role and announces it in every pstop frame's aux uplink (padding1), so the machine can decide re-arm eligibility from the remote's own claim instead of machine-side config alone. - NVS key 'role' (u8, default stop_only) with fail-safe read; a fresh or NVS-wiped remote can never arm until deliberately promoted. - Lock-free RAM mirror (dcs_role_get/dcs_role_set) so both safety cores encode an identical role byte — the comparator memcmp still covers it. - Encode the role on every heartbeat and on BOND (so the machine has the claim cached before it accepts the bond). - GET/POST /api/role (admin-authed) + a remote WebUI toggle; role exported in state.json. - Read+ignore the reserved machine->remote downlink field in the reply drain (future feedback hook). pstop_c untouched.
The machine now decides re-arm eligibility from the remote's announced role
ANDed with its existing operator allowlist, all in wrapper code (pstop_c
untouched).
- Lock-free {id -> claimed_role} map, written by the comparator before it
notifies the cores so details_default sees the right role at bond time.
- details_default AND-rule: operator only if the remote claims OPERATOR AND
its id is allowlisted; otherwise stop-only (fail-safe).
- Role transition on a bonded remote is contained via public API only
(pstop_remote_deactivate + machine_stop_robot on both instances): the bond
is dropped and STOP forced, so the remote must re-bond with the new
authorization and a fresh gesture is required (invariant 3).
- Reply emits a zeroed aux downlink (reserved for future feedback), identical
on both cores so the comparator still agrees.
- claimed_role surfaced per remote in state.json bonded_remotes[].
Mirror the machn role enforcement in the standalone host machine (single threaded, so a plain id->role map; pstop_c untouched). - Note the announced role before machine_process_message so the AND-rule in is_operator_allowed sees it at bond; operator authority now requires both a config operator entry and an OPERATOR announcement. - Contain a role transition on a bonded remote via public API (pstop_remote_deactivate + machine_stop_robot): drop the bond and force STOP so a fresh, correctly-authorized gesture is required. - Emit a zeroed aux downlink on replies (reserved for future feedback).
Mirror the machine role enforcement in the ROS 2 software backend and surface
claimed_role through the bridge (pstop_c untouched).
- software_backend: machine-thread-only {id -> role} map; note the role before
machine_process_message; AND-rule in cb_remote_details (operator requires
both the allowlist verdict and an OPERATOR announcement); contain a role
transition via pstop_remote_deactivate + machine_stop_robot; emit a zeroed
aux downlink on replies.
- backend RemoteInfo + BondedRemote.msg gain claimed_role; machine_bridge_node
publishes it on ~/remotes.
- hardware_backend passes claimed_role (and stop_only) through from the machn
state.json.
Add a udev rule tagging the pstop USB vendor id (303a) with ID_MM_DEVICE_IGNORE so ModemManager never probes the ttyACM. Without it, MM grabs the port on every download-mode re-enumeration and esptool fails to flash with 'Write timeout'/EPROTO. Installed by host/setup/install.sh.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also checked the host machine's analogous role-tracking table (host/machine_app_runner.c role_note()): unlike the ROS 2 backend's unbounded std::unordered_map, it's a fixed-size array (ROLE_MAP_MAX = 32) that simply stops recording new ids once full, so it isn't exposed to the same unbounded-growth DoS.
Extended reasoning...
Confirmed findings already exist for this PR (unbounded per-remote map growth in the ROS 2 software backend, STOP/rebond triggered by unauthenticated/unbonded device ids in machn, and a cross-core torn-read race on the announced role in the ESP32 remote firmware), so this run should not approve. I additionally spot-checked the host machine's equivalent role-tracking structure (host/machine_app_runner.c, s_role_id/s_role_val arrays sized ROLE_MAP_MAX) since it plays the same role as the vulnerable ROS 2 claimed_roles map, and confirmed it is a fixed-size array that silently stops accepting new ids once full rather than growing unbounded — so it is not exposed to the same memory-exhaustion path, even though the CRC-only authentication model is shared across all three machine implementations. This is offered as a narrow, concrete ruled-out item alongside the inline findings, not as a broader safety verdict on the PR.
Address PR review: - machn (F2, was: any id could force STOP): gate the role record and the forced-STOP/rebond on the id being a bonded remote (or an incoming BOND), matching the host + ROS 2 backends. An unbonded/spoofed id can no longer toggle its announced role to force a machine STOP. - ROS 2 software backend (F1, was: unbounded map DoS): only record roles for bonded-or-BOND ids, and reconcile claimed_roles against the live bond set in rebuild_snapshot() every iteration — the forget counterpart to machn's claimed_role_forget(), bounding the map to the bond-slot count even if a remote times out without an explicit unbond. - remote firmware (F3, cross-core encode race): snapshot the announced role into g_tick once per tick (like every other field) instead of each core reading dcs_role_get() live, so a concurrent /api/role write can no longer make the two cores encode different role bytes and withhold a tick.
|
Pushed 24ec076 addressing the review:
Verified: |
On-hardware re-validation of the fixes (live PSTOP45 remote + local ROS 2 Jazzy node)Re-flashed the F3 firmware and re-ran the enforcement end-to-end:
F2 (machn) is build-verified (ESP-IDF) and now mirrors the host/ROS 2 |
There was a problem hiding this comment.
Since my last review, the three findings I flagged (unbounded/DoS-able role maps in host and ROS 2, and the unauthenticated-id forced-STOP in machn, plus the firmware cross-core encode race) were addressed by commits 24ec076 and e6fdb6d — the latter goes further and removes the persistent role-map design entirely in favor of latching the role only from the BOND message being processed, which eliminates the DoS/spoofing surface by construction rather than by adding gates. I verified this directly in the diff for host/machine_app_runner.c, machn/main/main.c, and ros2/protective_stop_machine/src/software_backend.cpp, and confirmed no leftover references to the removed claimed_role/role-map APIs across the tree. No new findings beyond what's already been raised and fixed.
Extended reasoning...
This is the third review pass on this PR. My two prior reviews (posted as inline comments) flagged: (1) an unbounded, spoof-fillable role map in the ROS 2 software_backend.cpp causing a heap-growth DoS; (2) machn's role-change handler force-STOPping the machine for any device id, bonded or not, allowing an off-network spoofed-id DoS on the safety machine; (3) the host's 32-slot role map similarly fillable by spoofed ids with no eviction, permanently denying legitimate remotes operator status; and (4) a cross-core role-encode race in firmware/main.c.
Two follow-up commits landed after those reviews: 24ec076 ("harden role tracking against spoofed ids + cross-core race") gated the role record/forced-STOP on bonded-or-BOND ids and fixed the firmware race by snapshotting the role once per tick in the comparator rather than reading it live per core. Then e6fdb6d ("latch remote roles only at bond") went further and removed the persistent role-map/role-change-forces-STOP mechanism entirely across host, machn, and the ROS 2 backend, replacing it with a much simpler design: the role is read only from the BOND message currently being processed (exposed via a single scalar, not a map), and pstop_c latches the resulting stop_only/operator decision into the bonded client for the life of that bond. Role changes now only take effect via an explicit reboot (POST /api/role persists to NVS and calls esp_restart()), and the design doc, BondedRemote.msg, dcs_publish_machn_remote signature, and test_aux_channel.c were all updated consistently — I grep'd the tree and found no dangling references to the removed claimed_role/role-map APIs.
I re-read the diffs for all three enforcement sites (host/machine_app_runner.c, machn/main/main.c, ros2/protective_stop_machine/src/software_backend.cpp) plus common/pstop_aux_channel.h, docs/PSTOP_AUX_CHANNEL_DESIGN.md, host/test_aux_channel.c, and tools/pstop_multi_remote_test.py, and the new design is internally consistent: no persistent per-id state exists anywhere for the role channel, so the DoS/spoofing vectors I previously raised are structurally gone rather than merely mitigated. I was not able to actually execute make test in this sandbox (bash denied), so I relied on manual diff review rather than running the host test suite, but the change is small, mechanical, and consistent across all three backends.
Given all three of my prior findings were substantively fixed (not just patched defensively but redesigned away), and no new issues surfaced in this pass, the appropriate action is a brief informational note rather than a full re-review — the PR author already has the fix commits in front of them and doesn't need the original findings restated. This falls under the narrow "ruled out beyond the findings" exception: I'm recording that I checked the fix commits against the original findings and confirmed they were addressed, which is new information relative to my prior reviews (which predate these fixes).
There was a problem hiding this comment.
Checked the three commits pushed since the last review round (24ec076, e6fdb6d, 8b0c308) against my prior inline findings: the unbounded/spoofable per-id role maps in ros2/protective_stop_machine/src/software_backend.cpp and host/machine_app_runner.c were replaced with a single "latch role only while processing a valid BOND from a known-or-BOND sender" scalar that's cleared immediately after processing (no persistent per-id state to exhaust or spoof-toggle), and machn/main/main.c's unauthenticated forced-STOP-on-role-toggle path was removed along with it. The firmware cross-core race was resolved by making the role immutable at runtime (loaded once at boot; changes go through NVS + immediate esp_restart()) rather than snapshotting it per-tick.
Extended reasoning...
This run's bug hunt reported no new findings. I independently re-verified, by reading the current diff-tip code in ros2/protective_stop_machine/src/software_backend.cpp, host/machine_app_runner.c, machn/main/main.c, and firmware/components/dcs_support/src/dcs_support.c, that the three previously-posted inline findings (unbounded ROS2 claimed_roles map / unbounded host role table DoS, machn's unauthenticated forced-STOP on spoofed role toggles, and the firmware cross-core role-encode race) were addressed by commits 24ec076, e6fdb6d, and 8b0c308 — not merely mitigated but structurally removed (single BOND-scoped scalar instead of a per-id map; role now immutable at runtime instead of read live per core). Because this is the fourth review pass on a security-relevant safety-system PR (remote-announced authorization role affecting E-stop re-arm), and because a full independent re-audit of the whole diff (wire format, NVS persistence, admin API auth, three separate machine backends) is warranted rather than trusting my own prior-run's conclusions, I'm not approving — a human should do a final pass, particularly to confirm the "role immutable per boot" invariant holds across all three backends and that the AND-rule policy change is the intended security posture. This message is purely informational about what was re-verified as fixed; it does not restate the original finding content.
## Summary - record OSHWA open source hardware certification for Protective Stop v1.2 (`US002846`) - add the official UID-bearing OSHW wide mark and link it to the certification registry - add a generated license-facts graphic for CERN-OHL-P-2.0, Apache-2.0, and CC-BY-4.0 - rewrite the main README to be shorter, clearer, and more natural while preserving the safety boundary - replace stale fixed-10 Hz network claims with machine-controlled heartbeat language - update the hardware manifest, contribution licensing, and certification-mark guidance The fixed internal sensing/comparator cycle and historical 10 Hz test results remain documented where they are accurate. Only claims that described the network update rate as fixed were changed. ## Verification - verified the live OSHWA registry entry and certification metadata - generated the certification mark from OSHWA official v2.6 wide artwork - generated the license facts image from the linked OSHWA facts generator - validated both SVGs with `xmllint` and rendered them locally for visual inspection - `pre-commit run --all-files` - independent technical prose review and follow-up accuracy pass ## CI note All required checks, firmware builds, host checks, and ROS 2 builds pass. The optional aggregate coverage job is already failing on `main` after the pstop v2 wire-size change; #107 contains the v2 harness repair. This PR intentionally does not duplicate that fix so it can merge first without creating a conflict. ## Follow-up outside this repository The OSHWA registry description still says the remote heartbeats at 10 Hz. That text should be updated in the OSHWA application to describe a machine-controlled rate (5 Hz with the default 400 ms heartbeat window).
POST /admin/api/peers/allowed updated RAM + NVS and stopped. Every filter gate reads the live list, so removals and already-admitted peers were immediate — but a peer add_peer() had previously rejected holds no WG slot, and steady-state OmitPeers=true map deltas never re-offer it. It only came back on the next periodic coord re-register (ML_COORD_REREGISTER_MS): on bbd8 today the operator's host became reachable +286 s after the add. When the new list contains an ip that was not in the old one, request full peer redelivery on the next MapRequest and fire the hitless re-register now (same recipe as the pin-absent heal in ml_wg_mgr.c; peers/WG preserved, not counted as a flap). Removals and relabels do nothing extra.
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding on ml_config_httpd.c:867, I also checked two related spots and ruled them out: the fleet-server IP re-injection (ml_config_httpd.c:847) never needing added_new because ml_config_peer_is_allowed unconditionally allows the fleet IP before any list/WG-slot check runs; and the one-shot s_want_rereg flag (ml_coord.c:2941) being cleared only inside COORD_LONG_POLL's periodic block, which mirrors how s_last_rereg_ms is already handled there and is processed on the very next loop pass.
Extended reasoning...
This run's new commit (277ddd5, "make a newly allowlisted peer reachable immediately") is the first substantive change since my prior "no issues" review on this PR. It adds added_new tracking to handler_post_allowed in ml_config_httpd.c so a freshly-allowlisted peer gets an immediate coord re-register instead of waiting up to 5 minutes. The confirmed inline finding correctly identifies that this new logic only fires on additive changes (a new ip appearing in the posted list) and misses the symmetric case where filter_enabled transitions from true to false (via DELETE /api/peers/allowed or an empty POST when no fleet IP is configured) — in that case ml_config_peer_is_allowed's fast path immediately allows all peers, but any peer previously rejected by add_peer() still holds no WireGuard slot and won't get one until the periodic re-register fires. That is a real, newly-introduced latency/reachability gap worth flagging as an inline comment, which the bug hunter already did.
I additionally verified two other candidate gaps in the same area and ruled both out. First, the fleet-server IP auto re-injection block (around ml_config_httpd.c:847) doesn't set added_new when it adds the fleet IP back into the persisted list — but ml_config_peer_is_allowed() returns true for the fleet IP unconditionally, before the filter-enabled check or the list-membership check, so add_peer() never rejects it regardless of list state; it therefore never needs the reachability nudge this fix is providing. Second, s_want_rereg in ml_coord.c is only read/cleared inside the COORD_LONG_POLL case's periodic-check block (line ~2932-2941) — but that block runs on every pass through that state while connected (it sits alongside other periodic checks like STUN retries and the s_last_rereg_ms-driven periodic re-register, using the exact same pattern), so a request set while in another state or a busy loop iteration is simply picked up on the next pass, not lost.
Since a confirmed finding already exists as an inline comment and is sufficient to signal that a human should look at this PR, I'm not posting an approve/defer verdict about the PR as a whole — just this brief note recording the additional spots I checked and ruled out, per the narrow exception for that case.
| } | ||
| ctx->filter_enabled = (ctx->peer_list.count > 0); | ||
| xSemaphoreGive(ctx->peer_mutex); | ||
| free(old_ips); | ||
| } | ||
| cJSON_Delete(json); | ||
|
|
||
| config_save_peers(ctx); | ||
|
|
||
| ESP_LOGI(TAG, "Allowlist updated: %d peers, filter %s", ctx->peer_list.count, ctx->filter_enabled ? "ON" : "OFF"); | ||
|
|
||
| /* Make a NEW ip reachable now, not at the next 5-min periodic re-register: | ||
| * the same recipe the pin-absent heal uses (ml_wg_mgr.c) — full peer | ||
| * redelivery on the next MapRequest, and that MapRequest immediately via | ||
| * the hitless re-register path (peers/WG sessions preserved, not a flap). */ | ||
| if (added_new) { | ||
| ESP_LOGI(TAG, "Allowlist gained a new ip — requesting immediate coord re-register"); | ||
| ml_coord_request_full_peers(); | ||
| ml_coord_request_reregister(); | ||
| } |
There was a problem hiding this comment.
🟣 Pre-existing gap this fix leaves open: added_new only fires when a specific ip is newly listed, but never when filter_enabled flips true->false (allowlist cleared/disabled), even though ml_config_peer_is_allowed's fast path then allows ALL peers immediately. Those other peers still lack a WG slot and, exactly as before this diff, wait up to ML_COORD_REREGISTER_MS (5 min) for reachability -- the same problem this PR set out to fix, just for a different (and broader) transition. Fix: also call ml_coord_request_full_peers()/ml_coord_request_reregister() whenever ctx->filter_enabled transitions from true to false, not only when a specific ip is added to a still-nonempty list.
Extended reasoning...
Allowlist has {A}; filter_enabled=true; peer B was previously filtered out by ml_wg_mgr (no WG slot). Admin sends DELETE /api/peers/allowed (handler_delete_allowed, ml_config_httpd.c:876-895, untouched by this diff) or POST {"peers":[]} when CONFIG_ML_FLEET_SERVER_IP is unset -> ctx->filter_enabled becomes false (line 849/884). ml_config_peer_is_allowed's fast path (line ~309) now returns true for B unconditionally, but the new added_new tracking in handler_post_allowed never observed B in the submitted peers array (it was empty), so ml_coord_request_full_peers/reregister are never called; B stays unreachable until the periodic 5-minute timer, reproducing the exact bug this commit claims to fix for the 'disable filtering entirely' transition.
Verification: pre-existing. The mechanics are real: add_peer() at ml_wg_mgr.c:2014-2017 rejects a non-allowed peer with return -1 and no WG slot, so a previously-filtered peer B genuinely has no slot. The new reregister gate at ml_config_httpd.c:863 if (added_new) fires only when a specific new ip appears; on a filter true->false transition (POST {"peers":[]} → added_new stays false; count 0 →…
2 participants
What
Let a remote announce its own role (
operatororstop_only) in the CRC-protectedpadding1field added to the pstop v2 message. A machine grants re-arm authority only when the remote announcesOPERATORand its existing machine-side policy permits that ID.pstop_cremains unmodified. Wrapper code owns the schema and policy.Design
common/pstop_aux_channel.hdefines the versioned role values and fail-safe codec.POST /api/rolepersists the new role and reboots the remote.pstop_cthen latches the effectivestop_onlypolicy into the client.See
docs/PSTOP_AUX_CHANNEL_DESIGN.md.Scope
fix(microlink): a newly allowlisted peer becomes reachable immediately.POST /admin/api/peers/allowed(and the fleetPUT /api/remotes/<id>/peersproxy) now requests full peer redelivery + a hitless coord re-register when the new list contains an ip absent from the old one. Previously a peer thatadd_peer()had rejected waited for the 5-min periodic re-register (ML_COORD_REREGISTER_MS). Removals/relabels do nothing extra; not counted as a flap. Found while bench-testing this PR.Future machine-to-remote feedback, claimed-role telemetry, and unrelated host udev changes are intentionally excluded.
Rollout
The pstop v2 message is a hard 40-to-48-byte wire cutover. Update remotes and machine implementations together. Existing remotes without the role key boot as stop-only and require explicit promotion before they can re-arm.
Verification
On-target hardware (2026-09-02, build
v1.2-25-ga60c74athenv1.2-26-g277ddd5)Two remotes OTA'd over Tailscale; machine =
host/machine_app_runner(allow_unlisted=true,default_stop_only=false, so the announced role is the only gate). DUT01d7f344on the HIL rig: loops A/B and DUT power driven by the relay board./admin/api/ota), both unitsota_state=2, no rollback, no new crash recordstop_only(GET /api/role+state.json)POST /api/rolenegativesPOST /api/role?role=operatorpadding1=0x00000201(OPERATOR) /0x00000101(STOP_ONLY);padding2=0; counter monotonicSTOP_RECEIVED→ARMED (held 3.8 s)→ROBOT STATUS -> OKreset_reason=POWERON; fw +role=operatorpersisted; machine STOP via liveness; re-bond and re-arm on next pressoperator)stop_only→ wire flipped to0x0101after rebootv1.2-26-g277ddd5)peer_allowlist_rejects=677, add via fleet → reachable +7.4 s;coord_reregisters+1,ml_reconnects0Both
pstop_remote.binandmachn_machine.binforv1.2-26-g277ddd5are in the fleet registry (unassigned). ELFs inops/elf-archive/.Note for
tools/hil/:test_10_button.pyexpects press/release to arm without any role setup; on this build a fresh DUT defaults tostop_only, so the suite needs a fixture that POSTsrole=operator(or the DUT pre-provisioned). The rig DUT is currently left asoperator.Host / CI (merged head,
origin/main07211efmerged in, no conflicts)pstop_ccmake build (-Werror -fanalyzer)pstop_c/build/pstop/pstop_testpstop_c/build/pstop/pstop_requirements_test(incl. new req_3_18-3_23)make -C host test(clock_guard, crypto_kat, demote_veto, aux_channel)make -C firmware/test run CC=gcc-14(estop_verdict MC/DC)tools/pstop_multi_remote_test.py(live runner + chaos proxy)colcon test protective_stop_machine(Humble, local)