tools/twamp/signed: agent-side challenge-response inbound probing#3737
Merged
Conversation
…choes the Reply 0 nonce
ben-dz
force-pushed
the
bdz/challenge-response-inbound-agent
branch
from
d825351 to
9fdc6c4
Compare
Contributor
|
Claude: The new tests mix
Functionally identical; stylistically inconsistent. Worth a pass to align on require.* for "must hold or test is meaningless" and assert.* for "we want to keep checking after a failure." |
nikw9944
approved these changes
May 21, 2026
- ProbePacket Sec/Frac doc comments mention the challenged Probe 1 nonce-echo semantic. - state.nonce=0 comment clarifies that lastTxTime is intentionally preserved (overwritten on the next Reply Tx). - New tests standardize on require.* for setup-critical checks (NoError, Len for buffers indexed below) and assert.* for behavior assertions, replacing four t.Fatalf bail-outs and several require.NotZero/Zero/True/False/Equal on round-trip outcomes.
ben-dz
added a commit
that referenced
this pull request
May 21, 2026
…#3738) > Stacked on #3737 (RFC16 + reflector). Merge the base PR first. ## Summary of Changes - Adds an opt-in `--challenged` flag to `geoprobe-target-sender` (default `false`). When set, the sender extracts the nonce from `Reply0.SinceLastRxNs`, writes it into `Probe1.Sec || Frac`, signs Probe 1, and only then transmits — proving to the reflector (PR #3737) that it actually received Reply 0 before issuing Probe 1. - Default behavior (`--challenged=false`) is **byte-identical on the wire** to the pre-PR2 sender. The existing `ProbePair` body is moved verbatim into a private `probePairUnchallenged`; a new `probePairChallenged` adds the deferred-sign path; the exported `ProbePair` dispatches on a constructor flag. - Threads `Challenged` through the existing telemetry: startup `slog.Info`, per-pair `slog.Debug`, JSON output (with a `"challenged": bool` field that is intentionally **not** `omitempty` so downstream consumers always see the mode), and the human-readable text output. - Trade-off baked into the new path: signing Probe 1 after Reply 0 receipt inflates `Reply1.SinceLastRxNs` by 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 - During the rollout window (new sender against an old reflector that doesn't issue nonces), `Reply0.SinceLastRxNs == 0`, the sender echoes 0 in `Sec/Frac`, and the agent's `state.nonce != 0` guard keeps `Reply1.Challenged == false`. Sender doesn't crash and the operator sees `challenged=false` in the per-pair log, indistinguishable from "agent hasn't been upgraded yet" — which is exactly the desired signal. ## Diff Breakdown (vs PR1 base branch) | Category | Files | Lines (+/-) | Net | |--------------|-------|-------------|------| | Core logic | 5 | +97 / -8 | +89 | | Tests | 2 | +122 / -6 | +116 | | **Total** | 7 | +218 / -13 | +205 | Test-heavy. The core-logic diff is dominated by the new `probePairChallenged` method (deferred-sign flow) and the `newChallengedProbePacket` helper; everything else is plumbing (constructor signature, dispatcher, CLI flag, telemetry surfacing). <details> <summary>Key files (click to expand)</summary> - `tools/twamp/pkg/signed/sender_linux.go` — renames existing `ProbePair` body to `probePairUnchallenged` (verbatim, no logic change), adds `probePairChallenged` using `sendAndRecv` for both probes, adds a 4-line `ProbePair` dispatcher that branches on the new `challenged` field of `LinuxSender` - `tools/twamp/pkg/signed/sender_test.go` — two new subtests inside `TestSender_Linux`: `ProbePair challenged mode` (asserts `Reply1.Challenged == true` and non-zero timing fields against a real reflector goroutine) and `ProbePair unchallenged mode is still unchallenged` (asserts the byte-identical legacy path against the same reflector); 5 existing call sites updated with `false` - `tools/twamp/pkg/signed/packet.go` — new unexported `newChallengedProbePacket(seq, signer, nonce)` helper that takes the nonce as a parameter (does NOT generate one — that's the reflector's job in PR #3737), sets `Sec = uint32(nonce >> 32)`, `Frac = uint32(nonce & 0xFFFFFFFF)`, and signs the 44-byte payload - `controlplane/telemetry/cmd/geoprobe-target-sender/main_test.go` — `TestProbeOutput_ChallengedFieldRoundTrip` (true case: round-trip plus raw `"challenged":true` byte assertion) and `TestProbeOutput_ChallengedFalseStillSerialized` (false case: asserts `"challenged":false` appears so an accidental `omitempty` regression fails fast) - `controlplane/telemetry/cmd/geoprobe-target-sender/main.go` — `--challenged` flag declaration, threaded through `signed.NewSender`; new `Challenged bool` field on the `probeOutput` JSON struct (no `omitempty`); challenged status added to startup log, per-pair `slog.Debug`, JSON output, and text output ("Challenged Inbound:" line) - `tools/twamp/pkg/signed/sender.go` — `NewSender` constructor gains trailing `challenged bool` parameter - `tools/twamp/pkg/signed/stub_fallback.go` — non-Linux `NewLinuxSender` signature updated to match </details> ## Testing Verification - `go test -count=1 ./tools/twamp/pkg/signed/...` green (20s); `./controlplane/telemetry/cmd/geoprobe-target-sender/...` green - `golangci-lint run` on both packages: 0 issues - Sender-side challenged-mode test exercises the full Probe 0 → Reply 0 → extract-nonce → sign-Probe-1 → Reply 1 round-trip against a real in-process Linux reflector goroutine, asserting `Reply1.Challenged == true`. The unchallenged-mode test exercises the same harness with `challenged=false` and asserts `Reply1.Challenged == false`, confirming default behavior is preserved. - JSON serialization is tested for both `Challenged=true` and `Challenged=false` so the no-`omitempty` choice is regression-protected. Related: [RFC16 "Challenge-Response Inbound Probing"](rfcs/rfc16-geolocation-verification.md) (added in PR #3737) — see the "Why unchallenged inbound is still permitted" paragraph for the latency trade-off rationale.
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
2 participants
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
Reply0.SinceLastRxNs(previously always 0). On Probe 1, if the sender echoed that nonce inSec || Fracand re-signed the packet, the reflector sets bit 7 ofReply1.NumOffsets(Challengedflag) — cryptographic proof the sender actually received Reply 0 before sending Probe 1, defeating the pre-emit-Probe-1 attack onSinceLastRxNs.ReplyPacketwith aChallengedfield 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.NumOffsets ≤ 5unmarshal check still passes; legacy senders also ignoreReply0.SinceLastRxNs, so populating it with a nonce is invisible to them.Sec/Frac/SinceLastRxNs/NumOffsets). Folds in the wallet-bound-probing use case that was previously in Future Work.--challengedflag togeoprobe-target-senderto actually exercise the new flow; until then the reflector still answers ordinary unchallenged probes exactly as before.Diff Breakdown
Test-heavy: 73% of added lines are reflector / packet-codec tests covering the new behavior and the cohabiting bit-packed wire-format byte.
Key files (click to expand)
tools/twamp/pkg/signed/reflector_linux.go— addsnonceto per-sender state, generates a fresh nonce on each Probe 0, verifies the echoed nonce on Probe 1 (withstate.nonce != 0guard so acrypto/randfailure 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 unconditionallytools/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 0tools/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 exercisedtools/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 implementsTesting Verification
Related: RFC16 — see the new "Challenge-Response Inbound Probing" subsection under Detailed Design.
Related Stacked PR: #3738