Draft: Restore XEdDSA material in keygen-excluded identity loads#10984
Draft: Restore XEdDSA material in keygen-excluded identity loads#10984RCGV1 wants to merge 3 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughChangesXEdDSA key restoration
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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-11. Updated for |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
The label check requires an allowed repository label. This contributor account cannot apply labels; a maintainer will need to add the appropriate bug label. |
![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
🤖 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/CryptoEngine.cpp`:
- Around line 289-295: Add an all-zero 32-byte private-key check in
CryptoEngine::setDHPrivateKey before calling XEdDSA::priv_curve_to_ed_keys; skip
XEdDSA key derivation for blank keys while preserving the existing key copy
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: 5f8a600f-c42d-4652-855e-a5a00f96fbc2
📒 Files selected for processing (2)
src/mesh/CryptoEngine.cpptest/test_crypto/test_main.cpp
| void CryptoEngine::setDHPrivateKey(uint8_t *_private_key) | ||
| { | ||
| memcpy(private_key, _private_key, 32); | ||
| #if !(MESHTASTIC_EXCLUDE_XEDDSA) | ||
| XEdDSA::priv_curve_to_ed_keys(private_key, xeddsa_private_key, xeddsa_public_key); | ||
| #endif | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all callers of setDHPrivateKey and check whether any can pass a blank/unvalidated key
rg -n -B5 -A2 '\bsetDHPrivateKey\s*\(' --type=cpp
# Inspect priv_curve_to_ed_keys implementation to confirm derivation behavior on zero input
ast-grep run --pattern 'void priv_curve_to_ed_keys($$$) { $$$ }' --lang cpp
rg -n 'priv_curve_to_ed_keys' -g '*.cpp' -g '*.h'Repository: meshtastic/firmware
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map relevant symbols in CryptoEngine.cpp
ast-grep outline src/mesh/CryptoEngine.cpp --view expanded
# Find callers/usages of setDHPrivateKey and related key-setting flows
rg -n -C 3 '\bsetDHPrivateKey\s*\(' src
rg -n -C 3 '\bregeneratePublicKey\s*\(' src
# Find blank-key checks around XEdDSA material
rg -n -C 3 'memfll\s*\(\s*xeddsa_private_key|priv_curve_to_ed_keys|xeddsa_sign|memfll\s*\(\s*private_key' src/mesh/CryptoEngine.cpp srcRepository: meshtastic/firmware
Length of output: 25164
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the implementation of the key-generation helpers and call paths
wc -l src/mesh/CryptoEngine.cpp
sed -n '1,220p' src/mesh/CryptoEngine.cpp
sed -n '220,360p' src/mesh/CryptoEngine.cpp
sed -n '360,520p' src/mesh/CryptoEngine.cppRepository: meshtastic/firmware
Length of output: 15250
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map CryptoEngine.cpp and inspect key-related paths
ast-grep outline src/mesh/CryptoEngine.cpp --view expanded
printf '\n--- callers ---\n'
rg -n -C 3 '\bsetDHPrivateKey\s*\(' src
printf '\n--- regeneratePublicKey ---\n'
rg -n -C 3 '\bregeneratePublicKey\s*\(' src/mesh/CryptoEngine.cpp
printf '\n--- xeddsa guard / derivation ---\n'
rg -n -C 2 'memfll\s*\(\s*xeddsa_private_key|priv_curve_to_ed_keys|xeddsa_sign|memfll\s*\(\s*private_key' src/mesh/CryptoEngine.cpp
printf '\n--- file slice ---\n'
wc -l src/mesh/CryptoEngine.cpp
sed -n '1,220p' src/mesh/CryptoEngine.cpp
sed -n '220,360p' src/mesh/CryptoEngine.cpp
sed -n '360,520p' src/mesh/CryptoEngine.cppRepository: meshtastic/firmware
Length of output: 20910
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the NodeDB boot/config flow around the setDHPrivateKey call sites
sed -n '3840,3945p' src/mesh/NodeDB.cpp
sed -n '460,510p' src/mesh/NodeDB.cpp
# Find places that populate or reset config.security.private_key
rg -n -C 3 'private_key\.size|private_key\.bytes|public_key\.size|public_key\.bytes|ensurePkiKeys|createNewIdentity' src/mesh/NodeDB.cpp src/mesh/CryptoEngine.cppRepository: meshtastic/firmware
Length of output: 25559
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for validation/sanitization of the stored security private key
rg -n -C 3 'memfll\s*\(\s*config\.security\.private_key|private_key\.size == 32|private_key\.size > 0|setDHPrivateKey\(' src/mesh/NodeDB.cpp src/mesh/CryptoEngine.cpp
# Inspect the exact context around the PKI-only branch that calls setDHPrivateKey directly
sed -n '480,500p' src/mesh/NodeDB.cpp
# Find all direct calls to setDHPrivateKey outside NodeDB::generateCryptoKeyPair
rg -n -C 2 '\bsetDHPrivateKey\s*\(' src | sed -n '1,120p'Repository: meshtastic/firmware
Length of output: 7511
Guard XEdDSA derivation against all-zero private keys setDHPrivateKey() is still reached from the direct boot path in src/mesh/NodeDB.cpp for any persisted 32-byte key pair, so a stored all-zero private key would now populate xeddsa_private_key and let xeddsa_sign() start succeeding on an invalid identity. Add the blank-key check 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/mesh/CryptoEngine.cpp` around lines 289 - 295, Add an all-zero 32-byte
private-key check in CryptoEngine::setDHPrivateKey before calling
XEdDSA::priv_curve_to_ed_keys; skip XEdDSA key derivation for blank keys while
preserving the existing key copy behavior.
Source: Coding guidelines
RCGV1
changed the title
|
Added a guard for blank persisted private keys so loading one clears XEdDSA signing material instead of retaining or deriving a signing identity. The native Linux simulator test now covers valid-key load followed by zero-key reload; |
1 participant
Refs #10982
Related: #10966
Design: meshtastic/design#122
Summary
Normal keygen-capable builds already call
regeneratePublicKey()before installing the private key. This draft is intentionally limited to deciding and covering behavior forMESHTASTIC_EXCLUDE_PKI_KEYGENbuilds that restore an existing PKI identity.Validation
MESHTASTIC_EXCLUDE_XEDDSA=1: passed, 40.7% RAM, 96.8% flash;521c62015d39ca57b8180cf7e78e62ae48a2184b;git diff --check: clean.The regression fails on the pre-fix baseline because a fresh engine has no reconstructed XEdDSA material after loading a persisted Curve private key, and passes with this change.
Draft / decision status
Keep this PR draft pending maintainer direction on whether XEdDSA signing is intended for the keygen-excluded restore path. No internal bench logs or captures are included.
🤝 Attestations