Skip to content

Commit

Permalink
ssh: fix support for partial success authentication responses in client
Browse files Browse the repository at this point in the history 
The existing client side authentication does not handle correctly
the partial success flag in SSH_MSG_USERAUTH_FAILURE authentication
responses.

This commit fixes two problems in ssh library:
1) RetryableAuthMethod() now breaks out from the retry loop and
   returns  when underlying auth method fails with partial success
   set to true.
2) Book keeping of tried (and failed) auth methods in
   clientAuthenticate() does not mark an auth method failed if it
   fails with partial success set to true.

Fixes golang/go#23461

Change-Id: Ib2e1a1d54bfe2549496199bb2f66ebbce58d130d
Reviewed-on: https://go-review.googlesource.com/88035
Reviewed-by: Han-Wen Nienhuys <hanwen@google.com>
Run-TryBot: Han-Wen Nienhuys <hanwen@google.com>
  • Loading branch information
@samiponkanen @hanwen
samiponkanen authored and hanwen committed Feb 8, 2018
1 parent 771b178 commit a66270f
Show file tree
Hide file tree
Showing 5 changed files with 450 additions and 57 deletions.
95 changes: 55 additions & 40 deletions client_auth.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,14 @@ import (
"io"
)

type authResult int

const (
authFailure authResult = iota
authPartialSuccess
authSuccess
)

// clientAuthenticate authenticates with the remote server. See RFC 4252.
func (c *connection) clientAuthenticate(config *ClientConfig) error {
// initiate user auth session
Expand All @@ -37,11 +45,12 @@ func (c *connection) clientAuthenticate(config *ClientConfig) error {
if err != nil {
return err
}
if ok {
if ok == authSuccess {
// success
return nil
} else if ok == authFailure {
tried[auth.method()] = true
}
tried[auth.method()] = true
if methods == nil {
methods = lastMethods
}
Expand Down Expand Up @@ -82,7 +91,7 @@ type AuthMethod interface {
// If authentication is not successful, a []string of alternative
// method names is returned. If the slice is nil, it will be ignored
// and the previous set of possible methods will be reused.
auth(session []byte, user string, p packetConn, rand io.Reader) (bool, []string, error)
auth(session []byte, user string, p packetConn, rand io.Reader) (authResult, []string, error)

// method returns the RFC 4252 method name.
method() string
Expand All @@ -91,13 +100,13 @@ type AuthMethod interface {
// "none" authentication, RFC 4252 section 5.2.
type noneAuth int

func (n *noneAuth) auth(session []byte, user string, c packetConn, rand io.Reader) (bool, []string, error) {
func (n *noneAuth) auth(session []byte, user string, c packetConn, rand io.Reader) (authResult, []string, error) {
if err := c.writePacket(Marshal(&userAuthRequestMsg{
User: user,
Service: serviceSSH,
Method: "none",
})); err != nil {
return false, nil, err
return authFailure, nil, err
}

return handleAuthResponse(c)
Expand All @@ -111,7 +120,7 @@ func (n *noneAuth) method() string {
// a function call, e.g. by prompting the user.
type passwordCallback func() (password string, err error)

func (cb passwordCallback) auth(session []byte, user string, c packetConn, rand io.Reader) (bool, []string, error) {
func (cb passwordCallback) auth(session []byte, user string, c packetConn, rand io.Reader) (authResult, []string, error) {
type passwordAuthMsg struct {
User string `sshtype:"50"`
Service string
Expand All @@ -125,7 +134,7 @@ func (cb passwordCallback) auth(session []byte, user string, c packetConn, rand
// The program may only find out that the user doesn't have a password
// when prompting.
if err != nil {
return false, nil, err
return authFailure, nil, err
}

if err := c.writePacket(Marshal(&passwordAuthMsg{
Expand All @@ -135,7 +144,7 @@ func (cb passwordCallback) auth(session []byte, user string, c packetConn, rand
Reply: false,
Password: pw,
})); err != nil {
return false, nil, err
return authFailure, nil, err
}

return handleAuthResponse(c)
Expand Down Expand Up @@ -178,21 +187,21 @@ func (cb publicKeyCallback) method() string {
return "publickey"
}

func (cb publicKeyCallback) auth(session []byte, user string, c packetConn, rand io.Reader) (bool, []string, error) {
func (cb publicKeyCallback) auth(session []byte, user string, c packetConn, rand io.Reader) (authResult, []string, error) {
// Authentication is performed by sending an enquiry to test if a key is
// acceptable to the remote. If the key is acceptable, the client will
// attempt to authenticate with the valid key. If not the client will repeat
// the process with the remaining keys.

signers, err := cb()
if err != nil {
return false, nil, err
return authFailure, nil, err
}
var methods []string
for _, signer := range signers {
ok, err := validateKey(signer.PublicKey(), user, c)
if err != nil {
return false, nil, err
return authFailure, nil, err
}
if !ok {
continue
Expand All @@ -206,7 +215,7 @@ func (cb publicKeyCallback) auth(session []byte, user string, c packetConn, rand
Method: cb.method(),
}, []byte(pub.Type()), pubKey))
if err != nil {
return false, nil, err
return authFailure, nil, err
}

// manually wrap the serialized signature in a string
Expand All @@ -224,24 +233,24 @@ func (cb publicKeyCallback) auth(session []byte, user string, c packetConn, rand
}
p := Marshal(&msg)
if err := c.writePacket(p); err != nil {
return false, nil, err
return authFailure, nil, err
}
var success bool
var success authResult
success, methods, err = handleAuthResponse(c)
if err != nil {
return false, nil, err
return authFailure, nil, err
}

// If authentication succeeds or the list of available methods does not
// contain the "publickey" method, do not attempt to authenticate with any
// other keys. According to RFC 4252 Section 7, the latter can occur when
// additional authentication methods are required.
if success || !containsMethod(methods, cb.method()) {
if success == authSuccess || !containsMethod(methods, cb.method()) {
return success, methods, err
}
}

return false, methods, nil
return authFailure, methods, nil
}

func containsMethod(methods []string, method string) bool {
Expand Down Expand Up @@ -318,28 +327,31 @@ func PublicKeysCallback(getSigners func() (signers []Signer, err error)) AuthMet
// handleAuthResponse returns whether the preceding authentication request succeeded
// along with a list of remaining authentication methods to try next and
// an error if an unexpected response was received.
func handleAuthResponse(c packetConn) (bool, []string, error) {
func handleAuthResponse(c packetConn) (authResult, []string, error) {
for {
packet, err := c.readPacket()
if err != nil {
return false, nil, err
return authFailure, nil, err
}

switch packet[0] {
case msgUserAuthBanner:
if err := handleBannerResponse(c, packet); err != nil {
return false, nil, err
return authFailure, nil, err
}
case msgUserAuthFailure:
var msg userAuthFailureMsg
if err := Unmarshal(packet, &msg); err != nil {
return false, nil, err
return authFailure, nil, err
}
return false, msg.Methods, nil
if msg.PartialSuccess {
return authPartialSuccess, msg.Methods, nil
}
return authFailure, msg.Methods, nil
case msgUserAuthSuccess:
return true, nil, nil
return authSuccess, nil, nil
default:
return false, nil, unexpectedMessageError(msgUserAuthSuccess, packet[0])
return authFailure, nil, unexpectedMessageError(msgUserAuthSuccess, packet[0])
}
}
}
Expand Down Expand Up @@ -381,7 +393,7 @@ func (cb KeyboardInteractiveChallenge) method() string {
return "keyboard-interactive"
}

func (cb KeyboardInteractiveChallenge) auth(session []byte, user string, c packetConn, rand io.Reader) (bool, []string, error) {
func (cb KeyboardInteractiveChallenge) auth(session []byte, user string, c packetConn, rand io.Reader) (authResult, []string, error) {
type initiateMsg struct {
User string `sshtype:"50"`
Service string
Expand All @@ -395,39 +407,42 @@ func (cb KeyboardInteractiveChallenge) auth(session []byte, user string, c packe
Service: serviceSSH,
Method: "keyboard-interactive",
})); err != nil {
return false, nil, err
return authFailure, nil, err
}

for {
packet, err := c.readPacket()
if err != nil {
return false, nil, err
return authFailure, nil, err
}

// like handleAuthResponse, but with less options.
switch packet[0] {
case msgUserAuthBanner:
if err := handleBannerResponse(c, packet); err != nil {
return false, nil, err
return authFailure, nil, err
}
continue
case msgUserAuthInfoRequest:
// OK
case msgUserAuthFailure:
var msg userAuthFailureMsg
if err := Unmarshal(packet, &msg); err != nil {
return false, nil, err
return authFailure, nil, err
}
if msg.PartialSuccess {
return authPartialSuccess, msg.Methods, nil
}
return false, msg.Methods, nil
return authFailure, msg.Methods, nil
case msgUserAuthSuccess:
return true, nil, nil
return authSuccess, nil, nil
default:
return false, nil, unexpectedMessageError(msgUserAuthInfoRequest, packet[0])
return authFailure, nil, unexpectedMessageError(msgUserAuthInfoRequest, packet[0])
}

var msg userAuthInfoRequestMsg
if err := Unmarshal(packet, &msg); err != nil {
return false, nil, err
return authFailure, nil, err
}

// Manually unpack the prompt/echo pairs.
Expand All @@ -437,24 +452,24 @@ func (cb KeyboardInteractiveChallenge) auth(session []byte, user string, c packe
for i := 0; i < int(msg.NumPrompts); i++ {
prompt, r, ok := parseString(rest)
if !ok || len(r) == 0 {
return false, nil, errors.New("ssh: prompt format error")
return authFailure, nil, errors.New("ssh: prompt format error")
}
prompts = append(prompts, string(prompt))
echos = append(echos, r[0] != 0)
rest = r[1:]
}

if len(rest) != 0 {
return false, nil, errors.New("ssh: extra data following keyboard-interactive pairs")
return authFailure, nil, errors.New("ssh: extra data following keyboard-interactive pairs")
}

answers, err := cb(msg.User, msg.Instruction, prompts, echos)
if err != nil {
return false, nil, err
return authFailure, nil, err
}

if len(answers) != len(prompts) {
return false, nil, errors.New("ssh: not enough answers from keyboard-interactive callback")
return authFailure, nil, errors.New("ssh: not enough answers from keyboard-interactive callback")
}
responseLength := 1 + 4
for _, a := range answers {
Expand All @@ -470,7 +485,7 @@ func (cb KeyboardInteractiveChallenge) auth(session []byte, user string, c packe
}

if err := c.writePacket(serialized); err != nil {
return false, nil, err
return authFailure, nil, err
}
}
}
Expand All @@ -480,10 +495,10 @@ type retryableAuthMethod struct {
maxTries int
}

func (r *retryableAuthMethod) auth(session []byte, user string, c packetConn, rand io.Reader) (ok bool, methods []string, err error) {
func (r *retryableAuthMethod) auth(session []byte, user string, c packetConn, rand io.Reader) (ok authResult, methods []string, err error) {
for i := 0; r.maxTries <= 0 || i < r.maxTries; i++ {
ok, methods, err = r.authMethod.auth(session, user, c, rand)
if ok || err != nil { // either success or error terminate
if ok != authFailure || err != nil { // either success, partial success or error terminate
return ok, methods, err
}
}
Expand Down
24 changes: 12 additions & 12 deletions keys_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -234,19 +234,19 @@ func TestMarshalParsePublicKey(t *testing.T) {
}
}

type authResult struct {
type testAuthResult struct {
pubKey PublicKey
options []string
comments string
rest string
ok bool
}

func testAuthorizedKeys(t *testing.T, authKeys []byte, expected []authResult) {
func testAuthorizedKeys(t *testing.T, authKeys []byte, expected []testAuthResult) {
rest := authKeys
var values []authResult
var values []testAuthResult
for len(rest) > 0 {
var r authResult
var r testAuthResult
var err error
r.pubKey, r.comments, r.options, rest, err = ParseAuthorizedKey(rest)
r.ok = (err == nil)
Expand All @@ -264,7 +264,7 @@ func TestAuthorizedKeyBasic(t *testing.T) {
pub, pubSerialized := getTestKey()
line := "ssh-rsa " + pubSerialized + " user@host"
testAuthorizedKeys(t, []byte(line),
[]authResult{
[]testAuthResult{
{pub, nil, "user@host", "", true},
})
}
Expand All @@ -286,7 +286,7 @@ func TestAuth(t *testing.T) {
authOptions := strings.Join(authWithOptions, eol)
rest2 := strings.Join(authWithOptions[3:], eol)
rest3 := strings.Join(authWithOptions[6:], eol)
testAuthorizedKeys(t, []byte(authOptions), []authResult{
testAuthorizedKeys(t, []byte(authOptions), []testAuthResult{
{pub, []string{`env="HOME=/home/root"`, "no-port-forwarding"}, "user@host", rest2, true},
{pub, []string{`env="HOME=/home/root2"`}, "user2@host2", rest3, true},
{nil, nil, "", "", false},
Expand All @@ -297,15 +297,15 @@ func TestAuth(t *testing.T) {
func TestAuthWithQuotedSpaceInEnv(t *testing.T) {
pub, pubSerialized := getTestKey()
authWithQuotedSpaceInEnv := []byte(`env="HOME=/home/root dir",no-port-forwarding ssh-rsa ` + pubSerialized + ` user@host`)
testAuthorizedKeys(t, []byte(authWithQuotedSpaceInEnv), []authResult{
testAuthorizedKeys(t, []byte(authWithQuotedSpaceInEnv), []testAuthResult{
{pub, []string{`env="HOME=/home/root dir"`, "no-port-forwarding"}, "user@host", "", true},
})
}

func TestAuthWithQuotedCommaInEnv(t *testing.T) {
pub, pubSerialized := getTestKey()
authWithQuotedCommaInEnv := []byte(`env="HOME=/home/root,dir",no-port-forwarding ssh-rsa ` + pubSerialized + ` user@host`)
testAuthorizedKeys(t, []byte(authWithQuotedCommaInEnv), []authResult{
testAuthorizedKeys(t, []byte(authWithQuotedCommaInEnv), []testAuthResult{
{pub, []string{`env="HOME=/home/root,dir"`, "no-port-forwarding"}, "user@host", "", true},
})
}
Expand All @@ -314,11 +314,11 @@ func TestAuthWithQuotedQuoteInEnv(t *testing.T) {
pub, pubSerialized := getTestKey()
authWithQuotedQuoteInEnv := []byte(`env="HOME=/home/\"root dir",no-port-forwarding` + "\t" + `ssh-rsa` + "\t" + pubSerialized + ` user@host`)
authWithDoubleQuotedQuote := []byte(`no-port-forwarding,env="HOME=/home/ \"root dir\"" ssh-rsa ` + pubSerialized + "\t" + `user@host`)
testAuthorizedKeys(t, []byte(authWithQuotedQuoteInEnv), []authResult{
testAuthorizedKeys(t, []byte(authWithQuotedQuoteInEnv), []testAuthResult{
{pub, []string{`env="HOME=/home/\"root dir"`, "no-port-forwarding"}, "user@host", "", true},
})

testAuthorizedKeys(t, []byte(authWithDoubleQuotedQuote), []authResult{
testAuthorizedKeys(t, []byte(authWithDoubleQuotedQuote), []testAuthResult{
{pub, []string{"no-port-forwarding", `env="HOME=/home/ \"root dir\""`}, "user@host", "", true},
})
}
Expand All @@ -327,7 +327,7 @@ func TestAuthWithInvalidSpace(t *testing.T) {
_, pubSerialized := getTestKey()
authWithInvalidSpace := []byte(`env="HOME=/home/root dir", no-port-forwarding ssh-rsa ` + pubSerialized + ` user@host
#more to follow but still no valid keys`)
testAuthorizedKeys(t, []byte(authWithInvalidSpace), []authResult{
testAuthorizedKeys(t, []byte(authWithInvalidSpace), []testAuthResult{
{nil, nil, "", "", false},
})
}
Expand All @@ -337,7 +337,7 @@ func TestAuthWithMissingQuote(t *testing.T) {
authWithMissingQuote := []byte(`env="HOME=/home/root,no-port-forwarding ssh-rsa ` + pubSerialized + ` user@host
env="HOME=/home/root",shared-control ssh-rsa ` + pubSerialized + ` user@host`)

testAuthorizedKeys(t, []byte(authWithMissingQuote), []authResult{
testAuthorizedKeys(t, []byte(authWithMissingQuote), []testAuthResult{
{pub, []string{`env="HOME=/home/root"`, `shared-control`}, "user@host", "", true},
})
}
Expand Down
Loading

0 comments on commit a66270f

Please sign in to comment.