From 20f739d6ac2652977c071971c6e9ce42760c89d7 Mon Sep 17 00:00:00 2001
From: XananasX7 <xananax7@users.noreply.github.com>
Date: Wed, 3 Jun 2026 18:58:39 +0000
Subject: [PATCH] ssh: use errors.As for PartialSuccessError unwrapping in auth
 loop

The authentication loop in serverAuthenticate used a direct type assertion
to detect *PartialSuccessError:

    if partialSuccess, ok := authErr.(*PartialSuccessError); ok {

This is inconsistent with how *BannerError is detected in the same loop,
which correctly uses errors.As to unwrap the error chain.

When an authentication callback returns a BannerError wrapping a
PartialSuccessError, two bugs appear:

1. In the isQuery (public-key probe) path, the wrapped PartialSuccessError
   is not recognised, so the server never sends SSH_MSG_USERAUTH_PK_OK.
   The client therefore never sends the actual signature, and the
   PartialSuccess is never surfaced to the authentication loop at all.

2. In the main result path, the wrapped PartialSuccessError is similarly
   missed, so authFailures is incremented instead of switching authConfig
   to the Next callbacks and sending a partial-success reply.

Replace both direct type assertions with errors.As calls, consistent with
the existing BannerError handling, and add a regression test.

Change-Id: fix-banner-partial-success-unwrap
---
 ssh/server.go                 |  6 ++-
 ssh/server_multi_auth_test.go | 76 +++++++++++++++++++++++++++++++++++
 2 files changed, 80 insertions(+), 2 deletions(-)

diff --git a/ssh/server.go b/ssh/server.go
index 3c0fcc953e..c01a8f59c6 100644
--- a/ssh/server.go
+++ b/ssh/server.go
@@ -804,7 +804,8 @@ userAuthLoop:
 				if len(payload) > 0 {
 					return nil, parseError(msgUserAuthRequest)
 				}
-				_, isPartialSuccessError := candidate.result.(*PartialSuccessError)
+				var _partialSuccessForQuery *PartialSuccessError
+				isPartialSuccessError := errors.As(candidate.result, &_partialSuccessForQuery)
 				if candidate.result == nil || isPartialSuccessError {
 					okMsg := userAuthPubKeyOkMsg{
 						Algo:   algo,
@@ -946,7 +947,8 @@ userAuthLoop:
 
 		var failureMsg userAuthFailureMsg
 
-		if partialSuccess, ok := authErr.(*PartialSuccessError); ok {
+		var partialSuccess *PartialSuccessError
+		if errors.As(authErr, &partialSuccess) {
 			// Permissions are not preserved between authentication steps. To
 			// avoid confusion about the final state of the connection, we
 			// disallow returning non-nil Permissions combined with
diff --git a/ssh/server_multi_auth_test.go b/ssh/server_multi_auth_test.go
index 3b39802437..0722b8a3cd 100644
--- a/ssh/server_multi_auth_test.go
+++ b/ssh/server_multi_auth_test.go
@@ -410,3 +410,79 @@ func TestDynamicAuthCallbacks(t *testing.T) {
 		t.Fatal("server not returned partial success")
 	}
 }
+
+// TestBannerWrappingPartialSuccess verifies that a PartialSuccessError wrapped
+// inside a BannerError is correctly processed: the banner is sent and the
+// partial success is honoured (i.e. the Next auth callbacks are used for the
+// second step). Prior to the fix, the direct type-assertion
+// authErr.(*PartialSuccessError) missed the wrapped value while
+// errors.As(authErr, &bannerErr) had already unwrapped it, causing the
+// PartialSuccess to be silently discarded and authFailures to be incremented
+// instead.
+func TestBannerWrappingPartialSuccess(t *testing.T) {
+	var bannerReceived string
+
+	serverConfig := &ServerConfig{
+		PublicKeyCallback: func(conn ConnMetadata, key PublicKey) (*Permissions, error) {
+			if !bytes.Equal(key.Marshal(), testPublicKeys["rsa"].Marshal()) {
+				return nil, errors.New("unknown key")
+			}
+			// Return BannerError wrapping PartialSuccessError.
+			// The server wants to send a banner AND require a second factor.
+			return nil, &BannerError{
+				Err: &PartialSuccessError{
+					Next: ServerAuthCallbacks{
+						PasswordCallback: func(conn ConnMetadata, password []byte) (*Permissions, error) {
+							if string(password) == clientPassword {
+								return &Permissions{}, nil
+							}
+							return nil, errors.New("wrong password")
+						},
+					},
+				},
+				Message: "Two-factor auth required\n",
+			}
+		},
+		BannerCallback: func(conn ConnMetadata) string { return "" },
+	}
+
+	clientConfig := &ClientConfig{
+		User: "testuser",
+		Auth: []AuthMethod{
+			PublicKeys(testSigners["rsa"]),
+			Password(clientPassword),
+		},
+		HostKeyCallback:   InsecureIgnoreHostKey(),
+		BannerCallback: func(msg string) error {
+			bannerReceived = msg
+			return nil
+		},
+	}
+
+	serverAuthErrors, err := doClientServerAuth(t, serverConfig, clientConfig)
+	if err != nil {
+		t.Fatalf("expected successful login, got: %v", err)
+	}
+
+	// Verify banner was sent to the client.
+	if bannerReceived != "Two-factor auth required\n" {
+		t.Errorf("banner not received; got %q", bannerReceived)
+	}
+
+	// Auth log entries:
+	// 0: ErrNoAuth from the initial 'none' attempt (always happens)
+	// 1: BannerError wrapping PartialSuccessError (publickey step)
+	// 2: nil (password step succeeds)
+	if len(serverAuthErrors) != 3 {
+		t.Fatalf("expected 3 auth log entries, got %d: %v", len(serverAuthErrors), serverAuthErrors)
+	}
+	// The second entry wraps a *PartialSuccessError inside a *BannerError.
+	var partialSuccessErr *PartialSuccessError
+	if !errors.As(serverAuthErrors[1], &partialSuccessErr) {
+		t.Errorf("second auth log entry should wrap *PartialSuccessError, got %T: %v",
+			serverAuthErrors[1], serverAuthErrors[1])
+	}
+	if serverAuthErrors[2] != nil {
+		t.Errorf("third auth log entry should be nil (success), got: %v", serverAuthErrors[2])
+	}
+}