aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.61k stars 3.91k forks source link

aws-s3: The `autoDeleteObjects` prop of `Bucket` is too brittle #31358

Closed garysassano closed 1 month ago

garysassano commented 1 month ago

Describe the bug

We are going to deploy this CDK stack and observe the behavior.

const uniqueId = this.node.addr.substring(0, 8);

const myBucket = new Bucket(this, "MyBucket", {
  bucketName: `my-bucket-${uniqueId}`,
  enforceSSL: true,
  removalPolicy: RemovalPolicy.DESTROY,
  autoDeleteObjects: true,
});

const myBucketPolicy = new BucketPolicy(this, "MyBucketPolicy", {
  bucket: myBucket,
});

const myUser = new User(this, "MyUser", {
  userName: "my-user",
});

myBucketPolicy.document.addStatements(
  new PolicyStatement({
    effect: Effect.ALLOW,
    principals: [myUser],
    actions: ["s3:GetObject", "s3:ListBucket"],
    resources: [myBucket.bucketArn, `${myBucket.bucketArn}/*`],
  }),
);

Regression Issue

Last Known Working CDK Version

No response

Expected Behavior

I expected the cdk deploy to fail immediately due to a conflict between the autoDeleteObjects custom resource and the BucketPolicy we defined. Since only one bucket policy can exist at a time, the system should have detected this conflict and prevented the deployment.

Current Behavior

However, the stack deploys successfully without any error.

image

Inspecting the S3 bucket policy shows that the BucketPolicy we explicitly created overrides the one generated by autoDeleteObjects:

{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Effect": "Allow",
            "Principal": {
                "AWS": "arn:aws:iam::533267016779:user/my-user"
            },
            "Action": [
                "s3:GetObject",
                "s3:ListBucket"
            ],
            "Resource": [
                "arn:aws:s3:::my-bucket-c812be60",
                "arn:aws:s3:::my-bucket-c812be60/*"
            ]
        }
    ]
}

When attempting to destroy the stack with cdk destroy, it fails because the Lambda function linked to autoDeleteObjects custom resource lacks the necessary permissions to delete the S3 bucket. This occurs because the BucketPolicy we defined overrides the policy that grants the required permissions.

Reproduction Steps

see above

Possible Solution

The CDK should enforce a check to prevent defining a BucketPolicy that overrides the permissions needed by the autoDeleteObjects custom resource. Although the deployment works, it leads to conflicts during stack destruction due to missing permissions.

Additional Information/Context

No response

CDK CLI Version

2.156.0

Framework Version

No response

Node.js Version

20.17.0

OS

Ubuntu 22.04.3 LTS

Language

TypeScript

Language Version

No response

Other information

No response

khushail commented 1 month ago

Hi @garysassano , thanks for reaching out. I am able to reproduce the scenario with given bucket policy. Its observed that adding a new bucket policy with existing Bucket Policy with autoDeleteObject:true actually leads to error when deleting the stack.

Received response status [FAILED] from custom resource. Message returned: AccessDenied: User: arn:aws:sts::1234567890:assumed-role/BucketAutoDeleteObjectP
ol-CustomS3AutoDeleteObjects-mHw0z310uANA/BucketAutoDeleteObjectPol-CustomS3AutoDeleteObject-mSxwX4IBLB2P is not authorized to perform: s3:GetBucketTagging
on resource: "arn:aws:s3:::my-bucket-c8939193" because no identity-based policy allows the s3:GetBucketTagging action

Sharing my observation -

When a bucket is created with autoDeleteObject:true -

https://github.com/aws/aws-cdk/blob/7315a59795a783a94a576ce24c07f11498e5291d/packages/aws-cdk-lib/aws-s3/lib/bucket.ts#L2562

these are the permissions granted -

https://github.com/aws/aws-cdk/blob/7315a59795a783a94a576ce24c07f11498e5291d/packages/aws-cdk-lib/aws-s3/lib/bucket.ts#L2570

    this.addToResourcePolicy(new iam.PolicyStatement({
      actions: [
        // prevent further PutObject calls
        ...perms.BUCKET_PUT_POLICY_ACTIONS,
        // list objects
        ...perms.BUCKET_READ_METADATA_ACTIONS,
        ...perms.BUCKET_DELETE_ACTIONS, // and then delete them
      ],

which results in this snippet of synthesized template when deployed

      {
       "Action": [
        "s3:DeleteObject*",
        "s3:GetBucket*",
        "s3:List*",
        "s3:PutBucketPolicy"
       ],

but after defining the new policy, one sees the updation in Bucket policy-

            "Action": [
                "s3:GetObject",
                "s3:ListBucket"
            ],

Hence leading to error during deletion.

khushail commented 1 month ago

@garysassano , it says in the BucketPolicy doc that it should not be preferred way of adding policy -

The bucket policy for an Amazon S3 bucket.

Policies define the operations that are allowed on this resource.

You almost never need to define this construct directly.

All AWS resources that support resource policies have a method called addToResourcePolicy(), which will automatically create a new resource policy if one doesn't exist yet, otherwise it will add to the existing policy.

Prefer to use addToResourcePolicy() instead.
pahud commented 1 month ago

The BucketPolicy is not aware if there is another BucketPolicy being created for the same bucket hence it's not preferred.

The general recommended practice is always use bucket.addToResourcePolicy() instead.

khushail commented 1 month ago

@garysassano , I just rewrote the code with using addToResourcePolicy() and what it does it appends the Bucket policy and hence when the stack is destroyed, it succeeds without any error. Please see the shared snippet and template -


    const myBucket015 = new s3.Bucket(this, "MyBucket015", {
      bucketName: `my-bucket-015`,
      enforceSSL: true,
      removalPolicy: RemovalPolicy.DESTROY,
      autoDeleteObjects: true,
    });

    myBucket015.addToResourcePolicy( new iam.PolicyStatement({
      effect: iam.Effect.ALLOW,
      principals: [new iam.ArnPrincipal("3********6")],
      actions: ["s3:GetObject", "s3:ListBucket"],
      resources: [myBucket015.bucketArn, `${myBucket015.bucketArn}/*`],
    }));

{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Effect": "Deny",
            "Principal": {
                "AWS": "*"
            },
            "Action": "s3:*",
            "Resource": [
                "arn:aws:s3:::my-bucket-015",
                "arn:aws:s3:::my-bucket-015/*"
            ],
            "Condition": {
                "Bool": {
                    "aws:SecureTransport": "false"
                }
            }
        },
        {
            "Effect": "Allow",
            "Principal": {
                "AWS": "arn:aws:iam::3*******6:role/BucketV2IssueStack-CustomS3AutoDeleteObjectsCustomR-op0fiXi89QWA"
            },
            "Action": [
                "s3:DeleteObject*",
                "s3:GetBucket*",
                "s3:List*",
                "s3:PutBucketPolicy"
            ],
            "Resource": [
                "arn:aws:s3:::my-bucket-015",
                "arn:aws:s3:::my-bucket-015/*"
            ]
        },
        {
            "Effect": "Allow",
            "Principal": {
                "AWS": "arn:aws:iam::3*******6:root"
            },
            "Action": [
                "s3:GetObject",
                "s3:ListBucket"
            ],
            "Resource": [
                "arn:aws:s3:::my-bucket-015",
                "arn:aws:s3:::my-bucket-015/*"
            ]
        }
    ]
}
Screenshot 2024-09-10 at 2 29 24 PM

Hope that would be helpful.

github-actions[bot] commented 1 month ago

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.