(aws_cloudtrail >> Trail): (Invalid request provided: Incorrect S3 bucket policy is detected for bucket)

[ayrawat17]

Describe the bug

Unable to create Organization Trail in management account when using the i.e as per the doc :
++++++++++++++++++++++++++++++++++++++++
new cloudtrail.Trail(this, 'OrganizationTrail', {
isOrganizationTrail: true,
orgId: "o-xxxxxxxxx",
});
++++++++++++++++++++++++++++++++++++++++
This fail with the "Invalid request provided: Incorrect S3 bucket policy is detected for bucket" every-time.

Only when we add the trailName property explicitly to some string name it is then we are somehow able to mitigate the "Invalid request provided: Incorrect S3 bucket policy is detected for bucket"
++++++++++++++++++++++++++++++++++++++++
new cloudtrail.Trail(this, 'OrganizationTrail', {
isOrganizationTrail: true,
orgId: "o-xxxxxxxxx",
trailName: "trailname123"
});
++++++++++++++++++++++++++++++++++++++++

Regression Issue

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

Last Known Working CDK Version

2.157.0

Expected Behavior

To work without explicitly passing the trailName: property .

Current Behavior

CreateTrial API fails with the "Invalid request provided: Incorrect S3 bucket policy is detected for bucket" when using the following code to create Organization Trail:
++++++++++++++++++++++++++++++++++++++++
new cloudtrail.Trail(this, 'OrganizationTrail', {
isOrganizationTrail: true,
orgId: "o-xxxxxxxxx",
});
++++++++++++++++++++++++++++++++++++++++

Reproduction Steps

Use :
++++++++++++++++++++++++++++++++++++++++
new cloudtrail.Trail(this, 'OrganizationTrail', {
isOrganizationTrail: true,
orgId: "o-xxxxxxxxx",
});
++++++++++++++++++++++++++++++++++++++++

Possible Solution

To add trailName property explicitly

Additional Information/Context

NA

CDK CLI Version

2.157.0

Framework Version

No response

Node.js Version

v16.14.2

OS

MAC

Language

TypeScript

Language Version

No response

Other information

No response

[khushail]

Hi @ayrawat17 , thanks for reaching out.

The issue is reproducible. Sharing the observation -

    const cldtrail = new cloudtrail.Trail(this, 'cloudtrail001', {
      trailName: 'cloudtrail001',
      isOrganizationTrail: true,
    });

this is the snippet of synthesized bucket policy PutObject created -

      {
       "Action": "s3:PutObject",
       "Condition": {
        "StringEquals": {
         "s3:x-amz-acl": "bucket-owner-full-control",
         "aws:SourceArn": {
          "Fn::Join": [
           "",
           [
            "arn:",
            {
             "Ref": "AWS::Partition"
            },
            ":cloudtrail:",
            {
             "Ref": "AWS::Region"
            },
            ":",
            {
             "Ref": "AWS::AccountId"
            },
            ":trail/undefined"
           ]
          ]
         }
        }
       },

where ":trail/undefined" is worth concerning.

Ideally it should not have been undefined and expected to be like this. So when you add trailName , it gets updated to -

       "Action": "s3:PutObject",
       "Condition": {
        "StringEquals": {
         "s3:x-amz-acl": "bucket-owner-full-control",
         "aws:SourceArn": {
          "Fn::Join": [
           "",
           [
            "arn:",
            {
             "Ref": "AWS::Partition"
            },
            ":cloudtrail:",
            {
             "Ref": "AWS::Region"
            },
            ":",
            {
             "Ref": "AWS::AccountId"
            },
            ":trail/cloudtrail001"
           ]
          ]

Now according to CDK, this trailName should be generated by AWS Cloudformation but is left blank and unchecked

https://github.com/aws/aws-cdk/blob/95c49abdfa4ad77a0c0fcb82a230778dcc2ea59a/packages/aws-cdk-lib/aws-cloudtrail/lib/cloudtrail.ts#L100C1-L105C31

  /**
   * The name of the trail. We recommend customers do not set an explicit name.
   *
   * @default - AWS CloudFormation generated name.
   */
  readonly trailName?: string;

Referring further, it looks like the trailName is passed onto Bucket Policy which seems to be leading to error when left blank -

https://github.com/aws/aws-cdk/blob/95c49abdfa4ad77a0c0fcb82a230778dcc2ea59a/packages/aws-cdk-lib/aws-cloudtrail/lib/cloudtrail.ts#L278C1-L287C6

aws-cdk/packages/aws-cdk-lib/aws-cloudtrail/lib/cloudtrail.ts

Line 283 in 95c49ab

'aws:SourceArn': `arn:${this.stack.partition}:cloudtrail:${this.s3bucket.stack.region}:${this.s3bucket.stack.account}:trail/${props.trailName}`,

So yes, this is pretty much an issue when Trailname is left blank.
Thanks for reporting this. I am marking this as P3 as it has a workaround. Marking it as such means it won't be immediately addressed by the team but its on team's radar. Anyone from the community or team is welcome to work on it.

[badmintoncryer]

It seems that simply using CfnTrail.attrArn would suffice, but since the CfnTrail itself already has a dependency on the S3 Bucket Policy, it results in a circular reference.

 const trail = new CfnTrail(this, 'Resource', {...});

    if (props.isOrganizationTrail) {
      this.s3bucket.addToResourcePolicy(new iam.PolicyStatement({
        resources: [this.s3bucket.arnForObjects(
          `AWSLogs/${props.orgId}/*`,
        )],
        actions: ['s3:PutObject'],
        principals: [cloudTrailPrincipal],
        conditions: {
          StringEquals: {
            's3:x-amz-acl': 'bucket-owner-full-control',
            'aws:SourceArn': trail.attrArn,  // updated
          },
        },
      }));
    }

    // This dependency has already existed.
    if (this.s3bucket.policy) {
      trail.node.addDependency(this.s3bucket.policy);
    }

Result:

Template is undeployable, these resources have a dependency cycle: OrganizationTrailS3Policy140E0681 -> OrganizationTrail7F291DBA -> OrganizationTrailS3Policy140E0681:

{
      "OrganizationTrailS3Policy140E0681": {
        "Type": "AWS::S3::BucketPolicy",
        "Properties": {
          "Bucket": {
            "Ref": "OrganizationTrailS39966E64E"
          },
          "PolicyDocument": {
            "Statement": [
...
              {
                "Action": "s3:PutObject",
                "Condition": {
                  "StringEquals": {
                    "s3:x-amz-acl": "bucket-owner-full-control",
                    "aws:SourceArn": {
                      "Fn::GetAtt": [
                        "OrganizationTrail7F291DBA",
                        "Arn"
                      ]
                    }
                  }
                },
                "Effect": "Allow",
                "Principal": {
                  "Service": "cloudtrail.amazonaws.com"
                },
                "Resource": {
                  "Fn::Join": [
                    "",
                    [
                      {
                        "Fn::GetAtt": [
                          "OrganizationTrailS39966E64E",
                          "Arn"
                        ]
                      },
                      "/AWSLogs/undefined/*"
                    ]
                  ]
                }
              }
            ],
            "Version": "2012-10-17"
          }
        }
      },
      "OrganizationTrail7F291DBA": {
        "Type": "AWS::CloudTrail::Trail",
        "Properties": {
          "EnableLogFileValidation": true,
          "EventSelectors": [],
          "IncludeGlobalServiceEvents": true,
          "IsLogging": true,
          "IsMultiRegionTrail": true,
          "IsOrganizationTrail": true,
          "S3BucketName": {
            "Ref": "OrganizationTrailS39966E64E"
          }
        },
        "DependsOn": [
          "OrganizationTrailS3Policy140E0681"
        ]
      }
    }

Therefore, it seems challenging to create a correct Source Arn that includes the TrailName when props.trailName is undefined.