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

fix(ses-actions): permissions too wide for S3 action #29823

Closed
wants to merge 1 commit into from

Conversation

msambol
Copy link
Contributor

@msambol msambol commented Apr 13, 2024

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

@github-actions github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p1 labels Apr 13, 2024
@aws-cdk-automation aws-cdk-automation requested a review from a team April 13, 2024 01:41
@github-actions github-actions bot added the distinguished-contributor [Pilot] contributed 50+ PRs to the CDK label Apr 13, 2024
@msambol
Copy link
Contributor Author

msambol commented Apr 13, 2024

Exemption request: Per the following note, the integration test cannot be deployed:

/**********************************************************************************************************************
*
* Warning! This test case can not be deployed!
*
* Save yourself some time and move on.
* The latest given reason is:
* - 2023-08-30: Uses a hardcoded email address that is not verified, @mrgrain
*
*********************************************************************************************************************/

Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

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/*`,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is the closest we can get, given we don't have rule_set_name or receipt_rule_name.

Copy link
Contributor

Choose a reason for hiding this comment

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

You have access to rule.receiptRuleName, is it not "the name of the receipt rule that contains the deliver to Amazon S3 bucket action"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got a cycle: Template is undeployable, these resources have a dependency cycle: RuleSetRule0B1D6BCA -> BucketPolicyE9A3008A -> RuleSetRule0B1D6BCA. Will investigate more in the morning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, perhaps why this Condition is the way it is. May close this PR, I think aws:Referer with account ID is enough.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 6229602
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@msambol
Copy link
Contributor Author

msambol commented Apr 13, 2024

I think someone on the CDK team will need to run the integration test. Wherever cdk-ses-receipt-test@yopmail.com is verified.

@aws-cdk-automation
Copy link
Collaborator

The pull request linter fails with the following errors:

❌ Fixes must contain a change to an integration test file and the resulting snapshot.

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 Exemption Request and/or Clarification Request.

@msambol
Copy link
Contributor Author

msambol commented Apr 15, 2024

Closing in favor of #29833. Thanks, @nmussy !

@msambol msambol closed this Apr 15, 2024
@msambol msambol deleted the 29811 branch April 15, 2024 15:28
mergify bot pushed a commit that referenced this pull request Apr 19, 2024
### 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*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. distinguished-contributor [Pilot] contributed 50+ PRs to the CDK effort/small Small work item – less than a day of effort p1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aws_ses: SES ReceiptRuleSet S3 action grants too wide permissions
3 participants