Use signed plaintext for licensed remote Admin#6184
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughLicensed local nodes now send remote Admin traffic as signed plaintext on channel 0. Existing normal-mode PKI behavior remains intact, with tests covering mutations, session requests, self-destination handling, and fallback cases. ChangesLicensed Admin transport
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
@coderabbitai review |
|
The labeler check requires a repository label that this contributor account cannot apply. A maintainer will need to add the appropriate feature/enhancement label; implementation CI is still running independently. |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/CommandSenderImpl.kt (1)
102-108: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the duplicated documentation phrase.
Line 106 ends with
Protocol-level requests, and Line 107 starts with the same phrase. Remove the duplicate so the KDoc clearly describes the PKI fallback behavior.Proposed fix
- * is appropriate, except licensed local nodes use signed plaintext on channel 0. Protocol-level requests - * Protocol-level requests must NOT use PKI. Relay nodes need to read or modify the inner payload. For example, + * is appropriate, except licensed local nodes use signed plaintext on channel 0. + * Protocol-level requests must NOT use PKI. Relay nodes need to read or modify the inner payload. For example,🤖 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 `@core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/CommandSenderImpl.kt` around lines 102 - 108, Remove the repeated “Protocol-level requests” phrase in the KDoc above the channel-resolution method, keeping a single occurrence while preserving the explanation that such requests must not use PKI and fall back to the node’s heard-on channel.
🧹 Nitpick comments (1)
core/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/CommandSenderImplTest.kt (1)
260-296: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the session passkey on the await path.
This test verifies acceptance, plaintext transport, and
want_response, but a regression insendAdminAwaitcould still omit or altersession_passkeywithout failing. Configure a non-empty passkey, decode the capturedAdminMessage, and assert it.Proposed test enhancement
configureNodes(nodes) + val passkey = "session-passkey".encodeUtf8() + every { sessionManager.getPasskey(DEST_NODE) } returns passkey var capturedPacket: MeshPacket? = null ... - assertTrue(requireNotNull(capturedPacket).decoded?.want_response == true) + val packet = requireNotNull(capturedPacket) + assertTrue(packet.decoded?.want_response == true) + val adminMessage = AdminMessage.ADAPTER.decode(requireNotNull(packet.decoded).payload) + assertEquals(passkey, adminMessage.session_passkey)🤖 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 `@core/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/CommandSenderImplTest.kt` around lines 260 - 296, Enhance sendAdminAwait_licensedSessionRequest_usesSignedPlaintextPacket by configuring a non-empty session passkey, decoding the captured AdminMessage from the packet, and asserting that its session_passkey matches the configured value. Preserve the existing acceptance, plaintext transport, and want_response assertions.
🤖 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.
Outside diff comments:
In
`@core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/CommandSenderImpl.kt`:
- Around line 102-108: Remove the repeated “Protocol-level requests” phrase in
the KDoc above the channel-resolution method, keeping a single occurrence while
preserving the explanation that such requests must not use PKI and fall back to
the node’s heard-on channel.
---
Nitpick comments:
In
`@core/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/CommandSenderImplTest.kt`:
- Around line 260-296: Enhance
sendAdminAwait_licensedSessionRequest_usesSignedPlaintextPacket by configuring a
non-empty session passkey, decoding the captured AdminMessage from the packet,
and asserting that its session_passkey matches the configured value. Preserve
the existing acceptance, plaintext transport, and want_response assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 219b98cc-6ec3-4297-83c1-21ad3bc38b18
📒 Files selected for processing (2)
core/data/src/commonMain/kotlin/org/meshtastic/core/data/manager/CommandSenderImpl.ktcore/data/src/commonTest/kotlin/org/meshtastic/core/data/manager/CommandSenderImplTest.kt
|
Addressed both CodeRabbit suggestions in 11c9c2a: removed the duplicated KDoc phrase and strengthened the awaited licensed Admin test to decode and verify the exact destination-scoped session passkey. Focused 27/27 tests and formatting pass; the full GitHub validation matrix is green except the maintainer-only labeler. @coderabbitai review |
|
(=^･ω･^=) ✅ Action performedReview finished.
|
2 participants
Closes #6183
Design: meshtastic/design#123
Firmware dependency: meshtastic/firmware#10972
Summary
This is a transport/UI alignment for the existing XEdDSA signing path. It does not add a new authentication protocol or weaken firmware authorization. Firmware remains responsible for verifying the packet signature, matching the exact verified key against
security.admin_key[], and enforcing mutation sessions.Validation
./gradlew :core:data:spotlessApply :core:data:spotlessCheck :core:data:detekt :core:data:jvmTest11c9c2a83245c8b3b77211616c7d13816c87a147;CommandSenderImplTesttests passed with JDK 21.Tests cover licensed getters, mutation passkey preservation, awaited session requests, normal PKI, self, unknown-local fallback, and destination licensed-state non-influence.
Draft / hardware status
This remains draft until firmware#10972 is validated with two licensed RF nodes. Required hardware checks include signed plaintext getter/session/mutation/reply traffic, rejection of unauthorized or stale-session mutations, and confirmation that normal-mode PKI behavior is unchanged.
Summary by CodeRabbit