ssh/tailssh: send banner messages during auth, move more to conn

(VSCode Live Share between Brad & Maisem!)

Updates #3802

Change-Id: Id8edca4481b0811debfdf56d4ccb1a46f71dd6d3
Co-Authored-By: Brad Fitzpatrick <bradfitz@tailscale.com>
Signed-off-by: Maisem Ali <maisem@tailscale.com>

go.mod

@@ -40,7 +40,7 @@ require (
 	github.com/tailscale/certstore v0.1.1-0.20220316223106-78d6e1c49d8d
 	github.com/tailscale/depaware v0.0.0-20210622194025-720c4b409502
 	github.com/tailscale/goexpect v0.0.0-20210902213824-6e8c725cea41
-	github.com/tailscale/golang-x-crypto v0.0.0-20220420170900-3a580d9e7b34
+	github.com/tailscale/golang-x-crypto v0.0.0-20220420224200-c602b5dfaa7f
 	github.com/tailscale/goupnp v1.0.1-0.20210804011211-c64d0f06ea05
 	github.com/tailscale/hujson v0.0.0-20211105212140-3a0adc019d83
 	github.com/tailscale/netlink v1.1.1-0.20211101221916-cabfb018fe85

go.sum

@@ -1067,8 +1067,8 @@ github.com/tailscale/depaware v0.0.0-20210622194025-720c4b409502 h1:34icjjmqJ2HP
 github.com/tailscale/depaware v0.0.0-20210622194025-720c4b409502/go.mod h1:p9lPsd+cx33L3H9nNoecRRxPssFKUwwI50I3pZ0yT+8=
 github.com/tailscale/goexpect v0.0.0-20210902213824-6e8c725cea41 h1:/V2rCMMWcsjYaYO2MeovLw+ClP63OtXgCF2Y1eb8+Ns=
 github.com/tailscale/goexpect v0.0.0-20210902213824-6e8c725cea41/go.mod h1:/roCdA6gg6lQyw/Oz6gIIGu3ggJKYhF+WC/AQReE5XQ=
-github.com/tailscale/golang-x-crypto v0.0.0-20220420170900-3a580d9e7b34 h1:ibRgOygS0bLOht+rOfaTuTvHiwmqeteG+rlFYs18aD8=
+github.com/tailscale/golang-x-crypto v0.0.0-20220420224200-c602b5dfaa7f h1:3CuODoSnBXS+ZkQlGakDqtX1o2RteR1870yF+dS61PY=
-github.com/tailscale/golang-x-crypto v0.0.0-20220420170900-3a580d9e7b34/go.mod h1:95n9fbUCixVSI4QXLEvdKJjnYK2eUlkTx9+QwLPXFKU=
+github.com/tailscale/golang-x-crypto v0.0.0-20220420224200-c602b5dfaa7f/go.mod h1:95n9fbUCixVSI4QXLEvdKJjnYK2eUlkTx9+QwLPXFKU=
 github.com/tailscale/goupnp v1.0.1-0.20210804011211-c64d0f06ea05 h1:4chzWmimtJPxRs2O36yuGRW3f9SYV+bMTTvMBI0EKio=
 github.com/tailscale/goupnp v1.0.1-0.20210804011211-c64d0f06ea05/go.mod h1:PdCqy9JzfWMJf1H5UJW2ip33/d4YkoKN0r67yKH1mG8=
 github.com/tailscale/hujson v0.0.0-20211105212140-3a0adc019d83 h1:f7nwzdAHTUUOJjHZuDvLz9CEAlUM228amCRvwzlPvsA=

ssh/tailssh/incubator.go

@@ -59,11 +59,11 @@ var maybeStartLoginSession = func(logf logger.Logf, uid uint32, localUser, remot
 // If ss.srv.tailscaledPath is empty, this method is equivalent to
 // exec.CommandContext.
 func (ss *sshSession) newIncubatorCommand(ctx context.Context, name string, args []string) *exec.Cmd {
-	if ss.srv.tailscaledPath == "" {
+	if ss.conn.srv.tailscaledPath == "" {
 		return exec.CommandContext(ctx, name, args...)
 	}
-	lu := ss.localUser
+	lu := ss.conn.localUser
-	ci := ss.connInfo
+	ci := ss.conn.info
 	remoteUser := ci.uprof.LoginName
 	if len(ci.node.Tags) > 0 {
 		remoteUser = strings.Join(ci.node.Tags, ",")
@@ -85,7 +85,7 @@ func (ss *sshSession) newIncubatorCommand(ctx context.Context, name string, args
 		incubatorArgs = append(incubatorArgs, args...)
 	}
 
-	return exec.CommandContext(ctx, ss.srv.tailscaledPath, incubatorArgs...)
+	return exec.CommandContext(ctx, ss.conn.srv.tailscaledPath, incubatorArgs...)
 }
 
 const debugIncubator = false
@@ -166,7 +166,7 @@ func beIncubator(args []string) error {
 //
 // It sets ss.cmd, stdin, stdout, and stderr.
 func (ss *sshSession) launchProcess(ctx context.Context) error {
-	shell := loginShell(ss.localUser.Uid)
+	shell := loginShell(ss.conn.localUser.Uid)
 	var args []string
 	if rawCmd := ss.RawCommand(); rawCmd != "" {
 		args = append(args, "-c", rawCmd)
@@ -174,10 +174,10 @@ func (ss *sshSession) launchProcess(ctx context.Context) error {
 		args = append(args, "-l") // login shell
 	}
 
-	ci := ss.connInfo
+	ci := ss.conn.info
 	cmd := ss.newIncubatorCommand(ctx, shell, args)
-	cmd.Dir = ss.localUser.HomeDir
+	cmd.Dir = ss.conn.localUser.HomeDir
-	cmd.Env = append(cmd.Env, envForUser(ss.localUser)...)
+	cmd.Env = append(cmd.Env, envForUser(ss.conn.localUser)...)
 	cmd.Env = append(cmd.Env, ss.Environ()...)
 	cmd.Env = append(cmd.Env,
 		fmt.Sprintf("SSH_CLIENT=%s %d %d", ci.src.IP(), ci.src.Port(), ci.dst.Port()),

ssh/tailssh/tailssh.go

@@ -113,33 +113,41 @@ func (srv *server) OnPolicyChange() {
 // ssh.Server.
 type conn struct {
 	*ssh.Server
-	srv *server
+
-	ci  *sshConnInfo // set by NoClientAuthCallback
+	// now is the time to consider the present moment for the
+	// purposes of rule evaluation.
+	now time.Time
+
+	action0   *tailcfg.SSHAction // first matching action
+	srv       *server
+	info      *sshConnInfo // set by setInfo
+	localUser *user.User   // set by checkAuth
 
 	insecureSkipTailscaleAuth bool // used by tests.
 }
 
 func (c *conn) logf(format string, args ...any) {
-	if c.ci == nil {
+	if c.info == nil {
 		c.srv.logf(format, args...)
 		return
 	}
-	format = fmt.Sprintf("%v: %v", c.ci.String(), format)
+	format = fmt.Sprintf("%v: %v", c.info.String(), format)
 	c.srv.logf(format, args...)
 }
 
 // PublicKeyHandler implements ssh.PublicKeyHandler is called by the the
 // ssh.Server when the client presents a public key.
-func (c *conn) PublicKeyHandler(ctx ssh.Context, key ssh.PublicKey) bool {
+func (c *conn) PublicKeyHandler(ctx ssh.Context, pubKey ssh.PublicKey) error {
-	if c.ci == nil {
+	if c.info == nil {
-		return false
+		return gossh.ErrDenied
 	}
-	if _, ok := c.srv.canProceed(c.ci, key); !ok {
+	if err := c.checkAuth(pubKey); err != nil {
-		c.logf("rejecting SSH public key %s", bytes.TrimSpace(gossh.MarshalAuthorizedKey(key)))
+		// TODO(maisem/bradfitz): surface the error here.
-		return false
+		c.logf("rejecting SSH public key %s: %v", bytes.TrimSpace(gossh.MarshalAuthorizedKey(pubKey)), err)
+		return err
 	}
-	c.logf("accepting SSH public key %s", bytes.TrimSpace(gossh.MarshalAuthorizedKey(key)))
+	c.logf("accepting SSH public key %s", bytes.TrimSpace(gossh.MarshalAuthorizedKey(pubKey)))
-	return true
+	return nil
 }
 
 // errPubKeyRequired is returned by NoClientAuthCallback to make the client
@@ -153,19 +161,51 @@ func (c *conn) NoClientAuthCallback(cm gossh.ConnMetadata) (*gossh.Permissions,
 	if c.insecureSkipTailscaleAuth {
 		return nil, nil
 	}
-	ci, err := c.srv.connInfo(cm.User(), toIPPort(cm.LocalAddr()), toIPPort(cm.RemoteAddr()))
+	if err := c.setInfo(cm); err != nil {
-	if err != nil {
 		c.logf("failed to get conninfo: %v", err)
 		return nil, gossh.ErrDenied
 	}
-	c.ci = ci
+	return nil, c.checkAuth(nil /* no pub key */)
-	if _, ok := c.srv.canProceed(ci, nil /* without public key */); ok {
+}
-		return nil, nil
+
+// checkAuth verifies that conn can proceed with the specified (optional)
+// pubKey. It returns nil if the matching policy action is Accept or
+// HoldAndDelegate. If pubKey is nil, there was no policy match but there is a
+// policy that might match a public key it returns errPubKeyRequired. Otherwise,
+// it returns gossh.ErrDenied possibly wrapped in gossh.WithBannerError.
+func (c *conn) checkAuth(pubKey ssh.PublicKey) error {
+	a, localUser, err := c.evaluatePolicy(pubKey)
+	if err != nil {
+		if pubKey == nil && c.havePubKeyPolicy(c.info) {
+			return errPubKeyRequired
+		}
+		return fmt.Errorf("%w: %v", gossh.ErrDenied, err)
 	}
-	if c.srv.havePubKeyPolicy(ci) {
+	c.action0 = a
-		return nil, errPubKeyRequired
+	if a.Accept || a.HoldAndDelegate != "" {
+		lu, err := user.Lookup(localUser)
+		if err != nil {
+			c.logf("failed to lookup %v: %v", localUser, err)
+			return gossh.WithBannerError{
+				Err:     gossh.ErrDenied,
+				Message: fmt.Sprintf("failed to lookup %v\r\n", localUser),
+			}
+		}
+		c.localUser = lu
+		return nil
+	}
+	if a.Reject {
+		err := gossh.ErrDenied
+		if a.Message != "" {
+			err = gossh.WithBannerError{
+				Err:     err,
+				Message: a.Message,
+			}
+		}
+		return err
 	}
-	return nil, gossh.ErrDenied
+	// Shouldn't get here, but:
+	return gossh.ErrDenied
 }
 
 // ServerConfig implements ssh.ServerConfigCallback.
@@ -179,10 +219,10 @@ func (c *conn) ServerConfig(ctx ssh.Context) *gossh.ServerConfig {
 }
 
 func (srv *server) newConn() (*conn, error) {
-	c := &conn{srv: srv}
+	c := &conn{srv: srv, now: srv.now()}
 	c.Server = &ssh.Server{
 		Version:           "SSH-2.0-Tailscale",
-		Handler:           srv.handleSSH,
+		Handler:           c.handleConnPostSSHAuth,
 		RequestHandlers:   map[string]ssh.RequestHandler{},
 		SubsystemHandlers: map[string]ssh.SubsystemHandler{},
 
@@ -230,22 +270,22 @@ func (srv *server) mayForwardLocalPortTo(ctx ssh.Context, destinationHost string
 
 // havePubKeyPolicy reports whether any policy rule may provide access by means
 // of a ssh.PublicKey.
-func (srv *server) havePubKeyPolicy(ci *sshConnInfo) bool {
+func (c *conn) havePubKeyPolicy(ci *sshConnInfo) bool {
 	// Is there any rule that looks like it'd require a public key for this
 	// sshUser?
-	pol, ok := srv.sshPolicy()
+	pol, ok := c.sshPolicy()
 	if !ok {
 		return false
 	}
 	for _, r := range pol.Rules {
-		if ci.ruleExpired(r) {
+		if c.ruleExpired(r) {
 			continue
 		}
 		if mapLocalUser(r.SSHUsers, ci.sshUser) == "" {
 			continue
 		}
 		for _, p := range r.Principals {
-			if len(p.PubKeys) > 0 && principalMatchesTailscaleIdentity(p, ci) {
+			if len(p.PubKeys) > 0 && c.principalMatchesTailscaleIdentity(p) {
 				return true
 			}
 		}
@@ -253,29 +293,11 @@ func (srv *server) havePubKeyPolicy(ci *sshConnInfo) bool {
 	return false
 }
 
-// canProceed reports whether ci is allowed to proceed based on policy.
-// It also returns the local user on success.
-func (srv *server) canProceed(ci *sshConnInfo, pubKey gossh.PublicKey) (lu *user.User, ok bool) {
-	a, localUser, err := srv.evaluatePolicy(ci, pubKey)
-	if err != nil {
-		return nil, false
-	}
-	if !a.Accept && a.HoldAndDelegate == "" {
-		return nil, false
-	}
-	lu, err = user.Lookup(localUser)
-	if err != nil {
-		srv.logf("%v: failed to lookup %v", ci, localUser)
-		return nil, false
-	}
-	return lu, true
-}
-
 // sshPolicy returns the SSHPolicy for current node.
 // If there is no SSHPolicy in the netmap, it returns a debugPolicy
 // if one is defined.
-func (srv *server) sshPolicy() (_ *tailcfg.SSHPolicy, ok bool) {
+func (c *conn) sshPolicy() (_ *tailcfg.SSHPolicy, ok bool) {
-	lb := srv.lb
+	lb := c.srv.lb
 	nm := lb.NetMap()
 	if nm == nil {
 		return nil, false
@@ -284,15 +306,15 @@ func (srv *server) sshPolicy() (_ *tailcfg.SSHPolicy, ok bool) {
 		return pol, true
 	}
 	if debugPolicyFile != "" {
-		srv.logf("reading debug SSH policy file: %v", debugPolicyFile)
+		c.logf("reading debug SSH policy file: %v", debugPolicyFile)
 		f, err := os.ReadFile(debugPolicyFile)
 		if err != nil {
-			srv.logf("error reading debug SSH policy file: %v", err)
+			c.logf("error reading debug SSH policy file: %v", err)
 			return nil, false
 		}
 		p := new(tailcfg.SSHPolicy)
 		if err := json.Unmarshal(f, p); err != nil {
-			srv.logf("invalid JSON in %v: %v", debugPolicyFile, err)
+			c.logf("invalid JSON in %v: %v", debugPolicyFile, err)
 			return nil, false
 		}
 		return p, true
@@ -314,40 +336,39 @@ func toIPPort(a net.Addr) (ipp netaddr.IPPort) {
 
 // connInfo returns a populated sshConnInfo from the provided arguments,
 // validating only that they represent a known Tailscale identity.
-func (srv *server) connInfo(sshUser string, localAddr, remoteAddr netaddr.IPPort) (*sshConnInfo, error) {
+func (c *conn) setInfo(cm gossh.ConnMetadata) error {
 	ci := &sshConnInfo{
-		now:                srv.now(),
+		sshUser: cm.User(),
-		fetchPublicKeysURL: srv.fetchPublicKeysURL,
+		src:     toIPPort(cm.RemoteAddr()),
-		sshUser:            sshUser,
+		dst:     toIPPort(cm.LocalAddr()),
-		src:                remoteAddr,
-		dst:                localAddr,
 	}
-	if !tsaddr.IsTailscaleIP(remoteAddr.IP()) {
+	if !tsaddr.IsTailscaleIP(ci.dst.IP()) {
-		return ci, fmt.Errorf("tailssh: rejecting non-Tailscale remote address %v", remoteAddr)
+		return fmt.Errorf("tailssh: rejecting non-Tailscale local address %v", ci.dst)
 	}
-	if !tsaddr.IsTailscaleIP(localAddr.IP()) {
+	if !tsaddr.IsTailscaleIP(ci.src.IP()) {
-		return ci, fmt.Errorf("tailssh: rejecting non-Tailscale remote address %v", localAddr)
+		return fmt.Errorf("tailssh: rejecting non-Tailscale remote address %v", ci.src)
 	}
-	node, uprof, ok := srv.lb.WhoIs(remoteAddr)
+	node, uprof, ok := c.srv.lb.WhoIs(ci.src)
 	if !ok {
-		return ci, fmt.Errorf("unknown Tailscale identity from src %v", remoteAddr)
+		return fmt.Errorf("unknown Tailscale identity from src %v", ci.src)
 	}
-
 	ci.node = node
 	ci.uprof = &uprof
-	return ci, nil
+
+	c.info = ci
+	return nil
 }
 
 // evaluatePolicy returns the SSHAction, sshConnInfo and localUser after
 // evaluating the sshUser and remoteAddr against the SSHPolicy. The remoteAddr
 // and localAddr params must be Tailscale IPs. The pubKey may be nil for "none"
 // auth.
-func (srv *server) evaluatePolicy(ci *sshConnInfo, pubKey gossh.PublicKey) (_ *tailcfg.SSHAction, localUser string, _ error) {
+func (c *conn) evaluatePolicy(pubKey gossh.PublicKey) (_ *tailcfg.SSHAction, localUser string, _ error) {
-	pol, ok := srv.sshPolicy()
+	pol, ok := c.sshPolicy()
 	if !ok {
 		return nil, "", fmt.Errorf("tailssh: rejecting connection; no SSH policy")
 	}
-	a, localUser, ok := srv.evalSSHPolicy(pol, ci, pubKey)
+	a, localUser, ok := c.evalSSHPolicy(pol, pubKey)
 	if !ok {
 		return nil, "", fmt.Errorf("tailssh: rejecting connection; no matching policy")
 	}
@@ -397,6 +418,9 @@ func (srv *server) pubKeyClient() *http.Client {
 	return http.DefaultClient
 }
 
+// fetchPublicKeysURL fetches the public keys from a URL. The strings are in the
+// the typical public key "type base64-string [comment]" format seen at e.g.
+// https://github.com/USER.keys
 func (srv *server) fetchPublicKeysURL(url string) ([]string, error) {
 	if !strings.HasPrefix(url, "https://") {
 		return nil, errors.New("invalid URL scheme")
@@ -449,56 +473,38 @@ func (srv *server) fetchPublicKeysURL(url string) ([]string, error) {
 	return lines, err
 }
 
-// handleSSH is invoked when a new SSH connection attempt is made.
+// handleConnPostSSHAuth runs an SSH session after the SSH-level authentication,
-func (srv *server) handleSSH(s ssh.Session) {
+// but not necessarily before all the Tailscale-level extra verification has
-	logf := srv.logf
+// completed.
-
+func (c *conn) handleConnPostSSHAuth(s ssh.Session) {
 	sshUser := s.User()
-	ci, err := srv.connInfo(sshUser, toIPPort(s.LocalAddr()), toIPPort(s.RemoteAddr()))
+	action, err := c.resolveTerminalAction(s)
 	if err != nil {
-		logf(err.Error())
+		c.logf("resolveTerminalAction: %v", err)
-		s.Exit(1)
-		return
-	}
-	action, lu, err := srv.resolveTerminalAction(s, ci)
-	if err != nil {
-		srv.logf("%v: resolveTerminalAction: %v", ci, err)
 		io.WriteString(s.Stderr(), "Access Denied: failed during authorization check.\r\n")
 		s.Exit(1)
 		return
 	}
 	if action.Reject || !action.Accept {
-		srv.logf("access denied for %v (%v)", ci.uprof.LoginName, ci.src.IP())
+		c.logf("access denied for %v", c.info.uprof.LoginName)
 		s.Exit(1)
 		return
 	}
 
-	ss := srv.newSSHSession(s, ci, lu)
+	ss := c.newSSHSession(s, action)
-	ss.logf("handling new SSH connection from %v (%v) to ssh-user %q", ci.uprof.LoginName, ci.src.IP(), sshUser)
+	ss.logf("handling new SSH connection from %v (%v) to ssh-user %q", c.info.uprof.LoginName, c.info.src.IP(), sshUser)
-	ss.logf("access granted for %v (%v) to ssh-user %q", ci.uprof.LoginName, ci.src.IP(), sshUser)
+	ss.logf("access granted to %v as ssh-user %q", c.info.uprof.LoginName, sshUser)
-	ss.action = action
 	ss.run()
 }
 
-// resolveTerminalAction evaluates the policy and either returns action (if it's
+// resolveTerminalAction either returns action0 (if it's Accept or Reject) or
-// Accept or Reject) or else loops, fetching new SSHActions from the control
+// else loops, fetching new SSHActions from the control plane.
-// plane.
 //
 // Any action with a Message in the chain will be printed to s.
 //
 // The returned SSHAction will be either Reject or Accept.
-func (srv *server) resolveTerminalAction(s ssh.Session, ci *sshConnInfo) (*tailcfg.SSHAction, *user.User, error) {
+func (c *conn) resolveTerminalAction(s ssh.Session) (*tailcfg.SSHAction, error) {
-	action, localUser, err := srv.evaluatePolicy(ci, s.PublicKey())
+	action := c.action0
-	if err != nil {
-		return nil, nil, err
-	}
-	var lu *user.User
-	if localUser != "" {
-		lu, err = user.Lookup(localUser)
-		if err != nil {
-			return nil, nil, err
-		}
-	}
 	// 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
@@ -510,23 +516,24 @@ func (srv *server) resolveTerminalAction(s ssh.Session, ci *sshConnInfo) (*tailc
 			io.WriteString(s.Stderr(), strings.Replace(action.Message, "\n", "\r\n", -1))
 		}
 		if action.Accept || action.Reject {
-			return action, lu, nil
+			return action, nil
 		}
 		url := action.HoldAndDelegate
 		if url == "" {
-			return nil, nil, errors.New("reached Action that lacked Accept, Reject, and HoldAndDelegate")
+			return nil, errors.New("reached Action that lacked Accept, Reject, and HoldAndDelegate")
 		}
-		url = srv.expandDelegateURL(ci, lu, url)
+		url = c.expandDelegateURL(url)
 		var err error
-		action, err = srv.fetchSSHAction(s.Context(), url)
+		action, err = c.fetchSSHAction(s.Context(), url)
 		if err != nil {
-			return nil, nil, fmt.Errorf("fetching SSHAction from %s: %w", url, err)
+			return nil, fmt.Errorf("fetching SSHAction from %s: %w", url, err)
 		}
 	}
 }
 
-func (srv *server) expandDelegateURL(ci *sshConnInfo, lu *user.User, actionURL string) string {
+func (c *conn) expandDelegateURL(actionURL string) string {
-	nm := srv.lb.NetMap()
+	nm := c.srv.lb.NetMap()
+	ci := c.info
 	var dstNodeID string
 	if nm != nil {
 		dstNodeID = fmt.Sprint(int64(nm.SelfNode.ID))
@@ -537,18 +544,18 @@ func (srv *server) expandDelegateURL(ci *sshConnInfo, lu *user.User, actionURL s
 		"$DST_NODE_IP", url.QueryEscape(ci.dst.IP().String()),
 		"$DST_NODE_ID", dstNodeID,
 		"$SSH_USER", url.QueryEscape(ci.sshUser),
-		"$LOCAL_USER", url.QueryEscape(lu.Username),
+		"$LOCAL_USER", url.QueryEscape(c.localUser.Username),
 	).Replace(actionURL)
 }
 
-func (ci *sshConnInfo) expandPublicKeyURL(pubKeyURL string) string {
+func (c *conn) expandPublicKeyURL(pubKeyURL string) string {
 	if !strings.Contains(pubKeyURL, "$") {
 		return pubKeyURL
 	}
 	var localPart string
 	var loginName string
-	if ci.uprof != nil {
+	if c.info.uprof != nil {
-		loginName = ci.uprof.LoginName
+		loginName = c.info.uprof.LoginName
 		localPart, _, _ = strings.Cut(loginName, "@")
 	}
 	return strings.NewReplacer(
@@ -565,10 +572,8 @@ type sshSession struct {
 	logf     logger.Logf
 
 	ctx           *sshContext // implements context.Context
-	srv           *server
+	conn          *conn
-	connInfo      *sshConnInfo
 	action        *tailcfg.SSHAction
-	localUser     *user.User
 	agentListener net.Listener // non-nil if agent-forwarding requested+allowed
 
 	// initialized by launchProcess:
@@ -589,24 +594,35 @@ func (ss *sshSession) vlogf(format string, args ...interface{}) {
 	}
 }
 
-func (srv *server) newSSHSession(s ssh.Session, ci *sshConnInfo, lu *user.User) *sshSession {
+func (c *conn) newSSHSession(s ssh.Session, action *tailcfg.SSHAction) *sshSession {
-	sharedID := fmt.Sprintf("%s-%02x", ci.now.UTC().Format("20060102T150405"), randBytes(5))
+	sharedID := fmt.Sprintf("%s-%02x", c.now.UTC().Format("20060102T150405"), randBytes(5))
+	c.logf("starting session: %v", sharedID)
 	return &sshSession{
-		Session:   s,
+		Session:  s,
-		idH:       s.Context().(ssh.Context).SessionID(),
+		idH:      s.Context().(ssh.Context).SessionID(),
-		sharedID:  sharedID,
+		sharedID: sharedID,
-		ctx:       newSSHContext(),
+		ctx:      newSSHContext(),
-		srv:       srv,
+		conn:     c,
-		localUser: lu,
+		logf:     logger.WithPrefix(c.srv.logf, "ssh-session("+sharedID+"): "),
-		connInfo:  ci,
+		action:   action,
-		logf:      logger.WithPrefix(srv.logf, "ssh-session("+sharedID+"): "),
 	}
 }
 
+func (c *conn) isStillValid(pubKey ssh.PublicKey) bool {
+	a, localUser, err := c.evaluatePolicy(pubKey)
+	if err != nil {
+		return false
+	}
+	if !a.Accept && a.HoldAndDelegate == "" {
+		return false
+	}
+	return c.localUser.Username == localUser
+}
+
 // checkStillValid checks that the session is still valid per the latest SSHPolicy.
 // If not, it terminates the session.
 func (ss *sshSession) checkStillValid() {
-	if lu, ok := ss.srv.canProceed(ss.connInfo, ss.PublicKey()); ok && lu.Uid == ss.localUser.Uid {
+	if ss.conn.isStillValid(ss.PublicKey()) {
 		return
 	}
 	ss.logf("session no longer valid per new SSH policy; closing")
@@ -616,10 +632,10 @@ func (ss *sshSession) checkStillValid() {
 	})
 }
 
-func (srv *server) fetchSSHAction(ctx context.Context, url string) (*tailcfg.SSHAction, error) {
+func (c *conn) fetchSSHAction(ctx context.Context, url string) (*tailcfg.SSHAction, error) {
 	ctx, cancel := context.WithTimeout(ctx, 30*time.Minute)
 	defer cancel()
-	bo := backoff.NewBackoff("fetch-ssh-action", srv.logf, 10*time.Second)
+	bo := backoff.NewBackoff("fetch-ssh-action", c.logf, 10*time.Second)
 	for {
 		if err := ctx.Err(); err != nil {
 			return nil, err
@@ -628,7 +644,7 @@ func (srv *server) fetchSSHAction(ctx context.Context, url string) (*tailcfg.SSH
 		if err != nil {
 			return nil, err
 		}
-		res, err := srv.lb.DoNoiseRequest(req)
+		res, err := c.srv.lb.DoNoiseRequest(req)
 		if err != nil {
 			bo.BackOff(ctx, err)
 			continue
@@ -639,7 +655,7 @@ func (srv *server) fetchSSHAction(ctx context.Context, url string) (*tailcfg.SSH
 			if len(body) > 1<<10 {
 				body = body[:1<<10]
 			}
-			srv.logf("fetch of %v: %s, %s", url, res.Status, body)
+			c.logf("fetch of %v: %s, %s", url, res.Status, body)
 			bo.BackOff(ctx, fmt.Errorf("unexpected status: %v", res.Status))
 			continue
 		}
@@ -647,7 +663,7 @@ func (srv *server) fetchSSHAction(ctx context.Context, url string) (*tailcfg.SSH
 		err = json.NewDecoder(res.Body).Decode(a)
 		res.Body.Close()
 		if err != nil {
-			srv.logf("invalid next SSHAction JSON from %v: %v", url, err)
+			c.logf("invalid next SSHAction JSON from %v: %v", url, err)
 			bo.BackOff(ctx, err)
 			continue
 		}
@@ -669,7 +685,7 @@ func (ss *sshSession) killProcessOnContextDone() {
 				io.WriteString(ss.Stderr(), "\r\n\r\n"+msg+"\r\n\r\n")
 			}
 		}
-		ss.logf("terminating SSH session from %v: %v", ss.connInfo.src.IP(), err)
+		ss.logf("terminating SSH session from %v: %v", ss.conn.info.src.IP(), err)
 		ss.cmd.Process.Kill()
 	})
 }
@@ -764,7 +780,7 @@ var recordSSH = envknob.Bool("TS_DEBUG_LOG_SSH")
 // It handles ss once it's been accepted and determined
 // that it should run.
 func (ss *sshSession) run() {
-	srv := ss.srv
+	srv := ss.conn.srv
 	srv.startSession(ss)
 	defer srv.endSession(ss)
 
@@ -781,7 +797,7 @@ func (ss *sshSession) run() {
 	}
 
 	logf := srv.logf
-	lu := ss.localUser
+	lu := ss.conn.localUser
 	localUser := lu.Username
 
 	if euid := os.Geteuid(); euid != 0 {
@@ -882,14 +898,6 @@ func (ss *sshSession) shouldRecord() bool {
 }
 
 type sshConnInfo struct {
-	// now is the time to consider the present moment for the
-	// purposes of rule evaluation.
-	now time.Time
-	// fetchPublicKeysURL, if non-nil, is a func to fetch the public
-	// keys of a URL. The strings are in the the typical public
-	// key "type base64-string [comment]" format seen at e.g. https://github.com/USER.keys
-	fetchPublicKeysURL func(url string) ([]string, error)
-
 	// sshUser is the requested local SSH username ("root", "alice", etc).
 	sshUser string
 
@@ -910,19 +918,17 @@ func (ci *sshConnInfo) String() string {
 	return fmt.Sprintf("%v->%v@%v", ci.src, ci.sshUser, ci.dst)
 }
 
-func (ci *sshConnInfo) ruleExpired(r *tailcfg.SSHRule) bool {
+func (c *conn) ruleExpired(r *tailcfg.SSHRule) bool {
 	if r.RuleExpires == nil {
 		return false
 	}
-	return r.RuleExpires.Before(ci.now)
+	return r.RuleExpires.Before(c.now)
 }
 
-func (srv *server) evalSSHPolicy(pol *tailcfg.SSHPolicy, ci *sshConnInfo, pubKey gossh.PublicKey) (a *tailcfg.SSHAction, localUser string, ok bool) {
+func (c *conn) evalSSHPolicy(pol *tailcfg.SSHPolicy, pubKey gossh.PublicKey) (a *tailcfg.SSHAction, localUser string, ok bool) {
 	for _, r := range pol.Rules {
-		if a, localUser, err := matchRule(r, ci, pubKey); err == nil {
+		if a, localUser, err := c.matchRule(r, pubKey); err == nil {
 			return a, localUser, true
-		} else {
-			srv.logf(err.Error())
 		}
 	}
 	return nil, "", false
@@ -937,23 +943,23 @@ var (
 	errUserMatch      = errors.New("user didn't match")
 )
 
-func matchRule(r *tailcfg.SSHRule, ci *sshConnInfo, pubKey gossh.PublicKey) (a *tailcfg.SSHAction, localUser string, err error) {
+func (c *conn) matchRule(r *tailcfg.SSHRule, pubKey gossh.PublicKey) (a *tailcfg.SSHAction, localUser string, err error) {
 	if r == nil {
 		return nil, "", errNilRule
 	}
 	if r.Action == nil {
 		return nil, "", errNilAction
 	}
-	if ci.ruleExpired(r) {
+	if c.ruleExpired(r) {
 		return nil, "", errRuleExpired
 	}
 	if !r.Action.Reject || r.SSHUsers != nil {
-		localUser = mapLocalUser(r.SSHUsers, ci.sshUser)
+		localUser = mapLocalUser(r.SSHUsers, c.info.sshUser)
 		if localUser == "" {
 			return nil, "", errUserMatch
 		}
 	}
-	if ok, err := anyPrincipalMatches(r.Principals, ci, pubKey); err != nil {
+	if ok, err := c.anyPrincipalMatches(r.Principals, pubKey); err != nil {
 		return nil, "", err
 	} else if !ok {
 		return nil, "", errPrincipalMatch
@@ -972,12 +978,12 @@ func mapLocalUser(ruleSSHUsers map[string]string, reqSSHUser string) (localUser
 	return v
 }
 
-func anyPrincipalMatches(ps []*tailcfg.SSHPrincipal, ci *sshConnInfo, pubKey gossh.PublicKey) (bool, error) {
+func (c *conn) anyPrincipalMatches(ps []*tailcfg.SSHPrincipal, pubKey gossh.PublicKey) (bool, error) {
 	for _, p := range ps {
 		if p == nil {
 			continue
 		}
-		if ok, err := principalMatches(p, ci, pubKey); err != nil {
+		if ok, err := c.principalMatches(p, pubKey); err != nil {
 			return false, err
 		} else if ok {
 			return true, nil
@@ -986,17 +992,18 @@ func anyPrincipalMatches(ps []*tailcfg.SSHPrincipal, ci *sshConnInfo, pubKey gos
 	return false, nil
 }
 
-func principalMatches(p *tailcfg.SSHPrincipal, ci *sshConnInfo, pubKey gossh.PublicKey) (bool, error) {
+func (c *conn) principalMatches(p *tailcfg.SSHPrincipal, pubKey gossh.PublicKey) (bool, error) {
-	if !principalMatchesTailscaleIdentity(p, ci) {
+	if !c.principalMatchesTailscaleIdentity(p) {
 		return false, nil
 	}
-	return principalMatchesPubKey(p, ci, pubKey)
+	return c.principalMatchesPubKey(p, pubKey)
 }
 
 // principalMatchesTailscaleIdentity reports whether one of p's four fields
 // that match the Tailscale identity match (Node, NodeIP, UserLogin, Any).
 // This function does not consider PubKeys.
-func principalMatchesTailscaleIdentity(p *tailcfg.SSHPrincipal, ci *sshConnInfo) bool {
+func (c *conn) principalMatchesTailscaleIdentity(p *tailcfg.SSHPrincipal) bool {
+	ci := c.info
 	if p.Any {
 		return true
 	}
@@ -1014,7 +1021,7 @@ func principalMatchesTailscaleIdentity(p *tailcfg.SSHPrincipal, ci *sshConnInfo)
 	return false
 }
 
-func principalMatchesPubKey(p *tailcfg.SSHPrincipal, ci *sshConnInfo, clientPubKey gossh.PublicKey) (bool, error) {
+func (c *conn) principalMatchesPubKey(p *tailcfg.SSHPrincipal, clientPubKey gossh.PublicKey) (bool, error) {
 	if len(p.PubKeys) == 0 {
 		return true, nil
 	}
@@ -1023,11 +1030,8 @@ func principalMatchesPubKey(p *tailcfg.SSHPrincipal, ci *sshConnInfo, clientPubK
 	}
 	knownKeys := p.PubKeys
 	if len(knownKeys) == 1 && strings.HasPrefix(knownKeys[0], "https://") {
-		if ci.fetchPublicKeysURL == nil {
-			return false, fmt.Errorf("no public key fetcher")
-		}
 		var err error
-		knownKeys, err = ci.fetchPublicKeysURL(ci.expandPublicKeyURL(knownKeys[0]))
+		knownKeys, err = c.srv.fetchPublicKeysURL(c.expandPublicKeyURL(knownKeys[0]))
 		if err != nil {
 			return false, err
 		}
@@ -1081,7 +1085,7 @@ func (ss *sshSession) startNewRecording() (*recording, error) {
 		ss:    ss,
 		start: now,
 	}
-	varRoot := ss.srv.lb.TailscaleVarRoot()
+	varRoot := ss.conn.srv.lb.TailscaleVarRoot()
 	if varRoot == "" {
 		return nil, errors.New("no var root for recording storage")
 	}

ssh/tailssh/tailssh_test.go

@@ -63,7 +63,7 @@ func TestMatchRule(t *testing.T) {
 				Action:      someAction,
 				RuleExpires: timePtr(time.Unix(100, 0)),
 			},
-			ci:      &sshConnInfo{now: time.Unix(200, 0)},
+			ci:      &sshConnInfo{},
 			wantErr: errRuleExpired,
 		},
 		{
@@ -178,7 +178,11 @@ func TestMatchRule(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got, gotUser, err := matchRule(tt.rule, tt.ci, nil)
+			c := &conn{
+				now:  time.Unix(200, 0),
+				info: tt.ci,
+			}
+			got, gotUser, err := c.matchRule(tt.rule, nil)
 			if err != tt.wantErr {
 				t.Errorf("err = %v; want %v", err, tt.wantErr)
 			}
@@ -226,8 +230,8 @@ func TestSSH(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-
+	sc.localUser = u
-	ci := &sshConnInfo{
+	sc.info = &sshConnInfo{
 		sshUser: "test",
 		src:     netaddr.MustParseIPPort("1.2.3.4:32342"),
 		dst:     netaddr.MustParseIPPort("1.2.3.5:22"),
@@ -236,9 +240,7 @@ func TestSSH(t *testing.T) {
 	}
 
 	sc.Handler = func(s ssh.Session) {
-		ss := srv.newSSHSession(s, ci, u)
+		sc.newSSHSession(s, &tailcfg.SSHAction{Accept: true}).run()
-		ss.action = &tailcfg.SSHAction{Accept: true}
-		ss.run()
 	}
 
 	ln, err := net.Listen("tcp4", "127.0.0.1:0")
@@ -408,21 +410,24 @@ func TestPublicKeyFetching(t *testing.T) {
 }
 
 func TestExpandPublicKeyURL(t *testing.T) {
-	ci := &sshConnInfo{
+	c := &conn{
-		uprof: &tailcfg.UserProfile{
+		info: &sshConnInfo{
-			LoginName: "bar@baz.tld",
+			uprof: &tailcfg.UserProfile{
+				LoginName: "bar@baz.tld",
+			},
 		},
 	}
-	if got, want := ci.expandPublicKeyURL("foo"), "foo"; got != want {
+	if got, want := c.expandPublicKeyURL("foo"), "foo"; got != want {
 		t.Errorf("basic: got %q; want %q", got, want)
 	}
-	if got, want := ci.expandPublicKeyURL("https://example.com/$LOGINNAME_LOCALPART.keys"), "https://example.com/bar.keys"; got != want {
+	if got, want := c.expandPublicKeyURL("https://example.com/$LOGINNAME_LOCALPART.keys"), "https://example.com/bar.keys"; got != want {
 		t.Errorf("localpart: got %q; want %q", got, want)
 	}
-	if got, want := ci.expandPublicKeyURL("https://example.com/keys?email=$LOGINNAME_EMAIL"), "https://example.com/keys?email=bar@baz.tld"; got != want {
+	if got, want := c.expandPublicKeyURL("https://example.com/keys?email=$LOGINNAME_EMAIL"), "https://example.com/keys?email=bar@baz.tld"; got != want {
 		t.Errorf("email: got %q; want %q", got, want)
 	}
-	if got, want := new(sshConnInfo).expandPublicKeyURL("https://example.com/keys?email=$LOGINNAME_EMAIL"), "https://example.com/keys?email="; got != want {
+	c.info = new(sshConnInfo)
+	if got, want := c.expandPublicKeyURL("https://example.com/keys?email=$LOGINNAME_EMAIL"), "https://example.com/keys?email="; got != want {
 		t.Errorf("on empty: got %q; want %q", got, want)
 	}
 }

tempfork/gliderlabs/ssh/example_test.go

@@ -1,6 +1,7 @@
 package ssh_test
 
 import (
+	"errors"
 	"io"
 	"io/ioutil"
 
@@ -27,10 +28,19 @@ func ExampleNoPty() {
 
 func ExamplePublicKeyAuth() {
 	ssh.ListenAndServe(":2222", nil,
-		ssh.PublicKeyAuth(func(ctx ssh.Context, key ssh.PublicKey) bool {
+		ssh.PublicKeyAuth(func(ctx ssh.Context, key ssh.PublicKey) error {
-			data, _ := ioutil.ReadFile("/path/to/allowed/key.pub")
+			data, err := ioutil.ReadFile("/path/to/allowed/key.pub")
-			allowed, _, _, _, _ := ssh.ParseAuthorizedKey(data)
+			if err != nil {
-			return ssh.KeysEqual(key, allowed)
+				return err
+			}
+			allowed, _, _, _, err := ssh.ParseAuthorizedKey(data)
+			if err != nil {
+				return err
+			}
+			if !ssh.KeysEqual(key, allowed) {
+				return errors.New("some error")
+			}
+			return nil
 		}),
 	)
 }

tempfork/gliderlabs/ssh/server.go

@@ -144,8 +144,8 @@ func (srv *Server) config(ctx Context) *gossh.ServerConfig {
 	if srv.PublicKeyHandler != nil {
 		config.PublicKeyCallback = func(conn gossh.ConnMetadata, key gossh.PublicKey) (*gossh.Permissions, error) {
 			applyConnMetadata(ctx, conn)
-			if ok := srv.PublicKeyHandler(ctx, key); !ok {
+			if err := srv.PublicKeyHandler(ctx, key); err != nil {
-				return ctx.Permissions().Permissions, fmt.Errorf("permission denied")
+				return ctx.Permissions().Permissions, err
 			}
 			ctx.SetValue(ContextKeyPublicKey, key)
 			return ctx.Permissions().Permissions, nil

tempfork/gliderlabs/ssh/ssh.go

@@ -36,7 +36,7 @@ type Option func(*Server) error
 type Handler func(Session)
 
 // PublicKeyHandler is a callback for performing public key authentication.
-type PublicKeyHandler func(ctx Context, key PublicKey) bool
+type PublicKeyHandler func(ctx Context, key PublicKey) error
 
 // PasswordHandler is a callback for performing password authentication.
 type PasswordHandler func(ctx Context, password string) bool