Add AES-GMAC-SIV (proto v12) support#1
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds AES‑GMAC‑SIV authenticated‑encryption support, per‑peer/root protocol version propagation and selection of outbound keys, updates packet ID extraction to use encoded/armored headers, and extends tests to cover AES‑GMAC‑SIV, MAC‑only, and wrong‑key failure cases. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HelloClient
participant Runtime
participant PacketCrypto
participant Peer
Client->>HelloClient: HelloAsync()
HelloClient->>Peer: UDP HELLO (advertised pv)
Peer-->>HelloClient: HELLO_OK (remoteProto)
HelloClient-->>Client: return remoteProto
Client->>Runtime: init(rootProtocolVersion = remoteProto)
Runtime->>PacketCrypto: SelectOutboundKey(rootKey, peerProto)
Runtime->>PacketCrypto: Armor(packet, selectedKey)
PacketCrypto-->>Runtime: Armored packet (header contains packetId)
Runtime->>Peer: Send Armored packet
Peer-->>Runtime: Response (armored, includes packetId)
Runtime->>PacketCrypto: Dearmor(response, selectedKey)
PacketCrypto-->>Runtime: Plaintext (verified)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 758e41e1de
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (encryptPayload && key.Length >= SymmetricKeyLength) | ||
| { | ||
| ArmorAesGmacSiv(packet, key.Slice(0, SymmetricKeyLength)); |
There was a problem hiding this comment.
Switching to cipher suite 3 purely when key.Length >= 48 makes every encrypted packet use AES-GMAC-SIV, even when the peer/root reported an older protocol (the HELLO path still parses RemoteProtocolVersion but nothing in this selection checks it). In mixed deployments with protocol-11 nodes, WHOIS/MULTICAST_GATHER/NETCONF and peer traffic will be sent with cipher 3 and can be dropped, causing timeouts after HELLO despite having a valid shared key. Please tie this branch to negotiated peer capability rather than key size alone.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ZTSharp/ZeroTier/Internal/ZeroTierNetworkConfigClient.cs (1)
633-633:⚠️ Potential issue | 🟠 MajorProtocol-version metadata
"pv"is stale after the v12 bump.The comment on this line explicitly states the value should match
HELLO, butAdvertisedProtocolVersionwas just changed to12inZeroTierHelloClient.cs. Sendingpv=11inNETWORK_CONFIG_REQUESTwhile advertising protocol 12 inHELLOcreates an inconsistency that may confuse the controller.🐛 Proposed fix
- AppendHexKeyValue(sb, "pv", 11); // Protocol (matches our HELLO advert) + AppendHexKeyValue(sb, "pv", ZeroTierHelloClient.AdvertisedProtocolVersion); // Protocol (matches our HELLO advert)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ZTSharp/ZeroTier/Internal/ZeroTierNetworkConfigClient.cs` at line 633, The metadata key "pv" is hardcoded to 11, which is now inconsistent with the HELLO advert; update the NETWORK_CONFIG_REQUEST to use the same protocol version as the HELLO code by replacing the literal 11 passed to AppendHexKeyValue(sb, "pv", 11) with the current AdvertisedProtocolVersion (or the matching constant) used in ZeroTierHelloClient (e.g., reference ZeroTierHelloClient.AdvertisedProtocolVersion) so both messages advertise the same protocol version.
🧹 Nitpick comments (1)
ZTSharp.Tests/ZeroTierPacketCryptoTests.cs (1)
117-125: Consider adding a known-answer test (KAT) for AES-GMAC-SIV.The current coverage is a round-trip only. A KAT with a fixed key, fixed IV, fixed payload, and pre-computed expected encrypted output would catch silent regressions in the KBKDF, GMAC, or CTR implementations (e.g., label swap in
DeriveAesGmacSivKeys, counter increment bug) that a round-trip test cannot detect.Would you like me to generate a KAT fixture (fixed key/IV → expected ciphertext bytes) for the AES-GMAC-SIV path and open a tracking issue?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ZTSharp.Tests/ZeroTierPacketCryptoTests.cs` around lines 117 - 125, Add a deterministic known-answer test (KAT) for the AES-GMAC-SIV path to detect silent regressions: create a fixed 48-byte key, fixed nonce/IV and fixed payload bytes, call ZeroTierPacketCrypto.Armor(...) with encryptPayload: true, and assert the resulting ciphertext bytes equal a precomputed expected ciphertext (and verify the header bits check currently done: (encrypted[18] & 0x38) >> 3 == 3); then call ZeroTierPacketCrypto.Dearmor(...) and assert it returns true and the dearmored payload matches the original fixed plaintext. Use explicit constants and reference the key-derivation function DeriveAesGmacSivKeys, Armor, and Dearmor in the test so regressions in KBKDF/GMAC/CTR are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@ZTSharp/ZeroTier/Internal/ZeroTierNetworkConfigClient.cs`:
- Line 633: The metadata key "pv" is hardcoded to 11, which is now inconsistent
with the HELLO advert; update the NETWORK_CONFIG_REQUEST to use the same
protocol version as the HELLO code by replacing the literal 11 passed to
AppendHexKeyValue(sb, "pv", 11) with the current AdvertisedProtocolVersion (or
the matching constant) used in ZeroTierHelloClient (e.g., reference
ZeroTierHelloClient.AdvertisedProtocolVersion) so both messages advertise the
same protocol version.
---
Nitpick comments:
In `@ZTSharp.Tests/ZeroTierPacketCryptoTests.cs`:
- Around line 117-125: Add a deterministic known-answer test (KAT) for the
AES-GMAC-SIV path to detect silent regressions: create a fixed 48-byte key,
fixed nonce/IV and fixed payload bytes, call ZeroTierPacketCrypto.Armor(...)
with encryptPayload: true, and assert the resulting ciphertext bytes equal a
precomputed expected ciphertext (and verify the header bits check currently
done: (encrypted[18] & 0x38) >> 3 == 3); then call
ZeroTierPacketCrypto.Dearmor(...) and assert it returns true and the dearmored
payload matches the original fixed plaintext. Use explicit constants and
reference the key-derivation function DeriveAesGmacSivKeys, Armor, and Dearmor
in the test so regressions in KBKDF/GMAC/CTR are caught.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
ZTSharp.Tests/ZeroTierPacketCryptoTests.csZTSharp/ZeroTier/Internal/ZeroTierDataplaneRuntime.csZTSharp/ZeroTier/Internal/ZeroTierHelloClient.csZTSharp/ZeroTier/Internal/ZeroTierMulticastGatherClient.csZTSharp/ZeroTier/Internal/ZeroTierNetworkConfigClient.csZTSharp/ZeroTier/Internal/ZeroTierWhoisClient.csZTSharp/ZeroTier/Protocol/ZeroTierPacketCrypto.cs
- Keep NETWORK_CONFIG_REQUEST pv in sync with HELLO advert - Use negotiated remote protocol version to select cipher - Add AES-GMAC-SIV known-answer test
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 (1)
ZTSharp/ZeroTier/Protocol/ZeroTierPacketCrypto.cs (1)
35-53:⚠️ Potential issue | 🟠 MajorZeroTierIpv4Link bypasses protocol-negotiated key selection.
ZeroTierIpv4Linkpasses its_sharedKeydirectly toArmor()without callingSelectOutboundKey(). Since the constructor does not receive peer protocol version information, it cannot pre-select the key based on negotiated capabilities. Any peer communicating via ZeroTierIpv4Link will silently upgrade to AES-GMAC-SIV whenever the shared key is ≥48 bytes, regardless of protocol version. Add protocol version to the constructor and callSelectOutboundKey()beforeArmor()at lines 568 and via line 80 (ExtFramePacketBuilder).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ZTSharp/ZeroTier/Protocol/ZeroTierPacketCrypto.cs` around lines 35 - 53, ZeroTierIpv4Link currently passes its raw _sharedKey into ZeroTierPacketCrypto.Armor which lets Armor pick AES-GMAC-SIV whenever the key is ≥48 bytes; to fix, add a protocol version parameter to the ZeroTierIpv4Link constructor (and propagate it to ExtFramePacketBuilder), use that version to call SelectOutboundKey(_sharedKey, protocolVersion) to get the correctly negotiated outbound key, and pass that selected key into Armor instead of _sharedKey (ensure ExtFramePacketBuilder does the same before its call into Armor/ArmorAesGmacSiv); update all Armor call sites in ZeroTierIpv4Link and ExtFramePacketBuilder to use the SelectOutboundKey result.
🧹 Nitpick comments (1)
ZTSharp.Tests/ZeroTierHelloClientTests.cs (1)
88-96: Consider asserting the returned protocol version.HelloAsync now returns the remote protocol version; asserting it here would exercise the new contract and catch regressions.
💡 Possible assertion
- _ = await ZeroTierHelloClient.HelloAsync( + var remoteProtocolVersion = await ZeroTierHelloClient.HelloAsync( udp, localIdentity, planet, destination: remoteIdentity.NodeId, physicalDestination: remoteEndpoint, sharedKey: sharedKey, timeout: TimeSpan.FromSeconds(2), cancellationToken: CancellationToken.None); + Assert.Equal(11, remoteProtocolVersion);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ZTSharp.Tests/ZeroTierHelloClientTests.cs` around lines 88 - 96, The test currently discards HelloAsync's return value; capture the return from ZeroTierHelloClient.HelloAsync into a variable (e.g., protocolVersion or returnedVersion) and add an assertion that it equals the expected remote protocol version (use the known value for the test's planet/remoteIdentity); update the test around the HelloAsync call to assert the returned protocol version to exercise the new contract and catch regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ZTSharp/ZeroTier/Protocol/ZeroTierPacketCrypto.cs`:
- Around line 271-295: In DeriveAesGmacSivKeys change the KBKDF counter
parameter passed to KbkdfHmacSha384 from iter: 0 to iter: 1 for both derivations
so the KBKDF counter starts at 1 as required by NIST SP 800‑108; update the two
calls to KbkdfHmacSha384 (the ones using KbkdfLabelAesGmacSivK0 and
KbkdfLabelAesGmacSivK1) to use iter: 1 so the derived k0 and k1 match the
reference implementation.
---
Outside diff comments:
In `@ZTSharp/ZeroTier/Protocol/ZeroTierPacketCrypto.cs`:
- Around line 35-53: ZeroTierIpv4Link currently passes its raw _sharedKey into
ZeroTierPacketCrypto.Armor which lets Armor pick AES-GMAC-SIV whenever the key
is ≥48 bytes; to fix, add a protocol version parameter to the ZeroTierIpv4Link
constructor (and propagate it to ExtFramePacketBuilder), use that version to
call SelectOutboundKey(_sharedKey, protocolVersion) to get the correctly
negotiated outbound key, and pass that selected key into Armor instead of
_sharedKey (ensure ExtFramePacketBuilder does the same before its call into
Armor/ArmorAesGmacSiv); update all Armor call sites in ZeroTierIpv4Link and
ExtFramePacketBuilder to use the SelectOutboundKey result.
---
Nitpick comments:
In `@ZTSharp.Tests/ZeroTierHelloClientTests.cs`:
- Around line 88-96: The test currently discards HelloAsync's return value;
capture the return from ZeroTierHelloClient.HelloAsync into a variable (e.g.,
protocolVersion or returnedVersion) and add an assertion that it equals the
expected remote protocol version (use the known value for the test's
planet/remoteIdentity); update the test around the HelloAsync call to assert the
returned protocol version to exercise the new contract and catch regressions.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
ZTSharp.Tests/ZeroTierHelloClientTests.csZTSharp.Tests/ZeroTierMulticastGatherClientTests.csZTSharp.Tests/ZeroTierPacketCryptoTests.csZTSharp/ZeroTier/Internal/ZeroTierDataplaneRuntime.csZTSharp/ZeroTier/Internal/ZeroTierHelloClient.csZTSharp/ZeroTier/Internal/ZeroTierMulticastGatherClient.csZTSharp/ZeroTier/Internal/ZeroTierMulticastLikeClient.csZTSharp/ZeroTier/Internal/ZeroTierNetworkConfigClient.csZTSharp/ZeroTier/Internal/ZeroTierPeerKeyCache.csZTSharp/ZeroTier/Internal/ZeroTierWhoisClient.csZTSharp/ZeroTier/Protocol/ZeroTierPacketCrypto.csZTSharp/ZeroTier/ZeroTierSocket.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- ZTSharp/ZeroTier/Internal/ZeroTierHelloClient.cs
| private static void DeriveAesGmacSivKeys(ReadOnlySpan<byte> key48, Span<byte> k0, Span<byte> k1) | ||
| { | ||
| if (key48.Length != SymmetricKeyLength) | ||
| { | ||
| throw new ArgumentException($"Key must be exactly {SymmetricKeyLength} bytes.", nameof(key48)); | ||
| } | ||
|
|
||
| if (k0.Length != KeyLength) | ||
| { | ||
| throw new ArgumentException($"k0 must be {KeyLength} bytes.", nameof(k0)); | ||
| } | ||
|
|
||
| if (k1.Length != KeyLength) | ||
| { | ||
| throw new ArgumentException($"k1 must be {KeyLength} bytes.", nameof(k1)); | ||
| } | ||
|
|
||
| Span<byte> derived = stackalloc byte[SymmetricKeyLength]; | ||
|
|
||
| KbkdfHmacSha384(key48, label: KbkdfLabelAesGmacSivK0, context: 0, iter: 0, destination: derived); | ||
| derived.Slice(0, KeyLength).CopyTo(k0); | ||
|
|
||
| KbkdfHmacSha384(key48, label: KbkdfLabelAesGmacSivK1, context: 0, iter: 0, destination: derived); | ||
| derived.Slice(0, KeyLength).CopyTo(k1); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
ZeroTier protocol v12 KBKDF HMAC-SHA384 counter value specification
💡 Result:
For ZeroTier protocol v12’s KBKDF (HMAC‑SHA384) in counter mode, the counter field is the NIST SP 800‑108 “KDF in Counter Mode” counter:
- The PRF input for block i is
([i]₂ || Label || 0x00 || Context || [L]₂). - The counter i starts at 1 and increments up to n (the number of PRF blocks needed).
- The counter
[i]₂is encoded as a fixed-length r-bit binary string (with r chosen by the protocol; NIST requires the length and bit order be agreed/defined by the using protocol). [1]
RustCrypto’s kbkdf crate (a SP 800‑108 implementation) also points to SP 800‑108 for this definition. [2]
Sources: [1] [2]
🏁 Script executed:
# Search for ZeroTier protocol documentation in the repository
find . -type f \( -name "*.md" -o -name "*.txt" -o -name "*.rst" \) | head -20Repository: JKamsker/ZTSharp
Length of output: 431
🏁 Script executed:
# Look for test files or references to KBKDF counter values
rg "iter.*[0-9]|KbkdfHmacSha384|KBKDF" --type cs -iRepository: JKamsker/ZTSharp
Length of output: 3142
🏁 Script executed:
# Check the actual implementation of KbkdfHmacSha384 to understand the counter semantics
rg -A 10 "void KbkdfHmacSha384|KbkdfHmacSha384" --type cs | head -50Repository: JKamsker/ZTSharp
Length of output: 1625
KBKDF counter must start at 1 per NIST SP 800‑108.
The code currently passes iter: 0 for both k0 and k1 derivation in DeriveAesGmacSivKeys. However, NIST SP 800‑108 (which ZeroTier v12 protocol explicitly follows) specifies that the counter field starts at 1, not 0. The derived keys will not match the reference implementation.
Required fix
- KbkdfHmacSha384(key48, label: KbkdfLabelAesGmacSivK0, context: 0, iter: 0, destination: derived);
+ KbkdfHmacSha384(key48, label: KbkdfLabelAesGmacSivK0, context: 0, iter: 1, destination: derived);
derived.Slice(0, KeyLength).CopyTo(k0);
- KbkdfHmacSha384(key48, label: KbkdfLabelAesGmacSivK1, context: 0, iter: 0, destination: derived);
+ KbkdfHmacSha384(key48, label: KbkdfLabelAesGmacSivK1, context: 0, iter: 1, destination: derived);
derived.Slice(0, KeyLength).CopyTo(k1);📝 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.
| private static void DeriveAesGmacSivKeys(ReadOnlySpan<byte> key48, Span<byte> k0, Span<byte> k1) | |
| { | |
| if (key48.Length != SymmetricKeyLength) | |
| { | |
| throw new ArgumentException($"Key must be exactly {SymmetricKeyLength} bytes.", nameof(key48)); | |
| } | |
| if (k0.Length != KeyLength) | |
| { | |
| throw new ArgumentException($"k0 must be {KeyLength} bytes.", nameof(k0)); | |
| } | |
| if (k1.Length != KeyLength) | |
| { | |
| throw new ArgumentException($"k1 must be {KeyLength} bytes.", nameof(k1)); | |
| } | |
| Span<byte> derived = stackalloc byte[SymmetricKeyLength]; | |
| KbkdfHmacSha384(key48, label: KbkdfLabelAesGmacSivK0, context: 0, iter: 0, destination: derived); | |
| derived.Slice(0, KeyLength).CopyTo(k0); | |
| KbkdfHmacSha384(key48, label: KbkdfLabelAesGmacSivK1, context: 0, iter: 0, destination: derived); | |
| derived.Slice(0, KeyLength).CopyTo(k1); | |
| } | |
| private static void DeriveAesGmacSivKeys(ReadOnlySpan<byte> key48, Span<byte> k0, Span<byte> k1) | |
| { | |
| if (key48.Length != SymmetricKeyLength) | |
| { | |
| throw new ArgumentException($"Key must be exactly {SymmetricKeyLength} bytes.", nameof(key48)); | |
| } | |
| if (k0.Length != KeyLength) | |
| { | |
| throw new ArgumentException($"k0 must be {KeyLength} bytes.", nameof(k0)); | |
| } | |
| if (k1.Length != KeyLength) | |
| { | |
| throw new ArgumentException($"k1 must be {KeyLength} bytes.", nameof(k1)); | |
| } | |
| Span<byte> derived = stackalloc byte[SymmetricKeyLength]; | |
| KbkdfHmacSha384(key48, label: KbkdfLabelAesGmacSivK0, context: 0, iter: 1, destination: derived); | |
| derived.Slice(0, KeyLength).CopyTo(k0); | |
| KbkdfHmacSha384(key48, label: KbkdfLabelAesGmacSivK1, context: 0, iter: 1, destination: derived); | |
| derived.Slice(0, KeyLength).CopyTo(k1); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ZTSharp/ZeroTier/Protocol/ZeroTierPacketCrypto.cs` around lines 271 - 295, In
DeriveAesGmacSivKeys change the KBKDF counter parameter passed to
KbkdfHmacSha384 from iter: 0 to iter: 1 for both derivations so the KBKDF
counter starts at 1 as required by NIST SP 800‑108; update the two calls to
KbkdfHmacSha384 (the ones using KbkdfLabelAesGmacSivK0 and
KbkdfLabelAesGmacSivK1) to use iter: 1 so the derived k0 and k1 match the
reference implementation.
- Select outbound key in ExtFramePacketBuilder - Avoid AES-GMAC-SIV in ZeroTierIpv4Link for v11 peers - Assert HelloAsync returns peer protocol version
|
@coderabbitai review |
|
✅ Actions performedReview triggered.
|
|
Codex Review: Didn't find any major issues. Delightful! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Implements ZeroTier protocol v12+ crypto path (cipher suite 3 / AES-GMAC-SIV) for encrypted packets.
Key points:
dotnet test -c Release passes locally.
Summary by CodeRabbit
New Features
Updates
Tests