Make XEdDSASigning work with firmware generated signatures#131
Make XEdDSASigning work with firmware generated signatures#131lkosson wants to merge 17 commits into
Conversation
📝 WalkthroughWalkthroughXEdDSA signing and verification now use raw message bytes, with ChangesXEdDSA signing API simplification
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Meshtastic/Crypto/XEdDSASigning.cs (1)
39-47: 🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy liftMeshtastic/Crypto/XEdDSASigning.cs:39-47 — Derive the signing key with the same XEdDSA/Ed25519 scalar as the mapped public key
GenerateEdDSAKeysFromX25519returns the raw X25519 bytes asedPrivateKey, butSignpasses them toEd25519PrivateKeyParameters, which treats them as an Ed25519 seed and derives a different signing keypair. That keypair won’t match the birationally mappededPublicKey, so signatures from this path won’t verify with the key returned here.Signalso ignores itsedPublicKeyparameter.🤖 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 `@Meshtastic/Crypto/XEdDSASigning.cs` around lines 39 - 47, Generate the EdDSA signing key from the X25519 secret using the same XEdDSA/Ed25519 scalar as the mapped public key, instead of returning the raw X25519 bytes from GenerateEdDSAKeysFromX25519. Update Sign and any related key-conversion logic in XEdDSASigning so the private key passed into Ed25519PrivateKeyParameters corresponds to the returned edPublicKey, and either use the edPublicKey parameter for validation or remove it if it is not needed.Meshtastic.Test/Crypto/XEdDSASigningTests.cs (1)
70-82: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftUse a consistent key pair here
GenerateEdDSAKeysFromX25519returns the raw X25519 private key as the signing seed, butSigntreats it as an Ed25519 seed whileVerifyuses the Montgomery→Edwards-mapped public key. Those keys don’t form the same Ed25519 pair, so this assertion won’t hold. Fix the derivation inMeshtastic/Crypto/XEdDSASigning.csor switch this path to a real Ed25519 keypair.🤖 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 `@Meshtastic.Test/Crypto/XEdDSASigningTests.cs` around lines 70 - 82, The Verify_Should_ReturnTrue_ForValidSignature test is using mismatched key material: GenerateEdDSAKeysFromX25519 is returning a raw X25519 private key as the signing seed while Verify expects the corresponding Ed25519 pair. Update XEdDSASigning.GenerateEdDSAKeysFromX25519 and/or XEdDSASigning.Sign so the generated private/public values form a true Ed25519 keypair, or change this test path to use a real Ed25519 keypair consistently with XEdDSASigning.Verify.
🧹 Nitpick comments (1)
Meshtastic/Data/MessageFactories/NodeInfoMessageFactory.cs (1)
196-204: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
signedSha256Packet/signedSha512Packetare now identical and unused.After dropping
useShortHash, both calls are the same (signPacket: true) and neither variable is read afterward — the reported sizes are derived fromunsignedSize + signatureSizeOverhead(Lines 208-214). These twoCreateNodeInfoMessagecalls are now redundant dead work, and the SHA-256 vs SHA-512 naming is misleading.♻️ Proposed cleanup
- // Create signed packets - these will use mock signatures for testing - var signedSha256Packet = CreateNodeInfoMessage( - deviceStateContainer: testDeviceState, - user: user, - signPacket: true); - - var signedSha512Packet = CreateNodeInfoMessage( - deviceStateContainer: testDeviceState, - user: user, - signPacket: true); -🤖 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 `@Meshtastic/Data/MessageFactories/NodeInfoMessageFactory.cs` around lines 196 - 204, The two CreateNodeInfoMessage calls assigned to signedSha256Packet and signedSha512Packet are redundant and misleading because they now produce the same signed packet and are never read. Remove both unused variables and any related dead work in NodeInfoMessageFactory, and keep the size assertions based only on the unsigned message plus signature overhead where the actual verification is needed.
🤖 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 `@Meshtastic/Crypto/XEdDSASigning.cs`:
- Around line 104-114: Guard edPrivateKey in XEdDSASigning.Sign before accessing
Length so a null input throws ArgumentNullException like Verify, and either
remove the unused edPublicKey parameter or document its intentional presence if
it must remain. Update the XML docs on Sign, Verify, and VerifyWithX25519Key to
remove the stale useShortHash reference and make the parameter lists match the
actual method signatures, especially the unused edPublicKey on Sign.
---
Outside diff comments:
In `@Meshtastic.Test/Crypto/XEdDSASigningTests.cs`:
- Around line 70-82: The Verify_Should_ReturnTrue_ForValidSignature test is
using mismatched key material: GenerateEdDSAKeysFromX25519 is returning a raw
X25519 private key as the signing seed while Verify expects the corresponding
Ed25519 pair. Update XEdDSASigning.GenerateEdDSAKeysFromX25519 and/or
XEdDSASigning.Sign so the generated private/public values form a true Ed25519
keypair, or change this test path to use a real Ed25519 keypair consistently
with XEdDSASigning.Verify.
In `@Meshtastic/Crypto/XEdDSASigning.cs`:
- Around line 39-47: Generate the EdDSA signing key from the X25519 secret using
the same XEdDSA/Ed25519 scalar as the mapped public key, instead of returning
the raw X25519 bytes from GenerateEdDSAKeysFromX25519. Update Sign and any
related key-conversion logic in XEdDSASigning so the private key passed into
Ed25519PrivateKeyParameters corresponds to the returned edPublicKey, and either
use the edPublicKey parameter for validation or remove it if it is not needed.
---
Nitpick comments:
In `@Meshtastic/Data/MessageFactories/NodeInfoMessageFactory.cs`:
- Around line 196-204: The two CreateNodeInfoMessage calls assigned to
signedSha256Packet and signedSha512Packet are redundant and misleading because
they now produce the same signed packet and are never read. Remove both unused
variables and any related dead work in NodeInfoMessageFactory, and keep the size
assertions based only on the unsigned message plus signature overhead where the
actual verification is needed.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6093c37f-7751-4d87-8fe5-1c1b984c3a96
📒 Files selected for processing (3)
Meshtastic.Test/Crypto/XEdDSASigningTests.csMeshtastic/Crypto/XEdDSASigning.csMeshtastic/Data/MessageFactories/NodeInfoMessageFactory.cs
| public static byte[] Sign(byte[] message, byte[] edPrivateKey, byte[] edPublicKey) | ||
| { | ||
| if (message == null) throw new ArgumentNullException(nameof(message)); | ||
| if (edPrivateKey.Length != 32) throw new ArgumentException("Ed25519 private key must be 32 bytes", nameof(edPrivateKey)); | ||
|
|
||
| // Hash the message using the selected algorithm | ||
| var messageHash = useShortHash ? SHA256.HashData(message) : SHA512.HashData(message); | ||
|
|
||
| // Create Ed25519 signer | ||
| var signer = new Ed25519Signer(); | ||
| var privateKeyParams = new Ed25519PrivateKeyParameters(edPrivateKey, 0); | ||
|
|
||
| signer.Init(true, privateKeyParams); | ||
| signer.BlockUpdate(messageHash, 0, messageHash.Length); | ||
| signer.BlockUpdate(message, 0, message.Length); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Guard edPrivateKey against null and drop/document the unused edPublicKey parameter.
edPrivateKey.Length on line 107 dereferences before any null check, so a null argument throws NullReferenceException instead of ArgumentNullException — inconsistent with Verify. Also, edPublicKey is never used in Sign (standard Ed25519 derives the public key from the private key), and the XML doc still documents a removed useShortHash param (line 102) while omitting edPublicKey, which yields a CS1572 warning. The same stale useShortHash doc exists on Verify (line 125) and VerifyWithX25519Key (line 156).
🛡️ Proposed guard + doc fix
- /// <param name="edPrivateKey">Ed25519 private key</param>
- /// <param name="useShortHash">Use SHA-256 instead of SHA-512 for hashing before signing</param>
+ /// <param name="edPrivateKey">Ed25519 private key</param>
/// <returns>64-byte signature</returns>
public static byte[] Sign(byte[] message, byte[] edPrivateKey, byte[] edPublicKey)
{
if (message == null) throw new ArgumentNullException(nameof(message));
+ if (edPrivateKey == null) throw new ArgumentNullException(nameof(edPrivateKey));
if (edPrivateKey.Length != 32) throw new ArgumentException("Ed25519 private key must be 32 bytes", nameof(edPrivateKey));🤖 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 `@Meshtastic/Crypto/XEdDSASigning.cs` around lines 104 - 114, Guard
edPrivateKey in XEdDSASigning.Sign before accessing Length so a null input
throws ArgumentNullException like Verify, and either remove the unused
edPublicKey parameter or document its intentional presence if it must remain.
Update the XML docs on Sign, Verify, and VerifyWithX25519Key to remove the stale
useShortHash reference and make the parameter lists match the actual method
signatures, especially the unused edPublicKey on Sign.
… one derived from X25519 public key
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Meshtastic/Crypto/XEdDSASigning.cs (1)
166-179: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Signuses the wrong Ed25519 key form
Ed25519PrivateKeyParameterstreats the 32 bytes as a seed, but this path feeds it an X25519-derived scalar. That makesSignderive different key material than theedPublicKeyused byVerifyPacketSignature, so signatures from this method won’t match the public key path.edPublicKeyis also unused; either pass real Ed25519 seed/public-key material or replace this with an XEdDSA implementation that signs from the intended scalar.🤖 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 `@Meshtastic/Crypto/XEdDSASigning.cs` around lines 166 - 179, The Sign method is using the wrong key material by treating the 32-byte input as an Ed25519 seed, while the method is actually being fed an X25519-derived scalar and never uses edPublicKey. Update XEdDSASigning.Sign to either accept real Ed25519 seed/public-key material that matches VerifyPacketSignature, or replace the signing flow with a true XEdDSA implementation that derives signatures from the intended scalar. Keep the fix localized around Sign, Ed25519PrivateKeyParameters, and the unused edPublicKey parameter so the signing and verification paths use the same key form.
🤖 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 `@Meshtastic/Crypto/XEdDSASigning.cs`:
- Line 141: The packet-signature access in XEdDSASigning is using a field that
does not exist on the generated Data type, so the code fails to compile. Update
XEdDSASigning to use the actual generated protobuf member from
meshPacket.Decoded, or regenerate the protobuf types in Mesh.cs so the expected
signature field and its presence check exist. Verify the fix against the symbols
XEdDSASigning and meshPacket.Decoded before keeping the ToByteArray() call.
- Around line 55-71: The negation fallback in GenerateEdDSAKeysFromX25519 is
unreachable and incorrect, so remove it or replace it with the proper recovery
path. Keep the fast path based on the sign bit check from
ConvertX25519PublicKeyToEd25519, and if a fallback is still required, recompute
the X25519 public key from the negated scalar and then convert that result back
to Ed25519 instead of passing negatedPrivateKey directly into
ConvertX25519PublicKeyToEd25519. Ensure any byte conversion preserves unsigned
little-endian padding for the scalar and returned key material.
---
Outside diff comments:
In `@Meshtastic/Crypto/XEdDSASigning.cs`:
- Around line 166-179: The Sign method is using the wrong key material by
treating the 32-byte input as an Ed25519 seed, while the method is actually
being fed an X25519-derived scalar and never uses edPublicKey. Update
XEdDSASigning.Sign to either accept real Ed25519 seed/public-key material that
matches VerifyPacketSignature, or replace the signing flow with a true XEdDSA
implementation that derives signatures from the intended scalar. Keep the fix
localized around Sign, Ed25519PrivateKeyParameters, and the unused edPublicKey
parameter so the signing and verification paths use the same key form.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c8989411-0b62-4a36-b401-06de5fb077cc
📒 Files selected for processing (2)
Meshtastic.Test/Crypto/XEdDSASigningTests.csMeshtastic/Crypto/XEdDSASigning.cs
| public static bool VerifyPacketSignature(byte[] senderPublicKey, MeshPacket meshPacket) | ||
| { | ||
| var message = BuildSigningBuffer(meshPacket); | ||
| var signature = meshPacket.Decoded.XeddsaSignature.ToByteArray(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the MeshPacket/Data protobuf definitions and any signature field.
fd -e cs -e proto | xargs rg -n -i 'xeddsa|xeddsaSignature|message Data' 2>/dev/nullRepository: meshtastic/c-sharp
Length of output: 5289
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the crypto implementation and the packet/data definitions around the referenced lines.
sed -n '120,170p' Meshtastic/Crypto/XEdDSASigning.cs
printf '\n---\n'
fd -a -e cs -e proto -t f | grep -E 'MeshPacket|Data|protobuf|proto'
printf '\n---\n'
rg -n -i 'message\s+Data|XeddsaSignature|HasXeddsaSignature|xeddsaSignature' Meshtastic -g '!**/bin/**' -g '!**/obj/**'Repository: meshtastic/c-sharp
Length of output: 4721
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the generated MeshPacket/Data definitions and inspect their members.
fd -a -e cs Meshtastic | grep -E 'MeshPacket|Data|Generated|Proto|protobuf' || true
printf '\n---\n'
rg -n --glob '*.cs' 'partial class (MeshPacket|Data)|class (MeshPacket|Data)|XeddsaSignature|HasXeddsaSignature|xeddsaSignature' MeshtasticRepository: meshtastic/c-sharp
Length of output: 861
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the generated Data type for signature-related members.
sed -n '3889,4105p' Meshtastic/Generated/Mesh.cs
printf '\n---\n'
rg -n 'Xeddsa|Signature|Has.*Signature' Meshtastic/Generated/Mesh.csRepository: meshtastic/c-sharp
Length of output: 8040
Fix the missing XeddsaSignature field meshPacket.Decoded is the generated Data type, and Meshtastic/Generated/Mesh.cs doesn’t define XeddsaSignature or HasXeddsaSignature, so both packet-signature accesses fail to compile. Update the generated protobufs or switch to the actual field name.
🧰 Tools
🪛 GitHub Actions: CI / 0_test.txt
[error] 141-141: dotnet test failed. Compilation error CS1061: 'Data' does not contain a definition for 'XeddsaSignature' and no accessible extension method 'XeddsaSignature' accepting a first argument of type 'Data' could be found (are you missing a using directive or an assembly reference?).
🪛 GitHub Actions: CI / 1_build.txt
[error] 141-141: dotnet build failed with CS1061: 'Data' does not contain a definition for 'XeddsaSignature' and no accessible extension method 'XeddsaSignature' accepting a first argument of type 'Data' could be found (missing using directive or assembly reference?).
🪛 GitHub Actions: CI / build
[error] 141-141: dotnet build failed with CS1061: 'Data' does not contain a definition for 'XeddsaSignature' and no accessible extension method 'XeddsaSignature' accepting a first argument of type 'Data' could be found (are you missing a using directive or an assembly reference?).
🪛 GitHub Check: build
[failure] 141-141:
'Data' does not contain a definition for 'XeddsaSignature' and no accessible extension method 'XeddsaSignature' accepting a first argument of type 'Data' could be found (are you missing a using directive or an assembly reference?)
[failure] 141-141:
'Data' does not contain a definition for 'XeddsaSignature' and no accessible extension method 'XeddsaSignature' accepting a first argument of type 'Data' could be found (are you missing a using directive or an assembly reference?)
🪛 GitHub Check: test
[failure] 141-141:
'Data' does not contain a definition for 'XeddsaSignature' and no accessible extension method 'XeddsaSignature' accepting a first argument of type 'Data' could be found (are you missing a using directive or an assembly reference?)
[failure] 141-141:
'Data' does not contain a definition for 'XeddsaSignature' and no accessible extension method 'XeddsaSignature' accepting a first argument of type 'Data' could be found (are you missing a using directive or an assembly reference?)
🤖 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 `@Meshtastic/Crypto/XEdDSASigning.cs` at line 141, The packet-signature access
in XEdDSASigning is using a field that does not exist on the generated Data
type, so the code fails to compile. Update XEdDSASigning to use the actual
generated protobuf member from meshPacket.Decoded, or regenerate the protobuf
types in Mesh.cs so the expected signature field and its presence check exist.
Verify the fix against the symbols XEdDSASigning and meshPacket.Decoded before
keeping the ToByteArray() call.
Sources: Linters/SAST tools, Pipeline failures
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Meshtastic/Crypto/XEdDSASigning.cs (3)
62-65: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd null guards before length checks.
Both public helpers dereference
Lengthbefore validating null, so null inputs throwNullReferenceExceptioninstead of argument exceptions.🛡️ Proposed fix
public static (byte[] edPrivateKey, byte[] edPublicKey) GenerateEdDSAKeysFromX25519(byte[] x25519PrivateKey) { + if (x25519PrivateKey == null) + throw new ArgumentNullException(nameof(x25519PrivateKey)); if (x25519PrivateKey.Length != 32) throw new ArgumentException("X25519 private key must be 32 bytes", nameof(x25519PrivateKey));public static byte[] ConvertX25519PublicKeyToEd25519(byte[] x25519PublicKey, bool forcePositive = false) { + if (x25519PublicKey == null) + throw new ArgumentNullException(nameof(x25519PublicKey)); if (x25519PublicKey.Length != 32) throw new ArgumentException("X25519 public key must be 32 bytes", nameof(x25519PublicKey));Also applies to: 105-108
🤖 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 `@Meshtastic/Crypto/XEdDSASigning.cs` around lines 62 - 65, Add null checks before accessing Length in the public key-conversion helpers so null inputs raise argument exceptions instead of NullReferenceException. Update GenerateEdDSAKeysFromX25519 and the other helper referenced in this class to validate the byte[] parameter for null first, then perform the 32-byte length check and keep the existing ArgumentException/nameof parameter reporting.
148-155: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReturn
falsefor unsigned or malformed packets instead of throwing.
VerifyPacketSignaturecurrently dereferencesmeshPacket.Decodedand then passes any signature length intoVerify; unsigned packets or missing decoded payloads can crash verification instead of producingfalse.🛡️ Proposed guard
public static bool VerifyPacketSignature(byte[] senderPublicKey, MeshPacket meshPacket) { + if (senderPublicKey == null || senderPublicKey.Length != 32 || + meshPacket?.Decoded?.Payload == null || + meshPacket.Decoded.XeddsaSignature == null || + meshPacket.Decoded.XeddsaSignature.Length != 64) + return false; + var message = BuildSigningBuffer(meshPacket); var signature = meshPacket.Decoded.XeddsaSignature.ToByteArray();Also applies to: 164-169
🤖 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 `@Meshtastic/Crypto/XEdDSASigning.cs` around lines 148 - 155, VerifyPacketSignature should fail closed for unsigned or malformed packets instead of throwing. Add early guards before dereferencing meshPacket.Decoded or using the signature: if the packet, decoded payload, or signature is missing/invalid, return false. Update the flow around BuildSigningBuffer and the Verify call so they are only reached when the packet has a valid decoded payload and a signature of acceptable length, and make the same safety check in the related signing/verification path referenced by the same methods.
192-204: 🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy liftUse an XEdDSA-compatible signer here
GenerateEdDSAKeysFromX25519returns a clamped X25519 scalar, butEd25519PrivateKeyParameterstreats those 32 bytes as an Ed25519 seed and derives a different keypair internally. Signatures from this path won’t match the public key returned byGenerateEdDSAKeysFromX25519/VerifyPacketSignature; sign with a raw-scalar/XEdDSA implementation or convert to a real Ed25519 keypair first.🤖 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 `@Meshtastic/Crypto/XEdDSASigning.cs` around lines 192 - 204, The Sign method in XEdDSASigning is using Ed25519Signer with Ed25519PrivateKeyParameters, which interprets the 32-byte input as an Ed25519 seed instead of the X25519-derived scalar expected by GenerateEdDSAKeysFromX25519. Replace this path with an XEdDSA-compatible signing implementation or first convert the X25519 scalar into a real Ed25519 keypair before signing, and ensure the result is consistent with VerifyPacketSignature and the public key produced by the same key-generation flow.
🧹 Nitpick comments (1)
Meshtastic/Crypto/XEdDSASigning.cs (1)
150-153: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake the packet signing byte order explicit.
BitConverter.GetBytesuses host endianness; signatures should be deterministic across runtimes and match the firmware contract. UseBinaryPrimitives.WriteUInt32LittleEndianor the firmware-specified order.🤖 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 `@Meshtastic/Crypto/XEdDSASigning.cs` around lines 150 - 153, The packet signing payload currently relies on BitConverter.GetBytes in the XEdDSASigning byte-building path, which makes the signed byte order depend on host endianness. Update the byte construction in the packet-signing method that assembles meshPacket fields to use an explicit firmware-defined order, preferably little-endian via BinaryPrimitives, so the signature input is deterministic across runtimes and matches the contract.
🤖 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 `@Meshtastic.Test/Crypto/XEdDSASigningTests.cs`:
- Around line 59-65: The tests in XEdDSASigningTests should not rely on an
unbounded loop to find a “positive” Ed25519 key, since
ConvertX25519PublicKeyToEd25519(..., forcePositive: false) will not make that
sign-bit check terminate. Replace the while(true) fixture search in the
negative-test setup with deterministic X25519 private/public key fixtures that
directly exercise GenerateEdDSAKeysFromX25519, and if any random generation is
still kept, guard it with a small attempt limit and fail explicitly. Apply the
same fix pattern in both affected test sections.
In `@Meshtastic/Crypto/XEdDSASigning.cs`:
- Around line 51-53: The Ed25519 key generation path in XEdDSASigning relies on
the internal BouncyCastle lookup for ScalarMultBaseEncoded, which can become
null after package updates or trimming. Update the lookup code to cache the
MethodInfo from Ed25519, verify it is found before invoking it, and fail with a
clear exception if the non-public method is unavailable. If possible, replace
this reflection-based call with a supported public API to avoid the dependency
on internal BouncyCastle behavior.
- Around line 81-87: The negation logic in XEdDSASigning should normalize the
clamped X25519 scalar into the Ed25519 group before negating it. In the signing
path that builds privateKeyScalar, reduce the scalar modulo L first, then
compute the additive inverse modulo L instead of calling
L.Subtract(privateKeyScalar) directly. Update the handling around
privateKeyScalar and negatedPrivateKeyScalar so the resulting bytes represent
(-a mod L), not the magnitude of a negative BigInteger.
---
Outside diff comments:
In `@Meshtastic/Crypto/XEdDSASigning.cs`:
- Around line 62-65: Add null checks before accessing Length in the public
key-conversion helpers so null inputs raise argument exceptions instead of
NullReferenceException. Update GenerateEdDSAKeysFromX25519 and the other helper
referenced in this class to validate the byte[] parameter for null first, then
perform the 32-byte length check and keep the existing ArgumentException/nameof
parameter reporting.
- Around line 148-155: VerifyPacketSignature should fail closed for unsigned or
malformed packets instead of throwing. Add early guards before dereferencing
meshPacket.Decoded or using the signature: if the packet, decoded payload, or
signature is missing/invalid, return false. Update the flow around
BuildSigningBuffer and the Verify call so they are only reached when the packet
has a valid decoded payload and a signature of acceptable length, and make the
same safety check in the related signing/verification path referenced by the
same methods.
- Around line 192-204: The Sign method in XEdDSASigning is using Ed25519Signer
with Ed25519PrivateKeyParameters, which interprets the 32-byte input as an
Ed25519 seed instead of the X25519-derived scalar expected by
GenerateEdDSAKeysFromX25519. Replace this path with an XEdDSA-compatible signing
implementation or first convert the X25519 scalar into a real Ed25519 keypair
before signing, and ensure the result is consistent with VerifyPacketSignature
and the public key produced by the same key-generation flow.
---
Nitpick comments:
In `@Meshtastic/Crypto/XEdDSASigning.cs`:
- Around line 150-153: The packet signing payload currently relies on
BitConverter.GetBytes in the XEdDSASigning byte-building path, which makes the
signed byte order depend on host endianness. Update the byte construction in the
packet-signing method that assembles meshPacket fields to use an explicit
firmware-defined order, preferably little-endian via BinaryPrimitives, so the
signature input is deterministic across runtimes and matches the contract.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9563e6b8-487e-4f6d-b745-1d1271686129
📒 Files selected for processing (2)
Meshtastic.Test/Crypto/XEdDSASigningTests.csMeshtastic/Crypto/XEdDSASigning.cs
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Meshtastic/Crypto/XEdDSASigning.cs (2)
211-216: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReturn
falsefor malformed packet signatures.A packet with a missing/short
XeddsaSignaturecurrently throws fromVerifyinstead of being treated as an invalid signature.🛡️ Proposed fix
public static bool VerifyPacketSignature(byte[] senderPublicKey, MeshPacket meshPacket) { + if (senderPublicKey == null || senderPublicKey.Length != 32) return false; + if (meshPacket?.Decoded == null) return false; + var message = BuildSigningBuffer(meshPacket); var signature = meshPacket.Decoded.XeddsaSignature.ToByteArray(); - var edPublicKey = ConvertX25519PublicKeyToEd25519(senderPublicKey); - return Verify(message, signature, edPublicKey); + if (signature.Length != 64) return false; + + try + { + var edPublicKey = ConvertX25519PublicKeyToEd25519(senderPublicKey); + return Verify(message, signature, edPublicKey); + } + catch (ArgumentException) + { + return false; + } + catch (ArithmeticException) + { + return false; + } }🤖 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 `@Meshtastic/Crypto/XEdDSASigning.cs` around lines 211 - 216, The VerifyPacketSignature method currently lets malformed or missing XeddsaSignature data throw from Verify instead of treating it as an invalid packet. Update VerifyPacketSignature (and any helper path it uses, such as BuildSigningBuffer/Verify) to detect null, missing, or too-short signature bytes on meshPacket.Decoded.XeddsaSignature before verification and return false immediately. Keep the existing verification flow for valid signatures, but guard the conversion and Verify call so malformed packets are safely rejected.
197-202: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winMake the packet signature preimage endian-stable.
BitConverter.GetBytesreturns bytes in host architecture order, so this signature buffer can differ across runtimes; protocol/signature bytes should use an explicit byte order. (learn.microsoft.com)🛠️ Proposed fix
+using System.Buffers.Binary; + private static byte[] BuildSigningBuffer(MeshPacket meshPacket) { - return [ - ..BitConverter.GetBytes(meshPacket.From), - ..BitConverter.GetBytes(meshPacket.Id), - ..BitConverter.GetBytes((uint)meshPacket.Decoded.Portnum), - ..meshPacket.Decoded.Payload.ToByteArray() - ]; + var payload = meshPacket.Decoded.Payload.ToByteArray(); + var buffer = new byte[12 + payload.Length]; + BinaryPrimitives.WriteUInt32LittleEndian(buffer.AsSpan(0, 4), meshPacket.From); + BinaryPrimitives.WriteUInt32LittleEndian(buffer.AsSpan(4, 4), meshPacket.Id); + BinaryPrimitives.WriteUInt32LittleEndian(buffer.AsSpan(8, 4), (uint)meshPacket.Decoded.Portnum); + payload.CopyTo(buffer.AsSpan(12)); + return buffer; }🤖 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 `@Meshtastic/Crypto/XEdDSASigning.cs` around lines 197 - 202, The packet signature preimage in XEdDSASigning should not rely on BitConverter.GetBytes because it uses host endianness and can change the signed bytes across runtimes. Update the preimage construction in the signing path to use an explicit, fixed byte order for meshPacket.From, meshPacket.Id, and the casted meshPacket.Decoded.Portnum before appending meshPacket.Decoded.Payload.ToByteArray(), so the signature buffer is endian-stable.
🧹 Nitpick comments (1)
Meshtastic.Test/Crypto/XEdDSASigningTests.cs (1)
56-70: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that the fixtures exercise both scalar branches.
These tests now rely on fixed “positive”/“negative” keys, but they only assert public-key equality. Add an assertion on the returned private scalar so fixture drift cannot silently stop covering the negation branch.
🧪 Proposed test strengthening
var x25519PrivateKey = Convert.FromBase64String("+ATTWKM68dlArUzWrXWlQm45Gi3ZYrOdDt2z5VT5+2o="); var x25519PublicKey = PKIEncryption.GetPublicKeyFromPrivateKey(x25519PrivateKey); +var expectedClampedPrivateKey = (byte[])x25519PrivateKey.Clone(); +expectedClampedPrivateKey[0] &= 0xF8; +expectedClampedPrivateKey[31] &= 0x7F; +expectedClampedPrivateKey[31] |= 0x40; var edPublicKeyFromPublic = XEdDSASigning.ConvertX25519PublicKeyToEd25519(x25519PublicKey); -var (_, edPublicKeyFromPrivate) = XEdDSASigning.GenerateEdDSAKeysFromX25519(x25519PrivateKey); +var (edPrivateKeyFromPrivate, edPublicKeyFromPrivate) = XEdDSASigning.GenerateEdDSAKeysFromX25519(x25519PrivateKey); +Assert.That(edPrivateKeyFromPrivate, Is.EqualTo(expectedClampedPrivateKey)); Assert.That(edPublicKeyFromPrivate, Is.EqualTo(edPublicKeyFromPublic));var x25519PrivateKey = Convert.FromBase64String("wJNheemu5n2oPgpu0BpEdomsPlChBSM8gAO7RRWkT2w="); var x25519PublicKey = PKIEncryption.GetPublicKeyFromPrivateKey(x25519PrivateKey); +var clampedPrivateKey = (byte[])x25519PrivateKey.Clone(); +clampedPrivateKey[0] &= 0xF8; +clampedPrivateKey[31] &= 0x7F; +clampedPrivateKey[31] |= 0x40; var edPublicKeyFromPublic = XEdDSASigning.ConvertX25519PublicKeyToEd25519(x25519PublicKey); -var (_, edPublicKeyFromPrivate) = XEdDSASigning.GenerateEdDSAKeysFromX25519(x25519PrivateKey); +var (edPrivateKeyFromPrivate, edPublicKeyFromPrivate) = XEdDSASigning.GenerateEdDSAKeysFromX25519(x25519PrivateKey); +Assert.That(edPrivateKeyFromPrivate, Is.Not.EqualTo(clampedPrivateKey)); Assert.That(edPublicKeyFromPrivate, Is.EqualTo(edPublicKeyFromPublic));🤖 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 `@Meshtastic.Test/Crypto/XEdDSASigningTests.cs` around lines 56 - 70, The XEdDSASigning tests only compare the derived public keys, so they don’t verify that both scalar branches are actually exercised. In XEdDSASigningTests, strengthen both ConvertX25519PublicKeyToEd25519_Should_EqualToGenerateEdDSAKeysFromX25519 cases by also asserting the private scalar returned from GenerateEdDSAKeysFromX25519 matches the expected branch behavior, using the existing x25519PrivateKey fixtures to ensure fixture drift can’t hide a missed negation path.
🤖 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 `@Meshtastic/Crypto/XEdDSASigning.cs`:
- Around line 239-262: Validate all Ed25519 inputs at the start of
XEdDSASigning.Sign: it currently checks message and edPrivateKey length but
still dereferences edPrivateKey before a null check and never validates
edPublicKey. Add explicit argument validation for edPrivateKey, edPublicKey, and
their expected 32-byte length before any use, so invalid caller input is
rejected with clear API exceptions instead of internal failures.
---
Outside diff comments:
In `@Meshtastic/Crypto/XEdDSASigning.cs`:
- Around line 211-216: The VerifyPacketSignature method currently lets malformed
or missing XeddsaSignature data throw from Verify instead of treating it as an
invalid packet. Update VerifyPacketSignature (and any helper path it uses, such
as BuildSigningBuffer/Verify) to detect null, missing, or too-short signature
bytes on meshPacket.Decoded.XeddsaSignature before verification and return false
immediately. Keep the existing verification flow for valid signatures, but guard
the conversion and Verify call so malformed packets are safely rejected.
- Around line 197-202: The packet signature preimage in XEdDSASigning should not
rely on BitConverter.GetBytes because it uses host endianness and can change the
signed bytes across runtimes. Update the preimage construction in the signing
path to use an explicit, fixed byte order for meshPacket.From, meshPacket.Id,
and the casted meshPacket.Decoded.Portnum before appending
meshPacket.Decoded.Payload.ToByteArray(), so the signature buffer is
endian-stable.
---
Nitpick comments:
In `@Meshtastic.Test/Crypto/XEdDSASigningTests.cs`:
- Around line 56-70: The XEdDSASigning tests only compare the derived public
keys, so they don’t verify that both scalar branches are actually exercised. In
XEdDSASigningTests, strengthen both
ConvertX25519PublicKeyToEd25519_Should_EqualToGenerateEdDSAKeysFromX25519 cases
by also asserting the private scalar returned from GenerateEdDSAKeysFromX25519
matches the expected branch behavior, using the existing x25519PrivateKey
fixtures to ensure fixture drift can’t hide a missed negation path.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5622634d-009f-42de-8ed4-1a3f9b5dda99
📒 Files selected for processing (2)
Meshtastic.Test/Crypto/XEdDSASigningTests.csMeshtastic/Crypto/XEdDSASigning.cs
| public static byte[] Sign(byte[] message, byte[] edPrivateKey, byte[] edPublicKey) | ||
| { | ||
| if (message == null) throw new ArgumentNullException(nameof(message)); | ||
| if (edPrivateKey.Length != 32) throw new ArgumentException("Ed25519 private key must be 32 bytes", nameof(edPrivateKey)); | ||
| var signature = new byte[64]; | ||
| /* | ||
| * Ed25519Signer computes public key from private key differently than firmware (see comment for GeneratePublicKeyFromPrivateKey) | ||
| * and causes signature verification by actual radios to fail, so instead use Ed25519 primitive directly, explicitly passing | ||
| * public key computed derived as in XEdDSA::priv_curve_to_ed_keys. | ||
| * | ||
| * Also, Ed25519.Sign passes sk (private key) through SHA-512 before feeding it to actual ImplSign, which for some reason | ||
| * later fails to verify both by firmware and by Ed25519.Verify. | ||
| * | ||
| * The following code is inlined from BouncyCastle's Ed25519 class, method: | ||
| * private static void ImplSign(byte[] sk, int skOff, byte[] pk, int pkOff, byte[] ctx, byte phflag, byte[] m, int mOff, int mLen, byte[] sig, int sigOff) | ||
| */ | ||
|
|
||
| // Hash the message using the selected algorithm | ||
| var messageHash = useShortHash ? SHA256.HashData(message) : SHA512.HashData(message); | ||
| var digest = new Sha512Digest(); | ||
| var h = new byte[64]; | ||
|
|
||
| // Create Ed25519 signer | ||
| var signer = new Ed25519Signer(); | ||
| var privateKeyParams = new Ed25519PrivateKeyParameters(edPrivateKey, 0); | ||
|
|
||
| signer.Init(true, privateKeyParams); | ||
| signer.BlockUpdate(messageHash, 0, messageHash.Length); | ||
|
|
||
| return signer.GenerateSignature(); | ||
| digest.BlockUpdate(edPrivateKey, 0, 32); | ||
| digest.DoFinal(h, 0); | ||
|
|
||
| Ed25519_ImplSign(digest, h, edPrivateKey /* original argument: s, scalar computed from h */, edPublicKey, 0, null, 0, message, 0, message.Length, signature, 0); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file around the reviewed method
FILE="Meshtastic/Crypto/XEdDSASigning.cs"
wc -l "$FILE"
sed -n '200,290p' "$FILE" | cat -n
# Find the Sign method and any nearby validation helpers/usages
rg -n "Sign\(byte\[\] message, byte\[\] edPrivateKey, byte\[\] edPublicKey\)|edPublicKey|edPrivateKey" Meshtastic/Crypto/XEdDSASigning.cs Meshtastic -SRepository: meshtastic/c-sharp
Length of output: 17623
Validate both Ed25519 inputs before signing. Sign still accepts a null or non-32-byte edPublicKey, and edPrivateKey also needs a null check before Length is read. Add explicit argument validation here so bad caller input fails with a clear API error instead of an internal exception.
🤖 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 `@Meshtastic/Crypto/XEdDSASigning.cs` around lines 239 - 262, Validate all
Ed25519 inputs at the start of XEdDSASigning.Sign: it currently checks message
and edPrivateKey length but still dereferences edPrivateKey before a null check
and never validates edPublicKey. Add explicit argument validation for
edPrivateKey, edPublicKey, and their expected 32-byte length before any use, so
invalid caller input is rejected with clear API exceptions instead of internal
failures.
|
With those changes, firmware's signatures are properly validated, and this library's signatures are recognized by the firmware. Implementation details are ugly, however, because the way firmware implemented |
1 participant
Related to #130 - suggestion for changes to
XEdDSASigningto make it work with actual signatures as generated by latest firmware. Verified manually via packet dumps from MQTT. This could use some actual test cases.Summary by CodeRabbit
Breaking Changes
Sign/Verify/VerifyWithX25519Keyno longer accept the hashing-mode parameter;ConvertX25519PublicKeyToEd25519now supports an optionalforcePositiveflag.Bug Fixes
Tests