Revert "ssh,tempfork/gliderlabs/ssh: replace github.com/tailscale/golang-x-crypto/ssh with golang.org/x/crypto/ssh"

This reverts commit 46fd4e58a2. We don't want to include this in 1.80 yet, but can add it back post 1.80. Updates #8593 Signed-off-by: Percy Wegmann <percy@tailscale.com>
2025-08-11 13:18:53 +00:00 · 2025-01-29 10:25:50 -06:00
parent 52f88f782a
commit b60f6b849a
22 changed files with 234 additions and 175 deletions
--- a/ssh/tailssh/tailssh.go
+++ b/ssh/tailssh/tailssh.go
@@ -29,7 +29,7 @@ import (
 	"syscall"
 	"time"

-	gossh "golang.org/x/crypto/ssh"
+	gossh "github.com/tailscale/golang-x-crypto/ssh"
 	"tailscale.com/envknob"
 	"tailscale.com/ipn/ipnlocal"
 	"tailscale.com/logtail/backoff"
@@ -198,11 +198,8 @@ func (srv *server) OnPolicyChange() {
 // Setup and discover server info
 //   - ServerConfigCallback
 //
-// Get access to a ServerPreAuthConn (useful for sending banners)
-//
-// Do the user auth with a NoClientAuthCallback. If user specified
-// a username ending in "+password", follow this with password auth
-// (to work around buggy SSH clients that don't work with noauth).
+// Do the user auth
+//   - NoClientAuthHandler
 //
 // Once auth is done, the conn can be multiplexed with multiple sessions and
 // channels concurrently. At which point any of the following can be called
@@ -222,12 +219,15 @@ type conn struct {
 	idH    string
 	connID string // ID that's shared with control

-	// spac is a [gossh.ServerPreAuthConn] used for sending auth banners.
-	// Banners cannot be sent after auth completes.
-	spac gossh.ServerPreAuthConn
+	// anyPasswordIsOkay is whether the client is authorized but has requested
+	// password-based auth to work around their buggy SSH client. When set, we
+	// accept any password in the PasswordHandler.
+	anyPasswordIsOkay bool // set by NoClientAuthCallback

-	action0     *tailcfg.SSHAction // set by clientAuth
-	finalAction *tailcfg.SSHAction // set by clientAuth
+	action0        *tailcfg.SSHAction // set by doPolicyAuth; first matching action
+	currentAction  *tailcfg.SSHAction // set by doPolicyAuth, updated by resolveNextAction
+	finalAction    *tailcfg.SSHAction // set by doPolicyAuth or resolveNextAction
+	finalActionErr error              // set by doPolicyAuth or resolveNextAction

 	info         *sshConnInfo // set by setInfo
 	localUser    *userMeta    // set by doPolicyAuth
@@ -254,142 +254,141 @@ func (c *conn) vlogf(format string, args ...any) {
 	}
 }

+// isAuthorized walks through the action chain and returns nil if the connection
+// is authorized. If the connection is not authorized, it returns
+// errDenied. If the action chain resolution fails, it returns the
+// resolution error.
+func (c *conn) isAuthorized(ctx ssh.Context) error {
+	action := c.currentAction
+	for {
+		if action.Accept {
+			return nil
+		}
+		if action.Reject || action.HoldAndDelegate == "" {
+			return errDenied
+		}
+		var err error
+		action, err = c.resolveNextAction(ctx)
+		if err != nil {
+			return err
+		}
+		if action.Message != "" {
+			if err := ctx.SendAuthBanner(action.Message); err != nil {
+				return err
+			}
+		}
+	}
+}
+
 // errDenied is returned by auth callbacks when a connection is denied by the
-// policy. It returns a gossh.BannerError to make sure the message gets
-// displayed as an auth banner.
-func errDenied(message string) error {
-	if message == "" {
-		message = "tailscale: access denied"
-	}
-	return &gossh.BannerError{
-		Message: message,
-	}
-}
+// policy.
+var errDenied = errors.New("ssh: access denied")

-// bannerError creates a gossh.BannerError that will result in the given
-// message being displayed to the client. If err != nil, this also logs
-// message:error. The contents of err is not leaked to clients in the banner.
-func (c *conn) bannerError(message string, err error) error {
-	if err != nil {
-		c.logf("%s: %s", message, err)
-	}
-	return &gossh.BannerError{
-		Err:     err,
-		Message: fmt.Sprintf("tailscale: %s", message),
-	}
-}
-
-// clientAuth is responsible for performing client authentication.
+// NoClientAuthCallback implements gossh.NoClientAuthCallback and is called by
+// the ssh.Server when the client first connects with the "none"
+// authentication method.
 //
-// If policy evaluation fails, it returns an error.
-// If access is denied, it returns an error.
-func (c *conn) clientAuth(cm gossh.ConnMetadata) (*gossh.Permissions, error) {
+// It is responsible for continuing policy evaluation from BannerCallback (or
+// starting it afresh). It returns an error if the policy evaluation fails, or
+// if the decision is "reject"
+//
+// It either returns nil (accept) or errDenied (reject). The errors may be wrapped.
+func (c *conn) NoClientAuthCallback(ctx ssh.Context) error {
 	if c.insecureSkipTailscaleAuth {
-		return &gossh.Permissions{}, nil
+		return nil
+	}
+	if err := c.doPolicyAuth(ctx); err != nil {
+		return err
+	}
+	if err := c.isAuthorized(ctx); err != nil {
+		return err
 	}

-	if err := c.setInfo(cm); err != nil {
-		return nil, c.bannerError("failed to get connection info", err)
+	// Let users specify a username ending in +password to force password auth.
+	// This exists for buggy SSH clients that get confused by success from
+	// "none" auth.
+	if strings.HasSuffix(ctx.User(), forcePasswordSuffix) {
+		c.anyPasswordIsOkay = true
+		return errors.New("any password please") // not shown to users
+	}
+	return nil
+}
+
+func (c *conn) nextAuthMethodCallback(cm gossh.ConnMetadata, prevErrors []error) (nextMethod []string) {
+	switch {
+	case c.anyPasswordIsOkay:
+		nextMethod = append(nextMethod, "password")
 	}

-	action, localUser, acceptEnv, err := c.evaluatePolicy()
+	// The fake "tailscale" method is always appended to next so OpenSSH renders
+	// that in parens as the final failure. (It also shows up in "ssh -v", etc)
+	nextMethod = append(nextMethod, "tailscale")
+	return
+}
+
+// fakePasswordHandler is our implementation of the PasswordHandler hook that
+// checks whether the user's password is correct. But we don't actually use
+// passwords. This exists only for when the user's username ends in "+password"
+// to signal that their SSH client is buggy and gets confused by auth type
+// "none" succeeding and they want our SSH server to require a dummy password
+// prompt instead. We then accept any password since we've already authenticated
+// & authorized them.
+func (c *conn) fakePasswordHandler(ctx ssh.Context, password string) bool {
+	return c.anyPasswordIsOkay
+}
+
+// doPolicyAuth verifies that conn can proceed.
+// It returns nil if the matching policy action is Accept or
+// HoldAndDelegate. Otherwise, it returns errDenied.
+func (c *conn) doPolicyAuth(ctx ssh.Context) error {
+	if err := c.setInfo(ctx); err != nil {
+		c.logf("failed to get conninfo: %v", err)
+		return errDenied
+	}
+	a, localUser, acceptEnv, err := c.evaluatePolicy()
 	if err != nil {
-		return nil, c.bannerError("failed to evaluate SSH policy", err)
+		return fmt.Errorf("%w: %v", errDenied, err)
 	}
-
-	c.action0 = action
-
-	if action.Accept || action.HoldAndDelegate != "" {
-		// Immediately look up user information for purposes of generating
-		// hold and delegate URL (if necessary).
+	c.action0 = a
+	c.currentAction = a
+	c.acceptEnv = acceptEnv
+	if a.Message != "" {
+		if err := ctx.SendAuthBanner(a.Message); err != nil {
+			return fmt.Errorf("SendBanner: %w", err)
+		}
+	}
+	if a.Accept || a.HoldAndDelegate != "" {
+		if a.Accept {
+			c.finalAction = a
+		}
 		lu, err := userLookup(localUser)
 		if err != nil {
-			return nil, c.bannerError(fmt.Sprintf("failed to look up local user %q ", localUser), err)
+			c.logf("failed to look up %v: %v", localUser, err)
+			ctx.SendAuthBanner(fmt.Sprintf("failed to look up %v\r\n", localUser))
+			return err
 		}
 		gids, err := lu.GroupIds()
 		if err != nil {
-			return nil, c.bannerError("failed to look up local user's group IDs", err)
+			c.logf("failed to look up local user's group IDs: %v", err)
+			return err
 		}
 		c.userGroupIDs = gids
 		c.localUser = lu
-		c.acceptEnv = acceptEnv
+		return nil
 	}
-
-	for {
-		switch {
-		case action.Accept:
-			metricTerminalAccept.Add(1)
-			if action.Message != "" {
-				if err := c.spac.SendAuthBanner(action.Message); err != nil {
-					return nil, fmt.Errorf("error sending auth welcome message: %w", err)
-				}
-			}
-			c.finalAction = action
-			return &gossh.Permissions{}, nil
-		case action.Reject:
-			metricTerminalReject.Add(1)
-			c.finalAction = action
-			return nil, errDenied(action.Message)
-		case action.HoldAndDelegate != "":
-			if action.Message != "" {
-				if err := c.spac.SendAuthBanner(action.Message); err != nil {
-					return nil, fmt.Errorf("error sending hold and delegate message: %w", err)
-				}
-			}
-
-			url := action.HoldAndDelegate
-
-			ctx, cancel := context.WithTimeout(context.Background(), 30*time.Minute)
-			defer cancel()
-
-			metricHolds.Add(1)
-			url = c.expandDelegateURLLocked(url)
-
-			var err error
-			action, err = c.fetchSSHAction(ctx, url)
-			if err != nil {
-				metricTerminalFetchError.Add(1)
-				return nil, c.bannerError("failed to fetch next SSH action", fmt.Errorf("fetch failed from %s: %w", url, err))
-			}
-		default:
-			metricTerminalMalformed.Add(1)
-			return nil, c.bannerError("reached Action that had neither Accept, Reject, nor HoldAndDelegate", nil)
-		}
+	if a.Reject {
+		c.finalAction = a
+		return errDenied
 	}
+	// Shouldn't get here, but:
+	return errDenied
 }

 // ServerConfig implements ssh.ServerConfigCallback.
 func (c *conn) ServerConfig(ctx ssh.Context) *gossh.ServerConfig {
 	return &gossh.ServerConfig{
-		PreAuthConnCallback: func(spac gossh.ServerPreAuthConn) {
-			c.spac = spac
-		},
-		NoClientAuth: true, // required for the NoClientAuthCallback to run
-		NoClientAuthCallback: func(cm gossh.ConnMetadata) (*gossh.Permissions, error) {
-			// First perform client authentication, which can potentially
-			// involve multiple steps (for example prompting user to log in to
-			// Tailscale admin panel to confirm identity).
-			perms, err := c.clientAuth(cm)
-			if err != nil {
-				return nil, err
-			}
-
-			// Authentication succeeded. Buggy SSH clients get confused by
-			// success from the "none" auth method. As a workaround, let users
-			// specify a username ending in "+password" to force password auth.
-			// The actual value of the password doesn't matter.
-			if strings.HasSuffix(cm.User(), forcePasswordSuffix) {
-				return nil, &gossh.PartialSuccessError{
-					Next: gossh.ServerAuthCallbacks{
-						PasswordCallback: func(_ gossh.ConnMetadata, password []byte) (*gossh.Permissions, error) {
-							return &gossh.Permissions{}, nil
-						},
-					},
-				}
-			}
-
-			return perms, nil
-		},
+		NoClientAuth:           true, // required for the NoClientAuthCallback to run
+		NextAuthMethodCallback: c.nextAuthMethodCallback,
 	}
 }

@@ -400,7 +399,7 @@ func (srv *server) newConn() (*conn, error) {
 		// Stop accepting new connections.
 		// Connections in the auth phase are handled in handleConnPostSSHAuth.
 		// Existing sessions are terminated by Shutdown.
-		return nil, errDenied("tailscale: server is shutting down")
+		return nil, errDenied
 	}
 	srv.mu.Unlock()
 	c := &conn{srv: srv}
@@ -411,6 +410,9 @@ func (srv *server) newConn() (*conn, error) {
 		Version:              "Tailscale",
 		ServerConfigCallback: c.ServerConfig,

+		NoClientAuthHandler: c.NoClientAuthCallback,
+		PasswordHandler:     c.fakePasswordHandler,
+
 		Handler:                       c.handleSessionPostSSHAuth,
 		LocalPortForwardingCallback:   c.mayForwardLocalPortTo,
 		ReversePortForwardingCallback: c.mayReversePortForwardTo,
@@ -521,16 +523,16 @@ func toIPPort(a net.Addr) (ipp netip.AddrPort) {
 	return netip.AddrPortFrom(tanetaddr.Unmap(), uint16(ta.Port))
 }

-// connInfo populates the sshConnInfo from the provided arguments,
+// connInfo returns a populated sshConnInfo from the provided arguments,
 // validating only that they represent a known Tailscale identity.
-func (c *conn) setInfo(cm gossh.ConnMetadata) error {
+func (c *conn) setInfo(ctx ssh.Context) error {
 	if c.info != nil {
 		return nil
 	}
 	ci := &sshConnInfo{
-		sshUser: strings.TrimSuffix(cm.User(), forcePasswordSuffix),
-		src:     toIPPort(cm.RemoteAddr()),
-		dst:     toIPPort(cm.LocalAddr()),
+		sshUser: strings.TrimSuffix(ctx.User(), forcePasswordSuffix),
+		src:     toIPPort(ctx.RemoteAddr()),
+		dst:     toIPPort(ctx.LocalAddr()),
 	}
 	if !tsaddr.IsTailscaleIP(ci.dst.Addr()) {
 		return fmt.Errorf("tailssh: rejecting non-Tailscale local address %v", ci.dst)
@@ -545,7 +547,7 @@ func (c *conn) setInfo(cm gossh.ConnMetadata) error {
 	ci.node = node
 	ci.uprof = uprof

-	c.idH = string(cm.SessionID())
+	c.idH = ctx.SessionID()
 	c.info = ci
 	c.logf("handling conn: %v", ci.String())
 	return nil
@@ -592,6 +594,62 @@ func (c *conn) handleSessionPostSSHAuth(s ssh.Session) {
 	ss.run()
 }

+// resolveNextAction starts at c.currentAction and makes it way through the
+// action chain one step at a time. An action without a HoldAndDelegate is
+// considered the final action. Once a final action is reached, this function
+// will keep returning that action. It updates c.currentAction to the next
+// action in the chain. When the final action is reached, it also sets
+// c.finalAction to the final action.
+func (c *conn) resolveNextAction(sctx ssh.Context) (action *tailcfg.SSHAction, err error) {
+	if c.finalAction != nil || c.finalActionErr != nil {
+		return c.finalAction, c.finalActionErr
+	}
+
+	defer func() {
+		if action != nil {
+			c.currentAction = action
+			if action.Accept || action.Reject {
+				c.finalAction = action
+			}
+		}
+		if err != nil {
+			c.finalActionErr = err
+		}
+	}()
+
+	ctx, cancel := context.WithCancel(sctx)
+	defer cancel()
+
+	// Loop processing/fetching Actions until one reaches a
+	// terminal state (Accept, Reject, or invalid Action), or
+	// until fetchSSHAction times out due to the context being
+	// done (client disconnect) or its 30 minute timeout passes.
+	// (Which is a long time for somebody to see login
+	// instructions and go to a URL to do something.)
+	action = c.currentAction
+	if action.Accept || action.Reject {
+		if action.Reject {
+			metricTerminalReject.Add(1)
+		} else {
+			metricTerminalAccept.Add(1)
+		}
+		return action, nil
+	}
+	url := action.HoldAndDelegate
+	if url == "" {
+		metricTerminalMalformed.Add(1)
+		return nil, errors.New("reached Action that lacked Accept, Reject, and HoldAndDelegate")
+	}
+	metricHolds.Add(1)
+	url = c.expandDelegateURLLocked(url)
+	nextAction, err := c.fetchSSHAction(ctx, url)
+	if err != nil {
+		metricTerminalFetchError.Add(1)
+		return nil, fmt.Errorf("fetching SSHAction from %s: %w", url, err)
+	}
+	return nextAction, nil
+}
+
 func (c *conn) expandDelegateURLLocked(actionURL string) string {
 	nm := c.srv.lb.NetMap()
 	ci := c.info
--- a/ssh/tailssh/tailssh_integration_test.go
+++ b/ssh/tailssh/tailssh_integration_test.go
@@ -32,8 +32,8 @@ import (
 	"github.com/bramvdbogaerde/go-scp"
 	"github.com/google/go-cmp/cmp"
 	"github.com/pkg/sftp"
+	gossh "github.com/tailscale/golang-x-crypto/ssh"
 	"golang.org/x/crypto/ssh"
-	gossh "golang.org/x/crypto/ssh"
 	"golang.org/x/crypto/ssh/agent"
 	"tailscale.com/net/tsdial"
 	"tailscale.com/tailcfg"
--- a/ssh/tailssh/tailssh_test.go
+++ b/ssh/tailssh/tailssh_test.go
@@ -31,7 +31,7 @@ import (
 	"testing"
 	"time"

-	gossh "golang.org/x/crypto/ssh"
+	gossh "github.com/tailscale/golang-x-crypto/ssh"
 	"golang.org/x/net/http2"
 	"golang.org/x/net/http2/h2c"
 	"tailscale.com/ipn/ipnlocal"
@@ -805,8 +805,7 @@ func TestSSHAuthFlow(t *testing.T) {
 			state: &localState{
 				sshEnabled: true,
 			},
-			authErr:     true,
-			wantBanners: []string{"tailscale: failed to evaluate SSH policy"},
+			authErr: true,
 		},
 		{
 			name: "accept",