From f78e8f6ca6d00b6b030afc42e542d8017dd3a244 Mon Sep 17 00:00:00 2001 From: Percy Wegmann Date: Wed, 5 Jun 2024 12:12:31 -0500 Subject: [PATCH] ssh/tailssh: remove unused public key authentication logic Updates #8593 Signed-off-by: Percy Wegmann --- ssh/tailssh/tailssh.go | 314 ++++++++++++++++++++--------------------- 1 file changed, 151 insertions(+), 163 deletions(-) diff --git a/ssh/tailssh/tailssh.go b/ssh/tailssh/tailssh.go index 4aa969cfa..0854a4904 100644 --- a/ssh/tailssh/tailssh.go +++ b/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