From 20e5e6c8ddd307fe0e221f3c0cf68e2b0d0e051e Mon Sep 17 00:00:00 2001
From: Rob Taylor <robbytaylor@users.noreply.github.com>
Date: Tue, 24 Sep 2019 13:02:02 +0100
Subject: [PATCH] Fixed IAM permissions

---
 main.tf                                 |  1 +
 pipeline-account/iam_policy_document.tf | 20 +++--------------
 pipeline-account/output.tf              |  8 +++----
 pipeline-account/s3_bucket.tf           | 17 ++++++++++++++
 target-account/iam_policy_document.tf   | 30 ++++++++++++++++++++-----
 target-account/variables.tf             |  4 ++++
 6 files changed, 53 insertions(+), 27 deletions(-)

diff --git a/main.tf b/main.tf
index 59f2fd6..a697631 100644
--- a/main.tf
+++ b/main.tf
@@ -16,6 +16,7 @@ module "target" {
   source = "./target-account"
 
   artifact_bucket_arn   = module.pipeline.artifact_bucket_arn
+  artifact_kms_key_arn  = module.pipeline.artifact_kms_key_arn
   bucket_name           = var.target_bucket_name
   codepipeline_role_arn = module.pipeline.codepipeline_role_arn
 
diff --git a/pipeline-account/iam_policy_document.tf b/pipeline-account/iam_policy_document.tf
index ec496c0..6b21c5e 100644
--- a/pipeline-account/iam_policy_document.tf
+++ b/pipeline-account/iam_policy_document.tf
@@ -80,7 +80,8 @@ data "aws_iam_policy_document" "codepipeline" {
   statement {
     actions = [
       "s3:GetObject",
-      "s3:PutObject"
+      "s3:PutObject",
+      "s3:ListBucket"
     ]
 
     resources = [
@@ -99,21 +100,6 @@ data "aws_iam_policy_document" "codepipeline" {
     resources = ["*"]
   }
 
-  statement {
-    effect = "Allow"
-
-    actions = [
-      "s3:PutObject",
-      "s3:GetObject",
-      "s3:ListBucket",
-    ]
-
-    resources = [
-      "arn:aws:s3:::${var.target_bucket_name}/*",
-      "arn:aws:s3:::${var.target_bucket_name}"
-    ]
-  }
-
   statement {
     effect = "Allow"
 
@@ -123,7 +109,7 @@ data "aws_iam_policy_document" "codepipeline" {
     ]
 
     resources = [
-      var.target_kms_key_arn,
+      aws_kms_key.artifacts.arn,
     ]
   }
 }
diff --git a/pipeline-account/output.tf b/pipeline-account/output.tf
index c3bf8f4..b76cbad 100644
--- a/pipeline-account/output.tf
+++ b/pipeline-account/output.tf
@@ -2,12 +2,12 @@ output "artifact_bucket_arn" {
   value = aws_s3_bucket.artifact-bucket.arn
 }
 
-output "codepipeline_role_arn" {
-  value = aws_iam_role.codepipeline.arn
+output "artifact_kms_key_arn" {
+  value = aws_kms_key.artifacts.arn
 }
 
-output "kms_policy" {
-  value = data.aws_iam_policy_document.kms-usage.json
+output "codepipeline_role_arn" {
+  value = aws_iam_role.codepipeline.arn
 }
 
 output "pipeline_arn" {
diff --git a/pipeline-account/s3_bucket.tf b/pipeline-account/s3_bucket.tf
index 662afd2..4858dc9 100644
--- a/pipeline-account/s3_bucket.tf
+++ b/pipeline-account/s3_bucket.tf
@@ -5,4 +5,21 @@ resource "aws_s3_bucket" "artifact-bucket" {
   versioning {
     enabled = true
   }
+
+  policy = <<POLICY
+{
+  "Version": "2012-10-17",
+  "Statement": [
+    {
+      "Sid": "AllowRoleInTargetAccount",
+      "Effect": "Allow",
+      "Principal": {
+        "AWS": "${var.target_role_arn}"
+      },
+      "Action": "s3:*",
+      "Resource": "arn:aws:s3:::${var.artifact_bucket_name}/*"
+    }
+  ]
+}
+POLICY
 }
diff --git a/target-account/iam_policy_document.tf b/target-account/iam_policy_document.tf
index c8198a1..9921670 100644
--- a/target-account/iam_policy_document.tf
+++ b/target-account/iam_policy_document.tf
@@ -29,7 +29,7 @@ data "aws_iam_policy_document" "kms-usage" {
 
     principals {
       type        = "AWS"
-      identifiers = [var.codepipeline_role_arn]
+      identifiers = [aws_iam_role.target.arn]
     }
 
     resources = ["*"]
@@ -47,7 +47,7 @@ data "aws_iam_policy_document" "kms-usage" {
 
     principals {
       type        = "AWS"
-      identifiers = [var.codepipeline_role_arn]
+      identifiers = [aws_iam_role.target.arn]
     }
 
     resources = ["*"]
@@ -65,11 +65,29 @@ data "aws_iam_policy_document" "target" {
     effect = "Allow"
 
     actions = [
-      "s3:PutObject"
+      "s3:PutObject",
+      "s3:PutObjectAcl",
+      "s3:PutObjectVersionAcl",
+      "s3:GetObject",
+      "s3:ListBucket"
     ]
 
     resources = [
-      "arn:aws:s3:::${aws_s3_bucket.target.arn}/*"
+      aws_s3_bucket.target.arn,
+      "${aws_s3_bucket.target.arn}/*"
+    ]
+  }
+
+  statement {
+    effect = "Allow"
+
+    actions = [
+      "kms:Encrypt",
+      "kms:Decrypt"
+    ]
+
+    resources = [
+      "${aws_kms_key.target-key.arn}"
     ]
   }
 
@@ -81,6 +99,7 @@ data "aws_iam_policy_document" "target" {
     ]
 
     resources = [
+      var.artifact_bucket_arn,
       "${var.artifact_bucket_arn}/*"
     ]
   }
@@ -89,12 +108,11 @@ data "aws_iam_policy_document" "target" {
     effect = "Allow"
 
     actions = [
-      "kms:Encrypt",
       "kms:Decrypt"
     ]
 
     resources = [
-      "${aws_kms_key.target-key.arn}"
+      var.artifact_kms_key_arn
     ]
   }
 }
diff --git a/target-account/variables.tf b/target-account/variables.tf
index a5ed2aa..86ac2b9 100644
--- a/target-account/variables.tf
+++ b/target-account/variables.tf
@@ -2,6 +2,10 @@ variable "artifact_bucket_arn" {
   type = "string"
 }
 
+variable "artifact_kms_key_arn" {
+  type = "string"
+}
+
 variable "bucket_name" {
   type = "string"
 }