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.

Sign up for GitHub

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
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/aws-cdk-lib/aws-ses-actions/lib/s3.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

@nmussy yeah, not possible because the rule has a dependency on the policy: https://github.com/aws/aws-cdk/pull/29823/files#diff-824a0448a198a71c623bc8daaae829a1cd482701129ebd395fca0ee665e96ffaR67.

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.

},
},
});
Expand Down
16 changes: 15 additions & 1 deletion packages/aws-cdk-lib/aws-ses-actions/test/actions.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,9 +193,23 @@ test('add s3 action', () => {
Action: 's3:PutObject',
Condition: {
StringEquals: {
'aws:Referer': {
'aws:SourceAccount': {
Ref: 'AWS::AccountId',
},
'aws:SourceArn': {
'Fn::Join': [
'',
[
'arn:',
{ Ref: 'AWS::Partition' },
':ses:',
{ Ref: 'AWS::Region' },
':',
{ Ref: 'AWS::AccountId' },
':receipt-rule-set/*:receipt-rule/*',
],
],
},
},
},
Effect: 'Allow',
Expand Down
Loading