feat(s3): use Bucket Policy for Server Access Logging grant (under feature flag)

[laurelmay]

Using ACLs to grant access to buckets is no longer recommended. In fact,
it doesn't work if Object Ownership is set to be enforced for the
bucket. According to the service documentation for enabling server
access logging, it is now preferred to use a bucket policy to grant
permission to deliver logs to a bucket.

Changing the default would result in changes to deployed resources, so
the new behavior is added behind a feature flag.

An alternative here may be to use the Bucket Policy either when the feature
flag is enabled or when ownership is set to BUCKET_OWNER_ENFORCED since
the latter doesn't work with the current implementation anyway.

Closes: #22183

---

All Submissions:

• Have you followed the guidelines in our Contributing guide?

Adding new Construct Runtime Dependencies:

• This PR adds new construct runtime dependencies following the process described here

New Features

• Have you added the new feature to an integration test?
  • Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[gitpod-io]

[aws-cdk-automation]

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

[laurelmay]

Features must contain a change to an integration test file and the resulting snapshot.

I assume that like in #22740 this is just covered by the existing integration test. If there's a way to actually test both sides of the feature flag, I'd be happy to write an additional test but it seems like integ-runner uses the NEW_PROJECT_FLAGS.

[corymhall]

@kylelaker we had a previous PR that was going to do this, but we ended up closing it because some services still require the ACL #20898

[laurelmay]

@kylelaker we had a previous PR that was going to do this, but we ended up closing it because some services still require the ACL #20898

Oh I missed that. I guess in searching I only looked at open PRs. Sorry for the duplicate.

For what it's worth, the idea of a single shared logs bucket is a bit broken with that CloudFront example. The ACL grant is done by CloudFormation itself when specifying the Logging property (putting the ACL in the template isn't required like it is for S3). This results in conflicting ACLs being written (CloudFront requires FULL_CONTROL and S3 uses the canned ACL for read/write). So when combined, depending on ordering, CloudFront is unable to write logs anyway.

This PR also does not enforce BUCKET_OWNER_ENFORCED but purely adopts a bucket policy. That might even fix the CloudFront use case because there wouldn't be competing ACLs for the Log Delivery (but that certainly wasn't intentional as I was writing this).

Here is the stack I used for testing this:

import * as cdk from 'aws-cdk-lib';
import * as cloudfront from 'aws-cdk-lib/aws-cloudfront';
import * as origins from 'aws-cdk-lib/aws-cloudfront-origins';
import * as s3 from 'aws-cdk-lib/aws-s3';
import * as s3deploy from 'aws-cdk-lib/aws-s3-deployment';
import { Construct } from 'constructs';

export class TestS3Stack extends cdk.Stack {
  constructor(scope: Construct, id: string, props?: cdk.StackProps) {
    super(scope, id, props);
    const logsBucket = new s3.Bucket(this, 'LogsBucket');
    const content = new s3.Bucket(this, 'Content');
    const deployment = new s3deploy.BucketDeployment(this, 'ContentDeployment', {
      sources: [s3deploy.Source.asset('website')],
      destinationBucket: content,
    });
    const origin = new origins.S3Origin(content);

    const distribution = new cloudfront.Distribution(this, 'Distribution', {
      // CloudFormation handles the ACL grant for CloudFront to this bucket. It
      // will grant FULL_CONTROL to c4c1ede66af53448b93c283ce9448c4ba468c9432aa01d700d3878632f77d2d0
      // as well as to the current account
      logBucket: logsBucket,
      logFilePrefix: "distribution",
      defaultBehavior: { origin },
    });
  }

If any bucket starts logging to logsBucket after the initial creation of this stack, log delivery from CloudFront will start to fail because the FULL_CONTROL access to that Canonical User ID is no longer present. If the buckets stopped needing ACLs for log delivery and instead used the bucket policy, CloudFront would still work after adding a bucket that logs to logsBucket.

This feels like it's actually a CloudFormation bug (ideally, the Distribution wouldn't be so magical about ACLs or it'd be even more magical and figure out when the Bucket's ACL changed and update accordingly).

But because this PR doesn't start requiring BUCKET_OWNER_ENFORCED, the only thing that it would do is allow users to start adopting the policy format instead and elect to use BUCKET_OWNER_ENFORCED if that works for them. Instead of being behind a feature flag, this could just be done automatically if BUCKET_OWNER_ENFORCED is set because that would break any services that require ACLs anyway.

Overall, this whole situation is a bit of a mess. Hopefully all services can start working with Bucket Policy soon.

[corymhall]

But because this PR doesn't start requiring BUCKET_OWNER_ENFORCED, the only thing that it would do is allow users to start adopting the policy format instead and elect to use BUCKET_OWNER_ENFORCED if that works for them. Instead of being behind a feature flag, this could just be done automatically if BUCKET_OWNER_ENFORCED is set because that would break any services that require ACLs anyway.

Overall, this whole situation is a bit of a mess. Hopefully all services can start working with Bucket Policy soon.

@kylelaker thanks for the detailed explanation! I think I'm good with your solution. Can update the S3 readme to have a new entry that shows our recommended configuration?

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

Pull request has been modified.

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: cdcb49a
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).