From 66d5ff892443446aae7292e1b9a9ffa3547b1855 Mon Sep 17 00:00:00 2001 From: Alan Parra Date: Thu, 11 Jan 2024 15:24:47 -0300 Subject: [PATCH 1/2] fix: Verify MFA device locks during authentication (#36471) * Test authn and password change with a locked user * Verify MFA device locks during authentication * Configure a LockWatcher in the passwordSuite setup * Appease linter --- lib/auth/accountrecovery_test.go | 2 +- lib/auth/auth_test.go | 125 +++++++++++++++++++++++++++++++ lib/auth/methods.go | 101 ++++++++++++++++++++++--- lib/auth/password.go | 7 +- lib/auth/password_test.go | 11 +++ 5 files changed, 233 insertions(+), 13 deletions(-) diff --git a/lib/auth/accountrecovery_test.go b/lib/auth/accountrecovery_test.go index 0ba24a21f263b..9c808825fdfb3 100644 --- a/lib/auth/accountrecovery_test.go +++ b/lib/auth/accountrecovery_test.go @@ -1286,7 +1286,7 @@ func TestGetAccountRecoveryCodes(t *testing.T) { func triggerLoginLock(t *testing.T, srv *Server, username string) { for i := 1; i <= defaults.MaxLoginAttempts; i++ { - _, _, err := srv.authenticateUser(context.Background(), AuthenticateUserRequest{ + _, _, _, err := srv.authenticateUser(context.Background(), AuthenticateUserRequest{ Username: username, OTP: &OTPCreds{}, }) diff --git a/lib/auth/auth_test.go b/lib/auth/auth_test.go index 79cba2b30c831..61a6327fce3b0 100644 --- a/lib/auth/auth_test.go +++ b/lib/auth/auth_test.go @@ -60,6 +60,7 @@ import ( "github.com/gravitational/teleport/lib/auth/keystore" "github.com/gravitational/teleport/lib/auth/native" "github.com/gravitational/teleport/lib/auth/testauthority" + wantypes "github.com/gravitational/teleport/lib/auth/webauthntypes" "github.com/gravitational/teleport/lib/authz" "github.com/gravitational/teleport/lib/backend" "github.com/gravitational/teleport/lib/backend/lite" @@ -521,6 +522,130 @@ func TestAuthenticateSSHUser(t *testing.T) { require.Error(t, err) } +func TestAuthenticateUser_mfaDeviceLocked(t *testing.T) { + t.Parallel() + + testServer := newTestTLSServer(t) + authServer := testServer.Auth() + + ctx := context.Background() + const user = "llama" + const pass = "supersecret!!1!one" + + // Configure auth preferences. + authPref, err := authServer.GetAuthPreference(ctx) + require.NoError(t, err, "GetAuthPreference") + authPref.SetSecondFactor(constants.SecondFactorOptional) // good enough + authPref.SetWebauthn(&types.Webauthn{ + RPID: "localhost", + }) + require.NoError(t, + authServer.SetAuthPreference(ctx, authPref), + "SetAuthPreference") + + // Prepare user, password and MFA device. + _, _, err = CreateUserAndRole(authServer, user, []string{user}, nil /* allowRules */) + require.NoError(t, err, "CreateUserAndRole") + require.NoError(t, + authServer.UpsertPassword(user, []byte(pass)), + "UpsertPassword") + + userClient, err := testServer.NewClient(TestUser(user)) + require.NoError(t, err, "NewClient") + + // OTP devices would work for this test too. + dev1, err := RegisterTestDevice(ctx, userClient, "dev1", proto.DeviceType_DEVICE_TYPE_WEBAUTHN, nil /* authenticator */) + require.NoError(t, err, "RegisterTestDevice") + dev2, err := RegisterTestDevice(ctx, userClient, "dev2", proto.DeviceType_DEVICE_TYPE_WEBAUTHN, dev1 /* authenticator */) + require.NoError(t, err, "RegisterTestDevice") + + // Prepare an SSH public key for testing. + privKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err, "GenerateKey") + signer, err := ssh.NewSignerFromSigner(privKey) + require.NoError(t, err, "NewSignerFromSigner") + pubKey := ssh.MarshalAuthorizedKey(signer.PublicKey()) + + // Users initially authenticate via Proxy, as there isn't a userClient before + // authn. + proxyClient, err := testServer.NewClient(TestBuiltin(types.RoleProxy)) + require.NoError(t, err, "NewClient") + + authenticateSSH := func(dev *TestDevice) (*SSHLoginResponse, error) { + chal, err := proxyClient.CreateAuthenticateChallenge(ctx, &proto.CreateAuthenticateChallengeRequest{ + Request: &proto.CreateAuthenticateChallengeRequest_UserCredentials{ + UserCredentials: &proto.UserCredentials{ + Username: user, + Password: []byte(pass), + }, + }, + }) + if err != nil { + return nil, fmt.Errorf("create challenge: %w", err) + } + + chalResp, err := dev.SolveAuthn(chal) + if err != nil { + return nil, fmt.Errorf("solve challenge: %w", err) + } + + return proxyClient.AuthenticateSSHUser(ctx, AuthenticateSSHRequest{ + AuthenticateUserRequest: AuthenticateUserRequest{ + Username: user, + PublicKey: pubKey, + Pass: &PassCreds{ + Password: []byte(pass), + }, + Webauthn: wantypes.CredentialAssertionResponseFromProto(chalResp.GetWebauthn()), + }, + TTL: 1 * time.Hour, + }) + } + + // Lock dev1. + const lockMessage = "device locked for testing" + lock, err := types.NewLock("dev1-lock", types.LockSpecV2{ + Target: types.LockTarget{ + MFADevice: dev1.MFA.Id, + }, + Message: lockMessage, + }) + require.NoError(t, err, "NewLock") + require.NoError(t, + userClient.UpsertLock(ctx, lock), + "UpsertLock") + + t.Run("locked device", func(t *testing.T) { + _, err := authenticateSSH(dev1) + assert.ErrorContains(t, err, lockMessage) + }) + + t.Run("unlocked device", func(t *testing.T) { + _, err := authenticateSSH(dev2) + assert.NoError(t, err, "authenticateSSH failed unexpectedly") + }) + + t.Run("locked device password change", func(t *testing.T) { + chal, err := userClient.CreateAuthenticateChallenge(ctx, &proto.CreateAuthenticateChallengeRequest{ + Request: &proto.CreateAuthenticateChallengeRequest_ContextUser{}, + }) + require.NoError(t, err, "CreateAuthenticateChallenge") + + // dev1 is still locked. + chalResp, err := dev1.SolveAuthn(chal) + require.NoError(t, err, "SolveAuthn") + + assert.ErrorContains(t, + userClient.ChangePassword(ctx, &proto.ChangePasswordRequest{ + User: user, + OldPassword: []byte(pass), + NewPassword: []byte("evenmoresecret!!1!ONE"), + Webauthn: chalResp.GetWebauthn(), + }), + lockMessage) + }) +} + func TestUserLock(t *testing.T) { t.Parallel() s := newAuthSuite(t) diff --git a/lib/auth/methods.go b/lib/auth/methods.go index 8d649a31db143..f19397fe1e654 100644 --- a/lib/auth/methods.go +++ b/lib/auth/methods.go @@ -116,7 +116,7 @@ type SessionCreds struct { func (a *Server) AuthenticateUser(ctx context.Context, req AuthenticateUserRequest) (services.UserState, services.AccessChecker, error) { username := req.Username - mfaDev, actualUsername, err := a.authenticateUser(ctx, req) + verifyMFALocks, mfaDev, actualUsername, err := a.authenticateUser(ctx, req) if err != nil { // Log event after authentication failure if err := a.emitAuthAuditEvent(ctx, authAuditProps{ @@ -164,6 +164,23 @@ func (a *Server) AuthenticateUser(ctx context.Context, req AuthenticateUserReque return nil, nil, trace.Wrap(err) } + // Verify if the MFA device is locked. + if err := verifyMFALocks(verifyMFADeviceLocksParams{ + Checker: checker, + }); err != nil { + // Log MFA lock failure as an authn failure. + if err := a.emitAuthAuditEvent(ctx, authAuditProps{ + username: req.Username, + clientMetadata: req.ClientMetadata, + mfaDevice: mfaDev, + checker: checker, + authErr: err, + }); err != nil { + log.WithError(err).Warn("Failed to emit login event.") + } + return nil, nil, trace.Wrap(err) + } + // Log event after authentication success if err := a.emitAuthAuditEvent(ctx, authAuditProps{ username: username, @@ -257,14 +274,77 @@ func IsInvalidLocalCredentialError(err error) bool { return errors.Is(err, invalidUserPassError) || errors.Is(err, invalidUserPass2FError) } +type verifyMFADeviceLocksParams struct { + // Checker used to verify locks. + // Optional, created via a [UserState] fetch if nil. + Checker services.AccessChecker + + // ClusterLockingMode used to verify locks. + // Optional, acquired from [Server.GetAuthPreference] if nil. + ClusterLockingMode constants.LockingMode +} + // authenticateUser authenticates a user through various methods (password, MFA, // passwordless) -// Returns the device used to authenticate (if applicable) and the username. -func (a *Server) authenticateUser(ctx context.Context, req AuthenticateUserRequest) (*types.MFADevice, string, error) { +// +// Returns a callback to verify MFA device locks, the MFA device used to +// authenticate (if applicable), and the authenticated user name. +// +// Callers MUST call the verifyLocks callback. +func (a *Server) authenticateUser( + ctx context.Context, + req AuthenticateUserRequest, +) (verifyLocks func(verifyMFADeviceLocksParams) error, mfaDev *types.MFADevice, user string, err error) { + mfaDev, user, err = a.authenticateUserInternal(ctx, req) + if err != nil || mfaDev == nil { + return func(verifyMFADeviceLocksParams) error { return nil }, mfaDev, user, trace.Wrap(err) + } + + verifyLocks = func(p verifyMFADeviceLocksParams) error { + if p.Checker == nil { + userState, err := a.GetUserOrLoginState(ctx, user) + if err != nil { + return trace.Wrap(err) + } + accessInfo := services.AccessInfoFromUserState(userState) + clusterName, err := a.GetClusterName() + if err != nil { + return trace.Wrap(err) + } + checker, err := services.NewAccessChecker(accessInfo, clusterName.GetClusterName(), a) + if err != nil { + return trace.Wrap(err) + } + p.Checker = checker + } + + if p.ClusterLockingMode == "" { + authPref, err := a.GetAuthPreference(ctx) + if err != nil { + return trace.Wrap(err) + } + p.ClusterLockingMode = authPref.GetLockingMode() + } + + // The MFA device needs to be explicitly verified, as it won't be verified + // as part of certificate issuance in various scenarios (password change, + // non-session certificates, etc) + return a.verifyLocksForUserCerts(verifyLocksForUserCertsReq{ + checker: p.Checker, + defaultMode: p.ClusterLockingMode, + username: user, + mfaVerified: mfaDev.Id, + }) + } + return verifyLocks, mfaDev, user, nil +} + +// Do not use this method directly, use authenticateUser instead. +func (a *Server) authenticateUserInternal(ctx context.Context, req AuthenticateUserRequest) (mfaDev *types.MFADevice, user string, err error) { if err := req.CheckAndSetDefaults(); err != nil { return nil, "", trace.Wrap(err) } - user := req.Username + user = req.Username passwordless := user == "" // Only one path if passwordless, other variants shouldn't see an empty user. @@ -296,8 +376,8 @@ func (a *Server) authenticateUser(ctx context.Context, req AuthenticateUserReque Webauthn: wantypes.CredentialAssertionResponseToProto(req.Webauthn), }, } - dev, _, err := a.validateMFAAuthResponse(ctx, mfaResponse, user, passwordless) - return dev, trace.Wrap(err) + mfaDev, _, err := a.ValidateMFAAuthResponse(ctx, mfaResponse, user, passwordless) + return mfaDev, trace.Wrap(err) } authErr = authenticateWebauthnError case req.OTP != nil: @@ -313,10 +393,9 @@ func (a *Server) authenticateUser(ctx context.Context, req AuthenticateUserReque authErr = invalidUserPass2FError } if authenticateFn != nil { - var dev *types.MFADevice - err := a.WithUserLock(user, func() error { + err := a.WithUserLock(ctx, user, func() error { var err error - dev, err = authenticateFn() + mfaDev, err = authenticateFn() return err }) switch { @@ -327,13 +406,13 @@ func (a *Server) authenticateUser(ctx context.Context, req AuthenticateUserReque } return nil, "", trace.Wrap(authErr) - case dev == nil: + case mfaDev == nil: log.Debugf( "MFA authentication returned nil device (Webauthn = %v, TOTP = %v, Headless = %v): %v.", req.Webauthn != nil, req.OTP != nil, req.HeadlessAuthenticationID != "", err) return nil, "", trace.Wrap(authErr) default: - return dev, user, nil + return mfaDev, user, nil } } diff --git a/lib/auth/password.go b/lib/auth/password.go index 19d43f19cda0f..20ee2b1c9b0bc 100644 --- a/lib/auth/password.go +++ b/lib/auth/password.go @@ -136,7 +136,12 @@ func (a *Server) ChangePassword(ctx context.Context, req *proto.ChangePasswordRe Token: req.SecondFactorToken, } } - if _, _, err := a.authenticateUser(ctx, authReq); err != nil { + verifyMFALocks, _, _, err := a.authenticateUser(ctx, authReq) + if err != nil { + return trace.Wrap(err) + } + // Verify if the MFA device used is locked. + if err := verifyMFALocks(verifyMFADeviceLocksParams{}); err != nil { return trace.Wrap(err) } diff --git a/lib/auth/password_test.go b/lib/auth/password_test.go index 024c5b7aeae82..5c5db7d0b79e9 100644 --- a/lib/auth/password_test.go +++ b/lib/auth/password_test.go @@ -29,6 +29,7 @@ import ( "github.com/pquerna/otp/totp" "github.com/stretchr/testify/require" + "github.com/gravitational/teleport" "github.com/gravitational/teleport/api/client/proto" "github.com/gravitational/teleport/api/constants" "github.com/gravitational/teleport/api/types" @@ -87,6 +88,16 @@ func setupPasswordSuite(t *testing.T) *passwordSuite { err = s.a.SetClusterName(clusterName) require.NoError(t, err) + // set lock watcher + lockWatcher, err := services.NewLockWatcher(ctx, services.LockWatcherConfig{ + ResourceWatcherConfig: services.ResourceWatcherConfig{ + Component: teleport.ComponentAuth, + Client: s.a, + }, + }) + require.NoError(t, err, "NewLockWatcher") + s.a.SetLockWatcher(lockWatcher) + // set static tokens staticTokens, err := types.NewStaticTokens(types.StaticTokensSpecV2{ StaticTokens: []types.ProvisionTokenV1{}, From da2865bb08d27c1bb50d54017da0deeaeef204e4 Mon Sep 17 00:00:00 2001 From: Alan Parra Date: Thu, 11 Jan 2024 17:45:46 -0300 Subject: [PATCH 2/2] Apply fixes for v14 --- lib/auth/methods.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/auth/methods.go b/lib/auth/methods.go index f19397fe1e654..9f7c15f312903 100644 --- a/lib/auth/methods.go +++ b/lib/auth/methods.go @@ -376,7 +376,7 @@ func (a *Server) authenticateUserInternal(ctx context.Context, req AuthenticateU Webauthn: wantypes.CredentialAssertionResponseToProto(req.Webauthn), }, } - mfaDev, _, err := a.ValidateMFAAuthResponse(ctx, mfaResponse, user, passwordless) + mfaDev, _, err := a.validateMFAAuthResponse(ctx, mfaResponse, user, passwordless) return mfaDev, trace.Wrap(err) } authErr = authenticateWebauthnError @@ -393,7 +393,7 @@ func (a *Server) authenticateUserInternal(ctx context.Context, req AuthenticateU authErr = invalidUserPass2FError } if authenticateFn != nil { - err := a.WithUserLock(ctx, user, func() error { + err := a.WithUserLock(user, func() error { var err error mfaDev, err = authenticateFn() return err