ssh: use errors.As for PartialSuccessError unwrapping in auth loop#352
ssh: use errors.As for PartialSuccessError unwrapping in auth loop#352XananasX7 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. |
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
XananasX7
force-pushed
the
fix/ssh-banner-partial-success-unwrap
branch
from
20f739d to
4fe8209
Compare
|
Rewrote the commit with |
|
This PR (HEAD: 4fe8209) has been imported to Gerrit for code review. Please visit Gerrit at https://go-review.googlesource.com/c/crypto/+/790420. Important tips:
|
|
Message from Gopher Robot: Patch Set 1: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/790420. |
|
Updated the PR description to address the Gerrit bot feedback: removed all Markdown formatting (backticks, bold, code fences), wrapped all lines to ≤76 characters, and added 'Updates golang/go#79809' as the bug reference. |
|
Message from Ian Lance Taylor: Patch Set 2: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/790420. |
2 participants
ssh: use errors.As for PartialSuccessError unwrapping in auth loop
The authentication loop in serverAuthenticate detected
*PartialSuccessError using a direct type assertion:
This is inconsistent with how *BannerError is detected in the same
loop, which already uses errors.As to unwrap error chains. When an
authentication callback returns a BannerError wrapping a
PartialSuccessError -- a natural combination when a server wants to
display a banner and require a second authentication factor -- two
bugs occur:
Bug 1 (public-key query path, isQuery=true):
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 partial success is never surfaced to the
outer auth loop.
Bug 2 (main result path):
The main result check also uses a direct type assertion and misses the
wrapped value, incrementing authFailures and sending an ordinary
failure reply instead of a partial-success reply.
Net effect: a server callback returning
&BannerError{Err: &PartialSuccessError{Next: ...}, Message: "..."}
will have the banner sent correctly but the multi-factor continuation
silently dropped.
This change replaces both direct type assertions with errors.As calls,
matching the pattern already used for BannerError.
A new test TestBannerWrappingPartialSuccess is added to
server_multi_auth_test.go. It configures a server whose
PublicKeyCallback returns BannerError wrapping PartialSuccessError,
then verifies the banner is delivered, the client completes the
password second factor, and the connection is established. The test
fails on unpatched code and passes with this change.
Updates golang/go#79809