aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.48k stars 3.83k forks source link

aws_ses: SES ReceiptRuleSet S3 action grants too wide permissions #29811

Open markusl opened 4 months ago

markusl commented 4 months ago

Describe the bug

SES ReceiptRuleSet S3 action grants too wide permissions

Expected Behavior

Should work as documented https://docs.aws.amazon.com/ses/latest/dg/receiving-email-permissions.html#receiving-email-permissions-s3

Currently missing the following block:

      "Condition":{
        "StringEquals":{
          "AWS:SourceAccount":"111122223333",
          "AWS:SourceArn": "arn:aws:ses:region:111122223333:receipt-rule-set/rule_set_name:receipt-rule/receipt_rule_name"
        }
      }

Current Behavior

https://github.com/aws/aws-cdk/blob/a7384c282756890a3e211c064b4e8a2dee3dab2a/packages/aws-cdk-lib/aws-ses-actions/test/actions.test.ts#L186

Reproduction Steps

    const ruleSet = new ses.ReceiptRuleSet(this, 'RuleSet');

    const defaultRule = ruleSet.addRule('DefaultRule', {
      recipients: props.recipients,
      enabled: true,
    });

    defaultRule.addAction(new actions.S3({
      bucket: new s3.Bucket(this, 'EmailBucket', {
        removalPolicy: cdk.RemovalPolicy.DESTROY,
        bucketName: props.bucketName,
      }),
    }));

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.137.0

Framework Version

No response

Node.js Version

20

OS

all

Language

TypeScript

Language Version

No response

Other information

No response

pahud commented 4 months ago

Yes, it's missing the required conditions in the bucket policy.

// create a DummyStack
export class DummyStack extends Stack {
  constructor(scope: Construct, id: string, props: StackProps) {
    super(scope, id, props);

    const ruleSet = new ses.ReceiptRuleSet(this, 'RuleSet');

    const defaultRule = ruleSet.addRule('DefaultRule', {
      recipients: ['foo.com'],
      enabled: true,
    });

    defaultRule.addAction(new sesa.S3({
      bucket: new s3.Bucket(this, 'EmailBucket', {
        removalPolicy: RemovalPolicy.DESTROY,
        bucketName: 'mock-bucket-name',
      }),
    }));
  }
}

BucketPolicy:

  Type: AWS::S3::BucketPolicy
    Properties:
      Bucket:
        Ref: EmailBucket843A740F
      PolicyDocument:
        Statement:
          - Action: s3:PutObject
            Condition:
              StringEquals:
                aws:Referer:
                  Ref: AWS::AccountId
            Effect: Allow
            Principal:
              Service: ses.amazonaws.com
            Resource:
              Fn::Join:
                - ""
                - - Fn::GetAtt:
                      - EmailBucket843A740F
                      - Arn
                  - /*
        Version: "2012-10-17"

Guess we should fix here

https://github.com/aws/aws-cdk/blob/a7384c282756890a3e211c064b4e8a2dee3dab2a/packages/aws-cdk-lib/aws-ses-actions/lib/s3.ts#L56-L59

github-actions[bot] commented 4 months ago

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see. If you need more assistance, please either tag a team member or open a new issue that references this one. If you wish to keep having a conversation with other community members under this issue feel free to do so.

github-actions[bot] commented 4 months ago

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see. If you need more assistance, please either tag a team member or open a new issue that references this one. If you wish to keep having a conversation with other community members under this issue feel free to do so.

samson-keung commented 3 months ago

Since the PR for this issue is reverted, I am re-opening this issue, although I am not sure if there is a good way to fix without running into the same problem that caused the revert..