Skip to content
This repository has been archived by the owner on May 18, 2021. It is now read-only.

Login with source_profile doesn't work on current version #182

Closed
binman-docker opened this issue Jul 18, 2019 · 5 comments · Fixed by #186
Closed

Login with source_profile doesn't work on current version #182

binman-docker opened this issue Jul 18, 2019 · 5 comments · Fixed by #186
Labels

Comments

@binman-docker
Copy link

Hey there, we have some new users who are using the latest version of the tool that are having issues running aws-okta login <x> where profile x is configured with a source_profile. They get logged into the source account Console rather than the target account.

Looking through CloudTrail logs we see the assumerole into the target account (presumably to generate the federated login token) but do not see any actual ConsoleLogin attempt after that.

Giving these users an older binary off of my machine fixed the issue, so it may have been a somewhat recent change that broke this or somehow changed the behavior.

@nickatsegment
Copy link
Contributor

Interesting. We don't really use source_profile here at Segment any more, so I wouldn't expect a fix from us. If you can triage down to specific versions that work/don't work, that'd be a huge start. Also, consider running with debug logging.

@yhlee-tw
Copy link
Contributor

From git bisect

6658fa0 is the first bad commit

Related to #174

To be specific,
https://github.com/segmentio/aws-okta/pull/174/files#diff-83d18b94c3eba964d11ad830fb1eb4f0R160

-			session, err = p.assumeRoleFromSession(session, role)
+			creds, err := p.assumeRoleFromSession(creds, role)

causes the issue because it shadows outer creds variable.

My workaround

diff --git a/lib/provider.go b/lib/provider.go
index f956cfa..b2650d4 100644
--- a/lib/provider.go
+++ b/lib/provider.go
@@ -157,7 +157,8 @@ func (p *Provider) Retrieve() (credentials.Value, error) {
        // roles directly.
        if p.profile != source {
                if role, ok := p.profiles[p.profile]["role_arn"]; ok {
-                       creds, err := p.assumeRoleFromSession(creds, role)
+                       var err error
+                       creds, err = p.assumeRoleFromSession(creds, role)
                        if err != nil {
                                return credentials.Value{}, err
                        }

@nickatsegment
Copy link
Contributor

From git bisect

6658fa0 is the first bad commit

Related to #174

To be specific,
segmentio/aws-okta/pull/174/files#diff-83d18b94c3eba964d11ad830fb1eb4f0R160

-			session, err = p.assumeRoleFromSession(session, role)
+			creds, err := p.assumeRoleFromSession(creds, role)

causes the issue because it shadows outer creds variable.

My workaround

diff --git a/lib/provider.go b/lib/provider.go
index f956cfa..b2650d4 100644
--- a/lib/provider.go
+++ b/lib/provider.go
@@ -157,7 +157,8 @@ func (p *Provider) Retrieve() (credentials.Value, error) {
        // roles directly.
        if p.profile != source {
                if role, ok := p.profiles[p.profile]["role_arn"]; ok {
-                       creds, err := p.assumeRoleFromSession(creds, role)
+                       var err error
+                       creds, err = p.assumeRoleFromSession(creds, role)
                        if err != nil {
                                return credentials.Value{}, err
                        }

Nice, good work! I'm pretty sure vet would have caught this; I should wire that up to CI.

That patch looks good to me, @yhlee-tw; wanna submit a PR?

yhlee-tw added a commit to yhlee-tw/aws-okta that referenced this issue Jul 24, 2019
do not shadow outer creds

fixes segmentio#182
@surajbarkale
Copy link

Can we please get a bugfix release for this? One errant brew upgrade caused "mysterious" inability to access AWS.

nickatsegment pushed a commit that referenced this issue Jul 25, 2019
do not shadow outer creds

fixes #182
@nickatsegment
Copy link
Contributor

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants