chore: security best-practices + minor improvements

Author: gberenice

data.tf

@@ -3,7 +3,7 @@ data "aws_caller_identity" "current" {}
 
 # VPC lookup by name (when vpc_name is provided)
 data "aws_vpc" "selected" {
-  count = length(var.vpc_name) > 0 ? 1 : 0
+  count = var.vpc_name != null ? 1 : 0
 
   filter {
     name   = "tag:Name"
@@ -19,6 +19,10 @@ data "aws_subnet" "selected" {
     name   = "tag:Name"
     values = [each.value]
   }
+  filter {
+    name   = "vpc-id"
+    values = [local.vpc_id]
+  }
 }
 
 # Most recent Amazon Linux 2023 AMI
@@ -79,12 +83,6 @@ data "aws_iam_policy_document" "default" {
   }
 }
 
-# S3 bucket for session logging (when using existing bucket)
-data "aws_s3_bucket" "logs_bucket" {
-  count  = var.session_logging_enabled ? 1 : 0
-  bucket = try(coalesce(var.session_logging_bucket_name, module.logs_bucket.bucket_id), "")
-}
-
 # https://docs.aws.amazon.com/systems-manager/latest/userguide/getting-started-create-iam-instance-profile.html#create-iam-instance-profile-ssn-logging
 data "aws_iam_policy_document" "session_logging" {
   count = var.session_logging_enabled ? 1 : 0
@@ -95,19 +93,27 @@ data "aws_iam_policy_document" "session_logging" {
     actions = [
       "s3:PutObject"
     ]
-    resources = ["${join("", data.aws_s3_bucket.logs_bucket.*.arn)}/*"]
+    resources = ["${local.session_logging_bucket_arn}/*"]
   }
 
   statement {
     sid    = "SSMAgentSessionAllowCloudWatchLogging"
     effect = "Allow"
     actions = [
       "logs:CreateLogStream",
-      "logs:PutLogEvents",
+      "logs:PutLogEvents"
+    ]
+    resources = ["${local.session_logging_log_group_arn}:*"]
+  }
+
+  statement {
+    sid    = "SSMAgentSessionAllowCloudWatchDescribe"
+    effect = "Allow"
+    actions = [
       "logs:DescribeLogGroups",
       "logs:DescribeLogStreams"
     ]
-    resources = ["*"]
+    resources = [local.session_logging_log_group_arn]
   }
 
   statement {
@@ -116,7 +122,7 @@ data "aws_iam_policy_document" "session_logging" {
     actions = [
       "s3:GetEncryptionConfiguration"
     ]
-    resources = ["*"]
+    resources = [local.session_logging_bucket_arn]
   }
 
   statement {
@@ -125,6 +131,6 @@ data "aws_iam_policy_document" "session_logging" {
     actions = [
       "kms:GenerateDataKey"
     ]
-    resources = ["*"]
+    resources = [local.session_logging_kms_key_arn]
   }
 }

examples/with-names/outputs.tf

@@ -21,9 +21,3 @@ variable "availability_zones" {
   description = "List of availability zones"
   default     = ["us-east-1a", "us-east-1b"]
 }
-
-variable "ipv6_enabled" {
-  type        = bool
-  description = "Enable IPv6"
-  default     = false
-}

examples/with-names/variables.tf

@@ -15,7 +15,7 @@ locals {
   )
 
   # VPC and Subnet ID resolution - names take precedence over IDs
-  vpc_id     = length(var.vpc_name) > 0 ? one(data.aws_vpc.selected[*].id) : var.vpc_id
+  vpc_id     = var.vpc_name != null ? one(data.aws_vpc.selected[*].id) : var.vpc_id
   subnet_ids = length(var.subnet_names) > 0 ? values(data.aws_subnet.selected)[*].id : var.subnet_ids
 
 }
@@ -31,33 +31,6 @@ resource "null_resource" "validate_instance_type" {
   }
 }
 
-# Validation using terraform_data to halt execution if requirements aren't met
-resource "terraform_data" "vpc_subnet_validation" {
-  lifecycle {
-    precondition {
-      condition     = length(var.vpc_name) > 0 || length(var.vpc_id) > 0
-      error_message = "Either vpc_name or vpc_id must be provided."
-    }
-
-    precondition {
-      condition     = length(var.subnet_names) > 0 || length(var.subnet_ids) > 0
-      error_message = "Either subnet_names or subnet_ids must be provided."
-    }
-  }
-}
-
-# Warning checks for VPC and subnet configuration (non-blocking)
-check "vpc_subnet_warnings" {
-  assert {
-    condition     = !(length(var.vpc_name) > 0 && length(var.vpc_id) > 0)
-    error_message = "Both vpc_name and vpc_id are provided. When vpc_name is specified, vpc_id will be ignored."
-  }
-
-  assert {
-    condition     = !(length(var.subnet_names) > 0 && length(var.subnet_ids) > 0)
-    error_message = "Both subnet_names and subnet_ids are provided. When subnet_names are specified, subnet_ids will be ignored."
-  }
-}
 
 module "role_label" {
   source  = "cloudposse/label/null"
@@ -79,8 +52,10 @@ locals {
   region     = coalesce(var.region, data.aws_region.current.region)
   account_id = data.aws_caller_identity.current.account_id
 
-  session_logging_bucket_name = try(coalesce(var.session_logging_bucket_name, module.logs_label.id), "")
-  session_logging_kms_key_arn = try(coalesce(var.session_logging_kms_key_arn, module.kms_key.key_arn), "")
+  session_logging_bucket_name   = try(coalesce(var.session_logging_bucket_name, module.logs_label.id), "")
+  session_logging_kms_key_arn   = try(coalesce(var.session_logging_kms_key_arn, module.kms_key.key_arn), "")
+  session_logging_bucket_arn    = var.session_logging_enabled ? "arn:aws:s3:::${local.session_logging_bucket_name}" : ""
+  session_logging_log_group_arn = var.session_logging_enabled ? "arn:aws:logs:${local.region}:${local.account_id}:log-group:${module.logs_label.id}" : ""
 
   logs_bucket_enabled = var.session_logging_enabled && length(var.session_logging_bucket_name) == 0
 }

main.tf

@@ -24,11 +24,11 @@ output "role_id" {
 }
 
 output "session_logging_bucket_id" {
-  value       = local.logs_bucket_enabled ? join("", data.aws_s3_bucket.logs_bucket.*.id) : ""
+  value       = var.session_logging_enabled ? local.session_logging_bucket_name : ""
   description = "The ID of the SSM Agent Session Logging S3 Bucket."
 }
 
 output "session_logging_bucket_arn" {
-  value       = local.logs_bucket_enabled ? join("", data.aws_s3_bucket.logs_bucket.*.arn) : ""
+  value       = local.session_logging_bucket_arn
   description = "The ARN of the SSM Agent Session Logging S3 Bucket."
 }

outputs.tf

@@ -27,7 +27,7 @@ mock_provider "aws" {
 
   mock_data "aws_ami" {
     defaults = {
-      id = "ami-mock"
+      id               = "ami-mock"
       root_device_name = "/dev/xvda"
     }
   }
@@ -40,29 +40,29 @@ mock_provider "aws" {
 
   mock_data "aws_s3_bucket" {
     defaults = {
-      id = "mock-bucket"
+      id  = "mock-bucket"
       arn = "arn:aws:s3:::mock-bucket"
     }
   }
 
   # Mock AWS resources that get created in the module
   mock_resource "aws_launch_template" {
     defaults = {
-      id = "lt-mock123456"
+      id             = "lt-mock123456"
       latest_version = "1"
     }
   }
 }
 
 # Test precedence - names over IDs
-run "test_precedence_local" {
+run "verify_vpc_subnet_name_precedence" {
   command = plan
 
   variables {
-    vpc_id       = "vpc-should-be-ignored"
-    vpc_name     = "my-vpc"
-    subnet_ids   = ["subnet-should-be-ignored"]
-    subnet_names = ["my-subnet"]
+    vpc_id                  = "vpc-should-be-ignored"
+    vpc_name                = "my-vpc"
+    subnet_ids              = ["subnet-should-be-ignored"]
+    subnet_names            = ["my-subnet"]
     session_logging_enabled = false
 
     # Add required context variables for proper naming

tests/vpc-subnet-locals-only.tftest.hcl renamed to tests/vpc-subnet-locals.tftest.hcl

@@ -0,0 +1,27 @@
+# Validation using terraform_data to halt execution if requirements aren't met
+resource "terraform_data" "vpc_subnet_validation" {
+  lifecycle {
+    precondition {
+      condition     = var.vpc_name != null || var.vpc_id != null
+      error_message = "Either vpc_name or vpc_id must be provided."
+    }
+
+    precondition {
+      condition     = length(var.subnet_names) > 0 || length(var.subnet_ids) > 0
+      error_message = "Either subnet_names or subnet_ids must be provided."
+    }
+  }
+}
+
+# Warning checks for VPC and subnet configuration (non-blocking)
+check "vpc_subnet_warnings" {
+  assert {
+    condition     = !(var.vpc_name != null && var.vpc_id != null)
+    error_message = "Both vpc_name and vpc_id are provided. When vpc_name is specified, vpc_id will be ignored."
+  }
+
+  assert {
+    condition     = !(length(var.subnet_names) > 0 && length(var.subnet_ids) > 0)
+    error_message = "Both subnet_names and subnet_ids are provided. When subnet_names are specified, subnet_ids will be ignored."
+  }
+}

validations.tf

@@ -1,7 +1,7 @@
 variable "vpc_id" {
   type        = string
   description = "The ID of the VPC which the EC2 Instance will run in."
-  default     = ""
+  default     = null
 }
 
 variable "subnet_ids" {
@@ -13,7 +13,7 @@ variable "subnet_ids" {
 variable "vpc_name" {
   type        = string
   description = "The name of the VPC which the EC2 Instance will run in. If provided, vpc_id will be ignored."
-  default     = ""
+  default     = null
 }
 
 variable "subnet_names" {
@@ -172,6 +172,15 @@ variable "session_logging_bucket_name" {
   default     = ""
   type        = string
   description = "The name of the S3 Bucket to ship session logs to. This will remove creation of an independent session logging bucket. This is only relevant if the session_logging_enabled variable is `true`."
+
+  validation {
+    condition = var.session_logging_bucket_name == "" || (
+      can(regex("^[a-z0-9][a-z0-9.-]*[a-z0-9]$", var.session_logging_bucket_name)) &&
+      length(var.session_logging_bucket_name) >= 3 &&
+      length(var.session_logging_bucket_name) <= 63
+    )
+    error_message = "S3 bucket name must follow AWS naming conventions: 3-63 characters, lowercase letters, numbers, dots, and hyphens only, must start and end with letter or number."
+  }
 }
 
 variable "region" {