fix(ses-actions): permissions too wide for S3 action #29823
Conversation
github-actions
bot
added
bug
github-actions
bot
added
the
distinguished-contributor
|
Exemption request: Per the following note, the integration test cannot be deployed: |
There was a problem hiding this comment.
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.
| @@ -55,7 +55,8 @@ export class S3 implements ses.IReceiptRuleAction { | |||
| resources: [this.props.bucket.arnForObjects(`${keyPattern}*`)], | |||
| conditions: { | |||
| StringEquals: { | |||
| 'aws:Referer': cdk.Aws.ACCOUNT_ID, | |||
| 'aws:SourceAccount': cdk.Aws.ACCOUNT_ID, | |||
| 'aws:SourceArn': `arn:${cdk.Aws.PARTITION}:ses:${cdk.Aws.REGION}:${cdk.Aws.ACCOUNT_ID}:receipt-rule-set/*:receipt-rule/*`, | |||
There was a problem hiding this comment.
I think this is the closest we can get, given we don't have rule_set_name or receipt_rule_name.
There was a problem hiding this comment.
You have access to rule.receiptRuleName, is it not "the name of the receipt rule that contains the deliver to Amazon S3 bucket action"?
There was a problem hiding this comment.
I got a cycle: Template is undeployable, these resources have a dependency cycle: RuleSetRule0B1D6BCA -> BucketPolicyE9A3008A -> RuleSetRule0B1D6BCA. Will investigate more in the morning.
There was a problem hiding this comment.
@nmussy yeah, not possible because the rule has a dependency on the policy: https://github.com/aws/aws-cdk/pull/29823/files#diff-824a0448a198a71c623bc8daaae829a1cd482701129ebd395fca0ee665e96ffaR67.
There was a problem hiding this comment.
Correct me if I'm wrong, but we're not getting a lot of additional restraints on the allocated perms with the SourceArn condition in its current state. The only thing we're gaining is the region restriction. The uncoditional receipt-rule-set doesn't help much, given we already only had the SES principal. I think trying to retrieve both the ruleSet from the ReceiptRuleProps, and the receiptRuleName (either generated, passed as a constructor prop, or imported), should be a priority
There was a problem hiding this comment.
Yeah, perhaps why this Condition is the way it is. May close this PR, I think aws:Referer with account ID is enough.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
I think someone on the CDK team will need to run the integration test. Wherever |
|
The pull request linter fails with the following errors: PRs must pass status checks before we can provide a meaningful review. If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing |
### Issue # (if applicable) Closes #29811, continuation of @msambol 's #29823 ### Reason for this change Reduce overly broad permissions allocated to SES for the S3 receipt rule action ### Description of changes * Restrain by both rule set and rule name, as recommended in the [docs](https://docs.aws.amazon.com/ses/latest/dg/receiving-email-permissions.html#receiving-email-permissions-s3) * Accomplished by generating the permission lazily, when the rule is rendering the actions for CloudFormation ### Description of how you validated changes Updated the unit and integration tests. The integration now uses a free test WorkMail domain. It's a bit of manual setup upfront, but doesn't require the contributor to use one of their own domains ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
This matches the condition described here.
Closes #29811.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license