ssh/tailssh: make the SSH server a singleton, register with LocalBackend

Remove the weird netstack -> tailssh dependency and instead have tailssh
register itself with ipnlocal when linked.

This makes tailssh.server a singleton, so we can have a global map of
all sessions.

Updates #3802

Change-Id: Iad5caec3a26a33011796878ab66b8e7b49339f29
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>

cmd/tailscaled/depaware.txt

@@ -231,7 +231,7 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de
         tailscale.com/portlist                                       from tailscale.com/ipn/ipnlocal
         tailscale.com/safesocket                                     from tailscale.com/client/tailscale+
         tailscale.com/smallzstd                                      from tailscale.com/ipn/ipnserver+
-  LD 💣 tailscale.com/ssh/tailssh                                    from tailscale.com/wgengine/netstack
+  LD 💣 tailscale.com/ssh/tailssh                                    from tailscale.com/cmd/tailscaled
         tailscale.com/syncs                                          from tailscale.com/control/controlknobs+
         tailscale.com/tailcfg                                        from tailscale.com/client/tailscale+
   LD    tailscale.com/tempfork/gliderlabs/ssh                        from tailscale.com/ssh/tailssh

cmd/tailscaled/ssh.go

@@ -0,0 +1,12 @@
+// Copyright (c) 2022 Tailscale Inc & AUTHORS All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+//go:build linux || darwin
+// +build linux darwin
+
+package main
+
+// Force registration of tailssh with LocalBackend.
+import _ "tailscale.com/ssh/tailssh"
+

ipn/ipnlocal/local.go

@@ -73,6 +73,20 @@ func getControlDebugFlags() []string {
 	return nil
 }
 
+// SSHServer is the interface of the conditionally linked ssh/tailssh.server.
+type SSHServer interface {
+	HandleSSHConn(net.Conn) error
+}
+
+type newSSHServerFunc func(logger.Logf, *LocalBackend) (SSHServer, error)
+
+var newSSHServer newSSHServerFunc // or nil
+
+// RegisterNewSSHServer lets the conditionally linked ssh/tailssh package register itself.
+func RegisterNewSSHServer(fn newSSHServerFunc) {
+	newSSHServer = fn
+}
+
 // LocalBackend is the glue between the major pieces of the Tailscale
 // network software: the cloud control plane (via controlclient), the
 // network data plane (via wgengine), and the user-facing UIs and CLIs
@@ -103,6 +117,7 @@ type LocalBackend struct {
 	newDecompressor       func() (controlclient.Decompressor, error)
 	varRoot               string // or empty if SetVarRoot never called
 	sshAtomicBool         syncs.AtomicBool
+	sshServer             SSHServer // or nil
 
 	filterHash deephash.Sum
 
@@ -205,6 +220,12 @@ func NewLocalBackend(logf logger.Logf, logid string, store ipn.StateStore, diale
 		gotPortPollRes: make(chan struct{}),
 		loginFlags:     loginFlags,
 	}
+	if newSSHServer != nil {
+		b.sshServer, err = newSSHServer(logf, b)
+		if err != nil {
+			return nil, fmt.Errorf("newSSHServer: %w", err)
+		}
+	}
 
 	// Default filter blocks everything and logs nothing, until Start() is called.
 	b.setFilter(filter.NewAllowNone(logf, &netaddr.IPSet{}))
@@ -3225,3 +3246,10 @@ func (b *LocalBackend) DoNoiseRequest(req *http.Request) (*http.Response, error)
 	}
 	return cc.DoNoiseRequest(req)
 }
+
+func (b *LocalBackend) HandleSSHConn(c net.Conn) error {
+	if b.sshServer == nil {
+		return errors.New("no SSH server")
+	}
+	return b.sshServer.HandleSSHConn(c)
+}

ssh/tailssh/tailssh.go

@@ -42,27 +42,41 @@
 	"tailscale.com/types/logger"
 )
 
-var sshVerboseLogging = envknob.Bool("TS_DEBUG_SSH_VLOG")
+var (
+	debugPolicyFile             = envknob.String("TS_DEBUG_SSH_POLICY_FILE")
+	debugIgnoreTailnetSSHPolicy = envknob.Bool("TS_DEBUG_SSH_IGNORE_TAILNET_POLICY")
+	sshVerboseLogging           = envknob.Bool("TS_DEBUG_SSH_VLOG")
+)
 
-// TODO(bradfitz): this is all very temporary as code is temporarily
+type server struct {
-// being moved around; it will be restructured and documented in
+	lb             *ipnlocal.LocalBackend
-// following commits.
+	logf           logger.Logf
+	tailscaledPath string
 
-// Handle handles an SSH connection from c.
+	// mu protects activeSessions.
-func Handle(logf logger.Logf, lb *ipnlocal.LocalBackend, c net.Conn) error {
+	mu                      sync.Mutex
+	activeSessionByH        map[string]*sshSession // ssh.SessionID (DH H) => that session
+	activeSessionBySharedID map[string]*sshSession // yyymmddThhmmss-XXXXX => session
+}
+
+func init() {
+	ipnlocal.RegisterNewSSHServer(func(logf logger.Logf, lb *ipnlocal.LocalBackend) (ipnlocal.SSHServer, error) {
 		tsd, err := os.Executable()
 		if err != nil {
-		return err
+			return nil, err
 		}
-	// TODO(bradfitz): make just one server for the whole process. rearrange
-	// netstack's hooks to be a constructor given a lb instead. Then the *server
-	// will have a HandleTailscaleConn method.
 		srv := &server{
 			lb:             lb,
 			logf:           logf,
 			tailscaledPath: tsd,
 		}
-	ss, err := srv.newSSHServer()
+		return srv, nil
+	})
+}
+
+// HandleSSHConn handles a Tailscale SSH connection from c.
+func (s *server) HandleSSHConn(c net.Conn) error {
+	ss, err := s.newSSHServer()
 	if err != nil {
 		return err
 	}
@@ -121,22 +135,6 @@ func (srv *server) newSSHServer() (*ssh.Server, error) {
 	return ss, nil
 }
 
-type server struct {
-	lb             *ipnlocal.LocalBackend
-	logf           logger.Logf
-	tailscaledPath string
-
-	// mu protects activeSessions.
-	mu                      sync.Mutex
-	activeSessionByH        map[string]*sshSession // ssh.SessionID (DH H) => that session
-	activeSessionBySharedID map[string]*sshSession // yyymmddThhmmss-XXXXX => session
-}
-
-var (
-	debugPolicyFile             = envknob.String("TS_DEBUG_SSH_POLICY_FILE")
-	debugIgnoreTailnetSSHPolicy = envknob.Bool("TS_DEBUG_SSH_IGNORE_TAILNET_POLICY")
-)
-
 // mayForwardLocalPortTo reports whether the ctx should be allowed to port forward
 // to the specified host and port.
 // TODO(bradfitz/maisem): should we have more checks on host/port?

tstest/integration/tailscaled_deps_test_darwin.go

@@ -33,6 +33,7 @@
 	_ "tailscale.com/net/tstun"
 	_ "tailscale.com/paths"
 	_ "tailscale.com/safesocket"
+	_ "tailscale.com/ssh/tailssh"
 	_ "tailscale.com/tailcfg"
 	_ "tailscale.com/tsweb"
 	_ "tailscale.com/types/flagtype"

tstest/integration/tailscaled_deps_test_linux.go

@@ -33,6 +33,7 @@
 	_ "tailscale.com/net/tstun"
 	_ "tailscale.com/paths"
 	_ "tailscale.com/safesocket"
+	_ "tailscale.com/ssh/tailssh"
 	_ "tailscale.com/tailcfg"
 	_ "tailscale.com/tsweb"
 	_ "tailscale.com/types/flagtype"

wgengine/netstack/netstack.go

@@ -662,9 +662,8 @@ func (ns *Impl) acceptTCP(r *tcp.ForwarderRequest) {
 	c := gonet.NewTCPConn(&wq, ep)
 
 	if ns.lb != nil {
-		if reqDetails.LocalPort == 22 && ns.processSSH() && ns.isLocalIP(dialIP) && handleSSH != nil {
+		if reqDetails.LocalPort == 22 && ns.processSSH() && ns.isLocalIP(dialIP) {
-			ns.logf("handling SSH connection....")
+			if err := ns.lb.HandleSSHConn(c); err != nil {
-			if err := handleSSH(ns.logf, ns.lb, c); err != nil {
 				ns.logf("ssh error: %v", err)
 			}
 			return

wgengine/netstack/ssh.go

@@ -1,14 +0,0 @@
-// Copyright (c) 2021 Tailscale Inc & AUTHORS All rights reserved.
-// Use of this source code is governed by a BSD-style
-// license that can be found in the LICENSE file.
-
-//go:build linux || (darwin && !ios)
-// +build linux darwin,!ios
-
-package netstack
-
-import "tailscale.com/ssh/tailssh"
-
-func init() {
-	handleSSH = tailssh.Handle
-}