diff --git a/client/internal/portforward/manager.go b/client/internal/portforward/manager.go
index 019c2ad86..bf7533af9 100644
--- a/client/internal/portforward/manager.go
+++ b/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(`\s*725\s*`)
+
+// 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())
+}
diff --git a/client/internal/portforward/manager_js.go b/client/internal/portforward/manager_js.go
index d5db147f2..36c55063b 100644
--- a/client/internal/portforward/manager_js.go
+++ b/client/internal/portforward/manager_js.go
@@ -3,15 +3,18 @@ package portforward
import (
"context"
"net"
+ "time"
)
-// Mapping represents port mapping information.
+// 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
}
// Manager is a stub for js/wasm builds where NAT-PMP/UPnP is not supported.
diff --git a/client/internal/portforward/manager_test.go b/client/internal/portforward/manager_test.go
index 1029e87f5..1f66f9ccd 100644
--- a/client/internal/portforward/manager_test.go
+++ b/client/internal/portforward/manager_test.go
@@ -4,23 +4,25 @@ package portforward
import (
"context"
+ "fmt"
"net"
"testing"
"time"
- "github.com/libp2p/go-nat"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
type mockNAT struct {
- natType string
- deviceAddr net.IP
- externalAddr net.IP
- internalAddr net.IP
- mappings map[int]int
- addMappingErr error
- deleteMappingErr error
+ natType string
+ deviceAddr net.IP
+ externalAddr net.IP
+ internalAddr net.IP
+ mappings map[int]int
+ addMappingErr error
+ deleteMappingErr error
+ onlyPermanentLeases bool
+ lastTimeout time.Duration
}
func newMockNAT() *mockNAT {
@@ -53,8 +55,12 @@ func (m *mockNAT) AddPortMapping(ctx context.Context, protocol string, internalP
if m.addMappingErr != nil {
return 0, m.addMappingErr
}
+ if m.onlyPermanentLeases && timeout != 0 {
+ return 0, fmt.Errorf("SOAP fault. Code: | Explanation: | Detail: 725OnlyPermanentLeasesSupported")
+ }
externalPort := internalPort
m.mappings[internalPort] = externalPort
+ m.lastTimeout = timeout
return externalPort, nil
}
@@ -80,6 +86,7 @@ func TestManager_CreateMapping(t *testing.T) {
assert.Equal(t, uint16(51820), mapping.ExternalPort)
assert.Equal(t, "Mock-NAT", mapping.NATType)
assert.Equal(t, net.ParseIP("203.0.113.50").To4(), mapping.ExternalIP.To4())
+ assert.Equal(t, defaultMappingTTL, mapping.TTL)
}
func TestManager_GetMapping_ReturnsNilWhenNotReady(t *testing.T) {
@@ -131,29 +138,64 @@ func TestManager_Cleanup_NilMapping(t *testing.T) {
m.cleanup(context.Background(), gateway)
}
-func TestState_Cleanup(t *testing.T) {
- origDiscover := discoverGateway
- defer func() { discoverGateway = origDiscover }()
- mockGateway := newMockNAT()
- mockGateway.mappings[51820] = 51820
- discoverGateway = func(ctx context.Context) (nat.NAT, error) {
- return mockGateway, nil
- }
+func TestManager_CreateMapping_PermanentLeaseFallback(t *testing.T) {
+ m := NewManager()
+ m.wgPort = 51820
- state := &State{
- Protocol: "udp",
- InternalPort: 51820,
- }
+ gateway := newMockNAT()
+ gateway.onlyPermanentLeases = true
- err := state.Cleanup()
- assert.NoError(t, err)
+ mapping, err := m.createMapping(context.Background(), gateway)
+ require.NoError(t, err)
+ require.NotNil(t, mapping)
- _, exists := mockGateway.mappings[51820]
- assert.False(t, exists, "mapping should be deleted after cleanup")
+ assert.Equal(t, uint16(51820), mapping.InternalPort)
+ assert.Equal(t, time.Duration(0), mapping.TTL, "should return zero TTL for permanent lease")
+ assert.Equal(t, time.Duration(0), gateway.lastTimeout, "should have retried with zero duration")
}
-func TestState_Name(t *testing.T) {
- state := &State{}
- assert.Equal(t, "port_forward_state", state.Name())
+func TestIsPermanentLeaseRequired(t *testing.T) {
+ tests := []struct {
+ name string
+ err error
+ expected bool
+ }{
+ {
+ name: "nil error",
+ err: nil,
+ expected: false,
+ },
+ {
+ name: "UPnP error 725",
+ err: fmt.Errorf("SOAP fault. Code: | Detail: 725OnlyPermanentLeasesSupported"),
+ expected: true,
+ },
+ {
+ name: "wrapped error with 725",
+ err: fmt.Errorf("add port mapping: %w", fmt.Errorf("Detail: 725")),
+ expected: true,
+ },
+ {
+ name: "error 725 with newlines in XML",
+ err: fmt.Errorf("\n 725\n"),
+ expected: true,
+ },
+ {
+ name: "bare 725 without XML tag",
+ err: fmt.Errorf("error code 725"),
+ expected: false,
+ },
+ {
+ name: "unrelated error",
+ err: fmt.Errorf("connection refused"),
+ expected: false,
+ },
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ assert.Equal(t, tt.expected, isPermanentLeaseRequired(tt.err))
+ })
+ }
}
diff --git a/client/internal/portforward/state.go b/client/internal/portforward/state.go
deleted file mode 100644
index 3f939751a..000000000
--- a/client/internal/portforward/state.go
+++ /dev/null
@@ -1,50 +0,0 @@
-//go:build !js
-
-package portforward
-
-import (
- "context"
- "fmt"
-
- "github.com/libp2p/go-nat"
- log "github.com/sirupsen/logrus"
-)
-
-// discoverGateway is the function used for NAT gateway discovery.
-// It can be replaced in tests to avoid real network operations.
-var discoverGateway = nat.DiscoverGateway
-
-// State is persisted only for crash recovery cleanup
-type State struct {
- InternalPort uint16 `json:"internal_port,omitempty"`
- Protocol string `json:"protocol,omitempty"`
-}
-
-func (s *State) Name() string {
- return "port_forward_state"
-}
-
-// Cleanup implements statemanager.CleanableState for crash recovery
-func (s *State) Cleanup() error {
- if s.InternalPort == 0 {
- return nil
- }
-
- log.Infof("cleaning up stale port mapping for port %d", s.InternalPort)
-
- ctx, cancel := context.WithTimeout(context.Background(), discoveryTimeout)
- defer cancel()
-
- gateway, err := discoverGateway(ctx)
- if err != nil {
- // Discovery failure is not an error - gateway may not exist
- log.Debugf("cleanup: no gateway found: %v", err)
- return nil
- }
-
- if err := gateway.DeletePortMapping(ctx, s.Protocol, int(s.InternalPort)); err != nil {
- return fmt.Errorf("delete port mapping: %w", err)
- }
-
- return nil
-}
diff --git a/client/server/state_generic.go b/client/server/state_generic.go
index 3f794b611..86475ca42 100644
--- a/client/server/state_generic.go
+++ b/client/server/state_generic.go
@@ -10,10 +10,6 @@ import (
)
// registerStates registers all states that need crash recovery cleanup.
-// Note: portforward.State is intentionally NOT registered here to avoid blocking startup
-// for up to 10 seconds during NAT gateway discovery when no gateway is present.
-// The gateway reference cannot be persisted across restarts, so cleanup requires re-discovery.
-// Port forward cleanup is handled by the Manager during normal operation instead.
func registerStates(mgr *statemanager.Manager) {
mgr.RegisterState(&dns.ShutdownState{})
mgr.RegisterState(&systemops.ShutdownState{})
diff --git a/client/server/state_linux.go b/client/server/state_linux.go
index 655edfc53..b193d4dfa 100644
--- a/client/server/state_linux.go
+++ b/client/server/state_linux.go
@@ -12,10 +12,6 @@ import (
)
// registerStates registers all states that need crash recovery cleanup.
-// Note: portforward.State is intentionally NOT registered here to avoid blocking startup
-// for up to 10 seconds during NAT gateway discovery when no gateway is present.
-// The gateway reference cannot be persisted across restarts, so cleanup requires re-discovery.
-// Port forward cleanup is handled by the Manager during normal operation instead.
func registerStates(mgr *statemanager.Manager) {
mgr.RegisterState(&dns.ShutdownState{})
mgr.RegisterState(&systemops.ShutdownState{})