s3 bucket notifications creates IAM policy that has no resource boundary

[nija-at]

Forked off from #2781, specifically this comment

S3 bucket notifications creates an IAM role holding a policy that contains no resource boundary, specifically "Resource": "*".

Companies typically enforce that all IAM policies should be well bounded in their actions and resource.

https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/aws-s3-notifications/test/integ.notifications.expected.json#L188-L208

[iliapolo]

From an initial investigation, it seems that the * is used because the BucketNotificationsHandler policy is created once per stack, and not per bucket. It therefore doesn't know in advance which buckets it will have to configure notifications for.

Relevant code starts here: https://github.com/aws/aws-cdk/blob/master/packages/@aws-cdk/aws-s3/lib/notifications-resource/notifications-resource.ts#L52

We should explore an option to either incrementally update the permissions of this policy, or create a separate policy for each bucket.

[binhrobles]

I ran into this issue today as well! I'm working within an enterprise environment where IAM roles must follow a naming convention and have a permissions boundary attached. I've been getting around this by using CDK Escape Hatches, but the issue with this BucketNotificationsHandler construct is that it's a top level resource, so I can't access it from either the Lambda or the S3 Bucket resources.

For example:

const bucket = new Bucket(this, 'TestBucket');
const lambda = new Function(this, 'TestFunction', {
  code: Code.asset('./test-src'),
  runtime: Runtime.NodeJS10x,
  handler: 'index.handler',
  description: 'Example function',
});
lambda.addEventSource(new S3EventSource(bucket, {
  events: [EventType.ObjectCreated],
}));

generates a BucketNotificationsHandler with path: MyStack/BucketNotificationsHandler050a0587b7544547bf325f094a3db834/Resource. It's not a child of any of my existing resources.

I got around this by going to the Root Node and doing some string filtering, like:

const bucketNotificationsHandler = this.node.children.find((n) => n.node.id.startsWith('BucketNotificationsHandler'));
const bucketNotificationsHandlerRole = bucketNotificationsHandler.node.children.find((n) => n.node.path.endsWith('Role'));

// helper function for meeting enterprise reqs
addNamingAndPermissionsBoundary(bucketNotificationsHandlerRole, 'BucketNotifLambda');

For me, the primary issue is that addEventSource() returns void, and doesn't give me any easy way to get to the underlying resources. Is there an easier/clearer way to get to these constructs, or is this pretty much the way to do it for now?

[iliapolo]

@binhrobles

Is there an easier/clearer way to get to these constructs, or is this pretty much the way to do it for now?

Thats pretty the way as of now. You could simplify a little by using the well known notification handler id:

const bucketNotificationsHandler = Stack.of(this).node.tryFindChild('BucketNotificationsHandler050a0587b7544547bf325f094a3db834')!

But that's not a WHOLE lot better...actually for your use-cases, I would recommend having a look at Aspects - which give you a way of visiting every construct just before synthesis happens, and modify it to fit your needs.

Escape hatches are mainly for pin point mutations to the underlying CloudFormation resource properties.

[oll-isaiahgrant]

Background

It looks like the following open issues are all suffering from the same constraint:

#13241
#9918

Many started with bug and/or needs-triage labels, but have somehow become feature-request with varying levels of effort assigned 🤔 ? I am hoping that by highlighting them all we can work toward a collective solution.

Problem

I cannot speak for everyone, but i believe the key call out here is the inability to use event notifications in environments where IAM access is not available (typically enterprise).

Desired Solution

In short, I believe each of the attached issues require the ability to override the IAM Role utilized by the BucketNotificationsHandler.

Workaround

This worked enough (Python), but is far from desirable and has yet to pass review from the client. Thanks to @iliapolo for pointing out the well known notification handler id!

bucket_notification_handler = cdk.Stack.of(self).node.try_find_child('BucketNotificationsHandler050a0587b7544547bf325f094a3db834')
for child in bucket_notification_handler.node.children:
    if hasattr(child, 'role_arn'):
        # Removes the Role resource created
        bucket_notification_handler.node.try_remove_child(child.node.id)
    elif hasattr(child, 'cfn_resource_type') and child.cfn_resource_type == 'AWS::Lambda::Function':
        # Overrides the Role resource used
        child.add_property_override('Role', lambda_execution_role.role_arn)
        # Removes dependency on non-existent resources (the role removed above)
        child.add_override('DependsOn', None)

Cheers!

Resources

Construct Node
CfnResource
Bucket Event Notification Handler Definition
Escape Hatches

[github-actions]

This issue has not received any attention in 1 year. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

[spourali]

you don't need to do all these magics. you just need to have (or Create) your boundary first

Step 1

// This imports an existing policy.
const boundary = iam.ManagedPolicy.fromManagedPolicyArn(this, 'Boundary', 'arn:aws:iam::123456789012:policy/boundary');

OR

// Create a new Boundary
const boundary2 = new iam.ManagedPolicy(this, 'Boundary2', {
  statements: [
    new iam.PolicyStatement({
      effect: iam.Effect.DENY,
      actions: ['iam:*'],
      resources: ['*'],
    }),
  ],
});

Step2

Then apply the boundary to all Roles in a stack

// Apply the boundary to all Roles in a stack
iam.PermissionsBoundary.of(this).apply(boundary);

[Kirity]

Problem
Security checks are failing because of non-restrictive policy("*" in the policy).

Workaround in TypeScript
Remove the non-restrictive policy and add a restrictive policy.

const wellKnownNotificationHandlerId = "BucketNotificationsHandler050a0587b7544547bf325f094a3db834"

const bktNotificationsHandler = stack.node.tryFindChild( wellKnownNotificationHandlerId,)
    
const bktNotificationsHandlerRole =
      bktNotificationsHandler?.node.children.find((n) =>
        n.node.path.endsWith("Role"),
      ) as iam.Role

 // Removing the non-restrictive policy
bktNotificationsHandlerRole.node.tryRemoveChild("DefaultPolicy")
       
const putNotificationsRestricted = new iam.PolicyStatement({
      actions: ["s3:PutBucketNotification"],
      resources: [s3Bucket.bucketArn + "/", s3Bucket.bucketArn],
    })

const getNotificationsRestricted = new iam.PolicyStatement({
      actions: ["s3:GetBucketNotification"],
      resources: [s3Bucket.bucketArn + "/", s3Bucket.bucketArn],
    })
   
const customRestrictivePolicy = new iam.Policy(stack, "custom-policy", {
      statements: [putNotificationsRestricted, getNotificationsRestricted],
    })
   
bktNotificationsHandlerRole.attachInlinePolicy(customRestrictivePolicy)

[faridnsh]

My suggested solution is just add the permission where the singleton is instantiated. We already have another permission being added here, might as well have both permissions added in one place only. I'm happy to make a PR with this change, if there's no objection.

[faridnsh]

I have created a PR for it: #27274

I would appreciate if it could be reviewed.