(aws-cdk-lib/aws-s3): Too many BucketPolicy resources defined when defining/attaching BucketPolicy constructs to Buckets

[climbertjh2]

Describe the bug

Situation:

• Create two (2) Bucket constructs in a Stack
• Create two (2) BucketPolicy constructs in the same Stack
• Identify one of the buckets as an "access logging" bucket for the other bucket.

Result:

• three (3) AWS::S3::BucketPolicy CloudFormation resources are synthesized
• all three wind up getting created when the Stack is deployed
• two of the three point to the same AWS S3 bucket
• net result is that the "last" BucketPolicy (not the UNION of the BucketPolicy constructs) "wins"
• The resulting BucketPolicy does not match what is defined in the CDK application.

Expected Behavior

Expected Behavior is either:

• a) only the explicitly indicated BucketPolicy is created
• b) the implicitly created BucketPolicy and the explicitly created BucketPolicy is what is synthesized/deployed
• c) an error is indicated that the explicit BucketPolicy would NOT be appropriate if it lacks the necessary permissions (in this case, allowing the AWS logging service to be able to write to the identified access logging bucket)

Current Behavior

Result:

• three (3) AWS::S3::BucketPolicy CloudFormation resources are synthesized
• all three wind up getting created when the Stack is deployed
• two of the three point to the same AWS S3 bucket
• net result is that the "last" BucketPolicy (not the UNION of the BucketPolicy constructs) "wins"
• The resulting BucketPolicy does not match what is defined in the CDK application.

Reproduction Steps

See above.

Possible Solution

Suggested Behavior:

• c) an error is indicated that the explicit BucketPolicy is NOT appropriate since it lacks the necessary permissions (in this case, allowing the AWS logging service to be able to write to the identified access logging bucket)

Or a recommendation to use .addToResourcePolicy() rather than creating explicit BucketPolicy constructs.

Additional Information/Context

Sample code can be provided on request.

CDK CLI Version

2.141.0

Framework Version

[email protected]

Node.js Version

v20.12.2

OS

MacOS

Language

TypeScript

Language Version

[email protected]

Other information

No response

[khushail]

Hi @climbertjh2 , Thanks for reaching out. Could you please provide the minimum sample code for repro of this issue ?

[github-actions]

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

[climbertjh2]

@khushail - I will try to create a small sample. (I left instructions above in the description).

[climbertjh2]

I have tried to re-create without success. Closing this one.

[github-actions]

⚠️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.

[climbertjh2]

@khushail - correction - I have been able to re-create this. Re-opening the issue.

[climbertjh2]

Pasting in an example to re-create - with some commentary:

import { Stack, StackProps, RemovalPolicy, aws_s3 as s3, aws_iam as iam } from 'aws-cdk-lib';
import { Construct } from 'constructs';
import { resourceUsage } from 'process';
// import * as sqs from 'aws-cdk-lib/aws-sqs';

export class CdkBug30148Stack extends Stack {

  private bucket01: s3.Bucket;
  private bucketPolicy01: s3.CfnBucketPolicy;
  private bucket02: s3.Bucket;
  private bucketPolicy02: s3.CfnBucketPolicy;

  constructor(scope: Construct, id: string, props?: StackProps) {
    super(scope, id, props);

    // The code that defines your stack goes here

    const bucket01Name="my-favorite-bucket-name-01";
    this.bucket01 = new s3.Bucket(this, "Bucket01", {
      bucketName: bucket01Name,
      blockPublicAccess: s3.BlockPublicAccess.BLOCK_ALL,
      encryption: s3.BucketEncryption.S3_MANAGED,
      // enforceSSL: true,
      versioned: true,
      removalPolicy: RemovalPolicy.RETAIN,
    })

    // Instead of using enforceSSL: true, set the bucket policy explicitily
    // This creates a AWS::S3::BucketPolicy resource in the CloudFormation template

    this.bucketPolicy01 = new s3.CfnBucketPolicy( this, "BucketPolicy01", {
      bucket: bucket01Name,
      policyDocument: {
        Statement: [
          {
            Effect: "Deny",
            Action: [
              "s3:*"
            ],
            Principal: {
              "AWS": "*"
            },
            Resource: [
              // `arn:aws:s3:::${bucket01Name}`,
              // `arn:aws:s3:::${bucket01Name}/*`,
              this.bucket01.bucketArn,
              `${this.bucket01.bucketArn}/*`,
            ],
            Condition: {
              "Bool": {
                "aws:SecureTransport": false
              }
            }
          },
          {
            Effect: "Deny",
            Action: [
              "s3:PutObject"
            ],
            Principal: {
              "AWS": "*"
            },
            Resource: [
              // `arn:aws:s3:::${bucket01Name}`,
              // `arn:aws:s3:::${bucket01Name}/*`,
              this.bucket01.bucketArn,
              `${this.bucket01.bucketArn}/*`,
            ]
          }
        ]
      }
    })

    const bucket02Name="my-favorite-bucket-name-02";
    this.bucket02 = new s3.Bucket(this, "Bucket02", {
      bucketName: bucket02Name,
      blockPublicAccess: s3.BlockPublicAccess.BLOCK_ALL,
      encryption: s3.BucketEncryption.S3_MANAGED,
      // enforceSSL: true,
      versioned: true,
      removalPolicy: RemovalPolicy.RETAIN,
      // The addition of this property has the SIDE-EFFECT of adding (and overwriting)
      // the BucketPolicy that was associated with bucket01 above.
      //
      // The net result is that there are 3 AWS::S3::BucketPolicy resources defined
      // in the CloudFormation template, and only the "last" one created
      // (the one associated with setting up a bucket policy for the Logging service
      // to write to the Access Logs bucket) is what is associated with bucket01.
      serverAccessLogsBucket: this.bucket01
    })
  
    // Instead of using enforceSSL: true, set the bucket policy explicitily
    // This creates a AWS::S3::BucketPolicy resource in the CloudFormation template

    this.bucketPolicy02 = new s3.CfnBucketPolicy( this, "BucketPolicy02", {
      bucket: bucket02Name,
      policyDocument: {
        Statement: [
          {
            Effect: "Deny",
            Action: [
              "s3:*"
            ],
            Principal: {
              "AWS": "*"
            },
            Resource: [
              // `arn:aws:s3:::${bucket02Name}`,
              // `arn:aws:s3:::${bucket02Name}/*`,
              this.bucket02.bucketArn,
              `${this.bucket02.bucketArn}/*`,
            ],
            Condition: {
              Bool: {
                "aws:SecureTransport": false
              }
            }
          }
        ]
      }
    })
  
  }

}

The net result, after deploying that Stack, is that 3 BucketPolicies are created, but only 2 of them are in effect. The explicit BucketPolicy associated with this.bucket01 is overwritten entirely by the BucketPolicy that is set up to allow the Logging service to write to the designated serverAccessLogsBucket bucket (this.bucket01).

[climbertjh2]

NOTE - I also noticed something else while re-creating this issue.

IF serverAccessLogsBucket is NOT specified, but the two CfnBucketPolicy objects are created, THEN the cdk deploy runs WITHOUT verifying with the user that they are creating IAM resources.

This seems like a way around the --require-approval flag/setting (or a "miss" in checking for resources in the CloudFormation template that should trigger a verification from the user).

**IF serverAccessLogsBucket IS specified, THEN the user is asked about ALL THREE BucketPolicy resources to-be created.

[khushail]

Hey @climbertjh2 , thanks for sharing the code and having patience!

I repro'd the issue and here is my observation -

1. when used without serverAccessLogsBucket , 2 resource policy resources are generated along with corresponding buckets which are sucessfully associated with buckets -

Resources
[+] AWS::S3::Bucket Bucket0001 Bucket00014CEA8A3F 
[+] AWS::S3::BucketPolicy BucketPolicy0001 BucketPolicy0001 
[+] AWS::S3::Bucket Bucket0002 Bucket0002E47E4676 
[+] AWS::S3::BucketPolicy BucketPolicy0002 BucketPolicy0002 

2. This is the bucket Policy for Bucket0001 -

3. When serverAccessLogsBucket is enabled , another customer resource policy for Bucket0001 is created, hence we see this result in cdk diff which makes sense

Resources
[+] AWS::S3::BucketPolicy Bucket0001/Policy Bucket0001Policy335FA23C 
[~] AWS::S3::Bucket Bucket0002 Bucket0002E47E4676 
 └─ [+] LoggingConfiguration
     └─ {"DestinationBucketName":{"Ref":"Bucket00014CEA8A3F"}}

3. As a part of deployment, there is 1 new resource policy which overwrites the existent one, t and we have this -

{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Effect": "Allow",
            "Principal": {
                "Service": "logging.s3.amazonaws.com"
            },
            "Action": "s3:PutObject",
            "Resource": "arn:aws:s3:::my-favorite-bucket-0001/*",
            "Condition": {
                "StringEquals": {
                    "aws:SourceAccount": "*************"
                },
                "ArnLike": {
                    "aws:SourceArn": "arn:aws:s3:::my-favorite-bucket-name-0002"
                }
            }
        }
    

So yes, your point is absolutely Correct. The last bucket policy overwrites the existing one. Also enabling serverAccessLogsBucket enabling notifies the user.

Checking the CDK code, I think this might be the Custom policy overwrites the existing one -

aws-cdk/packages/aws-cdk-lib/aws-s3/lib/bucket.ts

Line 2472 in 8d55d86

private allowLogDelivery(from: IBucket, prefix?: string) {

and this is where its called -

aws-cdk/packages/aws-cdk-lib/aws-s3/lib/bucket.ts

Line 1976 in 8d55d86

props.serverAccessLogsBucket.allowLogDelivery(this, props.serverAccessLogsPrefix);

Thanks for reporting this!

[climbertjh2]

I didn't look at the code - but just considering the situation, it feels like the L2 construct isn't checking whether a L1 CfnBucketPolicy is associated with the bucket prior to creating a bucket policy (in this example, to add the access logging permissions).

This mixing of L2 and L1 constructs may be discouraged - but it is possible.

[GavinZZ]

This should not be classified as p1 bug. As pointed out, we do not encourage the mix of L1 and L2 constructs. If you want to explicitly create a Bucket Policy, the recommended way is to use

this.bucket01 = new s3.Bucket(this, "Bucket01", {...});
this.bucket01.addToResourcePolicy(...)

or additionally you can

this.bucketPolicy01 = new s3.CfnBucketPolicy( this, "BucketPolicy01", {...});
new s3.BucketPolicy.fromCfnBucketPolicy(this.bucketPolicy01);

[climbertjh2]

I agree that this is not p1 bug!

[GavinZZ]

it feels like the L2 construct isn't checking whether a L1 CfnBucketPolicy is associated with the bucket

Yes, you're right. And I believe this is the correct behaviour and L2 should not check for the existence of L1 constructs. If we start doing this here, there are plenty more places to check. For this issue, I think what I can do is to update the S3 documentation and explains the limitation and suggest the workarounds like above, either using addToResourcePolicy or wrap the L1 Bucket Policy with s3.BucketPolicy.fromCfnBucketPolicy.

[climbertjh2]

@GavinZZ - I'm OK with that resolution - update the documentation to point out not mixing L2 and L1 constructs - or if doing so, suggest the alternatives as noted.

[github-actions]

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

[github-actions]

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.