fix(s3-notifications): fixing circular dependency when Bucket and SQS are encrypted by same KMS is used for s3 notification

[stm29]

Issue #3067

Closes #3067

If you are reading this from discussion redirection (or landed here from searching in issue) run cdk synth in your app and look for warning displayed and look for @aws-cdk/aws-s3-notifications. You will find out the solution.

Reason for this change

• we can't cdk deploy, when we use same same KMS for S3 and SQS.
• This was partially addressed on fix(s3-notifications): notifications allowed with imported kms keys #18989 , rather than fixing, this PR adds a warning when policy is unable to add to KMS.
  • But issue arises when the policy is added. It creates Circular dependency.
  • When, we didn't add this permission, then we can't send message event to SQS.

So, we need to draw a fair line on how to handle this. approaches considered,

1. Issue arises, when we are constraining the polices in KMS. so what happens is, KMS depends on Bucket, Bucket depends on KMS, exactly explained by Circular dependency on s3 notification to a destination when both destination and s3 are encrypted by same CMK #3067 (comment)

• We can just expand the scope of Policy to Service level (s3.amazonaws.com), rather than constraining with Resource level (BucketName), this may not be security wise advised, but this better than allowing user to deploy than just showing circular dependency.

2. We can optionally, ask use wether to add this relaxed permission added. If doesn't wish to add this, he can opt out and add his constrained policy.
3. In both the cases we will show FAIR warning on how this will result in Security / Runtime Exception (Circular Dependency).

Description of changes

• Added optional parameter shouldAddGlobalS3PermissionToKMSandSQS , defaulted to true, as it will be backward compatible
• Based on this Flag, we are showing warning during cdk synth
  • Circular Dependency Violation when false is provided, as they will try to add imported Bucket Value to add permission
  • Security Warning - as Service Level permission is granted

Description of how you validated changes

• Unit Tests
• Integration Tests

Checklist

• My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[aws-cdk-automation]

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: 4ca7bf1
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[aws-cdk-automation]

This PR has been in the MERGE CONFLICTS state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

[laurelmay]

If we're going to tell the user that this is a better way of implementing this, it's probably best if we have a new integration test that does it and ensures that it works. I'm not entirely sure that I understand how moving this condition out of the SqsDestination.bind method and making the user do it manually actually resolves the circular dependency while still allowing the more specific policy.

[stm29]

I'm not entirely sure that I understand how moving this condition out of the SqsDestination.bind method and making the user do it manually actually resolves the circular dependency while still allowing the more specific policy

So, Given the Condition, we can't depend on Class's members to get the name, to add the policy like us do in CDK by itself.
There are 2 ways,

1. User Needs to add the permissions by himself, as a constant defined by himself (this will relax the Circular dependency between both), Only thing I am seeing here is, At this moment, we can't make CDK to do this automatically.
2. Allow Global access.

What are we trying to resolve here is, "cdk deploy" is even failing at this moment.
@laurelmay Please let me know wether there is any suggested alternatives.

[aws-cdk-automation]

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

[github-actions]

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.