Skip to content

Commit

Permalink
fix(cloudtrail): isOrganizationTrail attaches insufficient permission…
Browse files Browse the repository at this point in the history
…s 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*
  • Loading branch information
paulhcsun authored Feb 29, 2024
1 parent 8d07b85 commit 457afa9
Show file tree
Hide file tree
Showing 2 changed files with 154 additions and 0 deletions.
24 changes: 24 additions & 0 deletions packages/aws-cdk-lib/aws-cloudtrail/lib/cloudtrail.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand Down Expand Up @@ -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);
Expand Down
130 changes: 130 additions & 0 deletions packages/aws-cdk-lib/aws-cloudtrail/test/cloudtrail.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down

0 comments on commit 457afa9

Please sign in to comment.