Update deny_iamroles to allow getting S3 metadata/tags/etc. (#200)

* Update deny_iamroles to allow getting S3 metadata/tags/etc. * forgot ObjectVersion stuff * make list of denied S3 actions a variable * make list of denied S3 actions a variable * run terraform fmt on s3.tf * fix permission list * reformat file to how it used to be Co-authored-by: Scott Barnhart <[email protected]>
ExpediaGroup · Aug 18, 2021 · 63af892 · 63af892
1 parent 430582c
commit 63af892
Show file tree

Hide file tree

Showing 5 changed files with 33 additions and 9 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -3,6 +3,10 @@ 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).
 
+## [6.10.2] - 2021-08-18
+### Changed
+- Changed bucket policy for `deny_iamroles` to only deny "dangerous" actions, including `GetObject`.
+
 ## [6.10.1] - 2021-07-23
 ### Added
 - Variable to enable RDS encryption.

diff --git a/VARIABLES.md b/VARIABLES.md
@@ -8,7 +8,8 @@
 | apiary_customer_accounts | AWS account IDs for clients of this Metastore. | list | - | no |
 | apiary_customer_condition | IAM policy condition applied to customer account s3 access. | string | `` | no |
 | apiary_database_name | Database name to create in RDS for Apiary. | string | `apiary` | no |
-| apiary_deny_roles | AWS IAM roles denied access to Apiary managed S3 buckets. | list | - | yes |
+| apiary_deny_iamroles | AWS IAM roles denied access to Apiary managed S3 buckets. | list | - | no |
+| apiary_deny_iamrole_actions | List of S3 actions that `apiary_deny_iamroles` are not allowed to perform. | list | All actions except `Get*`, but not including `GetObject` | no |
 | apiary_domain_name | Apiary domain name for Route 53. | string | `` | no |
 | apiary_log_bucket | Bucket for Apiary logs. | string | - | yes |
 | apiary_log_prefix | Prefix for Apiary logs. | string | `` | no |

diff --git a/s3.tf b/s3.tf
@@ -19,13 +19,14 @@ data "template_file" "bucket_policy" {
     customer_principal = (length(var.apiary_shared_schemas) == 0 || contains(var.apiary_shared_schemas, each.key)) && each.value["customer_accounts"] != "" ? join("\",\"", formatlist("arn:aws:iam::%s:root", split(",", each.value["customer_accounts"]))) : ""
     customer_condition = var.apiary_customer_condition
 
-    bucket_name       = each.value["data_bucket"]
-    encryption        = each.value["encryption"]
-    kms_key_arn       = each.value["encryption"] == "aws:kms" ? aws_kms_key.apiary_kms[each.key].arn : ""
-    consumer_iamroles = join("\",\"", var.apiary_consumer_iamroles)
-    producer_iamroles = replace(lookup(var.apiary_producer_iamroles, each.key, ""), ",", "\",\"")
-    deny_iamroles     = join("\",\"", var.apiary_deny_iamroles)
-    client_roles      = replace(lookup(each.value, "client_roles", ""), ",", "\",\"")
+    bucket_name          = each.value["data_bucket"]
+    encryption           = each.value["encryption"]
+    kms_key_arn          = each.value["encryption"] == "aws:kms" ? aws_kms_key.apiary_kms[each.key].arn : ""
+    consumer_iamroles    = join("\",\"", var.apiary_consumer_iamroles)
+    producer_iamroles    = replace(lookup(var.apiary_producer_iamroles, each.key, ""), ",", "\",\"")
+    deny_iamroles        = join("\",\"", var.apiary_deny_iamroles)
+    deny_iamrole_actions = join("\",\"", var.apiary_deny_iamrole_actions)
+    client_roles         = replace(lookup(each.value, "client_roles", ""), ",", "\",\"")
   }
 }
 

diff --git a/templates/apiary-bucket-policy.json b/templates/apiary-bucket-policy.json
@@ -60,7 +60,7 @@
             "Sid": "Local role deny permissions",
             "Effect": "Deny",
             "Principal": "*",
-            "Action": "s3:*",
+            "Action": [ "${deny_iamrole_actions}" ],
             "Resource": "arn:aws:s3:::${bucket_name}/*",
             "Condition": {
               "StringLike": {

diff --git a/variables.tf b/variables.tf
@@ -144,6 +144,24 @@ variable "apiary_deny_iamroles" {
   default     = []
 }
 
+variable "apiary_deny_iamrole_actions" {
+  description = "List of S3 actions that 'apiary_deny_iamroles' are not allowed to perform."
+  type        = list(string)
+  default     = [
+    "s3:Abort*",
+    "s3:Bypass*",
+    "s3:Delete*",
+    "s3:GetObject",
+    "s3:GetObjectTorrent",
+    "s3:GetObjectVersion",
+    "s3:GetObjectVersionTorrent",
+    "s3:ObjectOwnerOverrideToBucketOwner",
+    "s3:Put*",
+    "s3:Replicate*",
+    "s3:Restore*"
+  ]
+}
+
 variable "apiary_assume_roles" {
   description = "Cross account AWS IAM roles allowed write access to managed Apiary S3 buckets using assume policy."
   type        = list(any)