Conversation
Replace v1-only message format with v2 supporting both IPv4 and IPv6 with CIDR prefix. Extract message construction into message.go, update IP resolution to use icanhazip.com with global IPv6 from interfaces as preferred source, and send packets to all resolved host addresses.
There was a problem hiding this comment.
Pull request overview
This PR upgrades the SPA client message format from v1 to v2 to support both IPv4 and IPv6 with a CIDR prefix, and adjusts client networking to resolve public addresses and send packets to all resolved server IPs.
Changes:
- Introduces
MessageV2message construction (v2 format) and adds unit tests for IPv4/IPv6 encoding. - Updates client IP resolution to prefer global IPv6 from interfaces and fall back to icanhazip.com, storing multiple source IPs (IPv4 + IPv6 CIDRs).
- Updates sending logic to resolve server hostnames to all IPs and transmit packets for all resolved client IPs.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| message.go | Adds v2 message builder supporting IPv4/IPv6 + prefix length. |
| message_test.go | Adds tests validating v2 message layout for IPv4/IPv6. |
| client.go | Switches client to multi-IP (IPNet) model, adds IP discovery and sends to all resolved host addresses. |
| client_test.go | Updates tests to use v2 messages and validate IPv4/IPv6 packet sending behavior. |
| go.mod | Updates the declared Go version. |
Comments suppressed due to low confidence (3)
message.go:22
- MessageV2 accepts any prefixLen without validation. This can produce invalid v2 messages (e.g., IPv4 with prefixLen > 32, IPv6 with prefixLen > 128) without returning an error. Consider validating prefixLen based on the detected IP family and returning a descriptive error for out-of-range values.
// MessageV2 builds a v2 message (34 or 46 bytes) for the given IP and CIDR prefix.
func MessageV2(ip net.IP, prefixLen uint8) ([]byte, error) {
ipv4 := ip.To4()
if ipv4 != nil {
return messageV2IPv4(ipv4, prefixLen)
}
ipv6 := ip.To16()
if ipv6 == nil {
return nil, fmt.Errorf("invalid IP address")
}
return messageV2IPv6(ipv6, prefixLen)
}
client.go:96
- fetchPublicIP() doesn’t check resp.StatusCode; non-200 responses (redirects, rate limits, HTML error pages) will be parsed as an IP and silently produce nil. It should validate a 200 OK response (and ideally a Content-Type / parse error) and return a meaningful error upstream.
func fetchPublicIP(url string) net.IP {
client := &http.Client{Timeout: 5 * time.Second}
resp, err := client.Get(url)
if err != nil {
return nil
}
defer resp.Body.Close()
body, err := io.ReadAll(resp.Body)
if err != nil {
return nil
}
return net.ParseIP(strings.TrimSpace(string(body)))
}
client.go:162
- send() now uses net.LookupHost without any timeout/cancellation. DNS resolution can block for a long time depending on system resolver configuration, which is a regression from the previous context-based timeout. Consider using a net.Resolver with LookupIPAddr/LookupHost and a context with timeout to bound latency.
func (c *Client) send(host string, port int) error {
addrs, err := net.LookupHost(host)
if err != nil {
return err
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (c *Client) resolvePublicIPs() { | ||
| if len(c.IPs) > 0 { | ||
| return | ||
| } | ||
| var v4 net.IP | ||
| var v6nets []net.IPNet | ||
| var wg sync.WaitGroup | ||
| wg.Go(func() { | ||
| v4 = fetchPublicIP("https://ipv4.icanhazip.com") | ||
| }) | ||
| wg.Go(func() { | ||
| v6nets = globalIPv6FromInterfaces() | ||
| if len(v6nets) == 0 { | ||
| if ip := fetchPublicIP("https://ipv6.icanhazip.com"); ip != nil { | ||
| v6nets = []net.IPNet{{IP: ip, Mask: net.CIDRMask(128, 128)}} | ||
| } | ||
| } | ||
| }) | ||
| wg.Wait() | ||
| if v4 != nil { | ||
| c.IPs = append(c.IPs, net.IPNet{IP: v4.To4(), Mask: net.CIDRMask(32, 32)}) | ||
| } | ||
| resolver := net.Resolver{ | ||
| PreferGo: true, | ||
| Dial: func(ctx context.Context, network, address string) (net.Conn, error) { | ||
| d := net.Dialer{} | ||
| return d.DialContext(ctx, "udp", "208.67.222.222:53") | ||
| }, | ||
| c.IPs = append(c.IPs, v6nets...) | ||
| } |
There was a problem hiding this comment.
resolvePublicIPs() swallows all resolution failures and leaves c.IPs empty; in that case sendAllPackets() will send nothing and still return nil, so Auth() can report success without transmitting any SPA packet. Consider returning an error when no public IPs were found and propagate it from NewClient/Auth (and/or have sendAllPackets error when len(c.IPs)==0).
WHY are these changes introduced?
Please fill me in.
WHAT is this pull request doing?
Replace v1-only message format with v2 supporting both IPv4 and IPv6
with CIDR prefix. Extract message construction into message.go, update
IP resolution to use icanhazip.com with global IPv6 from interfaces
as preferred source, and send packets to all resolved host addresses.
HOW was this pull request tested?
Specs and manually testing towards a server that runs v2 compatible sparoid server.