ssh: use errors.As to detect PartialSuccessError wrapped in BannerError#353
ssh: use errors.As to detect PartialSuccessError wrapped in BannerError#353XananasX7 wants to merge 1 commit into
Conversation
|
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. |
42022f0 to
33e1572
Compare
|
Fixes golang/go#79809 |
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
33e1572 to
1f9a96a
Compare
|
Rewrote the commit with |
|
This PR (HEAD: 1f9a96a) has been imported to Gerrit for code review. Please visit Gerrit at https://go-review.googlesource.com/c/crypto/+/790421. Important tips:
|
|
Message from Gopher Robot: Patch Set 1: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/790421. |
|
Updated the PR description to address the Gerrit bot feedback: removed all Markdown formatting (backticks, bold, headers, code fences), wrapped all lines to ≤76 characters, and added 'Updates golang/go#79809' as the bug reference. |
ssh: use errors.As to detect PartialSuccessError wrapped in BannerError
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.
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:
This allows a callback to simultaneously send a banner message to the
client and signal partial success requiring a further auth step.
With the old direct type assertion, when authErr is a *BannerError
wrapping a *PartialSuccessError, ok is false. The partial-success state
is lost: authFailures is incremented, 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 immediately above already handles this
pattern correctly using errors.As for *BannerError.
This change replaces the three direct *PartialSuccessError type
assertions with errors.As, matching that existing pattern:
partialSuccess.Next updates authConfig even when wrapped.
usage" detection works when wrapped.
response (SSH_MSG_USERAUTH_PK_OK) is sent correctly when the
callback result wraps a PartialSuccessError.
A new test TestPartialSuccessErrorWrappedInBannerError is added to
server_multi_auth_test.go. It sets up a PasswordCallback returning
&BannerError{Err: &PartialSuccessError{...}}, verifies the client can
authenticate successfully in two password steps, and confirms the test
fails without the fix.
Updates golang/go#79809