Budget the admin-key PKI decrypt fallback#11100
Merged
Merged
Conversation
Each run costs up to three X25519 operations and runs before the AEAD tag is checked, so unauthenticated unicasts can pin the CPU. p->from is attacker-controlled, so the budget is global rather than per-node. Successful runs refund their token, and a successful admin decrypt already persists the sender key so later packets take the single-operation fast path. Authorized administration is unaffected.
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughPKI admin-key fallback decryption now uses a global token-bucket limiter with refill and success refunds. Tests cover budget exhaustion, refill, successful decoding, sender-key persistence, and test registration. ChangesPKI admin-key fallback rate limiting
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant perhapsDecode
participant FallbackBudget
participant AdminKeyDecryptor
perhapsDecode->>FallbackBudget: Request fallback token
FallbackBudget-->>perhapsDecode: Allow or deny attempt
perhapsDecode->>AdminKeyDecryptor: Try configured admin keys
AdminKeyDecryptor-->>perhapsDecode: Return decryption result
perhapsDecode->>FallbackBudget: Refund token after successful fallback
🚥 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/mesh/Router.cpp (1)
555-558: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTrim duplicated multi-line rationale comments to one line each. Both sites restate the same budget-rationale explanation across 3-4 lines, exceeding the repo's comment-length limit.
src/mesh/Router.cpp#L555-L558: condense to a single line (e.g., note only that the budget is global becausep->fromis attacker-controlled and refunds on success).test/test_pki_admin_fallback/test_main.cpp#L205-L207: condense to a single line; the full rationale is already documented at the implementation site in Router.cpp.As per coding guidelines, "Keep code comments minimal—one or two lines maximum—and comment only when the reason is not obvious; do not restate straightforward code or add multi-paragraph explanatory blocks."
🤖 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 555 - 558, Condense the duplicated multi-line rationale comments to one line at src/mesh/Router.cpp lines 555-558 and test/test_pki_admin_fallback/test_main.cpp lines 205-207. In Router.cpp, retain only that the budget is global because p->from is attacker-controlled and successful runs refund the token; in the test file, use a brief reference to the implementation rationale without repeating it.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 `@src/mesh/Router.cpp`:
- Around line 555-558: Condense the duplicated multi-line rationale comments to
one line at src/mesh/Router.cpp lines 555-558 and
test/test_pki_admin_fallback/test_main.cpp lines 205-207. In Router.cpp, retain
only that the budget is global because p->from is attacker-controlled and
successful runs refund the token; in the test file, use a brief reference to the
implementation rationale without repeating it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7f8b1b0d-dd07-4ab9-80d6-280c9303eb05
📒 Files selected for processing (2)
src/mesh/Router.cpptest/test_pki_admin_fallback/test_main.cpp
Member
Author
|
Condensed both comment sites. The rationale now lives in one line at the implementation and the test refers to it rather than repeating it. Tests still pass, 5 cases. |
Contributor
⚡ 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 (30)
Build artifacts expire on 2026-08-19. Updated for |
# Conflicts: # test/test_pki_admin_fallback/test_main.cpp
thebentern
added a commit
to RCGV1/firmware-Fork
that referenced
this pull request
Jul 21, 2026
Resolve conflicts against the NodeDB signer/key primitives (meshtastic#11050) and the admin-key PKI decrypt budget (meshtastic#11100). - NodeDB: drop this branch's hasSeenXeddsaSigner in favour of develop's isKnownXeddsaSigner. They answer the same question, but develop's reads the dedicated warm signer bit (warmSignerOf) rather than the WarmProtected category, and TrafficManagementModule already depends on it. Keep develop's copyPublicKey/copyPublicKeyAuthoritative, isVerifiedSignerForKey and commitRemoteKey/KeyCommitTrust. - checkXeddsaReceivePolicy: keep this branch's Strict/Balanced/Compatible policy, which is a superset of develop's balanced-only downgrade gate, and call isKnownXeddsaSigner from it. develop's !pki_encrypted term is dropped because the policy returns early for PKI packets before that check. - perhapsDecode: keep develop's key resolution (NodeDB then pending-key, only for real PKI candidates) plus its admin-key token bucket, and re-apply this branch's pkiAttempted flag feeding the DECODE_OPAQUE verdict. Keep both passesRoutingAuthGate and adminKeyFallbackAllowed/Refund. - test_A17: model eviction the way NodeDB actually does it, passing the warm signer bit as well as the XeddsaSigner category, since isKnownXeddsaSigner reads the former. Native suite: 38 suites, 743/743 cases, no sanitizer findings.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Router::perhapsDecodetries each configured admin key when the sender's own key does not decrypt a PKI unicast. Each attempt is an X25519 scalar multiplication, and the AEAD tag that rejects a forged packet is only checked afterwards, so the expensive work happens before any authentication. An attacker needs a frame withchannel=0,toset to the victim, 13+ bytes of random ciphertext and a freshid; every such packet costs the full three operations because the loop only stops on success. Nothing on the path rate limited this.p->fromis unauthenticated header data, so a per-node throttle is evaded by varying it. The budget is therefore global: a token bucket of 8, refilling one per 250ms, spent only when the fallback actually runs.Successful runs refund their token. A successful admin decrypt already persists the admin key into the sender's NodeDB entry, so subsequent packets from that admin take the existing single-operation fast path and never re-enter the loop. Authorized administration is low volume and unaffected.
Trying admin keys for senders that are not yet in NodeDB is deliberate, so that remote administration works against a node that has not learned the admin's key. That behavior is unchanged.
Adds a regression test covering both directions: the fallback stops running once the burst is drained, and it resumes after the bucket refills.
Summary by CodeRabbit