fix(cloudtrail): isOrganizationTrail attaches insufficient permissions to bucket #29242

paulhcsun · 2024-02-24T00:56:31Z

Issue #22267 (if applicable)

Reason for this change

Setting the isOrganizationTrail property to true attaches insufficient permissions to the bucket thus failing to deploy the Trail with:

Invalid request provided: Incorrect S3 bucket policy is detected for bucket: ...

Description of changes

This PR adds a new property orgId to TrailProps that will be used to attach the missing permission

"Action": "s3:PutObject",
"Resource": "arn:aws:s3:::myOrganizationBucket/AWSLogs/o-organizationID/*",
"Condition": {
    "StringEquals": {
          "s3:x-amz-acl": "bucket-owner-full-control",
          "aws:SourceArn": "arn:aws:cloudtrail:region:managementAccountID:trail/trailName"
}

when isOrganizationTrail: true.

Description of how you validated changes

Validated locally that I can deploy when Trail.isOrganizationTrail: true with a valid orgId + added unit test case.

I can't think of a way to test this with an integ test as it requires a valid orgId to deploy but any suggestions are welcome.

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

…s to bucket

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.

paulhcsun · 2024-02-24T01:00:16Z

Exemption Request

I can't think of a way to test this with an integ test as it requires a valid orgId to deploy but any suggestions are welcome. I have validated locally that I can deploy when Trail.isOrganizationalTrail: true with a valid orgId

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

paulhcsun · 2024-02-26T17:41:35Z

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

@@ -262,6 +270,22 @@ export class Trail extends Resource {
      },
    }));

+    if (props.isOrganizationTrail) {


Could also add a warning in the case that isOrganizationTrai: true and orgId is not provided. Probably not an error even though it would fail to deploy since some users may have manually added the missing policy already and won't need to use the new orgId prop.

Warning could be something like this but any alternative suggestions are welcome:
Organization Trails require passing in an organization id using the orgId property to attach a missing policy.

GavinZZ

Some questions

GavinZZ · 2024-02-28T14:39:47Z

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

@@ -128,6 +128,14 @@ export interface TrailProps {
   */
  readonly isOrganizationTrail?: boolean;

+  /** The orgId.


Why does it call orgId? Reaching this link, it seems to be The name of the folder where the log files are stored, including the bucket name, a prefix (if you specified one), and your AWS account ID. So more like account id?

The missing permission is specified under this section for organization trails. Specifically the third policy in that section which requires the o-organizationID:

{ "Sid": "AWSCloudTrailOrganizationWrite20150319", "Effect": "Allow", "Principal": { "Service": [ "cloudtrail.amazonaws.com" ] }, "Action": "s3:PutObject", "Resource": "arn:aws:s3:::myOrganizationBucket/AWSLogs/o-organizationID/*", "Condition": { "StringEquals": { "s3:x-amz-acl": "bucket-owner-full-control", "aws:SourceArn": "arn:aws:cloudtrail:region:managementAccountID:trail/trailName" } } }

Apologies, I just realized the link to the exact code block just redirects you to the top of the page which is a bit misleading.

This makes a lot more sense! Thanks!

GavinZZ · 2024-02-28T14:44:51Z

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

@@ -262,6 +270,22 @@ export class Trail extends Resource {
      },
    }));

+    if (props.isOrganizationTrail) {
+      this.s3bucket.addToResourcePolicy(new iam.PolicyStatement({
+        resources: [this.s3bucket.arnForObjects(


In the doc, it mentions "arn:aws:s3:::myBucketName/[optionalPrefix]/AWSLogs/myAccountID/*". I haven't looked deep into it, but is the optional prefix part of the bucket and it would be handled by this.s3bucket.arnForObjects?

It would be this value for the resources: "arn:aws:s3:::myOrganizationBucket/AWSLogs/o-organizationID/*" where arn:aws:s3:::myOrganizationBucket/ is handled by this.s3bucket.arnForObjects. But for the case you're talking about it would not handle it, the optional prefix part is passed in from props:

this.s3bucket.addToResourcePolicy(new iam.PolicyStatement({ resources: [this.s3bucket.arnForObjects( `${props.s3KeyPrefix ? `${props.s3KeyPrefix}/` : ''}AWSLogs/${Stack.of(this).account}/*`, )],

GavinZZ · 2024-02-28T14:45:49Z

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

+    if (props.isOrganizationTrail) {
+      this.s3bucket.addToResourcePolicy(new iam.PolicyStatement({
+        resources: [this.s3bucket.arnForObjects(
+          `AWSLogs/${props.orgId}/*`,


Looking at the below snippet, wouldn't the orgId be this.s3bucket.stack.account?

Resource": "arn:aws:s3:::myBucketName/[optionalPrefix]/AWSLogs/myAccountID/*", "Condition": { "StringEquals": { "s3:x-amz-acl": "bucket-owner-full-control", "aws:SourceArn": "arn:aws:cloudtrail:region:myAccountID:trail/trailName" } }

That's a different permission from that one we are adding here:

"Resource": "arn:aws:s3:::myOrganizationBucket/AWSLogs/o-organizationID/*", "Condition": { "StringEquals": { "s3:x-amz-acl": "bucket-owner-full-control", "aws:SourceArn": "arn:aws:cloudtrail:region:managementAccountID:trail/trailName" } }

GavinZZ

LGTM

mergify · 2024-02-29T14:22:12Z

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

aws-cdk-automation · 2024-02-29T14:50:17Z

AWS CodeBuild CI Report

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

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

mergify · 2024-02-29T14:50:39Z

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

fix(cloudtrail): isOrganizationTrail attaches insufficient permission…

54ed725

…s to bucket

aws-cdk-automation requested a review from a team February 24, 2024 00:56

github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p1 labels Feb 24, 2024

mergify bot added the contribution/core This is a PR that came from AWS. label Feb 24, 2024

Update prop description

c414c46

aws-cdk-automation previously requested changes Feb 24, 2024

View reviewed changes

paulhcsun added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Feb 24, 2024

paulhcsun marked this pull request as ready for review February 24, 2024 01:00

Merge branch 'main' into cloudtrail-fix

352a8d0

aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Feb 24, 2024

paulhcsun commented Feb 26, 2024

View reviewed changes

paulhcsun and others added 4 commits February 26, 2024 10:08

fix linting

9669565

Merge branch 'main' into cloudtrail-fix

e87a44d

more linter fixes

c8ba648

update snapshots

8d88e17

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Feb 27, 2024

GavinZZ reviewed Feb 28, 2024

View reviewed changes

GavinZZ approved these changes Feb 29, 2024

View reviewed changes

Merge branch 'main' into cloudtrail-fix

d15f25e

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Feb 29, 2024

mergify bot merged commit 457afa9 into aws:main Feb 29, 2024
12 checks passed

paulhcsun deleted the cloudtrail-fix branch February 29, 2024 17:50

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

Monthly PR metrics report - Feb 2023 #29328

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cloudtrail): isOrganizationTrail attaches insufficient permissions to bucket #29242

fix(cloudtrail): isOrganizationTrail attaches insufficient permissions to bucket #29242

paulhcsun commented Feb 24, 2024 •

edited

aws-cdk-automation left a comment

paulhcsun commented Feb 24, 2024

paulhcsun Feb 26, 2024

GavinZZ left a comment

GavinZZ Feb 28, 2024

paulhcsun Feb 28, 2024 •

edited

GavinZZ Feb 29, 2024

GavinZZ Feb 28, 2024

paulhcsun Feb 28, 2024

GavinZZ Feb 28, 2024

paulhcsun Feb 28, 2024

GavinZZ left a comment

mergify bot commented Feb 29, 2024

aws-cdk-automation commented Feb 29, 2024

mergify bot commented Feb 29, 2024

fix(cloudtrail): isOrganizationTrail attaches insufficient permissions to bucket #29242

fix(cloudtrail): isOrganizationTrail attaches insufficient permissions to bucket #29242

Conversation

paulhcsun commented Feb 24, 2024 • edited

Issue #22267 (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

aws-cdk-automation left a comment

Choose a reason for hiding this comment

paulhcsun commented Feb 24, 2024

paulhcsun Feb 26, 2024

Choose a reason for hiding this comment

GavinZZ left a comment

Choose a reason for hiding this comment

GavinZZ Feb 28, 2024

Choose a reason for hiding this comment

paulhcsun Feb 28, 2024 • edited

Choose a reason for hiding this comment

GavinZZ Feb 29, 2024

Choose a reason for hiding this comment

GavinZZ Feb 28, 2024

Choose a reason for hiding this comment

paulhcsun Feb 28, 2024

Choose a reason for hiding this comment

GavinZZ Feb 28, 2024

Choose a reason for hiding this comment

paulhcsun Feb 28, 2024

Choose a reason for hiding this comment

GavinZZ left a comment

Choose a reason for hiding this comment

mergify bot commented Feb 29, 2024

aws-cdk-automation commented Feb 29, 2024

AWS CodeBuild CI Report

mergify bot commented Feb 29, 2024

paulhcsun commented Feb 24, 2024 •

edited

paulhcsun Feb 28, 2024 •

edited