fix: fixes use of possibly nil RemoteAddr() and LocalAddr() return values (#21076)

fixes: https://github.com/coder/internal/issues/1143

Both gVisor and the Go standard library implementations of `net.Conn` can under certain circumstances return `nil` for `RemoteAddr()` and `LocalAddr()` calls. If we call their methods, we segfault.

This PR fixes these calls and adds ruleguard rules.

Note that `slog.F("remote_addr", conn.RemoteAddr())` is fine because slog detects the `nil` before attempting to stringify the type.

agent/agent.go

@@ -1576,8 +1576,8 @@ func (a *agent) createTailnet(
 				break
 			}
 			clog := a.logger.Named("speedtest").With(
-				slog.F("remote", conn.RemoteAddr().String()),
-				slog.F("local", conn.LocalAddr().String()))
+				slog.F("remote", conn.RemoteAddr()),
+				slog.F("local", conn.LocalAddr()))
 			clog.Info(ctx, "accepted conn")
 			wg.Add(1)
 			closed := make(chan struct{})

agent/agent_internal_test.go

@@ -0,0 +1,45 @@
+package agent
+
+import (
+	"testing"
+
+	"github.com/google/uuid"
+	"github.com/stretchr/testify/require"
+
+	"cdr.dev/slog"
+	"cdr.dev/slog/sloggers/slogtest"
+
+	"github.com/coder/coder/v2/agent/proto"
+	"github.com/coder/coder/v2/testutil"
+)
+
+// TestReportConnectionEmpty tests that reportConnection() doesn't choke if given an empty IP string, which is what we
+// send if we cannot get the remote address.
+func TestReportConnectionEmpty(t *testing.T) {
+	t.Parallel()
+	connID := uuid.UUID{1}
+	logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug)
+	ctx := testutil.Context(t, testutil.WaitShort)
+
+	uut := &agent{
+		hardCtx: ctx,
+		logger:  logger,
+	}
+	disconnected := uut.reportConnection(connID, proto.Connection_TYPE_UNSPECIFIED, "")
+
+	require.Len(t, uut.reportConnections, 1)
+	req0 := uut.reportConnections[0]
+	require.Equal(t, proto.Connection_TYPE_UNSPECIFIED, req0.GetConnection().GetType())
+	require.Equal(t, "", req0.GetConnection().Ip)
+	require.Equal(t, connID[:], req0.GetConnection().GetId())
+	require.Equal(t, proto.Connection_CONNECT, req0.GetConnection().GetAction())
+
+	disconnected(0, "because")
+	require.Len(t, uut.reportConnections, 2)
+	req1 := uut.reportConnections[1]
+	require.Equal(t, proto.Connection_TYPE_UNSPECIFIED, req1.GetConnection().GetType())
+	require.Equal(t, "", req1.GetConnection().Ip)
+	require.Equal(t, connID[:], req1.GetConnection().GetId())
+	require.Equal(t, proto.Connection_DISCONNECT, req1.GetConnection().GetAction())
+	require.Equal(t, "because", req1.GetConnection().GetReason())
+}

agent/agentssh/agentssh.go

@@ -391,10 +391,19 @@ func (s *Server) sessionHandler(session ssh.Session) {
 	env := session.Environ()
 	magicType, magicTypeRaw, env := extractMagicSessionType(env)
 
+	// It's not safe to assume RemoteAddr() returns a non-nil value. slog.F usage is fine because it correctly
+	// handles nil.
+	// c.f. https://github.com/coder/internal/issues/1143
+	remoteAddr := session.RemoteAddr()
+	remoteAddrString := ""
+	if remoteAddr != nil {
+		remoteAddrString = remoteAddr.String()
+	}
+
 	if !s.trackSession(session, true) {
 		reason := "unable to accept new session, server is closing"
 		// Report connection attempt even if we couldn't accept it.
-		disconnected := s.config.ReportConnection(id, magicType, session.RemoteAddr().String())
+		disconnected := s.config.ReportConnection(id, magicType, remoteAddrString)
 		defer disconnected(1, reason)
 
 		logger.Info(ctx, reason)
@@ -429,7 +438,7 @@ func (s *Server) sessionHandler(session ssh.Session) {
 		scr := &sessionCloseTracker{Session: session}
 		session = scr
 
-		disconnected := s.config.ReportConnection(id, magicType, session.RemoteAddr().String())
+		disconnected := s.config.ReportConnection(id, magicType, remoteAddrString)
 		defer func() {
 			disconnected(scr.exitCode(), reason)
 		}()

agent/agentssh/x11.go

@@ -176,7 +176,7 @@ func (x *x11Forwarder) listenForConnections(
 		var originPort uint32
 
 		if tcpConn, ok := conn.(*net.TCPConn); ok {
-			if tcpAddr, ok := tcpConn.LocalAddr().(*net.TCPAddr); ok {
+			if tcpAddr, ok := tcpConn.LocalAddr().(*net.TCPAddr); ok && tcpAddr != nil {
 				originAddr = tcpAddr.IP.String()
 				// #nosec G115 - Safe conversion as TCP port numbers are within uint32 range (0-65535)
 				originPort = uint32(tcpAddr.Port)

agent/reconnectingpty/server.go

@@ -74,11 +74,21 @@ func (s *Server) Serve(ctx, hardCtx context.Context, l net.Listener) (retErr err
 			break
 		}
 		clog := s.logger.With(
-			slog.F("remote", conn.RemoteAddr().String()),
-			slog.F("local", conn.LocalAddr().String()))
+			slog.F("remote", conn.RemoteAddr()),
+			slog.F("local", conn.LocalAddr()))
 		clog.Info(ctx, "accepted conn")
+
+		// It's not safe to assume RemoteAddr() returns a non-nil value. slog.F usage is fine because it correctly
+		// handles nil.
+		// c.f. https://github.com/coder/internal/issues/1143
+		remoteAddr := conn.RemoteAddr()
+		remoteAddrString := ""
+		if remoteAddr != nil {
+			remoteAddrString = remoteAddr.String()
+		}
+
 		wg.Add(1)
-		disconnected := s.reportConnection(uuid.New(), conn.RemoteAddr().String())
+		disconnected := s.reportConnection(uuid.New(), remoteAddrString)
 		closed := make(chan struct{})
 		go func() {
 			defer wg.Done()