Skip to content
This repository has been archived by the owner on May 18, 2021. It is now read-only.

Implement config option for which MFA to use #108

Merged
merged 2 commits into from
Feb 5, 2019

Conversation

lsowen
Copy link
Contributor

@lsowen lsowen commented Dec 27, 2018

Supercedes #97/#73, and streamlines changes for this PR and #85.

@lsowen
Copy link
Contributor Author

lsowen commented Dec 27, 2018

@nickatsegment What is the preferred way(s) to set/override config options?

#97 used the AWS config file, #85 used a CLI switch and environment variables. Currently this PR uses the cli route, and I think env variable override would be useful. Thoughts?

@lsowen
Copy link
Contributor Author

lsowen commented Jan 9, 2019

hi @nickatsegment, happy new year! Wanted to see if you have some feedback on CLI vs AWS config to do the MFA settings.

@nickatsegment
Copy link
Contributor

@lsowen Hi, happy new year! Thanks for hanging in there despite our radio silence :)

Do you have (or can you imagine) a use case where you'd want different MFA methods depending on the account? For me, as a user, I only ever use 1 idP (same for every AWS account) and always pick the same method. So for my own personal use case, I'd prefer an env var because you can set it in your bash profile and not have to copy it to every aws-okta profile.

@nickatsegment nickatsegment self-requested a review January 10, 2019 02:15
@lsowen
Copy link
Contributor Author

lsowen commented Jan 10, 2019

@nickatsegment I think whichever method we use (cli flag or aws config) I will implement an ENV variable override.

Would it be useful to put plumbing in place to allow values to be set in the [okta] section and overridden in the individual [profile XYZ] sections? That way you could set it once, or override it if you wanted/needed.

@nickatsegment
Copy link
Contributor

@lsowen I'm guessing it would be useful for some users. Give it a shot and see how much complexity it adds. If it's a lot, I'd say wait for somebody with the need.

@Fauzyy
Copy link
Member

Fauzyy commented Jan 31, 2019

@lsowen #111 is in master now 👍 , if you could update this PR I'll merge it in soon! Excited to start using this feature

`MFADevice` is now `MFAConfig.DuoDevice` because it is only used
for DUO push auth
Order of precedence:
* CLI flags
* Environment variables
* Current profile
* Source profile
* `okta` "profile"

Update README
@lsowen
Copy link
Contributor Author

lsowen commented Feb 1, 2019

@Fauzyy just rebased, updated to use the new GetValue, and updated the README.

This version supports:

  • Setting from CLI
  • Setting from environment variables
  • Setting from AWS config

@Fauzyy
Copy link
Member

Fauzyy commented Feb 2, 2019

@lsowen this look good to me so far, I'll merge once I wrap up testing 👍 - thanks!

Copy link
Member

@Fauzyy Fauzyy left a comment

Choose a reason for hiding this comment

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

Great, thanks for this @lsowen 🥂

@manishtomar
Copy link

Strangely I don't see this working after taking the latest code and installing it.

$ aws-okta login --mfa-factor-type push ds
INFO[0001] Requesting MFA. Please complete two-factor authentication with your second device
INFO[0001] Select a MFA from the following list
INFO[0001] 0: OKTA (push)
INFO[0001] 1: OKTA (token:software:totp)
Select MFA method: ^C

Isn't it supposed to default to push instead of asking it? Did I misunderstand the documentation?

@lsowen
Copy link
Contributor Author

lsowen commented Feb 8, 2019

hi @manishtomar, while you can mix/match the methods of specifying which one you want to use (eg use environment variable and CLI switch), you have to specify both the provider and the factor-type for the config to have impact. eg

aws-okta login --mfa-provider OKTA --mfa-factor-type push ds

@manishtomar
Copy link

@lsowen Awesome. Thank you so much for the clarification and thanks a lot for this contribution!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants