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

Store login MFA secret with tokenhelper #17040

Merged
merged 25 commits into from
Oct 26, 2022
Merged

Store login MFA secret with tokenhelper #17040

merged 25 commits into from
Oct 26, 2022

Conversation

mpalmi
Copy link
Contributor

@mpalmi mpalmi commented Sep 7, 2022

This PR updates the login path to store a token when using single-phase MFA validation. Still need to do some testing to see if this breaks any of the pre-existing functionality (particularly two-phase MFA).

Fixes: #16928

@mpalmi mpalmi marked this pull request as ready for review October 13, 2022 22:18
@mpalmi mpalmi requested review from hghaf099 and a team October 13, 2022 22:18
command/login.go Outdated Show resolved Hide resolved
@raskchanky
Copy link
Contributor

I don't think your Fixes line in the description is correct. It's pointing to this PR (which, while technically correct, is I'm guessing not what you were meaning to link to).

@mpalmi
Copy link
Contributor Author

mpalmi commented Oct 13, 2022

I don't think your Fixes line in the description is correct. It's pointing to this PR (which, while technically correct, is I'm guessing not what you were meaning to link to).

Oops! Thanks, fixed.

@VioletHynes
Copy link
Contributor

Is it possible to write a test for this behaviour? It would make me feel a bit comfier that we won't have regressions in this area without it being caught!

@mpalmi
Copy link
Contributor Author

mpalmi commented Oct 17, 2022

Is it possible to write a test for this behaviour? It would make me feel a bit comfier that we won't have regressions in this area without it being caught!

Sure thing. I was actually considering it, so thanks for the nudge!

@mpalmi mpalmi closed this Oct 17, 2022
@mpalmi mpalmi reopened this Oct 17, 2022
command/write.go Outdated Show resolved Hide resolved
command/login.go Outdated Show resolved Hide resolved
command/base.go Outdated Show resolved Hide resolved
command/base.go Outdated Show resolved Hide resolved
command/login.go Show resolved Hide resolved
command/base.go Outdated Show resolved Hide resolved
@mpalmi mpalmi force-pushed the fix-mfa-tokenhelper branch 2 times, most recently from cb5ea70 to 1940983 Compare October 26, 2022 15:50
command/base.go Outdated Show resolved Hide resolved
command/base.go Show resolved Hide resolved
Copy link
Contributor

@hghaf099 hghaf099 left a comment

Choose a reason for hiding this comment

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

Some minor comments. Otherwise looks great!

helper/testhelpers/testhelpers.go Outdated Show resolved Hide resolved
helper/testhelpers/testhelpers.go Show resolved Hide resolved
helper/testhelpers/testhelpers.go Show resolved Hide resolved
@mpalmi mpalmi force-pushed the fix-mfa-tokenhelper branch from 765e3fc to a900108 Compare October 26, 2022 18:14
@mpalmi mpalmi merged commit 1a2ee3a into main Oct 26, 2022
@mpalmi mpalmi deleted the fix-mfa-tokenhelper branch October 26, 2022 21:02
mpalmi added a commit that referenced this pull request Oct 26, 2022
* Store login MFA secret with tokenhelper
* Clean up and refactor tokenhelper paths
* Refactor totp test code for re-use
* Add login MFA command tests
* Use longer sleep times and sha512 for totp test
* Add changelog
mpalmi added a commit that referenced this pull request Oct 26, 2022
* Store login MFA secret with tokenhelper
* Clean up and refactor tokenhelper paths
* Refactor totp test code for re-use
* Add login MFA command tests
* Use longer sleep times and sha512 for totp test
* Add changelog
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.

vault no longer stores token in ~/.vault-token when login MFA duo auth is enabled
4 participants