fix: find jetbrains on ipv6 when port collides

code-asher · code-asher · commit f17ecd582f47 · 2024-01-19T18:12:08.000-09:00
This commit was mostly just to add a test for ipv6, but in so doing I
discovered it was possible to miss a JetBrains process listening on ipv6
if there was something else non-JetBrains on ipv4 with the same port.
This is fixed now, so we should never miss processes (aside from
errors).

On the flip side, while it was already possible to get false positives
if JetBrains was on ipv4 and something else was forwarded on ipv6, it is
now possible to get them the other way around too.  I am not sure it is
worth trying to resolve these false positives as producing them requires
a setup that seems quite unlikely and the impact is low.
diff --git a/agent/agent_test.go b/agent/agent_test.go
@@ -199,72 +199,125 @@ func TestAgent_Stats_Magic(t *testing.T) {
 		if runtime.GOOS != "linux" {
 			t.Skip("JetBrains tracking is only supported on Linux")
 		}
+		tests := []struct {
+			name    string
+			network string
+			address string
+		}{
+			{"IPV4", "tcp4", "127.0.0.1"},
+			{"IPV4Localhost", "tcp4", "localhost"},
+			{"IPV6", "tcp6", "[::1]"},
+		}
+		for _, test := range tests {
+			test := test
+			t.Run(test.name, func(t *testing.T) {
+				t.Parallel()
+				verifyJetBrainsTracking(t, test.network, test.address)
+			})
+		}
+	})
+}
 
-		ctx := testutil.Context(t, testutil.WaitLong)
+func spawnEchoServer(t *testing.T, args ...string) (string, *exec.Cmd) {
+	_, b, _, ok := runtime.Caller(0)
+	require.True(t, ok)
+	dir := filepath.Join(filepath.Dir(b), "../scripts/echoserver/main.go")
+	//nolint:gosec
+	echoServerCmd := exec.Command("go", append([]string{"run", dir}, args...)...)
 
-		// JetBrains tracking works by looking at the process name listening on the
-		// forwarded port.  If the process's command line includes the magic string
-		// we are looking for, then we assume it is a JetBrains editor.  So when we
-		// connect to the port we must ensure the process includes that magic string
-		// to fool the agent into thinking this is JetBrains.  To do this we need to
-		// spawn an external process (in this case a simple echo server) so we can
-		// control the process name.  The -D here is just to mimic how Java options
-		// are set but is not necessary as the agent looks only for the magic
-		// string itself anywhere in the command.
-		_, b, _, ok := runtime.Caller(0)
-		require.True(t, ok)
-		dir := filepath.Join(filepath.Dir(b), "../scripts/echoserver/main.go")
-		echoServerCmd := exec.Command("go", "run", dir,
-			"-D", agentssh.MagicProcessCmdlineJetBrains)
-		stdout, err := echoServerCmd.StdoutPipe()
-		require.NoError(t, err)
-		err = echoServerCmd.Start()
-		require.NoError(t, err)
-		defer echoServerCmd.Process.Kill()
+	stderr, err := echoServerCmd.StderrPipe()
+	require.NoError(t, err)
+	go func() {
+		sc := bufio.NewScanner(stderr)
+		for sc.Scan() {
+			t.Logf("echo server stderr: %s", sc.Text())
+		}
+		t.Logf("echo server exited")
+	}()
 
-		// The echo server prints its port as the first line.
-		sc := bufio.NewScanner(stdout)
-		sc.Scan()
-		remotePort := sc.Text()
+	stdout, err := echoServerCmd.StdoutPipe()
+	require.NoError(t, err)
+	err = echoServerCmd.Start()
+	require.NoError(t, err)
+	t.Cleanup(func() {
+		_ = stderr.Close()
+		_ = echoServerCmd.Process.Kill()
+	})
 
-		//nolint:dogsled
-		conn, _, stats, _, _ := setupAgent(t, agentsdk.Manifest{}, 0)
-		sshClient, err := conn.SSHClient(ctx)
-		require.NoError(t, err)
+	// The echo server prints its port as the first line.
+	sc := bufio.NewScanner(stdout)
+	sc.Scan()
+	remotePort := sc.Text()
 
-		tunneledConn, err := sshClient.Dial("tcp", fmt.Sprintf("127.0.0.1:%s", remotePort))
-		require.NoError(t, err)
-		t.Cleanup(func() {
-			// always close on failure of test
-			_ = conn.Close()
-			_ = tunneledConn.Close()
-		})
+	return remotePort, echoServerCmd
+}
 
-		require.Eventuallyf(t, func() bool {
-			s, ok := <-stats
-			t.Logf("got stats with conn open: ok=%t, ConnectionCount=%d, SessionCountJetBrains=%d",
-				ok, s.ConnectionCount, s.SessionCountJetBrains)
-			return ok && s.ConnectionCount > 0 &&
-				s.SessionCountJetBrains == 1
-		}, testutil.WaitLong, testutil.IntervalFast,
-			"never saw stats with conn open",
-		)
+func verifyJetBrainsTracking(t *testing.T, network, address string) {
+	ctx := testutil.Context(t, testutil.WaitLong)
 
-		// Kill the server and connection after checking for the echo.
-		requireEcho(t, tunneledConn)
-		_ = echoServerCmd.Process.Kill()
-		_ = tunneledConn.Close()
+	//nolint:dogsled
+	conn, _, stats, _, _ := setupAgent(t, agentsdk.Manifest{}, 0)
+	sshClient, err := conn.SSHClient(ctx)
+	require.NoError(t, err)
 
-		require.Eventuallyf(t, func() bool {
-			s, ok := <-stats
-			t.Logf("got stats after disconnect %t, %d",
-				ok, s.SessionCountJetBrains)
-			return ok &&
-				s.SessionCountJetBrains == 0
-		}, testutil.WaitLong, testutil.IntervalFast,
-			"never saw stats after conn closes",
-		)
+	// On Linux it is permissible to listen on the same port on different
+	// addresses, and when this happens `localhost` can route to the ipv6 loopback
+	// address instead of the ipv4 address.  Since we make requests to coderd with
+	// `localhost` this behavior can cause those requests to go to the ipv6 echo
+	// server instead of coderd when there is a port collision.  Listening on ipv4
+	// here occupies the port, preventing coderd from using it and running into
+	// this issue.
+	port := "0"
+	if network == "tcp6" {
+		port, _ = spawnEchoServer(t, "tcp4", "0")
+	}
+
+	// JetBrains tracking works by looking at the process name listening on
+	// the forwarded port.  If the process's command line includes the magic
+	// string we are looking for, then we assume it is a JetBrains editor.
+	// So when we connect to the port we must ensure the process includes
+	// that magic string to fool the agent into thinking this is JetBrains.
+	// To do this we need to spawn an external process (in this case a
+	// simple echo server) so we can control the process name.  The -D here
+	// is just to mimic how Java options are set but is not necessary as the
+	// agent looks only for the magic string itself anywhere in the command.
+	gotPort, echoServerCmd := spawnEchoServer(t, network, port, "-D", agentssh.MagicProcessCmdlineJetBrains)
+	if port != "0" {
+		require.Equal(t, gotPort, port) // Sanity check.
+	}
+
+	tunneledConn, err := sshClient.Dial(network, fmt.Sprintf("%s:%s", address, gotPort))
+	require.NoError(t, err)
+	t.Cleanup(func() {
+		// always close on failure of test
+		_ = conn.Close()
+		_ = tunneledConn.Close()
 	})
+
+	require.Eventuallyf(t, func() bool {
+		s, ok := <-stats
+		t.Logf("got stats with conn open: ok=%t, ConnectionCount=%d, SessionCountJetBrains=%d",
+			ok, s.ConnectionCount, s.SessionCountJetBrains)
+		return ok && s.ConnectionCount > 0 &&
+			s.SessionCountJetBrains == 1
+	}, testutil.WaitLong, testutil.IntervalFast,
+		"never saw stats with conn open",
+	)
+
+	// Kill the server and connection after checking for the echo.
+	requireEcho(t, tunneledConn)
+	_ = echoServerCmd.Process.Kill()
+	_ = tunneledConn.Close()
+
+	require.Eventuallyf(t, func() bool {
+		s, ok := <-stats
+		t.Logf("got stats after disconnect %t, %d",
+			ok, s.SessionCountJetBrains)
+		return ok &&
+			s.SessionCountJetBrains == 0
+	}, testutil.WaitLong, testutil.IntervalFast,
+		"never saw stats after conn closes",
+	)
 }
 
 func TestAgent_SessionExec(t *testing.T) {
diff --git a/agent/agentssh/jetbrainstrack.go b/agent/agentssh/jetbrainstrack.go
@@ -10,6 +10,7 @@ import (
 	gossh "golang.org/x/crypto/ssh"
 
 	"cdr.dev/slog"
+	"github.com/coder/coder/v2/coderd/util/slice"
 )
 
 // localForwardChannelData is copied from the ssh package.
@@ -38,30 +39,51 @@ func NewJetbrainsChannelWatcher(ctx ssh.Context, logger slog.Logger, newChannel
 		return newChannel
 	}
 
-	// If we do get a port, we should be able to get the matching PID and from
-	// there look up the invocation.
-	cmdline, err := getListeningPortProcessCmdline(d.DestPort)
+	// If we do get a port, we should be able to get the matching PID(s) and from
+	// there look up the invocation(s).  For now, ignore the address because we
+	// would need to resolve the address (for example `localhost` has a number of
+	// possibilities, or ::1 might route to ::, and so on).  The consequence is
+	// that if a user has another forwarded process listening on a different
+	// address but the same port as an active JetBrains process then the count
+	// will be inflated by however many processes are doing that.
+	cmdlines, err := getListeningPortProcessCmdlines(d.DestPort)
+
+	// If any of these are JetBrains processes, wrap the channel in a watcher so
+	// we can increment and decrement the session count when the channel
+	// opens/closes.  We attempt to match on something that appears unique to
+	// JetBrains software.  As mentioned above, this can give false positives
+	// since we are not checking the address of each process.
+	if slice.ContainsCompare(cmdlines, strings.ToLower(MagicProcessCmdlineJetBrains),
+		func(magic, cmdline string) bool {
+			return strings.Contains(strings.ToLower(cmdline), magic)
+		}) {
+		logger.Debug(ctx, "discovered forwarded JetBrains process",
+			slog.F("destination_address", d.DestAddr),
+			slog.F("destination_port", d.DestPort))
+
+		return &JetbrainsChannelWatcher{
+			NewChannel:       newChannel,
+			jetbrainsCounter: counter,
+			logger: logger.With(
+				slog.F("destination_address", d.DestAddr),
+				slog.F("destination_port", d.DestPort),
+			),
+		}
+	}
+
+	// We do not want to break the forwarding if we were unable to inspect the
+	// port, so only log any errors.  The consequence of failing port inspection
+	// is that the JetBrains session count might be lower than it should be.
 	if err != nil {
 		logger.Warn(ctx, "failed to inspect port",
+			slog.F("destination_addr", d.DestAddr),
 			slog.F("destination_port", d.DestPort),
 			slog.Error(err))
-		return newChannel
 	}
 
-	// If this is not JetBrains, then we do not need to do anything special.  We
-	// attempt to match on something that appears unique to JetBrains software.
-	if !strings.Contains(strings.ToLower(cmdline), strings.ToLower(MagicProcessCmdlineJetBrains)) {
-		return newChannel
-	}
-
-	logger.Debug(ctx, "discovered forwarded JetBrains process",
-		slog.F("destination_port", d.DestPort))
-
-	return &JetbrainsChannelWatcher{
-		NewChannel:       newChannel,
-		jetbrainsCounter: counter,
-		logger:           logger.With(slog.F("destination_port", d.DestPort)),
-	}
+	// Either not a JetBrains process or we failed to figure it out.  Do nothing;
+	// just return the channel as-is.
+	return newChannel
 }
 
 func (w *JetbrainsChannelWatcher) Accept() (gossh.Channel, <-chan *gossh.Request, error) {
diff --git a/agent/agentssh/portinspection_supported.go b/agent/agentssh/portinspection_supported.go
@@ -11,41 +11,42 @@ import (
 	"golang.org/x/xerrors"
 )
 
-func getListeningPortProcessCmdline(port uint32) (string, error) {
+/**
+ * getListeningPortProcessCmdlines looks up the full command line for all
+ * processes listening on the specified port regardless of their address.  It
+ * returns all the commands it was able to find and any errors encountered while
+ * doing the search.
+ */
+func getListeningPortProcessCmdlines(port uint32) ([]string, error) {
 	acceptFn := func(s *netstat.SockTabEntry) bool {
 		return s.LocalAddr != nil && uint32(s.LocalAddr.Port) == port
 	}
+
 	tabs4, err4 := netstat.TCPSocks(acceptFn)
 	tabs6, err6 := netstat.TCP6Socks(acceptFn)
 
-	// In the common case, we want to check ipv4 listening addresses.  If this
-	// fails, we should return an error.  We also need to check ipv6.  The
-	// assumption is, if we have an err4, and 0 ipv6 addresses listed, then we are
-	// interested in the err4 (and vice versa).  So return both errors (at least 1
-	// is non-nil) if the other list is empty.
-	if (err4 != nil && len(tabs6) == 0) || (err6 != nil && len(tabs4) == 0) {
-		return "", xerrors.Errorf("inspect port %d: %w", port, errors.Join(err4, err6))
-	}
+	allErrs := errors.Join(err4, err6)
 
-	var proc *netstat.Process
-	if len(tabs4) > 0 {
-		proc = tabs4[0].Process
-	} else if len(tabs6) > 0 {
-		proc = tabs6[0].Process
-	}
-	if proc == nil {
-		// Either nothing is listening on this port or we were unable to read the
-		// process details (permission issues reading /proc/$pid/* potentially).
-		// Or, perhaps /proc/net/tcp{,6} is not listing the port for some reason.
-		return "", nil
+	var cmdlines []string
+	for _, tab := range append(tabs4, tabs6...) {
+		// If the process is nil then perhaps we were unable to read the process
+		// details (permission issues reading /proc/$pid/* maybe).
+		if tab.Process == nil {
+			allErrs = errors.Join(allErrs, xerrors.Errorf("read process on port %d", port))
+			continue
+		}
+		// The process name provided by go-netstat does not include the full command
+		// line so grab that instead.
+		data, err := os.ReadFile(fmt.Sprintf("/proc/%d/cmdline", tab.Process.Pid))
+		if err != nil {
+			allErrs = errors.Join(allErrs, xerrors.Errorf("read /proc/%d/cmdline: %w", tab.Process.Pid, err))
+			continue
+		}
+		cmdlines = append(cmdlines, string(data))
 	}
 
-	// The process name provided by go-netstat does not include the full command
-	// line so grab that instead.
-	pid := proc.Pid
-	data, err := os.ReadFile(fmt.Sprintf("/proc/%d/cmdline", pid))
-	if err != nil {
-		return "", xerrors.Errorf("read /proc/%d/cmdline: %w", pid, err)
-	}
-	return string(data), nil
+	// Always send back as much as we found and the errors.  Note that if cmdlines
+	// is empty then either nothing is listening on this port or perhaps there
+	// were permission issues reading /proc/net/tcp{,6}.
+	return cmdlines, allErrs
 }
diff --git a/agent/agentssh/portinspection_unsupported.go b/agent/agentssh/portinspection_unsupported.go
@@ -2,8 +2,8 @@
 
 package agentssh
 
-func getListeningPortProcessCmdline(uint32) (string, error) {
+func getListeningPortProcessCmdlines(uint32) ([]string, error) {
 	// We are not worrying about other platforms at the moment because Gateway
 	// only supports Linux anyway.
-	return "", nil
+	return nil, nil
 }
diff --git a/scripts/echoserver/main.go b/scripts/echoserver/main.go
@@ -1,18 +1,33 @@
 package main
 
-// A simple echo server.  It listens on a random port, prints that port, then
-// echos back anything sent to it.
+// A simple echo server that listens on the specified network (tcp4 or tcp6) and
+// port, prints the resulting port (since you can use 0 to get a random port),
+// then echos back anything sent to it.  This is to test counting applications
+// that use port forwarding; currently only JetBrains uses this method.
+// Example usage: go run ./scripts/echoserver tcp6 0 -Didea.vendor.name=JetBrains
 
 import (
 	"errors"
 	"fmt"
 	"io"
 	"log"
 	"net"
+	"os"
 )
 
 func main() {
-	l, err := net.Listen("tcp", "127.0.0.1:0")
+	network := os.Args[1]
+	var address string
+	switch network {
+	case "tcp4":
+		address = "127.0.0.1"
+	case "tcp6":
+		address = "[::]"
+	default:
+		log.Fatalf("invalid network: %s", network)
+	}
+	port := os.Args[2]
+	l, err := net.Listen(network, address+":"+port)
 	if err != nil {
 		log.Fatalf("listen error: err=%s", err)
 	}

Original file line number	Diff line number	Diff line change
`@@ -2,8 +2,8 @@`
`2`	`2`
`3`	`3`	`package agentssh`
`4`	`4`
`5`		`-func getListeningPortProcessCmdline(uint32) (string, error) {`
	`5`	`+func getListeningPortProcessCmdlines(uint32) ([]string, error) {`
`6`	`6`	`// We are not worrying about other platforms at the moment because Gateway`
`7`	`7`	`// only supports Linux anyway.`
`8`		`- return "", nil`
	`8`	`+ return nil, nil`
`9`	`9`	`}`