fix: fix deny exception policy (#272)

* fix: fix deny exception policy * fix: change variable type * fix: change deny exception policy * fix: fix condition * fix: fix policy * fix: fix deny exception roles * fix: fix typo * fix: fix list * fix: update change log * fix: update varaible name --------- Co-authored-by: janli <[email protected]>
ExpediaGroup · Aug 28, 2024 · 4d6a9e1 · 4d6a9e1
1 parent 085f4cf
commit 4d6a9e1
Show file tree

Hide file tree

Showing 5 changed files with 81 additions and 18 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -3,6 +3,13 @@ All notable changes to this project will be documented in this file.
 
 The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/) and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).
 
+## [7.3.2] - 2024-08-27
+### Fixed
+- Fixed schema deny exception policy.
+- Added `apiary_governance_iamroles` into deny exception policy.
+- Added new variable `apiary_managed_service_iamroles` to handle tagging service IAM roles in deny exception policy.
+- Added new variable `system_schema_producer_iamroles` to support system schema producer IAM roles.
+
 ## [7.3.1] - 2024-08-26
 ### Fixed
 - Fixed incorrect `s3-inventory` service account secret binding.

diff --git a/s3-other.tf b/s3-other.tf
@@ -240,16 +240,44 @@ resource "aws_s3_bucket" "apiary_system" {
             "arn:aws:s3:::${local.apiary_system_bucket}/*"
         ]
     },
+%{endif}
+%{if length(var.system_schema_producer_iamroles) > 0}
+    {
+        "Sid": "system schema customer account permissions",
+        "Effect": "Allow",
+        "Principal": {
+          "AWS": [ "${join("\",\"", var.system_schema_producer_iamroles)}" ]
+        },
+        "Action": [
+            "s3:GetBucketLocation",
+            "s3:GetObject",
+            "s3:GetObjectAcl",
+            "s3:GetBucketAcl",
+            "s3:ListBucket",
+            "s3:PutObject",
+            "s3:PutObjectAcl",
+            "s3:DeleteObject",
+            "s3:GetBucketVersioning",
+            "s3:PutBucketVersioning",
+            "s3:ReplicateObject",
+            "s3:ReplicateDelete",
+            "s3:ObjectOwnerOverrideToBucketOwner"
+        ],
+        "Resource": [
+            "arn:aws:s3:::${local.apiary_system_bucket}",
+            "arn:aws:s3:::${local.apiary_system_bucket}/*"
+        ]
+    },
 %{endif}
     {
       "Sid": "DenyUnSecureCommunications",
       "Effect": "Deny",
       "Principal": {"AWS": "*"},
       "Action": "s3:*",
       "Resource": [
-        "arn:aws:s3:::${local.apiary_system_bucket}",
-        "arn:aws:s3:::${local.apiary_system_bucket}/*"
-        ],
+          "arn:aws:s3:::${local.apiary_system_bucket}",
+          "arn:aws:s3:::${local.apiary_system_bucket}/*"
+      ],
       "Condition": {
         "Bool": {
           "aws:SecureTransport": "false"

diff --git a/s3.tf b/s3.tf
@@ -26,8 +26,7 @@ locals {
       governance_iamroles           = join("\",\"", var.apiary_governance_iamroles)
       consumer_prefix_roles         = lookup(var.apiary_consumer_prefix_iamroles, schema["schema_name"], {})
       common_producer_iamroles      = join("\",\"", var.apiary_common_producer_iamroles)
-      deny_global_write_access      = lookup(schema, "deny_global_write_access", var.deny_global_write_access)
-      producer_roles                = lookup(schema, "producer_roles", var.producer_roles)
+      deny_exception_iamroles       = lookup(schema, "deny_exception_iamroles", "") == "" ? "" : join("\",\"", compact(concat(split(",", schema["deny_exception_iamroles"]), var.apiary_managed_service_iamroles, var.apiary_governance_iamroles)))
     })
   }
 }

diff --git a/templates/apiary-bucket-policy.json b/templates/apiary-bucket-policy.json
@@ -85,7 +85,36 @@
 %{endif}
 %{endfor ~}
 %{endif}
-%{if deny_global_write_access == "true" && producer_roles != "" }
+%{if deny_exception_iamroles != "" }
+        {
+            "Sid": "Allow write permissions to the exception roles",
+            "Effect": "Allow",
+            "Principal": "*",
+            "Action": [
+                "s3:GetBucketLocation",
+                "s3:GetObject",
+                "s3:GetObjectAcl",
+                "s3:GetBucketAcl",
+                "s3:ListBucket",
+                "s3:PutObject",
+                "s3:PutObjectAcl",
+                "s3:DeleteObject",
+                "s3:GetBucketVersioning",
+                "s3:PutBucketVersioning",
+                "s3:ReplicateObject",
+                "s3:ReplicateDelete",
+                "s3:ObjectOwnerOverrideToBucketOwner"
+            ],
+            "Resource": [
+                "arn:aws:s3:::${bucket_name}",
+                "arn:aws:s3:::${bucket_name}/*"
+            ],
+            "Condition": {
+                "StringLike": {
+                  "aws:PrincipalArn": [ "${deny_exception_iamroles}" ]
+                }
+            }
+        },
         {
             "Sid": "Deny write permissions to everything except the specified roles",
             "Effect": "Deny",
@@ -97,7 +126,7 @@
             "Resource": "arn:aws:s3:::${bucket_name}/*",
             "Condition": {
               "StringNotLike": {
-                "aws:PrincipalArn": [ "${producer_roles}" ]
+                "aws:PrincipalArn": [ "${deny_exception_iamroles}" ]
               }
             }
         },
@@ -139,7 +168,7 @@
             }
         },
 %{endif}
-%{if producer_iamroles != ""}
+%{if deny_exception_iamroles == "" && producer_iamroles != ""}
         {
             "Sid": "Apiary producer iamrole permissions",
             "Effect": "Allow",
@@ -167,7 +196,7 @@
             ]
         },
 %{endif}
-%{if common_producer_iamroles != ""}
+%{if deny_exception_iamroles == "" && common_producer_iamroles != ""}
         {
             "Sid": "General read-write iamrole permissions",
             "Effect": "Allow",
@@ -198,7 +227,7 @@
             }
         },
 %{endif}
-%{if governance_iamroles != ""}
+%{if deny_exception_iamroles == "" && governance_iamroles != ""}
         {
             "Sid": "Apiary governance iamrole permissions",
             "Effect": "Allow",

diff --git a/variables.tf b/variables.tf
@@ -812,14 +812,14 @@ variable "tcp_keepalive_probes" {
   default     = 2
 }
 
-variable "deny_global_write_access" {
-  description = "Deny all write permissions from the S3 bucket except producer_roles. See VARIABLES.md for more information."
-  type        = bool
-  default     = false
+variable "system_schema_producer_iamroles" {
+  description = "AWS IAM roles allowed write access to APIARY system schema"
+  type        = list(string)
+  default     = []
 }
 
-variable "producer_roles" {
-  description = "Comma separated list of roles that are able to write into the bucket. See VARIABLES.md for more information."
-  type        = string
-  default     = ""
+variable "apiary_managed_service_iamroles" {
+  description = "apiary managed service IAM Roles read-write access to managed Apiary S3 buckets."
+  type        = list(string)
+  default     = []
 }