feat: Add attach_sns_policy

[kaykhan]

Description

This pull request adds the attach_sns_policy flag to attach a permission policy to the sns target to allow the sending of event bridge notifications to sns.

Motivation and Context

Without assigning resource permissions to the sns target, event bridge rule fails to send the notification to sns.
https://repost.aws/knowledge-center/sns-not-getting-eventbridge-notification

Fixes #88

Breaking Changes

How Has This Been Tested?

• I have updated at least one of the examples/* to demonstrate and validate my change(s)
• I have tested and validated these changes using one or more of the provided examples/* projects

• I have executed pre-commit run -a on my pull request

[antonbabenko]

Looks pretty good. There is just a minor comment.

[antonbabenko]

Split this policy statement into 2 - one for SNS and one for KMS actions. They should have different resources.

[kaykhan]

What do you mean they should have different resources?. Should the second statement which allows KMS actions not target the SNS resource?. I added the KMS actions as an afterthought to be able to publish encrypted messages - as descirbed here https://repost.aws/knowledge-center/sns-not-getting-eventbridge-notification

But I can see the existing SQS example uses a single policy statement. https://github.com/terraform-aws-modules/terraform-aws-eventbridge/blob/master/iam.tf#L139-L153

What should the resources be?

data "aws_iam_policy_document" "sns" {
  count = local.create_role && var.attach_sns_policy ? 1 : 0

  statement {
    sid    = "SNSAccess"
    effect = "Allow"
    actions = [
      "sns:Publish",
    ]
    resources = var.sns_target_arns
  }

  statement {
    sid    = "SNSKMSAccess"
    effect = "Allow"
    actions = [
      "kms:Decrypt",
      "kms:GenerateDataKey"
    ]
    resources = ???
  }

}

[antonbabenko]

resources = ??? => resources = ["*"] as it says in "Example IAM policy statement that allows EventBridge to publish messages to an encrypted Amazon SNS topic" section on https://repost.aws/knowledge-center/sns-not-getting-eventbridge-notification

[kaykhan]

Oh i see

[antonbabenko]

This PR is included in version 2.2.0 🎉

[denkojonasz]

Hello, based on my observations attach_sns_policy probably doesn't make sense and doesn't solve EventBridge -> SNS permission issue.

As you can see in the docs For Lambda, Amazon SNS, Amazon SQS, and Amazon CloudWatch Logs resources, EventBridge uses resource-based policies, which means that attaching identity-based permission to the EventBridge via attach_sns_policy doesn't fix the problem.

The same is mentioned in the https://repost.aws/knowledge-center/sns-not-getting-eventbridge-notification, what was the motivation for this PR - Your Amazon SNS topic's resource-based policy must allow EventBridge to publish messages to the topic

The real resolution on the permission problem, pointed in the motivation is to attach appropriate topic_policy_statements resource based policy, that allows publish SNS messages from EventBridge on the SNS resource level.

Because of the above I would propose to revert this commit, as it doesn't resolve anything and only makes a confusion.

[kaykhan]

Hello, based on my observations attach_sns_policy probably doesn't make sense and doesn't solve EventBridge -> SNS permission issue.

As you can see in the docs For Lambda, Amazon SNS, Amazon SQS, and Amazon CloudWatch Logs resources, EventBridge uses resource-based policies, which means that attaching identity-based permission to the EventBridge via attach_sns_policy doesn't fix the problem.

The same is mentioned in the https://repost.aws/knowledge-center/sns-not-getting-eventbridge-notification, what was the motivation for this PR - Your Amazon SNS topic's resource-based policy must allow EventBridge to publish messages to the topic

The real resolution on the permission problem, pointed in the motivation is to attach appropriate topic_policy_statements resource based policy, that allows publish SNS messages from EventBridge on the SNS resource level.

Because of the above I would propose to revert this commit, as it doesn't resolve anything and only makes a confusion.

Hi, i agree there might be confusion but i just want to push back on the idea that this commit did not resolve anything

if i check the SNS Topic, i can see that the access policy was updated to include the right permissions.

I'm not sure if this commit is unique it seems to be following the same architecture of this module.

---

Topic policy statements are new to me, i can see resource definition here https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/sns_topic_policy.html

What is the major difference between specifying an aws_sns_topic_policy compared to this https://github.com/terraform-aws-modules/terraform-aws-eventbridge/blob/master/iam.tf#L213 both seem to be use the underlining aws_iam_policy_document ?

[denkojonasz]

if i check the SNS Topic, i can see that the access policy was updated to include the right permissions.

I don't think that SNS Access policy from your Screenshot was updated because of adding identity-based policy to the AWS EventBridge (this one created via attach_sns_policy flag).

AWS automatically sets proper policy once AWS EventBridge rule is created/modified via Console. Any chance that your AWS EventBridge rule were updated via Console? Did you modify anything via console:

To test it, you can create completely new EventBridge rule and SNS topic with attach_sns_policy flag enabled, via Terraform, and confirm if the Access policy from your screenshot was attached or not.

---

What is the major difference between specifying an aws_sns_topic_policy compared to this https://github.com/terraform-aws-modules/terraform-aws-eventbridge/blob/master/iam.tf#L213 both seem to be use the underlining aws_iam_policy_document ?

Topic policy sets permissions on a resource level (it's resource-based policy on SNS directly). Specifying policy in this module, sets permissions on EventBridge service level.

Even if they are the same policies (both grant access to publish SNS messages on specific SNS queue), because of the some reasons AWS require to set them on resource level. One more time docs - For Lambda, Amazon SNS, Amazon SQS, and Amazon CloudWatch Logs resources, EventBridge uses resource-based policies.

[denkojonasz]

@kaykhan have you had a chance to verify my last comment?

[kushan-nv]

I can also confirm that setting attach_sns_policy doesn't update the SNS access policy.

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.