Allow key verification to work for unknown nodes. - #10669
Conversation
github-actions
Bot
added
needs-review
Firmware Size Report42 targets | vs
Show 37 more target(s)
Size budgets
Budgets live in Updated for b2a9d77 |
Resolved conflicts in the crypto/PKI path where both branches changed public-key resolution: - CryptoEngine.h: kept both branches' new protected members (key-verification pending-key fields + develop's XEDDSA fields). - Router.cpp perhapsDecode(): use develop's nodeDB->copyPublicKey() (hot-store/warm-tier lookup) as the primary sender-key source, then fall back to crypto->getPendingPublicKey() for an in-progress key-verification handshake. - Router.cpp send path: same combination for the destination key — copyPublicKey() primary, pending key fallback for KEY_VERIFICATION_APP follow-on packets. protobufs submodule advanced to develop's pointer (1ae3be3). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
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:
📝 WalkthroughWalkthroughAdds pending public-key storage, updates Router PKI key resolution, reworks the key-verification handshake bootstrap and commit flow, wires the UI into verification, and adds admin-fallback PKI tests. ChangesKey verification handshake feature
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/modules/KeyVerificationModule.cpp (2)
363-372: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCommitted key/verification flag is not persisted.
commitVerifiedRemoteNode()writes the peer's public key and the manual-verification bitfield into the in-memoryNodeInfoLite, but thetodo: initiate savemeans these are lost on reboot until some other path happens to flush NodeDB. Since manual verification is meant to be durable, this should trigger a NodeDB save.Want me to open an issue or wire up the appropriate NodeDB save call here?
🤖 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/modules/KeyVerificationModule.cpp` around lines 363 - 372, The committed peer key and manual-verification flag in commitVerifiedRemoteNode() are only updated in memory, so make this change durable by triggering a NodeDB save after updating node->public_key and node->bitfield. Use the existing NodeDB persistence path available to KeyVerificationModule (and any nearby NodeInfoModule/NodeDB helper methods) so the verified state survives reboot instead of relying on a later flush.
195-195: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSecurity number should come from a CSPRNG.
The security number is the entropy that gives this handshake its MitM resistance.
random()is not cryptographically secure and may be poorly seeded on embedded targets, making the number predictable to an on-path attacker. The existing// fixme, use better randomacknowledges this; consider sourcing it from the same secure RNG used for key material.Want me to open an issue to track switching to a CSPRNG here?
🤖 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/modules/KeyVerificationModule.cpp` at line 195, The security number generation in KeyVerificationModule::currentSecurityNumber uses random(), which is not cryptographically secure and can be predictable on embedded targets. Replace it with a CSPRNG-backed source, ideally the same secure RNG used for key material in this module, and keep the value in the same 1..999999 range if needed. Update the security-number assignment in KeyVerificationModule so the handshake entropy comes from a secure source instead of the current random() call.
🤖 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/graphics/draw/MenuHandler.cpp`:
- Line 2513: The accepted verification state is only changed in memory in
KeyVerificationModule::commitVerifiedRemoteNode(), so the manual-verification
flag and related pending key data are lost after restart. Update
commitVerifiedRemoteNode() to persist the verified state to storage after
setting the bitfield and clearing the pending public key, using the existing
save/persist path in MenuHandler so the change survives reboot.
In `@src/modules/KeyVerificationModule.cpp`:
- Line 369: The warning in KeyVerificationModule::manual verification logging
uses decimal output for currentRemoteNode, but 32-bit node IDs must be logged
consistently as zero-padded hex. Update the LOG_WARN call in
KeyVerificationModule.cpp to format currentRemoteNode as 0x%08x while keeping
currentSecurityNumber unchanged, matching the established node/packet ID logging
convention.
---
Nitpick comments:
In `@src/modules/KeyVerificationModule.cpp`:
- Around line 363-372: The committed peer key and manual-verification flag in
commitVerifiedRemoteNode() are only updated in memory, so make this change
durable by triggering a NodeDB save after updating node->public_key and
node->bitfield. Use the existing NodeDB persistence path available to
KeyVerificationModule (and any nearby NodeInfoModule/NodeDB helper methods) so
the verified state survives reboot instead of relying on a later flush.
- Line 195: The security number generation in
KeyVerificationModule::currentSecurityNumber uses random(), which is not
cryptographically secure and can be predictable on embedded targets. Replace it
with a CSPRNG-backed source, ideally the same secure RNG used for key material
in this module, and keep the value in the same 1..999999 range if needed. Update
the security-number assignment in KeyVerificationModule so the handshake entropy
comes from a secure source instead of the current random() call.
🪄 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: 2869bd9c-21ec-463b-8dbd-c341be88e5d8
📒 Files selected for processing (8)
src/graphics/Screen.cppsrc/graphics/Screen.hsrc/graphics/draw/MenuHandler.cppsrc/mesh/CryptoEngine.cppsrc/mesh/CryptoEngine.hsrc/mesh/Router.cppsrc/modules/KeyVerificationModule.cppsrc/modules/KeyVerificationModule.h
⚡ 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-08. Updated for |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/mesh/Router.cpp (1)
532-534: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShorten the expanded PKI comments.
These comments exceed the project’s one-to-two-line limit; the key source and admin persistence rationale can be kept concise.
Proposed cleanup
- // If we know the sender's key, make a single attempt with it. Otherwise fall back to trying each - // configured admin key, so an authorized admin can reach a node that has not yet learned their - // key. decryptCurve25519 is AES-CCM AEAD, so a wrong key fails authentication and we try the next. + // Try the sender key first, then configured admin keys. + // Wrong Curve25519 candidates fail AEAD authentication. @@ - // We decrypted this using a configured admin key, so we had no pubkey from the sender. - // Persist the key so future packets take the fast path and we can PKI-reply. The from - // nodenum is bound into the AEAD nonce, so this NodeNum->key binding is authenticated - // by the (trusted) admin that produced the packet. + // Persist the admin-authenticated NodeNum->key binding for future PKI replies.As per coding guidelines, “Keep code comments minimal: use at most one or two lines, only when the reason is not obvious.”
Also applies to: 567-570
🤖 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/Router.cpp` around lines 532 - 534, Shorten the expanded PKI comments in Router::decryptCurve25519-related logic so they fit the one-to-two-line comment guideline. Keep only the essential point: prefer the sender’s known key first, then fall back to configured admin keys so authorized admins can still reach nodes that haven’t learned their key yet; update both comment blocks referenced in Router.cpp to be concise and remove extra explanation.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 527-564: The PKI decrypt path in Router::processPacket is using an
unguarded remotePublic and also treats pending keys as sufficient for marking
the packet PKI-encrypted. Update the decrypt branch to only enter Curve25519
work when haveRemoteKey is true and the packet matches the channel 0 /
addressed-to-us / non-broadcast / size requirements, and keep
getPendingPublicKey results scoped to verification-only until the key is
actually committed. Make the acceptance of decrypted data depend on a verified
public key source before setting p->pki_encrypted, copying p->public_key, or
using the decoded payload.
---
Nitpick comments:
In `@src/mesh/Router.cpp`:
- Around line 532-534: Shorten the expanded PKI comments in
Router::decryptCurve25519-related logic so they fit the one-to-two-line comment
guideline. Keep only the essential point: prefer the sender’s known key first,
then fall back to configured admin keys so authorized admins can still reach
nodes that haven’t learned their key yet; update both comment blocks referenced
in Router.cpp to be concise and remove extra explanation.
🪄 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: d85a0d9e-3c1b-49fc-b7da-c28d92aedf06
📒 Files selected for processing (1)
src/mesh/Router.cpp
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/test_pki_admin_fallback/test_main.cpp (1)
1-11: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTrim multi-line comment blocks per coding guidelines.
Several comment blocks in this file span 3+ lines (e.g. the file header at Lines 1-11, and section headers at Lines 32-35, 72-74, 85-87, 137-139, 173-175, 211-212). The repo guideline restricts comments to 1-2 lines unless the reason is non-obvious.
As per coding guidelines, "Keep code comments minimal: one or two lines maximum, only when the reason is not obvious, and do not restate the next line."
Also applies to: 32-35, 72-74, 85-87, 137-139, 173-175, 211-212
🤖 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_pki_admin_fallback/test_main.cpp` around lines 1 - 11, Several comment blocks in this test file are too verbose and exceed the 1–2 line guideline. Trim the file header and the section headers around the test cases so each comment is brief, keeps only the non-obvious rationale, and does not restate what the following code already shows. Update the multi-line comments near the admin-key fallback tests in test_main.cpp, especially the header and the repeated section markers, to shorter single- or two-line notes.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.
Nitpick comments:
In `@test/test_pki_admin_fallback/test_main.cpp`:
- Around line 1-11: Several comment blocks in this test file are too verbose and
exceed the 1–2 line guideline. Trim the file header and the section headers
around the test cases so each comment is brief, keeps only the non-obvious
rationale, and does not restate what the following code already shows. Update
the multi-line comments near the admin-key fallback tests in test_main.cpp,
especially the header and the repeated section markers, to shorter single- or
two-line notes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6b6c3a08-07bf-4b3b-a9ef-6a698c400713
📒 Files selected for processing (2)
test/native-suite-counttest/test_pki_admin_fallback/test_main.cpp
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because no GitHub Actions runner was available. Make sure your repository has a runner available to run Copilot's review, or add a copilot-setup-steps.yml file specifying one with the runs-on attribute. See the docs for more details.
Enables key verification (and PKI encode/decode) to work when a peer’s public key is not yet in NodeDB by bootstrapping keys via the key-verification handshake and by adding admin-key fallback on PKI decryption.
Changes:
- Key-verification handshake now supports “unknown node” bootstrapping via public-key carriage + pending-key handling.
- Router PKI decode/encode now consults a pending key and adds admin-key fallback decryption for unknown senders.
- Adds a native test suite covering admin-key fallback behavior, and extends number picker to support hex mode.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test_pki_admin_fallback/test_main.cpp | New crypto-backed tests for admin-key fallback PKI decryption + persistence behavior. |
| test/native-suite-count | Increments native test suite count for newly added suite. |
| src/modules/KeyVerificationModule.h | Documents handshake + introduces commitVerifiedRemoteNode() and updateState(resetTimer). |
| src/modules/KeyVerificationModule.cpp | Implements bootstrap key exchange using pending keys; commits pending key on user acceptance. |
| src/mesh/Router.cpp | Uses pending keys for PKI encode/decode and attempts admin-key fallback decryption when sender key unknown. |
| src/mesh/CryptoEngine.h | Adds pending public key API/state for in-progress key verification. |
| src/mesh/CryptoEngine.cpp | Implements pending public key storage/retrieval. |
| src/graphics/draw/MenuHandler.cpp | Updates number test to use hex nodenum entry and trigger key verification. |
| src/graphics/Screen.h | Extends showNumberPicker signature to accept base-16 mode. |
| src/graphics/Screen.cpp | Implements base-16 mode selection (hex picker vs decimal picker). |
- Persist the committed key + manually-verified flag in commitVerifiedRemoteNode via saveToDisk(SEGMENT_NODEDATABASE), replacing the "todo: initiate save" - Guard the CryptoEngine pending-key slot with a dedicated internal lock; the Router reads it while already holding the non-recursive cryptLock, so the accessors cannot reuse that lock - Draw the security number from the hardware RNG (CryptRNG fallback) under cryptLock instead of random(); on nRF52 the entropy fill toggles the same CC310 the BLE task's packet crypto uses - Return true after fully handling the hash2 response (restores develop behavior; consistent with the hash1 branch) - Take uint32_t in the number-picker callbacks so 8-digit hex nodenums can't truncate through int - Trim over-long comments flagged by review
^ On the receiving end. Looks like it gets a bit further with this PR than without, but losing the node in nodedb causes the session to be lost too? |
Did this happen right after a reboot or a nodedb reset where the session key would be gone perhaps? |
On STM32WL, where it did work in the same session (no reboot) then after some time stopped working (or rather works intermittently) because nodes keep getting popped out of NodeDB (due to the 10-node limit) |
|
Dug into this — the So Worth stressing the PR's part worked here: the packet decrypted via the authorized admin key and got all the way to the session check, which is impossible without a successful decrypt. The session gate is a separate, pre-existing mechanism the PR doesn't touch. The reason it doesn't self-heal is that the @ndoo — in the receiving node's log from boot, do you see any |
It'll be a while before I can test this, unfortunately. But forcing the 10-node NodeDB limit might help you to reproduce it quickly. |
|
Based on what I'm finding locally this may be something specific to STM32 because I can't repro on NRF |
I’ll only be able to try this further tomorrow night, but don't let STM32 hold back this PR since it’s not a stable platform. |
here is what my phantom brain thinks: |
Would love to see the full log with this error. |
I'll raise an issue when I manage to capture it tomorrow. Seems related to STM32WL not having the warm store (see the CC analysis I linked) |
The canonical suite count drifted from the actual test/test_*/ directory count upstream on develop: meshtastic#10669 added two suites but bumped the count by one, and meshtastic#11037 added test_xmodem without bumping it at all - leaving native-suite-count at 35 against 37 real suites, which run-tests.sh reports as AMBER. Correct it to 37. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The canonical suite count drifted from the actual test/test_*/ directory count upstream on develop: meshtastic#10669 added two suites but bumped the count by one, and meshtastic#11037 added test_xmodem without bumping it at all - leaving native-suite-count at 35 against 37 real suites, which run-tests.sh reports as AMBER. Correct it to 37.
native-suite-count drifted from the actual test/test_*/ directory count: meshtastic#10669 added two suites (test_admin_session_repro, test_pki_admin_fallback) but bumped the count by only one, and meshtastic#11037 added test_xmodem without bumping it at all. The file reads 35 against 37 real suites, which bin/run-tests.sh reports as AMBER on every full run. Correct it to 37.
native-suite-count drifted from the actual test/test_*/ directory count: #10669 added two suites (test_admin_session_repro, test_pki_admin_fallback) but bumped the count by only one, and #11037 added test_xmodem without bumping it at all. The file reads 35 against 37 real suites, which bin/run-tests.sh reports as AMBER on every full run. Correct it to 37.
* Allow key verification to work for unknown nodes. * trunk * More reliable admin key decryption * Add admin key fallback tests * Actually check haveRemoteKey * Logging cleanup * Address review feedback - Persist the committed key + manually-verified flag in commitVerifiedRemoteNode via saveToDisk(SEGMENT_NODEDATABASE), replacing the "todo: initiate save" - Guard the CryptoEngine pending-key slot with a dedicated internal lock; the Router reads it while already holding the non-recursive cryptLock, so the accessors cannot reuse that lock - Draw the security number from the hardware RNG (CryptRNG fallback) under cryptLock instead of random(); on nRF52 the entropy fill toggles the same CC310 the BLE task's packet crypto uses - Return true after fully handling the hash2 response (restores develop behavior; consistent with the hash1 branch) - Take uint32_t in the number-picker callbacks so 8-digit hex nodenums can't truncate through int - Trim over-long comments flagged by review * feat(tests): add deterministic tests for admin session-key behavior --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Ben Meadors <benmmeadors@gmail.com>
Still slightly WIP, but tested working.
Summary by CodeRabbit