feat(security): enforce packet authenticity policies - #10967
Merged
Conversation
Member
Author
|
@coderabbitai review |
Contributor
⚡ Try this PR in the Web Flasher
Warning This is an automated, unreviewed CI test build. Back up your device configuration Supported boards built by this PR (30)
Build artifacts expire on 2026-08-20. Updated for |
8 tasks
Member
Author
|
CI label check requires an |
Contributor
✅ Action performedReview finished.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR centralizes packet-authenticity decisions, adds configurable XEdDSA receive-policy enforcement, preserves signer state through warm storage, supports opaque relay, gates routing side effects, updates ingress handling, and expands packet-signing and MQTT test coverage. ChangesPacket authenticity policy
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Ingress
participant Router
participant AuthCache
participant RelayOrRouting
Ingress->>Router: submit packet
Router->>AuthCache: reuse or evaluate authentication
AuthCache-->>Router: ACCEPT, REJECT, or OPAQUE_RELAY_ONLY
Router->>RelayOrRouting: relay opaque packet or continue routing
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Member
Author
|
CI dependency status: the current Native Simulator, WASM, and Docker failures all stop at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/mesh/Router.h`:
- Around line 178-181: Shorten the API comments near the receive authentication
policy and the related declarations around `Router` to one or two concise lines
each. Retain only the essential contract; remove detailed packet-routing and
policy behavior explanations from the header comments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 634c2153-5eb0-42bd-97e8-8d8610d3f738
📒 Files selected for processing (19)
src/main.cppsrc/mesh/FloodingRouter.cppsrc/mesh/FloodingRouter.hsrc/mesh/NextHopRouter.cppsrc/mesh/NextHopRouter.hsrc/mesh/NodeDB.cppsrc/mesh/NodeDB.hsrc/mesh/Router.cppsrc/mesh/Router.hsrc/mesh/WarmNodeStore.hsrc/mesh/udp/UdpMulticastHandler.hsrc/modules/AdminModule.cppsrc/modules/NodeInfoModule.cppsrc/mqtt/MQTT.cpptest/test_mqtt/MQTT.cpptest/test_packet_signing/test_main.cppvariants/stm32/CDEBYTE_E77-MBL/platformio.inivariants/stm32/rak3172/platformio.inivariants/stm32/stm32.ini
RCGV1
force-pushed
the
codex/packet-auth-policy
branch
from
July 10, 2026 01:09
a2415ec to
82ad09a
Compare
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
This was referenced Jul 10, 2026
RCGV1
marked this pull request as ready for review
July 13, 2026 19:44
This was referenced Jul 16, 2026
Contributor
|
@RCGV1 protos have been merged. This needs to be updated now. |
Member
Author
|
Updated this branch against current I resolved the Router/NodeDB/STM32 overlap by retaining the packet-authentication policy and incorporating current PKI handling. I also made the packet-signing fixture explicitly use Local validation passed:
I could not rerun the two STM32 builds locally because PlatformIO's first-time ARM compiler download stalled; CI should cover those targets on this updated head. The label workflow is currently red because the PR has no labels. I attempted to apply |
This was referenced Jul 20, 2026
Merged
thebentern
approved these changes
Jul 20, 2026
… assertion The packet-auth-policy change extends the DecodeState enum with DECODE_OPAQUE and DECODE_POLICY_REJECT. test_E1_perhaps_decode_fuzz drives arbitrary ciphertext through perhapsDecode and asserted the verdict was one of the original three states; random ciphertext that matches no channel with no PKI attempt now returns DECODE_OPAQUE, tripping the assertion. Broaden the check to accept all five valid verdicts, matching the test's stated 'any verdict is fine' contract.
sendLocal() now dispatches local packets through handleReceived() directly instead of the mock-overridden enqueueReceivedMessage(). The MQTT implicit ACK-to-self therefore runs the full receive pipeline (RoutingModule -> MeshService::handleFromRadio -> sendToPhone), enqueuing pooled MeshPacket copies into toPhoneQueue. Production drains that queue via PhoneAPI, but the test has no phone reader, so the copies leaked at teardown (LeakSanitizer: 42824 bytes / 101 objects across test_receiveFuzzServiceEnvelope and test_receiveAcksOwnSentMessages). Give MockMeshService a destructor that drains toPhoneQueue like the phone would. Test-only; the real firmware does not leak here.
Resolve conflicts against the NodeDB signer/key primitives (meshtastic#11050) and the admin-key PKI decrypt budget (meshtastic#11100). - NodeDB: drop this branch's hasSeenXeddsaSigner in favour of develop's isKnownXeddsaSigner. They answer the same question, but develop's reads the dedicated warm signer bit (warmSignerOf) rather than the WarmProtected category, and TrafficManagementModule already depends on it. Keep develop's copyPublicKey/copyPublicKeyAuthoritative, isVerifiedSignerForKey and commitRemoteKey/KeyCommitTrust. - checkXeddsaReceivePolicy: keep this branch's Strict/Balanced/Compatible policy, which is a superset of develop's balanced-only downgrade gate, and call isKnownXeddsaSigner from it. develop's !pki_encrypted term is dropped because the policy returns early for PKI packets before that check. - perhapsDecode: keep develop's key resolution (NodeDB then pending-key, only for real PKI candidates) plus its admin-key token bucket, and re-apply this branch's pkiAttempted flag feeding the DECODE_OPAQUE verdict. Keep both passesRoutingAuthGate and adminKeyFallbackAllowed/Refund. - test_A17: model eviction the way NodeDB actually does it, passing the warm signer bit as well as the XeddsaSigner category, since isKnownXeddsaSigner reads the former. Native suite: 38 suites, 743/743 cases, no sanitizer findings.
This was referenced Jul 21, 2026
8 tasks
jamesarich
added a commit
to meshtastic/Meshtastic-Android
that referenced
this pull request
Jul 23, 2026
Radio-config sub-screens correlate admin responses to requests via a request-id set, but the id was registered only after the suspending send returned — and the send suspends until the radio acks the packet via QueueStatus. Firmware 2.8 (meshtastic/firmware#10967) routes self-addressed packets through a synchronous local loopback, so the admin response now reaches the phone BEFORE that ack. The response flowed through meshPacketFlow while the request-id set was still empty, was silently dropped by the correlation guard, and every local config screen sat at a 0% loading overlay until the 30s timeout. Every request method on RadioConfigUseCase / AdminActionsUseCase now takes an onRequestId callback invoked with the packet id before the send is issued, and RadioConfigViewModel registers there instead of after the call. The manual-channel batch helper threads the same callback through writeChannel. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thebentern
added a commit
that referenced
this pull request
Jul 24, 2026
…FS (#11190) Since #10967 made Router::sendLocal handle self-addressed packets synchronously, the entire phone-API chain for a BLE client runs inline in the Bluefruit characteristic write callback: toRadioWriteCb -> PhoneAPI::handleToRadio -> admin set-config -> radio reconfigure -> NodeDB::saveToDisk. That callback executes on the Bluefruit BLE FreeRTOS task, whose stock stack is 5 KB (CFG_BLE_TASK_STACKSIZE = 256*5 words) - not the Arduino loop task that #10944 already raised to 8 KB. The loop-task fix therefore protects the wrong task for BLE-originated writes. On a Seeed Wio Tracker L1 the 5 KB stack overflows during pairing first-sync, resetting the device mid-LittleFS-write, every single time. Repeated mid-write resets tear the LittleFS metadata, lfs_assert fires on the next boot, and the corruption handler formats the whole filesystem: region, channels, module config, and the node's keypair are all lost (critical fault #13, new node identity on next region set). Reproduced end-to-end tonight on stock develop 6908d27; with this change the same device pairs, serves config screens, and survives back-to-back config.proto saves over BLE. Raise the BLE task to the same 2048 words (8 KB) as LOOP_STACK_SZ, for the same reason. bluefruit.cpp's #ifndef guard makes the -D take effect with no framework patch. Costs 3 KB of RAM on nrf52840 targets only. Credit where due: Ixitxachitl independently established in #11155 testing that the save-path crash persists after #11185 and that re-queueing sendLocal (moving the pipeline back to the Router thread) makes it go away - which corroborates this diagnosis from the other direction. This commit is the minimal capacity-side fix; #11155's relocation of the pipeline off the BLE task remains the right architectural follow-up, and this guard stays correct even after it lands. Likely also explains #10905 (L1 display-thread crash when a client requests full configuration) and the 2.8 field reports of idle nodes losing region and keys after a BLE session.
thebentern
added a commit
to cvaldess/firmware
that referenced
this pull request
Jul 24, 2026
…astic#11185) * Deliver locally-generated replies addressed to us to the phone Config get/set from the phone times out on every device: the client sends an admin request, the node handles it, and the response is silently dropped before it reaches the phone queue. meshtastic#10967 changed Router::sendLocal's isToUs branch from enqueueReceivedMessage() to handleReceived(p, src), so a local packet keeps its RxSource instead of being relabeled RX_SRC_RADIO by the queue round-trip. That is the right call for the new policy gates, but module replies go out through MeshService::sendToMesh() with the default RX_SRC_LOCAL, and a reply to a phone-originated request is addressed to our own node (setReplyTo resolves from == 0 to ourNodeNum). Those replies now re-enter callModules as RX_SRC_LOCAL, where the loopback gate skips every module whose loopbackOk is false - including RoutingModule, whose promiscuous sniff is the only path that moves a received packet into toPhoneQueue. The reply is released, never sent. Requests still work, because the phone's own packets arrive as RX_SRC_USER and pass the gate, so a set_config is applied and only its acknowledgement is lost. That is why a client can connect and download config but times out on every config screen and every setter. Deliver the phone's copy from sendToMesh() instead: for a local packet addressed to us, the loopback gate is doing its job in keeping the packet away from module re-dispatch, and the phone copy is exactly what is missing. Setting loopbackOk on RoutingModule would instead echo every locally-generated broadcast back to the phone, and relabeling replies RX_SRC_RADIO would undo the origin separation meshtastic#10967 added. Also stop reporting ERRNO_SHOULD_RELEASE (35) to the phone in the QueueStatus for these packets. It means "caller frees", not a send failure, and the same hunk changed it from the 0 the phone used to see. * Address review: trim comments, assert the QueueStatus count Condense the added comments to the one-or-two-line house style; the rationale lives in the commit message and PR. The reply test drained QueueStatus records in a while loop, which would have passed just as happily on an empty queue. Count them and require both the request's and the reply's.
jeremiah-k
pushed a commit
to jeremiah-k/meshtastic-firmware
that referenced
this pull request
Jul 25, 2026
…stic#11202) * Fix nRF52 freeze + watchdog reset when saving config over BLE Since meshtastic#10967 phone-originated admin messages are handled synchronously on Bluefruit's BLE event task. NRF52Bluetooth::disconnect() busy-waited for BLE_GAP_EVT_DISCONNECTED, which only that same task can process, so any config save that requires a reboot (e.g. position) deadlocked the device until the 90s watchdog fired. Bound the wait to 1s and sleep instead of spinning so lower-priority tasks (including the watchdog feed) keep running; the SoftDevice completes the link termination on its own. * Address review: use Throttle helper, tighten comment * Name the disconnect timeout constant * Log unconfirmed BLE disconnect at WARN with elapsed time
NomDeTom
pushed a commit
to NomDeTom/MeshtasticFirmware
that referenced
this pull request
Jul 25, 2026
Since meshtastic#10967 phone-originated admin messages are handled synchronously on Bluefruit's BLE event task. NRF52Bluetooth::disconnect() busy-waited for BLE_GAP_EVT_DISCONNECTED, which only that same task can process, so any config save that requires a reboot (e.g. position) deadlocked the device until the 90s watchdog fired. Bound the wait to 1s and sleep instead of spinning so lower-priority tasks (including the watchdog feed) keep running; the SoftDevice completes the link termination on its own.
madeofstown
pushed a commit
to madeofstown/meshtastic-firmware
that referenced
this pull request
Jul 26, 2026
…stic#11202) * Fix nRF52 freeze + watchdog reset when saving config over BLE Since meshtastic#10967 phone-originated admin messages are handled synchronously on Bluefruit's BLE event task. NRF52Bluetooth::disconnect() busy-waited for BLE_GAP_EVT_DISCONNECTED, which only that same task can process, so any config save that requires a reboot (e.g. position) deadlocked the device until the 90s watchdog fired. Bound the wait to 1s and sleep instead of spinning so lower-priority tasks (including the watchdog feed) keep running; the SoftDevice completes the link termination on its own. * Address review: use Throttle helper, tighten comment * Name the disconnect timeout constant * Log unconfirmed BLE disconnect at WARN with elapsed time
3 tasks
8 tasks
8 tasks
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
2 participants
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.
Summary
Adds a device-owned receive policy for Meshtastic's existing XEdDSA and PKI authentication results:
Strict applies across packet types and destinations, including positions, NodeInfo, telemetry, text, routing/control, broadcasts, and unicasts. The policy is enforced before NodeDB/history storage, modules, connected clients, plaintext MQTT, ACK side effects, or rebroadcast of decryptable rejected traffic.
This PR does not add a signature version, change the existing signed tuple, change PKI/AES-CCM bytes, or make the receive policy affect outbound traffic. Licensed/ham plaintext signing is intentionally separate in design#122 / #10966.
Implementation
SecurityConfig.packet_signature_policyand reportsDeviceMetadata.has_xeddsaValidation
trunk checkandgit diff --check: cleand6b12ea3f1fc83afcff63a7fb511d54c52cc871aNo RF/BLE/serial hardware verification has been performed. This PR must remain draft until real-device verification is recorded.
Depends on meshtastic/protobufs#983.
Closes #10963
Design: meshtastic/design#121
Summary by CodeRabbit