Functional aws_cloudtrail_trail resource #186

rx294 · 2018-01-17T18:17:13Z

Fixes #146

Fixes chef-boneyard#146 Signed-off-by: Rony Xavier <rx294@nyu.edu>

Signed-off-by: Rony Xavier <rx294@nyu.edu>

clintoncwolfe

Just a few tiny doc edits, and a question about an IAM policy. Very good work, and a very good contribution - thank you!

clintoncwolfe · 2018-01-18T16:23:02Z

docs/resources/aws_cloudtrail_trail.md

+
+### be_encrypted
+
+The test will pass if the logs delivered by identified trail is encrypted.


delivered by the identified trail

clintoncwolfe · 2018-01-18T16:25:07Z

docs/resources/aws_cloudtrail_trail.md

+The ARN identifier of the specified trail. An ARN uniquely identifies the trail within AWS.
+
+    describe aws_cloudtrail_trail('trail-name') do
+      its('trail_arn') { should cmp "trail-arn" }


I'd prefer that we had a real-looking ARN URI as the comparison value

clintoncwolfe · 2018-01-18T16:26:13Z

docs/resources/aws_cloudtrail_trail.md

+
+### be_log_file_validation_enabled
+
+The test will pass if the identified trail log file integrity validation is enabled.


The test will pass if the identified trail has log file integrity validation is enabled.

clintoncwolfe · 2018-01-18T16:26:40Z

libraries/aws_cloudtrail_trail.rb

@@ -0,0 +1,74 @@
+class AwsCloudTrailTrail < Inspec.resource(1)
+  name 'aws_cloudtrail_trail'
+  desc 'Verifies settings for individual AWS CloudTrail Trail'


Verifies settings for an individual AWS CloudTrail Trail

clintoncwolfe · 2018-01-18T16:30:47Z

test/integration/default/build/aws.tf

+
+output "aws_region" {
+  value = "${data.aws_region.region.name}"
+}


👍 to this, I've almost added it myself several times.

clintoncwolfe · 2018-01-18T16:34:15Z

test/integration/default/build/cloudtrail.tf

+      }
+    },
+    {
+      "Sid": "Enable cross account log decryption",


Do we require this ability?

Not necessary...i was going with the default Key Policy Created in CloudTrail Console.
I have removed it.

clintoncwolfe · 2018-01-18T16:37:11Z

test/integration/default/verify/controls/aws_cloudtrail_trail.rb

+  describe aws_cloudtrail_trail(fixtures['cloudtrail_trail_2_name']) do
+    it { should_not be_log_file_validation_enabled }
+  end
+end


💯 The fixtures and integration tests are very good here.

Signed-off-by: Rony Xavier <rx294@nyu.edu>

rx294 · 2018-01-18T18:34:30Z

Thank you @clintoncwolfe i have made the updates.

clintoncwolfe · 2018-01-19T17:25:19Z

I'm happy with the code, and unit tests pass cleanly. However, the integration tests fail for me, with an IAM Role error:

aws_cloudtrail.trail_1: Still creating... (10s elapsed)
Error applying plan:

1 error(s) occurred:

* aws_cloudtrail.trail_1: 1 error(s) occurred:

* aws_cloudtrail.trail_1: InvalidCloudWatchLogsLogGroupArnException: Access denied. Check the permissions for your role.
	status code: 400, request id: 6263e786-d490-4a99-9ae9-90ae11da9df8

Offhand, that looks like this now-fixed terraform-aws bug, but that had a workaround of re-running terraform, which doesn't work here; that makes me think the policy is actually bad, rather than being a timing issue.

I'll continue making my "rounds"; and then circle back to this later, if you haven't gotten to it yet. IAM policies are always pretty subtle, and I'm no expert on what CloudTrail needs.

rx294 · 2018-01-19T18:46:33Z

@clintoncwolfe thats weird the integration tests are working fine on my side.

I just had @samcornwell try the same and its working for him too.

clintoncwolfe · 2018-01-23T05:41:11Z

test/integration/default/build/cloudtrail.tf

+                "logs:CreateLogStream"
+            ],
+            "Resource": [
+                "arn:aws:logs:us-east-1:${data.aws_caller_identity.creds.account_id}:log-group:${aws_cloudwatch_log_group.trail_1_log_group.name}:log-stream:${data.aws_caller_identity.creds.account_id}_CloudTrail_${data.aws_region.region.name}*"


Here's the location triggering the IAM role permission issue - the us-east-1 here needs to be parameterized, that is, replaced with ${data.aws_region.region.name}.

I test in us-east-2, and this grants permission to work in us-east-1; thus the conflict.

clintoncwolfe · 2018-01-23T05:41:27Z

test/integration/default/build/cloudtrail.tf

+                "logs:PutLogEvents"
+            ],
+            "Resource": [
+                "arn:aws:logs:us-east-1:${data.aws_caller_identity.creds.account_id}:log-group:${aws_cloudwatch_log_group.trail_1_log_group.name}:log-stream:${data.aws_caller_identity.creds.account_id}_CloudTrail_${data.aws_region.region.name}*"


Same issue here, parameterize the region.

clintoncwolfe · 2018-01-23T05:42:28Z

Ok, I found where it was happening - a hardcoded region in the policy. The integration tests pass once that's in place. Once you make the change, we'll get it merged!

Signed-off-by: Rony Xavier <rx294@nyu.edu>

rx294 · 2018-01-23T14:02:17Z

Ah! my bad...I have made the corrections...thanks again @clintoncwolfe.

clintoncwolfe · 2018-01-23T16:26:08Z

Thanks for this excellent contribution!

Signed-off-by: Rony Xavier <rx294@nyu.edu>

rx294 added 6 commits January 17, 2018 13:14

Functional aws_cloudtrail_trail resource

cc70cb4

Fixes chef-boneyard#146 Signed-off-by: Rony Xavier <rx294@nyu.edu>

Functional aws_cloudtrail_trail resource

2ae3b2a

Fixes chef-boneyard#146 Signed-off-by: Rony Xavier <rx294@nyu.edu>

updates

3bd5459

Signed-off-by: Rony Xavier <rx294@nyu.edu>

updates

c344b7e

Signed-off-by: Rony Xavier <rx294@nyu.edu>

updates

228911a

Signed-off-by: Rony Xavier <rx294@nyu.edu>

tf code fix

16a689a

Signed-off-by: Rony Xavier <rx294@nyu.edu>

clintoncwolfe requested changes Jan 18, 2018

View reviewed changes

updates as per review

8f2643c

Signed-off-by: Rony Xavier <rx294@nyu.edu>

clintoncwolfe reviewed Jan 23, 2018

View reviewed changes

Updates as per review

322964c

Signed-off-by: Rony Xavier <rx294@nyu.edu>

clintoncwolfe approved these changes Jan 23, 2018

View reviewed changes

clintoncwolfe merged commit 0af7105 into chef-boneyard:master Jan 23, 2018

rx294 deleted the aws_cloudtrail_trail_pr branch January 23, 2018 18:59

HackerShark pushed a commit to HackerShark/inspec-aws that referenced this pull request Feb 7, 2018

Functional aws_cloudtrail_trail resource (chef-boneyard#186)

ec199bf

Signed-off-by: Rony Xavier <rx294@nyu.edu>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Functional aws_cloudtrail_trail resource #186

Functional aws_cloudtrail_trail resource #186

rx294 commented Jan 17, 2018

clintoncwolfe left a comment

clintoncwolfe Jan 18, 2018

clintoncwolfe Jan 18, 2018

clintoncwolfe Jan 18, 2018

clintoncwolfe Jan 18, 2018

clintoncwolfe Jan 18, 2018

clintoncwolfe Jan 18, 2018

rx294 Jan 18, 2018

clintoncwolfe Jan 18, 2018

rx294 commented Jan 18, 2018

clintoncwolfe commented Jan 19, 2018

rx294 commented Jan 19, 2018 •

edited

Loading

clintoncwolfe Jan 23, 2018

clintoncwolfe Jan 23, 2018

clintoncwolfe commented Jan 23, 2018

rx294 commented Jan 23, 2018

clintoncwolfe commented Jan 23, 2018


		### be_encrypted

		The test will pass if the logs delivered by identified trail is encrypted.


		### be_log_file_validation_enabled

		The test will pass if the identified trail log file integrity validation is enabled.

Functional aws_cloudtrail_trail resource #186

Functional aws_cloudtrail_trail resource #186

Conversation

rx294 commented Jan 17, 2018

clintoncwolfe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rx294 commented Jan 18, 2018

clintoncwolfe commented Jan 19, 2018

rx294 commented Jan 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clintoncwolfe commented Jan 23, 2018

rx294 commented Jan 23, 2018

clintoncwolfe commented Jan 23, 2018

rx294 commented Jan 19, 2018 •

edited

Loading