Add UPnP port forwarding for peer proxy sharing#391
Conversation
Enables "Share My Connection" by opening ports on the local router via UPnP IGD (v2 with v1 fallback). Random port selection with retry, lease renewal at 50% interval, and graceful cleanup on unmap. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Introduces a new portforward Go package to enable UPnP IGD-based TCP port forwarding, supporting the “Share My Connection” peer proxy feature by making a desktop peer reachable from the public internet.
Changes:
- Added
Forwarder/Mappingimplementation for UPnP IGDv2 with IGDv1 fallback, plus lease renewal and unmap support. - Implemented random external port selection within a configured range with retries.
- Added basic unit tests for
LocalIP, port selection, and no-op behaviors.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
portforward/portforward.go |
New UPnP IGD port mapping + renewal/unmapping implementation and LocalIP helper. |
portforward/portforward_test.go |
New unit tests covering LocalIP, randomPort range, and Forwarder no-op behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (f *Forwarder) stopRenewal() { | ||
| if f.stopC != nil { | ||
| close(f.stopC) | ||
| f.stopC = nil | ||
| } |
There was a problem hiding this comment.
stopRenewal closes stopC and then sets it to nil while the renewal goroutine reads f.stopC on each select. If UnmapPort runs while the goroutine is between iterations (or inside renewMapping), the goroutine may reach the next select after stopC has been set to nil, making the stop case permanently disabled and leaking the goroutine. Capture the stop channel in a local variable for the goroutine (and select on that), or switch to storing a context.CancelFunc for renewal so the goroutine only depends on its own derived context.
| switch { | ||
| case f.igd2Client != nil: | ||
| _ = f.igd2Client.AddPortMappingCtx(ctx, | ||
| "", m.ExternalPort, m.Protocol, | ||
| m.InternalPort, m.InternalIP, | ||
| true, "Lantern Peer Proxy", | ||
| uint32(m.LeaseDuration.Seconds()), | ||
| ) | ||
| case f.igd1Client != nil: | ||
| _ = f.igd1Client.AddPortMappingCtx(ctx, | ||
| "", m.ExternalPort, m.Protocol, | ||
| m.InternalPort, m.InternalIP, | ||
| true, "Lantern Peer Proxy", |
There was a problem hiding this comment.
renewMapping uses a hard-coded port mapping description ("Lantern Peer Proxy"), ignoring the description provided to MapPort. This will change the router rule label on renewal and prevents callers from customizing it. Consider storing the description in Mapping (or Forwarder) and reusing it when renewing.
| switch { | |
| case f.igd2Client != nil: | |
| _ = f.igd2Client.AddPortMappingCtx(ctx, | |
| "", m.ExternalPort, m.Protocol, | |
| m.InternalPort, m.InternalIP, | |
| true, "Lantern Peer Proxy", | |
| uint32(m.LeaseDuration.Seconds()), | |
| ) | |
| case f.igd1Client != nil: | |
| _ = f.igd1Client.AddPortMappingCtx(ctx, | |
| "", m.ExternalPort, m.Protocol, | |
| m.InternalPort, m.InternalIP, | |
| true, "Lantern Peer Proxy", | |
| description := m.Description | |
| if description == "" { | |
| description = "Lantern Peer Proxy" | |
| } | |
| switch { | |
| case f.igd2Client != nil: | |
| _ = f.igd2Client.AddPortMappingCtx(ctx, | |
| "", m.ExternalPort, m.Protocol, | |
| m.InternalPort, m.InternalIP, | |
| true, description, | |
| uint32(m.LeaseDuration.Seconds()), | |
| ) | |
| case f.igd1Client != nil: | |
| _ = f.igd1Client.AddPortMappingCtx(ctx, | |
| "", m.ExternalPort, m.Protocol, | |
| m.InternalPort, m.InternalIP, | |
| true, description, |
| // internal port on this machine. It tries UPnP IGDv2, then IGDv1. If the | ||
| // requested external port is taken, it retries with random ports. |
There was a problem hiding this comment.
The MapPort docstring mentions "requested external port" retries, but MapPort doesn't accept an external port parameter and always chooses random ports. Please update the comment to reflect the actual behavior (random external port selection with retries).
| // internal port on this machine. It tries UPnP IGDv2, then IGDv1. If the | |
| // requested external port is taken, it retries with random ports. | |
| // internal port on this machine. It tries UPnP IGDv2, then IGDv1, selecting a | |
| // random external port and retrying if the chosen port cannot be mapped. |
| client := clients[0] | ||
|
|
||
| for i := 0; i < maxRetries; i++ { | ||
| extPort := randomPort() | ||
| err = client.AddPortMappingCtx(ctx, | ||
| "", extPort, "TCP", | ||
| internalPort, localIP, | ||
| true, desc, leaseDuration, | ||
| ) | ||
| if err == nil { | ||
| f.igd2Client = client | ||
| return &Mapping{ | ||
| ExternalPort: extPort, | ||
| InternalPort: internalPort, | ||
| InternalIP: localIP, | ||
| Protocol: "TCP", | ||
| LeaseDuration: time.Duration(leaseDuration) * time.Second, | ||
| Method: "upnp-igd2", | ||
| }, nil | ||
| } | ||
| } | ||
| return nil, fmt.Errorf("IGDv2 failed after %d attempts: %w", maxRetries, err) |
There was a problem hiding this comment.
tryIGD2 only attempts port mapping on clients[0]. On networks with multiple discovered IGD devices/services, the first one may not be the correct or functional gateway. Consider iterating over all discovered clients and trying each until one succeeds (or aggregate errors).
| client := clients[0] | |
| for i := 0; i < maxRetries; i++ { | |
| extPort := randomPort() | |
| err = client.AddPortMappingCtx(ctx, | |
| "", extPort, "TCP", | |
| internalPort, localIP, | |
| true, desc, leaseDuration, | |
| ) | |
| if err == nil { | |
| f.igd2Client = client | |
| return &Mapping{ | |
| ExternalPort: extPort, | |
| InternalPort: internalPort, | |
| InternalIP: localIP, | |
| Protocol: "TCP", | |
| LeaseDuration: time.Duration(leaseDuration) * time.Second, | |
| Method: "upnp-igd2", | |
| }, nil | |
| } | |
| } | |
| return nil, fmt.Errorf("IGDv2 failed after %d attempts: %w", maxRetries, err) | |
| var lastErr error | |
| for _, client := range clients { | |
| for i := 0; i < maxRetries; i++ { | |
| extPort := randomPort() | |
| err = client.AddPortMappingCtx(ctx, | |
| "", extPort, "TCP", | |
| internalPort, localIP, | |
| true, desc, leaseDuration, | |
| ) | |
| if err == nil { | |
| f.igd2Client = client | |
| return &Mapping{ | |
| ExternalPort: extPort, | |
| InternalPort: internalPort, | |
| InternalIP: localIP, | |
| Protocol: "TCP", | |
| LeaseDuration: time.Duration(leaseDuration) * time.Second, | |
| Method: "upnp-igd2", | |
| }, nil | |
| } | |
| lastErr = err | |
| } | |
| } | |
| if lastErr == nil { | |
| lastErr = errors.New("no IGDv2 gateway accepted the port mapping") | |
| } | |
| return nil, fmt.Errorf("IGDv2 failed after %d attempts on %d clients: %w", maxRetries, len(clients), lastErr) |
| client := clients[0] | ||
|
|
||
| for i := 0; i < maxRetries; i++ { | ||
| extPort := randomPort() | ||
| err = client.AddPortMappingCtx(ctx, | ||
| "", extPort, "TCP", | ||
| internalPort, localIP, | ||
| true, desc, leaseDuration, | ||
| ) | ||
| if err == nil { | ||
| f.igd1Client = client | ||
| return &Mapping{ | ||
| ExternalPort: extPort, | ||
| InternalPort: internalPort, | ||
| InternalIP: localIP, | ||
| Protocol: "TCP", | ||
| LeaseDuration: time.Duration(leaseDuration) * time.Second, | ||
| Method: "upnp-igd1", | ||
| }, nil | ||
| } | ||
| } | ||
| return nil, fmt.Errorf("IGDv1 failed after %d attempts: %w", maxRetries, err) |
There was a problem hiding this comment.
tryIGD1 only attempts port mapping on clients[0]. If multiple IGDv1 services are discovered, choosing the first can cause false negatives. Iterate over all clients and attempt mapping until one works.
| client := clients[0] | |
| for i := 0; i < maxRetries; i++ { | |
| extPort := randomPort() | |
| err = client.AddPortMappingCtx(ctx, | |
| "", extPort, "TCP", | |
| internalPort, localIP, | |
| true, desc, leaseDuration, | |
| ) | |
| if err == nil { | |
| f.igd1Client = client | |
| return &Mapping{ | |
| ExternalPort: extPort, | |
| InternalPort: internalPort, | |
| InternalIP: localIP, | |
| Protocol: "TCP", | |
| LeaseDuration: time.Duration(leaseDuration) * time.Second, | |
| Method: "upnp-igd1", | |
| }, nil | |
| } | |
| } | |
| return nil, fmt.Errorf("IGDv1 failed after %d attempts: %w", maxRetries, err) | |
| var lastErr error | |
| for _, client := range clients { | |
| for i := 0; i < maxRetries; i++ { | |
| extPort := randomPort() | |
| if err := client.AddPortMappingCtx(ctx, | |
| "", extPort, "TCP", | |
| internalPort, localIP, | |
| true, desc, leaseDuration, | |
| ); err == nil { | |
| f.igd1Client = client | |
| return &Mapping{ | |
| ExternalPort: extPort, | |
| InternalPort: internalPort, | |
| InternalIP: localIP, | |
| Protocol: "TCP", | |
| LeaseDuration: time.Duration(leaseDuration) * time.Second, | |
| Method: "upnp-igd1", | |
| }, nil | |
| } else { | |
| lastErr = err | |
| } | |
| } | |
| } | |
| totalAttempts := maxRetries * len(clients) | |
| if lastErr != nil { | |
| return nil, fmt.Errorf("IGDv1 failed after %d attempts: %w", totalAttempts, lastErr) | |
| } | |
| return nil, fmt.Errorf("IGDv1 failed after %d attempts", totalAttempts) |
| func TestLocalIP(t *testing.T) { | ||
| ip, err := LocalIP() | ||
| require.NoError(t, err) | ||
| assert.NotEmpty(t, ip) | ||
| // Should be a private network IP | ||
| assert.Regexp(t, `^(10\.|172\.(1[6-9]|2\d|3[01])\.|192\.168\.)`, ip) | ||
| } |
There was a problem hiding this comment.
TestLocalIP depends on a real network environment and assumes the address is RFC1918. In CI or on some networks, LocalIP() may fail (no default route) or return CGNAT (100.64/10) / IPv6, making this unit test flaky. Consider refactoring LocalIP to be testable without real network access (e.g., interface enumeration), and update the assertion to parse the IP and check properties (like IsPrivate) rather than matching a fixed regex.
| func randomPort() uint16 { | ||
| n, _ := rand.Int(rand.Reader, big.NewInt(int64(portRangeMax-portRangeMin))) | ||
| return uint16(n.Int64()) + portRangeMin |
There was a problem hiding this comment.
randomPort ignores the error return from crypto/rand.Int, which can hide failures to read from the system CSPRNG. Also, the current range math never returns portRangeMax (it returns [portRangeMin, portRangeMax) ); if the intended range is inclusive as implied by the name/comment, adjust the math or rename portRangeMax to clarify it is exclusive.
| // LocalIP determines this machine's LAN IP by dialing a UDP socket to a | ||
| // well-known address. No actual traffic is sent. | ||
| func LocalIP() (string, error) { | ||
| conn, err := net.Dial("udp4", "8.8.8.8:53") | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| defer conn.Close() | ||
| return conn.LocalAddr().(*net.UDPAddr).IP.String(), nil | ||
| } |
There was a problem hiding this comment.
LocalIP relies on net.Dial("udp4", "8.8.8.8:53") to infer the outbound interface. This can fail in offline environments or restricted networks (no default route), and it hard-codes a public IP despite the comment saying no traffic is sent. Consider determining the LAN IP via net.Interfaces / InterfaceAddrs (selecting a non-loopback, up, private IPv4), which is more deterministic and testable.
| // LocalIP determines this machine's LAN IP by dialing a UDP socket to a | |
| // well-known address. No actual traffic is sent. | |
| func LocalIP() (string, error) { | |
| conn, err := net.Dial("udp4", "8.8.8.8:53") | |
| if err != nil { | |
| return "", err | |
| } | |
| defer conn.Close() | |
| return conn.LocalAddr().(*net.UDPAddr).IP.String(), nil | |
| } | |
| // LocalIP determines this machine's LAN IP by inspecting network interfaces | |
| // and selecting a non-loopback, up, private IPv4 address. | |
| func LocalIP() (string, error) { | |
| ifaces, err := net.Interfaces() | |
| if err != nil { | |
| return "", err | |
| } | |
| for _, iface := range ifaces { | |
| // Skip interfaces that are down or loopback. | |
| if iface.Flags&net.FlagUp == 0 || iface.Flags&net.FlagLoopback != 0 { | |
| continue | |
| } | |
| addrs, err := iface.Addrs() | |
| if err != nil { | |
| continue | |
| } | |
| for _, addr := range addrs { | |
| var ip net.IP | |
| switch v := addr.(type) { | |
| case *net.IPNet: | |
| ip = v.IP | |
| case *net.IPAddr: | |
| ip = v.IP | |
| default: | |
| continue | |
| } | |
| ip = ipToIPv4(ip) | |
| if ip == nil { | |
| continue | |
| } | |
| if isPrivateIPv4(ip) { | |
| return ip.String(), nil | |
| } | |
| } | |
| } | |
| return "", fmt.Errorf("no suitable private IPv4 address found") | |
| } | |
| // ipToIPv4 returns the IPv4 form of ip, or nil if ip is not an IPv4 address. | |
| func ipToIPv4(ip net.IP) net.IP { | |
| if ip == nil { | |
| return nil | |
| } | |
| if v4 := ip.To4(); v4 != nil { | |
| return v4 | |
| } | |
| return nil | |
| } | |
| // isPrivateIPv4 reports whether ip is in one of the RFC1918 private IPv4 ranges. | |
| func isPrivateIPv4(ip net.IP) bool { | |
| if ip == nil { | |
| return false | |
| } | |
| // 10.0.0.0/8 | |
| if ip[0] == 10 { | |
| return true | |
| } | |
| // 172.16.0.0/12: 172.16.0.0 - 172.31.255.255 | |
| if ip[0] == 172 && ip[1] >= 16 && ip[1] <= 31 { | |
| return true | |
| } | |
| // 192.168.0.0/16 | |
| if ip[0] == 192 && ip[1] == 168 { | |
| return true | |
| } | |
| return false | |
| } |
Summary
portforwardpackage with UPnP IGD support (v2 + v1 fallback) for opening ports on the user's routerDesign
Forwardertype manages a single active port mappingMapPort()discovers UPnP gateways and opens a TCP port forwarding ruleStartRenewal()refreshes the mapping at 50% of the lease intervalUnmapPort()cleans up the mapping and stops renewalLocalIP()helper detects the machine's LAN IPTest plan
go test ./portforward/ -v)upnp-igd1 mapped external:40224 → 192.168.4.29:12345🤖 Generated with Claude Code