fix(cloudtrail): do not attach s3 bucket permission when orgId is not set for organization trail by sarisia · Pull Request #30778 · aws/aws-cdk

sarisia · 2024-07-07T18:29:35Z

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 · 2024-07-07T18:32:00Z

Exemption Request: deploying this requires AWS organization management account

sarisia · 2024-10-15T12:25:54Z

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

GavinZZ · 2024-12-12T23:50:53Z

@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 · 2024-12-13T16:49:01Z

@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 · 2024-12-13T17:06:20Z

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 · 2025-01-13T21:49:54Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.41%. Comparing base (64b865b) to head (d7574c8).
Report is 360 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 · 2025-01-13T22:01:49Z

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 · 2025-01-16T22:13:53Z

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

aws-cdk-automation · 2025-02-02T00:11:46Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2025-02-03T00:11:12Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2025-02-10T00:11:21Z

This PR has been in the MERGE CONFLICTS state for 3 weeks, and looks abandoned. Note that PRs with failing linting check or builds are not reviewed, please ensure your build is passing

To prevent automatic closure:

Resume work on the PR
OR request an exemption by adding a comment containing 'Exemption Request' with justification e.x "Exemption Request: "
OR request clarification by adding a comment containing 'Clarification Request' with a question e.x "Clarification Request: "

This PR will automatically close in 7 days if no action is taken.

GavinZZ · 2025-02-13T19:39:22Z

@sarisia I see that this PR will be closed soon. Do you plan to work on this same PR or open a new PR for the changes we discussed?

sarisia · 2025-02-14T07:16:26Z

@GavinZZ sorry for delayed progress, been busy recentry. I'm planning to work on this PR.

mergify · 2025-10-30T15:53:37Z

gasolima · 2025-12-12T11:27:46Z

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

-            's3:x-amz-acl': 'bucket-owner-full-control',
-            'aws:SourceArn': `arn:${this.stack.partition}:cloudtrail:${this.s3bucket.stack.region}:${this.s3bucket.stack.account}:trail/${props.trailName}`,
+      if (props.orgId === undefined) {
+        Annotations.of(this).addWarningV2('@aws-cdk/aws-cloudtrail:missingOrgIdForOrganizationTrail', 'Skipped attaching a policy to the bucket which allows organization trail to write because of missing orgId. Consider specifying orgId to add missing permissions');


Can you explain to me the reason for choosing it warning vs throwing an error?
Is there any such use case that the user intentionally want to have isOrganizationTrail true but not send orgId or trailName?

For backward compatibility and avoiding breaking changes. See #30778 (comment)

My question was a bit unclear, is there a real use case for having isOrganizationTrail and not send orgId?
According to @Abogical for ppl who want to have the policy by themselves.
So if that's the argument is valid i'm happy with it.

but for other issues if we're going to break ppl cause we were not doing what they're expecting and now we're doing what they're expecting but failing we're happy to go this way, since it's better than failing silently.

I don't think there's a real use case too so I'happy to change these warnings to throwing error if introducing breaking changes is acceptable here!

gasolima · 2025-12-12T13:00:15Z

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

-            'aws:SourceArn': `arn:${this.stack.partition}:cloudtrail:${this.s3bucket.stack.region}:${this.s3bucket.stack.account}:trail/${props.trailName}`,
+      if (props.orgId === undefined) {
+        Annotations.of(this).addWarningV2('@aws-cdk/aws-cloudtrail:missingOrgIdForOrganizationTrail', 'Skipped attaching a policy to the bucket which allows organization trail to write because of missing orgId. Consider specifying orgId to add missing permissions');
+      } else if (props.trailName === undefined) {


is there any reason for this if condition?
I believe in L301
You can use something like

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

The trail name is not necessarily equal to the physical name. How the trail name is generated if its not defined can be different than the physicalName.

I can expect the majority users are under this category of not defining trailName.
I believe we can get the generated trailName from the arn, then we don't have this if condition at all.

But with introducing this PR we're fixing the bug, but introducing another bug

Referencing trail's cloudformation attributes in bucket policy makes circular dependency which cannot be deployed so we need hard-coded value here.

very good point thanks!

Can you write a code comment in line 301 stating this fact? cause otherwise ppl will try to do what i'm asking in the future

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

Pull request has been modified.

mergify · 2025-12-12T17:59:19Z

mergify · 2025-12-12T17:59:21Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

mergify · 2025-12-12T17:59:24Z

Merge Queue Status

🚫 The pull request has left the queue (rule: default-squash) at a303c01

This pull request spent 5 seconds in the queue, with no time running CI.

Reason

The pull request can't be updated

For security reasons, Mergify can't update this pull request. Try updating locally.
GitHub response: refusing to allow a GitHub App to create or update workflow .github/workflows/analytics-metadata-updater.yml without workflows permission

Hint

You should update or rebase your pull request manually. If you do, this pull request will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

Abogical · 2025-12-12T18:10:02Z

@Mergifyio update

mergify · 2025-12-12T18:10:13Z

update

❌ Mergify doesn't have permission to update

Details

For security reasons, Mergify can't update this pull request. Try updating locally.
GitHub response: refusing to allow a GitHub App to create or update workflow .github/workflows/analytics-metadata-updater.yml without workflows permission

github-actions · 2025-12-12T18:12:53Z

	Tests	Passed ❌️	Skipped	Failed
Security Guardian Results

Test	Result
No test annotations available

View Security Guardian Results

github-actions · 2025-12-12T18:12:54Z

	Tests	Passed ❌️	Skipped	Failed
Security Guardian Results with resolved templates

Test	Result
No test annotations available

View Security Guardian Results with resolved templates

mergify · 2025-12-12T18:39:31Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

mergify · 2025-12-12T18:39:41Z

github-actions · 2025-12-12T18:39:46Z

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

aws-cdk-automation requested a review from a team July 7, 2024 18:29

github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2 labels Jul 7, 2024

mergify bot mentioned this pull request Jul 7, 2024

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

Closed

1 task

aws-cdk-automation previously requested changes Jul 7, 2024

View reviewed changes

aws-cdk-automation added pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels Jul 7, 2024

github-actions bot mentioned this pull request Jul 8, 2024

Weekly PR metrics report #30783

Closed

github-actions bot mentioned this pull request Aug 1, 2024

Monthly PR metrics report #30999

Closed

GavinZZ added pr-linter/exempt-readme The PR linter will not require README changes pr-linter/exempt-integ-test The PR linter will not require integ test changes labels Dec 12, 2024

xazhao removed the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Jan 13, 2025

godwingrs22 assigned godwingrs22 and unassigned godwingrs22 Jan 13, 2025

GavinZZ added the pr-linter/do-not-close The PR linter will not close this PR while this label is present label Feb 19, 2025

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Feb 19, 2025

Abogical added the pr/do-not-merge This PR should not be merged at this time. label Oct 30, 2025

Abogical added pr/requires-two-approvers This PR is critical (e.g., security, broadly-impacting) and requires 2 approvers to be merged. and removed pr/do-not-merge This PR should not be merged at this time. labels Oct 30, 2025

gasolima reviewed Dec 12, 2025

View reviewed changes

packages/aws-cdk-lib/aws-cloudtrail/lib/cloudtrail.ts Show resolved Hide resolved

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

13dcdac

gasolima previously approved these changes Dec 12, 2025

View reviewed changes

Abogical approved these changes Dec 12, 2025

View reviewed changes

fix lint error

a303c01

Abogical approved these changes Dec 12, 2025

View reviewed changes

Abogical removed the pr/requires-two-approvers This PR is critical (e.g., security, broadly-impacting) and requires 2 approvers to be merged. label Dec 12, 2025

aws-cdk-automation removed the pr/needs-further-review PR requires additional review from our team specialists due to the scope or complexity of changes. label Dec 12, 2025

Merge branch 'main' into cloudtrail-missing-orgid

31bdb3f

Abogical approved these changes Dec 12, 2025

View reviewed changes

mergify bot merged commit 61ee074 into aws:main Dec 12, 2025
22 of 23 checks passed

github-actions bot locked as resolved and limited conversation to collaborators Dec 12, 2025

Conversation

sarisia commented Jul 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

sarisia commented Jul 7, 2024

Uh oh!

sarisia commented Oct 15, 2024

Uh oh!

GavinZZ commented Dec 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sarisia commented Dec 13, 2024

Uh oh!

sarisia commented Dec 13, 2024

Uh oh!

codecov bot commented Jan 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

aws-cdk-automation commented Jan 13, 2025

AWS CodeBuild CI Report

Uh oh!

GavinZZ commented Jan 16, 2025

Uh oh!

aws-cdk-automation commented Feb 2, 2025

Uh oh!

aws-cdk-automation commented Feb 3, 2025

Uh oh!

aws-cdk-automation commented Feb 10, 2025

Uh oh!

GavinZZ commented Feb 13, 2025

Uh oh!

sarisia commented Feb 14, 2025

Uh oh!

mergify bot commented Oct 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gasolima Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mergify bot commented Dec 12, 2025

Merge Queue Status

Reason

Hint

Uh oh!

mergify bot commented Dec 12, 2025

Uh oh!

mergify bot commented Dec 12, 2025

Merge Queue Status

sarisia commented Jul 7, 2024 •

edited

Loading

GavinZZ commented Dec 12, 2024 •

edited

Loading

codecov bot commented Jan 13, 2025 •

edited

Loading

gasolima Dec 12, 2025 •

edited

Loading