ssh/tailssh: display more useful error messages when authentication fails

Also add a trailing newline to error banners so that SSH client messages don't print on the same line.

Updates tailscale/corp#29138

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

ssh/tailssh/tailssh.go

@@ -281,7 +281,7 @@ func (c *conn) errBanner(message string, err error) error {
 	if err != nil {
 		c.logf("%s: %s", message, err)
 	}
-	if err := c.spac.SendAuthBanner("tailscale: " + message); err != nil {
+	if err := c.spac.SendAuthBanner("tailscale: " + message + "\n"); err != nil {
 		c.logf("failed to send auth banner: %s", err)
 	}
 	return errTerminal
@@ -324,9 +324,16 @@ func (c *conn) clientAuth(cm gossh.ConnMetadata) (perms *gossh.Permissions, retE
 		return nil, c.errBanner("failed to get connection info", err)
 	}
 
-	action, localUser, acceptEnv, err := c.evaluatePolicy()
-	if err != nil {
-		return nil, c.errBanner("failed to evaluate SSH policy", err)
+	action, localUser, acceptEnv, result := c.evaluatePolicy()
+	switch result {
+	case accepted:
+		// do nothing
+	case rejectedUser:
+		return nil, c.errBanner(fmt.Sprintf("tailnet policy does not permit you to SSH as user %q", c.info.sshUser), nil)
+	case rejected, noPolicy:
+		return nil, c.errBanner("tailnet policy does not permit you to SSH to this node", fmt.Errorf("failed to evaluate policy, result: %s", result))
+	default:
+		return nil, c.errBanner("failed to evaluate tailnet policy", fmt.Errorf("failed to evaluate policy, result: %s", result))
 	}
 
 	c.action0 = action
@@ -597,18 +604,23 @@ func (c *conn) setInfo(cm gossh.ConnMetadata) error {
 	return nil
 }
 
+type evalResult string
+
+const (
+	noPolicy     evalResult = "no policy"
+	rejected     evalResult = "rejected"
+	rejectedUser evalResult = "rejected user"
+	accepted     evalResult = "accept"
+)
+
 // evaluatePolicy returns the SSHAction and localUser after evaluating
 // the SSHPolicy for this conn.
-func (c *conn) evaluatePolicy() (_ *tailcfg.SSHAction, localUser string, acceptEnv []string, _ error) {
+func (c *conn) evaluatePolicy() (_ *tailcfg.SSHAction, localUser string, acceptEnv []string, result evalResult) {
 	pol, ok := c.sshPolicy()
 	if !ok {
-		return nil, "", nil, fmt.Errorf("tailssh: rejecting connection; no SSH policy")
-	}
-	a, localUser, acceptEnv, ok := c.evalSSHPolicy(pol)
-	if !ok {
-		return nil, "", nil, fmt.Errorf("tailssh: rejecting connection; no matching policy")
+		return nil, "", nil, noPolicy
 	}
-	return a, localUser, acceptEnv, nil
+	return c.evalSSHPolicy(pol)
 }
 
 // handleSessionPostSSHAuth runs an SSH session after the SSH-level authentication,
@@ -706,9 +718,9 @@ func (c *conn) newSSHSession(s ssh.Session) *sshSession {
 
 // isStillValid reports whether the conn is still valid.
 func (c *conn) isStillValid() bool {
-	a, localUser, _, err := c.evaluatePolicy()
-	c.vlogf("stillValid: %+v %v %v", a, localUser, err)
-	if err != nil {
+	a, localUser, _, result := c.evaluatePolicy()
+	c.vlogf("stillValid: %+v %v %v", a, localUser, result)
+	if result != accepted {
 		return false
 	}
 	if !a.Accept && a.HoldAndDelegate == "" {
@@ -1089,13 +1101,20 @@ func (c *conn) ruleExpired(r *tailcfg.SSHRule) bool {
 	return r.RuleExpires.Before(c.srv.now())
 }
 
-func (c *conn) evalSSHPolicy(pol *tailcfg.SSHPolicy) (a *tailcfg.SSHAction, localUser string, acceptEnv []string, ok bool) {
+func (c *conn) evalSSHPolicy(pol *tailcfg.SSHPolicy) (a *tailcfg.SSHAction, localUser string, acceptEnv []string, result evalResult) {
+	failedOnUser := false
 	for _, r := range pol.Rules {
 		if a, localUser, acceptEnv, err := c.matchRule(r); err == nil {
-			return a, localUser, acceptEnv, true
+			return a, localUser, acceptEnv, accepted
+		} else if errors.Is(err, errUserMatch) {
+			failedOnUser = true
 		}
 	}
-	return nil, "", nil, false
+	result = rejected
+	if failedOnUser {
+		result = rejectedUser
+	}
+	return nil, "", nil, result
 }
 
 // internal errors for testing; they don't escape to callers or logs.
@@ -1129,6 +1148,9 @@ func (c *conn) matchRule(r *tailcfg.SSHRule) (a *tailcfg.SSHAction, localUser st
 	if c.ruleExpired(r) {
 		return nil, "", nil, errRuleExpired
 	}
+	if !c.anyPrincipalMatches(r.Principals) {
+		return nil, "", nil, errPrincipalMatch
+	}
 	if !r.Action.Reject {
 		// For all but Reject rules, SSHUsers is required.
 		// If SSHUsers is nil or empty, mapLocalUser will return an
@@ -1138,9 +1160,6 @@ func (c *conn) matchRule(r *tailcfg.SSHRule) (a *tailcfg.SSHAction, localUser st
 			return nil, "", nil, errUserMatch
 		}
 	}
-	if !c.anyPrincipalMatches(r.Principals) {
-		return nil, "", nil, errPrincipalMatch
-	}
 	return r.Action, localUser, r.AcceptEnv, nil
 }
 

ssh/tailssh/tailssh_test.go

@@ -253,7 +253,7 @@ func TestEvalSSHPolicy(t *testing.T) {
 		name          string
 		policy        *tailcfg.SSHPolicy
 		ci            *sshConnInfo
-		wantMatch     bool
+		wantResult    evalResult
 		wantUser      string
 		wantAcceptEnv []string
 	}{
@@ -299,10 +299,20 @@ func TestEvalSSHPolicy(t *testing.T) {
 			ci:            &sshConnInfo{sshUser: "alice"},
 			wantUser:      "thealice",
 			wantAcceptEnv: []string{"EXAMPLE", "?_?", "TEST_*"},
-			wantMatch:     true,
+			wantResult:    accepted,
 		},
 		{
-			name: "no-matches-returns-failure",
+			name: "no-matches-returns-rejected",
+			policy: &tailcfg.SSHPolicy{
+				Rules: []*tailcfg.SSHRule{},
+			},
+			ci:            &sshConnInfo{sshUser: "alice"},
+			wantUser:      "",
+			wantAcceptEnv: nil,
+			wantResult:    rejected,
+		},
+		{
+			name: "no-user-matches-returns-rejected-user",
 			policy: &tailcfg.SSHPolicy{
 				Rules: []*tailcfg.SSHRule{
 					{
@@ -340,7 +350,7 @@ func TestEvalSSHPolicy(t *testing.T) {
 			ci:            &sshConnInfo{sshUser: "alice"},
 			wantUser:      "",
 			wantAcceptEnv: nil,
-			wantMatch:     false,
+			wantResult:    rejectedUser,
 		},
 	}
 	for _, tt := range tests {
@@ -349,14 +359,14 @@ func TestEvalSSHPolicy(t *testing.T) {
 				info: tt.ci,
 				srv:  &server{logf: tstest.WhileTestRunningLogger(t)},
 			}
-			got, gotUser, gotAcceptEnv, match := c.evalSSHPolicy(tt.policy)
-			if match != tt.wantMatch {
-				t.Errorf("match = %v; want %v", match, tt.wantMatch)
+			got, gotUser, gotAcceptEnv, result := c.evalSSHPolicy(tt.policy)
+			if result != tt.wantResult {
+				t.Errorf("result = %v; want %v", result, tt.wantResult)
 			}
 			if gotUser != tt.wantUser {
 				t.Errorf("user = %q; want %q", gotUser, tt.wantUser)
 			}
-			if tt.wantMatch == true && got == nil {
+			if tt.wantResult == accepted && got == nil {
 				t.Errorf("expected non-nil action on success")
 			}
 			if !slices.Equal(gotAcceptEnv, tt.wantAcceptEnv) {
@@ -467,7 +477,7 @@ func (ts *localState) NodeKey() key.NodePublic {
 func newSSHRule(action *tailcfg.SSHAction) *tailcfg.SSHRule {
 	return &tailcfg.SSHRule{
 		SSHUsers: map[string]string{
-			"*": currentUser,
+			"alice": currentUser,
 		},
 		Action: action,
 		Principals: []*tailcfg.SSHPrincipal{
@@ -789,6 +799,11 @@ func TestSSHAuthFlow(t *testing.T) {
 		Accept:  true,
 		Message: "Welcome to Tailscale SSH!",
 	})
+	bobRule := newSSHRule(&tailcfg.SSHAction{
+		Accept:  true,
+		Message: "Welcome to Tailscale SSH!",
+	})
+	bobRule.SSHUsers = map[string]string{"bob": "bob"}
 	rejectRule := newSSHRule(&tailcfg.SSHAction{
 		Reject:  true,
 		Message: "Go Away!",
@@ -808,7 +823,16 @@ func TestSSHAuthFlow(t *testing.T) {
 				sshEnabled: true,
 			},
 			authErr:     true,
-			wantBanners: []string{"tailscale: failed to evaluate SSH policy"},
+			wantBanners: []string{"tailscale: tailnet policy does not permit you to SSH to this node\n"},
+		},
+		{
+			name: "user-mismatch",
+			state: &localState{
+				sshEnabled:   true,
+				matchingRule: bobRule,
+			},
+			authErr:     true,
+			wantBanners: []string{`tailscale: tailnet policy does not permit you to SSH as user "alice"` + "\n"},
 		},
 		{
 			name: "accept",