Fix: Empty connections for NordVPN and Windscribe

internal/dns/loop.go

@@ -104,7 +104,11 @@ func (l *looper) Run(ctx context.Context, wg *sync.WaitGroup, signalDNSReady fun
 		var unboundCancel context.CancelFunc = func() {}
 		waitError := make(chan error)
 
-		for ctx.Err() == nil && l.GetSettings().Enabled {
+		for l.GetSettings().Enabled {
+			if ctx.Err() != nil {
+				l.logger.Warn("context canceled: exiting loop")
+				return
+			}
 			var err error
 			unboundCancel, err = l.setupUnbound(ctx, crashed, waitError)
 			if err != nil {
@@ -113,6 +117,7 @@ func (l *looper) Run(ctx context.Context, wg *sync.WaitGroup, signalDNSReady fun
 					l.useUnencryptedDNS(fallback)
 				}
 				l.logAndWait(ctx, err)
+				continue
 			}
 			break
 		}
@@ -121,6 +126,8 @@ func (l *looper) Run(ctx context.Context, wg *sync.WaitGroup, signalDNSReady fun
 			l.useUnencryptedDNS(fallback)
 		}
 
+		signalDNSReady()
+
 		stayHere := true
 		for stayHere {
 			select {

internal/dns/nameserver.go

@@ -26,7 +26,7 @@ func (c *configurator) UseDNSInternally(ip net.IP) {
 func (c *configurator) UseDNSSystemWide(ip net.IP, keepNameserver bool) error {
 	c.logger.Info("using DNS address %s system wide", ip.String())
 	const filepath = string(constants.ResolvConf)
-	file, err := c.openFile(filepath, os.O_RDWR|os.O_TRUNC, 0644)
+	file, err := c.openFile(filepath, os.O_RDONLY, 0)
 	if err != nil {
 		return err
 	}
@@ -35,24 +35,29 @@ func (c *configurator) UseDNSSystemWide(ip net.IP, keepNameserver bool) error {
 		_ = file.Close()
 		return err
 	}
+	if err := file.Close(); err != nil {
+		return err
+	}
+
 	s := strings.TrimSuffix(string(data), "\n")
-	lines := strings.Split(s, "\n")
-	if len(lines) == 1 && lines[0] == "" {
-		lines = nil
+
+	lines := []string{
+		"nameserver " + ip.String(),
 	}
-	found := false
-	if !keepNameserver { // default
-		for i := range lines {
-			if strings.HasPrefix(lines[i], "nameserver ") {
-				lines[i] = "nameserver " + ip.String()
-				found = true
-			}
+	for _, line := range strings.Split(s, "\n") {
+		if line == "" ||
+			(!keepNameserver && strings.HasPrefix(line, "nameserver ")) {
+			continue
 		}
+		lines = append(lines, line)
 	}
-	if !found {
-		lines = append(lines, "nameserver "+ip.String())
-	}
+
 	s = strings.Join(lines, "\n") + "\n"
+
+	file, err = c.openFile(filepath, os.O_WRONLY|os.O_TRUNC, 0644)
+	if err != nil {
+		return err
+	}
 	_, err = file.WriteString(s)
 	if err != nil {
 		_ = file.Close()

internal/dns/nameserver_test.go

@@ -17,38 +17,69 @@ import (
 
 func Test_UseDNSSystemWide(t *testing.T) {
 	t.Parallel()
+
 	tests := map[string]struct {
-		data        []byte
-		writtenData string
-		openErr     error
-		readErr     error
-		writeErr    error
-		closeErr    error
-		err         error
+		ip             net.IP
+		keepNameserver bool
+		data           []byte
+		firstOpenErr   error
+		readErr        error
+		firstCloseErr  error
+		secondOpenErr  error
+		writtenData    string
+		writeErr       error
+		secondCloseErr error
+		err            error
 	}{
 		"no data": {
+			ip:          net.IP{127, 0, 0, 1},
 			writtenData: "nameserver 127.0.0.1\n",
 		},
-		"open error": {
-			openErr: fmt.Errorf("error"),
-			err:     fmt.Errorf("error"),
+		"first open error": {
+			ip:           net.IP{127, 0, 0, 1},
+			firstOpenErr: fmt.Errorf("error"),
+			err:          fmt.Errorf("error"),
 		},
 		"read error": {
 			readErr: fmt.Errorf("error"),
 			err:     fmt.Errorf("error"),
 		},
+		"first close error": {
+			firstCloseErr: fmt.Errorf("error"),
+			err:           fmt.Errorf("error"),
+		},
+		"second open error": {
+			ip:            net.IP{127, 0, 0, 1},
+			secondOpenErr: fmt.Errorf("error"),
+			err:           fmt.Errorf("error"),
+		},
 		"write error": {
+			ip:          net.IP{127, 0, 0, 1},
 			writtenData: "nameserver 127.0.0.1\n",
 			writeErr:    fmt.Errorf("error"),
 			err:         fmt.Errorf("error"),
 		},
+		"second close error": {
+			ip:             net.IP{127, 0, 0, 1},
+			writtenData:    "nameserver 127.0.0.1\n",
+			secondCloseErr: fmt.Errorf("error"),
+			err:            fmt.Errorf("error"),
+		},
 		"lines without nameserver": {
+			ip:          net.IP{127, 0, 0, 1},
 			data:        []byte("abc\ndef\n"),
-			writtenData: "abc\ndef\nnameserver 127.0.0.1\n",
+			writtenData: "nameserver 127.0.0.1\nabc\ndef\n",
 		},
 		"lines with nameserver": {
+			ip:          net.IP{127, 0, 0, 1},
 			data:        []byte("abc\nnameserver abc def\ndef\n"),
-			writtenData: "abc\nnameserver 127.0.0.1\ndef\n",
+			writtenData: "nameserver 127.0.0.1\nabc\ndef\n",
+		},
+		"keep nameserver": {
+			ip:             net.IP{127, 0, 0, 1},
+			keepNameserver: true,
+			data:           []byte("abc\nnameserver abc def\ndef\n"),
+			writtenData:    "nameserver 127.0.0.1\nabc\nnameserver abc def\ndef\n",
 		},
 	}
 	for name, tc := range tests {
@@ -57,9 +88,20 @@ func Test_UseDNSSystemWide(t *testing.T) {
 			t.Parallel()
 			mockCtrl := gomock.NewController(t)
 
-			file := mock_os.NewMockFile(mockCtrl)
-			if tc.openErr == nil {
-				firstReadCall := file.EXPECT().
+			type fileCall struct {
+				path string
+				flag int
+				perm os.FileMode
+				file os.File
+				err  error
+			}
+
+			var fileCalls []fileCall
+
+			readOnlyFile := mock_os.NewMockFile(mockCtrl)
+
+			if tc.firstOpenErr == nil {
+				firstReadCall := readOnlyFile.EXPECT().
 					Read(gomock.AssignableToTypeOf([]byte{})).
 					DoAndReturn(func(b []byte) (int, error) {
 						copy(b, tc.data)
@@ -69,32 +111,60 @@ func Test_UseDNSSystemWide(t *testing.T) {
 				if readErr == nil {
 					readErr = io.EOF
 				}
-				finalReadCall := file.EXPECT().
+				finalReadCall := readOnlyFile.EXPECT().
 					Read(gomock.AssignableToTypeOf([]byte{})).
 					Return(0, readErr).After(firstReadCall)
-				if tc.readErr == nil {
-					writeCall := file.EXPECT().WriteString(tc.writtenData).
-						Return(0, tc.writeErr).After(finalReadCall)
-					file.EXPECT().Close().Return(tc.closeErr).After(writeCall)
-				} else {
-					file.EXPECT().Close().Return(tc.closeErr).After(finalReadCall)
-				}
+				readOnlyFile.EXPECT().Close().
+					Return(tc.firstCloseErr).
+					After(finalReadCall)
 			}
 
+			fileCalls = append(fileCalls, fileCall{
+				path: string(constants.ResolvConf),
+				flag: os.O_RDONLY,
+				perm: 0,
+				file: readOnlyFile,
+				err:  tc.firstOpenErr,
+			}) // always return readOnlyFile
+
+			if tc.firstOpenErr == nil && tc.readErr == nil && tc.firstCloseErr == nil {
+				writeOnlyFile := mock_os.NewMockFile(mockCtrl)
+				if tc.secondOpenErr == nil {
+					writeCall := writeOnlyFile.EXPECT().
+						WriteString(tc.writtenData).
+						Return(0, tc.writeErr)
+					writeOnlyFile.EXPECT().
+						Close().
+						Return(tc.secondCloseErr).
+						After(writeCall)
+				}
+				fileCalls = append(fileCalls, fileCall{
+					path: string(constants.ResolvConf),
+					flag: os.O_WRONLY | os.O_TRUNC,
+					perm: os.FileMode(0644),
+					file: writeOnlyFile,
+					err:  tc.secondOpenErr,
+				})
+			}
+
+			fileCallIndex := 0
 			openFile := func(name string, flag int, perm os.FileMode) (os.File, error) {
-				assert.Equal(t, string(constants.ResolvConf), name)
-				assert.Equal(t, os.O_RDWR|os.O_TRUNC, flag)
-				assert.Equal(t, os.FileMode(0644), perm)
-				return file, tc.openErr
+				fileCall := fileCalls[fileCallIndex]
+				fileCallIndex++
+				assert.Equal(t, fileCall.path, name)
+				assert.Equal(t, fileCall.flag, flag)
+				assert.Equal(t, fileCall.perm, perm)
+				return fileCall.file, fileCall.err
 			}
 
 			logger := mock_logging.NewMockLogger(mockCtrl)
-			logger.EXPECT().Info("using DNS address %s system wide", "127.0.0.1")
+			logger.EXPECT().Info("using DNS address %s system wide", tc.ip.String())
+
 			c := &configurator{
 				openFile: openFile,
 				logger:   logger,
 			}
-			err := c.UseDNSSystemWide(net.IP{127, 0, 0, 1}, false)
+			err := c.UseDNSSystemWide(tc.ip, tc.keepNameserver)
 			if tc.err != nil {
 				require.Error(t, err)
 				assert.Equal(t, tc.err.Error(), err.Error())

internal/provider/nordvpn.go

@@ -71,8 +71,7 @@ func (n *nordvpn) GetOpenVPNConnection(selection models.ServerSelection) (
 
 	connections := make([]models.OpenVPNConnection, len(servers))
 	for i := range servers {
-		connection := models.OpenVPNConnection{IP: servers[i].IP, Port: port, Protocol: selection.Protocol}
-		connections = append(connections, connection)
+		connections[i] = models.OpenVPNConnection{IP: servers[i].IP, Port: port, Protocol: selection.Protocol}
 	}
 
 	return pickRandomConnection(connections, n.randSource), nil

internal/provider/windscribe.go

@@ -65,8 +65,8 @@ func (w *windscribe) GetOpenVPNConnection(selection models.ServerSelection) (con
 	}
 
 	connections := make([]models.OpenVPNConnection, len(servers))
-	for _, server := range servers {
-		connections = append(connections, models.OpenVPNConnection{IP: server.IP, Port: port, Protocol: selection.Protocol})
+	for i := range servers {
+		connections[i] = models.OpenVPNConnection{IP: servers[i].IP, Port: port, Protocol: selection.Protocol}
 	}
 
 	return pickRandomConnection(connections, w.randSource), nil