From 65b5c47f60a13223fdcabd358942f2b2f31b67e0 Mon Sep 17 00:00:00 2001 From: Rahul Patil Date: Mon, 20 Apr 2020 15:29:52 +0200 Subject: [PATCH 1/3] :bug: read_prefix: Enable read_prefix condition only if configure --- main.tf | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/main.tf b/main.tf index 9cabdd0..425764c 100644 --- a/main.tf +++ b/main.tf @@ -3,6 +3,7 @@ locals { enable_read_accounts = var.enabled && length(var.read_accounts) > 0 enable_write_accounts = var.enabled && length(var.write_accounts) > 0 enable_protect = var.enabled && var.protect + enable_read_with_prefix = length(var.read_prefix) > 0 } data "aws_caller_identity" "current" {} @@ -67,7 +68,7 @@ locals { ########## S3 Bucket policy ########## data "aws_iam_policy_document" "bucket_policy_read" { - count = local.enable_read_accounts ? 1 : 0 + count = local.enable_read_accounts && !local.enable_read_with_prefix ? 1 : 0 statement { sid = "AllowCrossAccountList" @@ -77,15 +78,10 @@ data "aws_iam_policy_document" "bucket_policy_read" { type = "AWS" identifiers = var.read_accounts } - condition { - test = "StringLike" - variable = "s3:prefix" - values = ["${var.read_prefix}*"] - } } statement { sid = "AllowCrossAccountGet" - resources = ["${local.bucket_arn}/${var.read_prefix}*",] + resources = ["${local.bucket_arn}/*",] actions = ["s3:Get*"] principals { type = "AWS" From 43bc9fbf0ec48b3d3825ea2f0edc517e0638a135 Mon Sep 17 00:00:00 2001 From: Rahul Patil Date: Mon, 20 Apr 2020 15:41:08 +0200 Subject: [PATCH 2/3] :wrench: read_prefix: Add separate read_prefix policy --- main.tf | 37 ++++++++++++++++++++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-) diff --git a/main.tf b/main.tf index 425764c..f03b151 100644 --- a/main.tf +++ b/main.tf @@ -90,6 +90,34 @@ data "aws_iam_policy_document" "bucket_policy_read" { } } +data "aws_iam_policy_document" "bucket_policy_read_with_prifix" { + count = local.enable_read_accounts && local.enable_read_with_prefix ? 1 : 0 + + statement { + sid = "AllowCrossAccountList" + resources = [local.bucket_arn] + actions = ["s3:List*"] + principals { + type = "AWS" + identifiers = var.read_accounts + } + condition { + test = "StringLike" + variable = "s3:prefix" + values = ["${var.read_prefix}*"] + } + } + statement { + sid = "AllowCrossAccountGet" + resources = ["${local.bucket_arn}/${var.read_prefix}*",] + actions = ["s3:Get*"] + principals { + type = "AWS" + identifiers = var.read_accounts + } + } +} + data "aws_iam_policy_document" "bucket_policy_write" { count = local.enable_write_accounts ? 1 : 0 @@ -135,12 +163,19 @@ locals { } data "aws_iam_policy_document" "bucket_policy_read_and_write" { - count = local.enable_read_accounts || local.enable_write_accounts ? 1 : 0 + count = local.enable_read_accounts && !local.enable_read_with_prefix || local.enable_write_accounts ? 1 : 0 source_json = concat(data.aws_iam_policy_document.bucket_policy_read.*.json, [""])[0] override_json = concat(data.aws_iam_policy_document.bucket_policy_write.*.json, [""])[0] } +data "aws_iam_policy_document" "bucket_policy_read_prefix_and_write" { + count = local.enable_read_with_prefix || local.enable_write_accounts ? 1 : 0 + + source_json = concat(data.aws_iam_policy_document.bucket_policy_read_with_prifix.*.json, [""])[0] + override_json = concat(data.aws_iam_policy_document.bucket_policy_write.*.json, [""])[0] +} + data "aws_iam_policy_document" "bucket_policy" { count = local.enable_bucket_policy ? 1 : 0 From 3ebac7c4c9e47e3b3ecfc032d18c6dd14c15c698 Mon Sep 17 00:00:00 2001 From: Rahul Patil Date: Tue, 21 Apr 2020 14:07:25 +0200 Subject: [PATCH 3/3] :recycle: iam_policy: Use dynamic block instead of repetition --- main.tf | 46 +++++++++------------------------------------- 1 file changed, 9 insertions(+), 37 deletions(-) diff --git a/main.tf b/main.tf index f03b151..5a0c911 100644 --- a/main.tf +++ b/main.tf @@ -3,7 +3,6 @@ locals { enable_read_accounts = var.enabled && length(var.read_accounts) > 0 enable_write_accounts = var.enabled && length(var.write_accounts) > 0 enable_protect = var.enabled && var.protect - enable_read_with_prefix = length(var.read_prefix) > 0 } data "aws_caller_identity" "current" {} @@ -68,7 +67,7 @@ locals { ########## S3 Bucket policy ########## data "aws_iam_policy_document" "bucket_policy_read" { - count = local.enable_read_accounts && !local.enable_read_with_prefix ? 1 : 0 + count = local.enable_read_accounts ? 1 : 0 statement { sid = "AllowCrossAccountList" @@ -78,33 +77,13 @@ data "aws_iam_policy_document" "bucket_policy_read" { type = "AWS" identifiers = var.read_accounts } - } - statement { - sid = "AllowCrossAccountGet" - resources = ["${local.bucket_arn}/*",] - actions = ["s3:Get*"] - principals { - type = "AWS" - identifiers = var.read_accounts - } - } -} - -data "aws_iam_policy_document" "bucket_policy_read_with_prifix" { - count = local.enable_read_accounts && local.enable_read_with_prefix ? 1 : 0 - - statement { - sid = "AllowCrossAccountList" - resources = [local.bucket_arn] - actions = ["s3:List*"] - principals { - type = "AWS" - identifiers = var.read_accounts - } - condition { - test = "StringLike" - variable = "s3:prefix" - values = ["${var.read_prefix}*"] + dynamic "condition" { + for_each = length(var.read_prefix) > 0 ? [var.read_prefix] : [] + content { + test = "StringLike" + variable = "s3:prefix" + values = ["${var.read_prefix}*"] + } } } statement { @@ -163,19 +142,12 @@ locals { } data "aws_iam_policy_document" "bucket_policy_read_and_write" { - count = local.enable_read_accounts && !local.enable_read_with_prefix || local.enable_write_accounts ? 1 : 0 + count = local.enable_read_accounts || local.enable_write_accounts ? 1 : 0 source_json = concat(data.aws_iam_policy_document.bucket_policy_read.*.json, [""])[0] override_json = concat(data.aws_iam_policy_document.bucket_policy_write.*.json, [""])[0] } -data "aws_iam_policy_document" "bucket_policy_read_prefix_and_write" { - count = local.enable_read_with_prefix || local.enable_write_accounts ? 1 : 0 - - source_json = concat(data.aws_iam_policy_document.bucket_policy_read_with_prifix.*.json, [""])[0] - override_json = concat(data.aws_iam_policy_document.bucket_policy_write.*.json, [""])[0] -} - data "aws_iam_policy_document" "bucket_policy" { count = local.enable_bucket_policy ? 1 : 0