feat(s3): use Bucket Policy for Server Access Logging grant (under fe…

…ature flag) (#23386)

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][1], 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 

[1]: https://docs.aws.amazon.com/AmazonS3/latest/userguide/enable-server-access-logging.html


----

### All Submissions:

* [X] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Construct Runtime Dependencies:

* [ ] This PR adds new construct runtime dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-construct-runtime-dependencies)

### New Features

* [X] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [X] 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*

packages/@aws-cdk/aws-s3/README.md

@@ -349,7 +349,25 @@ const bucket = new s3.Bucket(this, 'MyBucket', {
 });
 ```
 
-[S3 Server access logging]: https://docs.aws.amazon.com/AmazonS3/latest/dev/ServerLogs.html
+### Allowing access log delivery using a Bucket Policy (recommended)
+
+When possible, it is recommended to use a bucket policy to grant access instead of
+using ACLs. When the `@aws-cdk/aws-s3:serverAccessLogsUseBucketPolicy` feature flag
+is enabled, this is done by default for server access logs. If S3 Server Access Logs
+are the only logs delivered to your bucket (or if all other services logging to the
+bucket support using bucket policy instead of ACLs), you can set object ownership
+to [bucket owner enforced](#bucket-owner-enforced-recommended), as is recommended.
+
+```ts
+const accessLogsBucket = new s3.Bucket(this, 'AccessLogsBucket', {
+  objectOwnership: s3.ObjectOwnership.BUCKET_OWNER_ENFORCED,
+});
+
+const bucket = new s3.Bucket(this, 'MyBucket', {
+  serverAccessLogsBucket: accessLogsBucket,
+  serverAccessLogsPrefix: 'logs',
+});
+```
 
 ## S3 Inventory
 
@@ -485,14 +503,23 @@ new s3.Bucket(this, 'MyBucket', {
 
 ### Bucket owner enforced (recommended)
 
-ACLs are disabled, and the bucket owner automatically owns and has full control over every object in the bucket. ACLs no longer affect permissions to data in the S3 bucket. The bucket uses policies to define access control.
+ACLs are disabled, and the bucket owner automatically owns and has full control
+over every object in the bucket. ACLs no longer affect permissions to data in the
+S3 bucket. The bucket uses policies to define access control.
 
 ```ts
 new s3.Bucket(this, 'MyBucket', {
   objectOwnership: s3.ObjectOwnership.BUCKET_OWNER_ENFORCED,
 });
 ```
 
+Some services may not not support log delivery to buckets that have object ownership
+set to bucket owner enforced, such as
+[S3 buckets using ACLs](#allowing-access-log-delivery-using-a-bucket-policy-recommended)
+or [CloudFront Distributions][CloudFront S3 Bucket].
+
+[CloudFront S3 Bucket]: https://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/AccessLogs.html#AccessLogsBucketAndFileOwnership
+
 ## Bucket deletion
 
 When a bucket is removed from a stack (or the stack is deleted), the S3

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

@@ -1832,7 +1832,9 @@ export class Bucket extends BucketBase {
     }
 
     if (props.serverAccessLogsBucket instanceof Bucket) {
-      props.serverAccessLogsBucket.allowLogDelivery();
+      props.serverAccessLogsBucket.allowLogDelivery(this, props.serverAccessLogsPrefix);
+    } else if (props.serverAccessLogsPrefix) {
+      this.allowLogDelivery(this, props.serverAccessLogsPrefix);
     }
 
     for (const inventory of props.inventories ?? []) {
@@ -2189,17 +2191,35 @@ export class Bucket extends BucketBase {
   }
 
   /**
-   * Allows the LogDelivery group to write, fails if ACL was set differently.
+   * Allows Log Delivery to the S3 bucket, using a Bucket Policy if the relevant feature
+   * flag is enabled, otherwise the canned ACL is used.
+   *
+   * If log delivery is to be allowed using the ACL and an ACL has already been set, this fails.
    *
    * @see
-   * https://docs.aws.amazon.com/AmazonS3/latest/dev/acl-overview.html#canned-acl
-   */
-  private allowLogDelivery() {
-    if (this.accessControl && this.accessControl !== BucketAccessControl.LOG_DELIVERY_WRITE) {
+   * https://docs.aws.amazon.com/AmazonS3/latest/userguide/enable-server-access-logging.html
+   */
+  private allowLogDelivery(from: IBucket, prefix?: string) {
+    if (FeatureFlags.of(this).isEnabled(cxapi.S3_SERVER_ACCESS_LOGS_USE_BUCKET_POLICY)) {
+      this.addToResourcePolicy(new iam.PolicyStatement({
+        effect: iam.Effect.ALLOW,
+        principals: [new iam.ServicePrincipal('logging.s3.amazonaws.com')],
+        actions: ['s3:PutObject'],
+        resources: [this.arnForObjects(prefix ? `${prefix}*`: '*')],
+        conditions: {
+          ArnLike: {
+            'aws:SourceArn': from.bucketArn,
+          },
+          StringEquals: {
+            'aws:SourceAccount': from.env.account,
+          },
+        },
+      }));
+    } else if (this.accessControl && this.accessControl !== BucketAccessControl.LOG_DELIVERY_WRITE) {
       throw new Error("Cannot enable log delivery to this bucket because the bucket's ACL has been set and can't be changed");
+    } else {
+      this.accessControl = BucketAccessControl.LOG_DELIVERY_WRITE;
     }
-
-    this.accessControl = BucketAccessControl.LOG_DELIVERY_WRITE;
   }
 
   private parseInventoryConfiguration(): CfnBucket.InventoryConfigurationProperty[] | undefined {

packages/@aws-cdk/aws-s3/test/bucket.test.ts

@@ -2205,6 +2205,71 @@ describe('bucket', () => {
     ).toThrow(/Cannot enable log delivery to this bucket because the bucket's ACL has been set and can't be changed/);
   });
 
+  test('Bucket Allow Log delivery should use the recommended policy when flag enabled', () => {
+    // GIVEN
+    const stack = new cdk.Stack();
+    stack.node.setContext('@aws-cdk/aws-s3:serverAccessLogsUseBucketPolicy', true);
+
+    // WHEN
+    const bucket = new s3.Bucket(stack, 'TestBucket', { serverAccessLogsPrefix: 'test' });
+
+    // THEN
+    const template = Template.fromStack(stack);
+    template.hasResourceProperties('AWS::S3::Bucket', {
+      AccessControl: Match.absent(),
+    });
+    template.hasResourceProperties('AWS::S3::BucketPolicy', {
+      Bucket: stack.resolve(bucket.bucketName),
+      PolicyDocument: Match.objectLike({
+        Statement: Match.arrayWith([Match.objectLike({
+          Effect: 'Allow',
+          Principal: { Service: 'logging.s3.amazonaws.com' },
+          Action: 's3:PutObject',
+          Resource: stack.resolve(`${bucket.bucketArn}/test*`),
+          Condition: {
+            ArnLike: {
+              'aws:SourceArn': stack.resolve(bucket.bucketArn),
+            },
+            StringEquals: {
+              'aws:SourceAccount': { 'Ref': 'AWS::AccountId' },
+            },
+          },
+        })]),
+      }),
+    });
+  });
+
+  test('Log Delivery bucket policy should properly set source bucket ARN/Account', () => {
+    // GIVEN
+    const stack = new cdk.Stack();
+    stack.node.setContext('@aws-cdk/aws-s3:serverAccessLogsUseBucketPolicy', true);
+
+    // WHEN
+    const targetBucket = new s3.Bucket(stack, 'TargetBucket');
+    const sourceBucket = new s3.Bucket(stack, 'SourceBucket', { serverAccessLogsBucket: targetBucket });
+
+    // THEN
+    Template.fromStack(stack).hasResourceProperties('AWS::S3::BucketPolicy', {
+      Bucket: stack.resolve(targetBucket.bucketName),
+      PolicyDocument: Match.objectLike({
+        Statement: Match.arrayWith([Match.objectLike({
+          Effect: 'Allow',
+          Principal: { Service: 'logging.s3.amazonaws.com' },
+          Action: 's3:PutObject',
+          Resource: stack.resolve(`${targetBucket.bucketArn}/*`),
+          Condition: {
+            ArnLike: {
+              'aws:SourceArn': stack.resolve(sourceBucket.bucketArn),
+            },
+            StringEquals: {
+              'aws:SourceAccount': stack.resolve(sourceBucket.env.account),
+            },
+          },
+        })]),
+      }),
+    });
+  });
+
   test('Defaults for an inventory bucket', () => {
     // Given
     const stack = new cdk.Stack();

packages/@aws-cdk/aws-s3/test/integ.bucket.server-access-logs.js.snapshot/aws-cdk-s3-access-logs.assets.json

@@ -1,15 +1,15 @@
 {
-  "version": "20.0.0",
+  "version": "22.0.0",
   "files": {
-    "8091e7796cbf83b5d8109a10ea23dfbe9ac04ad84434da5dd183b89eb89e351b": {
+    "f58a2b25314952a1a5a6b42c6b9092caf2710430af09ba4f5d807e60f2fd3542": {
       "source": {
         "path": "aws-cdk-s3-access-logs.template.json",
         "packaging": "file"
       },
       "destinations": {
         "current_account-current_region": {
           "bucketName": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}",
-          "objectKey": "8091e7796cbf83b5d8109a10ea23dfbe9ac04ad84434da5dd183b89eb89e351b.json",
+          "objectKey": "f58a2b25314952a1a5a6b42c6b9092caf2710430af09ba4f5d807e60f2fd3542.json",
           "assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-${AWS::Region}"
         }
       }

packages/@aws-cdk/aws-s3/test/integ.bucket.server-access-logs.js.snapshot/aws-cdk-s3-access-logs.template.json

@@ -2,12 +2,58 @@
  "Resources": {
   "MyAccessLogsBucketF7FE6635": {
    "Type": "AWS::S3::Bucket",
-   "Properties": {
-    "AccessControl": "LogDeliveryWrite"
-   },
    "UpdateReplacePolicy": "Delete",
    "DeletionPolicy": "Delete"
   },
+  "MyAccessLogsBucketPolicyEA9AB063": {
+   "Type": "AWS::S3::BucketPolicy",
+   "Properties": {
+    "Bucket": {
+     "Ref": "MyAccessLogsBucketF7FE6635"
+    },
+    "PolicyDocument": {
+     "Statement": [
+      {
+       "Action": "s3:PutObject",
+       "Condition": {
+        "ArnLike": {
+         "aws:SourceArn": {
+          "Fn::GetAtt": [
+           "MyBucketF68F3FF0",
+           "Arn"
+          ]
+         }
+        },
+        "StringEquals": {
+         "aws:SourceAccount": {
+          "Ref": "AWS::AccountId"
+         }
+        }
+       },
+       "Effect": "Allow",
+       "Principal": {
+        "Service": "logging.s3.amazonaws.com"
+       },
+       "Resource": {
+        "Fn::Join": [
+         "",
+         [
+          {
+           "Fn::GetAtt": [
+            "MyAccessLogsBucketF7FE6635",
+            "Arn"
+           ]
+          },
+          "/example*"
+         ]
+        ]
+       }
+      }
+     ],
+     "Version": "2012-10-17"
+    }
+   }
+  },
   "MyBucketF68F3FF0": {
    "Type": "AWS::S3::Bucket",
    "Properties": {

packages/@aws-cdk/aws-s3/test/integ.bucket.server-access-logs.js.snapshot/cdk.out

@@ -1 +1 @@
-{"version":"20.0.0"}
+{"version":"22.0.0"}

packages/@aws-cdk/aws-s3/test/integ.bucket.server-access-logs.js.snapshot/integ.json

@@ -1,5 +1,5 @@
 {
-  "version": "20.0.0",
+  "version": "22.0.0",
   "testCases": {
     "integ.bucket.server-access-logs": {
       "stacks": [

packages/@aws-cdk/aws-s3/test/integ.bucket.server-access-logs.js.snapshot/manifest.json

@@ -1,12 +1,6 @@
 {
-  "version": "20.0.0",
+  "version": "22.0.0",
   "artifacts": {
-    "Tree": {
-      "type": "cdk:tree",
-      "properties": {
-        "file": "tree.json"
-      }
-    },
     "aws-cdk-s3-access-logs.assets": {
       "type": "cdk:asset-manifest",
       "properties": {
@@ -23,7 +17,7 @@
         "validateOnSynth": false,
         "assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-deploy-role-${AWS::AccountId}-${AWS::Region}",
         "cloudFormationExecutionRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-cfn-exec-role-${AWS::AccountId}-${AWS::Region}",
-        "stackTemplateAssetObjectUrl": "s3://cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}/8091e7796cbf83b5d8109a10ea23dfbe9ac04ad84434da5dd183b89eb89e351b.json",
+        "stackTemplateAssetObjectUrl": "s3://cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}/f58a2b25314952a1a5a6b42c6b9092caf2710430af09ba4f5d807e60f2fd3542.json",
         "requiresBootstrapStackVersion": 6,
         "bootstrapStackVersionSsmParameter": "/cdk-bootstrap/hnb659fds/version",
         "additionalDependencies": [
@@ -45,6 +39,12 @@
             "data": "MyAccessLogsBucketF7FE6635"
           }
         ],
+        "/aws-cdk-s3-access-logs/MyAccessLogsBucket/Policy/Resource": [
+          {
+            "type": "aws:cdk:logicalId",
+            "data": "MyAccessLogsBucketPolicyEA9AB063"
+          }
+        ],
         "/aws-cdk-s3-access-logs/MyBucket/Resource": [
           {
             "type": "aws:cdk:logicalId",
@@ -65,6 +65,12 @@
         ]
       },
       "displayName": "aws-cdk-s3-access-logs"
+    },
+    "Tree": {
+      "type": "cdk:tree",
+      "properties": {
+        "file": "tree.json"
+      }
     }
   }
 }