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,11 +83,7 @@ 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" {
@@ -95,19 +95,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 +124,7 @@ data "aws_iam_policy_document" "session_logging" {
     actions = [
       "s3:GetEncryptionConfiguration"
     ]
-    resources = ["*"]
+    resources = [local.session_logging_bucket_arn]
   }
 
   statement {
@@ -125,6 +133,6 @@ data "aws_iam_policy_document" "session_logging" {
     actions = [
       "kms:GenerateDataKey"
     ]
-    resources = ["*"]
+    resources = [local.session_logging_kms_key_arn]
   }
 }

examples/complete/main.tf

@@ -11,7 +11,7 @@ module "vpc" {
   name      = var.name
 
   ipv4_primary_cidr_block          = "10.0.0.0/16"
-  assign_generated_ipv6_cidr_block = true
+  assign_generated_ipv6_cidr_block = var.ipv6_enabled
 }
 
 module "subnets" {

examples/with-names/variables.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
-}

main.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
 
 }
@@ -35,7 +35,7 @@ resource "null_resource" "validate_instance_type" {
 resource "terraform_data" "vpc_subnet_validation" {
   lifecycle {
     precondition {
-      condition     = length(var.vpc_name) > 0 || length(var.vpc_id) > 0
+      condition     = var.vpc_name != null || var.vpc_id != null
       error_message = "Either vpc_name or vpc_id must be provided."
     }
 
@@ -49,7 +49,7 @@ resource "terraform_data" "vpc_subnet_validation" {
 # 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)
+    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."
   }
 
@@ -81,6 +81,8 @@ locals {
 
   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
 }

outputs.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."
 }

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

@@ -55,7 +55,7 @@ mock_provider "aws" {
 }
 
 # Test precedence - names over IDs
-run "test_precedence_local" {
+run "verify_vpc_subnet_name_precedence" {
   command = plan
 
   variables {

variables.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" {