Skip to content

Commit 457afa9

Browse files
authored
fix(cloudtrail): isOrganizationTrail attaches insufficient permissions to bucket (#29242)
### Issue #22267 (if applicable) Closes #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*
1 parent 8d07b85 commit 457afa9

File tree

2 files changed

+154
-0
lines changed

2 files changed

+154
-0
lines changed

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

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,14 @@ export interface TrailProps {
128128
*/
129129
readonly isOrganizationTrail?: boolean;
130130

131+
/** The orgId.
132+
*
133+
* Required when `isOrganizationTrail` is set to true to attach the necessary permissions.
134+
*
135+
* @default - No orgId
136+
*/
137+
readonly orgId?: string;
138+
131139
/**
132140
* A JSON string that contains the insight types you want to log on a trail.
133141
*
@@ -262,6 +270,22 @@ export class Trail extends Resource {
262270
},
263271
}));
264272

273+
if (props.isOrganizationTrail) {
274+
this.s3bucket.addToResourcePolicy(new iam.PolicyStatement({
275+
resources: [this.s3bucket.arnForObjects(
276+
`AWSLogs/${props.orgId}/*`,
277+
)],
278+
actions: ['s3:PutObject'],
279+
principals: [cloudTrailPrincipal],
280+
conditions: {
281+
StringEquals: {
282+
's3:x-amz-acl': 'bucket-owner-full-control',
283+
'aws:SourceArn': `arn:${this.stack.partition}:cloudtrail:${this.s3bucket.stack.region}:${this.s3bucket.stack.account}:trail/${props.trailName}`,
284+
},
285+
},
286+
}));
287+
}
288+
265289
this.topic = props.snsTopic;
266290
if (this.topic) {
267291
this.topic.grantPublish(cloudTrailPrincipal);

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

Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -246,6 +246,136 @@ describe('cloudtrail', () => {
246246
});
247247
});
248248

249+
test('with orgId', () => {
250+
// GIVEN
251+
const stack = getTestStack();
252+
253+
// WHEN
254+
new Trail(stack, 'Trail', { isOrganizationTrail: true, orgId: 'o-xxxxxxxxx', trailName: 'trailname123' });
255+
256+
Template.fromStack(stack).resourceCountIs('AWS::CloudTrail::Trail', 1);
257+
Template.fromStack(stack).resourceCountIs('AWS::S3::Bucket', 1);
258+
Template.fromStack(stack).hasResourceProperties('AWS::S3::BucketPolicy', {
259+
Bucket: { Ref: 'TrailS30071F172' },
260+
PolicyDocument: {
261+
Statement: [
262+
{
263+
Action: 's3:*',
264+
Condition: {
265+
Bool: {
266+
'aws:SecureTransport': 'false',
267+
},
268+
},
269+
Effect: 'Deny',
270+
Principal: {
271+
AWS: '*',
272+
},
273+
Resource: [
274+
{
275+
'Fn::GetAtt': [
276+
'TrailS30071F172',
277+
'Arn',
278+
],
279+
},
280+
{
281+
'Fn::Join': [
282+
'',
283+
[
284+
{
285+
'Fn::GetAtt': [
286+
'TrailS30071F172',
287+
'Arn',
288+
],
289+
},
290+
'/*',
291+
],
292+
],
293+
},
294+
],
295+
},
296+
{
297+
Action: 's3:GetBucketAcl',
298+
Effect: 'Allow',
299+
Principal: {
300+
Service: 'cloudtrail.amazonaws.com',
301+
},
302+
Resource: {
303+
'Fn::GetAtt': [
304+
'TrailS30071F172',
305+
'Arn',
306+
],
307+
},
308+
},
309+
{
310+
Action: 's3:PutObject',
311+
Condition: {
312+
StringEquals: {
313+
's3:x-amz-acl': 'bucket-owner-full-control',
314+
},
315+
},
316+
Effect: 'Allow',
317+
Principal: {
318+
Service: 'cloudtrail.amazonaws.com',
319+
},
320+
Resource: {
321+
'Fn::Join': [
322+
'',
323+
[
324+
{
325+
'Fn::GetAtt': [
326+
'TrailS30071F172',
327+
'Arn',
328+
],
329+
},
330+
'/AWSLogs/123456789012/*',
331+
],
332+
],
333+
},
334+
},
335+
{
336+
Action: 's3:PutObject',
337+
Condition: {
338+
StringEquals: {
339+
's3:x-amz-acl': 'bucket-owner-full-control',
340+
'aws:SourceArn': {
341+
'Fn::Join': [
342+
'',
343+
[
344+
'arn:',
345+
{
346+
Ref: 'AWS::Partition',
347+
},
348+
':cloudtrail:us-east-1:123456789012:trail/trailname123',
349+
],
350+
],
351+
},
352+
},
353+
},
354+
Effect: 'Allow',
355+
Principal: {
356+
Service: 'cloudtrail.amazonaws.com',
357+
},
358+
Resource: {
359+
'Fn::Join': [
360+
'',
361+
[
362+
{
363+
'Fn::GetAtt': [
364+
'TrailS30071F172',
365+
'Arn',
366+
],
367+
},
368+
'/AWSLogs/o-xxxxxxxxx/*',
369+
],
370+
],
371+
},
372+
},
373+
],
374+
Version: '2012-10-17',
375+
},
376+
});
377+
});
378+
249379
test('encryption keys', () => {
250380
const stack = new Stack();
251381
const key = new kms.Key(stack, 'key');

0 commit comments

Comments
 (0)