From edbf83ac0091ddf6e640db284c8a596df25586d4 Mon Sep 17 00:00:00 2001 From: "Darryl Hughes (Linux Profile)" Date: Thu, 15 Aug 2019 15:58:41 +0100 Subject: [PATCH 1/5] Allow external/existing Bucket to be supplied (#3651) --- packages/@aws-cdk/aws-cloudtrail/lib/index.ts | 44 ++++++++++++------- 1 file changed, 27 insertions(+), 17 deletions(-) diff --git a/packages/@aws-cdk/aws-cloudtrail/lib/index.ts b/packages/@aws-cdk/aws-cloudtrail/lib/index.ts index 5c1330949fb65..b6375c193c13f 100644 --- a/packages/@aws-cdk/aws-cloudtrail/lib/index.ts +++ b/packages/@aws-cdk/aws-cloudtrail/lib/index.ts @@ -93,6 +93,12 @@ export interface TrailProps { * @default - No prefix. */ readonly s3KeyPrefix?: string; + + /** The Amazon S3 bucket + * + * @default - if not supplied a bucket will be created with all the correct permisions + */ + readonly s3Bucket?: s3.Bucket } export enum ReadWriteType { @@ -129,23 +135,27 @@ export class Trail extends Resource { physicalName: props.trailName, }); - const s3bucket = new s3.Bucket(this, 'S3', {encryption: s3.BucketEncryption.UNENCRYPTED}); - const cloudTrailPrincipal = new iam.ServicePrincipal("cloudtrail.amazonaws.com"); - - s3bucket.addToResourcePolicy(new iam.PolicyStatement({ - resources: [s3bucket.bucketArn], - actions: ['s3:GetBucketAcl'], - principals: [cloudTrailPrincipal], - })); - - s3bucket.addToResourcePolicy(new iam.PolicyStatement({ - resources: [s3bucket.arnForObjects(`AWSLogs/${Stack.of(this).account}/*`)], - actions: ["s3:PutObject"], - principals: [cloudTrailPrincipal], - conditions: { - StringEquals: {'s3:x-amz-acl': "bucket-owner-full-control"} - } - })); + let s3bucket = props.s3Bucket; + if (props.s3Bucket === undefined) { + + s3bucket = new s3.Bucket(this, 'S3', {encryption: s3.BucketEncryption.UNENCRYPTED}); + const cloudTrailPrincipal = new iam.ServicePrincipal("cloudtrail.amazonaws.com"); + + s3bucket.addToResourcePolicy(new iam.PolicyStatement({ + resources: [s3bucket.bucketArn], + actions: ['s3:GetBucketAcl'], + principals: [cloudTrailPrincipal], + })); + + s3bucket.addToResourcePolicy(new iam.PolicyStatement({ + resources: [s3bucket.arnForObjects(`AWSLogs/${Stack.of(this).account}/*`)], + actions: ["s3:PutObject"], + principals: [cloudTrailPrincipal], + conditions: { + StringEquals: {'s3:x-amz-acl': "bucket-owner-full-control"} + } + })); + } let logGroup: logs.CfnLogGroup | undefined; let logsRole: iam.IRole | undefined; From afef742f3ae25871c01b7ad5b873e3f50ec04a2b Mon Sep 17 00:00:00 2001 From: "Darryl Hughes (Linux Profile)" Date: Fri, 16 Aug 2019 00:52:04 +0100 Subject: [PATCH 2/5] added tests, fixed bug (#3651) --- packages/@aws-cdk/aws-cloudtrail/lib/index.ts | 24 ++-- ...oudtrail-supplied-bucket.lit.expected.json | 110 ++++++++++++++++++ .../integ.cloudtrail-supplied-bucket.lit.ts | 37 ++++++ .../aws-cloudtrail/test/test.cloudtrail.ts | 31 +++++ 4 files changed, 191 insertions(+), 11 deletions(-) create mode 100644 packages/@aws-cdk/aws-cloudtrail/test/integ.cloudtrail-supplied-bucket.lit.expected.json create mode 100644 packages/@aws-cdk/aws-cloudtrail/test/integ.cloudtrail-supplied-bucket.lit.ts diff --git a/packages/@aws-cdk/aws-cloudtrail/lib/index.ts b/packages/@aws-cdk/aws-cloudtrail/lib/index.ts index b6375c193c13f..7eeb922eb7c70 100644 --- a/packages/@aws-cdk/aws-cloudtrail/lib/index.ts +++ b/packages/@aws-cdk/aws-cloudtrail/lib/index.ts @@ -98,7 +98,7 @@ export interface TrailProps { * * @default - if not supplied a bucket will be created with all the correct permisions */ - readonly s3Bucket?: s3.Bucket + readonly s3Bucket?: s3.IBucket } export enum ReadWriteType { @@ -128,6 +128,7 @@ export class Trail extends Resource { */ public readonly trailSnsTopicArn: string; + private s3bucket: s3.IBucket; private eventSelectors: EventSelector[] = []; constructor(scope: Construct, id: string, props: TrailProps = {}) { @@ -135,27 +136,28 @@ export class Trail extends Resource { physicalName: props.trailName, }); - let s3bucket = props.s3Bucket; + const cloudTrailPrincipal = new iam.ServicePrincipal("cloudtrail.amazonaws.com"); + if (props.s3Bucket === undefined) { - s3bucket = new s3.Bucket(this, 'S3', {encryption: s3.BucketEncryption.UNENCRYPTED}); - const cloudTrailPrincipal = new iam.ServicePrincipal("cloudtrail.amazonaws.com"); + this.s3bucket = new s3.Bucket(this, 'S3', {encryption: s3.BucketEncryption.UNENCRYPTED}); - s3bucket.addToResourcePolicy(new iam.PolicyStatement({ - resources: [s3bucket.bucketArn], + this.s3bucket.addToResourcePolicy(new iam.PolicyStatement({ + resources: [this.s3bucket.bucketArn], actions: ['s3:GetBucketAcl'], principals: [cloudTrailPrincipal], })); - s3bucket.addToResourcePolicy(new iam.PolicyStatement({ - resources: [s3bucket.arnForObjects(`AWSLogs/${Stack.of(this).account}/*`)], + this.s3bucket.addToResourcePolicy(new iam.PolicyStatement({ + resources: [this.s3bucket.arnForObjects(`AWSLogs/${Stack.of(this).account}/*`)], actions: ["s3:PutObject"], principals: [cloudTrailPrincipal], conditions: { StringEquals: {'s3:x-amz-acl': "bucket-owner-full-control"} } })); - } + } else { + this.s3bucket = props.s3Bucket; } let logGroup: logs.CfnLogGroup | undefined; let logsRole: iam.IRole | undefined; @@ -187,7 +189,7 @@ export class Trail extends Resource { includeGlobalServiceEvents: props.includeGlobalServiceEvents == null ? true : props.includeGlobalServiceEvents, trailName: this.physicalName, kmsKeyId: props.kmsKey && props.kmsKey.keyArn, - s3BucketName: s3bucket.bucketName, + s3BucketName: this.s3bucket.bucketName, s3KeyPrefix: props.s3KeyPrefix, cloudWatchLogsLogGroupArn: logGroup && logGroup.attrArn, cloudWatchLogsRoleArn: logsRole && logsRole.roleArn, @@ -202,7 +204,7 @@ export class Trail extends Resource { }); this.trailSnsTopicArn = trail.attrSnsTopicArn; - const s3BucketPolicy = s3bucket.node.findChild("Policy").node.findChild("Resource") as s3.CfnBucketPolicy; + const s3BucketPolicy = this.s3bucket.node.findChild("Policy").node.findChild("Resource") as s3.CfnBucketPolicy; trail.node.addDependency(s3BucketPolicy); // If props.sendToCloudWatchLogs is set to true then the trail needs to depend on the created logsRole diff --git a/packages/@aws-cdk/aws-cloudtrail/test/integ.cloudtrail-supplied-bucket.lit.expected.json b/packages/@aws-cdk/aws-cloudtrail/test/integ.cloudtrail-supplied-bucket.lit.expected.json new file mode 100644 index 0000000000000..2a4bed6ae5e55 --- /dev/null +++ b/packages/@aws-cdk/aws-cloudtrail/test/integ.cloudtrail-supplied-bucket.lit.expected.json @@ -0,0 +1,110 @@ +{ + "Resources": { + "Bucket83908E77": { + "Type": "AWS::S3::Bucket", + "UpdateReplacePolicy": "Delete", + "DeletionPolicy": "Delete" + }, + "S3486F821D": { + "Type": "AWS::S3::Bucket", + "UpdateReplacePolicy": "Retain", + "DeletionPolicy": "Retain" + }, + "S3Policy2E4AA1D6": { + "Type": "AWS::S3::BucketPolicy", + "Properties": { + "Bucket": { + "Ref": "S3486F821D" + }, + "PolicyDocument": { + "Statement": [ + { + "Action": "s3:GetBucketAcl", + "Effect": "Allow", + "Principal": { + "Service": "cloudtrail.amazonaws.com" + }, + "Resource": { + "Fn::GetAtt": [ + "S3486F821D", + "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": [ + "S3486F821D", + "Arn" + ] + }, + "/AWSLogs/", + { + "Ref": "AWS::AccountId" + }, + "/*" + ] + ] + } + } + ], + "Version": "2012-10-17" + } + } + }, + "Trail022F0CF2": { + "Type": "AWS::CloudTrail::Trail", + "Properties": { + "IsLogging": true, + "S3BucketName": { + "Ref": "S3486F821D" + }, + "EnableLogFileValidation": true, + "EventSelectors": [ + { + "DataResources": [ + { + "Type": "AWS::S3::Object", + "Values": [ + { + "Fn::Join": [ + "", + [ + { + "Fn::GetAtt": [ + "Bucket83908E77", + "Arn" + ] + }, + "/" + ] + ] + } + ] + } + ] + } + ], + "IncludeGlobalServiceEvents": true, + "IsMultiRegionTrail": true + }, + "DependsOn": [ + "S3Policy2E4AA1D6" + ] + } + } +} \ No newline at end of file diff --git a/packages/@aws-cdk/aws-cloudtrail/test/integ.cloudtrail-supplied-bucket.lit.ts b/packages/@aws-cdk/aws-cloudtrail/test/integ.cloudtrail-supplied-bucket.lit.ts new file mode 100644 index 0000000000000..9787cae50030b --- /dev/null +++ b/packages/@aws-cdk/aws-cloudtrail/test/integ.cloudtrail-supplied-bucket.lit.ts @@ -0,0 +1,37 @@ +import iam = require('@aws-cdk/aws-iam'); +import s3 = require('@aws-cdk/aws-s3'); +import cdk = require('@aws-cdk/core'); +import { Stack } from '@aws-cdk/core'; + +import cloudtrail = require('../lib'); + +const app = new cdk.App(); +const stack = new cdk.Stack(app, 'integ-cloudtrail'); + +const bucket = new s3.Bucket(stack, 'Bucket', { removalPolicy: cdk.RemovalPolicy.DESTROY }); + +// using exctecy the same code as inside the cloudtrail class to produce the supplied bucket and policy +const cloudTrailPrincipal = new iam.ServicePrincipal("cloudtrail.amazonaws.com"); + +const Trailbucket = new s3.Bucket(stack, 'S3', {encryption: s3.BucketEncryption.UNENCRYPTED}); + +Trailbucket.addToResourcePolicy(new iam.PolicyStatement({ + resources: [Trailbucket.bucketArn], + actions: ['s3:GetBucketAcl'], + principals: [cloudTrailPrincipal], + })); + +Trailbucket.addToResourcePolicy(new iam.PolicyStatement({ + resources: [Trailbucket.arnForObjects(`AWSLogs/${Stack.of(stack).account}/*`)], + actions: ["s3:PutObject"], + principals: [cloudTrailPrincipal], + conditions: { + StringEquals: {'s3:x-amz-acl': "bucket-owner-full-control"} + } + })); + +const trail = new cloudtrail.Trail(stack, 'Trail', {s3Bucket: Trailbucket}); + +trail.addS3EventSelector([bucket.arnForObjects('')]); + +app.synth(); diff --git a/packages/@aws-cdk/aws-cloudtrail/test/test.cloudtrail.ts b/packages/@aws-cdk/aws-cloudtrail/test/test.cloudtrail.ts index 803ddaf8e8c91..d3e387152c63b 100644 --- a/packages/@aws-cdk/aws-cloudtrail/test/test.cloudtrail.ts +++ b/packages/@aws-cdk/aws-cloudtrail/test/test.cloudtrail.ts @@ -1,5 +1,7 @@ import { expect, haveResource, not, SynthUtils } from '@aws-cdk/assert'; +import iam = require('@aws-cdk/aws-iam'); import { RetentionDays } from '@aws-cdk/aws-logs'; +import s3 = require('@aws-cdk/aws-s3'); import { Stack } from '@aws-cdk/core'; import { Test } from 'nodeunit'; import { ReadWriteType, Trail } from '../lib'; @@ -67,6 +69,35 @@ export = { test.deepEqual(trail.DependsOn, ['MyAmazingCloudTrailS3Policy39C120B0']); test.done(); }, + 'with s3bucket'(test: Test) { + const stack = getTestStack(); + const Trailbucket = new s3.Bucket(stack, 'S3'); + const cloudTrailPrincipal = new iam.ServicePrincipal("cloudtrail.amazonaws.com"); + Trailbucket.addToResourcePolicy(new iam.PolicyStatement({ + resources: [Trailbucket.bucketArn], + actions: ['s3:GetBucketAcl'], + principals: [cloudTrailPrincipal], + })); + + Trailbucket.addToResourcePolicy(new iam.PolicyStatement({ + resources: [Trailbucket.arnForObjects(`AWSLogs/${Stack.of(stack).account}/*`)], + actions: ["s3:PutObject"], + principals: [cloudTrailPrincipal], + conditions: { + StringEquals: {'s3:x-amz-acl': "bucket-owner-full-control"} + } + })); + + new Trail(stack, 'Trail', {s3Bucket: Trailbucket}); + + expect(stack).to(haveResource("AWS::CloudTrail::Trail")); + expect(stack).to(haveResource("AWS::S3::Bucket")); + expect(stack).to(haveResource("AWS::S3::BucketPolicy", ExpectedBucketPolicyProperties)); + expect(stack).to(not(haveResource("AWS::Logs::LogGroup"))); + const trail: any = SynthUtils.synthesize(stack).template.Resources.MyAmazingCloudTrail54516E8D; + test.deepEqual(trail.DependsOn, ['MyAmazingCloudTrailS3Policy39C120B0']); + test.done(); + }, 'with cloud watch logs': { 'enabled'(test: Test) { const stack = getTestStack(); From 59b74f84739eeb76e12a2f3f880784fd7d070bc8 Mon Sep 17 00:00:00 2001 From: slipdexic Date: Fri, 16 Aug 2019 21:57:02 +0100 Subject: [PATCH 3/5] Simplied test, in this case we do not care about the actual bucket policy. The bucket is externally created, we just check on is present and the the LogGroup and trail is created. --- packages/@aws-cdk/aws-cloudtrail/test/test.cloudtrail.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/@aws-cdk/aws-cloudtrail/test/test.cloudtrail.ts b/packages/@aws-cdk/aws-cloudtrail/test/test.cloudtrail.ts index d3e387152c63b..ea9000030c627 100644 --- a/packages/@aws-cdk/aws-cloudtrail/test/test.cloudtrail.ts +++ b/packages/@aws-cdk/aws-cloudtrail/test/test.cloudtrail.ts @@ -92,10 +92,8 @@ export = { expect(stack).to(haveResource("AWS::CloudTrail::Trail")); expect(stack).to(haveResource("AWS::S3::Bucket")); - expect(stack).to(haveResource("AWS::S3::BucketPolicy", ExpectedBucketPolicyProperties)); + expect(stack).to(haveResource("AWS::S3::BucketPolicy")); expect(stack).to(not(haveResource("AWS::Logs::LogGroup"))); - const trail: any = SynthUtils.synthesize(stack).template.Resources.MyAmazingCloudTrail54516E8D; - test.deepEqual(trail.DependsOn, ['MyAmazingCloudTrailS3Policy39C120B0']); test.done(); }, 'with cloud watch logs': { From 06d48767e6f2d5e0cb59d492ac5086b43c9ea948 Mon Sep 17 00:00:00 2001 From: slipdexic Date: Tue, 20 Aug 2019 17:04:19 +0100 Subject: [PATCH 4/5] [chore] updated propery as ruqested in code review, --- packages/@aws-cdk/aws-cloudtrail/lib/index.ts | 6 +++--- .../test/integ.cloudtrail-supplied-bucket.lit.ts | 2 +- packages/@aws-cdk/aws-cloudtrail/test/test.cloudtrail.ts | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/@aws-cdk/aws-cloudtrail/lib/index.ts b/packages/@aws-cdk/aws-cloudtrail/lib/index.ts index 7eeb922eb7c70..ef261e2b03b4f 100644 --- a/packages/@aws-cdk/aws-cloudtrail/lib/index.ts +++ b/packages/@aws-cdk/aws-cloudtrail/lib/index.ts @@ -98,7 +98,7 @@ export interface TrailProps { * * @default - if not supplied a bucket will be created with all the correct permisions */ - readonly s3Bucket?: s3.IBucket + readonly bucket?: s3.IBucket } export enum ReadWriteType { @@ -138,7 +138,7 @@ export class Trail extends Resource { const cloudTrailPrincipal = new iam.ServicePrincipal("cloudtrail.amazonaws.com"); - if (props.s3Bucket === undefined) { + if (props.bucket === undefined) { this.s3bucket = new s3.Bucket(this, 'S3', {encryption: s3.BucketEncryption.UNENCRYPTED}); @@ -157,7 +157,7 @@ export class Trail extends Resource { } })); } else { - this.s3bucket = props.s3Bucket; } + this.s3bucket = props.bucket; } let logGroup: logs.CfnLogGroup | undefined; let logsRole: iam.IRole | undefined; diff --git a/packages/@aws-cdk/aws-cloudtrail/test/integ.cloudtrail-supplied-bucket.lit.ts b/packages/@aws-cdk/aws-cloudtrail/test/integ.cloudtrail-supplied-bucket.lit.ts index 9787cae50030b..e8872b1a1afb1 100644 --- a/packages/@aws-cdk/aws-cloudtrail/test/integ.cloudtrail-supplied-bucket.lit.ts +++ b/packages/@aws-cdk/aws-cloudtrail/test/integ.cloudtrail-supplied-bucket.lit.ts @@ -30,7 +30,7 @@ Trailbucket.addToResourcePolicy(new iam.PolicyStatement({ } })); -const trail = new cloudtrail.Trail(stack, 'Trail', {s3Bucket: Trailbucket}); +const trail = new cloudtrail.Trail(stack, 'Trail', {bucket: Trailbucket}); trail.addS3EventSelector([bucket.arnForObjects('')]); diff --git a/packages/@aws-cdk/aws-cloudtrail/test/test.cloudtrail.ts b/packages/@aws-cdk/aws-cloudtrail/test/test.cloudtrail.ts index ea9000030c627..4384c2665e194 100644 --- a/packages/@aws-cdk/aws-cloudtrail/test/test.cloudtrail.ts +++ b/packages/@aws-cdk/aws-cloudtrail/test/test.cloudtrail.ts @@ -88,7 +88,7 @@ export = { } })); - new Trail(stack, 'Trail', {s3Bucket: Trailbucket}); + new Trail(stack, 'Trail', {bucket: Trailbucket}); expect(stack).to(haveResource("AWS::CloudTrail::Trail")); expect(stack).to(haveResource("AWS::S3::Bucket")); From 1ee3d6db5b3f529e223d94ac5aa852d9992d76a0 Mon Sep 17 00:00:00 2001 From: slipdexic Date: Thu, 22 Aug 2019 14:49:55 +0200 Subject: [PATCH 5/5] (chore) applied change sugested in code review closes #3651 --- packages/@aws-cdk/aws-cloudtrail/lib/index.ts | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/packages/@aws-cdk/aws-cloudtrail/lib/index.ts b/packages/@aws-cdk/aws-cloudtrail/lib/index.ts index ef261e2b03b4f..c71ef322f1793 100644 --- a/packages/@aws-cdk/aws-cloudtrail/lib/index.ts +++ b/packages/@aws-cdk/aws-cloudtrail/lib/index.ts @@ -115,6 +115,10 @@ export enum ReadWriteType { * * const cloudTrail = new CloudTrail(this, 'MyTrail'); * + * NOTE the above example creates an UNENCRYPTED bucket by default, + * If you are required to use an Encrypted bucket you can supply a preconfigured bucket + * via TrailProps + * */ export class Trail extends Resource { @@ -138,17 +142,15 @@ export class Trail extends Resource { const cloudTrailPrincipal = new iam.ServicePrincipal("cloudtrail.amazonaws.com"); - if (props.bucket === undefined) { - - this.s3bucket = new s3.Bucket(this, 'S3', {encryption: s3.BucketEncryption.UNENCRYPTED}); + this.s3bucket = props.bucket || new s3.Bucket(this, 'S3', {encryption: s3.BucketEncryption.UNENCRYPTED}); - this.s3bucket.addToResourcePolicy(new iam.PolicyStatement({ + this.s3bucket.addToResourcePolicy(new iam.PolicyStatement({ resources: [this.s3bucket.bucketArn], actions: ['s3:GetBucketAcl'], principals: [cloudTrailPrincipal], })); - this.s3bucket.addToResourcePolicy(new iam.PolicyStatement({ + this.s3bucket.addToResourcePolicy(new iam.PolicyStatement({ resources: [this.s3bucket.arnForObjects(`AWSLogs/${Stack.of(this).account}/*`)], actions: ["s3:PutObject"], principals: [cloudTrailPrincipal], @@ -156,11 +158,10 @@ export class Trail extends Resource { StringEquals: {'s3:x-amz-acl': "bucket-owner-full-control"} } })); - } else { - this.s3bucket = props.bucket; } let logGroup: logs.CfnLogGroup | undefined; let logsRole: iam.IRole | undefined; + if (props.sendToCloudWatchLogs) { logGroup = new logs.CfnLogGroup(this, "LogGroup", { retentionInDays: props.cloudWatchLogsRetention || logs.RetentionDays.ONE_YEAR @@ -173,6 +174,7 @@ export class Trail extends Resource { resources: [logGroup.attrArn], })); } + if (props.managementEvents) { const managementEvent = { includeManagementEvents: true,