fix(security): restore implicit ACK for opaque relays of our own PKI DMs - #11503
fix(security): restore implicit ACK for opaque relays of our own PKI DMs#11503Ixitxachitl wants to merge 1 commit into
Conversation
… implicit ACK meshtastic#10967 (packet authenticity policies) routes DECODE_OPAQUE packets straight to relayOpaquePacket() and returns, so they never reach shouldFilterReceived(). That's correct for genuinely unauthenticated third-party traffic, but a relay of our own outgoing PKI DM also decodes as opaque from the originating node's point of view (we're never the recipient, so we can never PKI-decrypt it). The result: hearing our own PKI DM get rebroadcast no longer cancels the retransmission or generates the local implicit ACK, so every PKI DM now pays the full retry cost even when a relay already carried it onward. Route this specific case through a new narrow hook, noteOpaqueOwnRebroadcast(), instead of widening the opaque path itself. It re-runs only the existing p->from == getNodeNum() plaintext-header match against our own pending retransmissions (extracted from ReliableRouter::shouldFilterReceived into handleOwnRebroadcastImplicitAck() so both paths share it) - the same authentication boundary that check has always used, broadcast or not. It does not admit the opaque packet to PacketHistory, module dispatch, MQTT, or the NextHop/Flooding dedup state that shouldFilterReceived() also drives, so the unauthenticated-traffic surface meshtastic#10967 closed for third-party opaque packets stays closed. Adds test_C17 alongside the existing C-group opaque/authenticity tests, covering both the restored implicit ACK/cancellation and the absence of any wider pipeline admission.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
⚡ Try this PR in the Web FlasherNote Building this pull request… the flash button, badges and supported-board |
1 participant
Summary
#10967 ("feat(security): enforce packet authenticity policies") added a
passesRoutingAuthGate()step ahead ofshouldFilterReceived(). Packets that decode asDECODE_OPAQUEare routed straight torelayOpaquePacket()and returned early — they never reachshouldFilterReceived()at all.That's the right call for genuinely unauthenticated third-party traffic: we shouldn't admit packets we can't make sense of to
PacketHistory, module dispatch, MQTT, or the NextHop/Flooding dedup state.But it also silently swallows a case that isn't third-party traffic: a relay of our own outgoing PKI DM. The originating node is never the recipient, so it can never PKI-decrypt its own DM — from its own point of view that packet always decodes as opaque. Before #10967, every packet (opaque or not) reached
shouldFilterReceived(), whosep->from == getNodeNum()check would match the rebroadcast against our own pending-retransmission map and generate the implicit ACK / cancel the retransmission. After #10967, opaque packets never reach that check, so every PKI DM now pays its full retry cost even when a relay has already carried it onward — the implicit-ACK optimization (and the "stop retransmitting once someone relays it" behavior) is gone specifically for PKI DMs.Fix
Rather than widening the opaque-relay path itself (which would reopen the unauthenticated-traffic surface #10967 closed —
shouldFilterReceived()'s NextHop/Flooding paths do realstopRetransmission/dedup/history work keyed only on the plaintext header), this adds a narrow, purpose-built hook:Router::noteOpaqueOwnRebroadcast()— new virtual, no-op by default.ReliableRouter::noteOpaqueOwnRebroadcast()— overrides it, calling a newhandleOwnRebroadcastImplicitAck()helper extracted from the existingp->from == getNodeNum()block inReliableRouter::shouldFilterReceived(). Same check, same plaintext-header-only authentication boundary that's always backed it, broadcast or unicast.Router::perhapsHandleReceived()calls this new hook forOPAQUE_RELAY_ONLYpackets, right afterrelayOpaquePacket().The packet itself still never touches
PacketHistory, modules, MQTT, or the NextHop/Flooding dedup logic — only the implicit-ACK match, exactly as it worked before #10967.Testing
test_C17_opaque_own_rebroadcast_still_generates_implicit_ackalongside the existingtest_C*opaque/authenticity group, covering both the restored implicit ACK/retransmission-cancel and the continued absence of history/module/MQTT admission../bin/run-tests.sh— full suite green (58/58 suites)../bin/run-tests.sh -f test_packet_signing,-f test_nexthop_routing,-f test_mqtt— all clean.🤝 Attestations
Tested on a Seeed Studio Wio Tracker L1 only — sending a PKI DM and observing the retransmission get canceled once a relay is overheard, matching pre-#10967 behavior. Not yet tested against any other hardware target; the change is router-logic-only (no platform-specific code), but independent confirmation on other boards is welcome before merge.