Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
1 task
garysassano opened this issue Sep 7, 2024 · 5 comments · Fixed by #31395
Closed
1 task

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

garysassano opened this issue Sep 7, 2024 · 5 comments · Fixed by #31395
Assignees
Labels
@aws-cdk/aws-s3 Related to Amazon S3 bug This issue is a bug. effort/small Small work item – less than a day of effort p2

Comments

@garysassano
Copy link

garysassano commented Sep 7, 2024

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

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

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

@garysassano garysassano added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Sep 7, 2024
@github-actions github-actions bot added the @aws-cdk/aws-s3 Related to Amazon S3 label Sep 7, 2024
@khushail khushail added investigating This issue is being investigated and/or work is in progress to resolve the issue. p2 and removed needs-triage This issue or PR still needs to be triaged. labels Sep 10, 2024
@khushail khushail self-assigned this Sep 10, 2024
@khushail
Copy link
Contributor

khushail commented Sep 10, 2024

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 -

private enableAutoDeleteObjects() {

these are the permissions granted -

this.addToResourcePolicy(new iam.PolicyStatement({

    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 khushail added p1 effort/small Small work item – less than a day of effort and removed investigating This issue is being investigated and/or work is in progress to resolve the issue. p2 labels Sep 10, 2024
@khushail
Copy link
Contributor

khushail commented Sep 10, 2024

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

@khushail khushail added p2 and removed p1 labels Sep 10, 2024
@pahud
Copy link
Contributor

pahud commented Sep 10, 2024

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.

@pahud pahud added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Sep 10, 2024
@khushail
Copy link
Contributor

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

Copy link

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 github-actions bot locked as resolved and limited conversation to collaborators Sep 10, 2024
@khushail khushail removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Sep 11, 2024
mergify bot pushed a commit that referenced this issue Sep 17, 2024
### Issue #[31358](#31358)

Closes #31358 .

### Reason for this change

Exsiting [CDK Doc on BucketPolicy](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_s3.BucketPolicy.html) mentions as `Prefer to use` which is misleading as it does not clearly states the reprecussions. 

### Description of changes

I have added a sample of what would happen if this is used along with other Bucket properties.

### Description of how you validated changes

This is a minor documentation change

### Checklist
- [ ] My code adheres to the [CONTRIBUTING GUIDE](https:/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https:/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
@aws-cdk/aws-s3 Related to Amazon S3 bug This issue is a bug. effort/small Small work item – less than a day of effort p2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants