fix(aws-s3): log delivery may be incorrectly configured when target bucket is imported

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": {}
+}