ssh/tailssh: remove unused public key authentication logic

Updates #8593

Signed-off-by: Percy Wegmann <percy@tailscale.com>

ssh/tailssh/tailssh.go

@@ -30,7 +30,7 @@
 	"syscall"
 	"time"
 
-	gossh "github.com/tailscale/golang-x-crypto/ssh"
+	gossh "golang.org/x/crypto/ssh"
 	"tailscale.com/envknob"
 	"tailscale.com/ipn/ipnlocal"
 	"tailscale.com/logtail/backoff"
@@ -200,7 +200,9 @@ func (srv *server) OnPolicyChange() {
 //   - ServerConfigCallback
 //
 // Do the user auth
-//   - NoClientAuthHandler
+//   - welcomeBanner
+//   - noClientAuthCallback
+//   - passwordCallback (if username ends in +password)
 //
 // 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,17 +224,16 @@ type conn struct {
 
 	// 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.
+	// accept any password in the passwordCallback.
 	anyPasswordIsOkay bool // set by NoClientAuthCallback
 
-	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
+	action0      *tailcfg.SSHAction // the first action from authentication
+	action0Error error              // the first error from authentication
+	finalAction  *tailcfg.SSHAction // the final action from authentication
 
 	info         *sshConnInfo // set by setInfo
-	localUser    *userMeta    // set by doPolicyAuth
-	userGroupIDs []string     // set by doPolicyAuth
+	localUser    *userMeta    // set by authenticate
+	userGroupIDs []string     // set by authenticate
 
 	// mu protects the following fields.
 	//
@@ -254,139 +255,185 @@ 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
+// clientAuthCallback is a callback that handles the authentication of
+// clients, irrespective of whether they're authenticating with none, password
+// or public key. It picks up where welcomeBanner() left off.
+func (c *conn) clientAuthCallback() (*gossh.Permissions, error) {
+	if c.action0Error != nil {
+		metricTerminalReject.Add(1)
+		return nil, c.action0Error
+	}
+
+	if c.finalAction != nil {
+		switch {
+		case c.finalAction.Reject:
+			// This should never happen, as c.action0Error should have already been set
+			panic("finalAction unexpectedly Reject")
+		case c.finalAction.Accept:
+			// Already authenticated.
+			metricTerminalAccept.Add(1)
+			return &gossh.Permissions{}, nil
 		}
-		if action.Reject || action.HoldAndDelegate == "" {
-			return errDenied
-		}
-		var err error
-		action, err = c.resolveNextAction(ctx)
+	}
+
+	// Further steps are required
+	url := c.action0.HoldAndDelegate
+	if url == "" {
+		metricTerminalMalformed.Add(1)
+		return nil, errors.New("reached Action that lacked Accept, Reject, and HoldAndDelegate")
+	}
+
+	ctx, cancel := context.WithTimeout(context.Background(), 30*time.Minute)
+	defer cancel()
+
+	metricHolds.Add(1)
+	url = c.expandDelegateURLLocked(url)
+
+	var err error
+	c.finalAction, err = c.fetchSSHAction(ctx, url)
+	if err != nil {
+		metricTerminalFetchError.Add(1)
+		return nil, fmt.Errorf("fetching SSHAction from %s: %w", url, err)
+	}
+
+	switch {
+	case c.finalAction.Accept:
+		metricTerminalReject.Add(1)
+		return &gossh.Permissions{}, nil
+	case c.finalAction.Reject:
+		metricTerminalAccept.Add(1)
+		return nil, errDenied(c.finalAction.Message)
+	default:
+		metricTerminalMalformed.Add(1)
+		return nil, errors.New("final action was neither accept nor reject")
+	}
+}
+
+// authenticate authenticates the connection, returning the next (possibly
+// final) tailcfg.SSHAction and, if it encounters a final error, the error.
+func (c *conn) authenticate() (*tailcfg.SSHAction, error) {
+	a, localUser, err := c.evaluatePolicy()
+	if err != nil {
+		return nil, fmt.Errorf("%w: %v", errDenied(""), err)
+	}
+
+	switch {
+	case a.Reject:
+		return nil, errDenied(a.Message)
+	case a.Accept || a.HoldAndDelegate != "":
+		lu, err := userLookup(localUser)
 		if err != nil {
-			return err
+			c.logf("failed to look up %v: %v", localUser, err)
+			return nil, bannerError(fmt.Sprintf("failed to look up %v\r\n", localUser), err)
 		}
-		if action.Message != "" {
-			if err := ctx.SendAuthBanner(action.Message); err != nil {
-				return err
-			}
+		gids, err := lu.GroupIds()
+		if err != nil {
+			c.logf("failed to look up local user's group IDs: %v", err)
+			return nil, bannerError("failed to look up local user's group IDs\r\n", err)
 		}
+		c.userGroupIDs = gids
+		c.localUser = lu
+		return a, nil
+	default:
+		// Shouldn't get here, but:
+		return nil, errDenied("")
 	}
 }
 
 // errDenied is returned by auth callbacks when a connection is denied by the
-// policy.
-var errDenied = errors.New("ssh: access denied")
+// policy. If message is non-empty, it returns a gossh.BannerError to make sure
+// the message gets displayed as an auth banner.
+func errDenied(message string) error {
+	err := errors.New("ssh: access denied")
+	if message == "" {
+		return err
+	}
+	return bannerError(message, err)
+}
 
-// NoClientAuthCallback implements gossh.NoClientAuthCallback and is called by
+func bannerError(message string, err error) error {
+	return &gossh.BannerError{
+		Err:     err,
+		Message: message,
+	}
+}
+
+// noClientAuthCallback implements gossh.noClientAuthCallback and is called by
 // the ssh.Server when the client first connects with the "none"
 // authentication method.
 //
 // 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"
+// 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 {
+func (c *conn) noClientAuthCallback(cm gossh.ConnMetadata) (*gossh.Permissions, error) {
 	if c.insecureSkipTailscaleAuth {
-		return nil
+		return &gossh.Permissions{}, nil
 	}
-	if err := c.doPolicyAuth(ctx); err != nil {
-		return err
+	perms, err := c.clientAuthCallback()
+	if err != nil {
+		return nil, err
 	}
-	if err := c.isAuthorized(ctx); err != nil {
-		return 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) {
+	if strings.HasSuffix(cm.User(), forcePasswordSuffix) {
 		c.anyPasswordIsOkay = true
-		return errors.New("any password please") // not shown to users
+		return nil, &gossh.PartialSuccessError{
+			Next: gossh.ServerAuthCallbacks{
+				PasswordCallback: c.passwordCallback,
+			},
+		}
 	}
-	return nil
+	return perms, nil
 }
 
-func (c *conn) nextAuthMethodCallback(cm gossh.ConnMetadata, prevErrors []error) (nextMethod []string) {
-	if c.anyPasswordIsOkay {
-		nextMethod = append(nextMethod, "password")
-	}
-
-	// 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
+// passwordCallback is our implementation of the PasswordCallback 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
+func (c *conn) passwordCallback(_ gossh.ConnMetadata, password []byte) (*gossh.Permissions, error) {
+	return &gossh.Permissions{}, nil
 }
 
-// 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 {
+// welcomeBanner looks for a welcome banner to display prior to authentication.
+// For example, if SSH session recording is enabled, control will give us an
+// action message informing the user of this.
+// This function actually begins authentication in order to make sure it knows
+// if there's a banner to send.
+func (c *conn) welcomeBanner(cm gossh.ConnMetadata) (banner string) {
+	if c.insecureSkipTailscaleAuth {
+		return ""
+	}
+
+	if err := c.setInfo(cm); err != nil {
 		c.logf("failed to get conninfo: %v", err)
-		return errDenied
+		c.action0Error = errDenied("")
+		return ""
 	}
-	a, localUser, err := c.evaluatePolicy()
-	if err != nil {
-		return fmt.Errorf("%w: %v", errDenied, err)
-	}
-	c.action0 = a
-	c.currentAction = a
-	if a.Message != "" {
-		if err := ctx.SendAuthBanner(a.Message); err != nil {
-			return fmt.Errorf("SendBanner: %w", err)
+
+	c.action0, c.action0Error = c.authenticate()
+	if c.action0Error == nil {
+		if c.action0.Accept || c.action0.Reject {
+			c.finalAction = c.action0
 		}
+		return c.action0.Message
 	}
-	if a.Accept || a.HoldAndDelegate != "" {
-		if a.Accept {
-			c.finalAction = a
-		}
-		lu, err := userLookup(localUser)
-		if err != nil {
-			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 {
-			c.logf("failed to look up local user's group IDs: %v", err)
-			return err
-		}
-		c.userGroupIDs = gids
-		c.localUser = lu
-		return nil
-	}
-	if a.Reject {
-		c.finalAction = a
-		return errDenied
-	}
-	// Shouldn't get here, but:
-	return errDenied
+
+	return ""
 }
 
 // ServerConfig implements ssh.ServerConfigCallback.
 func (c *conn) ServerConfig(ctx ssh.Context) *gossh.ServerConfig {
 	return &gossh.ServerConfig{
-		NoClientAuth:           true, // required for the NoClientAuthCallback to run
-		NextAuthMethodCallback: c.nextAuthMethodCallback,
+		BannerCallback:       c.welcomeBanner,
+		NoClientAuth:         true, // required for the NoClientAuthCallback to run
+		NoClientAuthCallback: c.noClientAuthCallback,
 	}
 }
 
@@ -397,7 +444,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
+		return nil, errDenied("")
 	}
 	srv.mu.Unlock()
 	c := &conn{srv: srv}
@@ -408,9 +455,6 @@ 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,
@@ -523,14 +567,14 @@ func toIPPort(a net.Addr) (ipp netip.AddrPort) {
 
 // connInfo returns a populated sshConnInfo from the provided arguments,
 // validating only that they represent a known Tailscale identity.
-func (c *conn) setInfo(ctx ssh.Context) error {
+func (c *conn) setInfo(cm gossh.ConnMetadata) error {
 	if c.info != nil {
 		return nil
 	}
 	ci := &sshConnInfo{
-		sshUser: strings.TrimSuffix(ctx.User(), forcePasswordSuffix),
-		src:     toIPPort(ctx.RemoteAddr()),
-		dst:     toIPPort(ctx.LocalAddr()),
+		sshUser: strings.TrimSuffix(cm.User(), forcePasswordSuffix),
+		src:     toIPPort(cm.RemoteAddr()),
+		dst:     toIPPort(cm.LocalAddr()),
 	}
 	if !tsaddr.IsTailscaleIP(ci.dst.Addr()) {
 		return fmt.Errorf("tailssh: rejecting non-Tailscale local address %v", ci.dst)
@@ -545,7 +589,7 @@ func (c *conn) setInfo(ctx ssh.Context) error {
 	ci.node = node
 	ci.uprof = uprof
 
-	c.idH = ctx.SessionID()
+	c.idH = string(cm.SessionID())
 	c.info = ci
 	c.logf("handling conn: %v", ci.String())
 	return nil
@@ -592,62 +636,6 @@ 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