Skip to content

geoprobe-target: replayed offsets still reach location_offsets (no MeasurementSlot check) #4286

Description

@nikw9944

Follow-up from #4271, which closed the forgery hole in the geoprobe measurement chain. That PR added RFC-16's MeasurementSlot replay check to geoprobe-agent only. geoprobe-target — the process that writes the location_offsets table lake's public geolocation explorer aggregates — has no slot comparison at all.

What is still open

1. No replay bound on the target's ingest path

controlplane/telemetry/cmd/geoprobe-target/main.go handleOffset now drops offsets whose signature chain does not verify, which is the important half. But a signature stays valid forever, so a validly signed offset captured off the wire and replayed passes the gate unchanged and is written to ClickHouse. git grep MeasurementSlot in that file returns only display code.

This is the same attack RFC-16's MeasurementSlot field exists to stop, against the surface that feeds the public page. It is narrower than the forgery hole #4271 closed — it needs an on-path capture rather than just a UDP socket — which is why it was deliberately left out of that PR rather than growing it.

The agent's implementation is the reference to follow: offsetSlotFresh, slotReference and the maxOffsetSlotLag / maxOffsetSlotLead / maxSlotReferenceAge constants in controlplane/telemetry/cmd/geoprobe-agent/main.go. Note that geoprobe-target does not currently hold a ledger RPC client, so wiring a slot source into it is the non-trivial part of this change and should be scoped before implementing. Fail closed on an unusable slot, as the agent does.

One sizing note carried over from review of that PR: the agent sets maxSlotReferenceAge = 2 * geoprobe.SlotCacheTTL (10m) while maxOffsetSlotLead is 5m. Because a legitimate offset's slot is at most the true slot, a reference staler than the lead allowance already rejects fresh offsets while still admitting replays near the frozen slot — so the two bounds interact, and a target-side implementation should not copy the 2x multiplier without deciding it is what it wants.

2. MinCache.Update still resets best's clock on an equal RTT

controlplane/telemetry/internal/geoprobe/mincache.go:120 reads if rttNs <= c.best.rttNs and, on that branch, replaces best — which also resets its receivedAt. This is the identical defect #4271 fixed in the agent's own offsetCache.Put, where the comment now reads: "An equal-RTT offset must not replace best, because replacing also resets best's receivedAt clock — a replayed offset would otherwise hold best forever."

MinCache was not touched by that PR. In geoprobe-target the cache drives console and JSON output rather than anything persisted, so the impact is lower than it was in the agent — but the code is wrong in the same way and should match its sibling.

3. signature_valid is now a constant true

Small cleanup in the same function. Since handleOffset returns early on a failed chain, verifyError is always nil by the time the ClickHouse row is built, so the sigErrStr computation is dead and signature_valid is written true unconditionally.

Two things worth deciding rather than just deleting:

  • With -verify-signatures=false (the flag defaults to true, and infra's ansible sets verify_signatures: true, so this is not the deployed configuration), nothing is verified but rows are still written with signature_valid = true. That is a mislabel, and it matters more now that the column is supposed to carry a real assertion. Recording false or a distinct "unverified" state would be more honest than asserting a check that did not run.
  • Historical rows written before geoprobe: reject forged, replayed and spoofed geolocation input #4271 include signature_valid = false, and lake's geolocation handlers do not filter on the column — grep signature_valid across lake's api/ finds it only in test fixtures. So forged offsets already in mainnet-beta.location_offsets keep rendering on the public explorer even though the intake is now closed. Closing that is lake-side work, tracked separately; it is noted here so whoever picks this up knows the column's consumer does not currently read it.

Context

All of the above was found while reviewing #4271 (implemented by instance 413). None of it is a regression from that PR — items 1 and 2 predate it, and item 3 is a consequence of its fix. Read that PR's diff for the shape the fixes should follow.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions