From f6d553e4b90b9a09dcd618c50ee76fc9b8c580a5 Mon Sep 17 00:00:00 2001 From: UNITRONIX <36471318+UNITRONIX@users.noreply.github.com> Date: Thu, 7 May 2026 01:11:50 +0200 Subject: [PATCH] Keep same-NAT relay on public address When two peers register from the same public IP, SameNATRelay should force the public/configured relay address across every punch-hole and relay path. The previous logic still let isSameNetwork choose the server LAN relay in TCP RequestRelay, PunchHoleSent, and forwarded RelayResponse flows, which broke cloud-hosted servers for clients on the same Wi-Fi. Add regression coverage for shared-public-IP relay selection and preserve LAN relay behavior for private same-subnet peers. Refs: #121 Reported-by: @eternalliving --- betterdesk-server/signal/handler.go | 124 ++++++++++++----------- betterdesk-server/signal/handler_test.go | 79 +++++++++++++++ 2 files changed, 142 insertions(+), 61 deletions(-) diff --git a/betterdesk-server/signal/handler.go b/betterdesk-server/signal/handler.go index 27dbd03d..32148201 100644 --- a/betterdesk-server/signal/handler.go +++ b/betterdesk-server/signal/handler.go @@ -457,43 +457,25 @@ func (s *Server) handlePunchHoleRequest(msg *pb.PunchHoleRequest, raddr *net.UDP return } - relayServer := s.getRelayServer() - - // Early LAN detection for ForceRelay path (needs relay before the check). - if target.UDPAddr != nil && isSameNetwork(raddr, target.UDPAddr) { - relayServer = s.getLANRelayServer() + relayServer, sameNetwork, hairpin := s.selectPeerRelayServer(s.getRelayServer(), raddr, target.UDPAddr) + if sameNetwork { + log.Printf("[signal] LAN detected: %s and %s on same network, relay=%s", raddr.IP, target.UDPAddr.IP, relayServer) + } + if hairpin { + log.Printf("[signal] PunchHole: shared public IP %s detected (issue #121 hairpin) → forcing relay for %s", + raddr.IP, targetID) } log.Printf("[signal] PunchHole: target %s found (addr=%s, status=%s, lastReg=%v ago), relay=%s", targetID, target.UDPAddr, target.StatusTier, time.Since(target.LastReg), relayServer) // If force relay or always use relay - if msg.ForceRelay || s.cfg.AlwaysUseRelay { + if msg.ForceRelay || s.cfg.AlwaysUseRelay || hairpin { log.Printf("[signal] PunchHole: force relay for %s", targetID) s.sendRelayResponse(target, raddr, msg, relayServer) return } - // Issue #121: NAT hairpin fallback. When both peers connect from the - // same public IP, they sit behind the same NAT gateway. Many consumer - // routers (and most cellular gateways) drop hairpin packets, so the - // usual LAN-address exchange times out. Force the relay path instead - // — it always works. Disable with SAME_NAT_RELAY=N. - if s.cfg.SameNATRelay && target.UDPAddr != nil && isSamePublicIP(raddr, target.UDPAddr) { - log.Printf("[signal] PunchHole: shared public IP %s detected (issue #121 hairpin) → forcing relay for %s", - raddr.IP, targetID) - s.sendRelayResponse(target, raddr, msg, relayServer) - return - } - - // LAN detection: if both peers share the same public IP or are on the same - // private /24 subnet, they are on the same local network (matching Rust hbbs). - sameNetwork := isSameNetwork(raddr, target.UDPAddr) - if sameNetwork { - relayServer = s.getLANRelayServer() - log.Printf("[signal] LAN detected: %s and %s on same network, relay=%s", raddr.IP, target.UDPAddr.IP, relayServer) - } - // Send PunchHole to the TARGET peer (tell it the initiator's address) punchHole := &pb.RendezvousMessage{ Union: &pb.RendezvousMessage_PunchHole{ @@ -609,11 +591,13 @@ func (s *Server) handlePunchHoleRequestTCP(msg *pb.PunchHoleRequest, raddr *net. } } - relayServer := s.getRelayServer() - - // Early LAN detection (needed before ForceRelay check). - if target.UDPAddr != nil && isSameNetwork(raddr, target.UDPAddr) { - relayServer = s.getLANRelayServer() + relayServer, sameNetwork, hairpin := s.selectPeerRelayServer(s.getRelayServer(), raddr, target.UDPAddr) + if sameNetwork { + log.Printf("[signal] LAN detected (TCP): %s and %s on same network, relay=%s", raddr.IP, target.UDPAddr.IP, relayServer) + } + if hairpin { + log.Printf("[signal] PunchHole (TCP): shared public IP %s detected (issue #121 hairpin) → forcing relay for %s", + raddr.IP, targetID) } log.Printf("[signal] PunchHole (TCP): target %s found (addr=%s, status=%s), relay=%s", @@ -634,16 +618,6 @@ func (s *Server) handlePunchHoleRequestTCP(msg *pb.PunchHoleRequest, raddr *net. // PunchHoleResponse), generate their own UUID, and connect to relay with it // — while the target connects with the server's UUID. This broke relay // pairing every time (Issue #66). - // Issue #121: NAT hairpin fallback (see UDP handler for full rationale). - // When both peers connect from the same public IP, force relay because - // most consumer routers drop hairpin packets and the LAN exchange will - // silently time out. Disable with SAME_NAT_RELAY=N. - hairpin := s.cfg.SameNATRelay && target.UDPAddr != nil && isSamePublicIP(raddr, target.UDPAddr) - if hairpin { - log.Printf("[signal] PunchHole (TCP): shared public IP %s detected (issue #121 hairpin) → forcing relay for %s", - raddr.IP, targetID) - } - if msg.ForceRelay || s.cfg.AlwaysUseRelay || hairpin { log.Printf("[signal] PunchHole (TCP): force relay for %s (returning SYMMETRIC to let client drive relay UUID)", targetID) @@ -691,14 +665,6 @@ func (s *Server) handlePunchHoleRequestTCP(msg *pb.PunchHoleRequest, raddr *net. s.sendToPeer(targetID, punchHole) log.Printf("[signal] PunchHole (TCP): forwarded to target %s (connType=%s)", targetID, target.ConnType) - // LAN detection: if both peers share the same public IP or are on the same - // private /24 subnet, they are on the same local network. - sameNetwork := isSameNetwork(raddr, target.UDPAddr) - if sameNetwork { - relayServer = s.getLANRelayServer() - log.Printf("[signal] LAN detected (TCP): %s and %s on same network, relay=%s", raddr.IP, target.UDPAddr.IP, relayServer) - } - // Sign the target's PK with server's Ed25519 key for E2E verification. var signedPk []byte if len(target.PK) > 0 { @@ -802,13 +768,19 @@ func (s *Server) handlePunchHoleSent(phs *pb.PunchHoleSent, senderAddr *net.UDPA // Build PunchHoleResponse for the initiator. // socket_addr = target's (sender's) address, pk = SIGNED target's public key. - // LAN detection: set is_local when sender and initiator are on the same network. + // LAN detection: set is_local only for genuine LAN cases. Shared public IP + // peers keep the public relay to avoid NAT hairpin failures (#121). relayServer := phs.RelayServer - sameNetwork := isSameNetwork(senderAddr, initiatorAddr) + if relayServer == "" { + relayServer = s.getRelayServer() + } + relayServer, sameNetwork, hairpin := s.selectPeerRelayServer(relayServer, senderAddr, initiatorAddr) if sameNetwork { - relayServer = s.getLANRelayServer() log.Printf("[signal] PunchHoleSent LAN detected: %s and %s on same network, relay=%s", senderAddr.IP, initiatorAddr.IP, relayServer) } + if hairpin { + log.Printf("[signal] PunchHoleSent shared public IP %s detected (issue #121 hairpin) → keeping public relay=%s", senderAddr.IP, relayServer) + } phr := &pb.PunchHoleResponse{ SocketAddr: crypto.EncodeAddr(senderAddr), @@ -904,11 +876,15 @@ func (s *Server) handleRequestRelay(msg *pb.RequestRelay, raddr *net.UDPAddr) { return } - // LAN detection: use server's LAN IP for relay when both peers are on same network. - if target.UDPAddr != nil && isSameNetwork(raddr, target.UDPAddr) { - relayServer = s.getLANRelayServer() + // LAN detection: use server's LAN IP only for genuine LAN cases. Shared + // public IP peers keep the public relay to avoid NAT hairpin failures (#121). + relayServer, sameNetwork, hairpin := s.selectPeerRelayServer(relayServer, raddr, target.UDPAddr) + if sameNetwork { log.Printf("[signal] RequestRelay LAN detected: %s and %s on same network, relay=%s", raddr.IP, target.UDPAddr.IP, relayServer) } + if hairpin { + log.Printf("[signal] RequestRelay shared public IP %s detected (issue #121 hairpin) → keeping public relay=%s", raddr.IP, relayServer) + } // Forward relay request to target peer (supports UDP, TCP, and WebSocket targets). // NOTE: Must use RequestRelay type, not RelayResponse — RustDesk client's @@ -1006,11 +982,15 @@ func (s *Server) handleRequestRelayTCP(msg *pb.RequestRelay, raddr *net.UDPAddr) } } - // LAN detection: use server's LAN IP for relay when both peers are on same network. + // LAN detection: use server's LAN IP only for genuine LAN cases. Shared + // public IP peers keep the public relay to avoid NAT hairpin failures (#121). // Only applicable when target has a known UDP address for comparison. - if target.UDPAddr != nil && isSameNetwork(raddr, target.UDPAddr) { - relayServer = s.getLANRelayServer() + var sameNetwork, hairpin bool + relayServer, sameNetwork, hairpin = s.selectPeerRelayServer(relayServer, raddr, target.UDPAddr) + if sameNetwork { log.Printf("[signal] RequestRelay (TCP) LAN detected: %s and %s on same network, relay=%s", raddr.IP, target.UDPAddr.IP, relayServer) + } else if hairpin { + log.Printf("[signal] RequestRelay (TCP) shared public IP %s detected (issue #121 hairpin) → keeping public relay=%s", raddr.IP, relayServer) } else { // Debug: log why LAN detection failed if target.UDPAddr == nil { @@ -1145,10 +1125,15 @@ func (s *Server) handleRelayResponseForward(msg *pb.RendezvousMessage, senderAdd rr.SocketAddr = nil rr.SocketAddrV6 = nil - // LAN detection: use LAN relay when both peers are on same network. + // LAN detection: use LAN relay only for genuine LAN cases. Shared public IP + // peers keep the public relay to avoid NAT hairpin failures (#121). relayServer := s.getRelayServer() - if senderAddr != nil && isSameNetwork(senderAddr, initiatorAddr) { - relayServer = s.getLANRelayServer() + relayServer, sameNetwork, hairpin := s.selectPeerRelayServer(relayServer, senderAddr, initiatorAddr) + if sameNetwork { + log.Printf("[signal] RelayResponse LAN detected: %s and %s on same network, relay=%s", senderAddr.IP, initiatorAddr.IP, relayServer) + } + if hairpin && senderAddr != nil { + log.Printf("[signal] RelayResponse shared public IP %s detected (issue #121 hairpin) → keeping public relay=%s", senderAddr.IP, relayServer) } rr.RelayServer = relayServer @@ -1395,6 +1380,23 @@ func (s *Server) getLANRelayServer() string { return s.getRelayServer() } +func (s *Server) selectPeerRelayServer(defaultRelay string, a, b *net.UDPAddr) (relay string, sameLAN bool, samePublicIP bool) { + if defaultRelay == "" { + defaultRelay = s.getRelayServer() + } + if a == nil || b == nil { + return defaultRelay, false, false + } + + if s.cfg.SameNATRelay && isSamePublicIP(a, b) { + return s.getRelayServer(), false, true + } + if isSameNetwork(a, b) { + return s.getLANRelayServer(), true, false + } + return defaultRelay, false, false +} + // registerPkResponse is a helper to create a RegisterPkResponse message. func registerPkResponse(result pb.RegisterPkResponse_Result) *pb.RendezvousMessage { return &pb.RendezvousMessage{ diff --git a/betterdesk-server/signal/handler_test.go b/betterdesk-server/signal/handler_test.go index db2dfe2e..51ded113 100644 --- a/betterdesk-server/signal/handler_test.go +++ b/betterdesk-server/signal/handler_test.go @@ -2,11 +2,14 @@ package signal import ( "bytes" + "net" "path/filepath" "testing" + "time" "github.com/unitronix/betterdesk-server/config" "github.com/unitronix/betterdesk-server/db" + "github.com/unitronix/betterdesk-server/peer" pb "github.com/unitronix/betterdesk-server/proto" ) @@ -42,6 +45,10 @@ func newRegisterPk(peerID string) *pb.RegisterPk { } } +func udpAddr(ip string, port int) *net.UDPAddr { + return &net.UDPAddr{IP: net.ParseIP(ip), Port: port} +} + func TestProcessRegisterPkManagedRejectsUnknownPeer(t *testing.T) { srv, database := newTestSignalServer(t, config.EnrollmentModeManaged) @@ -130,3 +137,75 @@ func TestProcessRegisterPkManagedAllowsTokenBoundPeer(t *testing.T) { t.Fatalf("token-bound peer was not persisted with PK: %+v", peer) } } + +func TestSelectPeerRelayServerKeepsPublicRelayForSharedPublicIP(t *testing.T) { + srv, _ := newTestSignalServer(t, config.EnrollmentModeOpen) + srv.localIP.Store("198.51.100.20") + srv.lanIP.Store("10.0.0.20") + + relay, sameLAN, samePublic := srv.selectPeerRelayServer( + "10.0.0.20:21117", + udpAddr("203.0.113.44", 51000), + udpAddr("203.0.113.44", 52000), + ) + + if relay != "198.51.100.20:21117" { + t.Fatalf("relay = %q, want public relay", relay) + } + if sameLAN { + t.Fatal("shared public IP must not be treated as LAN when SameNATRelay is enabled") + } + if !samePublic { + t.Fatal("shared public IP hairpin flag was not set") + } +} + +func TestSelectPeerRelayServerUsesLANRelayForPrivateSubnet(t *testing.T) { + srv, _ := newTestSignalServer(t, config.EnrollmentModeOpen) + srv.localIP.Store("198.51.100.20") + srv.lanIP.Store("10.0.0.20") + + relay, sameLAN, samePublic := srv.selectPeerRelayServer( + "198.51.100.20:21117", + udpAddr("192.168.1.10", 51000), + udpAddr("192.168.1.42", 52000), + ) + + if relay != "10.0.0.20:21117" { + t.Fatalf("relay = %q, want LAN relay", relay) + } + if !sameLAN { + t.Fatal("private same-subnet peers should use LAN relay") + } + if samePublic { + t.Fatal("private same-subnet peers should not be marked as shared public IP") + } +} + +func TestHandleRequestRelayTCPSamePublicIPIgnoresPrivateRelayHint(t *testing.T) { + srv, _ := newTestSignalServer(t, config.EnrollmentModeOpen) + srv.localIP.Store("198.51.100.20") + srv.lanIP.Store("10.0.0.20") + + srv.peers.Put(&peer.Entry{ + ID: "TARGET121", + UDPAddr: udpAddr("203.0.113.44", 52000), + ConnType: peer.ConnTCP, + LastReg: time.Now(), + StatusTier: peer.StatusOnline, + }) + + resp := srv.handleRequestRelayTCP(&pb.RequestRelay{ + Id: "TARGET121", + Uuid: "issue-121-relay-uuid", + RelayServer: "10.0.0.20:21117", + }, udpAddr("203.0.113.44", 51000)) + + rr := resp.GetRelayResponse() + if rr == nil { + t.Fatalf("expected RelayResponse, got %+v", resp) + } + if rr.RelayServer != "198.51.100.20:21117" { + t.Fatalf("relay = %q, want public relay", rr.RelayServer) + } +}