Skip to content

Commit

Permalink
[v14] fix: Emit login failure event in the /mfa/login/begin step (#41433
Browse files Browse the repository at this point in the history
)

* Test failed user/pass audit on CreateAuthenticateChallenge

* Emit login failure event in the /mfa/login/begin step

* nit: Remove final stop from logging statements

* nit: Remove seemingly outdated comment

* Apply fixes for v14
  • Loading branch information
codingllama committed May 13, 2024
1 parent 8f126fd commit 20b8b97
Show file tree
Hide file tree
Showing 3 changed files with 55 additions and 4 deletions.
9 changes: 9 additions & 0 deletions lib/auth/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -3086,6 +3086,15 @@ func (a *Server) CreateAuthenticateChallenge(ctx context.Context, req *proto.Cre
if err := a.WithUserLock(username, func() error {
return a.checkPasswordWOToken(username, req.GetUserCredentials().GetPassword())
}); err != nil {
// This is only ever used as a means to acquire a login challenge, so
// let's issue an authentication failure event.
if err := a.emitAuthAuditEvent(ctx, authAuditProps{
username: username,
authErr: err,
}); err != nil {
log.WithError(err).Warn("Failed to emit login event")
// err swallowed on purpose.
}
return nil, trace.Wrap(err)
}

Expand Down
44 changes: 43 additions & 1 deletion lib/auth/auth_login_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ import (
"github.com/gravitational/teleport/lib/auth/mocku2f"
wantypes "github.com/gravitational/teleport/lib/auth/webauthntypes"
"github.com/gravitational/teleport/lib/defaults"
"github.com/gravitational/teleport/lib/events"
"github.com/gravitational/teleport/lib/events/eventstest"
"github.com/gravitational/teleport/lib/modules"
"github.com/gravitational/teleport/lib/services"
)
Expand Down Expand Up @@ -578,6 +580,47 @@ func TestCreateAuthenticateChallenge_mfaVerification(t *testing.T) {
}
}

// TestCreateAuthenticateChallenge_failedLoginAudit tests a password+webauthn
// login scenario where the user types the wrong password.
// This should issue a "Local Login Failed" audit event.
func TestCreateAuthenticateChallenge_failedLoginAudit(t *testing.T) {
t.Parallel()

testServer := newTestTLSServer(t)
emitter := &eventstest.MockRecorderEmitter{}
authServer := testServer.Auth()
authServer.SetEmitter(emitter)

ctx := context.Background()

// Set the cluster to require 2nd factor, create the user, set a password and
// register a webauthn device.
// password+OTP logins go through another route.
mfa := configureForMFA(t, testServer)

// Proxy identity is used during login.
proxyClient, err := testServer.NewClient(TestBuiltin(types.RoleProxy))
require.NoError(t, err, "NewClient(RoleProxy) failed")

t.Run("emits audit event", func(t *testing.T) {
emitter.Reset()
_, err := proxyClient.CreateAuthenticateChallenge(ctx, &proto.CreateAuthenticateChallengeRequest{
Request: &proto.CreateAuthenticateChallengeRequest_UserCredentials{
UserCredentials: &proto.UserCredentials{
Username: mfa.User,
Password: []byte(mfa.Password + "BAD"),
},
},
})
assert.ErrorContains(t, err, "password", "CreateAuthenticateChallenge error mismatch")

event := emitter.LastEvent()
require.NotNil(t, event, "No audit event emitted")
assert.Equal(t, events.UserLoginEvent, event.GetType(), "event.Type mismatch")
assert.Equal(t, events.UserLocalLoginFailureCode, event.GetCode(), "event.Code mismatch")
})
}

func TestCreateRegisterChallenge(t *testing.T) {
t.Parallel()
ctx := context.Background()
Expand Down Expand Up @@ -1503,7 +1546,6 @@ func configureForMFA(t *testing.T, srv *TestTLSServer) *configureMFAResp {
Webauthn: &types.Webauthn{
RPID: "localhost",
},
// Use default Webauthn config.
})
require.NoError(t, err)

Expand Down
6 changes: 3 additions & 3 deletions lib/auth/methods.go
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ func (a *Server) authenticateUserLogin(ctx context.Context, req AuthenticateUser
clientMetadata: req.ClientMetadata,
authErr: err,
}); err != nil {
log.WithError(err).Warn("Failed to emit login event.")
log.WithError(err).Warn("Failed to emit login event")
}
return nil, nil, trace.Wrap(err)
}
Expand Down Expand Up @@ -178,7 +178,7 @@ func (a *Server) authenticateUserLogin(ctx context.Context, req AuthenticateUser
checker: checker,
authErr: err,
}); err != nil {
log.WithError(err).Warn("Failed to emit login event.")
log.WithError(err).Warn("Failed to emit login event")
}
return nil, nil, trace.Wrap(err)
}
Expand All @@ -190,7 +190,7 @@ func (a *Server) authenticateUserLogin(ctx context.Context, req AuthenticateUser
mfaDevice: mfaDev,
checker: checker,
}); err != nil {
log.WithError(err).Warn("Failed to emit login event.")
log.WithError(err).Warn("Failed to emit login event")
}

return userState, checker, trace.Wrap(err)
Expand Down

0 comments on commit 20b8b97

Please sign in to comment.