fix(aws-s3): log delivery may be incorrectly configured when target b…

…ucket is imported (#23552)

This resolves issues with log delivery for situations where the target bucket for S3 Access Logs is in another stack. This situation was previously missing from unit and integration tests. This adds additional checks to make sure that this behavior works. There are two primary issues here:

1. Regardless of `@aws-cdk/aws-s3:serverAccessLogsUseBucketPolicy`, if the target bucket was imported (not a concrete `Bucket`), the grant would be applied to the _source_ bucket for logs improperly. This resulted in the ACL or Bucket Policy being added to the wrong stack.
2. When `@aws-cdk/aws-s3:serverAccessLogsUseBucketPolicy` was _enabled_ the created bucket policy resulted in a cyclical dependency because of the conditions; these conditions are not necessary for successful delivery. This omits them unless the bucket is concrete and in the same stack.

Unit tests and integration tests now cover importing a bucket between stacks.

Closes: #23547
Closes: #23588

----

### 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

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

@@ -19,6 +19,7 @@ import {
   Tags,
   Token,
   Tokenization,
+  Annotations,
 } from '@aws-cdk/core';
 import { CfnReference } from '@aws-cdk/core/lib/private/cfn-reference';
 import * as cxapi from '@aws-cdk/cx-api';
@@ -1833,8 +1834,19 @@ export class Bucket extends BucketBase {
 
     if (props.serverAccessLogsBucket instanceof Bucket) {
       props.serverAccessLogsBucket.allowLogDelivery(this, props.serverAccessLogsPrefix);
-    } else if (props.serverAccessLogsPrefix) {
+    // It is possible that `serverAccessLogsBucket` was specified but is some other `IBucket`
+    // that cannot have the ACLs or bucket policy applied. In that scenario, we should only
+    // setup log delivery permissions to `this` if a bucket was not specified at all, as documented.
+    // For example, we should not allow log delivery to `this` if given an imported bucket or
+    // another situation that causes `instanceof` to fail
+    } else if (!props.serverAccessLogsBucket && props.serverAccessLogsPrefix) {
       this.allowLogDelivery(this, props.serverAccessLogsPrefix);
+    } else if (props.serverAccessLogsBucket) {
+      // A `serverAccessLogsBucket` was provided but it is not a concrete `Bucket` and it
+      // may not be possible to configure the ACLs or bucket policy as required.
+      Annotations.of(this).addWarning(
+        `Unable to add necessary logging permissions to imported target bucket: ${props.serverAccessLogsBucket}`,
+      );
     }
 
     for (const inventory of props.inventories ?? []) {
@@ -2201,19 +2213,26 @@ export class Bucket extends BucketBase {
    */
   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: {
+      let conditions = undefined;
+      // The conditions for the bucket policy can be applied only when the buckets are in
+      // the same stack and a concrete bucket instance (not imported). Otherwise, the
+      // necessary imports may result in a cyclic dependency between the stacks.
+      if (from instanceof Bucket && Stack.of(this) === Stack.of(from)) {
+        conditions = {
           ArnLike: {
             'aws:SourceArn': from.bucketArn,
           },
           StringEquals: {
             'aws:SourceAccount': from.env.account,
           },
-        },
+        };
+      }
+      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: conditions,
       }));
     } 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");

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

@@ -1,5 +1,5 @@
 import { EOL } from 'os';
-import { Match, Template } from '@aws-cdk/assertions';
+import { Annotations, Match, Template } from '@aws-cdk/assertions';
 import * as iam from '@aws-cdk/aws-iam';
 import * as kms from '@aws-cdk/aws-kms';
 import * as cdk from '@aws-cdk/core';
@@ -2205,6 +2205,31 @@ 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 skips setting up access log ACL but configures delivery for an imported target bucket', () => {
+    // GIVEN
+    const stack = new cdk.Stack();
+
+    // WHEN
+    const accessLogBucket = s3.Bucket.fromBucketName(stack, 'TargetBucket', 'target-logs-bucket');
+    new s3.Bucket(stack, 'TestBucket', {
+      serverAccessLogsBucket: accessLogBucket,
+      serverAccessLogsPrefix: 'test/',
+    });
+
+    // THEN
+    const template = Template.fromStack(stack);
+    template.hasResourceProperties('AWS::S3::Bucket', {
+      LoggingConfiguration: {
+        DestinationBucketName: stack.resolve(accessLogBucket.bucketName),
+        LogFilePrefix: 'test/',
+      },
+    });
+    template.allResourcesProperties('AWS::S3::Bucket', {
+      AccessControl: Match.absent(),
+    });
+    Annotations.fromStack(stack).hasWarning('*', Match.stringLikeRegexp('Unable to add necessary logging permissions to imported target bucket'));
+  });
+
   test('Bucket Allow Log delivery should use the recommended policy when flag enabled', () => {
     // GIVEN
     const stack = new cdk.Stack();

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

@@ -0,0 +1,19 @@
+{
+  "version": "22.0.0",
+  "files": {
+    "21fbb51d7b23f6a6c262b46a9caee79d744a3ac019fd45422d988b96d44b2a22": {
+      "source": {
+        "path": "ServerAccessLogsImportTestDefaultTestDeployAssert076DA7F5.template.json",
+        "packaging": "file"
+      },
+      "destinations": {
+        "current_account-current_region": {
+          "bucketName": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}",
+          "objectKey": "21fbb51d7b23f6a6c262b46a9caee79d744a3ac019fd45422d988b96d44b2a22.json",
+          "assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-${AWS::Region}"
+        }
+      }
+    }
+  },
+  "dockerImages": {}
+}

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

@@ -0,0 +1,36 @@
+{
+ "Parameters": {
+  "BootstrapVersion": {
+   "Type": "AWS::SSM::Parameter::Value<String>",
+   "Default": "/cdk-bootstrap/hnb659fds/version",
+   "Description": "Version of the CDK Bootstrap resources in this environment, automatically retrieved from SSM Parameter Store. [cdk:skip]"
+  }
+ },
+ "Rules": {
+  "CheckBootstrapVersion": {
+   "Assertions": [
+    {
+     "Assert": {
+      "Fn::Not": [
+       {
+        "Fn::Contains": [
+         [
+          "1",
+          "2",
+          "3",
+          "4",
+          "5"
+         ],
+         {
+          "Ref": "BootstrapVersion"
+         }
+        ]
+       }
+      ]
+     },
+     "AssertDescription": "CDK bootstrap stack version 6 required. Please run 'cdk bootstrap' with a recent version of the CDK CLI."
+    }
+   ]
+  }
+ }
+}

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

@@ -0,0 +1,32 @@
+{
+  "version": "22.0.0",
+  "files": {
+    "33e2651435a0d472a75c1e033c9832b21321d9e56711926b04c5705e5f63874c": {
+      "source": {
+        "path": "asset.33e2651435a0d472a75c1e033c9832b21321d9e56711926b04c5705e5f63874c",
+        "packaging": "zip"
+      },
+      "destinations": {
+        "current_account-current_region": {
+          "bucketName": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}",
+          "objectKey": "33e2651435a0d472a75c1e033c9832b21321d9e56711926b04c5705e5f63874c.zip",
+          "assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-${AWS::Region}"
+        }
+      }
+    },
+    "f7cd2df39343e955bf8713c630fe64d9469d0fb8da00f7858ca7874cf6aac7de": {
+      "source": {
+        "path": "aws-cdk-s3-access-logs-delivery.template.json",
+        "packaging": "file"
+      },
+      "destinations": {
+        "current_account-current_region": {
+          "bucketName": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}",
+          "objectKey": "f7cd2df39343e955bf8713c630fe64d9469d0fb8da00f7858ca7874cf6aac7de.json",
+          "assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-${AWS::Region}"
+        }
+      }
+    }
+  },
+  "dockerImages": {}
+}