mfa: prevent the user from deleting the last MFA device #6585
Conversation
When the cluster requires MFA for all users (when `second_factor` is `on`, `u2f` or `totp`, and not `off` or `optional`), users could lock themselves out by deleting the last device. Prevent that. Fixes #5803
awly
force-pushed
the
andrew/delete-last-mfa-device
branch
from
81ec8b6 to
f113536
Compare
Separate by the type of the device and which type the cluster enforces.
awly
force-pushed
the
andrew/delete-last-mfa-device
branch
from
f113536 to
6257686
Compare
* mfa: prevent the user from deleting the last MFA device When the cluster requires MFA for all users (when `second_factor` is `on`, `u2f` or `totp`, and not `off` or `optional`), users could lock themselves out by deleting the last device. Prevent that. Fixes #5803 * Make last MFA device deletion check more strict Separate by the type of the device and which type the cluster enforces.
| 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")) | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
* mfa: prevent the user from deleting the last MFA device When the cluster requires MFA for all users (when `second_factor` is `on`, `u2f` or `totp`, and not `off` or `optional`), users could lock themselves out by deleting the last device. Prevent that. Fixes #5803 * Make last MFA device deletion check more strict Separate by the type of the device and which type the cluster enforces.
When the cluster requires MFA for all users (when
second_factorison,u2fortotp, and notofforoptional), users could lockthemselves out by deleting the last device. Prevent that.
Fixes #5803