Authorize signed plaintext remote admin in licensed mode - #10972
Conversation
|
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:
📝 WalkthroughWalkthroughLicensed-mode routing authentication, identity migration, remote admin authorization, channel sanitization, persistence, and related tests are updated across radio, MQTT, boot, and configuration flows. ChangesLicensed authentication flow
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
⚡ 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 (27)
Build artifacts expire on 2026-08-10. Updated for |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
CI label check requires an |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
CI dependency status: the current Native Simulator, WASM, and Docker failures stop at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/mesh/FloodingRouter.cpp (1)
71-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep this rationale to two lines.
This three-line comment exceeds the repository’s comment limit.
Proposed cleanup
- // Verify the replacement before deleting the valid lower-hop copy waiting in the TX queue. - // This is intentionally redundant with ReliableRouter's ingress gate: it keeps this helper - // safe if another caller is introduced later. + // Re-authenticate before replacing the queued lower-hop copy so future callers remain safe.As per coding guidelines, “Keep code comments minimal: one or two lines maximum.”
🤖 Prompt for 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. In `@src/mesh/FloodingRouter.cpp` around lines 71 - 73, Shorten the explanatory comment above the replacement verification in FloodingRouter to no more than two lines while preserving its key rationale: verify the replacement before deleting the valid lower-hop TX-queued copy, even though ReliableRouter already enforces this at ingress.Source: Coding guidelines
🤖 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.cpp`:
- Around line 98-105: Guard storeRoutingAuthCache with an early return when
routingAuthCacheLock is null, matching routingAuthCacheMatches,
applyRoutingAuthCache, and clearRoutingAuthCache, before constructing the
LockGuard.
- Around line 655-671: Sanitize the already-decoded fallback packet before
returning from perhapsDecode(), because the routing-auth cache may be clobbered
between passesRoutingAuthGate() and handleReceived(). In the
authCandidate.which_payload_variant == meshtastic_MeshPacket_decoded_tag path,
clear serialized authentication fields directly on *p (including xeddsa_signed,
public_key, and pki_encrypted) before copying or caching the packet, or
otherwise make the gate-to-handle handoff atomic.
In `@test/test_admin_radio/test_main.cpp`:
- Around line 1029-1066: Restore all modified global test state before each test
returns. In test_bootDefense_sanitizesStaleLicensedChannelsOnce and
test_restorePreferences_sanitizesLicensedBackupBeforeReturn, save the original
owner and relevant channel state at the start, then restore them on every exit
path; ensure the restore test also cleans up its temporary NodeDB and backup
resources. Use the existing test setup/cleanup patterns so later tests do not
inherit licensed or sanitized state.
---
Nitpick comments:
In `@src/mesh/FloodingRouter.cpp`:
- Around line 71-73: Shorten the explanatory comment above the replacement
verification in FloodingRouter to no more than two lines while preserving its
key rationale: verify the replacement before deleting the valid lower-hop
TX-queued copy, even though ReliableRouter already enforces this at ingress.
🪄 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: ec360569-c027-4563-9843-1875992d71d6
📒 Files selected for processing (24)
src/main.cppsrc/mesh/Channels.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/AdminModule.hsrc/modules/NodeInfoModule.cppsrc/mqtt/MQTT.cpptest/support/AdminModuleTestShim.htest/support/MockMeshService.htest/test_admin_radio/test_main.cpptest/test_mqtt/MQTT.cpptest/test_packet_signing/test_main.cppvariants/stm32/CDEBYTE_E77-MBL/platformio.inivariants/stm32/rak3172/platformio.inivariants/stm32/stm32.ini
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Native simulation fixes are now pushed for MQTT ingress ownership and signed-packet metadata handling, with companion test-fixture stabilization. Validation completed with the targeted native suites passing 141/141 and the full coverage suite passing 618/618. |
Hardware validation: PASSValidated the exact PR head
Results over RF using directed
Cleanup completed: the temporary Muzi allowlist entry was removed, diagnostic streaming disabled, and both devices passed final serial liveness checks. Note: current meshtastic-python remote admin defaults to PKI encryption, so this validation intentionally used the new required plaintext signed transport path directly ( |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/test_mqtt/MQTT.cpp (1)
58-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKeep the teardown comment within two lines.
The rationale is useful, but this three-line comment exceeds the project limit.
Proposed fix
- // handleFromRadio queues packet copies for the phone. There is no phone poller in this - // unit test, so release any queued copies before LeakSanitizer checks process shutdown. + // This test has no phone poller; drain queued copies so LeakSanitizer sees no leak.As per coding guidelines, comments must be minimal—one or two lines—and only explain non-obvious reasons.
🤖 Prompt for 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. In `@test/test_mqtt/MQTT.cpp` around lines 58 - 60, Shorten the teardown comment above the getForPhone() loop to no more than two lines while preserving its explanation that queued packet copies must be released because the unit test has no phone poller before LeakSanitizer shutdown checks. Leave the getForPhone() cleanup loop unchanged.Source: Coding guidelines
🤖 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 `@protobufs`:
- Line 1: Update the protobufs submodule reference in the .gitmodules
configuration to a reachable commit that contains the
meshtastic_DeviceMetadata.has_xeddsa field, ensuring downstream builds can
resolve the submodule and regenerate the matching protobuf.
In `@src/mqtt/MQTT.cpp`:
- Around line 95-97: Shorten the comments near the local MQTT acknowledgement
handling and the authentication logic to no more than two lines each, retaining
only the non-obvious ownership or authentication rationale. Remove explanatory
details about otherwise obvious code behavior while preserving the
implementation unchanged.
---
Nitpick comments:
In `@test/test_mqtt/MQTT.cpp`:
- Around line 58-60: Shorten the teardown comment above the getForPhone() loop
to no more than two lines while preserving its explanation that queued packet
copies must be released because the unit test has no phone poller before
LeakSanitizer shutdown checks. Leave the getForPhone() cleanup loop unchanged.
🪄 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: 459dbfd4-6a98-4444-85e5-1bcdd26cbfce
⛔ Files ignored due to path filters (5)
src/mesh/generated/meshtastic/config.pb.cppis excluded by!**/generated/**,!src/mesh/generated/**src/mesh/generated/meshtastic/config.pb.his excluded by!**/generated/**,!src/mesh/generated/**src/mesh/generated/meshtastic/deviceonly.pb.his excluded by!**/generated/**,!src/mesh/generated/**src/mesh/generated/meshtastic/localonly.pb.his excluded by!**/generated/**,!src/mesh/generated/**src/mesh/generated/meshtastic/mesh.pb.his excluded by!**/generated/**,!src/mesh/generated/**
📒 Files selected for processing (7)
protobufssrc/modules/AdminModule.cppsrc/mqtt/MQTT.cpptest/test_admin_radio/test_main.cpptest/test_fuzz_packets/test_main.cpptest/test_mqtt/MQTT.cpptest/test_packet_signing/test_main.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- test/test_admin_radio/test_main.cpp
- src/modules/AdminModule.cpp
Preserve and publish identity keys in licensed mode so existing XEdDSA signatures can authenticate plaintext traffic. Sign licensed broadcasts and direct messages when they fit, while keeping PKI encryption disabled and normal routing behavior unchanged.
Remove the process-global signer context and carry only the exact Router-verified key on the authenticated packet. Clear serialized authentication metadata before every untrusted decode, require both the verified-signature marker and packet-local key in licensed Admin authorization, and expand authorization/session/isolation coverage while cleaning routing test ownership.
Make the routing authentication gate input const and collapse licensed Admin authorization into a single branch so legacy response, local, channel, and PKI checks run only for non-licensed-remote traffic.
65db823 to
ba1be30
Compare
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/modules/AdminModule.cpp`:
- Around line 147-170: Update handleReceivedProtobuf’s accept-chain logic to
treat an authorized licensed signer as an accepted admin payload, allowing
directed plaintext allowlisted mutations to reach the existing passkey/handler
path instead of the final NOT_AUTHORIZED branch. Reuse authorizedLicensedSigner
from the licensed-signer validation and preserve the existing response,
local-admin, admin-channel, and PKI-encrypted acceptance behavior.
🪄 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: 2e2ffc8b-8c35-44ca-b395-753fab340c72
📒 Files selected for processing (15)
src/main.cppsrc/mesh/Channels.cppsrc/mesh/FloodingRouter.cppsrc/mesh/NodeDB.cppsrc/mesh/NodeDB.hsrc/mesh/Router.cppsrc/mesh/Router.hsrc/modules/AdminModule.cppsrc/modules/AdminModule.hsrc/modules/NodeInfoModule.cppsrc/mqtt/MQTT.cpptest/support/AdminModuleTestShim.htest/support/MockMeshService.htest/test_admin_radio/test_main.cpptest/test_packet_signing/test_main.cpp
🚧 Files skipped from review as they are similar to previous changes (4)
- test/support/MockMeshService.h
- src/mesh/Channels.cpp
- test/test_packet_signing/test_main.cpp
- src/mesh/FloodingRouter.cpp
1 participant
Closes #10971
Design: meshtastic/design#123
Stack dependencies
This is a stacked draft that combines and depends on:
Review and merge the prerequisite drafts first. This PR intentionally targets
developbecause both prerequisite heads are contributor branches, so its current diff contains their changes as merge parents.Summary
security.admin_key[]entry;TOO_LARGEwhen the packet cannot carry its required signature;from | id | portnum | payloadtuple.This does not introduce an authentication version, new signature format, certificate system, new protobuf field, or encryption in licensed mode.
Security properties
MeshPacket.public_keyandxeddsa_signedare cleared at untrusted ingress and become authoritative only after successful Router verification for that packet.Validation
git diff --check: clean;4cd335d03fd9498cc3c8fa12cfc51b5834d65e5a.Local full native execution is host-blocked by existing macOS/Linux build incompatibilities: NotificationRenderer initialized VLAs, Linux input headers, and the coverage environment's GCC-only
-fprofile-abs-pathunder Apple Clang. GitHub Linux CI is the executable validation source for this stacked draft.Draft / hardware status
No RF hardware validation has been performed. Before this leaves draft:
pki_encrypted=falseand uses verified-signature shield semantics rather than an encryption lock.Operators remain responsible for their licence conditions, jurisdiction, permitted modes, and station-identification requirements.
Summary by CodeRabbit
New Features
Bug Fixes
🤝 Attestations