named Login MFA methods #18610
named Login MFA methods #18610
Conversation
hghaf099
requested review from
ncabatoff,
ccapurso,
raskchanky,
divyaac and
zofskeez
|
This looks good to me! I'm hesitant to approve given my lack of familiarity with our MFA implementation but I'll keep an eye out for other discussions. |
* make use of passcode factor consistent for all MFA types * improved type for MFA factors * add method name to login CLI * minor refactoring
Co-authored-by: Nick Cabatoff <[email protected]>
vault/login_mfa.go
Outdated
| } | ||
| } | ||
|
|
||
| // don't return an error unless all entries were invalid | ||
| if mfaFactor.passcode != "" { |
There was a problem hiding this comment.
Your current approach is forgiving: so long as we can find something in creds that looks right, you'll use it. That's an acceptable approach, and maybe the right one. The downside of being less strict is that it may result in confusing user behaviour, e.g. garbage was provided and ignored in the past, now I'm putting that garbage in a different position in the creds list and it's failing.
Not asking you to change this unless you agree that a stricter approach might be better. One reason we might not want to: maybe this isn't the right place. After all, why do we even have a slice of creds here? Simply because original the creds came from a header (and still can, in single-phase), and in HTTP headers can be provided multiple times. There's no legitimate use case that I know of for having more than one creds value for a given mfa method.
There was a problem hiding this comment.
I think being more strict ends up with better UX, at least users are not confused about it as you described. I will return an error immediately in the second case.
About the creds though, my motivation of using the slice was that if in the future we would want to add other factors, then those would all be in the same creds slice. So, it made sense to loop through creds.
Having said the above, we could still expect to have one passcode in the creds, and we can be strict about it.
Addresses https://hashicorp.atlassian.net/browse/VAULT-12529
Here is an outline of changes:
method_nameis added to all supported login MFA methods.** In creating an MFA method, the extracted namespace path is checked against the current namespace to make sure they match. An error is returned if there is a mismatch. In addition, if a method name is specified, we look through the cached MFA methods in the same namespace to make sure there is not any MFA config with the same name. An MFA method name should be unique across a single namespace.