From 72cbd3e8212c0bea636e90887649e09618efad04 Mon Sep 17 00:00:00 2001
From: Tim Ross <tim.ross@goteleport.com>
Date: Tue, 5 Dec 2023 11:03:44 -0500
Subject: [PATCH] Validate the user still exists during account reset

Prior to this, any reset links created before a user was deleted
would still be valid and when used would recreate the deleted user.
With this change, the user is validated to still exist prior to
honoring the reset request and if the user no longer exists any
remaining reset tokens are deleted.
---
 lib/auth/password.go | 19 +++++++---
 lib/auth/tls_test.go | 82 ++++++++++++++++++++++++++++++++------------
 2 files changed, 76 insertions(+), 25 deletions(-)

diff --git a/lib/auth/password.go b/lib/auth/password.go
index 319b92719be13..19d43f19cda0f 100644
--- a/lib/auth/password.go
+++ b/lib/auth/password.go
@@ -306,16 +306,27 @@ func (a *Server) changeUserAuthentication(ctx context.Context, req *proto.Change
 		return nil, trace.BadParameter("expired token")
 	}
 
-	err = a.changeUserSecondFactor(ctx, req, token)
-	if err != nil {
+	// Check if the user still exists before potentially recreating the user
+	// below. If the user was deleted, do NOT honor the request and delete any
+	// other tokens associated with the user.
+	if _, err := a.GetUser(token.GetUser(), false); err != nil {
+		if trace.IsNotFound(err) {
+			// Delete any remaining tokens for users that no longer exist.
+			if err := a.deleteUserTokens(ctx, token.GetUser()); err != nil {
+				return nil, trace.Wrap(err)
+			}
+		}
+		return nil, trace.Wrap(err)
+	}
+
+	if err := a.changeUserSecondFactor(ctx, req, token); err != nil {
 		return nil, trace.Wrap(err)
 	}
 
 	username := token.GetUser()
 	// Delete this token first to minimize the chances
 	// of partially updated user with still valid token.
-	err = a.deleteUserTokens(ctx, username)
-	if err != nil {
+	if err := a.deleteUserTokens(ctx, username); err != nil {
 		return nil, trace.Wrap(err)
 	}
 
diff --git a/lib/auth/tls_test.go b/lib/auth/tls_test.go
index ccaf5edc7c446..10614c1f7d99b 100644
--- a/lib/auth/tls_test.go
+++ b/lib/auth/tls_test.go
@@ -2846,7 +2846,10 @@ func TestChangeUserAuthenticationSettings(t *testing.T) {
 
 	authPreference, err := types.NewAuthPreference(types.AuthPreferenceSpecV2{
 		Type:         constants.Local,
-		SecondFactor: constants.SecondFactorOTP,
+		SecondFactor: constants.SecondFactorWebauthn,
+		Webauthn: &types.Webauthn{
+			RPID: "localhost",
+		},
 	})
 	require.NoError(t, err)
 
@@ -2858,32 +2861,69 @@ func TestChangeUserAuthenticationSettings(t *testing.T) {
 	clt, err := testSrv.NewClient(TestAdmin())
 	require.NoError(t, err)
 
-	_, _, err = CreateUserAndRole(clt, username, []string{"role1"}, nil)
-	require.NoError(t, err)
+	t.Run("Reset works when user exists", func(t *testing.T) {
+		_, _, err = CreateUserAndRole(clt, username, []string{"role1"}, nil)
+		require.NoError(t, err)
 
-	token, err := testSrv.Auth().CreateResetPasswordToken(ctx, CreateUserTokenRequest{
-		Name: username,
-		TTL:  time.Hour,
-	})
-	require.NoError(t, err)
+		token, err := testSrv.Auth().CreateResetPasswordToken(ctx, CreateUserTokenRequest{
+			Name: username,
+			TTL:  time.Hour,
+		})
+		require.NoError(t, err)
 
-	res, err := testSrv.Auth().CreateRegisterChallenge(ctx, &proto.CreateRegisterChallengeRequest{
-		TokenID:    token.GetName(),
-		DeviceType: proto.DeviceType_DEVICE_TYPE_TOTP,
+		res, err := testSrv.Auth().CreateRegisterChallenge(ctx, &proto.CreateRegisterChallengeRequest{
+			TokenID:    token.GetName(),
+			DeviceType: proto.DeviceType_DEVICE_TYPE_WEBAUTHN,
+		})
+		require.NoError(t, err)
+
+		_, registerSolved, err := NewTestDeviceFromChallenge(res)
+		require.NoError(t, err)
+
+		_, err = testSrv.Auth().ChangeUserAuthentication(ctx, &proto.ChangeUserAuthenticationRequest{
+			TokenID:                token.GetName(),
+			NewPassword:            []byte("qweqweqwe"),
+			NewMFARegisterResponse: registerSolved,
+		})
+		require.NoError(t, err)
 	})
-	require.NoError(t, err)
 
-	otpToken, err := totp.GenerateCode(res.GetTOTP().GetSecret(), testSrv.Clock().Now())
-	require.NoError(t, err)
+	t.Run("Reset link not allowed when user does not exist", func(t *testing.T) {
+		var tokenID string
+		var resp *proto.MFARegisterResponse
+		for i := 0; i < 5; i++ {
+			token, err := testSrv.Auth().CreateResetPasswordToken(ctx, CreateUserTokenRequest{
+				Name: username,
+				TTL:  time.Hour,
+			})
+			require.NoError(t, err)
+
+			res, err := testSrv.Auth().CreateRegisterChallenge(ctx, &proto.CreateRegisterChallengeRequest{
+				TokenID:    token.GetName(),
+				DeviceType: proto.DeviceType_DEVICE_TYPE_WEBAUTHN,
+			})
+			require.NoError(t, err)
+
+			_, registerSolved, err := NewTestDeviceFromChallenge(res)
+			require.NoError(t, err)
 
-	_, err = testSrv.Auth().ChangeUserAuthentication(ctx, &proto.ChangeUserAuthenticationRequest{
-		TokenID:     token.GetName(),
-		NewPassword: []byte("qweqweqwe"),
-		NewMFARegisterResponse: &proto.MFARegisterResponse{Response: &proto.MFARegisterResponse_TOTP{
-			TOTP: &proto.TOTPRegisterResponse{Code: otpToken},
-		}},
+			tokenID = token.GetName()
+			resp = registerSolved
+		}
+
+		require.NoError(t, testSrv.Auth().DeleteUser(ctx, username))
+
+		_, err = testSrv.Auth().ChangeUserAuthentication(ctx, &proto.ChangeUserAuthenticationRequest{
+			TokenID:                tokenID,
+			NewPassword:            []byte("qweqweqwe"),
+			NewMFARegisterResponse: resp,
+		})
+		require.Error(t, err)
+
+		tokens, err := testSrv.Auth().GetUserTokens(ctx)
+		require.NoError(t, err)
+		require.Empty(t, tokens)
 	})
-	require.NoError(t, err)
 }
 
 // TestLoginNoLocalAuth makes sure that logins for local accounts can not be