UDP Unicast for Meshtasticd#10656
Conversation
github-actions
Bot
added
needs-review
There was a problem hiding this comment.
Pull request overview
This PR adds a Portduino/meshtasticd-specific unicast UDP connector intended to forward MeshPackets to a single configured peer (e.g., a meshswitch routing daemon), reusing concepts from the existing UDP multicast pathway.
Changes:
- Adds
General.UDPUnicastPeerPortduino config and YAML serialization/loading. - Introduces
UdpUnicastConnector(POSIX-socket-based) to send/receive MeshPackets over connected UDP. - Updates Router decrypt flow to optionally bypass channel-hash checks for UNICAST_UDP-origin packets.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/platform/portduino/PortduinoGlue.h | Adds udp_unicast_peer config field and YAML output. |
| src/platform/portduino/PortduinoGlue.cpp | Loads General.UDPUnicastPeer from YAML. |
| src/mesh/udp/UdpUnicastConnector.h | New unicast UDP connector implementation for meshtasticd. |
| src/mesh/Router.cpp | Mirrors outbound packets to the unicast connector; refactors AES decrypt attempt and adds UDP bypass path. |
| src/mesh/Channels.h | Makes setCrypto() callable from Router for UDP bypass decrypt attempts. |
| src/main.h | Exposes global udpUnicastConnector on Portduino builds. |
| src/main.cpp | Starts the unicast connector during setup when configured. |
| std::string host = peer; | ||
| uint16_t port = UDP_UNICAST_DEFAULT_PORT; | ||
| auto colon = peer.rfind(':'); | ||
| if (colon != std::string::npos) { | ||
| host = peer.substr(0, colon); | ||
| port = (uint16_t)atoi(peer.substr(colon + 1).c_str()); | ||
| } |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Jorropo
left a comment
Jorropo
left a comment
There was a problem hiding this comment.
-
please use dtls or some other authenticated encryption, don't be part of the problem of poorly secured iot devices, it's not too hard here.
Imo something like making the connection key:1.2.3.4:4403@327d6a5dafc9ac5e5459f93225ee3996a66c9a4749e64e61c6ab4ff51df4104dwith raw Ed25519 or ECC keys.
Ping me if you want me to claude it up and review it. -
It's kinda weird that you are implementing this only on meshtasticd.
An ESP32 is way good enough to also do this. I think it would make more sense if you implement the AsyncUDP unicast API in portduino, then write this code to use AsyncUDP, thus it would also work on ESP32s.
Firmware Size Report21 targets | no baseline available yet
Show 16 more target(s)
Updated for d8e488e |
How about AES-256 and AES-CCM based on x25519? trollface
Somehow I didn't think AsyncUDP had a unicast option. If we test this out and find it useful, moving it to that library makes sense, as a future PR. |
|
The point isn't to encrypt the data again, I want there to be some authentication that means someone random who finds an open meshtastic UDP port can't easily forge a couple packets to spam the mesh with garbage. |
|
The meshtasticd instance is the sender, and filters the UDP port for the connect address. While this isn't bulletproof security, it does mean that you can't just do a portscan for listening meshtasticd nodes. Some authentication would eventually be handy. There's no published client yet, this is still a WIP. It's just a pain to juggle a bunch of patches to try to test things out. |
📝 WalkthroughWalkthroughPortduino gains configurable UDP unicast transport. Startup initializes the connector, Router forwards outbound packets, received encrypted packets are enqueued, and channel decryption supports UDP-unicast hash bypasses. ChangesPortduino UDP unicast transport
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Router
participant UdpUnicastConnector
participant UDP_Peer
Router->>UdpUnicastConnector: Forward outbound mesh packet
UdpUnicastConnector->>UDP_Peer: Send protobuf datagram
UDP_Peer->>UdpUnicastConnector: Return encrypted datagram
UdpUnicastConnector->>Router: Enqueue received packet
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/platform/portduino/PortduinoGlue.h (1)
645-646: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
empty()for string emptiness checks.♻️ Proposed refactor
- if (udp_unicast_peer != "") + if (!udp_unicast_peer.empty()) out << YAML::Key << "UDPUnicastPeer" << YAML::Value << udp_unicast_peer;🤖 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/platform/portduino/PortduinoGlue.h` around lines 645 - 646, Update the UDP unicast peer check in the surrounding YAML serialization logic to use udp_unicast_peer.empty() instead of comparing it with an empty string, while preserving the existing conditional output behavior.
🤖 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/udp/UdpUnicastConnector.h`:
- Around line 45-48: Replace the unchecked atoi conversion in the peer parsing
logic with std::stoi inside a try-catch block, validating malformed port input
and handling conversion errors explicitly before assigning the result to port.
Preserve the existing host extraction and valid port parsing behavior in the
colon branch.
---
Nitpick comments:
In `@src/platform/portduino/PortduinoGlue.h`:
- Around line 645-646: Update the UDP unicast peer check in the surrounding YAML
serialization logic to use udp_unicast_peer.empty() instead of comparing it with
an empty string, while preserving the existing conditional output 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: 25e016fd-e420-4029-a009-7efc405cff7a
📒 Files selected for processing (7)
src/main.cppsrc/main.hsrc/mesh/Channels.hsrc/mesh/Router.cppsrc/mesh/udp/UdpUnicastConnector.hsrc/platform/portduino/PortduinoGlue.cppsrc/platform/portduino/PortduinoGlue.h
| if (colon != std::string::npos) { | ||
| host = peer.substr(0, colon); | ||
| port = (uint16_t)atoi(peer.substr(colon + 1).c_str()); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate string-to-integer conversion.
The atoi function performs no error detection and returns 0 on non-numeric input, which evaluates to an invalid port. Use std::stoi in a try-catch block to properly handle malformed input.
🛠️ Proposed fix
auto colon = peer.rfind(':');
if (colon != std::string::npos) {
host = peer.substr(0, colon);
- port = (uint16_t)atoi(peer.substr(colon + 1).c_str());
+ try {
+ port = (uint16_t)std::stoi(peer.substr(colon + 1));
+ } catch (...) {
+ LOG_ERROR("UDP unicast: bad port in peer address '%s'", peer.c_str());
+ return false;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (colon != std::string::npos) { | |
| host = peer.substr(0, colon); | |
| port = (uint16_t)atoi(peer.substr(colon + 1).c_str()); | |
| } | |
| auto colon = peer.rfind(':'); | |
| if (colon != std::string::npos) { | |
| host = peer.substr(0, colon); | |
| try { | |
| port = (uint16_t)std::stoi(peer.substr(colon + 1)); | |
| } catch (...) { | |
| LOG_ERROR("UDP unicast: bad port in peer address '%s'", peer.c_str()); | |
| return false; | |
| } | |
| } |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 46-46: Avoid atoi/atol/atoll/atof: they perform no error detection, returning 0 on non-numeric input and invoking undefined behavior on out-of-range values. This rule flags every use of these functions regardless of data provenance. Convert strings with strtol/strtoul/strtod and check errno (and the endptr) so malformed or overflowing input is rejected.
Context: atoi(peer.substr(colon + 1).c_str())
Note: [CWE-20] Improper Input Validation.
(atoi-no-error-detection-c)
🤖 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/udp/UdpUnicastConnector.h` around lines 45 - 48, Replace the
unchecked atoi conversion in the peer parsing logic with std::stoi inside a
try-catch block, validating malformed port input and handling conversion errors
explicitly before assigning the result to port. Preserve the existing host
extraction and valid port parsing behavior in the colon branch.
Source: Linters/SAST tools
Retreads our UDP multicast support for a unicast endpoint.
Summary by CodeRabbit
New Features
Bug Fixes