AWS plugin tries to manually assume role

[wnkz]

Describe the bug
The asp command from aws plugin now assume role itself when role_arn is defined in the AWS profile.
This is not how it's supposed to work ; AWS CLI is perfectly capable of assuming roles on the fly and does not expose AWS credentials to environment variables.
Tracked it down to this PR: #8419.

I don't know if this is a specific need around MFA but checking only for role_arn attribute is not enough to trigger "manual" assume role.

Maybe @maksyms can comment as the PR author.

To Reproduce
Steps to reproduce the behavior, for example:

1. Enable this plugin 'aws'
2. Have a profile "myprofile" using role_arn
3. Run command 'asp myprofile'
4. Function tries to assume role and sets credentials to environment variables

Expected behavior
Unless MFA is needed, asp should only set AWS_PROFILE environment variables

Desktop (please complete the following information):

• OS / Distro: macOS
• Latest ohmyzsh update?: Yes
• ZSH Version: 5.8
• Terminal emulator: iTerm2

[maksyms]

Thank you for your report. I will investigate and revert.

[robvadai]

My setup:

1. AWS credentials under a profile, using asp profilename
2. Using https://github.com/joepjoosten/aws-cli-mfa-oh-my-zsh to authenticate with MFA
3. When now using asp otherprofile (where otherprofile has the role_arn and source_profile set up in credentials) I got:

Assuming role arn:aws:iam::9999999999999:role/myrole using profile otherprofile

An error occurred (AccessDenied) when calling the AssumeRole operation: User: arn:aws:iam::11111111111:user/original_user is not authorized to perform: sts:AssumeRole on resource: arn:aws:iam::9999999999999:role/myrole
Switched to AWS Profile: otherprofile

So it switches the profile while failing

[robvadai]

The script that actually works (for ideas), if I do aar otherprofile:

aar () {
	if [[ -z "$1" ]]
	then
		unset AWS_DEFAULT_PROFILE AWS_PROFILE AWS_EB_PROFILE
		echo AWS profile cleared.
		return
	fi
	local aws_profile_name=${1}
	local role_arn=$(aws configure --profile ${aws_profile_name} get role_arn)
	if [[ -z "$role_arn" ]]
	then
		echo Role ARN for profile \(${aws_profile_name}\) cannot be found.
		echo Please set the \'role_arn\' attribute on the profile.
		return 1
	fi
	local session_name="${aws_profile_name}-session"
	local credentials_output=$(aws sts assume-role --role-arn ${role_arn} --role-session-name ${session_name} --query '[Credentials.AccessKeyId,Credentials.SecretAccessKey,Credentials.SessionToken]' --output text | tr "\t" "\n")
	local credentials=("${(f)credentials_output}")
	local access_key_id=${credentials[1]}
	local secret_access_key=${credentials[2]}
	local session_token=${credentials[3]}
	aws configure --profile ${aws_profile_name} set aws_access_key_id ${access_key_id}
	aws configure --profile ${aws_profile_name} set aws_secret_access_key ${secret_access_key}
	aws configure --profile ${aws_profile_name} set aws_session_token ${session_token}
	export AWS_ACCESS_KEY_ID=${access_key_id}
	export AWS_SECRET_ACCESS_KEY=${secret_access_key}
	export AWS_SESSION_TOKEN=${session_token}
	export AWS_DEFAULT_PROFILE=${aws_profile_name}
	export AWS_PROFILE=${aws_profile_name}
	export AWS_EB_PROFILE=${aws_profile_name}
	echo Credentials set for AWS profile \(${aws_profile_name}\)
}

[wnkz]

I don't mind having different functions for MFA and whatnot but I don't think the asp function should go beyond setting up environment variables and should be kept that way for compatibility reasons.

[robvadai]

@wnkz that was the case with an earlier version of the AWS plugin, it was changed recently (not sure why, I only noticed it when pulled the master branch)

[maksyms]

@robvadai

So it switches the profile while failing

It actually didn't switch the profile - it set the environment variables. It could do error handling better, fair enough. I'll look into what can be done.

[maksyms]

@wnkz

I don't mind having different functions for MFA and whatnot but I don't think the asp function should go beyond setting up environment variables and should be kept that way for compatibility reasons.

Not so sure about that - the purpose of asp is to switch the profile, not to set environment variables, right?

[maksyms]

but checking only for role_arn attribute is not enough to trigger "manual" assume role.

@wknz Why do you say that? Can you name any other reasons to have role_arn in your profile config?

[maksyms]

@wknz I've read again through your description and I can confirm this is not a bug. Assuming IAM roles is how AWS recommends securing access to your resources: https://docs.aws.amazon.com/IAM/latest/UserGuide/best-practices.html#delegate-using-roles

If that is not what you want, you quite rightly hinted at the workaround: remove role_arn from ~/.aws/config for those profiles that do not require assuming the role.

[robvadai]

Still it doesn't work at least if MFA is enabled which should be standard for everyone.

[maksyms]

Still it doesn't work at least if MFA is enabled which should be standard for everyone.

@robvadai I'm checking your case now. Ideally, I'd have it in another bug (as the conclusion on the original one is above), but I'll check nevertheless.

In the meantime, I can tell you that at my company, over 20 engineers are using it with MFA successfully. So there must be something specific to your setup, which I'm checking.

[robvadai]

I appreciate that, however I tried it on AWS accounts for 2 companies (I'm a consultant) and doesn't work for either - these are 2 companies, located geographically in different places and completely different staff so it'd be unlikely the same people set up their AWS account.

[maksyms]

@robvadai , I'm looking. In the meantime, what is common between those 2 companies where you are a consultant? That's why I'm keen to check how you are doing it. 👍

In the meantime, here is a screenshot of using the plugin in one of my 4 different AWS accounts:

[robvadai]

My suspicion is our credentials are set up differently. Could you share your ~/.aws/credentials and ~/.aws/config for the profiles default and dev_admin_cli? Of course replace the access keys with ABC etc.

[maksyms]

@robvadai
Is there any chance you could share your ~/.aws/config, blanking out all the account numbers and company names?

Here is mine, from one of the machines, as an example:

[maksyms]

:-D

And credentials:

[robvadai]

OK I'm not using mfa_serial and external_id what are those? This issue might be resolved by some instructions in the README :)

[maksyms]

@robvadai You might be right about the README.

So mfa_serial is the registered MFA device ARN to perform MFA. If it doesn't exist, the script will ignore it.

And external_id is just an optional additional layer of security - that one is only necessary if you want to have an additional shared secret between whoever assumes the role and whoever set the role up. That one is completely optional and the plugin does not require it to work.

I'm trying to understand how you are assuming role - it doesn't seem like the MFA is set up in your case, right? It looks like your account can assume some roles without any additional authorization? What is the use case for that?

[maksyms]

We do not allow logging in or assuming any roles without MFA, as per the AWS best practices, so keen to understand the use case here.

[robvadai]

OK so I switch to a role, used that aar script to trigger MFA auth, and then switch to another role again. Certainly this was the case with an old version of the AWS plugin and I wasn't aware of its newer features. Let me change my config and get back on this.

[maksyms]

Thanks, @robvadai ! In the meantime, I'll try to think of a use case like yours.

Based on the error message you provided:

An error occurred (AccessDenied) when calling the AssumeRole operation: User: arn:aws:iam::11111111111:user/original_user is not authorized to perform: sts:AssumeRole on resource: arn:aws:iam::9999999999999:role/myrole

user/original_user does not have the right to assume role/myrole.

Looking at the code, the behaviour should be identical between aar and the new asp. Here is the line from aar that, according to you, succeeds for you:

local credentials_output=$(aws sts assume-role --role-arn ${role_arn} --role-session-name ${session_name} --query '[Credentials.AccessKeyId,Credentials.SecretAccessKey,Credentials.SessionToken]' --output text | tr "\t" "\n")

This is the line from asp that, according to you, fails for you:

local assume_cmd=(aws sts assume-role "--profile=$profile" "--role-arn $role_arn" "--role-session-name "$profile"" "$mfa_opt" "$extid_opt")

In both cases:

• role-arn will be the same
• role-session-name would be different by a -session at the end of the profile name (and that's irrelevant really)
• both mfa_opt and extid_opt would be empty

So both commands should execute the same actual command.

[maksyms]

Although I must say, the script gave me an idea as to how to get rid of jq as a dependency for asp to work. Nice one.

[robvadai]

Changed my config, tested for one profile and it works without the additional aar script!

However, when I didn't type in a session duration I got:

Please enter your MFA token for arn:aws:iam::1111111111:mfa/myuser:
123456
Please enter the session duration in seconds (900-43200; default: 3600, which is the default maximum for a role):

asp:31: command not found: sess_duration
Assuming role arn:aws:iam::11111111111111111:role/muyser using profile AAAAA
usage: aws [options] <command> <subcommand> [<subcommand> ...] [parameters]
To see help text, you can run:

  aws help
  aws <command> help
  aws <command> <subcommand> help
aws: error: argument --duration-seconds: expected one argument
Switched to AWS Profile: AAAAA

[maksyms]

Thanks for confirming and glad it worked! I will check why it behaves like that with session duration for you. The idea is that if you just press Enter without entering the session duration, the AWS default value will be used, because session duration variable is empty.

Do you mind sharing the details of the system on which you are running it? Specifically, what OS it is and what version of zsh (the shell itself) you are running it on? Thanks!

[robvadai]

Yes all good now, just another thing: I need [profile PROFILENAME] so the profile prefix in the config file while the credentials file doesn't need it. The Java SDK complains about this prefix too - could you change the script so we do not need this profile prefix?

[robvadai]

@maksyms macOS 10.15.7 (19H2)
ZSH version zsh 5.7.1 (x86_64-apple-darwin19.0)

[maksyms]

Yes all good now, just another thing: I need [profile PROFILENAME] so the profile prefix in the config file while the credentials file doesn't need it. The Java SDK complains about this prefix too - could you change the script so we do not need this profile prefix?

That's not me - that is the bog-standard AWS CLI, not me, that puts [profile PROFILENAME] in config file, so not sure how I can help. The old asp expected the same thing.

[maksyms]

However, when I didn't type in a session duration I got:

I reproduced this. This is a bug, @robvadai. Could you please report it as one and tag me in the report? I can then look at fixing it.

[wnkz]

@wnkz

I don't mind having different functions for MFA and whatnot but I don't think the asp function should go beyond setting up environment variables and should be kept that way for compatibility reasons.

Not so sure about that - the purpose of asp is to switch the profile, not to set environment variables, right?

but checking only for role_arn attribute is not enough to trigger "manual" assume role.

@wknz Why do you say that? Can you name any other reasons to have role_arn in your profile config?

@maksyms I am very confused by your questions 🤔
Have you ever read https://docs.aws.amazon.com/cli/latest/userguide/cli-configure-role.html ?

role_arn + source_profile is the very basic setup for using roles with AWS CLI. I don't use it often but it can handle MFA if I remember correctly.
With this configuration, AWS CLI automatically assumes the role configured in the profile, cache credentials and renew them when needed.

When you use a role, the AWS CLI caches the temporary credentials locally until they expire. The next time you try to use them, the AWS CLI attempts to renew them on your behalf.

The way to tell the AWS CLI which profile to use is via --profile flag or AWS_PROFILE environment variables.
IMHO, the asp function's only use is to list existing profiles and set environment variable easily so AWS CLI can do the rest of the work for you as intended.

Now, with the updated function the role is assumed manually, credentials are stored in environment variables and never renewed. Furthermore, assuming the role straight away cause problems with other type of more advanced configurations using credential_process or AWS SSO from CLI v2.

I really don't want to sound rude but this change only gave me trouble. I don't really know what made you do this and it might be a perfectly fine use case but this is a breaking change to me and should at least be implemented in a new function but not replace the existing one.

👋

[maksyms]

@wnkz Thanks a lot for your extensive write-up.

I'm sorry you feel the change gave you trouble. Judging by the reaction of a few other individuals, the change simplified their workflows already - and that's in addition to circa 30-ish users that were using this change prior to it being merged.

The reason for implementing the change is exactly related to the documentation you quoted (https://docs.aws.amazon.com/cli/latest/userguide/cli-configure-role.html).

The purpose of asp is to enable running AWS CLI and many other commands (in our case, kubectl and derivatives, for example) in the context of a specific profile, assuming a specific role, at all or at least without having to specify long command lines. For the cases where MFA was enforced, it was impossible to do so with the old asp. With this update, it is possible to do so.

The very specific example use case is like this:

1. I'm an AWS IAM user maksyms in AWS root account
2. I'm not allowed to run any commands in AWS CLI, apart from assuming a certain role
3. Once I assume a role (including cross-account one), I can run whatever commands I need within the context of that role. One example is kubectl

Here is the illustration:

Here is what is happening here:

1. No role is assumed, just a profile is set by setting AWS_PROFILE environment variable
2. Switching to the right K8s context
3. Trying to get the list of pods - as you can see, AWS CLI fails to assume the role
4. Calling the updated asp to perform MFA and assume the role instead of just setting the AWS_PROFILE environment variable
5. Successfully listing the pods
6. Clearing the profile using asp (which simply clears environment variables!) and setting AWS_PROFILE to the right profile again (which is what you are advocating)
7. Failing to get the list of pods, as AWS CLI fails to assume the role

I hope this explains the problem this PR solves in better details.

Now back to your use case.

What I'm guessing based on your description is that the way you prefer to use AWS CLI is by specifying --profile every time or setting the environment variable using a "dumb" asp and then having AWS CLI figure the rest out. As you can see, kubectl, as well as a few other tools does not work in that situation.

asp is a way to switch profiles, not just set environment variables.

I'm wondering though what the best way to help you is. Any suggestions beyond reverting the change?

[wnkz]

🙏 Please maintainers, read this before merging any more stuff in this plugin.
@53jk1 @mcornella I mention you because you reviewed and merged previous PRs.

The change introduced by #8419 is bad for multiple reasons:

• It is an unannounced breaking change
• It is, as best, useless because this "functionality" is supposed to be implemented in the AWS SDKs (and therefore AWS CLI) themselves

@maksyms

The only thing I saw with this change is that my daily workflow is completely broken and I'm pretty sure I'm not the only one.
I really appreciate someone contributing to this plugin but this is bad software development.

I understand your problem with tools like kubectl ; similar things happens with terraform when using MFA.
But again, those tools are responsible for using the correct AWS SDK versions that are supposed to read common configuration format and automatically assume roles using MFA or not.

The new asp function only works for your specific use case and completely ignores workflows using different tools (e.g. aws-okta, aws-vault, ...).

I saw this because I update oh-my-zsh daily but I can guarantee that many AWS users are about to be very mad very soon as they update oh-my-zsh. This is the only change that made me manually roll back oh-my-zsh in years.

My suggestion to find a compromise is:

• revert asp function to what it was previously (what you call dumb)
• rename current asp function to something new like asp-mfa or whatever.

[maksyms]

@wnkz It so happens that you are talking to a recent maintainer.

1. Not sure how you imagine an "announced" change in a software tool developed like this? Would be good to hear your thoughts.
2. The PR was available for anyone to look for about 1 year. A few people commented. No one raised this issue. I'm not saying it is not important - I'm just saying that I struggle to see how more feedback could be collected.
3. Given a choice of updating a lot of "broken" tools and updating one plugin that can take care of that, which approach is more pragmatic?

Ok, let me think about this.

One more thought: would introducing some env var to indicate whether to assume or not assume the role be an option?

[wnkz]

You were arbitrarily declared maintainer five hours ago so please stay calm and put the "maintainer" card away.
We're all human beings having a discussion about software. I'm sorry if I hurt your feelings ; there is nothing personal in my comments about the changes your brought.

1.

Well, it's basic development good practice ; you don't suddenly completely replace the behavior of a tool / function / method without giving users a warning and proper time to adapt or find workarounds. If you desperately need your change to go through, at least add a new function with a new name but dont force people to use your solution. In my point of view, you just broke something that wasn't with no solution but rollback to a previous version. I'm not saying this is useless for everyone, but saying it is useful for 30 does not mean it is for everyone else.

2.

The fact that PR #8419 was opened over a year ago is completely irrelevant. It's not because I'm a user of oh-my-zsh or this plugin that I'm supposed to go have a look at oh-my-zsh pull requests. I don't know how many OSS you are using but I'm pretty sure you don't have a look at every pull requests opened on every one of those repositories.

3.

Again, you're trying to fix something that isn't broken. Of course forcing every third party tools to correctly use AWS SDKs will be hard if not impossible but it is the only sane way of doing it.

To conclude, if you just added a new function with a new name and told everyone having your problem to use it instead of asp we would have never had this discussion ; your problem would still be solved and my workflow (and the one of many others) would not be broken.

I don't think I'll spend more time discussing this as I've presented most of my arguments. Right now, this plugin is useless to me so either you'll do something about it as the new maintainer or I will bring back the original functionality in another plugin.

Have a good day 👋

[maksyms]

@wnkz You are right, we are discussing software, and my only intent is to help make this piece of functionality better for the community, not just for you or me. Most of your suggestions are non-constructive and impossible to implement in this specific case; your tone reads aggressive, the approach comes across as idealistic, and the behaviour amounts to demagogic. While I understand that you are upset that a change in an open-source code that you pull in daily broke your workflow, it doesn't justify toxic behaviour. I suggest this stops now.

So far, I've been trying to focus on understanding and fixing the specific issue you are raising, that get in the way of your workflow, not on the tone of this conversation. The one constructive suggestion I have heard from you is to introduce a function with a different name to assume roles. Ok, perhaps, as this commit hasn't been out for long, it is a reasonable way forward. I will be implementing it now.

[robvadai]

Hi there,

it works fine when I use a role but it doesn't prompt for MFA for the source profile anymore.

I added mfa_serial to my source profile.

So this source profile has mfa_serial but not role_arn and source_profile obviously.

Any idea?

[robvadai]

https://github.com/maksyms/oh-my-zsh/blob/a656e4ea4fe4e715ab12265f54f03c7db52cf8b5/plugins/aws/aws.plugin.zsh#L49

This is the problem.

For source profiles we don't have role_arn's.....

[robvadai]

I fixed it but the code has some duplications, need to tidy it up: https://github.com/ohmyzsh/ohmyzsh/compare/master...robvadai:bugfix/aws-mfa-with-source?expand=1

[robvadai]

By the way, omitting the time still results in error:

acp myprofile
Please enter your MFA token for arn:aws:iam::1111111111:mfa/myuser:
123456
Please enter the session duration in seconds (900-43200; default: 3600, which is the default maximum for a role):


An error occurred (AccessDenied) when calling the GetSessionToken operation: MultiFactorAuthentication failed, unable to validate MFA code.  Please verify your MFA serial number is valid and associated with this user.
Switched to AWS Profile: myprofile

[maksyms]

@robvadai Could you please log it as a separate problem and share your config and credentials? I'm not sure I understand what you mean when you say "for source profiles we don't have role_arn" - I don't either.

The reason the second issue when you don't specify the duration now is because the MFA authentication fails, not any other reason - so it is not to do with the plugin.

[robvadai]

#9409

#9408