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

feat: add aws_iam_policy_attachment_exclusive_attachment rule #786

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

kayman-mk
Copy link

@kayman-mk kayman-mk commented Dec 12, 2024

Add aws_iam_policy_attachment_exclusive_attachment_rule.

This rule warns when there are recommended alternatives to the resource, and is disabled by default

This recommendation is documented by HashiCorp in the AWS Provider documentation due to common misuse: https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_policy_attachment

In this particular case the blast radius of misuse can be severe across an entire organization account and cause production outages due to the unexpected removal of permissions.

After reviewing past issues (#35) and PR's (terraform-linters/tflint#769) I think that adding individual rules informed by HashiCorp recommendations is the most practical approach for community maintenance and this PR could be used as an approachable template for further rules without deep knowledge of the project (I used #355)

Thank you for your consideration.

Closes #35

@kayman-mk
Copy link
Author

This is a copy of #384. It seems that there are only minor things to fix, so let's get it done.

Motivation: We had an outage due to misuse of the iam_policy_attachement

Or aws_iam_policy_attachment_exclusive_attachment may be better.

I prefer names that are descriptive of what issue we are warning about. Additionally, the prefix should preferably match the resource name. What do you think?
@kayman-mk kayman-mk changed the title feat: add aws_iam_policy_attachment_has_alternatives rule feat: add aws_iam_policy_attachment_exclusive_attachement rule Dec 12, 2024
@kayman-mk kayman-mk changed the title feat: add aws_iam_policy_attachment_exclusive_attachement rule feat: add aws_iam_policy_attachment_exclusive_attachment rule Dec 12, 2024
@kayman-mk kayman-mk marked this pull request as ready for review December 12, 2024 10:22
Copy link
Member

@bendrucker bendrucker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good! I have a few copy edits and am reviewing via mobile so I'm adding them as comments. You're welcome to address them but I can also just take care of them and then merge in the next few days.

.gitignore Outdated
@@ -1 +1,3 @@
.idea/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revert this

@@ -0,0 +1,35 @@
# aws_iam_policy_attachment_exclusive_attachment

Consider alternative resources to `aws_iam_policy_attachment`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Get a little bit of the why up here. The section below can expand but this intro should give the reader a 1-2 sentence summary of what and why.


## Why

The `aws_iam_policy_attachment` resource creates exclusive attachments of IAM policies. Across the entire AWS account, all the users/roles/groups to which a single policy is attached must be declared by a single `aws_iam_policy_attachment` resource. This means that even any users/roles/groups that have the attached policy via any other mechanism (including other Terraform resources) will have that attached policy revoked by this resource. https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_policy_attachment
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hyperlink the first use of the identifier (in backticks) instead of appending the URL at the end


runner.EmitIssue(
r,
"Consider aws_iam_role_policy_attachment, aws_iam_user_policy_attachment, or aws_iam_group_policy_attachment instead.",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Include a brief bit of why here (exclusive for whole account)

Copy link
Member

@bendrucker bendrucker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok actually I do have a question here. Guessing this is just copy pasted and could be removed.

}

for _, resource := range resources.Blocks {
attribute, exists := resource.Body.Attributes[r.attributeName]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this logic apply? If there's no name attribute, the resource should be skipped?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see this logic on my machine. We don't need it here.

@kayman-mk kayman-mk requested a review from bendrucker December 16, 2024 07:18
@kayman-mk
Copy link
Author

Not sure how to decide about "enabled by default" and "severity".

@kayman-mk kayman-mk changed the title feat: add aws_iam_policy_attachment_exclusive_attachment rule feat: add aws_iam_policy_attachment_exclusive_attachment rule Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

feat: Generic rule to prevent certain dangerous resources like aws_iam_policy_attachment
3 participants