From 254f9797b8e021b8eaa4a60f12999334bbffcc5e Mon Sep 17 00:00:00 2001 From: Alan Parra Date: Mon, 18 Dec 2023 14:42:07 -0300 Subject: [PATCH 1/2] Stop users from deleting their last passwordless device (#35794) --- lib/auth/auth.go | 32 ++++++++++++++++++-- lib/auth/grpcserver_test.go | 60 +++++++++++++++++++++++++++++++++++-- 2 files changed, 88 insertions(+), 4 deletions(-) diff --git a/lib/auth/auth.go b/lib/auth/auth.go index acb435bb8303f..02e54c00cc07f 100644 --- a/lib/auth/auth.go +++ b/lib/auth/auth.go @@ -3089,8 +3089,13 @@ func (a *Server) DeleteMFADeviceSync(ctx context.Context, req *proto.DeleteMFADe return trace.Wrap(err) } -// deleteMFADeviceSafely deletes the user's mfa device while preventing users from deleting their last device -// for clusters that require second factors, which prevents users from being locked out of their account. +// deleteMFADeviceSafely deletes the user's mfa device while preventing users +// from locking themselves out of their account. +// +// Deletes are not allowed in the following situations: +// - Last MFA device when the cluster requires MFA +// - Last resident key credential in a passwordless-capable cluster (avoids +// passwordless users from locking themselves out). func (a *Server) deleteMFADeviceSafely(ctx context.Context, user, deviceName string) (*types.MFADevice, error) { devs, err := a.Services.GetMFADevices(ctx, user, true) if err != nil { @@ -3110,6 +3115,11 @@ func (a *Server) deleteMFADeviceSafely(ctx context.Context, user, deviceName str sfToCount := make(map[constants.SecondFactorType]int) var knownDevices int var deviceToDelete *types.MFADevice + var numResidentKeys int + + isResidentKey := func(d *types.MFADevice) bool { + return d.GetWebauthn() != nil && d.GetWebauthn().ResidentKey + } // Find the device to delete and count devices. for _, d := range devs { @@ -3129,6 +3139,10 @@ func (a *Server) deleteMFADeviceSafely(ctx context.Context, user, deviceName str sfToCount[sf]++ knownDevices++ + + if isResidentKey(d) { + numResidentKeys++ + } } if deviceToDelete == nil { return nil, trace.NotFound("MFA device %q does not exist", deviceName) @@ -3152,6 +3166,20 @@ func (a *Server) deleteMFADeviceSafely(ctx context.Context, user, deviceName str return nil, trace.BadParameter("unexpected second factor type: %s", sf) } + // Stop users from deleting their last resident key. This prevents + // passwordless users from locking themselves out, at the cost of not letting + // regular users do it either. + // + // A better logic would be to apply this only to passwordless users, but we + // cannot distinguish users in that manner. + // See https://github.com/gravitational/teleport/issues/13219#issuecomment-1148255979. + // + // TODO(codingllama): Check if the last login type used was passwordless, if + // not then we could let this device be deleted. + if authPref.GetAllowPasswordless() && numResidentKeys == 1 && isResidentKey(deviceToDelete) { + return nil, trace.BadParameter("cannot delete last passwordless credential for user") + } + if err := a.DeleteMFADevice(ctx, user, deviceToDelete.Id); err != nil { return nil, trace.Wrap(err) } diff --git a/lib/auth/grpcserver_test.go b/lib/auth/grpcserver_test.go index 873ddb5ce22eb..90024568ae673 100644 --- a/lib/auth/grpcserver_test.go +++ b/lib/auth/grpcserver_test.go @@ -292,6 +292,19 @@ func TestMFADeviceManagement(t *testing.T) { }) } + // Register a 2nd passwordless device, so we can test removal of the + // 2nd-to-last resident credential. + // This is already tested above so we just use RegisterTestDevice here. + const pwdless2DevName = "pwdless2" + pwdless2Dev, err := RegisterTestDevice(ctx, + cl, + pwdless2DevName, + proto.DeviceType_DEVICE_TYPE_WEBAUTHN, + &TestDevice{Key: devs.WebKey}, + WithPasswordless(), + ) + require.NoError(t, err, "RegisterTestDevice") + // Check that all new devices are registered. resp, err = cl.GetMFADevices(ctx, &proto.GetMFADevicesRequest{}) require.NoError(t, err) @@ -302,7 +315,7 @@ func TestMFADeviceManagement(t *testing.T) { deviceIDs[dev.GetName()] = dev.Id } sort.Strings(deviceNames) - require.Equal(t, deviceNames, []string{pwdlessDevName, devs.TOTPName, devs.WebName, webDev2Name}) + require.Equal(t, []string{pwdlessDevName, pwdless2DevName, devs.TOTPName, devs.WebName, webDev2Name}, deviceNames) // Delete several of the MFA devices. deleteTests := []struct { @@ -387,6 +400,22 @@ func TestMFADeviceManagement(t *testing.T) { checkErr: require.NoError, }, }, + { + desc: "delete last passwordless device fails", + opts: mfaDeleteTestOpts{ + initReq: &proto.DeleteMFADeviceRequestInit{ + DeviceName: pwdless2DevName, + }, + authHandler: devs.webAuthHandler, + checkErr: func(t require.TestingT, err error, _ ...interface{}) { + require.ErrorContains(t, + err, + "last passwordless credential", + "Unexpected error deleting last passwordless device", + ) + }, + }, + }, { desc: "delete webauthn device by name", opts: mfaDeleteTestOpts{ @@ -423,7 +452,34 @@ func TestMFADeviceManagement(t *testing.T) { }) } - // Check the remaining number of devices + t.Run("delete last passwordless device", func(t *testing.T) { + authPref, err := srv.Auth().GetAuthPreference(ctx) + require.NoError(t, err, "GetAuthPreference") + + // Deleting the last passwordless device is only allowed if passwordless is + // off, so let's do that. + authPref.SetAllowPasswordless(false) + require.NoError(t, srv.Auth().SetAuthPreference(ctx, authPref), "SetAuthPreference") + + defer func() { + authPref.SetAllowPasswordless(true) + assert.NoError(t, srv.Auth().SetAuthPreference(ctx, authPref), "Resetting AuthPreference") + }() + + testDeleteMFADevice(ctx, t, cl, mfaDeleteTestOpts{ + initReq: &proto.DeleteMFADeviceRequestInit{ + DeviceName: pwdless2DevName, + }, + authHandler: func(t *testing.T, c *proto.MFAAuthenticateChallenge) *proto.MFAAuthenticateResponse { + resp, err := pwdless2Dev.SolveAuthn(c) + require.NoError(t, err, "SolveAuthn") + return resp + }, + checkErr: require.NoError, + }) + }) + + // Check no remaining devices. resp, err = cl.GetMFADevices(ctx, &proto.GetMFADevicesRequest{}) require.NoError(t, err) require.Empty(t, resp.Devices) From 887b6d25b6dfe1d039f4325e2ce4541e59caec5d Mon Sep 17 00:00:00 2001 From: Alan Parra Date: Mon, 18 Dec 2023 15:50:17 -0300 Subject: [PATCH 2/2] Fix RegisterTestDevice behavior when using WithPasswordless --- lib/auth/helpers_mfa.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/lib/auth/helpers_mfa.go b/lib/auth/helpers_mfa.go index c021bb399679a..ae922c39277b7 100644 --- a/lib/auth/helpers_mfa.go +++ b/lib/auth/helpers_mfa.go @@ -103,12 +103,18 @@ func (d *TestDevice) registerStream( return trace.Wrap(err) } + var usage proto.DeviceUsage + if d.passwordless { + usage = proto.DeviceUsage_DEVICE_USAGE_PASSWORDLESS + } + // Inform device name and type. if err := stream.Send(&proto.AddMFADeviceRequest{ Request: &proto.AddMFADeviceRequest_Init{ Init: &proto.AddMFADeviceRequestInit{ - DeviceName: devName, - DeviceType: devType, + DeviceName: devName, + DeviceType: devType, + DeviceUsage: usage, }, }, }); err != nil {