onelogin / onelogin-python-aws-assume-role

MIT License
58 stars 52 forks source link

Added SAML caching and bumped minor version #25

Closed kylecrawshaw closed 4 years ago

kylecrawshaw commented 5 years ago

This PR adds SAML caching as a default and adds a new flag --no-cache if a user does not want use the cache file. This allows a user to authenticate multiple AWS roles and associate them to multiple AWS profiles without the need to authenticate on every role. The cache is only valid for 5 min. onelogin-aws-assume-role can be called as many times as necessary within 5 min. This is great for users to work across many different AWS roles and AWS accounts.

The password key is removed from get_saml_response as well because it is not used anywhere and should not be written to disk.

pitbulk commented 5 years ago

Instead storing the SAMLResponse on a file, why not reuse the value from memory and reuse it in the loop as we also do for example with the username,password,domain..?

If you are currently running the script 1 time for 1 role and later want to execute it for another role in less than 5 min... why no set the loop <> 1 and enable the interactive mode to be asked to retrieve a new role?

I saw you removed the password field and that is wrong, if we keep the password we will use it in a future iteration, when we need to get a new SAMLResponse due the temp cred expired.

In fact you already have the saml response in the second iteration..

kylecrawshaw commented 5 years ago

I took this approach because there is a use case where you may want a wrapper script to get SAML credentials for a bunch of accounts and assign them to different profiles without using the loop feature and having to interact. The loop feature is only helpful to keep renewing credentials, but this feature allows you to get credentials for many roles at once. The goal is to not have to interact with the tool for every role I need to get credentials for.

I'll take a look at the password and see how to make that function properly.

kylecrawshaw commented 5 years ago

Ok I see what you're talking about with the password. I can add a check that if the loop feature is used it won't use the SAML cache. One thing to note, the loop feature does not work properly if you use 2FA.

pitbulk commented 5 years ago

Yes, for that reason makes sense to whitelist the IP and provide the IP on the script

mide commented 5 years ago

Hey! Thanks for proposing this @kylecrawshaw, my organization is using this but we want SAML caching so we can easily switch between roles.

We did originally try to go down the route of having the cached in-memory password so we could switch roles (and it'd keep us logged in), but we recently were required to enable 2FA and it completely broke our workflow. I would much prefer SAML over the credential caching.

My CISO also raised concerns about the fact that a forgotten script running would keep a machine authenticated forever, invalidating our whole "expiring credential" security requirement, so we shouldn't have been using that loop feature to begin with anyway.

That said, the fact that we don't use the feature doesn't mean no one does/will, but I wanted to provide some feedback.

kylecrawshaw commented 5 years ago

@pitbulk I've added some changes that will prompt for the password if the user is in a loop. This still allows for SAML caching by default. I've tested with and without a valid SAML cache in a loop and the behavior still works as it previously did before adding this feature.

kylecrawshaw commented 5 years ago

@pitbulk Anything I can answer here to get this merged?

pitbulk commented 5 years ago

Sorry for the delay. I will review and test today and then merge and release nee version if all is ok.

btw I think this feature requires more than a minor bump on the version.

kylecrawshaw commented 5 years ago

Do you think it should be 2.0.0 then? I opted for a bump from 1.5.0 to 1.6.0 because https://semver.org recommends MINOR version when you add functionality in a backwards-compatible manner.

pitbulk commented 5 years ago

Sorry 1.6.0 is ok, I though I saw 1.5.1

kylecrawshaw commented 5 years ago

Ok great. No problem!

kylecrawshaw commented 5 years ago

Hey @pitbulk. Don't mean to be annoying, but we're really hoping to use this with some teams as soon as possible. Have you had a chance to test at all?

pitbulk commented 5 years ago

Sorry for the delay but I was kinda busy and took time to figure out what were you trying to achieve and provide a complete solution.

With a valid cached SAMLResponse, there is no need of interacting with OneLogin, the SAMLResponse is directly sent to AWS, so there is no need to introduce username/password/app_id/mfa.

Once a cached SAMLResponse is invalidated, we should request that info if was not retrieved before in order to get a new valid SAML Response.

I gonna add the cache functionality in case you want to run the script several times for different roles, but at the same time I will try to allow register in different profiles cred for different roles with the same samlresponse retrieved without the need of cache it.

pitbulk commented 5 years ago

@kylecrawshaw take a look https://github.com/onelogin/onelogin-python-aws-assume-role/pull/28

pitbulk commented 4 years ago

https://github.com/onelogin/onelogin-python-aws-assume-role/commit/9e8137b7606d315ea1c491c093e689e38c5a16b3