Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[v14] Stop users from deleting their last passwordless device #35855

Merged
merged 2 commits into from Dec 18, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 30 additions & 2 deletions lib/auth/auth.go
Expand Up @@ -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 {
Expand All @@ -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 {
Expand All @@ -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)
Expand All @@ -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)
}
Expand Down
60 changes: 58 additions & 2 deletions lib/auth/grpcserver_test.go
Expand Up @@ -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)
Expand All @@ -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 {
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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)
Expand Down
10 changes: 8 additions & 2 deletions lib/auth/helpers_mfa.go
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