telemetry/geoprobe-target-sender: opt-in --challenged inbound probing#3738
Merged
Conversation
d825351 to
9fdc6c4
Compare
97a2d21 to
601e4e4
Compare
ben-dz
added a commit
that referenced
this pull request
May 21, 2026
) ## Summary of Changes - Adds an opt-in nonce handshake to signed-TWAMP inbound probing. The reflector now issues a fresh 8-byte random nonce on every Probe 0 and places it in `Reply0.SinceLastRxNs` (previously always 0). On Probe 1, if the sender echoed that nonce in `Sec || Frac` and re-signed the packet, the reflector sets bit 7 of `Reply1.NumOffsets` (`Challenged` flag) — cryptographic proof the sender actually received Reply 0 before sending Probe 1, defeating the pre-emit-Probe-1 attack on `SinceLastRxNs`. - Extends `ReplyPacket` with a `Challenged` field encoded as the top bit of byte 212 (lower 7 bits remain the offset count 0–5). The flag is covered by the Ed25519 signature, so a MITM cannot flip it. - Backwards compatible in both directions: legacy senders never echo the nonce, so bit 7 stays 0 and the existing `NumOffsets ≤ 5` unmarshal check still passes; legacy senders also ignore `Reply0.SinceLastRxNs`, so populating it with a nonce is invisible to them. - Updates RFC16 to document the new wire-format semantics (a new "Challenge-Response Inbound Probing" subsection under Detailed Design, plus byte-table entries for `Sec`/`Frac`/`SinceLastRxNs`/`NumOffsets`). Folds in the wallet-bound-probing use case that was previously in Future Work. - **Agent-side only.** The follow-up PR adds a `--challenged` flag to `geoprobe-target-sender` to actually exercise the new flow; until then the reflector still answers ordinary unchallenged probes exactly as before. ## Diff Breakdown | Category | Files | Lines (+/-) | Net | |--------------|-------|-------------|------| | Core logic | 2 | +61 / -11 | +50 | | Tests | 3 | +328 / -27 | +301 | | Docs | 1 | +24 / -15 | +9 | | **Total** | 6 | +413 / -53 | +360 | Test-heavy: 73% of added lines are reflector / packet-codec tests covering the new behavior and the cohabiting bit-packed wire-format byte. <details> <summary>Key files (click to expand)</summary> - `tools/twamp/pkg/signed/reflector_linux.go` — adds `nonce` to per-sender state, generates a fresh nonce on each Probe 0, verifies the echoed nonce on Probe 1 (with `state.nonce != 0` guard so a `crypto/rand` failure doesn't spuriously authenticate), clears the nonce on pair completion / rate-limit reset / stale-pair reset so the documented "0 outside a pair" invariant holds unconditionally - `tools/twamp/pkg/signed/reflector_test.go` — adds tests for non-zero nonce in Reply 0, distinct nonces across pairs (gated by \`testing.Short\`), Reply 1 challenged-on-match, Reply 1 unchallenged-on-mismatch; updates pre-existing assertions that expected Reply 0 SinceLastRxNs to be 0 - `tools/twamp/pkg/signed/packet_test.go` — adds round-trip tests for \`Challenged=true\`, \`Challenged=false\`, and the combined case (challenged + non-zero offset count) so the bit-cohabits-with-count path is exercised - `tools/twamp/pkg/signed/packet.go` — adds \`Challenged bool\` to \`ReplyPacket\`, named \`numOffsetsChallengedBit\`/\`numOffsetsCountMask\` constants, encodes/decodes the flag as the top bit of byte 212, threads \`challenged bool\` through \`NewReplyPacket\` - `rfcs/rfc16-geolocation-verification.md` — new Challenge-Response Inbound Probing subsection (flow, why-unchallenged-is-permitted, backwards-compat, wallet-bound use case); byte-table entries updated for \`Sec\`/\`Frac\`/\`SinceLastRxNs\`/\`NumOffsets\`; resolves a stale "Reply 0 SinceLastRxNs is 0" note in the flow narrative; removes the obsolete Future Work entry that this PR implements </details> ## Testing Verification - \`go test -count=1 ./tools/twamp/pkg/signed/...\` green (20s); \`./controlplane/telemetry/cmd/geoprobe-agent/...\` green - \`golangci-lint run\` on both packages: 0 issues - New unit tests directly assert the wire-format byte (\`buf[212]&0x80\`, \`buf[212]&0x7F\`) so the bit-packing contract is tested at the byte level - End-to-end reflector tests (in-process UDP via the existing Linux harness) exercise both the challenged and unchallenged Probe 1 paths against a real reflector goroutine Related: [RFC16](rfcs/rfc16-geolocation-verification.md) — see the new "Challenge-Response Inbound Probing" subsection under Detailed Design. Related Stacked PR: #3738
601e4e4 to
88d6474
Compare
…e existing fast path
…o inbound probing
…overy Reuses the existing devnet setup: target-sender runs once unchallenged then once with --challenged, asserting the JSON output for each shows the expected challenged flag. runTargetSender is now synchronous and takes a log path + challenged flag so the two runs don't overlap on shared sender state. Adds a verifyInterval knob on geoprobeAgentOpts (set to 1s for this test) so the reflector's default 29s rate-limit window doesn't drop the second run's probes.
88d6474 to
9f29492
Compare
nikw9944
approved these changes
May 21, 2026
…lenged path probePairUnchallenged returns Reply 0 to the caller regardless of signature validity; the caller's .Verify() then surfaces sig failures as reply0_sig_valid=false in the per-pair log. probePairChallenged was passing verify=true on the same call, so a bad-sig Reply 0 was silently dropped and the operator saw a generic timeout instead. Switch to verify=false for Reply 0 to match. Reply 1 stays verify=true to match the unchallenged path's Reply 1 (both silently drop bad-sig Reply 1). A spoofed Reply 0 just produces a bogus nonce; the legitimate reflector won't recognize it, so Reply 1 either doesn't arrive or arrives without Challenged=true — the security model holds.
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
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 of Changes
--challengedflag togeoprobe-target-sender(defaultfalse). When set, the sender extracts the nonce fromReply0.SinceLastRxNs, writes it intoProbe1.Sec || Frac, signs Probe 1, and only then transmits — proving to the reflector (PR tools/twamp/signed: agent-side challenge-response inbound probing #3737) that it actually received Reply 0 before issuing Probe 1.--challenged=false) is byte-identical on the wire to the pre-PR2 sender. The existingProbePairbody is moved verbatim into a privateprobePairUnchallenged; a newprobePairChallengedadds the deferred-sign path; the exportedProbePairdispatches on a constructor flag.Challengedthrough the existing telemetry: startupslog.Info, per-pairslog.Debug, JSON output (with a"challenged": boolfield that is intentionally notomitemptyso downstream consumers always see the mode), and the human-readable text output.Reply1.SinceLastRxNsby the sender's compute time, in exchange for cryptographic proof of Reply 0 receipt. This is the documented RFC16 trade-off — TEE senders should stay unchallenged; bare-metal senders use--challenged.Stacking & rollout
Reply0.SinceLastRxNs == 0, the sender echoes 0 inSec/Frac, and the agent'sstate.nonce != 0guard keepsReply1.Challenged == false. Sender doesn't crash and the operator seeschallenged=falsein the per-pair log, indistinguishable from "agent hasn't been upgraded yet" — which is exactly the desired signal.Diff Breakdown (vs PR1 base branch)
Test-heavy. The core-logic diff is dominated by the new
probePairChallengedmethod (deferred-sign flow) and thenewChallengedProbePackethelper; everything else is plumbing (constructor signature, dispatcher, CLI flag, telemetry surfacing).Key files (click to expand)
tools/twamp/pkg/signed/sender_linux.go— renames existingProbePairbody toprobePairUnchallenged(verbatim, no logic change), addsprobePairChallengedusingsendAndRecvfor both probes, adds a 4-lineProbePairdispatcher that branches on the newchallengedfield ofLinuxSendertools/twamp/pkg/signed/sender_test.go— two new subtests insideTestSender_Linux:ProbePair challenged mode(assertsReply1.Challenged == trueand non-zero timing fields against a real reflector goroutine) andProbePair unchallenged mode is still unchallenged(asserts the byte-identical legacy path against the same reflector); 5 existing call sites updated withfalsetools/twamp/pkg/signed/packet.go— new unexportednewChallengedProbePacket(seq, signer, nonce)helper that takes the nonce as a parameter (does NOT generate one — that's the reflector's job in PR tools/twamp/signed: agent-side challenge-response inbound probing #3737), setsSec = uint32(nonce >> 32),Frac = uint32(nonce & 0xFFFFFFFF), and signs the 44-byte payloadcontrolplane/telemetry/cmd/geoprobe-target-sender/main_test.go—TestProbeOutput_ChallengedFieldRoundTrip(true case: round-trip plus raw"challenged":truebyte assertion) andTestProbeOutput_ChallengedFalseStillSerialized(false case: asserts"challenged":falseappears so an accidentalomitemptyregression fails fast)controlplane/telemetry/cmd/geoprobe-target-sender/main.go—--challengedflag declaration, threaded throughsigned.NewSender; newChallenged boolfield on theprobeOutputJSON struct (noomitempty); challenged status added to startup log, per-pairslog.Debug, JSON output, and text output ("Challenged Inbound:" line)tools/twamp/pkg/signed/sender.go—NewSenderconstructor gains trailingchallenged boolparametertools/twamp/pkg/signed/stub_fallback.go— non-LinuxNewLinuxSendersignature updated to matchTesting Verification
go test -count=1 ./tools/twamp/pkg/signed/...green (20s);./controlplane/telemetry/cmd/geoprobe-target-sender/...greengolangci-lint runon both packages: 0 issuesReply1.Challenged == true. The unchallenged-mode test exercises the same harness withchallenged=falseand assertsReply1.Challenged == false, confirming default behavior is preserved.Challenged=trueandChallenged=falseso the no-omitemptychoice is regression-protected.Related: RFC16 "Challenge-Response Inbound Probing" (added in PR #3737) — see the "Why unchallenged inbound is still permitted" paragraph for the latency trade-off rationale.