From bbd0a000c6dd3836b980f4bec103ec240c417103 Mon Sep 17 00:00:00 2001
From: Kostya <53430368+kostyaplis@users.noreply.github.com>
Date: Fri, 9 Apr 2021 19:18:54 +0800
Subject: [PATCH] feat: Add ability to create deny insecure transport policy
 (#77)

---
 .github/workflows/pre-commit.yml | 38 ++++++++++++------------
 README.md                        |  3 ++
 examples/complete/main.tf        | 11 ++++---
 main.tf                          | 51 ++++++++++++++++++++++++++++++--
 variables.tf                     |  6 ++++
 5 files changed, 83 insertions(+), 26 deletions(-)

diff --git a/.github/workflows/pre-commit.yml b/.github/workflows/pre-commit.yml
index 7eaa782b..59cd0a89 100644
--- a/.github/workflows/pre-commit.yml
+++ b/.github/workflows/pre-commit.yml
@@ -7,30 +7,30 @@ on:
       - master
 
 jobs:
-# Min Terraform version(s)
+  # Min Terraform version(s)
   getDirectories:
-      name: Get root directories
-      runs-on: ubuntu-latest
-      steps:
-          - name: Checkout
-            uses: actions/checkout@v2
-          - name: Install Python
-            uses: actions/setup-python@v2
-          - name: Build matrix
-            id: matrix
-            run: |
-              DIRS=$(python -c "import json; import glob; print(json.dumps([x.replace('/versions.tf', '') for x in glob.glob('./**/versions.tf', recursive=True)]))")
-              echo "::set-output name=directories::$DIRS"
-      outputs:
-          directories: ${{ steps.matrix.outputs.directories }}
+    name: Get root directories
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout
+        uses: actions/checkout@v2
+      - name: Install Python
+        uses: actions/setup-python@v2
+      - name: Build matrix
+        id: matrix
+        run: |
+          DIRS=$(python -c "import json; import glob; print(json.dumps([x.replace('/versions.tf', '') for x in glob.glob('./**/versions.tf', recursive=True)]))")
+          echo "::set-output name=directories::$DIRS"
+    outputs:
+      directories: ${{ steps.matrix.outputs.directories }}
 
   preCommitMinVersions:
     name: Min TF validate
     needs: getDirectories
     runs-on: ubuntu-latest
     strategy:
-        matrix:
-            directory: ${{ fromJson(needs.getDirectories.outputs.directories) }}
+      matrix:
+        directory: ${{ fromJson(needs.getDirectories.outputs.directories) }}
     steps:
       - name: Checkout
         uses: actions/checkout@v2
@@ -59,7 +59,7 @@ jobs:
           pre-commit run terraform_validate --color=always --show-diff-on-failure --files $(ls *.tf)
 
 
-# Max Terraform version
+  # Max Terraform version
   getBaseVersion:
     name: Module max TF version
     runs-on: ubuntu-latest
@@ -94,7 +94,7 @@ jobs:
       - name: Install pre-commit dependencies
         run: |
           pip install pre-commit
-          curl -L "$(curl -s https://api.github.com/repos/terraform-docs/terraform-docs/releases/latest | grep -o -E "https://.+?-v0.12.0-linux-amd64" | head -n1)" > terraform-docs && chmod +x terraform-docs && sudo mv terraform-docs /usr/bin/
+          curl -L "$(curl -s https://api.github.com/repos/terraform-docs/terraform-docs/releases/latest | grep -o -E "https://.+?-v0.12\..+?-linux-amd64" | head -n1)" > terraform-docs && chmod +x terraform-docs && sudo mv terraform-docs /usr/bin/
           curl -L "$(curl -s https://api.github.com/repos/terraform-linters/tflint/releases/latest | grep -o -E "https://.+?_linux_amd64.zip")" > tflint.zip && unzip tflint.zip && rm tflint.zip && sudo mv tflint /usr/bin/
       - name: Execute pre-commit
         # Run all pre-commit checks on max version supported
diff --git a/README.md b/README.md
index b684853a..f6266ecc 100644
--- a/README.md
+++ b/README.md
@@ -103,6 +103,8 @@ No modules.
 | [aws_s3_bucket_policy.this](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/s3_bucket_policy) | resource |
 | [aws_s3_bucket_public_access_block.this](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/s3_bucket_public_access_block) | resource |
 | [aws_elb_service_account.this](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/elb_service_account) | data source |
+| [aws_iam_policy_document.combined](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/iam_policy_document) | data source |
+| [aws_iam_policy_document.deny_insecure_transport](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/iam_policy_document) | data source |
 | [aws_iam_policy_document.elb_log_delivery](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/iam_policy_document) | data source |
 
 ## Inputs
@@ -111,6 +113,7 @@ No modules.
 |------|-------------|------|---------|:--------:|
 | <a name="input_acceleration_status"></a> [acceleration\_status](#input\_acceleration\_status) | (Optional) Sets the accelerate configuration of an existing bucket. Can be Enabled or Suspended. | `string` | `null` | no |
 | <a name="input_acl"></a> [acl](#input\_acl) | (Optional) The canned ACL to apply. Defaults to 'private'. Conflicts with `grant` | `string` | `"private"` | no |
+| <a name="input_attach_deny_insecure_transport_policy"></a> [attach\_deny\_insecure\_transport\_policy](#input\_attach\_deny\_insecure\_transport\_policy) | Controls if S3 bucket should have deny non-SSL transport policy attached | `bool` | `false` | no |
 | <a name="input_attach_elb_log_delivery_policy"></a> [attach\_elb\_log\_delivery\_policy](#input\_attach\_elb\_log\_delivery\_policy) | Controls if S3 bucket should have ELB log delivery policy attached | `bool` | `false` | no |
 | <a name="input_attach_policy"></a> [attach\_policy](#input\_attach\_policy) | Controls if S3 bucket should have bucket policy attached (set to `true` to use value of `policy` as bucket policy) | `bool` | `false` | no |
 | <a name="input_attach_public_policy"></a> [attach\_public\_policy](#input\_attach\_public\_policy) | Controls if a user defined public bucket policy will be attached (set to `false` to allow upstream to apply defaults to the bucket) | `bool` | `true` | no |
diff --git a/examples/complete/main.tf b/examples/complete/main.tf
index 90bd6f9b..587a19c2 100644
--- a/examples/complete/main.tf
+++ b/examples/complete/main.tf
@@ -51,10 +51,11 @@ data "aws_iam_policy_document" "bucket_policy" {
 module "log_bucket" {
   source = "../../"
 
-  bucket                         = "logs-${random_pet.this.id}"
-  acl                            = "log-delivery-write"
-  force_destroy                  = true
-  attach_elb_log_delivery_policy = true
+  bucket                                = "logs-${random_pet.this.id}"
+  acl                                   = "log-delivery-write"
+  force_destroy                         = true
+  attach_elb_log_delivery_policy        = true
+  attach_deny_insecure_transport_policy = true
 }
 
 module "cloudfront_log_bucket" {
@@ -86,6 +87,8 @@ module "s3_bucket" {
   attach_policy = true
   policy        = data.aws_iam_policy_document.bucket_policy.json
 
+  attach_deny_insecure_transport_policy = true
+
   tags = {
     Owner = "Anton"
   }
diff --git a/main.tf b/main.tf
index 98d288af..650ff419 100644
--- a/main.tf
+++ b/main.tf
@@ -1,3 +1,7 @@
+locals {
+  attach_policy = var.attach_elb_log_delivery_policy || var.attach_deny_insecure_transport_policy || var.attach_policy
+}
+
 resource "aws_s3_bucket" "this" {
   count = var.create_bucket ? 1 : 0
 
@@ -231,10 +235,20 @@ resource "aws_s3_bucket" "this" {
 }
 
 resource "aws_s3_bucket_policy" "this" {
-  count = var.create_bucket && (var.attach_elb_log_delivery_policy || var.attach_policy) ? 1 : 0
+  count = var.create_bucket && local.attach_policy ? 1 : 0
 
   bucket = aws_s3_bucket.this[0].id
-  policy = var.attach_elb_log_delivery_policy ? data.aws_iam_policy_document.elb_log_delivery[0].json : var.policy
+  policy = data.aws_iam_policy_document.combined[0].json
+}
+
+data "aws_iam_policy_document" "combined" {
+  count = var.create_bucket && local.attach_policy ? 1 : 0
+
+  source_policy_documents = compact([
+    var.attach_elb_log_delivery_policy ? data.aws_iam_policy_document.elb_log_delivery[0].json : "",
+    var.attach_deny_insecure_transport_policy ? data.aws_iam_policy_document.deny_insecure_transport[0].json : "",
+    var.attach_policy ? var.policy : ""
+  ])
 }
 
 # AWS Load Balancer access log delivery policy
@@ -265,12 +279,43 @@ data "aws_iam_policy_document" "elb_log_delivery" {
   }
 }
 
+data "aws_iam_policy_document" "deny_insecure_transport" {
+  count = var.create_bucket && var.attach_deny_insecure_transport_policy ? 1 : 0
+
+  statement {
+    sid    = "denyInsecureTransport"
+    effect = "Deny"
+
+    actions = [
+      "s3:*",
+    ]
+
+    resources = [
+      aws_s3_bucket.this[0].arn,
+      "${aws_s3_bucket.this[0].arn}/*",
+    ]
+
+    principals {
+      type        = "*"
+      identifiers = ["*"]
+    }
+
+    condition {
+      test     = "Bool"
+      variable = "aws:SecureTransport"
+      values = [
+        "false"
+      ]
+    }
+  }
+}
+
 resource "aws_s3_bucket_public_access_block" "this" {
   count = var.create_bucket && var.attach_public_policy ? 1 : 0
 
   # Chain resources (s3_bucket -> s3_bucket_policy -> s3_bucket_public_access_block)
   # to prevent "A conflicting conditional operation is currently in progress against this resource."
-  bucket = (var.attach_elb_log_delivery_policy || var.attach_policy) ? aws_s3_bucket_policy.this[0].id : aws_s3_bucket.this[0].id
+  bucket = local.attach_policy ? aws_s3_bucket_policy.this[0].id : aws_s3_bucket.this[0].id
 
   block_public_acls       = var.block_public_acls
   block_public_policy     = var.block_public_policy
diff --git a/variables.tf b/variables.tf
index 9e2b7b1d..f07d4169 100644
--- a/variables.tf
+++ b/variables.tf
@@ -10,6 +10,12 @@ variable "attach_elb_log_delivery_policy" {
   default     = false
 }
 
+variable "attach_deny_insecure_transport_policy" {
+  description = "Controls if S3 bucket should have deny non-SSL transport policy attached"
+  type        = bool
+  default     = false
+}
+
 variable "attach_policy" {
   description = "Controls if S3 bucket should have bucket policy attached (set to `true` to use value of `policy` as bucket policy)"
   type        = bool