-
Notifications
You must be signed in to change notification settings - Fork 594
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
Iam sid errors #1812
base: main
Are you sure you want to change the base?
Iam sid errors #1812
Conversation
self.resource_property_types.append(resource_type) | ||
for resource_type in self.idp_and_keys: | ||
self.resource_property_types.append(resource_type) | ||
self.sid_regex = re.compile('^[A-Za-z0-9]*$') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SIDs for ManagedPolicies have a different regex than SIDs in an inline policy, IIRC. I believe spaces are allowed in an inline policy SID
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar discussion here: #708
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started working off the cloudformation error I revieved on my stack.
Then I turned to the AWS docs to try and find more details.
The docs are pretty vague - particularly for resource policies
If I get a chance I'll set up some test templates and try them out. Seems to be where #708 got to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can confirm that ECR and KMS policies do not limit to that regex or require uniqueness.
I have not tested the other types but it seems that the rules are definitely different between resource types.
Given that, I think for now these rules should only apply to IAM resources.
I will update my PR to reflect that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still to confirm the correctness for inline policies
any progress planned on this? |
Issue #, if available:
Description of changes:
Validate IAM Sid against the specified regex and check for uniqueness in the policy document.
I based it on the IAM Version validator. I can't say I really understand how that works so happy to make changes.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.