feat(sam): Datagram2/Datagram3 session styles and offline signatures - #2
Open
gg582 wants to merge 5 commits into
Open
feat(sam): Datagram2/Datagram3 session styles and offline signatures#2gg582 wants to merge 5 commits into
gg582 wants to merge 5 commits into
Conversation
Add DATAGRAM2/DATAGRAM3 session styles mapped to protocols 19 and 20, extend private destination parsing with the offline signature section, and carry the transient signing key through LocalDestination so datagram signing works when the long-term key is kept offline.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 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 |
PR Summary by QodoAdd SAM Datagram2/3 and offline signature support
AI Description
Diagram
High-Level Assessment
Files changed (21)
|
Code Review by Qodo
1. Looped SAM connections leak on failure
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
 T-Rex LogsWhat T-Rex did
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+305
to
+315
| if d.offline != nil { | ||
| switch d.offline.keyType { | ||
| case SigningEdDSASHA512Ed25519: | ||
| return ed25519.Sign(ed25519.NewKeyFromSeed(d.offline.private), message), nil | ||
| case SigningRedDSASHA512Ed25519: | ||
| var private [32]byte | ||
| copy(private[:], d.offline.private) | ||
| return Red25519Sign(private, message) | ||
| default: | ||
| return nil, ErrEncryptedSigningKey | ||
| } |
There was a problem hiding this comment.
Offline destinations now sign LeaseSet2 payloads with their transient signing key, but LeaseSet2 publication still emits zero flags and no offline authorization. Receiving peers consequently verify the transient-key signature with the destination’s long-term public key and reject the LeaseSet2, leaving the offline SAM destination unreachable. Serialize the offline authorization, set the offline flag, and size and sign the LeaseSet2 using the transient signature type before merging.
Artifacts
- Temporary test authored for this validation; it creates destinations, serializes and parses LeaseSet2 payloads, and asserts the online and offline verification outcomes, with the takeaway that the candidate path is directly exercised.
- Temporary runner that copies the test into the internal package, runs baseline and offline cases, captures their command output, and removes the copied test, with the takeaway that repository production code was not modified.
Online LeaseSet2 baseline verification output
- Executed online baseline command output showing flags zero with no offline section still verifies successfully, with the takeaway that the standard non-offline serialization remains valid.
Offline LeaseSet2 peer verification output
- Executed offline destination command output showing flags zero and no offline section while peer verification returns false, with the takeaway that the offline-published LeaseSet2 is rejected.
LeaseSet2 signing and verification source evidence
- Executed capture of the exact relevant source lines showing transient signing, literal LS2 flags zero, and the verifier’s conditional offline-key path, with the takeaway that the observed rejection matches the code path.
Prompt To Fix With AI
This is a comment left during a code review.
Path: foundation/address_generator.go
Line: 305-315
Comment:
**Offline LeaseSets Are Rejected**
Offline destinations now sign LeaseSet2 payloads with their transient signing key, but LeaseSet2 publication still emits zero flags and no offline authorization. Receiving peers consequently verify the transient-key signature with the destination’s long-term public key and reject the LeaseSet2, leaving the offline SAM destination unreachable. Serialize the offline authorization, set the offline flag, and size and sign the LeaseSet2 using the transient signature type before merging.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Qodo Fixer🍒 Ready to be cherry-picked — ✅ Merged (0) · ☑ Fixed (6) 🔗 Fix PR: #3 This fix PR was closed automatically. Its branch is preserved so you can cherry pick the changes into the original PR. Prompt for coding agent Process — 6 fixed
|
LocalDestination.Sign now fails with ErrOfflineSignatureExpired once the offline authorization has lapsed, so stale transient keys cannot keep producing Datagram2 traffic. Also wipe the throwaway X25519 key generated for ElGamal private destinations and document that Datagram3's FROM field is attacker-controlled.
Member
Author
|
Follow-up security audit fixes (830e457):
|
|
Code review by qodo was updated up to the latest commit 830e457 |
|
Reviews (2): Last reviewed commit: "fix(foundation): reject expired offline ..." | Re-trigger Greptile |
- Inject clock into SAM server and foundation offline signing instead of reading the wall clock in verification and expiry paths - Parse SAM offline private key sections as zero-copy views over the caller-owned wire buffer - Unify datagram offline signature type with foundation.OfflineSignature - Wipe expanded Ed25519 keys and Red25519 key copies after signing - Add bounded MarshalSignedContentTo serializer with explicit capacity errors - Reject SESSION CREATE for offline destinations combined with legacy DATAGRAM style or encrypted LeaseSet options; Datagram1 cannot carry the offline authorization and blinding requires the long-term key - Publish LeaseSet2 with the offline signature section and flag so peers can verify transient-key signatures, refusing expired authorizations - Deflake loopback test: RECEIVED lines may precede the STATUS reply on the shared control connection
Member
Author
|
CI failure and review findings addressed in f612c45. CI: TestEmbeddedServerLiveLoopbackStylesAndRecovery was racing — inbound forwarding may write DATAGRAM RECEIVED on the control connection before the STATUS reply. The test now accepts either ordering (same fix applied to the RAW section). Review findings, all fixed:
Verified: go build ./..., full go test -count=1 ./... (the only intermittent failure is the unrelated pre-existing TestSSU2LiveVectorReadAuthDispatchWriteAllocations timing test), importformatter, gojgp lint. |
| ctx, cancel := context.WithCancel(root.ctx) | ||
| child := &samSession{server: s, root: root, id: id, style: style, endpoint: root.endpoint, control: connection, ctx: ctx, cancel: cancel, sourceIP: root.sourceIP, fromPort: fromPort, toPort: toPort, listenPort: listenPort, protocol: protocol, listenProtocol: listenProtocol, rawHeader: rawHeader, udpTarget: udpTarget, children: make(map[string]*samSession), attachments: make(map[net.Conn]struct{}), queueBytes: newByteBudget(s.config.MaxSessionQueueBytes), acceptRequests: make(chan acceptRequest, s.config.SessionQueue)} | ||
| child.datagramOverhead = root.datagramOverhead | ||
| child := &samSession{server: s, root: root, id: id, style: style, endpoint: root.endpoint, control: connection, ctx: ctx, cancel: cancel, sourceIP: root.sourceIP, fromPort: fromPort, toPort: toPort, listenPort: listenPort, protocol: protocol, listenProtocol: listenProtocol, rawHeader: rawHeader, udpTarget: udpTarget, now: root.now, children: make(map[string]*samSession), attachments: make(map[net.Conn]struct{}), queueBytes: newByteBudget(s.config.MaxSessionQueueBytes), acceptRequests: make(chan acceptRequest, s.config.SessionQueue)} |
There was a problem hiding this comment.
2. Offline primary permits datagram1 child 🐞 Bug ≡ Correctness
An offline STYLE=PRIMARY session can add a legacy STYLE=DATAGRAM child because addSubsession reuses the offline root endpoint without applying the new Datagram1 restriction. Outbound packets are then signed by the transient key, but Datagram1 cannot carry its authorization, so peers reject every packet.
Agent Prompt
## Issue description
Offline PRIMARY sessions can add a legacy DATAGRAM subsession, bypassing the root-session validation. Reject such subsessions because Datagram1 cannot transmit the offline authorization needed to verify transient-key signatures.
## Issue Context
The child reuses the root endpoint and therefore signs with the offline transient key. DATAGRAM2 supports the authorization section and DATAGRAM3 is unsigned, so only legacy DATAGRAM must be rejected.
## Fix Focus Areas
- client/internal/sam/protocol.go[275-275]
- client/internal/sam/offline_test.go[168-230]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+126
to
+130
| if offline != nil { | ||
| // Peers verify the LS2 signature with the transient key authorized by | ||
| // this offline signature; publishing past its expiry is useless. | ||
| if published > uint64(offline.Expires) { | ||
| return 0, ErrLocalLeaseSet2 |
There was a problem hiding this comment.
3. Offline leasesets outlive authorization 🐞 Bug ⛨ Security
The offline expiry check rejects only a publication whose published time is already expired, then allows leases whose end time extends past the transient-key authorization. Remote LS2 verification and database retention do not enforce Offline.Expires, so that LeaseSet remains usable until its normal lease expiry after the offline authorization has expired.
Agent Prompt
## Issue description
Offline-signed LeaseSet2 payloads can advertise lease validity beyond the offline authorization expiry. The local serializer checks only whether publication begins after expiry, while receivers retain a signature-valid LeaseSet based on ordinary lease dates.
## Issue Context
Ensure an offline signature limits the complete validity period of a LeaseSet signed with its transient key. Cap or reject local lease ranges extending beyond `offline.Expires`, and enforce the same bound during remote LS2 admission/validation so externally produced payloads cannot bypass it.
## Fix Focus Areas
- networking/internal/netdb/local_ls2.go[126-155]
- networking/internal/netdb/database.go[291-329]
- networking/internal/netdb/structures.go[542-556]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit f612c45 |
TestSSU2LiveVectorReadAuthDispatchWriteAllocations timed out on loaded CI runners: the 500ms handshake timeout and 5s warmup/delivery waits left no room for scheduling delays. Raise the handshake timeout to 2s and the waits to 30s; the allocation assertions are unchanged. TestDatagramModernSendWithoutEndpointSupport reused the session ID dg across styles, but session teardown after connection close is asynchronous, so the second SESSION CREATE raced with removal and could fail with DUPLICATED_ID. Use a distinct ID per style.
T-Rex LogsWhat T-Rex did
Reviews (3): Last reviewed commit: "fix: address review findings on offline ..." | Re-trigger Greptile |
|
Code review by qodo was updated up to the latest commit f260e77 |
|
Reviews (4): Last reviewed commit: "test: deflake SSU2 allocation and SAM en..." | Re-trigger Greptile |
gg582
marked this pull request as ready for review
September 5, 2026 04:23
- SESSION ADD under an offline PRIMARY root now rejects STYLE=DATAGRAM, matching the createSession restriction; Datagram1 cannot carry the offline authorization - LocalLeaseSet2.MarshalTo caps lease end times at the offline authorization expiry so no published lease outlives the transient key - Wipe the expanded transient Ed25519 key in the netdb offline test helper
Comment on lines
+284
to
+288
| _, _ = io.WriteString(control, "SESSION CREATE STYLE="+style+" ID="+id+" DESTINATION=TRANSIENT\n") | ||
| if line := readSAMLine(t, reader); !strings.Contains(line, "RESULT=OK") { | ||
| t.Fatalf("%s create = %q", style, line) | ||
| } | ||
| _, _ = io.WriteString(control, "DATAGRAM SEND ID="+id+" DESTINATION=peer.i2p SIZE=4\nDATA") |
There was a problem hiding this comment.
1. Looped sam connections leak on failure 📜 Skill insight ▣ Testability
Each loop iteration creates a control connection, but control.Close() is reached only on the success path; a failed readSAMLine or assertion exits via t.Fatalf and leaves that connection open. This violates the requirement that tests clean up every resource they create.
Agent Prompt
## Issue description
The loop creates a SAM control connection but closes it only after all assertions succeed, so test failures can leak connections and associated session resources.
## Issue Context
Use per-iteration cleanup that runs when `t.Fatalf` aborts the current test iteration. Preserve the distinct session IDs introduced to avoid asynchronous teardown races.
## Fix Focus Areas
- client/internal/sam/datagram_modern_test.go[284-288]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit f260e77 |
Qodo Fixer🍒 Ready to be cherry-picked — ✅ Merged (0) · ☑ Fixed (4) 🔗 Fix PR: #4 This fix PR was closed automatically. Its branch is preserved so you can cherry pick the changes into the original PR. Prompt for coding agent Process — 4 fixed
|
Member
Author
|
Latest review round addressed in 50ab427:
Added regression tests for both behaviors (TestSessionAddOfflinePrimaryDatagram1Rejected, TestLocalLeaseSet2OfflineCapsLeaseExpiry). Full suite, importformatter, and gojgp lint all pass. |
Comment on lines
+146
to
+149
| if offline != nil && leases[i].EndDate > offline.Expires { | ||
| // Remote verifiers stop trusting the transient key at the offline | ||
| // authorization expiry, so no lease may outlive it. | ||
| leases[i].EndDate = offline.Expires |
There was a problem hiding this comment.
1. Publisher reuses expired capped ls2 🐞 Bug ☼ Reliability
LocalLeaseSet2.MarshalTo caps only its serialized lease copy, while LeaseSetPublisher schedules and caches that payload using the original later inbound expiry. After the offline authorization expires, unchanged inbound leases therefore leave the expired cached LS2 eligible for forced or scheduled republication, producing rejected network advertisements.
Agent Prompt
## Issue description
Offline LS2 serialization caps lease end dates at the offline authorization expiry, but `LeaseSetPublisher` continues tracking the original inbound lease expiry. This allows its cached LS2 payload to be republished after the serialized lease and authorization have expired.
## Issue Context
The effective expiry used for publication scheduling and cached-payload validity must match the expiry actually serialized into the LS2. Expired cached payloads must not be sent, including forced publication paths.
## Fix Focus Areas
- networking/internal/netdb/local_ls2.go[145-155]
- networking/internal/netdb/publication.go[269-355]
- networking/internal/netdb/publication.go[402-405]
- networking/internal/netdb/local_ls2_offline_test.go[120-157]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 50ab427 |
|
Reviews (5): Last reviewed commit: "fix: close remaining offline-signature g..." | Re-trigger Greptile |
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.
Summary
Adds modern I2P datagram protocols and offline signature support to the SAM bridge.
Testing