fix(cloudtrail): do not attach s3 bucket permission when orgId is not set for organization trail

[sarisia]

Recreate #30490

Issue # (if applicable)

no open issue

Reason for this change

Organization trail without orgId attaches improper s3 bucket policy
which allows cloudtrail to write to /AWSLogs/undefined/* prefix.

                        {
                            "Action": "s3:PutObject",
                            "Condition": {
                                "StringEquals": {
                                    "s3:x-amz-acl": "bucket-owner-full-control",
                                    "aws:SourceArn": {
                                        "Fn::Join": [
                                            "",
                                            [
                                                "arn:",
                                                {
                                                    "Ref": "AWS::Partition"
                                                },
                                                ":cloudtrail:us-east-1:123456789012:trail/undefined"
                                            ]
                                        ]
                                    }
                                }
                            },
                            "Effect": "Allow",
                            "Principal": {
                                "Service": "cloudtrail.amazonaws.com"
                            },
                            "Resource": {
                                "Fn::Join": [
                                    "",
                                    [
                                        {
                                            "Fn::GetAtt": [
                                                "TrailS30071F172",
                                                "Arn"
                                            ]
                                        },
                                        "/AWSLogs/undefined/*"
                                    ]
                                ]
                            }
                        }

Description of changes

• do not attach s3 bucket policy if isOrganizationTrail is set and orgId is not set
  • generate warning if attaching s3 bucket policy is skipped

Description of how you validated changes

• added unit test

Checklist

• My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

---

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

[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.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

[sarisia]

Exemption Request: deploying this requires AWS organization management account

[sarisia]

could someone review this please? I'm waiting this for 4 months, thanks!

[GavinZZ]

@sarisia Hello, apologize for the extraa long wait time on the PR review. This seems to be an accident when we introduced orgId field and somehow missed the validation. I think this PR makes sense to me, but just curious what's your use case? Are you using organization trail without orgId?

The reason I asked this is because I'm thinking why don't we validate and raise an error if organization trail is used but no orgId is provided.

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

[sarisia]

@GavinZZ Thank you for reviewing!

I think this PR makes sense to me, but just curious what's your use case? Are you using organization trail without orgId?

Actually yes. Back in 2023 there were no orgId prop in cloudtrail.Trail so I have to write the code which manually attaches insufficient permissions, like this:

        # trail
        trail = cloudtrail.Trail(
            self,
            "Trail",
            bucket=bucket,
            is_organization_trail=True
        )

        # add bucket policy
        # https://docs.aws.amazon.com/ja_jp/awscloudtrail/latest/userguide/create-s3-bucket-policy-for-cloudtrail.html
        cloudtrail_principal = iam.ServicePrincipal(
            "cloudtrail.amazonaws.com"
        )
        bucket.add_to_resource_policy(
            iam.PolicyStatement(
                actions=["s3:GetBucketAcl"],
                principals=[cloudtrail_principal],
                resources=[bucket.bucket_arn],
            )
        )
        bucket.add_to_resource_policy(
            iam.PolicyStatement(
                actions=["s3:PutObject"],
                principals=[cloudtrail_principal],
                resources=[
                    bucket.arn_for_objects(f"AWSLogs/{Stack.of(self).account}/*"),
                    bucket.arn_for_objects(
                        f"AWSLogs/{organization_principal.organization_id}/*"
                    ),
                ],
                conditions={
                    "StringEquals": {"s3:x-amz-acl": "bucket-owner-full-control"}
                },
            )
        )

Recently I upgraded CDK and found there's a diff with new weird permission attached to the bucket, so I'm trying to get rid of them without breaking deploys and forcing code changes.

[sarisia]

Actually there's another issue which props.trailName in generated policy becomes undefined too.

Initially I tried to fix them in separated PR (#30489) which makes props.trailName required but now I think it's better to fix this PR to check both props.orgId and props.trailName without making them required. WDYT?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.41%. Comparing base (64b865b) to head (d7574c8).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #30778   +/-   ##
=======================================
  Coverage   81.41%   81.41%           
=======================================
  Files         223      223           
  Lines       13721    13721           
  Branches     2416     2416           
=======================================
  Hits        11171    11171           
  Misses       2271     2271           
  Partials      279      279           

Flag	Coverage Δ
suite.unit	81.41% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	80.75% <ø> (ø)
packages/aws-cdk-lib/core	82.10% <ø> (ø)

[aws-cdk-automation]

AWS CodeBuild CI Report

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

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

[GavinZZ]

@sarisia sorry for the delayed response. I think that makes sense. Let's fix both issue here.