Skip to content

ssh: use errors.As to detect PartialSuccessError wrapped in BannerError#353

Open
XananasX7 wants to merge 1 commit into
golang:masterfrom
XananasX7:ssh-partial-success-errors-as
Open

ssh: use errors.As to detect PartialSuccessError wrapped in BannerError#353
XananasX7 wants to merge 1 commit into
golang:masterfrom
XananasX7:ssh-partial-success-errors-as

Conversation

@XananasX7
Copy link
Copy Markdown

Summary

The authentication loop in newServerConn used a direct type assertion
authErr.(*PartialSuccessError) to detect partial-success responses from
authentication callbacks. This silently fails when a callback returns a
*BannerError that wraps a *PartialSuccessError.

Background

BannerError has an Err field and implements Unwrap(), making it a
standard Go error-wrapping type. The PartialSuccessError doc says it "can
be returned by any of the ServerConfig authentication callbacks" — the
same callbacks that may also return BannerError. It is therefore valid and
expected for a callback to return:

return nil, &BannerError{
    Message: "First factor accepted; please provide second factor.",
    Err:     &PartialSuccessError{Next: ServerAuthCallbacks{...}},
}

This allows a callback to simultaneously send a banner message to the client
and signal partial success requiring a further authentication step.

Bug

With the old direct type assertion:

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

When authErr is a *BannerError (wrapping a *PartialSuccessError), ok
is false. The partial-success state is lost: authFailures is incremented,
the authConfig is not updated to partialSuccess.Next, and the client
receives SSH_MSG_USERAUTH_FAILURE instead of SSH_MSG_USERAUTH_PARTIAL_SUCCESS.

The banner-extraction code on the lines immediately above (lines 934-940)
already handles exactly this pattern correctly using errors.As:

var bannerErr *BannerError
if errors.As(authErr, &bannerErr) {
    ...
}

Fix

Replace the three direct *PartialSuccessError type assertions with
errors.As, matching the pattern already used for *BannerError:

  1. Main auth loop (authErr path, line ~949): the primary fix — ensures
    partialSuccess.Next is used to update authConfig even when wrapped.

  2. PublicKeyCallback cache, first check (line ~782): ensures the
    "invalid library usage" detection works when wrapped.

  3. PublicKeyCallback cache, isQuery check (line ~807): ensures the
    public-key query response (SSH_MSG_USERAUTH_PK_OK) is sent correctly
    when the callback result wraps a PartialSuccessError.

Test

A new test TestPartialSuccessErrorWrappedInBannerError is added to
server_multi_auth_test.go that:

  • Sets up a PasswordCallback returning &BannerError{Err: &PartialSuccessError{...}}
  • Verifies the client can authenticate successfully in two password steps
  • Confirms the test fails without the fix (auth fails with "no supported
    methods remain" because the partial-success state is discarded)

Relation to Previous Fix

This is the symmetric counterpart to the *BannerError errors.As fix
already applied in the codebase: that fix ensures a BannerError wrapped
in another error is detected; this fix ensures a PartialSuccessError
wrapped in a BannerError is detected.

@google-cla
Copy link
Copy Markdown

google-cla Bot commented Jun 3, 2026

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

The auth loop in newServerConn used a direct type assertion
(authErr.(*PartialSuccessError)) to detect partial-success authentication
results. This fails when a callback returns a *BannerError that wraps a
*PartialSuccessError, because the outer error is a *BannerError not a
*PartialSuccessError.

The banner-extraction code immediately above (lines 934-940) already
uses errors.As correctly for *BannerError. Apply the same treatment to
the *PartialSuccessError check: replace the direct type assertion with
errors.As so that wrapped PartialSuccessErrors are also handled.

The same fix is applied to the two candidate.result type assertions in
the PublicKeyCallback cache path, which can receive errors from the same
callbacks and is subject to the same wrapping pattern.

Without this fix, a callback returning &BannerError{Err: &PartialSuccessError{...}}
would cause the partial-success response to be silently treated as an
authentication failure, incrementing authFailures and sending the client
a failure message instead of a partial-success message.

Fixes golang/go#79809
@XananasX7 XananasX7 force-pushed the ssh-partial-success-errors-as branch from 42022f0 to 33e1572 Compare June 3, 2026 19:54
@XananasX7
Copy link
Copy Markdown
Author

Fixes golang/go#79809

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant