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

Fix sts credentials reusage #222

Closed
wants to merge 5 commits into from

Conversation

dsumsky
Copy link

@dsumsky dsumsky commented Nov 3, 2016

STS credentials for one AWS account seem to be reused when connecting to another AWS account before they expire. The issue is reported as #221.

I want to write a function to check limits/thresholds in multiple AWS accounts at once. The code below returns number of S3 buckets in ACCOUNT01 account correctly but it is incorrect for ACCOUNT02 account as the STS credentials from ACCOUNT01 account are reused.

Testing code to reproduce the issue:

#!/usr/bin/env python
from awslimitchecker.checker import AwsLimitChecker

def foo(event):
    # input parameters for checker
    service = event['service']
    region = event['region']
    account_name = event['account_name']
    account_id = str(event['accounts'][account_name]['id'])
    account_role = event['account_role']

    checker = AwsLimitChecker(account_id=account_id, 
                              account_role=account_role, region=region)
    checker.check_thresholds(service)
    result = checker.get_limits(service)[service]

    for limit_name, limit in result.items():
        limitstr = str(limit.get_limit())
        usagestr = str(limit.get_current_usage_str())

        print("Service usage: account_name: {}, region: {}, service: {}, \
              limit_name: {}, limit: {}, limit_usage: {}".format(account_name, \
              region, service, limit_name, limitstr, usagestr))


if __name__ == "__main__":
    event = {
        u'service': 'S3',
        u'region': 'us-east-1',
        u'account_name': 'ACCOUNT01',
        u'account_role': 'ALC_ROLE',
        u'accounts': {
            u'ACCOUNT01': {
                u'id': XXXXXXXXXXX01
            },
            u'ACCOUNT02': {
                u'id': XXXXXXXXXXX02
            }

        }
    }

    foo(event)

    event['account_name'] = 'ACCOUNT02'

    foo(event)

Contributor License Agreement

By submitting this work for inclusion in awslimitchecker, I agree to the following terms:

  • The contribution included in this request (and any subsequent revisions or versions of it)
    is being made under the same license as the awslimitchecker project (the Affero GPL v3,
    or any subsequent version of that license if adopted by awslimitchecker).
  • My contribution may perpetually be included in and distributed with awslimitchecker; submitting
    this pull request grants a perpetual, global, unlimited license for it to be used and distributed
    under the terms of awslimitchecker's license.
  • I have the legal power and rights to agree to these terms.

@jantman
Copy link
Owner

jantman commented Nov 3, 2016

At first glance this looks good to me. I'm swamped today, but should be able to look deeper at this tonight or tomorrow, and I can make the test additions/fixes for it.

Have you used this code yourself, and can you confirm that it's working and fixes the issue?

@dsumsky
Copy link
Author

dsumsky commented Nov 3, 2016

I tested it today and it works fine with pure STS/AssumeRole but without MFA token. I'm just not sure if it does not interfere with MFA related code.

@jantman
Copy link
Owner

jantman commented Nov 10, 2016

@dsumsky sorry for the delay on this.

I should be able to fix the tests, but this needs to be rebased on develop. If it's OK with you, I'll pull this PR into a branch of mine, rebase it, fix the tests, and open a new PR from that. You'll still be tracked as the contributor as far as the commits are concerned...

@jantman
Copy link
Owner

jantman commented Nov 10, 2016

Oh wow... there's now an "Update branch" button where, apparently, I can merge the target branch into your branch...

@dsumsky
Copy link
Author

dsumsky commented Nov 10, 2016

it's definitely ok, do what you need to do.

Dne 10. 11. 2016 10:50 odpoledne napsal uživatel "Jason Antman" <
notifications@github.com>:

@dsumsky https://github.com/dsumsky sorry for the delay on this.

I should be able to fix the tests, but this needs to be rebased on
develop. If it's OK with you, I'll pull this PR into a branch of mine,
rebase it, fix the tests, and open a new PR from that. You'll still be
tracked as the contributor as far as the commits are concerned...


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#222 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACpdPfgRbJkPHH6J4BawafTl82a4Ntk5ks5q85GJgaJpZM4Koenr
.

jantman added a commit that referenced this pull request Nov 11, 2016
jantman added a commit that referenced this pull request Nov 11, 2016
@codecov-io
Copy link

codecov-io commented Nov 11, 2016

Current coverage is 99.93% (diff: 100%)

Merging #222 into develop will decrease coverage by <.01%

@@            develop       #222   diff @@
==========================================
  Files            22         22          
  Lines          1660       1650    -10   
  Methods           0          0          
  Messages          0          0          
  Branches        257        253     -4   
==========================================
- Hits           1659       1649    -10   
  Misses            0          0          
  Partials          1          1          

Powered by Codecov. Last update dd02d47...8559e25

@jantman
Copy link
Owner

jantman commented Nov 11, 2016

I'm closing this in favor of #225 which has my fixes in it. Thanks so much for this!!!!

@jantman jantman closed this Nov 11, 2016
jantman added a commit that referenced this pull request Nov 11, 2016
@jantman
Copy link
Owner

jantman commented Nov 12, 2016

This has been released in 0.6.0 and is now live on PyPI

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

Successfully merging this pull request may close these issues.

4 participants