Harcoded ARNS Bad: There are more partitions than standard. #53936
Conversation
All throughout the boto salt code are hard coded strings starting with: 'arn:aws:'. This means this code will only ever support the standard partition. There are at least 4 'known' partitions in the world: * standard - aws * govcloud - aws-us-gov * chinacloud - aws-cn * D-O-Dcloud - ??? Limting salt's ability to be an enterprise grade tool to 1/4. Additionally throughout the code are queries to 'get_user' and other services to return the accountID. Which leads to chicken and the egg scenarios as well as granting additional permissions. Thankfully AWS provides the `sts` endpoint which allows you to ask the philosophical questions of 'who am i?', 'where am i?', but saddly not 'are you my mother?'. This MR implements a 'boto_sts' module so that we can get introspective and patches the boto_iam/lambda module to make use of it. There are close to a hundred additional hardcoded strings to be fixed, but for now this solves our use cases. https://docs.aws.amazon.com/general/latest/gr/aws-arns-and-namespaces.html
|
@thenoid, thanks for your PR! By analyzing the history of the files in this pull request, we identified @ryan-lane and @tkwilliams to be potential reviewers. |
There was a problem hiding this comment.
I like the generalization of the ARN strings -- good catch! :)
WRT the IAM->STS changes, I distinctly recall TRYING to go this route before (in fact the URL to that boto3 function call is still red, showing "recently visited", in my google search results); but IIRC, it behaved unreliably in different situations. Note that the boto modules all support a variety of bind methods: IAM instance creds, STS creds, key-pair, etc. I don't remember exactly which, but the STS call failed with one or the other of these. Please do verify all the possible auth methods before merge (hopefully updating the test cases appropriately).
I've not used the get_partition function, and it looks useful, but the same concerns would be there WRT whether these structures are actually populated in all authentication scenarios. Please do verify, and create test-cases if possible.
Also, on a stupidly pedantic note, the comments still mention get_user even though you're using get_caller_identity :)
PS, grabbing the info from the FAILURE return data if it gets an error back?!? GENIUS!!
|
So we have been using this code in production for over a year now, using all those authentication schemes you mentioned, with 0 issues. the get_caller_identity function is there explicitly for asking Thanks for the comments catch - looking at the CI failures guess I need to become adept at writing salt tests. |
…k at all, so just going to throw this at the wall and see if it sticks.
|
@fattybenji This needs to be re-opened against the |
|
@thenoid we apologize for the delay. We have been doing a lot of planning to change our release and branch strategy to increase our release velocity, which unfortunately required taking a delay. All of the details of these changes are in this SEP here saltstack/salt-enhancement-proposals#20 I'm more than willing to help you port this to the master branch as well just let me know. |
What does this PR do?
All throughout the boto salt code are hard coded strings starting with:
'arn:aws:'. This means this code will only ever support the standard partition.
There are at least 4 'known' partitions in the world:
Limting salt's ability to be an enterprise grade tool to 1/4 partitions.
Additionally throughout the code are queries to 'ec2.get_user' and other services to return the accountID. Which leads to chicken and the egg scenarios and granting additional permissions beyond what is needed to function
Thankfully AWS provides the
stsendpoint which allows you to ask the philosophical questions of 'who am i?', 'where am i?', but saddly not 'are you my mother?'.This MR implements a 'boto_sts' module so that we can get introspective and patches the boto_iam module to make use of it. There are close to a hundred additional hardcoded strings to be fixed, but for now this solves our use cases.
https://docs.aws.amazon.com/general/latest/gr/aws-arns-and-namespaces.html
What issues does this PR fix or reference?
Salt's ability to support partitions beyond standard.
Having to grant additional permissions when managing other services.
Previous Behavior
Attempting manage any resource in any partition other than standard would result in failure. In addition in the standard partition, you had to grant the IAM/LAMBDA module access to EC2 api endpoints....even if it wasn't managing EC2
New Behavior
You can now create IAM/LAMBDA resources in non-standard partitions. Also no longer have to grant superfluous permissions.
Tests written?
No
Commits signed with GPG?
No
Please review Salt's Contributing Guide for best practices.
See GitHub's page on GPG signing for more information about signing commits with GPG.