prolane / samltoawsstskeys

Google Chrome Extension which converts a SAML 2.0 assertion to AWS STS Keys.
MIT License
138 stars 91 forks source link

Additional Named Roles are Not Populating #26

Open bgshacklett opened 5 years ago

bgshacklett commented 5 years ago

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:

Expected behavior:

Actual behavior:

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

prolane commented 5 years ago

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:

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 commented 5 years ago

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 commented 5 years ago

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.

In the current state I guess it would mean to intervene somewhere around this point: https://github.com/prolane/samltoawsstskeys/blob/20133da493b2a95038f6dfc384387611103b3c15/background/script.js#L171

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 commented 3 years ago

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 commented 1 year ago

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:

https://github.com/prolane/samltoawsstskeys/blob/20133da493b2a95038f6dfc384387611103b3c15/background/script.js#L171

* 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 commented 1 year ago

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 commented 1 year ago

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