From 4318660ee1d731828a6c676b7769b11c077e59e8 Mon Sep 17 00:00:00 2001
From: ian-hoyle <ian.hoyle@nationalarchives.gov.uk>
Date: Mon, 16 Dec 2024 09:12:11 +0000
Subject: [PATCH 01/13] s3 module terraform warnings refactor

---
 s3/main.tf                                | 133 +++++++++++++---------
 s3/templates/alb_logging_euwest2.json.tpl |  14 +++
 s3/templates/cloudfront_origin.json.tpl   |  14 +++
 s3/templates/cloudtrail.json.tpl          |  14 +++
 s3/templates/export_bucket.json.tpl       |  14 +++
 s3/templates/lambda_update.json.tpl       |  14 +++
 s3/templates/log-data.json.tpl            |  14 +++
 s3/templates/secure_transport.json.tpl    |  14 +++
 8 files changed, 177 insertions(+), 54 deletions(-)

diff --git a/s3/main.tf b/s3/main.tf
index 8618609c..90ec0fb2 100644
--- a/s3/main.tf
+++ b/s3/main.tf
@@ -1,13 +1,8 @@
 resource "aws_s3_bucket" "log_bucket" {
   count         = var.access_logs == true && var.apply_resource == true ? 1 : 0
-  acl           = "log-delivery-write"
   bucket        = "${local.bucket_name}-logs"
   force_destroy = var.force_destroy
 
-  versioning {
-    enabled = true
-  }
-
   tags = merge(
     var.common_tags,
     tomap(
@@ -16,6 +11,23 @@ resource "aws_s3_bucket" "log_bucket" {
   )
 }
 
+resource "aws_s3_bucket_versioning" "log_bucket_versioning" {
+  count = var.access_logs == true && var.apply_resource == true ? 1 : 0
+
+  bucket = aws_s3_bucket.log_bucket[0].id
+
+  versioning_configuration {
+    status = "Enabled"
+  }
+}
+
+resource "aws_s3_bucket_acl" "log_bucket_acl" {
+  count = var.access_logs == true && var.apply_resource == true ? 1 : 0
+
+  bucket = aws_s3_bucket.log_bucket[count.index].id
+  acl    = "log-delivery-write"
+}
+
 resource "aws_s3_bucket_server_side_encryption_configuration" "bucket_encryption" {
   bucket = local.s3_bucket_id
 
@@ -41,7 +53,7 @@ resource "aws_s3_bucket_public_access_block" "log_bucket" {
 resource "aws_s3_bucket_policy" "log_bucket" {
   count      = var.access_logs == true && var.apply_resource == true ? 1 : 0
   bucket     = aws_s3_bucket.log_bucket.*.id[0]
-  policy     = templatefile("./tdr-terraform-modules/s3/templates/secure_transport.json.tpl", { bucket_name = aws_s3_bucket.log_bucket.*.id[0] })
+  policy     = templatefile("./tdr-terraform-modules/s3/templates/secure_transport.json.tpl", { bucket_name = aws_s3_bucket.log_bucket.*.id[0], canonical_user_grants = var.canonical_user_grants })
   depends_on = [aws_s3_bucket_public_access_block.log_bucket]
 }
 
@@ -59,60 +71,71 @@ resource "aws_s3_bucket_notification" "log_bucket_notification" {
 resource "aws_s3_bucket" "bucket" {
   count         = var.apply_resource == true ? 1 : 0
   bucket        = local.bucket_name
-  acl           = length(var.canonical_user_grants) == 0 ? var.acl : null
   force_destroy = var.force_destroy
+  tags = merge(
+    var.common_tags,
+    tomap(
+      { "Name" = local.bucket_name }
+    )
+  )
+}
+resource "aws_s3_bucket_acl" "bucket_acl" {
+  count = var.apply_resource == true && length(var.canonical_user_grants) == 0 ? 1 : 0
 
-  dynamic "grant" {
-    for_each = var.canonical_user_grants
-    content {
-      permissions = grant.value.permissions
-      type        = "CanonicalUser"
-      id          = grant.value.id
-    }
-  }
+  bucket = aws_s3_bucket.bucket[0].id
+  acl    = var.acl
+}
 
-  versioning {
-    enabled = var.versioning
-  }
+resource "aws_s3_bucket_versioning" "bucket_versioning" {
+  count = var.apply_resource == true && length(var.canonical_user_grants) == 0 ? 1 : 0
 
-  dynamic "lifecycle_rule" {
-    for_each = var.abort_incomplete_uploads == true ? ["include_block"] : []
-    content {
-      id                                     = "abort-incomplete-uploads"
-      enabled                                = true
-      abort_incomplete_multipart_upload_days = 7
-      expiration {
-        days                         = 0
-        expired_object_delete_marker = false
-      }
-    }
+  bucket = aws_s3_bucket.bucket[0].id
+
+  versioning_configuration {
+    status = "Enabled" # Use "Suspended" to disable versioning
   }
+}
+
+resource "aws_s3_bucket_logging" "bucket_logging" {
+  count = var.access_logs == true && var.apply_resource == true ? 1 : 0
+
+  bucket        = aws_s3_bucket.bucket[0].id
+  target_bucket = aws_s3_bucket.log_bucket[0].id
+  target_prefix = "${local.bucket_name}/${data.aws_caller_identity.current.account_id}/"
+}
 
-  dynamic "logging" {
-    for_each = var.access_logs == true ? ["include_block"] : []
-    content {
-      target_bucket = aws_s3_bucket.log_bucket.*.id[0]
-      target_prefix = "${local.bucket_name}/${data.aws_caller_identity.current.account_id}/"
+resource "aws_s3_bucket_lifecycle_configuration" "bucket_lifecycle" {
+  count = var.apply_resource == true && var.abort_incomplete_uploads == true ? 1 : 0
+
+  bucket = aws_s3_bucket.bucket[0].id
+
+  rule {
+    id     = "abort-incomplete-uploads"
+    status = "Enabled"
+
+    abort_incomplete_multipart_upload {
+      days_after_initiation = 7
     }
-  }
 
-  dynamic "cors_rule" {
-    for_each = length(var.cors_urls) > 0 ? ["include-cors"] : []
-    content {
-      allowed_headers = ["*"]
-      allowed_methods = ["PUT", "POST", "GET"]
-      allowed_origins = var.cors_urls
-      expose_headers  = ["ETag", "x-amz-server-side-encryption", "x-amz-request-id", "x-amz-id-2"]
-      max_age_seconds = 3000
+    expiration {
+      days                         = 0
+      expired_object_delete_marker = false
     }
   }
+}
 
-  tags = merge(
-    var.common_tags,
-    tomap(
-      { "Name" = local.bucket_name }
-    )
-  )
+resource "aws_s3_bucket_cors_configuration" "bucket_cors" {
+  count = var.apply_resource == true && length(var.cors_urls) > 0 ? 1 : 0
+
+  bucket = aws_s3_bucket.bucket[0].id
+
+  cors_rule {
+    allowed_headers = ["*"]
+    allowed_methods = ["PUT", "POST", "GET"]
+    allowed_origins = var.cors_urls
+    expose_headers  = ["ETag", "x-amz-server-side-encryption", "x-amz-request-id", "x-amz-id-2"]
+    max_age_seconds = 3000
+  }
 }
 
 resource "aws_s3_bucket_policy" "bucket" {
@@ -120,11 +143,12 @@ resource "aws_s3_bucket_policy" "bucket" {
   bucket = aws_s3_bucket.bucket.*.id[0]
   policy = local.environment == "mgmt" && contains(["log-data", "lambda_update"], var.bucket_policy) ? templatefile("./tdr-terraform-modules/s3/templates/${var.bucket_policy}.json.tpl",
     {
-      bucket_name        = aws_s3_bucket.bucket.*.id[0],
-      account_id         = data.aws_caller_identity.current.account_id,
-      external_account_1 = data.aws_ssm_parameter.intg_account_number.*.value[0],
-      external_account_2 = data.aws_ssm_parameter.staging_account_number.*.value[0],
-      external_account_3 = data.aws_ssm_parameter.prod_account_number.*.value[0]
+      bucket_name           = aws_s3_bucket.bucket.*.id[0],
+      account_id            = data.aws_caller_identity.current.account_id,
+      external_account_1    = data.aws_ssm_parameter.intg_account_number.*.value[0],
+      external_account_2    = data.aws_ssm_parameter.staging_account_number.*.value[0],
+      external_account_3    = data.aws_ssm_parameter.prod_account_number.*.value[0]
+      canonical_user_grants = var.canonical_user_grants
     }) : templatefile("./tdr-terraform-modules/s3/templates/${var.bucket_policy}.json.tpl",
     {
       bucket_name                  = aws_s3_bucket.bucket.*.id[0],
@@ -134,6 +158,7 @@ resource "aws_s3_bucket_policy" "bucket" {
       environment                  = local.environment, title_environment = title(local.environment),
       read_access_roles            = var.read_access_role_arns,
       cloudfront_distribution_arns = jsonencode(var.cloudfront_distribution_arns)
+      canonical_user_grants        = var.canonical_user_grants
   })
   depends_on = [aws_s3_bucket_public_access_block.bucket]
 }
diff --git a/s3/templates/alb_logging_euwest2.json.tpl b/s3/templates/alb_logging_euwest2.json.tpl
index bbb37612..ddfae636 100644
--- a/s3/templates/alb_logging_euwest2.json.tpl
+++ b/s3/templates/alb_logging_euwest2.json.tpl
@@ -2,6 +2,20 @@
   "Id": "alb-logging-${bucket_name}",
   "Version": "2012-10-17",
   "Statement": [
+    %{ for grant in canonical_user_grants ~}
+    {
+      "Sid": "GrantPermissions-${grant.id}",
+      "Effect": "Allow",
+      "Principal": {
+        "AWS": "${grant.id}"
+      },
+      "Action": ${jsonencode(grant.permissions)},
+      "Resource": [
+        "arn:aws:s3:::${bucket_name}",
+        "arn:aws:s3:::${bucket_name}/*"
+      ]
+    },
+    %{ endfor ~}
     {
       "Effect": "Allow",
       "Principal": {
diff --git a/s3/templates/cloudfront_origin.json.tpl b/s3/templates/cloudfront_origin.json.tpl
index 6cbdeaed..17832c29 100644
--- a/s3/templates/cloudfront_origin.json.tpl
+++ b/s3/templates/cloudfront_origin.json.tpl
@@ -2,6 +2,20 @@
   "Version": "2012-10-17",
   "Id": "CloudfrontUploadBucketPolicy",
   "Statement": [
+    %{ for grant in canonical_user_grants ~}
+    {
+      "Sid": "GrantPermissions-${grant.id}",
+      "Effect": "Allow",
+      "Principal": {
+          "AWS": "${grant.id}"
+      },
+      "Action": ${jsonencode(grant.permissions)},
+      "Resource": [
+          "arn:aws:s3:::${bucket_name}",
+          "arn:aws:s3:::${bucket_name}/*"
+      ]
+    },
+    %{ endfor ~}
     {
       "Effect": "Allow",
       "Principal": {
diff --git a/s3/templates/cloudtrail.json.tpl b/s3/templates/cloudtrail.json.tpl
index e4cb3cc3..efe16b90 100644
--- a/s3/templates/cloudtrail.json.tpl
+++ b/s3/templates/cloudtrail.json.tpl
@@ -1,6 +1,20 @@
 {
   "Version": "2012-10-17",
   "Statement": [
+    %{ for grant in canonical_user_grants ~}
+    {
+      "Sid": "GrantPermissions-${grant.id}",
+      "Effect": "Allow",
+      "Principal": {
+        "AWS": "${grant.id}"
+      },
+      "Action": ${jsonencode(grant.permissions)},
+      "Resource": [
+        "arn:aws:s3:::${bucket_name}",
+        "arn:aws:s3:::${bucket_name}/*"
+      ]
+    },
+    %{ endfor ~}
     {
       "Sid": "AWSCloudTrailAclCheck",
       "Effect": "Allow",
diff --git a/s3/templates/export_bucket.json.tpl b/s3/templates/export_bucket.json.tpl
index 6aa1b561..9f4855e6 100644
--- a/s3/templates/export_bucket.json.tpl
+++ b/s3/templates/export_bucket.json.tpl
@@ -2,6 +2,20 @@
   "Id": "secure-transport-${bucket_name}",
   "Version": "2012-10-17",
   "Statement": [
+    %{ for grant in canonical_user_grants ~}
+      {
+      "Sid": "GrantPermissions-${grant.id}",
+      "Effect": "Allow",
+      "Principal": {
+        "AWS": "${grant.id}"
+      },
+      "Action": ${jsonencode(grant.permissions)},
+      "Resource": [
+        "arn:aws:s3:::${bucket_name}",
+        "arn:aws:s3:::${bucket_name}/*"
+      ]
+    },
+    %{ endfor ~}
     {
       "Sid": "AllowSSLRequestsOnly",
       "Action": "s3:*",
diff --git a/s3/templates/lambda_update.json.tpl b/s3/templates/lambda_update.json.tpl
index 04d98dd1..a8de2608 100644
--- a/s3/templates/lambda_update.json.tpl
+++ b/s3/templates/lambda_update.json.tpl
@@ -2,6 +2,20 @@
   "Version": "2012-10-17",
   "Id": "secure-transport-tdr-backend-code-mgmt",
   "Statement": [
+    %{ for grant in canonical_user_grants ~}
+    {
+      "Sid": "GrantPermissions-${grant.id}",
+      "Effect": "Allow",
+      "Principal": {
+        "AWS": "${grant.id}"
+      },
+      "Action": ${jsonencode(grant.permissions)},
+      "Resource": [
+        "arn:aws:s3:::${bucket_name}",
+        "arn:aws:s3:::${bucket_name}/*"
+      ]
+    },
+    %{ endfor ~}
     {
       "Sid": "AllowSSLRequestsOnly",
       "Effect": "Deny",
diff --git a/s3/templates/log-data.json.tpl b/s3/templates/log-data.json.tpl
index a63cc04f..f7f2cfbc 100644
--- a/s3/templates/log-data.json.tpl
+++ b/s3/templates/log-data.json.tpl
@@ -2,6 +2,20 @@
   "Id": "secure-logging-${bucket_name}",
   "Version": "2012-10-17",
   "Statement": [
+    %{ for grant in canonical_user_grants ~}
+    {
+      "Sid": "GrantPermissions-${grant.id}",
+      "Effect": "Allow",
+      "Principal": {
+        "AWS": "${grant.id}"
+      },
+      "Action": ${jsonencode(grant.permissions)},
+      "Resource": [
+        "arn:aws:s3:::${bucket_name}",
+        "arn:aws:s3:::${bucket_name}/*"
+      ]
+    },
+    %{ endfor ~}
     {
       "Sid": "AllowSSLRequestsOnly",
       "Action": "s3:*",
diff --git a/s3/templates/secure_transport.json.tpl b/s3/templates/secure_transport.json.tpl
index ce4c0fe1..3fd918c3 100644
--- a/s3/templates/secure_transport.json.tpl
+++ b/s3/templates/secure_transport.json.tpl
@@ -2,7 +2,21 @@
   "Id": "secure-transport-${bucket_name}",
   "Version": "2012-10-17",
   "Statement": [
+  %{ for grant in canonical_user_grants ~}
     {
+      "Sid": "GrantPermissions-${grant.id}",
+      "Effect": "Allow",
+      "Principal": {
+        "AWS": "${grant.id}"
+      },
+      "Action": ${jsonencode(grant.permissions)},
+      "Resource": [
+        "arn:aws:s3:::${bucket_name}",
+        "arn:aws:s3:::${bucket_name}/*"
+       ]
+      },
+  %{ endfor ~}
+{
       "Sid": "AllowSSLRequestsOnly",
       "Action": "s3:*",
       "Effect": "Deny",

From 2639284302d8d09a11d8df8cb33b99223e7e59bf Mon Sep 17 00:00:00 2001
From: ian-hoyle <ian.hoyle@nationalarchives.gov.uk>
Date: Mon, 16 Dec 2024 09:27:42 +0000
Subject: [PATCH 02/13] redundant vpc argument

---
 vpc/main.tf | 1 -
 1 file changed, 1 deletion(-)

diff --git a/vpc/main.tf b/vpc/main.tf
index 84617f6b..c7f8c3ec 100644
--- a/vpc/main.tf
+++ b/vpc/main.tf
@@ -65,7 +65,6 @@ resource "aws_route" "internet_access" {
 
 resource "aws_eip" "gw" {
   count      = var.az_count
-  vpc        = true
   depends_on = [aws_internet_gateway.gw]
 
   tags = merge(

From 6053124b7c995a6e572cdf54edfcab50b23e421f Mon Sep 17 00:00:00 2001
From: ian-hoyle <ian.hoyle@nationalarchives.gov.uk>
Date: Mon, 16 Dec 2024 13:22:27 +0000
Subject: [PATCH 03/13] fix apply errors

---
 s3/main.tf                                | 6 ------
 s3/templates/alb_logging_euwest2.json.tpl | 4 ++--
 s3/templates/cloudfront_origin.json.tpl   | 4 ++--
 s3/templates/cloudtrail.json.tpl          | 4 ++--
 s3/templates/export_bucket.json.tpl       | 4 ++--
 s3/templates/lambda_update.json.tpl       | 4 ++--
 s3/templates/log-data.json.tpl            | 4 ++--
 s3/templates/secure_transport.json.tpl    | 4 ++--
 8 files changed, 14 insertions(+), 20 deletions(-)

diff --git a/s3/main.tf b/s3/main.tf
index 90ec0fb2..f021189c 100644
--- a/s3/main.tf
+++ b/s3/main.tf
@@ -79,12 +79,6 @@ resource "aws_s3_bucket" "bucket" {
     )
   )
 }
-resource "aws_s3_bucket_acl" "bucket_acl" {
-  count = var.apply_resource == true && length(var.canonical_user_grants) == 0 ? 1 : 0
-
-  bucket = aws_s3_bucket.bucket[0].id
-  acl    = var.acl
-}
 
 resource "aws_s3_bucket_versioning" "bucket_versioning" {
   count = var.apply_resource == true && length(var.canonical_user_grants) == 0 ? 1 : 0
diff --git a/s3/templates/alb_logging_euwest2.json.tpl b/s3/templates/alb_logging_euwest2.json.tpl
index ddfae636..954eea57 100644
--- a/s3/templates/alb_logging_euwest2.json.tpl
+++ b/s3/templates/alb_logging_euwest2.json.tpl
@@ -9,12 +9,12 @@
       "Principal": {
         "AWS": "${grant.id}"
       },
-      "Action": ${jsonencode(grant.permissions)},
+      "Action": ${grant.permissions},
       "Resource": [
         "arn:aws:s3:::${bucket_name}",
         "arn:aws:s3:::${bucket_name}/*"
       ]
-    },
+    }%{ if for.index != canonical_user_grants.length - 1 ~},%{ endif }
     %{ endfor ~}
     {
       "Effect": "Allow",
diff --git a/s3/templates/cloudfront_origin.json.tpl b/s3/templates/cloudfront_origin.json.tpl
index 17832c29..08617e26 100644
--- a/s3/templates/cloudfront_origin.json.tpl
+++ b/s3/templates/cloudfront_origin.json.tpl
@@ -9,12 +9,12 @@
       "Principal": {
           "AWS": "${grant.id}"
       },
-      "Action": ${jsonencode(grant.permissions)},
+      "Action": ${grant.permissions},
       "Resource": [
           "arn:aws:s3:::${bucket_name}",
           "arn:aws:s3:::${bucket_name}/*"
       ]
-    },
+    }%{ if for.index != canonical_user_grants.length - 1 ~},%{ endif }
     %{ endfor ~}
     {
       "Effect": "Allow",
diff --git a/s3/templates/cloudtrail.json.tpl b/s3/templates/cloudtrail.json.tpl
index efe16b90..020fa244 100644
--- a/s3/templates/cloudtrail.json.tpl
+++ b/s3/templates/cloudtrail.json.tpl
@@ -8,12 +8,12 @@
       "Principal": {
         "AWS": "${grant.id}"
       },
-      "Action": ${jsonencode(grant.permissions)},
+      "Action": ${grant.permissions},
       "Resource": [
         "arn:aws:s3:::${bucket_name}",
         "arn:aws:s3:::${bucket_name}/*"
       ]
-    },
+    }%{ if for.index != canonical_user_grants.length - 1 ~},%{ endif }
     %{ endfor ~}
     {
       "Sid": "AWSCloudTrailAclCheck",
diff --git a/s3/templates/export_bucket.json.tpl b/s3/templates/export_bucket.json.tpl
index 9f4855e6..c9a94a0e 100644
--- a/s3/templates/export_bucket.json.tpl
+++ b/s3/templates/export_bucket.json.tpl
@@ -9,12 +9,12 @@
       "Principal": {
         "AWS": "${grant.id}"
       },
-      "Action": ${jsonencode(grant.permissions)},
+      "Action": ${grant.permissions},
       "Resource": [
         "arn:aws:s3:::${bucket_name}",
         "arn:aws:s3:::${bucket_name}/*"
       ]
-    },
+    }%{ if for.index != canonical_user_grants.length - 1 ~},%{ endif }
     %{ endfor ~}
     {
       "Sid": "AllowSSLRequestsOnly",
diff --git a/s3/templates/lambda_update.json.tpl b/s3/templates/lambda_update.json.tpl
index a8de2608..cd4f5dc6 100644
--- a/s3/templates/lambda_update.json.tpl
+++ b/s3/templates/lambda_update.json.tpl
@@ -9,12 +9,12 @@
       "Principal": {
         "AWS": "${grant.id}"
       },
-      "Action": ${jsonencode(grant.permissions)},
+      "Action": ${grant.permissions},
       "Resource": [
         "arn:aws:s3:::${bucket_name}",
         "arn:aws:s3:::${bucket_name}/*"
       ]
-    },
+    }%{ if for.index != canonical_user_grants.length - 1 ~},%{ endif }
     %{ endfor ~}
     {
       "Sid": "AllowSSLRequestsOnly",
diff --git a/s3/templates/log-data.json.tpl b/s3/templates/log-data.json.tpl
index f7f2cfbc..42340bb5 100644
--- a/s3/templates/log-data.json.tpl
+++ b/s3/templates/log-data.json.tpl
@@ -9,12 +9,12 @@
       "Principal": {
         "AWS": "${grant.id}"
       },
-      "Action": ${jsonencode(grant.permissions)},
+      "Action": ${grant.permissions},
       "Resource": [
         "arn:aws:s3:::${bucket_name}",
         "arn:aws:s3:::${bucket_name}/*"
       ]
-    },
+    }%{ if for.index != canonical_user_grants.length - 1 ~},%{ endif }
     %{ endfor ~}
     {
       "Sid": "AllowSSLRequestsOnly",
diff --git a/s3/templates/secure_transport.json.tpl b/s3/templates/secure_transport.json.tpl
index 3fd918c3..cf2cb045 100644
--- a/s3/templates/secure_transport.json.tpl
+++ b/s3/templates/secure_transport.json.tpl
@@ -9,12 +9,12 @@
       "Principal": {
         "AWS": "${grant.id}"
       },
-      "Action": ${jsonencode(grant.permissions)},
+      "Action": ${grant.permissions},
       "Resource": [
         "arn:aws:s3:::${bucket_name}",
         "arn:aws:s3:::${bucket_name}/*"
        ]
-      },
+      }%{ if for.index != canonical_user_grants.length - 1 ~},%{ endif }
   %{ endfor ~}
 {
       "Sid": "AllowSSLRequestsOnly",

From db42514c0ea12b7e1398ae1c30a80fb4bcccf571 Mon Sep 17 00:00:00 2001
From: ian-hoyle <ian.hoyle@nationalarchives.gov.uk>
Date: Mon, 16 Dec 2024 16:40:22 +0000
Subject: [PATCH 04/13] revert

---
 s3/main.tf                                | 26 +++++++++++++++++++++++
 s3/templates/alb_logging_euwest2.json.tpl |  4 ++--
 s3/templates/cloudfront_origin.json.tpl   |  4 ++--
 s3/templates/cloudtrail.json.tpl          |  4 ++--
 s3/templates/export_bucket.json.tpl       |  4 ++--
 s3/templates/lambda_update.json.tpl       |  4 ++--
 s3/templates/log-data.json.tpl            |  4 ++--
 s3/templates/secure_transport.json.tpl    |  6 +++---
 8 files changed, 41 insertions(+), 15 deletions(-)

diff --git a/s3/main.tf b/s3/main.tf
index f021189c..3374ff07 100644
--- a/s3/main.tf
+++ b/s3/main.tf
@@ -131,6 +131,29 @@ resource "aws_s3_bucket_cors_configuration" "bucket_cors" {
     max_age_seconds = 3000
   }
 }
+# resource "aws_s3_bucket_policy" "bucket_canonical_grants" {
+#   count  = var.apply_resource == true && length(var.canonical_user_grants) > 0 ? 1 : 0
+#   bucket = aws_s3_bucket.bucket[0].id
+#
+#   policy = jsonencode({
+#     Version = "2012-10-17"
+#     Statement = [
+#       for grant in var.canonical_user_grants : {
+#         Sid       = "GrantPermissions-${grant.id}"
+#         Effect    = "Allow"
+#         Principal = {
+#           AWS = grant.id
+#         }
+#         Action   = grant.permissions
+#         Resource = [
+#           "arn:aws:s3:::${aws_s3_bucket.bucket[0].id}",
+#           "arn:aws:s3:::${aws_s3_bucket.bucket[0].id}/*"
+#         ]
+#       }
+#     ]
+#   })
+# }
+
 
 resource "aws_s3_bucket_policy" "bucket" {
   count  = var.apply_resource == true ? 1 : 0
@@ -156,6 +179,9 @@ resource "aws_s3_bucket_policy" "bucket" {
   })
   depends_on = [aws_s3_bucket_public_access_block.bucket]
 }
+output "canonical_user_grants_debug" {
+  value = var.canonical_user_grants
+}
 
 resource "aws_s3_bucket_public_access_block" "bucket" {
   count                   = var.apply_resource == true ? 1 : 0
diff --git a/s3/templates/alb_logging_euwest2.json.tpl b/s3/templates/alb_logging_euwest2.json.tpl
index 954eea57..ddfae636 100644
--- a/s3/templates/alb_logging_euwest2.json.tpl
+++ b/s3/templates/alb_logging_euwest2.json.tpl
@@ -9,12 +9,12 @@
       "Principal": {
         "AWS": "${grant.id}"
       },
-      "Action": ${grant.permissions},
+      "Action": ${jsonencode(grant.permissions)},
       "Resource": [
         "arn:aws:s3:::${bucket_name}",
         "arn:aws:s3:::${bucket_name}/*"
       ]
-    }%{ if for.index != canonical_user_grants.length - 1 ~},%{ endif }
+    },
     %{ endfor ~}
     {
       "Effect": "Allow",
diff --git a/s3/templates/cloudfront_origin.json.tpl b/s3/templates/cloudfront_origin.json.tpl
index 08617e26..17832c29 100644
--- a/s3/templates/cloudfront_origin.json.tpl
+++ b/s3/templates/cloudfront_origin.json.tpl
@@ -9,12 +9,12 @@
       "Principal": {
           "AWS": "${grant.id}"
       },
-      "Action": ${grant.permissions},
+      "Action": ${jsonencode(grant.permissions)},
       "Resource": [
           "arn:aws:s3:::${bucket_name}",
           "arn:aws:s3:::${bucket_name}/*"
       ]
-    }%{ if for.index != canonical_user_grants.length - 1 ~},%{ endif }
+    },
     %{ endfor ~}
     {
       "Effect": "Allow",
diff --git a/s3/templates/cloudtrail.json.tpl b/s3/templates/cloudtrail.json.tpl
index 020fa244..efe16b90 100644
--- a/s3/templates/cloudtrail.json.tpl
+++ b/s3/templates/cloudtrail.json.tpl
@@ -8,12 +8,12 @@
       "Principal": {
         "AWS": "${grant.id}"
       },
-      "Action": ${grant.permissions},
+      "Action": ${jsonencode(grant.permissions)},
       "Resource": [
         "arn:aws:s3:::${bucket_name}",
         "arn:aws:s3:::${bucket_name}/*"
       ]
-    }%{ if for.index != canonical_user_grants.length - 1 ~},%{ endif }
+    },
     %{ endfor ~}
     {
       "Sid": "AWSCloudTrailAclCheck",
diff --git a/s3/templates/export_bucket.json.tpl b/s3/templates/export_bucket.json.tpl
index c9a94a0e..9f4855e6 100644
--- a/s3/templates/export_bucket.json.tpl
+++ b/s3/templates/export_bucket.json.tpl
@@ -9,12 +9,12 @@
       "Principal": {
         "AWS": "${grant.id}"
       },
-      "Action": ${grant.permissions},
+      "Action": ${jsonencode(grant.permissions)},
       "Resource": [
         "arn:aws:s3:::${bucket_name}",
         "arn:aws:s3:::${bucket_name}/*"
       ]
-    }%{ if for.index != canonical_user_grants.length - 1 ~},%{ endif }
+    },
     %{ endfor ~}
     {
       "Sid": "AllowSSLRequestsOnly",
diff --git a/s3/templates/lambda_update.json.tpl b/s3/templates/lambda_update.json.tpl
index cd4f5dc6..a8de2608 100644
--- a/s3/templates/lambda_update.json.tpl
+++ b/s3/templates/lambda_update.json.tpl
@@ -9,12 +9,12 @@
       "Principal": {
         "AWS": "${grant.id}"
       },
-      "Action": ${grant.permissions},
+      "Action": ${jsonencode(grant.permissions)},
       "Resource": [
         "arn:aws:s3:::${bucket_name}",
         "arn:aws:s3:::${bucket_name}/*"
       ]
-    }%{ if for.index != canonical_user_grants.length - 1 ~},%{ endif }
+    },
     %{ endfor ~}
     {
       "Sid": "AllowSSLRequestsOnly",
diff --git a/s3/templates/log-data.json.tpl b/s3/templates/log-data.json.tpl
index 42340bb5..f7f2cfbc 100644
--- a/s3/templates/log-data.json.tpl
+++ b/s3/templates/log-data.json.tpl
@@ -9,12 +9,12 @@
       "Principal": {
         "AWS": "${grant.id}"
       },
-      "Action": ${grant.permissions},
+      "Action": ${jsonencode(grant.permissions)},
       "Resource": [
         "arn:aws:s3:::${bucket_name}",
         "arn:aws:s3:::${bucket_name}/*"
       ]
-    }%{ if for.index != canonical_user_grants.length - 1 ~},%{ endif }
+    },
     %{ endfor ~}
     {
       "Sid": "AllowSSLRequestsOnly",
diff --git a/s3/templates/secure_transport.json.tpl b/s3/templates/secure_transport.json.tpl
index cf2cb045..cbb48203 100644
--- a/s3/templates/secure_transport.json.tpl
+++ b/s3/templates/secure_transport.json.tpl
@@ -9,12 +9,12 @@
       "Principal": {
         "AWS": "${grant.id}"
       },
-      "Action": ${grant.permissions},
+      "Action": ${jsonencode(grant.permissions)},
       "Resource": [
         "arn:aws:s3:::${bucket_name}",
         "arn:aws:s3:::${bucket_name}/*"
-       ]
-      }%{ if for.index != canonical_user_grants.length - 1 ~},%{ endif }
+      ]
+    },
   %{ endfor ~}
 {
       "Sid": "AllowSSLRequestsOnly",

From 235fec5c9ce106cb4b46501d51a8ed913c90d37a Mon Sep 17 00:00:00 2001
From: ian-hoyle <ian.hoyle@nationalarchives.gov.uk>
Date: Thu, 19 Dec 2024 12:01:30 +0000
Subject: [PATCH 05/13] trying to fix

---
 s3/templates/alb_logging_euwest2.json.tpl | 2 +-
 s3/templates/cloudfront_origin.json.tpl   | 2 +-
 s3/templates/cloudtrail.json.tpl          | 2 +-
 s3/templates/export_bucket.json.tpl       | 2 +-
 s3/templates/lambda_update.json.tpl       | 2 +-
 s3/templates/log-data.json.tpl            | 2 +-
 s3/templates/secure_transport.json.tpl    | 2 +-
 7 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/s3/templates/alb_logging_euwest2.json.tpl b/s3/templates/alb_logging_euwest2.json.tpl
index ddfae636..fd51eeb4 100644
--- a/s3/templates/alb_logging_euwest2.json.tpl
+++ b/s3/templates/alb_logging_euwest2.json.tpl
@@ -9,7 +9,7 @@
       "Principal": {
         "AWS": "${grant.id}"
       },
-      "Action": ${jsonencode(grant.permissions)},
+      "Action": ${grant.permissions},
       "Resource": [
         "arn:aws:s3:::${bucket_name}",
         "arn:aws:s3:::${bucket_name}/*"
diff --git a/s3/templates/cloudfront_origin.json.tpl b/s3/templates/cloudfront_origin.json.tpl
index 17832c29..51183d8c 100644
--- a/s3/templates/cloudfront_origin.json.tpl
+++ b/s3/templates/cloudfront_origin.json.tpl
@@ -9,7 +9,7 @@
       "Principal": {
           "AWS": "${grant.id}"
       },
-      "Action": ${jsonencode(grant.permissions)},
+      "Action": ${grant.permissions},
       "Resource": [
           "arn:aws:s3:::${bucket_name}",
           "arn:aws:s3:::${bucket_name}/*"
diff --git a/s3/templates/cloudtrail.json.tpl b/s3/templates/cloudtrail.json.tpl
index efe16b90..a2864b26 100644
--- a/s3/templates/cloudtrail.json.tpl
+++ b/s3/templates/cloudtrail.json.tpl
@@ -8,7 +8,7 @@
       "Principal": {
         "AWS": "${grant.id}"
       },
-      "Action": ${jsonencode(grant.permissions)},
+      "Action": ${grant.permissions},
       "Resource": [
         "arn:aws:s3:::${bucket_name}",
         "arn:aws:s3:::${bucket_name}/*"
diff --git a/s3/templates/export_bucket.json.tpl b/s3/templates/export_bucket.json.tpl
index 9f4855e6..73f59006 100644
--- a/s3/templates/export_bucket.json.tpl
+++ b/s3/templates/export_bucket.json.tpl
@@ -9,7 +9,7 @@
       "Principal": {
         "AWS": "${grant.id}"
       },
-      "Action": ${jsonencode(grant.permissions)},
+      "Action": ${grant.permissions},
       "Resource": [
         "arn:aws:s3:::${bucket_name}",
         "arn:aws:s3:::${bucket_name}/*"
diff --git a/s3/templates/lambda_update.json.tpl b/s3/templates/lambda_update.json.tpl
index a8de2608..00a350b6 100644
--- a/s3/templates/lambda_update.json.tpl
+++ b/s3/templates/lambda_update.json.tpl
@@ -9,7 +9,7 @@
       "Principal": {
         "AWS": "${grant.id}"
       },
-      "Action": ${jsonencode(grant.permissions)},
+      "Action": ${grant.permissions},
       "Resource": [
         "arn:aws:s3:::${bucket_name}",
         "arn:aws:s3:::${bucket_name}/*"
diff --git a/s3/templates/log-data.json.tpl b/s3/templates/log-data.json.tpl
index f7f2cfbc..c349e01b 100644
--- a/s3/templates/log-data.json.tpl
+++ b/s3/templates/log-data.json.tpl
@@ -9,7 +9,7 @@
       "Principal": {
         "AWS": "${grant.id}"
       },
-      "Action": ${jsonencode(grant.permissions)},
+      "Action": ${grant.permissions},
       "Resource": [
         "arn:aws:s3:::${bucket_name}",
         "arn:aws:s3:::${bucket_name}/*"
diff --git a/s3/templates/secure_transport.json.tpl b/s3/templates/secure_transport.json.tpl
index 3fd918c3..566478e6 100644
--- a/s3/templates/secure_transport.json.tpl
+++ b/s3/templates/secure_transport.json.tpl
@@ -9,7 +9,7 @@
       "Principal": {
         "AWS": "${grant.id}"
       },
-      "Action": ${jsonencode(grant.permissions)},
+      "Action": ${grant.permissions},
       "Resource": [
         "arn:aws:s3:::${bucket_name}",
         "arn:aws:s3:::${bucket_name}/*"

From 1f5312e3e163059ee12718032798edbe6ba25c20 Mon Sep 17 00:00:00 2001
From: ian-hoyle <ian.hoyle@nationalarchives.gov.uk>
Date: Thu, 19 Dec 2024 13:39:41 +0000
Subject: [PATCH 06/13] resolved terraform problems

---
 s3/main.tf                                | 4 ++--
 s3/templates/alb_logging_euwest2.json.tpl | 2 +-
 s3/templates/cloudfront_origin.json.tpl   | 2 +-
 s3/templates/cloudtrail.json.tpl          | 2 +-
 s3/templates/export_bucket.json.tpl       | 2 +-
 s3/templates/lambda_update.json.tpl       | 2 +-
 s3/templates/log-data.json.tpl            | 2 +-
 s3/templates/secure_transport.json.tpl    | 4 ++--
 8 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/s3/main.tf b/s3/main.tf
index 3374ff07..407a033a 100644
--- a/s3/main.tf
+++ b/s3/main.tf
@@ -53,7 +53,7 @@ resource "aws_s3_bucket_public_access_block" "log_bucket" {
 resource "aws_s3_bucket_policy" "log_bucket" {
   count      = var.access_logs == true && var.apply_resource == true ? 1 : 0
   bucket     = aws_s3_bucket.log_bucket.*.id[0]
-  policy     = templatefile("./tdr-terraform-modules/s3/templates/secure_transport.json.tpl", { bucket_name = aws_s3_bucket.log_bucket.*.id[0], canonical_user_grants = var.canonical_user_grants })
+  policy     = templatefile("./tdr-terraform-modules/s3/templates/secure_transport.json.tpl", { bucket_name = aws_s3_bucket.log_bucket.*.id[0], canonical_user_grants = jsonencode(var.canonical_user_grants) })
   depends_on = [aws_s3_bucket_public_access_block.log_bucket]
 }
 
@@ -175,7 +175,7 @@ resource "aws_s3_bucket_policy" "bucket" {
       environment                  = local.environment, title_environment = title(local.environment),
       read_access_roles            = var.read_access_role_arns,
       cloudfront_distribution_arns = jsonencode(var.cloudfront_distribution_arns)
-      canonical_user_grants        = var.canonical_user_grants
+      canonical_user_grants        = jsonencode(var.canonical_user_grants)
   })
   depends_on = [aws_s3_bucket_public_access_block.bucket]
 }
diff --git a/s3/templates/alb_logging_euwest2.json.tpl b/s3/templates/alb_logging_euwest2.json.tpl
index fd51eeb4..88976fde 100644
--- a/s3/templates/alb_logging_euwest2.json.tpl
+++ b/s3/templates/alb_logging_euwest2.json.tpl
@@ -2,7 +2,7 @@
   "Id": "alb-logging-${bucket_name}",
   "Version": "2012-10-17",
   "Statement": [
-    %{ for grant in canonical_user_grants ~}
+    %{ for grant in jsondecode(canonical_user_grants) ~}
     {
       "Sid": "GrantPermissions-${grant.id}",
       "Effect": "Allow",
diff --git a/s3/templates/cloudfront_origin.json.tpl b/s3/templates/cloudfront_origin.json.tpl
index 51183d8c..195b1173 100644
--- a/s3/templates/cloudfront_origin.json.tpl
+++ b/s3/templates/cloudfront_origin.json.tpl
@@ -2,7 +2,7 @@
   "Version": "2012-10-17",
   "Id": "CloudfrontUploadBucketPolicy",
   "Statement": [
-    %{ for grant in canonical_user_grants ~}
+    %{ for grant in jsondecode(canonical_user_grants) ~}
     {
       "Sid": "GrantPermissions-${grant.id}",
       "Effect": "Allow",
diff --git a/s3/templates/cloudtrail.json.tpl b/s3/templates/cloudtrail.json.tpl
index a2864b26..1e7a1e0d 100644
--- a/s3/templates/cloudtrail.json.tpl
+++ b/s3/templates/cloudtrail.json.tpl
@@ -1,7 +1,7 @@
 {
   "Version": "2012-10-17",
   "Statement": [
-    %{ for grant in canonical_user_grants ~}
+    %{ for grant in jsondecode(canonical_user_grants) ~}
     {
       "Sid": "GrantPermissions-${grant.id}",
       "Effect": "Allow",
diff --git a/s3/templates/export_bucket.json.tpl b/s3/templates/export_bucket.json.tpl
index 73f59006..c8f21e8b 100644
--- a/s3/templates/export_bucket.json.tpl
+++ b/s3/templates/export_bucket.json.tpl
@@ -2,7 +2,7 @@
   "Id": "secure-transport-${bucket_name}",
   "Version": "2012-10-17",
   "Statement": [
-    %{ for grant in canonical_user_grants ~}
+    %{ for grant in jsondecode(canonical_user_grants) ~}
       {
       "Sid": "GrantPermissions-${grant.id}",
       "Effect": "Allow",
diff --git a/s3/templates/lambda_update.json.tpl b/s3/templates/lambda_update.json.tpl
index 00a350b6..1872b64b 100644
--- a/s3/templates/lambda_update.json.tpl
+++ b/s3/templates/lambda_update.json.tpl
@@ -2,7 +2,7 @@
   "Version": "2012-10-17",
   "Id": "secure-transport-tdr-backend-code-mgmt",
   "Statement": [
-    %{ for grant in canonical_user_grants ~}
+    %{ for grant in jsondecode(canonical_user_grants) ~}
     {
       "Sid": "GrantPermissions-${grant.id}",
       "Effect": "Allow",
diff --git a/s3/templates/log-data.json.tpl b/s3/templates/log-data.json.tpl
index c349e01b..498e5255 100644
--- a/s3/templates/log-data.json.tpl
+++ b/s3/templates/log-data.json.tpl
@@ -2,7 +2,7 @@
   "Id": "secure-logging-${bucket_name}",
   "Version": "2012-10-17",
   "Statement": [
-    %{ for grant in canonical_user_grants ~}
+    %{ for grant in jsondecode(canonical_user_grants) ~}
     {
       "Sid": "GrantPermissions-${grant.id}",
       "Effect": "Allow",
diff --git a/s3/templates/secure_transport.json.tpl b/s3/templates/secure_transport.json.tpl
index 69f6a66f..4f6289d2 100644
--- a/s3/templates/secure_transport.json.tpl
+++ b/s3/templates/secure_transport.json.tpl
@@ -2,14 +2,14 @@
   "Id": "secure-transport-${bucket_name}",
   "Version": "2012-10-17",
   "Statement": [
-  %{ for grant in canonical_user_grants ~}
+  %{ for grant in jsondecode(canonical_user_grants) ~}
     {
       "Sid": "GrantPermissions-${grant.id}",
       "Effect": "Allow",
       "Principal": {
         "AWS": "${grant.id}"
       },
-      "Action": ${grant.permissions},
+      "Action": ${jsonencode(grant.permissions)},
       "Resource": [
         "arn:aws:s3:::${bucket_name}",
         "arn:aws:s3:::${bucket_name}/*"

From fba0f4dd15a1151c70f7a4d59d5a3c5b4c59a506 Mon Sep 17 00:00:00 2001
From: ian-hoyle <ian.hoyle@nationalarchives.gov.uk>
Date: Thu, 19 Dec 2024 13:55:24 +0000
Subject: [PATCH 07/13] set eip domain='vpc'

---
 vpc/main.tf | 1 +
 1 file changed, 1 insertion(+)

diff --git a/vpc/main.tf b/vpc/main.tf
index c7f8c3ec..16983051 100644
--- a/vpc/main.tf
+++ b/vpc/main.tf
@@ -65,6 +65,7 @@ resource "aws_route" "internet_access" {
 
 resource "aws_eip" "gw" {
   count      = var.az_count
+  domain     = "vpc"
   depends_on = [aws_internet_gateway.gw]
 
   tags = merge(

From cde87dde80310a0aa68296a293b47c53b4515053 Mon Sep 17 00:00:00 2001
From: ian-hoyle <ian.hoyle@nationalarchives.gov.uk>
Date: Thu, 19 Dec 2024 14:29:14 +0000
Subject: [PATCH 08/13] all templates consistent

---
 s3/templates/alb_logging_euwest2.json.tpl | 2 +-
 s3/templates/cloudfront_origin.json.tpl   | 2 +-
 s3/templates/cloudtrail.json.tpl          | 2 +-
 s3/templates/export_bucket.json.tpl       | 2 +-
 s3/templates/lambda_update.json.tpl       | 2 +-
 s3/templates/log-data.json.tpl            | 2 +-
 s3/templates/secure_transport.json.tpl    | 4 ++--
 7 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/s3/templates/alb_logging_euwest2.json.tpl b/s3/templates/alb_logging_euwest2.json.tpl
index 88976fde..154a05e8 100644
--- a/s3/templates/alb_logging_euwest2.json.tpl
+++ b/s3/templates/alb_logging_euwest2.json.tpl
@@ -9,7 +9,7 @@
       "Principal": {
         "AWS": "${grant.id}"
       },
-      "Action": ${grant.permissions},
+      "Action": ${jsonencode(grant.permissions)},
       "Resource": [
         "arn:aws:s3:::${bucket_name}",
         "arn:aws:s3:::${bucket_name}/*"
diff --git a/s3/templates/cloudfront_origin.json.tpl b/s3/templates/cloudfront_origin.json.tpl
index 195b1173..7ec56fa0 100644
--- a/s3/templates/cloudfront_origin.json.tpl
+++ b/s3/templates/cloudfront_origin.json.tpl
@@ -9,7 +9,7 @@
       "Principal": {
           "AWS": "${grant.id}"
       },
-      "Action": ${grant.permissions},
+      "Action": ${jsonencode(grant.permissions)},
       "Resource": [
           "arn:aws:s3:::${bucket_name}",
           "arn:aws:s3:::${bucket_name}/*"
diff --git a/s3/templates/cloudtrail.json.tpl b/s3/templates/cloudtrail.json.tpl
index 1e7a1e0d..2d8de4a8 100644
--- a/s3/templates/cloudtrail.json.tpl
+++ b/s3/templates/cloudtrail.json.tpl
@@ -8,7 +8,7 @@
       "Principal": {
         "AWS": "${grant.id}"
       },
-      "Action": ${grant.permissions},
+      "Action": ${jsonencode(grant.permissions)},
       "Resource": [
         "arn:aws:s3:::${bucket_name}",
         "arn:aws:s3:::${bucket_name}/*"
diff --git a/s3/templates/export_bucket.json.tpl b/s3/templates/export_bucket.json.tpl
index c8f21e8b..70535f69 100644
--- a/s3/templates/export_bucket.json.tpl
+++ b/s3/templates/export_bucket.json.tpl
@@ -9,7 +9,7 @@
       "Principal": {
         "AWS": "${grant.id}"
       },
-      "Action": ${grant.permissions},
+      "Action": ${jsonencode(grant.permissions)},
       "Resource": [
         "arn:aws:s3:::${bucket_name}",
         "arn:aws:s3:::${bucket_name}/*"
diff --git a/s3/templates/lambda_update.json.tpl b/s3/templates/lambda_update.json.tpl
index 1872b64b..6d78b6ee 100644
--- a/s3/templates/lambda_update.json.tpl
+++ b/s3/templates/lambda_update.json.tpl
@@ -9,7 +9,7 @@
       "Principal": {
         "AWS": "${grant.id}"
       },
-      "Action": ${grant.permissions},
+      "Action": ${jsonencode(grant.permissions)},
       "Resource": [
         "arn:aws:s3:::${bucket_name}",
         "arn:aws:s3:::${bucket_name}/*"
diff --git a/s3/templates/log-data.json.tpl b/s3/templates/log-data.json.tpl
index 498e5255..110af337 100644
--- a/s3/templates/log-data.json.tpl
+++ b/s3/templates/log-data.json.tpl
@@ -9,7 +9,7 @@
       "Principal": {
         "AWS": "${grant.id}"
       },
-      "Action": ${grant.permissions},
+      "Action": ${jsonencode(grant.permissions)},
       "Resource": [
         "arn:aws:s3:::${bucket_name}",
         "arn:aws:s3:::${bucket_name}/*"
diff --git a/s3/templates/secure_transport.json.tpl b/s3/templates/secure_transport.json.tpl
index 4f6289d2..eb77c169 100644
--- a/s3/templates/secure_transport.json.tpl
+++ b/s3/templates/secure_transport.json.tpl
@@ -16,7 +16,7 @@
       ]
     },
   %{ endfor ~}
-{
+   {
       "Sid": "AllowSSLRequestsOnly",
       "Action": "s3:*",
       "Effect": "Deny",
@@ -30,6 +30,6 @@
         }
       },
       "Principal": "*"
-    }
+   }
   ]
 }
\ No newline at end of file

From cab3c800824e42038136199f7990cfd29eccd99e Mon Sep 17 00:00:00 2001
From: ian-hoyle <ian.hoyle@nationalarchives.gov.uk>
Date: Thu, 19 Dec 2024 14:41:36 +0000
Subject: [PATCH 09/13] remove redundant code

---
 s3/main.tf | 28 +---------------------------
 1 file changed, 1 insertion(+), 27 deletions(-)

diff --git a/s3/main.tf b/s3/main.tf
index 407a033a..258c11cc 100644
--- a/s3/main.tf
+++ b/s3/main.tf
@@ -131,29 +131,6 @@ resource "aws_s3_bucket_cors_configuration" "bucket_cors" {
     max_age_seconds = 3000
   }
 }
-# resource "aws_s3_bucket_policy" "bucket_canonical_grants" {
-#   count  = var.apply_resource == true && length(var.canonical_user_grants) > 0 ? 1 : 0
-#   bucket = aws_s3_bucket.bucket[0].id
-#
-#   policy = jsonencode({
-#     Version = "2012-10-17"
-#     Statement = [
-#       for grant in var.canonical_user_grants : {
-#         Sid       = "GrantPermissions-${grant.id}"
-#         Effect    = "Allow"
-#         Principal = {
-#           AWS = grant.id
-#         }
-#         Action   = grant.permissions
-#         Resource = [
-#           "arn:aws:s3:::${aws_s3_bucket.bucket[0].id}",
-#           "arn:aws:s3:::${aws_s3_bucket.bucket[0].id}/*"
-#         ]
-#       }
-#     ]
-#   })
-# }
-
 
 resource "aws_s3_bucket_policy" "bucket" {
   count  = var.apply_resource == true ? 1 : 0
@@ -165,7 +142,7 @@ resource "aws_s3_bucket_policy" "bucket" {
       external_account_1    = data.aws_ssm_parameter.intg_account_number.*.value[0],
       external_account_2    = data.aws_ssm_parameter.staging_account_number.*.value[0],
       external_account_3    = data.aws_ssm_parameter.prod_account_number.*.value[0]
-      canonical_user_grants = var.canonical_user_grants
+      canonical_user_grants = jsonencode(var.canonical_user_grants)
     }) : templatefile("./tdr-terraform-modules/s3/templates/${var.bucket_policy}.json.tpl",
     {
       bucket_name                  = aws_s3_bucket.bucket.*.id[0],
@@ -179,9 +156,6 @@ resource "aws_s3_bucket_policy" "bucket" {
   })
   depends_on = [aws_s3_bucket_public_access_block.bucket]
 }
-output "canonical_user_grants_debug" {
-  value = var.canonical_user_grants
-}
 
 resource "aws_s3_bucket_public_access_block" "bucket" {
   count                   = var.apply_resource == true ? 1 : 0

From 8c15972904bef32bd3ee1d5277a4a511217110d4 Mon Sep 17 00:00:00 2001
From: ian-hoyle <ian.hoyle@nationalarchives.gov.uk>
Date: Thu, 19 Dec 2024 14:47:22 +0000
Subject: [PATCH 10/13] make legible

---
 s3/main.tf | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/s3/main.tf b/s3/main.tf
index 258c11cc..f98aca78 100644
--- a/s3/main.tf
+++ b/s3/main.tf
@@ -53,7 +53,11 @@ resource "aws_s3_bucket_public_access_block" "log_bucket" {
 resource "aws_s3_bucket_policy" "log_bucket" {
   count      = var.access_logs == true && var.apply_resource == true ? 1 : 0
   bucket     = aws_s3_bucket.log_bucket.*.id[0]
-  policy     = templatefile("./tdr-terraform-modules/s3/templates/secure_transport.json.tpl", { bucket_name = aws_s3_bucket.log_bucket.*.id[0], canonical_user_grants = jsonencode(var.canonical_user_grants) })
+  policy     = templatefile("./tdr-terraform-modules/s3/templates/secure_transport.json.tpl",
+    {
+      bucket_name = aws_s3_bucket.log_bucket.*.id[0],
+      canonical_user_grants = jsonencode(var.canonical_user_grants)
+    })
   depends_on = [aws_s3_bucket_public_access_block.log_bucket]
 }
 

From 35ae4f7c652c2d764f82391e28c17091676348b4 Mon Sep 17 00:00:00 2001
From: ian-hoyle <ian.hoyle@nationalarchives.gov.uk>
Date: Fri, 20 Dec 2024 11:01:39 +0000
Subject: [PATCH 11/13] resolving grants warning on s3

---
 s3/main.tf                                | 14 ++++++++------
 s3/templates/alb_logging_euwest2.json.tpl | 14 --------------
 s3/templates/cloudfront_origin.json.tpl   | 14 --------------
 s3/templates/cloudtrail.json.tpl          | 16 +---------------
 s3/templates/export_bucket.json.tpl       | 14 --------------
 s3/templates/lambda_update.json.tpl       | 14 --------------
 s3/templates/log-data.json.tpl            | 14 --------------
 s3/templates/secure_transport.json.tpl    | 13 +++++++++++--
 s3/variables.tf                           |  3 ++-
 9 files changed, 22 insertions(+), 94 deletions(-)

diff --git a/s3/main.tf b/s3/main.tf
index f98aca78..10e12d78 100644
--- a/s3/main.tf
+++ b/s3/main.tf
@@ -51,13 +51,13 @@ resource "aws_s3_bucket_public_access_block" "log_bucket" {
 }
 
 resource "aws_s3_bucket_policy" "log_bucket" {
-  count      = var.access_logs == true && var.apply_resource == true ? 1 : 0
-  bucket     = aws_s3_bucket.log_bucket.*.id[0]
-  policy     = templatefile("./tdr-terraform-modules/s3/templates/secure_transport.json.tpl",
+  count  = var.access_logs == true && var.apply_resource == true ? 1 : 0
+  bucket = aws_s3_bucket.log_bucket.*.id[0]
+  policy = templatefile("./tdr-terraform-modules/s3/templates/secure_transport.json.tpl",
     {
-      bucket_name = aws_s3_bucket.log_bucket.*.id[0],
+      bucket_name           = aws_s3_bucket.log_bucket.*.id[0],
       canonical_user_grants = jsonencode(var.canonical_user_grants)
-    })
+  })
   depends_on = [aws_s3_bucket_public_access_block.log_bucket]
 }
 
@@ -71,7 +71,9 @@ resource "aws_s3_bucket_notification" "log_bucket_notification" {
   }
   depends_on = [aws_s3_bucket_policy.log_bucket]
 }
-
+# This module is to be deprecated and caused many terraform warnings.
+# Attributes have been removed and added as resources.
+# The grants are now passed as a variable to the bucket policy template.
 resource "aws_s3_bucket" "bucket" {
   count         = var.apply_resource == true ? 1 : 0
   bucket        = local.bucket_name
diff --git a/s3/templates/alb_logging_euwest2.json.tpl b/s3/templates/alb_logging_euwest2.json.tpl
index 154a05e8..bbb37612 100644
--- a/s3/templates/alb_logging_euwest2.json.tpl
+++ b/s3/templates/alb_logging_euwest2.json.tpl
@@ -2,20 +2,6 @@
   "Id": "alb-logging-${bucket_name}",
   "Version": "2012-10-17",
   "Statement": [
-    %{ for grant in jsondecode(canonical_user_grants) ~}
-    {
-      "Sid": "GrantPermissions-${grant.id}",
-      "Effect": "Allow",
-      "Principal": {
-        "AWS": "${grant.id}"
-      },
-      "Action": ${jsonencode(grant.permissions)},
-      "Resource": [
-        "arn:aws:s3:::${bucket_name}",
-        "arn:aws:s3:::${bucket_name}/*"
-      ]
-    },
-    %{ endfor ~}
     {
       "Effect": "Allow",
       "Principal": {
diff --git a/s3/templates/cloudfront_origin.json.tpl b/s3/templates/cloudfront_origin.json.tpl
index 7ec56fa0..6cbdeaed 100644
--- a/s3/templates/cloudfront_origin.json.tpl
+++ b/s3/templates/cloudfront_origin.json.tpl
@@ -2,20 +2,6 @@
   "Version": "2012-10-17",
   "Id": "CloudfrontUploadBucketPolicy",
   "Statement": [
-    %{ for grant in jsondecode(canonical_user_grants) ~}
-    {
-      "Sid": "GrantPermissions-${grant.id}",
-      "Effect": "Allow",
-      "Principal": {
-          "AWS": "${grant.id}"
-      },
-      "Action": ${jsonencode(grant.permissions)},
-      "Resource": [
-          "arn:aws:s3:::${bucket_name}",
-          "arn:aws:s3:::${bucket_name}/*"
-      ]
-    },
-    %{ endfor ~}
     {
       "Effect": "Allow",
       "Principal": {
diff --git a/s3/templates/cloudtrail.json.tpl b/s3/templates/cloudtrail.json.tpl
index 2d8de4a8..8a8c2168 100644
--- a/s3/templates/cloudtrail.json.tpl
+++ b/s3/templates/cloudtrail.json.tpl
@@ -1,21 +1,7 @@
 {
   "Version": "2012-10-17",
   "Statement": [
-    %{ for grant in jsondecode(canonical_user_grants) ~}
-    {
-      "Sid": "GrantPermissions-${grant.id}",
-      "Effect": "Allow",
-      "Principal": {
-        "AWS": "${grant.id}"
-      },
-      "Action": ${jsonencode(grant.permissions)},
-      "Resource": [
-        "arn:aws:s3:::${bucket_name}",
-        "arn:aws:s3:::${bucket_name}/*"
-      ]
-    },
-    %{ endfor ~}
-    {
+   {
       "Sid": "AWSCloudTrailAclCheck",
       "Effect": "Allow",
       "Principal": {
diff --git a/s3/templates/export_bucket.json.tpl b/s3/templates/export_bucket.json.tpl
index 70535f69..6aa1b561 100644
--- a/s3/templates/export_bucket.json.tpl
+++ b/s3/templates/export_bucket.json.tpl
@@ -2,20 +2,6 @@
   "Id": "secure-transport-${bucket_name}",
   "Version": "2012-10-17",
   "Statement": [
-    %{ for grant in jsondecode(canonical_user_grants) ~}
-      {
-      "Sid": "GrantPermissions-${grant.id}",
-      "Effect": "Allow",
-      "Principal": {
-        "AWS": "${grant.id}"
-      },
-      "Action": ${jsonencode(grant.permissions)},
-      "Resource": [
-        "arn:aws:s3:::${bucket_name}",
-        "arn:aws:s3:::${bucket_name}/*"
-      ]
-    },
-    %{ endfor ~}
     {
       "Sid": "AllowSSLRequestsOnly",
       "Action": "s3:*",
diff --git a/s3/templates/lambda_update.json.tpl b/s3/templates/lambda_update.json.tpl
index 6d78b6ee..04d98dd1 100644
--- a/s3/templates/lambda_update.json.tpl
+++ b/s3/templates/lambda_update.json.tpl
@@ -2,20 +2,6 @@
   "Version": "2012-10-17",
   "Id": "secure-transport-tdr-backend-code-mgmt",
   "Statement": [
-    %{ for grant in jsondecode(canonical_user_grants) ~}
-    {
-      "Sid": "GrantPermissions-${grant.id}",
-      "Effect": "Allow",
-      "Principal": {
-        "AWS": "${grant.id}"
-      },
-      "Action": ${jsonencode(grant.permissions)},
-      "Resource": [
-        "arn:aws:s3:::${bucket_name}",
-        "arn:aws:s3:::${bucket_name}/*"
-      ]
-    },
-    %{ endfor ~}
     {
       "Sid": "AllowSSLRequestsOnly",
       "Effect": "Deny",
diff --git a/s3/templates/log-data.json.tpl b/s3/templates/log-data.json.tpl
index 110af337..a63cc04f 100644
--- a/s3/templates/log-data.json.tpl
+++ b/s3/templates/log-data.json.tpl
@@ -2,20 +2,6 @@
   "Id": "secure-logging-${bucket_name}",
   "Version": "2012-10-17",
   "Statement": [
-    %{ for grant in jsondecode(canonical_user_grants) ~}
-    {
-      "Sid": "GrantPermissions-${grant.id}",
-      "Effect": "Allow",
-      "Principal": {
-        "AWS": "${grant.id}"
-      },
-      "Action": ${jsonencode(grant.permissions)},
-      "Resource": [
-        "arn:aws:s3:::${bucket_name}",
-        "arn:aws:s3:::${bucket_name}/*"
-      ]
-    },
-    %{ endfor ~}
     {
       "Sid": "AllowSSLRequestsOnly",
       "Action": "s3:*",
diff --git a/s3/templates/secure_transport.json.tpl b/s3/templates/secure_transport.json.tpl
index eb77c169..e4d05e14 100644
--- a/s3/templates/secure_transport.json.tpl
+++ b/s3/templates/secure_transport.json.tpl
@@ -7,9 +7,18 @@
       "Sid": "GrantPermissions-${grant.id}",
       "Effect": "Allow",
       "Principal": {
-        "AWS": "${grant.id}"
+        "CanonicalUser": "${grant.id}"
       },
-      "Action": ${jsonencode(grant.permissions)},
+        "Action": [
+        "s3:GetObject",
+        "s3:PutObject",
+        "s3:DeleteObject",
+        "s3:ListBucket",
+        "s3:GetBucketLocation",
+        "s3:GetBucketAcl",
+        "s3:PutBucketAcl",
+        "s3:DeleteBucket"
+    ],
       "Resource": [
         "arn:aws:s3:::${bucket_name}",
         "arn:aws:s3:::${bucket_name}/*"
diff --git a/s3/variables.tf b/s3/variables.tf
index baece2d0..b9534624 100644
--- a/s3/variables.tf
+++ b/s3/variables.tf
@@ -23,7 +23,8 @@ variable "environment_suffix" {
 variable "acl" {
   default = "private"
 }
-
+# If this is set, the id will be used to set bucket policies for the user equivalent to 'FULL_CONTROL'.Permissions value ignored
+# Only specified for the log bucket and used in the se
 variable "canonical_user_grants" {
   description = "A list of canonical user IDs and their permissions. If this is set, you cannot use a canned ACL"
   type        = list(object({ id = string, permissions = list(string) }))

From b2e3208fcfe97dad354d1c8371764900449dd84e Mon Sep 17 00:00:00 2001
From: ian-hoyle <ian.hoyle@nationalarchives.gov.uk>
Date: Mon, 6 Jan 2025 10:45:19 +0000
Subject: [PATCH 12/13] review changes

---
 s3/main.tf                             |  6 ++----
 s3/templates/secure_transport.json.tpl | 12 ++++++------
 s3/variables.tf                        |  2 +-
 3 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/s3/main.tf b/s3/main.tf
index 10e12d78..d77c42f3 100644
--- a/s3/main.tf
+++ b/s3/main.tf
@@ -71,9 +71,7 @@ resource "aws_s3_bucket_notification" "log_bucket_notification" {
   }
   depends_on = [aws_s3_bucket_policy.log_bucket]
 }
-# This module is to be deprecated and caused many terraform warnings.
-# Attributes have been removed and added as resources.
-# The grants are now passed as a variable to the bucket policy template.
+# This module is to be deprecated
 resource "aws_s3_bucket" "bucket" {
   count         = var.apply_resource == true ? 1 : 0
   bucket        = local.bucket_name
@@ -92,7 +90,7 @@ resource "aws_s3_bucket_versioning" "bucket_versioning" {
   bucket = aws_s3_bucket.bucket[0].id
 
   versioning_configuration {
-    status = "Enabled" # Use "Suspended" to disable versioning
+    status = "Enabled"
   }
 }
 
diff --git a/s3/templates/secure_transport.json.tpl b/s3/templates/secure_transport.json.tpl
index e4d05e14..f18a49a9 100644
--- a/s3/templates/secure_transport.json.tpl
+++ b/s3/templates/secure_transport.json.tpl
@@ -10,14 +10,14 @@
         "CanonicalUser": "${grant.id}"
       },
         "Action": [
-        "s3:GetObject",
-        "s3:PutObject",
+        "s3:DeleteBucket",
         "s3:DeleteObject",
-        "s3:ListBucket",
-        "s3:GetBucketLocation",
         "s3:GetBucketAcl",
+        "s3:GetBucketLocation",
+        "s3:GetObject",
+        "s3:ListBucket",
         "s3:PutBucketAcl",
-        "s3:DeleteBucket"
+        "s3:PutObject"
     ],
       "Resource": [
         "arn:aws:s3:::${bucket_name}",
@@ -41,4 +41,4 @@
       "Principal": "*"
    }
   ]
-}
\ No newline at end of file
+}
diff --git a/s3/variables.tf b/s3/variables.tf
index b9534624..2d614111 100644
--- a/s3/variables.tf
+++ b/s3/variables.tf
@@ -24,7 +24,7 @@ variable "acl" {
   default = "private"
 }
 # If this is set, the id will be used to set bucket policies for the user equivalent to 'FULL_CONTROL'.Permissions value ignored
-# Only specified for the log bucket and used in the se
+# Only specified for the log bucket and used in the secure_transport.json.tpl
 variable "canonical_user_grants" {
   description = "A list of canonical user IDs and their permissions. If this is set, you cannot use a canned ACL"
   type        = list(object({ id = string, permissions = list(string) }))

From 369c8731a16ecec5459787fcaac795e485c995c7 Mon Sep 17 00:00:00 2001
From: ian-hoyle <ian.hoyle@nationalarchives.gov.uk>
Date: Mon, 6 Jan 2025 13:20:13 +0000
Subject: [PATCH 13/13] reduce permisions FULL_CONTROL: allows grantee the
 READ, WRITE, READ_ACP, and WRITE_ACP permissions on the bucket

---
 s3/templates/secure_transport.json.tpl | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/s3/templates/secure_transport.json.tpl b/s3/templates/secure_transport.json.tpl
index f18a49a9..c4449963 100644
--- a/s3/templates/secure_transport.json.tpl
+++ b/s3/templates/secure_transport.json.tpl
@@ -10,11 +10,7 @@
         "CanonicalUser": "${grant.id}"
       },
         "Action": [
-        "s3:DeleteBucket",
-        "s3:DeleteObject",
         "s3:GetBucketAcl",
-        "s3:GetBucketLocation",
-        "s3:GetObject",
         "s3:ListBucket",
         "s3:PutBucketAcl",
         "s3:PutObject"