S3 Event Notification: SNS destination access policy not setting "aws:SourceAccount"

[rkraman-leo]

Describe the bug

Ref: https://github.com/aws/aws-cdk/blob/v2.105.0/packages/aws-cdk-lib/aws-s3-notifications/lib/sns.ts

As per the documentation - https://docs.aws.amazon.com/AmazonS3/latest/userguide/ways-to-add-notification-config-to-bucket.html#step1-create-sns-topic-for-notification

The expected Access policy should include in "Condition", but missing.

"StringEquals": {
    "aws:SourceAccount": "bucket-owner-account-id"
}

I believe this is causing S3 object create event to not trigger SNS notification

Expected Behavior

SNS notification triggered on S3 object creation

Current Behavior

SNS notification not triggered

Reproduction Steps

const bucket = new Bucket(this, `some-bucket-name`, {
      bucketName: 'some-bucket',
      encryption: BucketEncryption.S3_MANAGED,
      blockPublicAccess: BlockPublicAccess.BLOCK_ALL,
      enforceSSL: true,
      autoDeleteObjects: true,
      removalPolicy: RemovalPolicy.DESTROY,
    });\

    //Setup SNS
    const topicName = `some-topic-name`
    const topic = new Topic(this, 'some-id', {
      displayName: `some name`,
      topicName: topicName
    });

    //Setup S3 event notification publish to SNS
    bucket.addEventNotification(EventType.OBJECT_CREATED, new SnsDestination(topic), {prefix: 'folder/subfolder/'});

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

v2.105.0

Framework Version

No response

Node.js Version

v2.105.0

OS

MacOS

Language

TypeScript

Language Version

No response

Other information

No response

[khushail]

thanks @rkraman-leo for reporting this.

[msambol]

I'll take this.

[rix0rrr]

The aws:SourceAccount condition is not necessary for SNS notifications to work.

One might argue they prevent a confused deputy attack, but since we already have a condition on aws:SourceArn, that confused deputy attack is already prevented.

I'm also concerned that if someone calls Bucket.fromBucketArn(), we won't correctly parse the Account ID from the ARN (given that an S3 Bucket's ARN does not contain the account ID), and adding this additional condition is going to break some people's code.

I'm seeing no upside, and only downside to merging this. Anyone feel differently?