s3: BucketPolicy.fromCfnBucketPolicy causes a SynthesisError

[ramblingenzyme]

Describe the bug

When using .fromCfnBucketPolicy, the method extends the normal BucketPolicy class which creates an extra CfnBucketPolicy in its constructor.
https://github.com/aws/aws-cdk/blame/44f6d1616b1a0c2a32fd27556db28b2ebfb275bb/packages/aws-cdk-lib/aws-s3/lib/bucket-policy.ts#L81-L85

    const ret = new class extends BucketPolicy {
      public readonly document = PolicyDocument.fromJson(cfnBucketPolicy.policyDocument);
    }(cfnBucketPolicy, id, {
      bucket,
    });

https://github.com/aws/aws-cdk/blame/44f6d1616b1a0c2a32fd27556db28b2ebfb275bb/packages/aws-cdk-lib/aws-s3/lib/bucket-policy.ts#L110-L113

    this.resource = new CfnBucketPolicy(this, 'Resource', {
      bucket: this.bucket.bucketName,
      policyDocument: this.document,
    });

Regression Issue

• Select this option if this issue appears to be a regression.

Last Known Working CDK Version

No response

Expected Behavior

The stack to synth

Current Behavior

A SynthesisError gets thrown

CfnSynthesisError: Resolution error: Supplied properties not correct for "CfnBucketPolicyProps"
  policyDocument: required but missing.
    at ValidationResult.assertSuccess (/workspaces/repro/node_modules/aws-cdk-lib/core/lib/runtime.js:1:2801)
    at convertCfnBucketPolicyPropsToCloudFormation (/workspaces/repro/node_modules/aws-cdk-lib/aws-s3/lib/s3.generated.js:1:160394)
    at CfnBucketPolicy.renderProperties (/workspaces/repro/node_modules/aws-cdk-lib/aws-s3/lib/s3.generated.js:1:159177)
    at PostResolveToken.Resources (/workspaces/repro/node_modules/aws-cdk-lib/core/lib/cfn-resource.js:1:7901)
    at PostResolveToken.postProcess (/workspaces/repro/node_modules/aws-cdk-lib/core/lib/util.js:1:1648)
    at Object.postProcess (/workspaces/repro/node_modules/aws-cdk-lib/core/lib/private/resolve.js:1:1241)
    at DefaultTokenResolver.resolveToken (/workspaces/repro/node_modules/aws-cdk-lib/core/lib/resolvable.js:1:1483)
    at resolve (/workspaces/repro/node_modules/aws-cdk-lib/core/lib/private/resolve.js:1:2747)
    at Object.resolve (/workspaces/repro/node_modules/aws-cdk-lib/core/lib/private/resolve.js:1:1115)
    at resolve (/workspaces/repro/node_modules/aws-cdk-lib/core/lib/private/resolve.js:1:3026) {
  type: 'CfnSynthesisError'
}

Reproduction Steps

Adapted from https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_s3.BucketPolicy.html#example

1. Run cdk init app --language typescript
2. Replace the contents of lib/$file.ts with the code below
3. Run cdk synth

import * as cdk from 'aws-cdk-lib';
import { PolicyStatement, ServicePrincipal } from 'aws-cdk-lib/aws-iam';
import { Bucket, BucketPolicy, CfnBucketPolicy } from 'aws-cdk-lib/aws-s3';
import { Construct } from 'constructs';
// import * as sqs from 'aws-cdk-lib/aws-sqs';

export class ReproStack extends cdk.Stack {
  constructor(scope: Construct, id: string, props?: cdk.StackProps) {
    super(scope, id, props);

    const bucketName = "amzn-s3-demo-bucket";

    const bucket = new Bucket(this, "Bucket", {
      bucketName
    })

    const bucketPolicy = new CfnBucketPolicy(this, "BucketPolicy", {
      bucket: bucketName,
      policyDocument: {
        Statement: [
          {
            Action: 's3:*',
            Effect: 'Deny',
            Principal: {
              AWS: '*',
            },
            Resource: [
              `arn:aws:s3:::${bucketName}`,
              `arn:aws:s3:::${bucketName}/*`,
            ],
          },
        ],
        Version: '2012-10-17',
      },
    });

    BucketPolicy.fromCfnBucketPolicy(bucketPolicy);

    bucket.addToResourcePolicy(new PolicyStatement({
      actions: ["s3:ListObject"],
      principals: [new ServicePrincipal("cloudfront.amazonaws.com")]
    }))
  }
}

Possible Solution

It may be required to create an IBucketPolicy interface & BucketPolicyBase class which doesn't create a CfnBucketPolicy in the constructor.

Alternatively,

Additional Information/Context

No response

CDK CLI Version

2.1013.0 (build 054afef)

Framework Version

No response

Node.js Version

18.20.5

OS

macOS

Language

TypeScript

Language Version

5.6.3

Other information

No response

[pahud]

Root Cause Analysis

The core issue lies in the interaction between the static fromCfnBucketPolicy method and the BucketPolicy constructor:

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

Lines 110 to 113 in e37faed

	this.resource = new CfnBucketPolicy(this, 'Resource', {
	bucket: this.bucket.bucketName,
	policyDocument: this.document,
	});

The BucketPolicy.fromCfnBucketPolicy method creates an anonymous class that extends BucketPolicy. Instantiating this anonymous class implicitly calls the base BucketPolicy constructor, triggering the creation of the problematic second CfnBucketPolicy.

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

Lines 81 to 85 in e37faed

	const ret = new class extends BucketPolicy {
	public readonly document = PolicyDocument.fromJson(cfnBucketPolicy.policyDocument);
	}(cfnBucketPolicy, id, {
	bucket,
	});

This second CfnBucketPolicy is synthesized without a valid policyDocument, causing the CfnSynthesisError: Resolution error: Supplied properties not correct for "CfnBucketPolicyProps" policyDocument: required but missing.

Possible Fixes/Solutions

Yes, inntroducing an IBucketPolicy interface and a BucketPolicyBase class (which would not create the CfnBucketPolicy resource in its constructor) appears to be the most robust solution.

1. Define IBucketPolicy interface.
2. Create BucketPolicyBase implementing IBucketPolicy without the resource creation logic.
3. Refactor the current BucketPolicy to extend BucketPolicyBase and move the CfnBucketPolicy creation into its own constructor.
4. Update BucketPolicy.fromCfnBucketPolicy to return an object based on BucketPolicyBase or IBucketPolicy, avoiding the problematic constructor call chain.

Thank you for bringing this to our attention. I will share this with the team for further discussion and input.

[ramblingenzyme]

Thanks @pahud! 🙇🏽

For completeness' sake, the hacky alternative I forgot to actually add in my original report was to change the extended class to delete the extra CfnBucketPolicy in its constructor, something along the lines of:

     const ret = new class extends BucketPolicy {
      public readonly document: PolicyDocument;

      constructor() {
        super(cfnBucketPolicy, id, { bucket: bucket! });

        // reassign the internal resource and document references
        this.document = PolicyDocument.fromJson(cfnBucketPolicy.policyDocument);
        this.resource = cfnBucketPolicy;

        // Remove the CfnBucketPolicy created by running `super()`
        const childName = this.node.defaultChild?.node.id || 'Resource';
        this.node.tryRemoveChild(childName);

        this.node.defaultChild = cfnBucketPolicy;
      }
    }();

Untested but the logic seems sound.

[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.