wallix / awless

A Mighty CLI for AWS
http://awless.io/
Apache License 2.0
4.97k stars 263 forks source link

SIGSEGV when trying to use current head version with multi-profile and mfa #196

Closed deinspanjer closed 6 years ago

deinspanjer commented 6 years ago

I've had a working setup for a while like this: ~/.aws/credentials [default] key_id/access with read-only privs [me_mfa] key_id/access for my aws role which requires mfa

~/.aws/config [profile me] source_profile = me_mfa role_arn = ..Admin.. mfa_serial .../me

I did a go get -u today and the default profile was working fine, but when I tried to do anything with my me account, either by passing -p me or by using awl switch, it immediately returned a SIGSEGV:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x1d61b71]

goroutine 1 [running]:
github.com/wallix/awless/commands.hasEmbeddedRegionInSharedConfigForProfile(0xc4203f5198, 0x3, 0x7fff5fbff932, 0x3, 0x0, 0x0, 0x2e7b680)
    /Users/me/go/src/github.com/wallix/awless/commands/hooks.go:250 +0xc1
github.com/wallix/awless/commands.applyRegionAndProfilePrecedence(0x0, 0x0)
    /Users/me/go/src/github.com/wallix/awless/commands/hooks.go:70 +0xb9
github.com/wallix/awless/commands.initAwlessEnvHook(0x2ebf5c0, 0xc4203fb2c0, 0x0, 0x2, 0x22c0e48, 0x117f509)
    /Users/me/go/src/github.com/wallix/awless/commands/hooks.go:50 +0xa7
github.com/wallix/awless/commands.applyHooks.func1(0x2ebf5c0, 0xc4203fb2c0, 0x0, 0x2)
    /Users/me/go/src/github.com/wallix/awless/commands/hooks.go:38 +0x88
github.com/wallix/awless/vendor/github.com/spf13/cobra.(*Command).execute(0x2ebf5c0, 0xc4203fb260, 0x2, 0x2, 0x2ebf5c0, 0xc4203fb260)
    /Users/me/go/src/github.com/wallix/awless/vendor/github.com/spf13/cobra/command.go:730 +0x237
github.com/wallix/awless/vendor/github.com/spf13/cobra.(*Command).ExecuteC(0x2ec0a00, 0x0, 0x42, 0xc420036d70)
    /Users/me/go/src/github.com/wallix/awless/vendor/github.com/spf13/cobra/command.go:831 +0x2e4
github.com/wallix/awless/vendor/github.com/spf13/cobra.(*Command).Execute(0x2ec0a00, 0xc4201cbf78, 0xc4200aa058)
    /Users/me/go/src/github.com/wallix/awless/vendor/github.com/spf13/cobra/command.go:784 +0x2b
main.main()
    /Users/me/go/src/github.com/wallix/awless/main.go:22 +0x2d

I tried reverting back to the latest release installed view Homebrew and it works fine.

Please let me know what other info you might want to track this down.

simcap commented 6 years ago

@deinspanjer Thanks for reporting and sorry for the delay!! (I got resourced on another project).

Also I do not know which 2 versions (commit sha) you ran, I am pretty sure that it has to do with this commit.

I shall be fixing that today.

deinspanjer commented 6 years ago

Okay, happy to test it out. If necessary, I can probably go back and figure out the specific commits I was working against.

simcap commented 6 years ago

@deinspanjer Thanks for helping. It would be better if you give it a try with the latest master and show me the output. (Partial fix above should avoid nil reference and print what is wrong with the config I hope).

Can you do that?

deinspanjer commented 6 years ago

Hmm.. well, I tried a simple brew install --HEAD awless, and it fails with Error: No such file or directory - awless. I'll clean that up and try doing a pure go install instead.

➜  ~ brew install --HEAD awless
==> Installing awless from wallix/awless
==> Cloning https://github.com/wallix/awless.git
Updating /Users/dre/Library/Caches/Homebrew/awless--git
==> Checking out branch master
==> go run release.go -tag vHEAD-9fba342 -brew -arch amd64 -os darwin
Error: No such file or directory - awless
deinspanjer commented 6 years ago

Okay, installing via go worked fine. Here is the (sanitized) output I get from exercising the two profiles:

➜  ~ go get -u github.com/wallix/awless
# (Git commit installed: 9fba342)
➜  ~ ln -s ~/go/bin/awless /usr/local/bin
➜  ~ awl whoami
cannot check profile 'dre' has embedded region in shared config file: AssumeRoleTokenProviderNotSetError: assume role with MFA enabled, but AssumeRoleTokenProvider session option not set.
Assume Role MFA token code: ######
ResourceType: assumed-role, Resource: AdminRole/###, Id: XXX:YYY, Account: ZZZ
➜  ~ awl switch default
cannot check profile 'dre' has embedded region in shared config file: AssumeRoleTokenProviderNotSetError: assume role with MFA enabled, but AssumeRoleTokenProvider session option not set.
[info]    region precedence: 'us-east-1' loaded through profile 'default' (see AWS config files $HOME/.aws/{credentials,config})
➜  ~ awl whoami
Username: read_only, Id: XXX, Account: ZZZ

Attached policies (i.e. managed):
    - ReadOnlyAccess

Inlined policies: none
➜  ~ awl -p dre whoami
cannot check profile 'dre' has embedded region in shared config file: AssumeRoleTokenProviderNotSetError: assume role with MFA enabled, but AssumeRoleTokenProvider session option not set.
ResourceType: assumed-role, Resource: AdminRole/###, Id: XXX:YYY, Account: ZZZ
➜  ~
simcap commented 6 years ago

Thanks!

To clarify, the check that we make is only to detect if a region is in the running embedded profile as shown here.

The code for this check is legit but I am guessing some loading options are missing in the session.NewSessionWithOptions for more advanced configuration (failing in our case with the error AssumeRoleTokenProviderNotSetError).

@deinspanjer Can you confirm your configuration in ~/.aws/{credentials, config} is valid? (i.e. it works correctly with the AWS CLI). I am guessing yes ;)

If yes, on my side I will try some options to not fail on loading configuration such as yours.

deinspanjer commented 6 years ago

Yes, the config definitely works fine with core aws cli as well as boto3 and other libraries/utils.

Also note that awless actually successfully performs whatever operations are requested, it just gives that error first in the current version, and in the version released in brew on March 1, it gave no error at all.

deinspanjer commented 6 years ago

Here is the .aws/config and .aws.credentials file structures I'm running with:

config:

[default]
region = us-east-1
output = text

[profile dre]
source_profile = dre_mfa
role_arn = arn:aws:iam::xxx:role/AdminRole
mfa_serial = arn:aws:iam::xxx:mfa/dre
region = us-east-1
output = json

[profile gfw_xxx]
source_profile = gfw_ecr_xxx
role_arn = arn:aws:iam::xxx:role/AdminRole

[profile gfw_ecr_xxx]
region = us-east-1

[profile kops]
region = us-east-1
output = json
[profile ecr]
region = us-east-1

credentials:

[default]
aws_access_key_id = xxx
aws_secret_access_key = yyy

[gfw_ecr_dev]
aws_access_key_id = xxx
aws_secret_access_key = yyy
aws_session_token = zzz==

[dre_mfa]
aws_access_key_id = xxx
aws_secret_access_key = yyy

[ecr]
aws_access_key_id = xxx
aws_secret_access_key = yyy
deinspanjer commented 6 years ago

I found an issue in the k8s github that mentions an AssumeRoleTokenProvider option added a few months back: https://github.com/kubernetes/kops/issues/226#issuecomment-352783632

I'll try that out real quick and see if it has any effect.

Okay, I don't know the full ramifications of the change, but adding that option does definitely eliminate the error: https://github.com/wallix/awless/pull/199

simcap commented 6 years ago

Thanks for the input! I will have a look at that. In the meantime, using the current master will only print out the error. Everything should work fine expect when you have an embedded region in that config profile: in that case awless will not pick up the region. That is what need to be fixed.

simcap commented 6 years ago

This is the right fix concerning awless. We use this option in our official way to load the AWS session as shown here (compared to the hook quick session loading to see if a region is embedded).

When I implemented the code to look for an embedded region I overlook this option!

So your PR is valid and I will merge it. I will also revert back to returning an error instead of printing it (i.e. used for debugging that issue)

simcap commented 6 years ago

It got closed automatically!

@deinspanjer Anyway you can pull master that contains a commit and your PR. Any issues let me know. And thanks for the work.

Hopefully the version 0.1.10 should be out soon.