[client] Handle UPnP routers that only support permanent leases (#5826)

client/internal/portforward/manager.go

@@ -6,6 +6,7 @@ import (
 	"context"
 	"fmt"
 	"net"
+	"regexp"
 	"sync"
 	"time"
 
@@ -15,19 +16,29 @@ import (
 
 const (
 	defaultMappingTTL  = 2 * time.Hour
-	renewalInterval    = defaultMappingTTL / 2
 	discoveryTimeout   = 10 * time.Second
 	mappingDescription = "NetBird"
 )
 
+// upnpErrPermanentLeaseOnly matches UPnP error 725 in SOAP fault XML,
+// allowing for whitespace/newlines between tags from different router firmware.
+var upnpErrPermanentLeaseOnly = regexp.MustCompile(`<errorCode>\s*725\s*</errorCode>`)
+
+// Mapping represents an active NAT port mapping.
 type Mapping struct {
 	Protocol     string
 	InternalPort uint16
 	ExternalPort uint16
 	ExternalIP   net.IP
 	NATType      string
+	// TTL is the lease duration. Zero means a permanent lease that never expires.
+	TTL time.Duration
 }
 
+// TODO: persist mapping state for crash recovery cleanup of permanent leases.
+// Currently not done because State.Cleanup requires NAT gateway re-discovery,
+// which blocks startup for ~10s when no gateway is present (affects all clients).
+
 type Manager struct {
 	cancel context.CancelFunc
 
@@ -43,6 +54,7 @@ type Manager struct {
 	mu sync.Mutex
 }
 
+// NewManager creates a new port forwarding manager.
 func NewManager() *Manager {
 	return &Manager{
 		stopCtx: make(chan context.Context, 1),
@@ -77,8 +89,7 @@ func (m *Manager) Start(ctx context.Context, wgPort uint16) {
 
 	gateway, mapping, err := m.setup(ctx)
 	if err != nil {
-		log.Errorf("failed to setup NAT port mapping: %v", err)
-
+		log.Infof("port forwarding setup: %v", err)
 		return
 	}
 
@@ -86,7 +97,7 @@ func (m *Manager) Start(ctx context.Context, wgPort uint16) {
 	m.mapping = mapping
 	m.mappingLock.Unlock()
 
-	m.renewLoop(ctx, gateway)
+	m.renewLoop(ctx, gateway, mapping.TTL)
 
 	select {
 	case cleanupCtx := <-m.stopCtx:
@@ -145,16 +156,14 @@ func (m *Manager) setup(ctx context.Context) (nat.NAT, *Mapping, error) {
 
 	gateway, err := nat.DiscoverGateway(discoverCtx)
 	if err != nil {
-		log.Infof("NAT gateway discovery failed: %v (port forwarding disabled)", err)
-		return nil, nil, err
+		return nil, nil, fmt.Errorf("discover gateway: %w", err)
 	}
 
 	log.Infof("discovered NAT gateway: %s", gateway.Type())
 
 	mapping, err := m.createMapping(ctx, gateway)
 	if err != nil {
-		log.Warnf("failed to create port mapping: %v", err)
-		return nil, nil, err
+		return nil, nil, fmt.Errorf("create port mapping: %w", err)
 	}
 	return gateway, mapping, nil
 }
@@ -163,9 +172,18 @@ func (m *Manager) createMapping(ctx context.Context, gateway nat.NAT) (*Mapping,
 	ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
 	defer cancel()
 
-	externalPort, err := gateway.AddPortMapping(ctx, "udp", int(m.wgPort), mappingDescription, defaultMappingTTL)
+	ttl := defaultMappingTTL
+	externalPort, err := gateway.AddPortMapping(ctx, "udp", int(m.wgPort), mappingDescription, ttl)
 	if err != nil {
-		return nil, err
+		if !isPermanentLeaseRequired(err) {
+			return nil, err
+		}
+		log.Infof("gateway only supports permanent leases, retrying with indefinite duration")
+		ttl = 0
+		externalPort, err = gateway.AddPortMapping(ctx, "udp", int(m.wgPort), mappingDescription, ttl)
+		if err != nil {
+			return nil, err
+		}
 	}
 
 	externalIP, err := gateway.GetExternalAddress()
@@ -180,6 +198,7 @@ func (m *Manager) createMapping(ctx context.Context, gateway nat.NAT) (*Mapping,
 		ExternalPort: uint16(externalPort),
 		ExternalIP:   externalIP,
 		NATType:      gateway.Type(),
+		TTL:          ttl,
 	}
 
 	log.Infof("created port mapping: %d -> %d via %s (external IP: %s)",
@@ -187,8 +206,14 @@ func (m *Manager) createMapping(ctx context.Context, gateway nat.NAT) (*Mapping,
 	return mapping, nil
 }
 
-func (m *Manager) renewLoop(ctx context.Context, gateway nat.NAT) {
-	ticker := time.NewTicker(renewalInterval)
+func (m *Manager) renewLoop(ctx context.Context, gateway nat.NAT, ttl time.Duration) {
+	if ttl == 0 {
+		// Permanent mappings don't expire, just wait for cancellation.
+		<-ctx.Done()
+		return
+	}
+
+	ticker := time.NewTicker(ttl / 2)
 	defer ticker.Stop()
 
 	for {
@@ -208,7 +233,7 @@ func (m *Manager) renewMapping(ctx context.Context, gateway nat.NAT) error {
 	ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
 	defer cancel()
 
-	externalPort, err := gateway.AddPortMapping(ctx, m.mapping.Protocol, int(m.mapping.InternalPort), mappingDescription, defaultMappingTTL)
+	externalPort, err := gateway.AddPortMapping(ctx, m.mapping.Protocol, int(m.mapping.InternalPort), mappingDescription, m.mapping.TTL)
 	if err != nil {
 		return fmt.Errorf("add port mapping: %w", err)
 	}
@@ -248,3 +273,8 @@ func (m *Manager) startTearDown(ctx context.Context) {
 	default:
 	}
 }
+
+// isPermanentLeaseRequired checks if a UPnP error indicates the gateway only supports permanent leases (error 725).
+func isPermanentLeaseRequired(err error) bool {
+	return err != nil && upnpErrPermanentLeaseOnly.MatchString(err.Error())
+}