-
Notifications
You must be signed in to change notification settings - Fork 3
fix: credential method selection for chained vs non-chained MFA #87
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
base: main
Are you sure you want to change the base?
Conversation
Thanks @propilideno ! I'll review this shorty, but would you mind opening a PR from a for of this repo instead of the upstream - I think that's why PR checks might be failing :) |
Hello @mbevc1, it's failing because I used |
It's checking PR title, which I've fixed. But I think failure now might be related to integration access. I was wondering if we could test this with the for from here and now upstream. Mind, not a blocker for review of the change. |
You're right, it seems to be it.
|
Yeah, I've already changed that but I reckon using other forks might break this 🤔 . Would you be able to give it a go to test this out? |
It looks working for me: propilideno#1 I don't truly understand if it was the test you was looking for. Or if you asked to me to fork from ByteNess/aws-vault |
So, back to the PR - this is now breaking source profile chaining where it keeps asking for MFA and not assuming the role:
|
Oh, you are right. I do not use this feature, so i couldn't notice it. I also runned unit tests but all cases it passed. |
I'm busy right now, i'll review it ASAP |
No worries mate! |
Hey @propilideno 👋 Did you manage to review this? |
Hello @mbevc1, I'll do it this weekend. The last one i was traveling. |
No worries, thanks! |
Hello @mbevc1, sorry for the slow response. I didn’t have time to work on my open source contributions this month. I tested some scenarios, and everything below is working for me:
I'll wait for your tests to confirm if the issue is resolved. Best Regards, |
Hi @propilideno . Thanks for revisiting this, but there still seems to be an issue using chained roles:
vs. now:
|
Introduction
After my changes, I notice that I broke some features when I was trying to pass the session duration to my
PROFILENAME
.In this example, you can notice that we are dealing it as a
(chained MFA)
, that is not the truth.Fixed
This problem occurs because the:
aws-vault/vault/vault.go
Lines 344 to 346 in 7500108
Notice that previously I was handling both methods as chained credentials, so to use this feature
--duration
, we must use the right method at the right timeI could get it working in both cases by using
NewSessionTokenProvider
in the SOURCE profile when we haveconfig.ChainedFromProfile != nil
Cached Credentials
aws-vault/vault/sessionkeyring.go
Lines 103 to 114 in 7500108
After debugging, I found it:
Notice that we only reuse if we use the same sts get credential method,
I tried to bypass it but got a 403 Forbidden. So I'll leave it unchanged, because i cannot find a way to do it better
Conclusion
aws-vault after a login without role chaining
aws-vault after a login with role chaining
aws-vault after both
So, we must provide
Enter MFA code for arn:aws:iam::***:mfa/***:
for each method.