geolocation: route LocationOffsets to result destination in geoprobe-agent

[ben-dz]

Parent: #3430
Depends on: #3468

Summary

Update the geoprobe-agent to check a user's result destination when sending LocationOffsets, and route offsets there instead of the target IP when set.

The result destination is a single string field on GeolocationUser in host:port format (e.g., 185.199.108.1:9000 or results.example.com:9000). Empty string means not set. The field supports both IP addresses and domain names.

Target discovery changes

controlplane/telemetry/internal/geoprobe/target_discovery.go:

• TargetUpdate and ICMPTargetUpdate need to carry the result destination alongside each target
• When extracting targets from GeolocationUser, read the user's result_destination string
• If non-empty, override the "send-to" address for all of that user's outbound targets
• The ProbeAddress struct (or a wrapper) needs to distinguish the measure address (ICMP ping / TWAMP target) from the delivery address (where to send the LocationOffset)

Offset delivery changes

controlplane/telemetry/cmd/geoprobe-agent/main.go (sendCompositeOffsets):

• Currently builds targetAddr from addr.Host:addr.Port (the measurement target)
• When a delivery address is present, use that instead
• ICMP targets with no result destination: log a warning and skip sending (nothing is listening)

DNS resolution for domain destinations

When the result destination contains a domain name (not an IP), the agent needs to resolve it:

• Parse the host portion: try net.ParseIP first; if that fails, treat as domain
• Resolve domain via net.LookupHost (or net.ResolveUDPAddr)
• Cache the resolved IP with a TTL (~5 minutes) to avoid per-cycle DNS lookups
• If resolution fails, log a warning and skip sending for that cycle (don't block other targets)

Testing

• Unit tests for target discovery: user with result destination set → delivery address overrides target address
• Unit tests for target discovery: user without result destination → delivery address equals target address
• Unit tests: ICMP target without result destination → warning logged, offset not sent
• Unit tests: domain destination → DNS resolution used, result cached
• E2E test (if feasible): ICMP target with result destination receives offsets at the specified endpoint

[ben-dz]

Architecture review findings:

• Critical: 0
• High: 2
• Medium: 3
• Low: 3

Architecture Review

Critical

None

High

• No validation of ResultDestination format at discovery time: The ResultDestination string from onchain data is passed through unchecked until it reaches dnsCache.Resolve() inside sendCompositeOffsets. A malformed value (e.g., missing port, garbage string) will cause a warning log and skip that target every single measurement cycle, forever, until the onchain data changes. Validating at discovery time would surface the problem once (at target discovery) rather than on every send attempt.

  • Location: target_discovery.go:216 and target_discovery.go:241 (the if resultDest != "" { deliveryAddrs[addr] = resultDest } blocks)
  • Recommendation: Add a lightweight validation (e.g., net.SplitHostPort) when populating deliveryAddrs in discover(). Log a warning and omit the entry on failure, similar to how invalid IPs are handled. This keeps the hot path clean of repeated resolve-and-warn noise.

• sendCompositeOffsets parameter list is unwieldy (11 parameters): This function has accumulated too many positional parameters, making call sites hard to read and error-prone. Three new parameters were added in this change alone.

  • Location: main.go:928-940
  • Recommendation: Introduce a compositeOffsetSender struct that captures the stable dependencies (cache, signer, senderConn, getCurrentSlot, metrics, dnsCache) as fields, then expose a method like Send(ctx, log, rttData, deliveryAddrs, icmpTargets). This improves readability and testability.

Medium

• DNSCache has no eviction or size bound: The cache map grows indefinitely as new hostnames appear. While the realistic cardinality of ResultDestination hostnames is likely small, there is no mechanism to remove entries for domains that are no longer in use. Expired entries remain in the map forever and are only refreshed on the next lookup, but never reaped.

  • Location: dns_cache.go:13-18
  • Recommendation: Either add a periodic sweep that removes entries older than some multiple of the TTL, or add a maximum size with LRU eviction. Given the expected low cardinality, a simple periodic purge is sufficient.

• TOCTOU race in DNS cache on expired entries: Between the RLock check (line 59-61) and the write Lock (line 78), multiple goroutines with an expired entry can all proceed to call c.lookup(host) concurrently, performing redundant DNS lookups. This is a thundering-herd problem if many goroutines resolve the same expired domain simultaneously.

  • Location: dns_cache.go:59-85
  • Recommendation: Use singleflight.Group to deduplicate concurrent lookups for the same host, or upgrade to a full write lock for the resolve-and-store path. The performance impact is minor at current scale, but it is a correctness gap in the concurrency model.

• discoverAndSend split logic assumes outbound/ICMP ProbeAddresses never collide: The delivery address splitting in discoverAndSend (lines 105-117) classifies an address as outbound if it is in outboundSet, otherwise ICMP. If the same IP:port were somehow assigned as both outbound and outbound-ICMP (different TWAMPPort would make them distinct ProbeAddresses, but same IP:port:0 could theoretically appear), the outbound classification would silently win and the ICMP delivery entry would be lost.

  • Location: target_discovery.go:105-117
  • Recommendation: This is likely fine given that targetToProbeAddress and icmpTargetToProbeAddress produce different TWAMPPort values. Add a brief comment documenting this invariant so future maintainers do not need to trace it.

Low

• Repeated icmpSet construction: The icmpSet map is constructed in three separate places: the ICMP update handler in run() (line 842-844), the runCycle() method (lines 914-917), and conceptually in discoverAndSend. Each construction iterates the ICMP targets to build an identical set.

  • Location: main.go:842-844, main.go:914-917
  • Recommendation: Consider caching the icmpSet as a field on measurementLoop, updating it alongside ml.icmpTargets. This is minor but removes boilerplate.

• Debug log says "Sent composite offset" with delivery field always printed: When there is no delivery override and the offset goes to the measurement address itself, the "delivery" log field still prints the target address redundantly alongside "target". This is mildly noisy.

  • Location: main.go:1008-1009
  • Recommendation: Only include "delivery" when it differs from the target, or rename to clarify semantics (e.g., "dest_addr").

• deliveryAddrsEqual is fine but could be maps.Equal (Go 1.21+): Since the project uses Go 1.25, the standard library maps.Equal function is available and would replace this helper.

  • Location: target_discovery.go:311-320
  • Recommendation: Replace with maps.Equal(a, b) from maps package for less code to maintain.

Summary

The change is well-structured with clean separation between discovery (populating deliveryAddrs from onchain state) and delivery (resolving and routing in sendCompositeOffsets). The DNS cache is a sensible addition. The main structural concern is the growing parameter list on sendCompositeOffsets and the missing input validation on ResultDestination at discovery time, which would push error handling from the hot path to the configuration path.

[ben-dz]

Security review findings:

• Critical: 0
• High: 2
• Medium: 2
• Low: 2

Security Review

Critical

None.

High

• [SSRF / UDP Traffic Redirection via DNS Rebinding]: The ResultDestination field accepts domain names. When a domain is used, the DNS-resolved IP is used as the UDP send target but is never validated against the same ValidateScope() check that measurement target IPs go through. An attacker who controls a domain could initially resolve it to a public IP (passing the onchain validate_public_ip check, which only validates literal IPs), then later switch DNS to resolve to 127.0.0.1, 10.x.x.x, 169.254.169.254 (cloud metadata), or other internal addresses. The 5-minute DNS cache TTL provides the rebinding window. Measurement targets are protected by ProbeAddress.ValidateScope(), but delivery addresses bypass this entirely.

  • Location: controlplane/telemetry/cmd/geoprobe-agent/main.go:963-970 (the dnsCache.Resolve(deliveryDest) call in sendCompositeOffsets) and controlplane/telemetry/internal/geoprobe/dns_cache.go:76 (the resolved IP is returned with no scope check)
  • Recommendation: After DNS resolution in DNSCache.Resolve() (or at the call site in sendCompositeOffsets), validate that the resolved IP passes the same public-unicast scope check that ProbeAddress.ValidateScope() enforces. Reject and log if the resolved IP is loopback, private, link-local, or any other non-public address. This closes the DNS rebinding window.

• [Open UDP Reflector / Traffic Amplification]: A malicious user who registers onchain as an activated, paid GeolocationUser can set ResultDestination to any arbitrary public IP:port. The geoprobe-agent will then send signed LocationOffset UDP packets to that address on every measurement cycle. This turns the geoprobe-agent into a UDP reflector: the attacker controls the destination, and the agent sends unsolicited packets to a victim. There is no verification that the result destination actually belongs to or is authorized by the receiving party. Combined with multiple targets, an attacker could amplify traffic volume.

  • Location: controlplane/telemetry/internal/geoprobe/target_discovery.go:216-218 (delivery address blindly accepted from onchain data) and controlplane/telemetry/cmd/geoprobe-agent/main.go:963-970 (packets sent to user-controlled address)
  • Recommendation: Consider requiring some form of destination ownership verification (e.g., a challenge-response handshake, or requiring the destination IP to match the user's registered probe IP). At minimum, implement per-destination rate limiting to bound the amplification factor.

Medium

• [Unbounded DNS Cache Growth]: The DNSCache.entries map grows without bound. Each unique domain in ResultDestination adds an entry that is never evicted (expired entries are replaced on re-resolve, but domains that are no longer referenced remain in the map forever). A large number of users with distinct domain-based result destinations could cause unbounded memory growth over the agent's lifetime.

  • Location: controlplane/telemetry/internal/geoprobe/dns_cache.go:13-15 (entries map with no size limit or eviction)
  • Recommendation: Add a periodic sweep that removes entries not referenced in recent discovery cycles, or cap the map size with an LRU eviction policy.

• [No Port Range Validation on Delivery Address]: The DNSCache.Resolve() method parses the port via strconv.Atoi() and stores it in net.UDPAddr.Port (an int). It does not reject port 0 or ports above 65535. While Go's strconv.Atoi will parse any integer, and WriteToUDP may silently truncate or reject out-of-range ports depending on the OS, the lack of explicit validation means unexpected behavior is possible. The onchain validate_destination does enforce u16 parsing, but if the field is ever populated from a different source, this gap would matter.

  • Location: controlplane/telemetry/internal/geoprobe/dns_cache.go:46-49
  • Recommendation: Add an explicit check that port > 0 && port <= 65535 after parsing.

Low

• [Delivery Address Logged Without Sanitization]: The deliveryDest string (user-controlled onchain data) is logged in warning and debug messages. While structured logging (slog) mitigates injection into unstructured log parsers, if logs are consumed by systems that interpret special characters, this could be a minor log injection vector.

  • Location: controlplane/telemetry/cmd/geoprobe-agent/main.go:967 ("delivery", deliveryDest)
  • Recommendation: This is low risk given structured logging is used. No immediate action required, but note it for defense-in-depth if log sinks change.

• [TOCTOU in DNS Cache]: There is a time-of-check-to-time-of-use gap between the RLock cache check and the Lock for cache write. Under concurrent access, multiple goroutines could simultaneously miss the cache and all perform DNS lookups for the same host. This is a correctness/efficiency issue rather than a security issue, since the resolved IPs should be identical.

  • Location: controlplane/telemetry/internal/geoprobe/dns_cache.go:59-85
  • Recommendation: Acceptable for the current use case. Could use sync.Map or a singleflight pattern if concurrent resolution becomes a concern.

Summary

The most significant finding is that DNS-resolved delivery addresses are never validated for public-IP scope, creating a DNS rebinding vulnerability that could direct UDP traffic to internal/cloud-metadata endpoints. The onchain validate_destination only checks literal IPs at write time, not what the domain resolves to later. Additionally, the design allows any paid user to direct probe traffic to arbitrary third-party addresses, functioning as a UDP reflector. The DNS rebinding issue should be fixed before merge; the reflector concern is a design-level issue worth discussing.