diff --git a/internal/portforward/service/mocks_generate_test.go b/internal/portforward/service/mocks_generate_test.go index 776a506df..c65d73f0d 100644 --- a/internal/portforward/service/mocks_generate_test.go +++ b/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 diff --git a/internal/portforward/service/mocks_test.go b/internal/portforward/service/mocks_test.go index 272ee2eaa..823c9cc14 100644 --- a/internal/portforward/service/mocks_test.go +++ b/internal/portforward/service/mocks_test.go @@ -1,15 +1,16 @@ // Code generated by MockGen. DO NOT EDIT. -// Source: github.com/qdm12/gluetun/internal/portforward/service (interfaces: Logger) +// Source: github.com/qdm12/gluetun/internal/portforward/service (interfaces: Logger,PortAllower) // // Generated by this command: // -// mockgen -destination=mocks_test.go -package=service . Logger +// mockgen -destination=internal/portforward/service/mocks_test.go -package=service github.com/qdm12/gluetun/internal/portforward/service Logger,PortAllower // // Package service is a generated GoMock package. package service import ( + context "context" reflect "reflect" gomock "go.uber.org/mock/gomock" @@ -86,3 +87,69 @@ func (mr *MockLoggerMockRecorder) Warn(s any) *gomock.Call { mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Warn", reflect.TypeOf((*MockLogger)(nil).Warn), s) } + +// MockPortAllower is a mock of PortAllower interface. +type MockPortAllower struct { + ctrl *gomock.Controller + recorder *MockPortAllowerMockRecorder + isgomock struct{} +} + +// MockPortAllowerMockRecorder is the mock recorder for MockPortAllower. +type MockPortAllowerMockRecorder struct { + mock *MockPortAllower +} + +// NewMockPortAllower creates a new mock instance. +func NewMockPortAllower(ctrl *gomock.Controller) *MockPortAllower { + mock := &MockPortAllower{ctrl: ctrl} + mock.recorder = &MockPortAllowerMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockPortAllower) EXPECT() *MockPortAllowerMockRecorder { + return m.recorder +} + +// RedirectPort mocks base method. +func (m *MockPortAllower) RedirectPort(ctx context.Context, intf string, sourcePort, destinationPort uint16) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "RedirectPort", ctx, intf, sourcePort, destinationPort) + ret0, _ := ret[0].(error) + return ret0 +} + +// RedirectPort indicates an expected call of RedirectPort. +func (mr *MockPortAllowerMockRecorder) RedirectPort(ctx, intf, sourcePort, destinationPort any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RedirectPort", reflect.TypeOf((*MockPortAllower)(nil).RedirectPort), ctx, intf, sourcePort, destinationPort) +} + +// RemoveAllowedPort mocks base method. +func (m *MockPortAllower) RemoveAllowedPort(ctx context.Context, port uint16) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "RemoveAllowedPort", ctx, port) + ret0, _ := ret[0].(error) + return ret0 +} + +// RemoveAllowedPort indicates an expected call of RemoveAllowedPort. +func (mr *MockPortAllowerMockRecorder) RemoveAllowedPort(ctx, port any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RemoveAllowedPort", reflect.TypeOf((*MockPortAllower)(nil).RemoveAllowedPort), ctx, port) +} + +// SetAllowedPort mocks base method. +func (m *MockPortAllower) SetAllowedPort(ctx context.Context, port uint16, intf string) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "SetAllowedPort", ctx, port, intf) + ret0, _ := ret[0].(error) + return ret0 +} + +// SetAllowedPort indicates an expected call of SetAllowedPort. +func (mr *MockPortAllowerMockRecorder) SetAllowedPort(ctx, port, intf any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetAllowedPort", reflect.TypeOf((*MockPortAllower)(nil).SetAllowedPort), ctx, port, intf) +} diff --git a/internal/portforward/service/ports_changed_test.go b/internal/portforward/service/ports_changed_test.go new file mode 100644 index 000000000..999c06091 --- /dev/null +++ b/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()) +} diff --git a/internal/portforward/service/start.go b/internal/portforward/service/start.go index 7ee6875a4..4344c67ae 100644 --- a/internal/portforward/service/start.go +++ b/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)) diff --git a/internal/provider/protonvpn/portforward.go b/internal/provider/protonvpn/portforward.go index 841055a7e..3bf07a335 100644 --- a/internal/provider/protonvpn/portforward.go +++ b/internal/provider/protonvpn/portforward.go @@ -6,6 +6,7 @@ import ( "fmt" "maps" "net/netip" + "slices" "strings" "time" @@ -13,6 +14,12 @@ import ( "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) } } diff --git a/internal/provider/protonvpn/portforward_test.go b/internal/provider/protonvpn/portforward_test.go new file mode 100644 index 000000000..8b87b5f62 --- /dev/null +++ b/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) + }) + } +} diff --git a/internal/provider/utils/portforward.go b/internal/provider/utils/portforward.go index 6f03646f8..6dd332588 100644 --- a/internal/provider/utils/portforward.go +++ b/internal/provider/utils/portforward.go @@ -1,6 +1,7 @@ package utils import ( + "context" "net/http" "net/netip" ) @@ -27,6 +28,15 @@ type PortForwardObjects struct { Password string // PortsCount is used by ProtonVPN for port forwarding. PortsCount uint16 + // OnPortsChanged is called by a KeepPortForward implementation when the VPN + // gateway assigns different external ports whilst refreshing an existing + // mapping. It updates the firewall, the port forwarded file and runs the up + // command for the new ports, so that a reassignment does not require tearing + // down and restarting the whole port forwarding service. + // It is nil when the port forwarding service does not support reassignment, + // in which case a KeepPortForward implementation must return an error rather + // than silently keep forwarding a port the client no longer listens on. + OnPortsChanged func(ctx context.Context, internalToExternalPorts map[uint16]uint16) error } type Routing interface {