Skip to content

Commit

Permalink
[v13] Stop users from deleting their last passwordless device (#35856)
Browse files Browse the repository at this point in the history
* Stop users from deleting their last passwordless device (#35794)

* Fix RegisterTestDevice behavior when using WithPasswordless
  • Loading branch information
codingllama committed Dec 18, 2023
1 parent d5dd004 commit 2a9d3da
Show file tree
Hide file tree
Showing 3 changed files with 96 additions and 6 deletions.
32 changes: 30 additions & 2 deletions lib/auth/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -2991,8 +2991,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 {
Expand All @@ -3012,6 +3017,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 {
Expand All @@ -3031,6 +3041,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)
Expand All @@ -3054,6 +3068,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)
}
Expand Down
60 changes: 58 additions & 2 deletions lib/auth/grpcserver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,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)
Expand All @@ -303,7 +316,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 {
Expand Down Expand Up @@ -388,6 +401,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{
Expand Down Expand Up @@ -424,7 +453,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)
Expand Down
10 changes: 8 additions & 2 deletions lib/auth/helpers_mfa.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down

0 comments on commit 2a9d3da

Please sign in to comment.