Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add token cache for MFA users and credentials #4194

Merged
merged 12 commits into from
Sep 14, 2021
Merged

Add token cache for MFA users and credentials #4194

merged 12 commits into from
Sep 14, 2021

Conversation

Skarlso
Copy link
Contributor

@Skarlso Skarlso commented Sep 8, 2021

Description

NOTE: Going to re-write this whole thing. We don't need a lock that is this complex. We don't handle multiple creds, only the current one for example. And the mocking can be done a lot cleaner I think.

NOTE2: Significantly simplified the caching. We don't need locks as eksctl is not expected to be called concurrently. And we don't need role / cluster caching. We only cache per profile. I removed all the mocks, because I want to test with temporary filesystem rather than os mocks.

Closes #4052

TODO:

  • Switch to turn caching on. This shouldn't be the default so existing functionality doesn't change.
  • Test the new cacher.

Checklist

  • Added tests that cover your change (if possible)
  • Added/modified documentation as required (such as the README.md, or the userdocs directory)
  • Manually tested
  • Made sure the title of the PR is a good description that can go into the release notes
  • (Core team) Added labels for change area (e.g. area/nodegroup) and kind (e.g. kind/improvement)

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

  • Backfilled missing tests for code in same general area 🎉
  • Refactored something and made the world a better place 🌟

@Skarlso Skarlso added the kind/feature New feature or request label Sep 8, 2021
@Skarlso Skarlso marked this pull request as draft September 8, 2021 18:09
@Skarlso Skarlso marked this pull request as ready for review September 10, 2021 14:01
@Skarlso Skarlso requested a review from Callisto13 September 10, 2021 14:01
Copy link
Contributor

@Himangini Himangini left a comment

Choose a reason for hiding this comment

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

LGTM, although have a second pair of eyes for review 👀 👍🏻

@Skarlso Skarlso requested a review from aclevername September 14, 2021 07:54
@Skarlso
Copy link
Contributor Author

Skarlso commented Sep 14, 2021

@Callisto13 Are you okay now with merging this?

@Callisto13
Copy link
Contributor

The test still has that duplication in it, should that be there?

otherwise yeh all good

@Skarlso
Copy link
Contributor Author

Skarlso commented Sep 14, 2021

The test still has that duplication in it, should that be there?

otherwise yeh all good

@Callisto13 Wait, what duplication? I thought you mean the IsExpire check which was duplicated. :D The file content checking is not. It's checking that the file contains whatever the value object returned plus the expiry date and the profile at which it was cached. Or I really can't read. :D

@Skarlso Skarlso merged commit 378e624 into eksctl-io:main Sep 14, 2021
@Skarlso Skarlso deleted the cache_mfa_creds branch September 14, 2021 15:04
@ckdarby
Copy link

ckdarby commented Jan 10, 2022

# This works
aws s3 ls
....
....bunch of info...

# Right after try to create cluster prompted for MFA
eksctl create cluster
Assume Role MFA token code:
eksctl version                                                          
0.77.0

This is no longer working by the way. @Skarlso

@Skarlso
Copy link
Contributor Author

Skarlso commented Jan 10, 2022

You have to explicitly enable caching like this:

export EKSCTL_ENABLE_CREDENTIAL_CACHE=1

If you do that, does it still ask?

@Skarlso
Copy link
Contributor Author

Skarlso commented Jan 10, 2022

@ckdarby ^^

@ckdarby
Copy link

ckdarby commented Jan 10, 2022

@Skarlso It doesn't look like any of the AWS sts is carrying forward anymore.

export EKSCTL_ENABLE_CREDENTIAL_CACHE=1

## Valid sts
aws sts get-caller-identity
{...}

##Valid output
aws s3 ls
...


╰─ ~/bin ❯ eksctl create cluster                                                   
2022-01-10 14:57:13 [!]  Cache file /home/ckdarby/.eksctl/cache/credentials.yaml does not exist.
2022-01-10 14:57:13 [ℹ]  No cached credential available.  Refreshing...
Assume Role MFA token code: ******
Assume Role MFA token code: ******
Error: checking AWS STS access – cannot get role ARN for current session: AccessDenied: MultiFactorAuthentication failed, unable to validate MFA code.  Please verify your MFA serial number is valid and associated with this user.
	status code: 403

@Skarlso
Copy link
Contributor Author

Skarlso commented Jan 10, 2022

That is super strange. Can you check if the aws cli is working? It looks like it can't authenticate in the first place so it's unable to cache the credentials.

@ckdarby
Copy link

ckdarby commented Jan 10, 2022

@Skarlso Yes, I am able to do the normal aws calls without issue

@Skarlso
Copy link
Contributor Author

Skarlso commented Jan 10, 2022

No, I mean, can you try the aws cli WITH mfa?

@Skarlso
Copy link
Contributor Author

Skarlso commented Jan 10, 2022

Oh, right, you are saying that you are able to use aws normally. Ummm... can you try deleting the aws cred cache and try your MFA again please?

@ckdarby
Copy link

ckdarby commented Jan 10, 2022

rm -fr ~/.aws/cli/cache/
export AWS_PROFILE=st4

aws sts get-caller-identity
Enter MFA code for arn:aws:iam::**********:mfa/cdarby: 
## successful sts output {userId, Account, Arn}

export EKSCTL_ENABLE_CREDENTIAL_CACHE=1
eksctl create cluster
2022-01-10 15:39:26 [!]  Cache file /home/ckdarby/.eksctl/cache/credentials.yaml does not exist.
2022-01-10 15:39:26 [ℹ]  No cached credential available.  Refreshing...
Assume Role MFA token code:

🥲

@Skarlso
Copy link
Contributor Author

Skarlso commented Jan 10, 2022

And if you try again now, it's still not working?

@ckdarby
Copy link

ckdarby commented Jan 10, 2022

@Skarlso Correct.

@Skarlso
Copy link
Contributor Author

Skarlso commented Jan 10, 2022

Okay, we updated the aws go-sdk pretty recently. :/ They might have messed something up. I will have to take a look at that. In the mean time, maybe pop over and see in recent issue if there is something about this.

It's rather late in my TZ so I'm gonna drop off and take a look this tomorrow. Thanks for telling me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature New feature or request
Projects
None yet
5 participants