From 457afa9d01fca8c9b91648175d6aa3183611e504 Mon Sep 17 00:00:00 2001 From: paulhcsun <47882901+paulhcsun@users.noreply.github.com> Date: Thu, 29 Feb 2024 06:50:36 -0800 Subject: [PATCH] fix(cloudtrail): isOrganizationTrail attaches insufficient permissions to bucket (#29242) ### Issue https://github.com/aws/aws-cdk/issues/22267 (if applicable) Closes https://github.com/aws/aws-cdk/issues/22267. ### 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](https://docs.aws.amazon.com/awscloudtrail/latest/userguide/create-s3-bucket-policy-for-cloudtrail.html#org-trail-bucket-policy) ``` "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 - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- .../aws-cloudtrail/lib/cloudtrail.ts | 24 ++++ .../aws-cloudtrail/test/cloudtrail.test.ts | 130 ++++++++++++++++++ 2 files changed, 154 insertions(+) diff --git a/packages/aws-cdk-lib/aws-cloudtrail/lib/cloudtrail.ts b/packages/aws-cdk-lib/aws-cloudtrail/lib/cloudtrail.ts index 7e85a0821278a..301c32eaa22b3 100644 --- a/packages/aws-cdk-lib/aws-cloudtrail/lib/cloudtrail.ts +++ b/packages/aws-cdk-lib/aws-cloudtrail/lib/cloudtrail.ts @@ -128,6 +128,14 @@ export interface TrailProps { */ readonly isOrganizationTrail?: boolean; + /** The orgId. + * + * Required when `isOrganizationTrail` is set to true to attach the necessary permissions. + * + * @default - No orgId + */ + readonly orgId?: string; + /** * A JSON string that contains the insight types you want to log on a trail. * @@ -262,6 +270,22 @@ export class Trail extends 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': `arn:${this.stack.partition}:cloudtrail:${this.s3bucket.stack.region}:${this.s3bucket.stack.account}:trail/${props.trailName}`, + }, + }, + })); + } + this.topic = props.snsTopic; if (this.topic) { this.topic.grantPublish(cloudTrailPrincipal); diff --git a/packages/aws-cdk-lib/aws-cloudtrail/test/cloudtrail.test.ts b/packages/aws-cdk-lib/aws-cloudtrail/test/cloudtrail.test.ts index 0950d68c185cc..55d762db6efe0 100644 --- a/packages/aws-cdk-lib/aws-cloudtrail/test/cloudtrail.test.ts +++ b/packages/aws-cdk-lib/aws-cloudtrail/test/cloudtrail.test.ts @@ -246,6 +246,136 @@ describe('cloudtrail', () => { }); }); + test('with orgId', () => { + // GIVEN + const stack = getTestStack(); + + // WHEN + new Trail(stack, 'Trail', { isOrganizationTrail: true, orgId: 'o-xxxxxxxxx', trailName: 'trailname123' }); + + Template.fromStack(stack).resourceCountIs('AWS::CloudTrail::Trail', 1); + Template.fromStack(stack).resourceCountIs('AWS::S3::Bucket', 1); + Template.fromStack(stack).hasResourceProperties('AWS::S3::BucketPolicy', { + Bucket: { Ref: 'TrailS30071F172' }, + PolicyDocument: { + Statement: [ + { + Action: 's3:*', + Condition: { + Bool: { + 'aws:SecureTransport': 'false', + }, + }, + Effect: 'Deny', + Principal: { + AWS: '*', + }, + Resource: [ + { + 'Fn::GetAtt': [ + 'TrailS30071F172', + 'Arn', + ], + }, + { + 'Fn::Join': [ + '', + [ + { + 'Fn::GetAtt': [ + 'TrailS30071F172', + 'Arn', + ], + }, + '/*', + ], + ], + }, + ], + }, + { + Action: 's3:GetBucketAcl', + Effect: 'Allow', + Principal: { + Service: 'cloudtrail.amazonaws.com', + }, + Resource: { + 'Fn::GetAtt': [ + 'TrailS30071F172', + 'Arn', + ], + }, + }, + { + Action: 's3:PutObject', + Condition: { + StringEquals: { + 's3:x-amz-acl': 'bucket-owner-full-control', + }, + }, + Effect: 'Allow', + Principal: { + Service: 'cloudtrail.amazonaws.com', + }, + Resource: { + 'Fn::Join': [ + '', + [ + { + 'Fn::GetAtt': [ + 'TrailS30071F172', + 'Arn', + ], + }, + '/AWSLogs/123456789012/*', + ], + ], + }, + }, + { + 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/trailname123', + ], + ], + }, + }, + }, + Effect: 'Allow', + Principal: { + Service: 'cloudtrail.amazonaws.com', + }, + Resource: { + 'Fn::Join': [ + '', + [ + { + 'Fn::GetAtt': [ + 'TrailS30071F172', + 'Arn', + ], + }, + '/AWSLogs/o-xxxxxxxxx/*', + ], + ], + }, + }, + ], + Version: '2012-10-17', + }, + }); + }); + test('encryption keys', () => { const stack = new Stack(); const key = new kms.Key(stack, 'key');