Skip to content

Commit

Permalink
fix(s3): Reduce notifications handler permissions (#5925)
Browse files Browse the repository at this point in the history
This restricts them to resources only the buckets that is required. To
ensure permissions don't get too large minize is also enabled on the
policy.

Closes #5925
  • Loading branch information
Farid Nouri Neshat committed Sep 25, 2023
1 parent ab52bb5 commit 75976a6
Show file tree
Hide file tree
Showing 3 changed files with 140 additions and 13 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -52,24 +52,28 @@ export class NotificationsResourceHandler extends Construct {
public readonly functionArn: string;

/**
* The role of the handler's lambda function.
* The default policy of role of the handler's lambda function.
*/
public readonly role: iam.IRole;
private readonly policy: iam.Policy;

constructor(scope: Construct, id: string, props: NotificationsResourceHandlerProps = {}) {
super(scope, id);

this.role = props.role ?? new iam.Role(this, 'Role', {
const role = props.role ?? new iam.Role(this, 'Role', {
assumedBy: new iam.ServicePrincipal('lambda.amazonaws.com'),
});

this.role.addManagedPolicy(
role.addManagedPolicy(
iam.ManagedPolicy.fromAwsManagedPolicyName('service-role/AWSLambdaBasicExecutionRole'),
);
this.role.addToPrincipalPolicy(new iam.PolicyStatement({
actions: ['s3:PutBucketNotification'],
resources: ['*'],
}));

// We create our own default policy so we can set minimize to true and statements size
// remain small
const policy = new iam.Policy(this, 'DefaultPolicy', {
document: new iam.PolicyDocument({ minimize: true }),
});
role.attachInlinePolicy(policy);
this.policy = policy;

const resourceType = 'AWS::Lambda::Function';
class InLineLambda extends cdk.CfnResource {
Expand Down Expand Up @@ -97,17 +101,18 @@ export class NotificationsResourceHandler extends Construct {
Description: 'AWS CloudFormation handler for "Custom::S3BucketNotifications" resources (@aws-cdk/aws-s3)',
Code: { ZipFile: handlerSourceWithoutComments },
Handler: 'index.handler',
Role: this.role.roleArn,
Role: role.roleArn,
Runtime: 'python3.9',
Timeout: 300,
},
});
resource.node.addDependency(this.role);
resource.node.addDependency(role);
resource.node.addDependency(policy);

this.functionArn = resource.getAtt('Arn').toString();
}

public addToRolePolicy(statement: iam.PolicyStatement) {
this.role.addToPrincipalPolicy(statement);
this.policy.addStatements(statement);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -116,13 +116,17 @@ export class BucketNotifications extends Construct {
const handler = NotificationsResourceHandler.singleton(this, {
role: this.handlerRole,
});
handler.addToRolePolicy(new iam.PolicyStatement({
actions: ['s3:PutBucketNotification'],
resources: [this.bucket.bucketArn],
}));

const managed = this.bucket instanceof Bucket;

if (!managed) {
handler.addToRolePolicy(new iam.PolicyStatement({
actions: ['s3:GetBucketNotification'],
resources: ['*'],
resources: [this.bucket.bucketArn],
}));
}

Expand Down
120 changes: 119 additions & 1 deletion packages/aws-cdk-lib/aws-s3/test/notification.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ describe('notification', () => {
],
},
},
DependsOn: ['BucketNotificationsHandler050a0587b7544547bf325f094a3db834RoleDefaultPolicy2CF63D36',
DependsOn: ['BucketNotificationsHandler050a0587b7544547bf325f094a3db834DefaultPolicy8DF38B44',
'BucketNotificationsHandler050a0587b7544547bf325f094a3db834RoleB6FB88EC'],
});
});
Expand Down Expand Up @@ -185,4 +185,122 @@ describe('notification', () => {
Runtime: 'python3.9',
});
});
test('Check notifications handler permissions for single imported bucket', () => {
const stack = new cdk.Stack();

const bucket = s3.Bucket.fromBucketAttributes(stack, 'MyBucket', {
bucketName: 'foo-bar',
});

bucket.addEventNotification(s3.EventType.OBJECT_CREATED, {
bind: () => ({
arn: 'ARN',
type: s3.BucketNotificationDestinationType.TOPIC,
}),
});

Template.fromStack(stack).hasResourceProperties('AWS::IAM::Policy', {
PolicyDocument: {
Statement: [
{
Action: [
's3:GetBucketNotification',
's3:PutBucketNotification',
],
Effect: 'Allow',
Resource: {
'Fn::Join':
['', [
'arn:',
{ Ref: 'AWS::Partition' },
':s3:::foo-bar',
]],
},
},
],
},
});
});
test('Check notifications handler permissions for two imported bucket', () => {
const stack = new cdk.Stack();

const bucket = s3.Bucket.fromBucketAttributes(stack, 'MyBucket', {
bucketName: 'foo-bar',
});

bucket.addEventNotification(s3.EventType.OBJECT_CREATED, {
bind: () => ({
arn: 'ARN',
type: s3.BucketNotificationDestinationType.TOPIC,
}),
});

const bucket2 = s3.Bucket.fromBucketAttributes(stack, 'MyBucket2', {
bucketName: 'foo-bar2',
});

bucket2.addEventNotification(s3.EventType.OBJECT_CREATED, {
bind: () => ({
arn: 'ARN',
type: s3.BucketNotificationDestinationType.TOPIC,
}),
});

Template.fromStack(stack).hasResourceProperties('AWS::IAM::Policy', {
PolicyDocument: {
Statement: [
{
Action: [
's3:GetBucketNotification',
's3:PutBucketNotification',
],
Effect: 'Allow',
Resource: [{
'Fn::Join':
['', [
'arn:',
{ Ref: 'AWS::Partition' },
':s3:::foo-bar',
]],
}, {
'Fn::Join':
['', [
'arn:',
{ Ref: 'AWS::Partition' },
':s3:::foo-bar2',
]],
}],
},
],
},
});
});
test('Check notifications handler permissions for single bucket', () => {
const stack = new cdk.Stack();

const bucket = new s3.Bucket(stack, 'MyBucket', {
bucketName: 'foo-bar',
});

bucket.addEventNotification(s3.EventType.OBJECT_CREATED, {
bind: () => ({
arn: 'ARN',
type: s3.BucketNotificationDestinationType.TOPIC,
}),
});

Template.fromStack(stack).hasResourceProperties('AWS::IAM::Policy', {
PolicyDocument: {
Statement: [
{
Action: 's3:PutBucketNotification',
Effect: 'Allow',
Resource: {
'Fn::GetAtt': ['MyBucketF68F3FF0', 'Arn'],
},
},
],
},
});
});
});

0 comments on commit 75976a6

Please sign in to comment.