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

Additional Named Roles are Not Populating #26

Open
bgshacklett opened this issue Dec 4, 2018 · 7 comments
Open

Additional Named Roles are Not Populating #26

bgshacklett opened this issue Dec 4, 2018 · 7 comments

Comments

@bgshacklett
Copy link

Summary:
Testing with some of my coworkers has shown that at least some additional named roles are not populating in the downloaded Credentials file for us.

Steps to Reproduce:

  • Configure the extension with named roles.
  • Log into AWS via IDP.
  • Select a role to be set as the Default role.
  • Complete the log-in process.

Expected behavior:

  • The role chosen when logging into the AWS page will be set as the default profile.
  • All configured roles will be added to the Credentials file.

Actual behavior:

  • Only the default and, perhaps one named role will be added to the credentials file.

Other notes:
Looking at locations in the source code where changes occurred, I suspect this might have started happening around 35e4917.

@prolane
Copy link
Owner

prolane commented Dec 6, 2018

Hi @bgshacklett
I think you might have misunderstood how the credentials are fetched for those optional additional roles. Although I have to admit, its not that clear from the description either, so I don't blame you. :-)

Its not the SAML response that is used to fetch credentials for those additional roles. How it works is as follows:

  • Log in to AWS via IDP
  • Select the role you want to assume (this is going to be the default profile in your creds file)
  • The fetched keys for the default profile will be used to call the assume-role API for fetching the credentials for the additionally configured roles.

This feature comes from an user who mentioned they have an AWS environment where they login to one master IAM account, and hop between accounts/role from there by assuming other roles. This is actually a security best practice from AWS, so I see this setup more often.

So as you can see, you'll only see the additional profiles in your credentials file if the default profile has the permissions to assume the other roles.

@bgshacklett
Copy link
Author

I found that as I was looking at the code the end of last week. Sadly I didn't ever push the branch I was on and I lost the work I did experimenting with enabling it to assume all of the roles as named profiles. The way I've done this in my Node.js module is to use the assumeRoleWithSAML function for all available roles in the SAML response from the IDP: https://github.com/bgshacklett/samlogin/blob/master/index.js#L93

I'm hoping to put a PR in for this early next week. In the interim, I'd be interested in any feedback you have on that method being utilized in your extension.

@prolane
Copy link
Owner

prolane commented Dec 10, 2018

Well, first of all, I definitely welcome contributions to the code, so lets see how we can make this work. But on first glance I'm thinking I might need to refactor the code to make it easier to integrate new features. The majority of the extension was written before I was even aware of Javascript Promises for example. I guess if I would start from scratch today I would do things differently. Maybe somewhere during the xmas holidays I'll do that.

I can tell you from a conceptual point of view how I'd like to see it integrated.

  • The feature can be enabled/disabled from the option panel
  • The default profile will still be the role you select upon logging in
  • Keep the profile name unique. Seems like you already thought of that when implementing [${accountNumber}-${roleName}]

In the current state I guess it would mean to intervene somewhere around this point:

  • If the new feature is disabled and there are no additional roles configured in the options panel, then write out the creds file.
  • If the new feature is disabled and there are additional roles configured, execute assumeAdditionalRole
  • If the new feature is enabled, run your function, but at the end check if additional roles are configured. If so, execute assumeAdditionalRole.

Yes, it will be a bit messy and contain duplicate code, but until some refactoring is done its probably the best way to integrate.

@viyullas
Copy link

I think this is still unsolved, isn't it?
I have defined 2 roles but just one shows in the credentials file.

Also it would be nice to be able to asign a name to the default profile, instead of "default"

@penchala-services-inc
Copy link

Well, first of all, I definitely welcome contributions to the code, so lets see how we can make this work. But on first glance I'm thinking I might need to refactor the code to make it easier to integrate new features. The majority of the extension was written before I was even aware of Javascript Promises for example. I guess if I would start from scratch today I would do things differently. Maybe somewhere during the xmas holidays I'll do that.

I can tell you from a conceptual point of view how I'd like to see it integrated.

* The feature can be enabled/disabled from the option panel

* The default profile will still be the role you select upon logging in

* Keep the profile name unique. Seems like you already thought of that when implementing `[${accountNumber}-${roleName}]`

In the current state I guess it would mean to intervene somewhere around this point:

* If the new feature is disabled and there are no additional roles configured in the options panel, then write out the creds file.

* If the new feature is disabled and there are additional roles configured, execute `assumeAdditionalRole`

* If the new feature is enabled, run your function, but at the end check if additional roles are configured. If so, execute `assumeAdditionalRole`.

Yes, it will be a bit messy and contain duplicate code, but until some refactoring is done its probably the best way to integrate.

I have forked and created a new version of the extension with some of the features we are discussing here.
https://chrome.google.com/webstore/detail/saml-to-aws-sts-keys-conv/nhcoglopkiacbogcjdcbhooedkaddimi

https://github.com/penchala-services-inc/chrome-samltoawsstskeys-mp/tree/master

@penchala-services-inc
Copy link

I found that as I was looking at the code the end of last week. Sadly I didn't ever push the branch I was on and I lost the work I did experimenting with enabling it to assume all of the roles as named profiles. The way I've done this in my Node.js module is to use the assumeRoleWithSAML function for all available roles in the SAML response from the IDP: https://github.com/bgshacklett/samlogin/blob/master/index.js#L93

I'm hoping to put a PR in for this early next week. In the interim, I'd be interested in any feedback you have on that method being utilized in your extension.

This may help you.

I have forked and created a new version of the extension with some of the features you are discussing.
https://chrome.google.com/webstore/detail/saml-to-aws-sts-keys-conv/nhcoglopkiacbogcjdcbhooedkaddimi

https://github.com/penchala-services-inc/chrome-samltoawsstskeys-mp/tree/master

@penchala-services-inc
Copy link

I think this is still unsolved, isn't it? I have defined 2 roles but just one shows in the credentials file.

Also it would be nice to be able to asign a name to the default profile, instead of "default"

I have forked and created a new version of the extension with what you are looking for here.

https://chrome.google.com/webstore/detail/saml-to-aws-sts-keys-conv/nhcoglopkiacbogcjdcbhooedkaddimi

https://github.com/penchala-services-inc/chrome-samltoawsstskeys-mp/tree/master

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

No branches or pull requests

4 participants