common-fate / granted

The easiest way to access your cloud.
https://granted.dev
MIT License
1.06k stars 94 forks source link

IAM + MFA does not work after adding credentials to the granted keyring #543

Closed subpardaemon closed 10 months ago

subpardaemon commented 11 months ago

Hi, i can confirm that this is an issue still present in 0.20.0.

We use classic IAM + role assumption. If we keep the credentials in its plaintext form, ie. not using the credential_process = granted credential-process --profile=base line granted adds after an import, then MFA works splendidly. Once we add the credentials to the keyring and this line appears in ~/.aws/config it no longer asks for the MFA even if the session expires. Tried granted cache clear with removing just the current session, this does not help. Removing the cached credentials removes the whole keyring, therefore leaving Granted in an inoperable state (empty ~/.aws/credentials file).

It looks like the credential-process module does not have a concept of needing to ask for an MFA if Granted is not being used with SSO.

~/.aws/credentials before encoding (when MFA prompt works):

[base]
aws_access_key_id=<AWS access key from AWS setup>
aws_secret_access_key=<AWS access key secret from AWS setup>

~/.aws/config before encoding:

[profile base]
region             = us-east-1
mfa_serial         = arn:aws:iam::<whatever>

After granted credentials import base: ~/.aws/credentials is empty

~/.aws/config before encoding:

[profile base]
region             = us-east-1
mfa_serial         = arn:aws:iam::<whatever>
credential_process = granted credential-process --profile=base
subpardaemon commented 11 months ago

See https://github.com/common-fate/granted/issues/405#issuecomment-1748718937 -- that issue is closed but the issue itself is still present. My biggest concern is that we cannot use granted without MFA, but we cannot use granted safely with the cleartext secrets.

subpardaemon commented 11 months ago

There is a very convoluted workaround, tho, once the credentials are encrypted:

assume base --export
# comment out credential_process line in ~/.aws/config
granted cache clear
# choose session and base
assume base
# will ask for MFA
# empty out ~/.aws/credentials

But to do this every day... 🀣

subpardaemon commented 11 months ago

i've browsed through the code and i think i have a few ideas where the process might be going wrong:

i assume pkg/cfaws/assumer_aws_iam.go is correct as it does work correctly when not using the credential process.

@shwethaumashanker let me know if i can help in debugging. i'm not familiar with go but i'm a developer so i can be of help.

shwethaumashanker commented 11 months ago

Thank you so much for reporting the issue and providing the additional context, @subpardaemon. I'm working on replicating and debugging the issue. I'll keep you informed once I have any updates 😁

subpardaemon commented 11 months ago

@shwethaumashanker let me know if i can help. If there is a debug mode in the app, generating logs, i'm more than happy to use it and send you the results. I can also debug the environment variables, etc.

mfzl commented 10 months ago

Just to add some context, this was fixed with #429 because I had a similar issue, it could even be the same issue

The work done on that PR is now refactored and merged with #474, which reintroduced the issue for me.

The issue is that:

https://github.com/common-fate/granted/blob/fcb95bbd07261d2cbaf46738b186a19623378fe3/pkg/cfaws/assumer_aws_iam.go#L25-L28

Assumes that if credentials are stored in secure storage it's good to go, but it should still consider the case of MFA in that block and ask for it. Which is what #429 did

subpardaemon commented 10 months ago

any progress regarding this? my team has a commitment to migrate to granted, and this is a blocking issue. let me know if i can help with this.

shwethaumashanker commented 10 months ago

Hi @subpardaemon, I apologize for the delay. Thank you for using Granted and recommending it to your team. I am able to replicate the error. I've encountered some unexpected hurdles during debugging, but I'm working on resolving it

shwethaumashanker commented 10 months ago

@subpardaemon @mfzl, v0.20.3 includes the fix for this issue. Please let us know if it works for you!

subpardaemon commented 10 months ago

@subpardaemon @mfzl, v0.20.3 includes the fix for this issue. Please let us know if it works for you!

do you know when 0.20.3 is expected to hit brew? i did an upgrade this morning but it only got me to 0.20.2 from 0.20.0.

subpardaemon commented 10 months ago

my bad for not executing brew update before brew upgrade. πŸ˜„ will test 0.20.3 tomorrow as i already have a valid token for today.

mfzl commented 10 months ago

Hey @shwethaumashanker πŸ‘‹πŸΎ

It is working for me. Thanks so much.

subpardaemon commented 10 months ago

I can also confirm it's working. Thank you @shwethaumashanker for the speedy fix!