fix(efs): correct EFS policy

infrastructure/modules/alb/README.md

@@ -660,7 +660,8 @@ For **NLB (network load balancer):**
 | <a name="input_service"></a> [service](#input\_service) | ID element. Usually an abbreviation of your service directorate name, e.g. 'bcss' or 'csms', to help ensure generated IDs are globally unique | `string` | `null` | no |
 | <a name="input_service_category"></a> [service\_category](#input\_service\_category) | The tag service\_category | `string` | `"n/a"` | no |
 | <a name="input_stack"></a> [stack](#input\_stack) | ID element. The name of the stack/component, e.g. `database`, `web`, `waf`, `eks` | `string` | `null` | no |
-| <a name="input_subnets"></a> [subnets](#input\_subnets) | List of subnet IDs to attach to the load balancer. For internet-facing ALBs, use public subnets. | `list(string)` | n/a | yes |
+| <a name="input_subnet_mapping"></a> [subnet\_mapping](#input\_subnet\_mapping) | Subnet mapping configuration for the load balancer. Useful for NLB static EIPs (allocation\_id per subnet). When set, this takes precedence over subnets. Passed through to the upstream module. | `any` | `null` | no |
+| <a name="input_subnets"></a> [subnets](#input\_subnets) | List of subnet IDs to attach to the load balancer. If subnet\_mapping is also set, this input is ignored and subnet\_mapping is used. For internet-facing ALBs, use public subnets. | `list(string)` | `null` | no |
 | <a name="input_tag_version"></a> [tag\_version](#input\_tag\_version) | Used to identify the tagging version in use | `string` | `"1.0"` | no |
 | <a name="input_tags"></a> [tags](#input\_tags) | Additional tags (e.g. `{'BusinessUnit': 'XYZ'}`).<br/>Neither the tag keys nor the tag values will be modified by this module. | `map(string)` | `{}` | no |
 | <a name="input_target_groups"></a> [target\_groups](#input\_target\_groups) | Map of target group configurations to create. Passed directly to the upstream module.<br/>See https://registry.terraform.io/modules/terraform-aws-modules/alb/aws/latest<br/>for full schema documentation. | `any` | `{}` | no |

infrastructure/modules/alb/locals.tf

@@ -1,5 +1,12 @@
 locals {
-  is_alb = var.load_balancer_type == "application"
+  is_alb             = var.load_balancer_type == "application"
+  has_subnets        = var.subnets != null && length(var.subnets) > 0
+  has_subnet_mapping = var.subnet_mapping != null && length(var.subnet_mapping) > 0
+
+  # Keep wrapper behaviour flexible: if both are provided, prefer subnet_mapping
+  # and suppress subnets to satisfy AWS one-of semantics.
+  effective_subnet_mapping = local.has_subnet_mapping ? var.subnet_mapping : null
+  effective_subnets        = local.has_subnet_mapping ? null : (local.has_subnets ? var.subnets : null)
 
   # ALB-only derived defaults. NLB keeps null for these upstream inputs.
   effective_drop_invalid_header_fields = local.is_alb ? true : null

infrastructure/modules/alb/main.tf

@@ -30,7 +30,8 @@ module "alb" {
   load_balancer_type = var.load_balancer_type
   internal           = var.internal
   vpc_id             = var.vpc_id
-  subnets            = var.subnets
+  subnets            = local.effective_subnets
+  subnet_mapping     = local.effective_subnet_mapping
 
   # ----------------------------------------------------------------
   # Security baseline — drop_invalid_header_fields is hardcoded.

infrastructure/modules/alb/validations.tf

@@ -10,6 +10,11 @@ resource "terraform_data" "validation" {
   count = module.this.enabled ? 1 : 0
 
   lifecycle {
+    precondition {
+      condition     = local.has_subnets || local.has_subnet_mapping
+      error_message = "Set at least one of var.subnets or var.subnet_mapping. If both are set, this wrapper prefers var.subnet_mapping."
+    }
+
     precondition {
       condition     = var.internal || var.access_logs != null
       error_message = "Internet-facing ALB/NLB should have access_logs enabled for security compliance, auditing, and troubleshooting. Set access_logs block or set internal = true."

infrastructure/modules/alb/variables.tf

@@ -29,7 +29,14 @@ variable "internal" {
 
 variable "subnets" {
   type        = list(string)
-  description = "List of subnet IDs to attach to the load balancer. For internet-facing ALBs, use public subnets."
+  default     = null
+  description = "List of subnet IDs to attach to the load balancer. If subnet_mapping is also set, this input is ignored and subnet_mapping is used. For internet-facing ALBs, use public subnets."
+}
+
+variable "subnet_mapping" {
+  type        = any
+  default     = null
+  description = "Subnet mapping configuration for the load balancer. Useful for NLB static EIPs (allocation_id per subnet). When set, this takes precedence over subnets. Passed through to the upstream module."
 }
 
 variable "vpc_id" {

infrastructure/modules/ecs-cluster/validations.tf

@@ -38,7 +38,7 @@ resource "terraform_data" "validations" {
     }
 
     precondition {
-      condition     = !var.s3_bucket_encryption_enabled || (var.s3_kms_key_id != null && var.s3_kms_key_id != "")
+      condition     = var.s3_bucket_encryption_enabled != true || (var.s3_kms_key_id != null && var.s3_kms_key_id != "")
       error_message = "When s3_bucket_encryption_enabled = true, s3_kms_key_id is REQUIRED. Provide a KMS key ARN or ID for S3 encryption of ECS Exec session logs."
     }
   }

infrastructure/modules/efs/README.md

@@ -220,9 +220,9 @@ module "replicated_efs" {
 }
 ```
 
-### Secure EFS with TLS 1.2 enforcement and IP restrictions
+### Secure EFS with TLS 1.2 enforcement
 
-Enforce strong TLS version and restrict access to specific network ranges.
+Enforce strong TLS version via file system policy controls.
 
 ```hcl
 module "secure_efs" {
@@ -235,9 +235,8 @@ module "secure_efs" {
 
   kms_key_arn = module.efs_kms.key_arn
 
-  # Enforce TLS 1.2 minimum and restrict to VPC CIDR
+  # Enforce TLS 1.2 minimum
   require_tls_version = "1.2"
-  allowed_source_ips  = ["10.0.0.0/8"]  # Your VPC CIDR
 
   # Prevent accidental deletion (must explicitly allow in custom policy)
   deny_destructive_operations = true
@@ -342,9 +341,8 @@ module "production_efs" {
     destination = "eu-west-1"
   }
 
-  # Security: enforce TLS 1.2 and restrict to VPC
+  # Security: enforce TLS 1.2
   require_tls_version = "1.2"
-  allowed_source_ips  = ["10.0.0.0/8"]
 
   # Access control: application isolation via access points
   access_points = {
@@ -421,26 +419,28 @@ This module automatically adds security-focused policy statements to the EFS fil
 | Statement | Default | Purpose |
 | --- | --- | --- |
 | `DenyUnsecureTransport` | Enabled | Denies all EFS operations over non-TLS connections (`aws:SecureTransport = false`) |
+| `AccessedViaMountTarget` | Enabled (with `deny_unsecure_transport`) | Allows EFS client mount/write/root actions only when accessed via mount targets |
 | `DenyOldTLSVersion` | Disabled | Denies operations using TLS versions older than specified via `var.require_tls_version` |
-| `DenyUnauthorizedSourceIPs` | Disabled | Restricts EFS access to specific CIDR blocks via `var.allowed_source_ips` |
 | `DenyDestructiveOperations` | Enabled | Denies `DeleteFileSystem`, `DeleteAccessPoint`, etc. by default (callers must explicitly allow via custom policy) |
 
+All default policy documents are assembled from conditional `aws_iam_policy_document` data sources and merged via `source_policy_documents`.
+This produces a single combined file system policy document.
+
+Resource scoping: default statements target the created file system ARN, not `*`.
+Using `*` works functionally in an EFS file system policy, but scoping to the concrete file system ARN is preferred for least privilege and clearer intent.
+
 ### Controlling Policy Statements
 
 ```hcl
 # Require TLS 1.2 or higher
 require_tls_version = "1.2"
 
-# Restrict to specific VPC CIDR blocks
-allowed_source_ips = ["10.0.0.0/8", "172.16.0.0/12"]
-
 # Disable automatic deny of destructive operations (not recommended)
 deny_destructive_operations = false
 
 # Disable all automatic policy statements
 deny_unsecure_transport = false
 require_tls_version     = null
-allowed_source_ips      = []
 ```
 
 ### Custom Policy Statements
@@ -457,7 +457,7 @@ file_system_policy = jsonencode({
         AWS = "arn:aws:iam::ACCOUNT:role/AdminRole"
       }
       Action   = ["elasticfilesystem:DeleteFileSystem"]
-      Resource = "*"
+      Resource = "arn:aws:elasticfilesystem:eu-west-2:ACCOUNT_ID:file-system/fs-EXAMPLE"
     }
   ]
 })
@@ -523,14 +523,17 @@ The following cross-variable constraints are enforced in `validations.tf`:
 | [aws_efs_access_point.this](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/efs_access_point) | resource |
 | [aws_efs_file_system_policy.this](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/efs_file_system_policy) | resource |
 | [terraform_data.validations](https://registry.terraform.io/providers/hashicorp/terraform/latest/docs/resources/data) | resource |
+| [aws_iam_policy_document.combined_file_system_policy](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/iam_policy_document) | data source |
+| [aws_iam_policy_document.deny_destructive_operations](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/iam_policy_document) | data source |
+| [aws_iam_policy_document.deny_unsecure_transport](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/iam_policy_document) | data source |
+| [aws_iam_policy_document.require_tls_version](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/iam_policy_document) | data source |
 
 ## Inputs
 
 | Name | Description | Type | Default | Required |
 | ---- | ----------- | ---- | ------- | :------: |
 | <a name="input_access_points"></a> [access\_points](#input\_access\_points) | Map of EFS Access Point configurations for application-level mount points.<br/>Access Points enforce POSIX user identities and enforce a file system root.<br/>Leave as {} to create no access points.<br/><br/>Example:<br/>  access\_points = {<br/>    "app-root" = {<br/>      enforced\_user\_id = "1000"<br/>      root\_directory\_path = "/app"<br/>      permissions\_mode = "755"<br/>    }<br/>    "db-root" = {<br/>      enforced\_user\_id = "1001"<br/>      root\_directory\_path = "/data"<br/>      permissions\_mode = "700"<br/>    }<br/>  } | `any` | `{}` | no |
 | <a name="input_additional_tag_map"></a> [additional\_tag\_map](#input\_additional\_tag\_map) | Additional key-value pairs to add to each map in `tags_as_list_of_maps`. Not added to `tags` or `id`.<br/>This is for some rare cases where resources want additional configuration of tags<br/>and therefore take a list of maps with tag key, value, and additional configuration. | `map(string)` | `{}` | no |
-| <a name="input_allowed_source_ips"></a> [allowed\_source\_ips](#input\_allowed\_source\_ips) | List of CIDR blocks allowed to access the EFS. When set, a Deny statement restricts access to these IPs. Leave as [] to skip IP-based restrictions. | `list(string)` | `[]` | no |
 | <a name="input_application_role"></a> [application\_role](#input\_application\_role) | The role the application is performing | `string` | `"General"` | no |
 | <a name="input_attributes"></a> [attributes](#input\_attributes) | ID element. Additional attributes (e.g. `workers` or `cluster`) to add to `id`,<br/>in the order they appear in the list. New attributes are appended to the<br/>end of the list. The elements of the list are joined by the `delimiter`<br/>and treated as a single ID element. | `list(string)` | `[]` | no |
 | <a name="input_availability_zone_name"></a> [availability\_zone\_name](#input\_availability\_zone\_name) | AWS Availability Zone for One Zone storage class. When set, the file system uses single-AZ storage for lower cost. Leave null for multi-AZ. | `string` | `null` | no |

infrastructure/modules/efs/locals.tf

@@ -6,82 +6,121 @@
 ################################################################
 
 locals {
-  # Naming logic — derive from context, allow caller override
+  # Naming logic - derive from context, allow caller override
   efs_name = var.custom_name != null ? var.custom_name : module.this.id
 
-  # Build list of default security statements to add to the policy
-  default_policy_statements = concat(
-    # Deny unsecure (non-TLS) transport
-    var.deny_unsecure_transport ? [
-      {
-        Sid       = "DenyUnsecureTransport"
-        Effect    = "Deny"
-        Principal = "*"
-        Action    = "elasticfilesystem:*"
-        Resource  = "*"
-        Condition = {
-          Bool = {
-            "aws:SecureTransport" = "false"
-          }
-        }
-      }
-    ] : [],
-
-    # Deny old TLS versions (require TLS 1.2+)
-    var.require_tls_version != null ? [
-      {
-        Sid       = "DenyOldTLSVersion"
-        Effect    = "Deny"
-        Principal = "*"
-        Action    = "elasticfilesystem:*"
-        Resource  = "*"
-        Condition = {
-          StringLessThan = {
-            "aws:TlsVersion" = var.require_tls_version
-          }
-        }
-      }
-    ] : [],
-
-    # Deny access from IPs outside allowed list
-    length(var.allowed_source_ips) > 0 ? [
-      {
-        Sid       = "DenyUnauthorizedSourceIPs"
-        Effect    = "Deny"
-        Principal = "*"
-        Action    = "elasticfilesystem:*"
-        Resource  = "*"
-        Condition = {
-          NotIpAddress = {
-            "aws:SourceIp" = var.allowed_source_ips
-          }
-        }
-      }
-    ] : [],
-
-    # Deny destructive operations by default
-    var.deny_destructive_operations ? [
-      {
-        Sid       = "DenyDestructiveOperations"
-        Effect    = "Deny"
-        Principal = "*"
-        Action = [
-          "elasticfilesystem:DeleteFileSystem",
-          "elasticfilesystem:DeleteAccessPoint",
-          "elasticfilesystem:DeleteMountTarget",
-          "elasticfilesystem:DeleteReplicationConfiguration"
-        ]
-        Resource = "*"
-      }
-    ] : []
+  # Use "*" because the resource-based policy is already scoped to the
+  # specific file system via file_system_id. Using module.efs.arn would
+  # be unknown at plan time on first-time deploys, breaking count.
+  file_system_arn = "*"
+}
+
+data "aws_iam_policy_document" "deny_unsecure_transport" {
+  count = module.this.enabled && var.deny_unsecure_transport ? 1 : 0
+
+  statement {
+    sid       = "DenyUnsecureTransport"
+    effect    = "Deny"
+    actions   = ["*"]
+    resources = [local.file_system_arn]
+
+    principals {
+      type        = "AWS"
+      identifiers = ["*"]
+    }
+
+    condition {
+      test     = "Bool"
+      variable = "aws:SecureTransport"
+      values   = ["false"]
+    }
+  }
+
+  statement {
+    sid    = "AccessedViaMountTarget"
+    effect = "Allow"
+    actions = [
+      "elasticfilesystem:ClientRootAccess",
+      "elasticfilesystem:ClientWrite",
+      "elasticfilesystem:ClientMount"
+    ]
+    resources = [local.file_system_arn]
+
+    principals {
+      type        = "AWS"
+      identifiers = ["*"]
+    }
+
+    condition {
+      test     = "Bool"
+      variable = "elasticfilesystem:AccessedViaMountTarget"
+      values   = ["true"]
+    }
+  }
+}
+
+data "aws_iam_policy_document" "require_tls_version" {
+  count = module.this.enabled && var.require_tls_version != null ? 1 : 0
+
+  statement {
+    sid       = "DenyOldTLSVersion"
+    effect    = "Deny"
+    actions   = ["elasticfilesystem:*"]
+    resources = [local.file_system_arn]
+
+    principals {
+      type        = "AWS"
+      identifiers = ["*"]
+    }
+
+    condition {
+      test     = "StringLessThan"
+      variable = "aws:TlsVersion"
+      values   = [var.require_tls_version]
+    }
+  }
+}
+
+data "aws_iam_policy_document" "deny_destructive_operations" {
+  count = module.this.enabled && var.deny_destructive_operations ? 1 : 0
+
+  statement {
+    sid    = "DenyDestructiveOperations"
+    effect = "Deny"
+    actions = [
+      "elasticfilesystem:DeleteFileSystem",
+      "elasticfilesystem:DeleteAccessPoint",
+      "elasticfilesystem:DeleteMountTarget",
+      "elasticfilesystem:DeleteReplicationConfiguration"
+    ]
+    resources = [local.file_system_arn]
+
+    principals {
+      type        = "AWS"
+      identifiers = ["*"]
+    }
+  }
+}
+
+locals {
+  # Build list of default policy documents to merge.
+  default_policy_documents = concat(
+    length(data.aws_iam_policy_document.deny_unsecure_transport) > 0 ? [data.aws_iam_policy_document.deny_unsecure_transport[0].json] : [],
+    length(data.aws_iam_policy_document.require_tls_version) > 0 ? [data.aws_iam_policy_document.require_tls_version[0].json] : [],
+    length(data.aws_iam_policy_document.deny_destructive_operations) > 0 ? [data.aws_iam_policy_document.deny_destructive_operations[0].json] : []
   )
+}
+
+data "aws_iam_policy_document" "combined_file_system_policy" {
+  count = module.this.enabled && (var.file_system_policy != null || length(local.default_policy_documents) > 0) ? 1 : 0
+
+  source_policy_documents = concat(
+    var.file_system_policy != null ? [var.file_system_policy] : [],
+    local.default_policy_documents
+  )
+}
 
-  # File system policy: merge caller policy with default security statements
-  file_system_policy_doc = length(local.default_policy_statements) > 0 || var.file_system_policy != null ? jsonencode({
-    Version = "2012-10-17"
-    Statement = concat(
-      var.file_system_policy != null ? jsondecode(var.file_system_policy).Statement : [],
-      local.default_policy_statements
-    )
-  }) : null
+locals {
+  # Final merged file system policy JSON.
+  file_system_policy_doc = try(data.aws_iam_policy_document.combined_file_system_policy[0].json, null)
 }

infrastructure/modules/efs/variables.tf

@@ -217,19 +217,6 @@ variable "require_tls_version" {
   }
 }
 
-variable "allowed_source_ips" {
-  description = "List of CIDR blocks allowed to access the EFS. When set, a Deny statement restricts access to these IPs. Leave as [] to skip IP-based restrictions."
-  type        = list(string)
-  default     = []
-
-  validation {
-    condition = alltrue([
-      for cidr in var.allowed_source_ips : can(regex("^[0-9]{1,3}\\.[0-9]{1,3}\\.[0-9]{1,3}\\.[0-9]{1,3}(/[0-9]{1,2})?$", cidr))
-    ])
-    error_message = "allowed_source_ips must contain valid CIDR blocks (e.g., '10.0.0.0/8')."
-  }
-}
-
 variable "deny_destructive_operations" {
   description = "Whether to add a Deny statement for destructive operations (DeleteFileSystem, DeleteAccessPoint) by default. Callers must explicitly allow these via var.file_system_policy. Recommended: true."
   type        = bool