okta / okta-aws-cli

A CLI for having Okta as the IdP for AWS CLI operations
https://github.com/okta/okta-aws-cli
Other
128 stars 34 forks source link

Configuration priority is non-standard - env vars override CLI flags #120

Closed JohnPaton closed 1 year ago

JohnPaton commented 1 year ago

Experimentally, it seems that environment variables override CLI flags, at least for --profile (see below). This should either be documented (since it's nonstandard), or even better imo reversed so that CLI flags override environment variables as is usual for CLIs.

$ okta-aws-cli --version
okta-aws-cli version 1.1.0

$ OKTA_AWSCLI_PROFILE=default okta-aws-cli login --profile test ${OTHER_ARGS}
Open the following URL to begin Okta device authorization for the AWS CLI

https://okta.company.com/activate?user_code=ABCDEFG

? Choose an IdP: arn:aws:iam::012345678910:saml-provider/company-okta-idp
? Choose a Role: arn:aws:iam::012345678910:role/selected_role
Updated profile "default" in credentials file "/Users/jpaton/.aws/credentials".
monde commented 1 year ago

Thanks @JohnPaton I can document that. But where is the standard for app config preference (CLI flags vs .env file vs other config file vs env vars)? Giving ENV VAR precedence over CLI flags was a conscious choice deferring to principles of the 12 factor app https://12factor.net/config

JohnPaton commented 1 year ago

Thanks for the response @monde. I guess it's a philosophical difference but I see okta-aws-cli as not an application but a CLI tool, and therefore better suited following something like CLIG. Users should be able to set up their environment as they usually want it, but override those settings for single commands using flags (the aws --profile is a perfect example of this, and flags over env vars is how they handle it too). Feel free to try out other CLIs you use and see whether env vars or CLI flags take precedence.

I can live with simply having this fact documented of course, but my humble opinion is that it will cause confusion.

monde commented 1 year ago

Thanks @JohnPaton , I hear you. I should have elaborated further. The behavior we are trying to encourage is the use of ENV VARs (see eval examples in the README) and not writing creds to a file on disk which is considered a poor practice in terms of security standards even if creds have expiry. Given we are encouraging using creds as ENV VARs on output, input via ENV VARs is on par with that philosophy. There has been push back from the community that okta-aws-cli should be similar or even a replacement for other tools but we have taken the point of veiw that a) okta-aws-cli is its own product, not a copy b) Okta is a security company c) we will encourage good security practices where possible.

JohnPaton commented 1 year ago

Fair enough @monde, I can respect that, thanks for the explanation. I can see both sides of the coin!

Cheers,

John

palotasb commented 1 year ago

Hi @monde! I would like to add a few points towards making command line options higher priority than environment variables.

CLI options have higher precedence in most tools than environment variables. This is a very common convention with few exceptions. Examples: AWS CLI, Docker, Python, Git, and most Unix tools. You can test these by invoking them with contradicting env var and CLI option values and see that CLI optins always win.

It is more logical for CLI options to have precedence because they are more specific. In general, environment variables are set for an operating environment that contain many process invocations, and this is especially true for interactive CLI tools. Think about the env vars set in your ~/.bashrc or ~/.zshrc. While CLI options for processes by definition pertain only to that single process, and not a wider operating environment. The more specific options should take precedence over a more general option.

The config factor of the 12-factor app design does not apply. First, the 12-factor app concerns web applications and their deployment, not interactive CLI tools. Second, even though 12-factor prefers environment variables over config files, it does not mention command line options and does not imply that environment variables should take precedence over them.

Specific use case: Defaults set in a ~/.bashrc or ~/.zshrc or a CLI wrapper make command line options unusable. I believe it is a very common development setup to have default environment variables set in developers' ~/.bashrc or ~/.zshrc files. I would wager many customers of Okta have their own org's OKTA_ORG_DOMAIN and OKTA_OIDC_CLIENT_ID set there, since it never changes. We (I'm in the same team as John at Booking) have a wrapper script for okta-aws-cli, that sets these environment variables (if they are not set already), and also sets e.g. OKTA_AWSCLI_PROFILE="${AWS_PROFILE:-default}" (to sync up Okta-AWS-CLI env vars to AWS CLI env vars). However, this breaks command line options, because they can no longer be used to override the defaults set earlier on a per-invocation basis. Other tools would support this pattern by default since explicit command line options override environment variables.

Summary: CLI options are overwhelmingly higher precedence than environment variables in other Unix tools, and for good reason. Please reconsider your current policy on this in a future version of okta-aws-cli.

monde commented 1 year ago

Follow up for future readers:

I stand corrected, the viper library we used for command argument evaluation has an order precedence of flag, env, config file when there isn't a default value in the code of the app. We were doing some juggling of the "profile" argument given some quirks with viper default value evaluation. I've changed the evaluation to be that of viper w/o defaults which is what AWS CLI is doing as well: flag first, then env var.

12 factor apps are more than just web apps, fwiw.

JohnPaton commented 1 year ago

So just to clarify, flag, env, config file was already the precedence order for everything except profile (which is what prompted me to open this issue), and in the future profile will adhere to this order as well?