Skip to content
New issue

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

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

Already on GitHub? # to your account

mfa: prevent the user from deleting the last MFA device #6585

Merged
merged 2 commits into from
Apr 27, 2021
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 37 additions & 1 deletion lib/auth/grpcserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -1723,13 +1723,49 @@ func (g *GRPCServer) DeleteMFADevice(stream proto.AuthService_DeleteMFADeviceSer
// Find the device and delete it from backend.
devs, err := auth.GetMFADevices(ctx, user)
if err != nil {
return trace.Wrap(err)
return trail.ToGRPC(err)
}
authPref, err := auth.GetAuthPreference()
if err != nil {
return trail.ToGRPC(err)
}
var numTOTPDevs, numU2FDevs int
for _, d := range devs {
switch d.Device.(type) {
case *types.MFADevice_Totp:
numTOTPDevs++
case *types.MFADevice_U2F:
numU2FDevs++
default:
log.Warningf("Unknown MFA device type: %T", d.Device)
}
}
for _, d := range devs {
// Match device by name or ID.
if d.Metadata.Name != initReq.DeviceName && d.Id != initReq.DeviceName {
continue
}

// Make sure that the user won't be locked out by deleting the last MFA
// device. This only applies when the cluster requires MFA.
switch authPref.GetSecondFactor() {
case constants.SecondFactorOff, constants.SecondFactorOptional: // MFA is not required, allow deletion
case constants.SecondFactorOTP:
if numTOTPDevs == 1 {
return trail.ToGRPC(trace.BadParameter("cannot delete the last OTP device for this user; add a replacement device first to avoid getting locked out"))
}
Comment on lines +1753 to +1756
Copy link
Contributor

@andrejtokarcik andrejtokarcik Apr 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does SecondFactorOTP mean there can be no U2F devices stored for the user? If a user had a single OTP device but he intended to delete some of the (now unusable) U2F devices, wouldn't this error be incorrectly returned since the type of the device to be deleted doesn't seem to be taken into account?

Analogically for SecondFactorU2F when deleting an OTP device.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block only returns an error if cluster requires SecondFactorOTP and there is exactly 1 OTP device registered (regardless of how many U2F devices there are).
Same thing for SecondFactorU2F, the error only happens when there's 1 U2F device, regardless of any OTP devices.

Maybe I misunderstand the question?

Copy link
Contributor

@andrejtokarcik andrejtokarcik Apr 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I understand that.

My worry is that this error would be returned even if a user requested to delete an U2F device and not the last OTP device. In other words, the type of the device to be deleted should be checked and the user should not be prevented from deleting an U2F device even if there is only a single OTP device left.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OTP is a sub-type of MFA, not a separate group.
Each user has multiple MFA devices. Each of those devices is either U2F or OTP.
In the loop above I calculate the number of devices of each type (numTOTPDevs and numU2FDevs).
Does that clarify the logic?

case constants.SecondFactorU2F:
if numU2FDevs == 1 {
return trail.ToGRPC(trace.BadParameter("cannot delete the last U2F device for this user; add a replacement device first to avoid getting locked out"))
}
case constants.SecondFactorOn:
if len(devs) == 1 {
return trail.ToGRPC(trace.BadParameter("cannot delete the last MFA device for this user; add a replacement device first to avoid getting locked out"))
}
default:
log.Warningf("Unknown second factor value in cluster AuthPreference: %q", authPref.GetSecondFactor())
}

if err := auth.DeleteMFADevice(ctx, user, d.Id); err != nil {
return trail.ToGRPC(err)
}
Expand Down
98 changes: 97 additions & 1 deletion lib/auth/grpcserver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ func TestMFADeviceManagement(t *testing.T) {
// Enable U2F support.
authPref, err := services.NewAuthPreference(types.AuthPreferenceSpecV2{
Type: teleport.Local,
SecondFactor: constants.SecondFactorOn,
SecondFactor: constants.SecondFactorOptional,
U2F: &types.U2F{
AppID: "teleport",
Facets: []string{"teleport"},
Expand Down Expand Up @@ -922,3 +922,99 @@ func TestIsMFARequired(t *testing.T) {
})
}
}

func TestDeleteLastMFADevice(t *testing.T) {
ctx := context.Background()
srv := newTestTLSServer(t)
clock := srv.Clock().(clockwork.FakeClock)

// Enable MFA support.
authPref, err := services.NewAuthPreference(types.AuthPreferenceSpecV2{
Type: teleport.Local,
SecondFactor: constants.SecondFactorOn,
U2F: &types.U2F{
AppID: "teleport",
Facets: []string{"teleport"},
},
})
require.NoError(t, err)
err = srv.Auth().SetAuthPreference(authPref)
require.NoError(t, err)

// Create a fake user.
user, _, err := CreateUserAndRole(srv.Auth(), "mfa-user", []string{"role"})
require.NoError(t, err)
cl, err := srv.NewClient(TestUser(user.GetName()))
require.NoError(t, err)

// Register a U2F device.
var u2fDev *mocku2f.Key
testAddMFADevice(ctx, t, cl, mfaAddTestOpts{
initReq: &proto.AddMFADeviceRequestInit{
DeviceName: "u2f-dev",
Type: proto.AddMFADeviceRequestInit_U2F,
},
authHandler: func(t *testing.T, req *proto.MFAAuthenticateChallenge) *proto.MFAAuthenticateResponse {
// The challenge should be empty for the first device.
require.Empty(t, cmp.Diff(req, &proto.MFAAuthenticateChallenge{}))
return &proto.MFAAuthenticateResponse{}
},
checkAuthErr: require.NoError,
registerHandler: func(t *testing.T, req *proto.MFARegisterChallenge) *proto.MFARegisterResponse {
u2fRegisterChallenge := req.GetU2F()
require.NotEmpty(t, u2fRegisterChallenge)

mdev, err := mocku2f.Create()
require.NoError(t, err)
u2fDev = mdev
mresp, err := mdev.RegisterResponse(&u2f.RegisterChallenge{
Challenge: u2fRegisterChallenge.Challenge,
AppID: u2fRegisterChallenge.AppID,
})
require.NoError(t, err)

return &proto.MFARegisterResponse{Response: &proto.MFARegisterResponse_U2F{U2F: &proto.U2FRegisterResponse{
RegistrationData: mresp.RegistrationData,
ClientData: mresp.ClientData,
}}}
},
checkRegisterErr: require.NoError,
wantDev: func(t *testing.T) *types.MFADevice {
wantDev, err := u2f.NewDevice(
"u2f-dev",
&u2f.Registration{
KeyHandle: u2fDev.KeyHandle,
PubKey: u2fDev.PrivateKey.PublicKey,
},
clock.Now(),
)
require.NoError(t, err)
return wantDev
},
})

// Try to delete the only MFA device of the user.
testDeleteMFADevice(ctx, t, cl, mfaDeleteTestOpts{
initReq: &proto.DeleteMFADeviceRequestInit{
DeviceName: "u2f-dev",
},
authHandler: func(t *testing.T, req *proto.MFAAuthenticateChallenge) *proto.MFAAuthenticateResponse {
require.Len(t, req.U2F, 1)
chal := req.U2F[0]

mresp, err := u2fDev.SignResponse(&u2f.AuthenticateChallenge{
Challenge: chal.Challenge,
KeyHandle: chal.KeyHandle,
AppID: chal.AppID,
})
require.NoError(t, err)

return &proto.MFAAuthenticateResponse{Response: &proto.MFAAuthenticateResponse_U2F{U2F: &proto.U2FResponse{
KeyHandle: mresp.KeyHandle,
ClientData: mresp.ClientData,
Signature: mresp.SignatureData,
}}}
},
checkErr: require.Error,
})
}