fix(aws-cloudtrail): correct S3 bucket policy and dependency chain (#…

…1268) Fixes #1172
aws · Dec 3, 2018 · 0de2da8 · 0de2da8
1 parent a45c3bd
commit 0de2da8
Show file tree

Hide file tree

Showing 3 changed files with 67 additions and 9 deletions.
diff --git a/packages/@aws-cdk/aws-cloudtrail/lib/index.ts b/packages/@aws-cdk/aws-cloudtrail/lib/index.ts
@@ -138,7 +138,7 @@ export class CloudTrail extends cdk.Construct {
       .addServicePrincipal(cloudTrailPrincipal));
 
     s3bucket.addToResourcePolicy(new iam.PolicyStatement()
-      .addResource(s3bucket.arnForObjects(new cdk.FnConcat('/AWSLogs/', new cdk.AwsAccountId())))
+      .addResource(s3bucket.arnForObjects(new cdk.FnConcat('AWSLogs/', new cdk.AwsAccountId(), "/*")))
       .addActions("s3:PutObject")
       .addServicePrincipal(cloudTrailPrincipal)
       .setCondition("StringEquals", {'s3:x-amz-acl': "bucket-owner-full-control"}));
@@ -182,6 +182,8 @@ export class CloudTrail extends cdk.Construct {
       eventSelectors: this.eventSelectors
     });
     this.cloudTrailArn = trail.trailArn;
+    const s3BucketPolicy = s3bucket.findChild("Policy").findChild("Resource") as s3.cloudformation.BucketPolicyResource;
+    trail.addDependency(s3BucketPolicy);
   }
 
   /**

diff --git a/packages/@aws-cdk/aws-cloudtrail/package.json b/packages/@aws-cdk/aws-cloudtrail/package.json
@@ -70,4 +70,4 @@
     "@aws-cdk/aws-kms": "^0.18.1",
     "@aws-cdk/cdk": "^0.18.1"
   }
-}
+}
diff --git a/packages/@aws-cdk/aws-cloudtrail/test/test.cloudtrail.ts b/packages/@aws-cdk/aws-cloudtrail/test/test.cloudtrail.ts
@@ -3,16 +3,68 @@ import { Stack } from '@aws-cdk/cdk';
 import { Test } from 'nodeunit';
 import { CloudTrail, LogRetention, ReadWriteType } from '../lib';
 
+const ExpectedBucketPolicyProperties = {
+  PolicyDocument: {
+    Statement: [
+      {
+        Action: "s3:GetBucketAcl",
+        Effect: "Allow",
+        Principal: {
+          Service: "cloudtrail.amazonaws.com"
+        },
+        Resource: {
+          "Fn::GetAtt": [
+            "MyAmazingCloudTrailS3A580FE27",
+            "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": [
+                  "MyAmazingCloudTrailS3A580FE27",
+                  "Arn"
+                ]
+              },
+              "/AWSLogs/",
+              {
+                Ref: "AWS::AccountId"
+              },
+              "/*"
+            ]
+          ]
+        }
+      }
+    ],
+    Version: "2012-10-17"
+  }
+};
+
 export = {
   'constructs the expected resources': {
     'with no properties'(test: Test) {
       const stack = getTestStack();
       new CloudTrail(stack, 'MyAmazingCloudTrail');
-
       expect(stack).to(haveResource("AWS::CloudTrail::Trail"));
       expect(stack).to(haveResource("AWS::S3::Bucket"));
-      expect(stack).to(haveResource("AWS::S3::BucketPolicy"));
+      expect(stack).to(haveResource("AWS::S3::BucketPolicy", ExpectedBucketPolicyProperties));
       expect(stack).to(not(haveResource("AWS::Logs::LogGroup")));
+      const trail: any = stack.toCloudFormation().Resources.MyAmazingCloudTrail54516E8D;
+      test.deepEqual(trail.DependsOn, ['MyAmazingCloudTrailS3Policy39C120B0']);
       test.done();
     },
     'with cloud watch logs': {
@@ -24,13 +76,14 @@ export = {
 
         expect(stack).to(haveResource("AWS::CloudTrail::Trail"));
         expect(stack).to(haveResource("AWS::S3::Bucket"));
-        expect(stack).to(haveResource("AWS::S3::BucketPolicy"));
+        expect(stack).to(haveResource("AWS::S3::BucketPolicy", ExpectedBucketPolicyProperties));
         expect(stack).to(haveResource("AWS::Logs::LogGroup"));
         expect(stack).to(haveResource("AWS::IAM::Role"));
         expect(stack).to(haveResource("AWS::Logs::LogGroup", {
           RetentionInDays: 365
         }));
-
+        const trail: any = stack.toCloudFormation().Resources.MyAmazingCloudTrail54516E8D;
+        test.deepEqual(trail.DependsOn, ['MyAmazingCloudTrailS3Policy39C120B0']);
         test.done();
       },
       'enabled and custom retention'(test: Test) {
@@ -42,12 +95,14 @@ export = {
 
         expect(stack).to(haveResource("AWS::CloudTrail::Trail"));
         expect(stack).to(haveResource("AWS::S3::Bucket"));
-        expect(stack).to(haveResource("AWS::S3::BucketPolicy"));
+        expect(stack).to(haveResource("AWS::S3::BucketPolicy", ExpectedBucketPolicyProperties));
         expect(stack).to(haveResource("AWS::Logs::LogGroup"));
         expect(stack).to(haveResource("AWS::IAM::Role"));
         expect(stack).to(haveResource("AWS::Logs::LogGroup", {
           RetentionInDays: 7
         }));
+        const trail: any = stack.toCloudFormation().Resources.MyAmazingCloudTrail54516E8D;
+        test.deepEqual(trail.DependsOn, ['MyAmazingCloudTrailS3Policy39C120B0']);
         test.done();
       },
     },
@@ -60,7 +115,7 @@ export = {
 
         expect(stack).to(haveResource("AWS::CloudTrail::Trail"));
         expect(stack).to(haveResource("AWS::S3::Bucket"));
-        expect(stack).to(haveResource("AWS::S3::BucketPolicy"));
+        expect(stack).to(haveResource("AWS::S3::BucketPolicy", ExpectedBucketPolicyProperties));
         expect(stack).to(not(haveResource("AWS::Logs::LogGroup")));
         expect(stack).to(not(haveResource("AWS::IAM::Role")));
 
@@ -74,13 +129,14 @@ export = {
         test.equals(dataResource.Type, "AWS::S3::Object", "Expected the data resrouce type to be AWS::S3::Object");
         test.equals(dataResource.Values.length, 1, "Expected there to be one value");
         test.equals(dataResource.Values[0], "arn:aws:s3:::", "Expected the first type value to be the S3 type");
+        test.deepEqual(trail.DependsOn, ['MyAmazingCloudTrailS3Policy39C120B0']);
         test.done();
       },
 
       'with management event'(test: Test) {
         const stack = getTestStack();
 
-        new CloudTrail(stack, 'MyAmazingCloudTrail', {managementEvents: ReadWriteType.WriteOnly});
+        new CloudTrail(stack, 'MyAmazingCloudTrail', { managementEvents: ReadWriteType.WriteOnly });
 
         const trail: any = stack.toCloudFormation().Resources.MyAmazingCloudTrail54516E8D;
         test.equals(trail.Properties.EventSelectors.length, 1);