vault icon indicating copy to clipboard operation
vault copied to clipboard

Add Login MFA support for Okta TOTP

Open cognifloyd opened this issue 2 years ago • 15 comments

Closes #24562

I tested this by running the tests in vault/external_tests/identity/login_mfa_okta_test.go. I ran them separately with Okta push and with Okta TOTP. I had to modify the tests to get them working with Okta TOTP since that is a new feature.

I cannot test the Policy MFA feature as that is only in Vault Enterprise. Would someone from HashiCorp please run that test and make sure I didn't break anything?

cognifloyd avatar Dec 15 '23 21:12 cognifloyd

CLA assistant check
All committers have signed the CLA.

hashicorp-cla avatar Dec 15 '23 21:12 hashicorp-cla

I needed a backport to 1.12.x, so I went ahead and created a branch on top of each of these release branches. Also, this means that my contribution can be used under the appropriate license of the older branches.

I have run the Okta TOTP and Okta Push test in each of these backport branches successfully.

I'm sharing this here for reference, I don't expect HashiCorp to release this feature for any previous releases.

cognifloyd avatar Dec 16 '23 04:12 cognifloyd

Thanks for this contribution and I apologize for the delay! We'll have our engineer on Community assignment check it out ASAP. Again, I'm sorry it took longer than we'd like for the review to happen.

heatherezell avatar Jan 18 '24 20:01 heatherezell

Hey there! Just wanted to give you a heads up that I spent most of my day on this and I've made progress, but I'll be handing this over to the next engineer on Monday when our 'shifts' change over, and making sure they have all of the context to give this their full attention, as I don't want to rush this review and I had some trouble with my Okta setup.

I mostly wanted to let you know so that you know we're working on it, and also the heads up not to be alarmed when someone else starts looking at this (it's not that I abandoned it, quite the opposite)! Thanks again for the submission!

VioletHynes avatar Jan 19 '24 21:01 VioletHynes

It took me some time to get my dev Okta setup as well. 🙂 Thank you for reviewing and for letting me know how it's going.

cognifloyd avatar Jan 19 '24 23:01 cognifloyd

@cognifloyd wanted to keep you up to date with this as it's been a few days.

This week is the last week before code freeze for out next major vault release (1.16) RC so, although everyone is really excited about this feature we don't think that we can give enough testing attention and lead time to others who need to know what is changing etc. to get it merged before Friday. We still want to target getting merged for 1.17 though!

Violet handed over to me so I will continue to work on testing and getting this ready to merge, but I'll need to focus this week on some other priorities that need to be done before Friday.

I'll pick testing this PR up next week, assess what's left to do and either work through or get the completion on our roadmap in time for 1.17. I'll keep you posted!

Thanks again for a great contribution, we're really excited to have a meaningful feature contribution and really grateful for all your hard work on this so far ❤️ .

banks avatar Jan 24 '24 14:01 banks

After some days of learning about Okta...

I've verified the following:

  • TestOktaEngineMFA appears to behave the same(ish) way before and after this PR other than the sensible ENV var changes.
    • This one is a bit odd though because I'm not really sure that it's behaving correctly - even when my Okta org is setup to require MFA it still seems to allow a login without pinging my phone and passes the test, but this was true before too so is more likely my Okta set up.
  • TestInteg_LoginMFAOkta works both with OKTA_USE_TOTP unset and set to 1
    • In Community Edition ✅
    • In Enterprise ✅
  • TestInteg_PolicyMFAOkta ❌ in Enterprise (after merging this branch changes locally)
    • Needed a few minor tweaks (e.g. to add use_passcode to the Ent-only endpoint schema
    • So far I've not gotten that test to pass possibly due to configuration issues on the OKTA side. I'm still seeing TestInteg_PolicyMFAOkta.core0.system.mfa: validation failed: method=my_okta error="no users found for e-mail address" even those the OKTA_USERNAME which is set to the entitie's metadata.email and then consumed from there in the username_format config all looks to be correct. The same credentials/account worked for LoginMFA so it's likely something in the config here rather than this PR.

Next Steps

I'm going to keep working on this for the rest of my day here to see if I can work out what is going on with that PolicyMFA test, but either way I'm not going to get this merged today.

I'm going to create internal issue tracking the last bits to get this merged for 1.17 and make sure that is triaged by the core team so they can ensure it happens early rather than be handed off to successive engineers on our sustaining rotation.

@cognifloyd this does probably mean that it will be a few weeks before it is picked up again as we will wait until after the 1.16 release branch is cut before we merge this now. That is going to be 10 days from now earliest. Apologies that is probably slower than you might have hoped! We have the usual organizational machinery to align with here!

I don't anticipate any issues with this getting merged for 1.17. TODO (internally):

  • Get PolicyMFA test to pass in Ent
  • PR the Ent-only counterpart to this just adding the required plumbing to ent-only code
  • Figure out whether we need to refactor any of the other Okta testing while we are here since there is still some uncertainty about whether the test behavior we see is actually expected. (That behavior isn't changed by this PR so not a blocker but some of the comments added to TestOktaEngineMFA provoked interesting uncertainty about how things were intended to work around Okta auth method + MFA).

banks avatar Feb 02 '24 16:02 banks

Thanks for working on this! I'm glad to see progress even if it's not quite to the finish line.

If it would be helpful, I can jump on a call to show you how I configured my dev okta org.

cognifloyd avatar Feb 02 '24 17:02 cognifloyd

I reworded the comment. I think it makes more sense now. wdyt?

cognifloyd avatar Feb 03 '24 00:02 cognifloyd

Now that the 1.16 branch has been cut, can we get this merged for 1.17? release/1.16.x branch: https://github.com/hashicorp/vault/tree/release/1.16.x 1.16.0 release: https://github.com/hashicorp/vault/releases/tag/v1.16.0 (released March 26, 2024) 1.16.1 release: https://github.com/hashicorp/vault/releases/tag/v1.16.1 (released April 4, 2024)

cognifloyd avatar Feb 20 '24 17:02 cognifloyd

Hey 👋🏼 any news on this PR's progress? Thank you all for your valuable work!

kda-jt avatar Apr 05 '24 09:04 kda-jt

@banks @VioletHynes @hsimon-hashicorp 1.16 has been released. What is left before we can merge this?

cognifloyd avatar Apr 10 '24 16:04 cognifloyd

@banks @VioletHynes @hsimon-hashicorp 1.16 has been released. What is left before we can merge this?

Hi there! Thanks for the ping. I've bubbled this up to our teams, as there's some additional work we'd like to do for this. We'll try not to keep you waiting too long. Please feel free to re-ping as needed at any time! :)

heatherezell avatar Apr 10 '24 22:04 heatherezell

@hsimon-hashicorp @VioletHynes @banks Has anyone picked up / started working on the internal tasks required to get this merged? Is there anything I can do to facilitate that work?

cognifloyd avatar Apr 25 '24 21:04 cognifloyd

Amazing. This is looking great!

Thanks again @cognifloyd for the awesome feature.

We're going to have to sit on this approval for a little bit until the 1.16 release branch is cut probably next week before we merge, we'll also need to have some code ready on the Enterprise side to ensure it merges smoothly (e.g. to add the use_passcode field to the ent-only MFA endpoints). I've created a ticket internally to track that though and set the milestone to 1.17 so it should certainly happen now, hopefully soon but that will be depend on how soon others on the team transition onto tasks for that release!

@banks @VioletHynes @hsimon-hashicorp This PR was approved at the beginning of February. It is now half way through May. Has anyone been assigned your internal enterprise ticket yet?

cognifloyd avatar May 18 '24 00:05 cognifloyd

@banks @VioletHynes @hsimon-hashicorp @peteski22 Please please please would someone please work on the enterprise side of this? What is the hold up? Is there something I can sign so that I can work on the enterprise code?

Another month has passed and another release has been cut:

  • https://github.com/hashicorp/vault/releases/tag/v1.17.0
  • https://github.com/hashicorp/vault/tree/release/1.17.x

cognifloyd avatar Jun 12 '24 20:06 cognifloyd

@banks @VioletHynes @hsimon-hashicorp @peteski22 Please please please would someone please work on the enterprise side of this? What is the hold up? Is there something I can sign so that I can work on the enterprise code?

Another month has passed and another release has been cut:

Hi there! I'm sorry for the delay. At this time I don't have any news to share, but I am going to bubble this feedback up to our engineering and product teams. I appreciate your efforts, as well as your patience. Thanks so much for hanging in there with us.

heatherezell avatar Jun 12 '24 21:06 heatherezell