fix(protonvpn): adopt reassigned forwarded ports instead of restarting port forwarding

internal/portforward/service/mocks_generate_test.go

@@ -1,3 +1,3 @@
 package service
 
-//go:generate mockgen -destination=mocks_test.go -package=$GOPACKAGE . Logger
+//go:generate mockgen -destination=mocks_test.go -package=$GOPACKAGE . Logger,PortAllower

internal/portforward/service/ports_changed_test.go

@@ -0,0 +1,108 @@
+package service
+
+import (
+	"context"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	gomock "go.uber.org/mock/gomock"
+)
+
+// Test_Service_onPortsChanged checks that adopting ports reassigned by the
+// gateway closes the firewall for the previous port before opening the new one,
+// and leaves the port forwarded file holding the new port. Leaving the previous
+// port allowed would keep a hole open for a port that is no longer forwarded.
+func Test_Service_onPortsChanged(t *testing.T) {
+	t.Parallel()
+
+	const (
+		vpnInterface = "tun0"
+		previousPort = uint16(37928)
+		newPort      = uint16(60557)
+	)
+
+	portFilepath := filepath.Join(t.TempDir(), "forwarded_port")
+
+	ctrl := gomock.NewController(t)
+	logger := NewMockLogger(ctrl)
+	logger.EXPECT().Info(gomock.Any()).AnyTimes()
+	logger.EXPECT().Debug(gomock.Any()).AnyTimes()
+
+	portAllower := NewMockPortAllower(ctrl)
+	gomock.InOrder(
+		// The previous port must be blocked before the new one is allowed.
+		portAllower.EXPECT().RemoveAllowedPort(gomock.Any(), previousPort).Return(nil),
+		portAllower.EXPECT().SetAllowedPort(gomock.Any(), newPort, vpnInterface).Return(nil),
+	)
+
+	service := &Service{
+		ports: []uint16{previousPort},
+		settings: Settings{
+			Filepath:       portFilepath,
+			Interface:      vpnInterface,
+			ListeningPorts: []uint16{0},
+		},
+		portAllower: portAllower,
+		logger:      logger,
+		puid:        os.Getuid(),
+		pgid:        os.Getgid(),
+	}
+
+	err := service.onPortsChanged(context.Background(), map[uint16]uint16{newPort: newPort})
+	require.NoError(t, err)
+
+	assert.Equal(t, []uint16{newPort}, service.GetPortsForwarded())
+
+	written, err := os.ReadFile(portFilepath)
+	require.NoError(t, err)
+	assert.Equal(t, "60557", string(written))
+}
+
+// Test_Service_onPortsChanged_keepsPreviousPortsOnFailure checks that a failure
+// to open the new port does not silently leave the service reporting ports it
+// is no longer forwarding.
+func Test_Service_onPortsChanged_keepsPreviousPortsOnFailure(t *testing.T) {
+	t.Parallel()
+
+	const (
+		vpnInterface = "tun0"
+		previousPort = uint16(37928)
+		newPort      = uint16(60557)
+	)
+
+	portFilepath := filepath.Join(t.TempDir(), "forwarded_port")
+
+	ctrl := gomock.NewController(t)
+	logger := NewMockLogger(ctrl)
+	logger.EXPECT().Info(gomock.Any()).AnyTimes()
+	logger.EXPECT().Debug(gomock.Any()).AnyTimes()
+
+	errAllow := assert.AnError
+	portAllower := NewMockPortAllower(ctrl)
+	portAllower.EXPECT().RemoveAllowedPort(gomock.Any(), previousPort).Return(nil)
+	portAllower.EXPECT().SetAllowedPort(gomock.Any(), newPort, vpnInterface).Return(errAllow)
+
+	service := &Service{
+		ports: []uint16{previousPort},
+		settings: Settings{
+			Filepath:       portFilepath,
+			Interface:      vpnInterface,
+			ListeningPorts: []uint16{0},
+		},
+		portAllower: portAllower,
+		logger:      logger,
+		puid:        os.Getuid(),
+		pgid:        os.Getgid(),
+	}
+
+	err := service.onPortsChanged(context.Background(), map[uint16]uint16{newPort: newPort})
+	require.Error(t, err)
+	assert.ErrorIs(t, err, errAllow)
+
+	// The previous port was already blocked, so nothing must still be reported
+	// as forwarded.
+	assert.Empty(t, service.GetPortsForwarded())
+}

internal/portforward/service/start.go

@@ -44,6 +44,7 @@ func (s *Service) Start(ctx context.Context) (runError <-chan error, err error)
 		Username:       s.settings.Username,
 		Password:       s.settings.Password,
 		PortsCount:     s.settings.PortsCount,
+		OnPortsChanged: s.onPortsChanged,
 	}
 	internalToExternalPorts, err := s.settings.PortForwarder.PortForward(ctx, obj)
 	if err != nil {
@@ -88,6 +89,30 @@ func (s *Service) Start(ctx context.Context) (runError <-chan error, err error)
 	return runErrorCh, nil
 }
 
+// onPortsChanged applies ports newly assigned by the VPN gateway whilst the
+// service is running, without restarting it. It is passed to the port forwarder
+// through [utils.PortForwardObjects] and is called from the KeepPortForward
+// goroutine, so it must not take the start-stop mutex: Stop holds that mutex
+// whilst waiting for the very same goroutine to return.
+func (s *Service) onPortsChanged(ctx context.Context, internalToExternalPorts map[uint16]uint16) (err error) {
+	s.portMutex.Lock()
+	defer s.portMutex.Unlock()
+
+	// Close the firewall for the previous ports before opening the new ones,
+	// so a reassignment cannot leave a stale port allowed.
+	err = s.cleanup()
+	if err != nil {
+		return fmt.Errorf("cleaning up previously forwarded ports: %w", err)
+	}
+
+	err = s.onNewPorts(ctx, internalToExternalPorts)
+	if err != nil {
+		return fmt.Errorf("handling newly assigned ports: %w", err)
+	}
+
+	return nil
+}
+
 func (s *Service) onNewPorts(ctx context.Context, internalToExternalPorts map[uint16]uint16) (err error) {
 	s.logger.Info(portPairsToString(internalToExternalPorts))
 

internal/provider/protonvpn/portforward.go

@@ -6,13 +6,20 @@ import (
 	"fmt"
 	"maps"
 	"net/netip"
+	"slices"
 	"strings"
 	"time"
 
 	"github.com/qdm12/gluetun/internal/natpmp"
 	"github.com/qdm12/gluetun/internal/provider/utils"
 )
 
+// ErrPortsReassigned is returned when the gateway reassigns external ports
+// whilst refreshing a mapping and the port forwarding service has no way to
+// apply them. Continuing would leave the client listening on a port that is
+// no longer forwarded, so the caller restarts port forwarding instead.
+var ErrPortsReassigned = errors.New("gateway reassigned external ports")
+
 // PortForward obtains a VPN server side port forwarded from ProtonVPN gateway.
 func (p *Provider) PortForward(ctx context.Context, objects utils.PortForwardObjects) (
 	internalToExternalPorts map[uint16]uint16, err error,
@@ -92,12 +99,13 @@ func addPortMappingTCPUDP(ctx context.Context, client *natpmp.Client, logger uti
 		}
 		protocolToExternalPort[protocol] = assignedExternalPort
 		checkLifetime(logger, protocolStr, lifetime, assignedLifetime)
+		// Note the external port is deliberately not validated against the
+		// requested one. The gateway is free to hand back a different external
+		// port, notably when refreshing an existing mapping, and the caller
+		// decides what to do with the port it actually received.
 		if internalPort != assignedInternalPort {
 			return 0, 0, fmt.Errorf("%s internal port requested as %d but received %d",
 				protocolStr, internalPort, assignedInternalPort)
-		} else if externalPort != 0 && externalPort != 1 && externalPort != assignedExternalPort {
-			return 0, 0, fmt.Errorf("%s external port requested as %d but received %d",
-				protocolStr, externalPort, assignedExternalPort)
 		}
 	}
 
@@ -109,6 +117,21 @@ func addPortMappingTCPUDP(ctx context.Context, client *natpmp.Client, logger uti
 	return assignedInternalPort, assignedExternalPort, nil
 }
 
+// portsMapToString describes an external port reassignment as a sorted list of
+// "was -> now" pairs, keyed by internal port so the output is deterministic.
+func portsMapToString(previous, current map[uint16]uint16) string {
+	internalPorts := slices.Sorted(maps.Keys(current))
+	pairs := make([]string, 0, len(internalPorts))
+	for _, internalPort := range internalPorts {
+		was, ok := previous[internalPort]
+		if !ok || was == current[internalPort] {
+			continue
+		}
+		pairs = append(pairs, fmt.Sprintf("%d -> %d", was, current[internalPort]))
+	}
+	return strings.Join(pairs, ", ")
+}
+
 func checkLifetime(logger utils.Logger, protocol string,
 	requested, actual time.Duration,
 ) {
@@ -135,15 +158,37 @@ func (p *Provider) KeepPortForward(ctx context.Context,
 
 		objects.Logger.Debug("refreshing forwarded ports since 45 seconds have elapsed")
 		const lifetime = 60 * time.Second
+		refreshedPorts := make(map[uint16]uint16, len(p.internalToExternalPorts))
+		portsChanged := false
 		for internalPort, externalPort := range p.internalToExternalPorts {
-			_, _, err := addPortMappingTCPUDP(ctx, client, logger, objects.Gateway, internalPort, externalPort, lifetime)
+			assignedInternalPort, assignedExternalPort, err := addPortMappingTCPUDP(ctx,
+				client, logger, objects.Gateway, internalPort, externalPort, lifetime)
 			if err != nil {
 				return fmt.Errorf("refreshing port mapping for internal port %d and external port %d: %w",
 					internalPort, externalPort, err)
 			}
+			refreshedPorts[assignedInternalPort] = assignedExternalPort
+			if assignedExternalPort != externalPort {
+				portsChanged = true
+				logger.Info(fmt.Sprintf("gateway reassigned external port %d to %d",
+					externalPort, assignedExternalPort))
+				continue
+			}
 			objects.Logger.Debug(fmt.Sprintf("port forwarded %d maintained", externalPort))
 		}
 
+		if portsChanged {
+			if objects.OnPortsChanged == nil {
+				return fmt.Errorf("%w: %s", ErrPortsReassigned,
+					portsMapToString(p.internalToExternalPorts, refreshedPorts))
+			}
+			p.internalToExternalPorts = refreshedPorts
+			err = objects.OnPortsChanged(ctx, maps.Clone(refreshedPorts))
+			if err != nil {
+				return fmt.Errorf("handling reassigned ports: %w", err)
+			}
+		}
+
 		timer.Reset(refreshTimeout)
 	}
 }

internal/provider/protonvpn/portforward_test.go

@@ -0,0 +1,53 @@
+package protonvpn
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func Test_portsMapToString(t *testing.T) {
+	t.Parallel()
+
+	testCases := map[string]struct {
+		previous map[uint16]uint16
+		current  map[uint16]uint16
+		s        string
+	}{
+		"no_change": {
+			previous: map[uint16]uint16{37928: 37928},
+			current:  map[uint16]uint16{37928: 37928},
+			s:        "",
+		},
+		"symmetric_port_reassigned": {
+			previous: map[uint16]uint16{37928: 37928},
+			current:  map[uint16]uint16{37928: 60557},
+			s:        "37928 -> 60557",
+		},
+		"multiple_ports_sorted_by_internal_port": {
+			previous: map[uint16]uint16{56789: 111, 37928: 222},
+			current:  map[uint16]uint16{56789: 333, 37928: 444},
+			s:        "222 -> 444, 111 -> 333",
+		},
+		"only_changed_ports_reported": {
+			previous: map[uint16]uint16{56789: 111, 37928: 222},
+			current:  map[uint16]uint16{56789: 111, 37928: 444},
+			s:        "222 -> 444",
+		},
+		"unknown_internal_port_skipped": {
+			previous: map[uint16]uint16{},
+			current:  map[uint16]uint16{37928: 60557},
+			s:        "",
+		},
+	}
+
+	for name, testCase := range testCases {
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+
+			s := portsMapToString(testCase.previous, testCase.current)
+
+			assert.Equal(t, testCase.s, s)
+		})
+	}
+}