Skip to content

Commit

Permalink
ssh: respect MaxAuthTries also for "none" auth attempts
Browse files Browse the repository at this point in the history
Only the first "none" auth attempt is allowed without penality

Change-Id: Ibe776e968ba406445eeb94e8f1959383b88c98f7
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/575995
Reviewed-by: Filippo Valsorda <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
Auto-Submit: Nicola Murino <[email protected]>
Commit-Queue: Nicola Murino <[email protected]>
Reviewed-by: Than McIntosh <[email protected]>
  • Loading branch information
drakkan authored and gopherbot committed Apr 4, 2024
1 parent 6f79b5a commit b92bf94
Show file tree
Hide file tree
Showing 2 changed files with 133 additions and 2 deletions.
6 changes: 4 additions & 2 deletions ssh/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -468,6 +468,7 @@ func (s *connection) serverAuthenticate(config *ServerConfig) (*Permissions, err
var perms *Permissions

authFailures := 0
noneAuthCount := 0
var authErrs []error
var displayedBanner bool
partialSuccessReturned := false
Expand Down Expand Up @@ -534,6 +535,7 @@ userAuthLoop:

switch userAuthReq.Method {
case "none":
noneAuthCount++
// We don't allow none authentication after a partial success
// response.
if config.NoClientAuth && !partialSuccessReturned {
Expand Down Expand Up @@ -755,7 +757,7 @@ userAuthLoop:
failureMsg.PartialSuccess = true
} else {
// Allow initial attempt of 'none' without penalty.
if authFailures > 0 || userAuthReq.Method != "none" {
if authFailures > 0 || userAuthReq.Method != "none" || noneAuthCount != 1 {
authFailures++
}
if config.MaxAuthTries > 0 && authFailures >= config.MaxAuthTries {
Expand All @@ -777,7 +779,7 @@ userAuthLoop:
// disconnect, should we only send that message.)
//
// Either way, OpenSSH disconnects immediately after the last
// failed authnetication attempt, and given they are typically
// failed authentication attempt, and given they are typically
// considered the golden implementation it seems reasonable
// to match that behavior.
continue
Expand Down
129 changes: 129 additions & 0 deletions ssh/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,10 @@
package ssh

import (
"errors"
"io"
"net"
"strings"
"sync/atomic"
"testing"
"time"
Expand Down Expand Up @@ -62,6 +64,133 @@ func TestClientAuthRestrictedPublicKeyAlgos(t *testing.T) {
}
}

func TestMaxAuthTriesNoneMethod(t *testing.T) {
username := "testuser"
serverConfig := &ServerConfig{
MaxAuthTries: 2,
PasswordCallback: func(conn ConnMetadata, password []byte) (*Permissions, error) {
if conn.User() == username && string(password) == clientPassword {
return nil, nil
}
return nil, errors.New("invalid credentials")
},
}
c1, c2, err := netPipe()
if err != nil {
t.Fatalf("netPipe: %v", err)
}
defer c1.Close()
defer c2.Close()

var serverAuthErrors []error

serverConfig.AddHostKey(testSigners["rsa"])
serverConfig.AuthLogCallback = func(conn ConnMetadata, method string, err error) {
serverAuthErrors = append(serverAuthErrors, err)
}
go newServer(c1, serverConfig)

clientConfig := ClientConfig{
User: username,
HostKeyCallback: InsecureIgnoreHostKey(),
}
clientConfig.SetDefaults()
// Our client will send 'none' auth only once, so we need to send the
// requests manually.
c := &connection{
sshConn: sshConn{
conn: c2,
user: username,
clientVersion: []byte(packageVersion),
},
}
c.serverVersion, err = exchangeVersions(c.sshConn.conn, c.clientVersion)
if err != nil {
t.Fatalf("unable to exchange version: %v", err)
}
c.transport = newClientTransport(
newTransport(c.sshConn.conn, clientConfig.Rand, true /* is client */),
c.clientVersion, c.serverVersion, &clientConfig, "", c.sshConn.RemoteAddr())
if err := c.transport.waitSession(); err != nil {
t.Fatalf("unable to wait session: %v", err)
}
c.sessionID = c.transport.getSessionID()
if err := c.transport.writePacket(Marshal(&serviceRequestMsg{serviceUserAuth})); err != nil {
t.Fatalf("unable to send ssh-userauth message: %v", err)
}
packet, err := c.transport.readPacket()
if err != nil {
t.Fatal(err)
}
if len(packet) > 0 && packet[0] == msgExtInfo {
packet, err = c.transport.readPacket()
if err != nil {
t.Fatal(err)
}
}
var serviceAccept serviceAcceptMsg
if err := Unmarshal(packet, &serviceAccept); err != nil {
t.Fatal(err)
}
for i := 0; i <= serverConfig.MaxAuthTries; i++ {
auth := new(noneAuth)
_, _, err := auth.auth(c.sessionID, clientConfig.User, c.transport, clientConfig.Rand, nil)
if i < serverConfig.MaxAuthTries {
if err != nil {
t.Fatal(err)
}
continue
}
if err == nil {
t.Fatal("client: got no error")
} else if !strings.Contains(err.Error(), "too many authentication failures") {
t.Fatalf("client: got unexpected error: %v", err)
}
}
if len(serverAuthErrors) != 3 {
t.Fatalf("unexpected number of server auth errors: %v, errors: %+v", len(serverAuthErrors), serverAuthErrors)
}
for _, err := range serverAuthErrors {
if !errors.Is(err, ErrNoAuth) {
t.Errorf("go error: %v; want: %v", err, ErrNoAuth)
}
}
}

func TestMaxAuthTriesFirstNoneAuthErrorIgnored(t *testing.T) {
username := "testuser"
serverConfig := &ServerConfig{
MaxAuthTries: 1,
PasswordCallback: func(conn ConnMetadata, password []byte) (*Permissions, error) {
if conn.User() == username && string(password) == clientPassword {
return nil, nil
}
return nil, errors.New("invalid credentials")
},
}
clientConfig := &ClientConfig{
User: username,
Auth: []AuthMethod{
Password(clientPassword),
},
HostKeyCallback: InsecureIgnoreHostKey(),
}

serverAuthErrors, err := doClientServerAuth(t, serverConfig, clientConfig)
if err != nil {
t.Fatalf("client login error: %s", err)
}
if len(serverAuthErrors) != 2 {
t.Fatalf("unexpected number of server auth errors: %v, errors: %+v", len(serverAuthErrors), serverAuthErrors)
}
if !errors.Is(serverAuthErrors[0], ErrNoAuth) {
t.Errorf("go error: %v; want: %v", serverAuthErrors[0], ErrNoAuth)
}
if serverAuthErrors[1] != nil {
t.Errorf("unexpected error: %v", serverAuthErrors[1])
}
}

func TestNewServerConnValidationErrors(t *testing.T) {
serverConf := &ServerConfig{
PublicKeyAuthAlgorithms: []string{CertAlgoRSAv01},
Expand Down

0 comments on commit b92bf94

Please sign in to comment.