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

[Vault-5248] MFA support for api login helpers #14900

Merged
merged 11 commits into from
Apr 15, 2022
Merged

Conversation

VinnyHC
Copy link
Contributor

@VinnyHC VinnyHC commented Apr 4, 2022

This PR adds MFA support for auth methods (login helpers).

Single Phase MFA with Login Helper
Pass in credentials using same format as (*client).SetMFACreds() as variadic parameter. This is a simple convenience approach around our existing code.

// existing code
(*client).SetMFACreds(fmt.Sprintf("%s:%s", methodID, passcode))
(*client).Auth().Login(context.Context, AuthMethod)

// PR simplification
(*client).Auth().MFALogin(context.Context, AuthMethod, fmt.Sprintf("%s:%s", methodID, passcode))

Two Phase MFA with Login Helper
Flow when no credentials are provided. Performs checks on secret to confirm it contains MFARequirements for follow up call.

mfaSecret, err := (*client).Auth().MFALogin(context.Background(), &AuthMethod)
mfaPayload := map[string]interface{}{
  "method-id": []string{"a-passcode"},
}
authSecret, err := (*client).Auth().MFAValidate(context.Background(), mfaSecret, mfaPayload) 

This PR also adds the new method (*client).Sys().MFAValidateWithContext() a convenience method for calling sys/mfa/validate.

@vercel vercel bot temporarily deployed to Preview – vault April 4, 2022 22:41 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook April 4, 2022 22:41 Inactive
@VinnyHC VinnyHC force-pushed the vinnyhc/VAULT-5248_MFA-login branch from 5427d47 to 60750a5 Compare April 4, 2022 22:46
@vercel vercel bot temporarily deployed to Preview – vault April 4, 2022 22:46 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook April 4, 2022 22:46 Inactive
@VinnyHC VinnyHC force-pushed the vinnyhc/VAULT-5248_MFA-login branch from 60750a5 to a136c68 Compare April 4, 2022 22:51
@vercel vercel bot temporarily deployed to Preview – vault-storybook April 4, 2022 22:51 Inactive
@vercel vercel bot temporarily deployed to Preview – vault April 4, 2022 22:51 Inactive
@VinnyHC VinnyHC added this to the 1.11.0-rc1 milestone Apr 4, 2022
@vercel vercel bot temporarily deployed to Preview – vault-storybook April 4, 2022 23:14 Inactive
@vercel vercel bot temporarily deployed to Preview – vault April 4, 2022 23:14 Inactive
@VinnyHC VinnyHC requested review from a team and raskchanky April 4, 2022 23:18
@vercel vercel bot temporarily deployed to Preview – vault-storybook April 5, 2022 17:20 Inactive
@vercel vercel bot temporarily deployed to Preview – vault April 5, 2022 17:20 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook April 5, 2022 20:27 Inactive
@vercel vercel bot temporarily deployed to Preview – vault April 5, 2022 20:27 Inactive
Copy link
Contributor

@averche averche left a comment

Choose a reason for hiding this comment

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

Looks really good, left a couple small comments

api/auth.go Outdated Show resolved Hide resolved
api/auth_test.go Show resolved Hide resolved
api/auth_test.go Show resolved Hide resolved
api/sys_mfa.go Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview – vault-storybook April 5, 2022 20:46 Inactive
@vercel vercel bot temporarily deployed to Preview – vault April 5, 2022 20:46 Inactive
api/auth.go Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview – vault-storybook April 5, 2022 22:12 Inactive
@vercel vercel bot temporarily deployed to Preview – vault April 5, 2022 22:12 Inactive
"net/http"
)

func (c *Sys) MFAValidate(requestID string, payload map[string]interface{}) (*Secret, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't the only reason we provide a difference between MethodName and MethodNameWithContext because we're trying to maintain backwards compatibility for users who are already using the one that doesn't take a context?

In this case, this is a brand new method, so I thought we would just want to have MFAValidate take a context as its first arg.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could be convinced either way!
The motivation here was that this lives in the existing (*client).Sys() space where most methods have both MethodName and MethodNameWithContext whereas (*client).Auth() is the opposite (outside of (*client).Auth().Token()), so I wanted (*client).Auth().MFALogin() to be as close to the existing (*client).Auth().Login() as possible.

Copy link
Contributor

@raskchanky raskchanky left a comment

Choose a reason for hiding this comment

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

Looks great! I don't have anything to add beyond what's already been commented on. Thanks for updating the existing tests to use the new methods here!

@VinnyHC VinnyHC force-pushed the vinnyhc/VAULT-5248_MFA-login branch from db67e9c to 2b6ced7 Compare April 14, 2022 21:58
@vercel vercel bot temporarily deployed to Preview – vault-storybook April 14, 2022 21:58 Inactive
@vercel vercel bot temporarily deployed to Preview – vault April 14, 2022 21:58 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook April 14, 2022 22:06 Inactive
@vercel vercel bot temporarily deployed to Preview – vault April 14, 2022 22:06 Inactive
Copy link
Contributor

@averche averche left a comment

Choose a reason for hiding this comment

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

LGTM, awesome job!

VinnyHC and others added 2 commits April 15, 2022 08:39
Co-authored-by: Anton Averchenkov <84287187+averche@users.noreply.github.com>
Co-authored-by: Anton Averchenkov <84287187+averche@users.noreply.github.com>
@vercel vercel bot temporarily deployed to Preview – vault April 15, 2022 15:39 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook April 15, 2022 15:39 Inactive
@VinnyHC VinnyHC merged commit da3fb1d into main Apr 15, 2022
@VinnyHC VinnyHC deleted the vinnyhc/VAULT-5248_MFA-login branch April 15, 2022 18:13
kitography pushed a commit that referenced this pull request Apr 24, 2022
schultz-is pushed a commit that referenced this pull request Apr 27, 2022
schultz-is pushed a commit that referenced this pull request May 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants