From 7e9b569f68ff148a9eded1db4c46c43ba8716a91 Mon Sep 17 00:00:00 2001 From: Oliver Slater Date: Wed, 29 Jul 2026 17:15:54 +0100 Subject: [PATCH 01/10] fix(vpc)!: update flow log configuration and validation for consumer-managed destinations --- infrastructure/modules/vpc/README.md | 66 ++++++++++++++++++----- infrastructure/modules/vpc/main.tf | 27 ++++------ infrastructure/modules/vpc/outputs.tf | 4 +- infrastructure/modules/vpc/validations.tf | 38 +++++++++++++ infrastructure/modules/vpc/variables.tf | 45 ++++++++-------- 5 files changed, 125 insertions(+), 55 deletions(-) diff --git a/infrastructure/modules/vpc/README.md b/infrastructure/modules/vpc/README.md index 49dd62a2..2662ddde 100644 --- a/infrastructure/modules/vpc/README.md +++ b/infrastructure/modules/vpc/README.md @@ -42,10 +42,19 @@ Subnet CIDRs are auto-calculated from the VPC CIDR across the first three availa - **Naming and tagging** via `context.tf` / `module.this` (tags module v2.5.0) - **NAT gateways** — one per AZ by default, with `single_nat_gateway` option for cost savings -- **VPC Flow Logs** — enabled by default, sending to CloudWatch Logs with a 365-day retention. Implemented as standalone resources (upstream deprecated flow logs in v6.x, removing in v7.0.0) +- **VPC Flow Logs** — enabled by default with destination type configurable as `cloud-watch-logs` or `s3`, implemented via the upstream standalone flow-log submodule - **Security defaults** — default security group adopted and stripped of all rules - **Firewall subnets** — standalone resources (upstream module has no firewall tier) +## Flow Logs + +Flow logs are consumer-managed in this wrapper. + +- Set `flow_log_destination_type` to `cloud-watch-logs` or `s3` +- Always provide `flow_log_destination_arn` +- For `cloud-watch-logs`, also provide `flow_log_cloudwatch_iam_role_arn` +- For `s3`, do not set `flow_log_cloudwatch_iam_role_arn` + ## Usage ### Standard VPC (all subnet tiers) @@ -61,7 +70,27 @@ module "vpc" { vpc_cidr = "10.0.0.0/16" single_nat_gateway = false # one NAT per AZ for HA - flow_log_kms_key_id = aws_kms_key.cloudwatch.arn # optional encryption + # Recommended: consumer-managed CloudWatch destination + IAM role + flow_log_destination_type = "cloud-watch-logs" + flow_log_destination_arn = aws_cloudwatch_log_group.vpc_flow_logs.arn + flow_log_cloudwatch_iam_role_arn = aws_iam_role.vpc_flow_logs.arn +} +``` + +### VPC with consumer-managed S3 flow-log destination + +```terraform +module "vpc" { + source = "git::https://github.com/NHSDigital/screening-terraform-modules-aws.git//infrastructure/modules/vpc?ref=" + + environment = "prod" + service = "bcss" + name = "vpc" + + vpc_cidr = "10.0.0.0/16" + + flow_log_destination_type = "s3" + flow_log_destination_arn = module.logs_bucket.bucket_arn } ``` @@ -88,8 +117,10 @@ module "database_vpc" { # Adjust subnet prefix for /24 VPC (must be larger than /24, e.g., /26, /27, /28) intra_subnet_prefix = 26 - enable_flow_log = true - flow_log_retention_in_days = 30 + enable_flow_log = true + flow_log_destination_type = "cloud-watch-logs" + flow_log_destination_arn = aws_cloudwatch_log_group.database_vpc_flow_logs.arn + flow_log_cloudwatch_iam_role_arn = aws_iam_role.database_vpc_flow_logs.arn } ``` @@ -155,11 +186,21 @@ module "vpc_public" { | `availability_zones` | Explicit AZs for subnet placement; defaults to the first three available AZs | `null` | | `single_nat_gateway` | Use one shared NAT instead of per-AZ | `false` | | `enable_flow_log` | Enable VPC flow logs | `true` | -| `flow_log_retention_in_days` | CloudWatch log retention | `365` | +| `flow_log_destination_type` | Flow log destination type (`cloud-watch-logs` or `s3`) | `cloud-watch-logs` | +| `flow_log_destination_arn` | Consumer-managed destination ARN | `null` | +| `flow_log_cloudwatch_iam_role_arn` | Consumer-managed CloudWatch delivery role ARN (CloudWatch destination only) | `null` | | `flow_log_traffic_type` | ACCEPT, REJECT, or ALL | `ALL` | -| `flow_log_kms_key_id` | KMS key ARN for log encryption | `null` | | `map_public_ip_on_launch` | Auto-assign public IPs in public subnets | `false` | +## Validation + +Cross-variable checks in `validations.tf` enforce flow-log destination requirements: + +- `flow_log_destination_type` must be `cloud-watch-logs` or `s3` +- For `cloud-watch-logs`, `flow_log_destination_arn` is required +- For `cloud-watch-logs`, `flow_log_cloudwatch_iam_role_arn` is required +- For `s3`, `flow_log_destination_arn` is required and `flow_log_cloudwatch_iam_role_arn` must be unset + ## Key outputs | Output | Description | @@ -220,7 +261,6 @@ module "vpc_public" { | [attributes](#input\_attributes) | ID element. Additional attributes (e.g. `workers` or `cluster`) to add to `id`,
in the order they appear in the list. New attributes are appended to the
end of the list. The elements of the list are joined by the `delimiter`
and treated as a single ID element. | `list(string)` | `[]` | no | | [availability\_zones](#input\_availability\_zones) | Availability zones to use for the VPC. Leave null to use the first three available AZs in the current region. | `list(string)` | `null` | no | | [aws\_region](#input\_aws\_region) | The AWS region | `string` | `"eu-west-2"` | no | -| [cloudwatch\_log\_group\_tags](#input\_cloudwatch\_log\_group\_tags) | Additional tags for the CloudWatch log group. | `map(string)` | `{}` | no | | [context](#input\_context) | Single object for setting entire context at once.
See description of individual variables for details.
Leave string and numeric variables as `null` to use default value.
Individual variable settings (non-null) override settings in context object,
except for attributes, tags, and additional\_tag\_map, which are merged. | `any` | 
{
  "additional_tag_map": {},
  "attributes": [],
  "delimiter": null,
  "descriptor_formats": {},
  "enabled": true,
  "environment": null,
  "id_length_limit": null,
  "label_key_case": null,
  "label_order": [],
  "label_value_case": null,
  "labels_as_tags": [
    "unset"
  ],
  "name": null,
  "project": null,
  "regex_replace_chars": null,
  "region": null,
  "service": null,
  "stack": null,
  "tags": {},
  "terraform_source": null,
  "workspace": null
}
| no | | [create\_firewall\_subnets](#input\_create\_firewall\_subnets) | Whether to create firewall subnets (required for Network Firewall routing mode). | `bool` | `true` | no | | [create\_intra\_subnets](#input\_create\_intra\_subnets) | Whether to create intra subnets (no internet access). | `bool` | `true` | no | @@ -237,7 +277,7 @@ module "vpc_public" { | [enable\_dhcp\_options](#input\_enable\_dhcp\_options) | Create a custom DHCP option set and associate it with the VPC. | `bool` | `false` | no | | [enable\_dns\_hostnames](#input\_enable\_dns\_hostnames) | Enable DNS hostnames in the VPC. | `bool` | `true` | no | | [enable\_dns\_support](#input\_enable\_dns\_support) | Enable DNS support in the VPC. | `bool` | `true` | no | -| [enable\_flow\_log](#input\_enable\_flow\_log) | Enable VPC flow logs to CloudWatch Logs. | `bool` | `true` | no | +| [enable\_flow\_log](#input\_enable\_flow\_log) | Enable VPC flow logs. Destination is controlled by flow\_log\_destination\_type. | `bool` | `true` | no | | [enable\_nat\_gateway](#input\_enable\_nat\_gateway) | Provision NAT Gateway(s) for private subnet internet egress. Not applicable if private subnets are disabled. Set to false for database-only VPCs with no internet-routed workloads. | `bool` | `true` | no | | [enable\_network\_firewall](#input\_enable\_network\_firewall) | When true, the VPC module creates firewall subnets, takes over
IGW management from the community module, and reconfigures
routing for AWS Network Firewall inspection:
- Firewall subnets created as standalone resources
- IGW created as a standalone resource (community module's create\_igw = false)
- Firewall subnets get a default route (0.0.0.0/0) to the IGW
- Public subnet default route is NOT created (callers must
inject 0.0.0.0/0 → firewall VPCE at the stack level)
When false (default), no firewall subnets are created, the
community module creates the IGW and public → IGW route as
normal — no Network Firewall in the path. | `bool` | `false` | no | | [enabled](#input\_enabled) | Set to false to prevent the module from creating any resources | `bool` | `null` | no | @@ -245,12 +285,12 @@ module "vpc_public" { | [firewall\_subnet\_prefix](#input\_firewall\_subnet\_prefix) | Prefix length for firewall subnets (e.g. 28 = /28, 16 IPs each). AWS allows /16 to /28. Must be more specific (larger numerically) than vpc\_cidr when auto-calculating. Used only when firewall\_subnets list is empty; when explicit firewall\_subnets are provided, this value is ignored. Highly recommended: /28 to minimize wasted IPs. | `number` | `28` | no | | [firewall\_subnet\_tags](#input\_firewall\_subnet\_tags) | Additional tags for the firewall subnets. | `map(string)` | `{}` | no | | [firewall\_subnets](#input\_firewall\_subnets) | Explicit CIDR blocks for firewall subnets (one per AZ). Leave empty to auto-calculate. | `list(string)` | `[]` | no | -| [flow\_log\_kms\_key\_id](#input\_flow\_log\_kms\_key\_id) | ARN of a KMS key to encrypt the CloudWatch log group. Leave null for no encryption. | `string` | `null` | no | +| [flow\_log\_cloudwatch\_iam\_role\_arn](#input\_flow\_log\_cloudwatch\_iam\_role\_arn) | Existing IAM role ARN used by VPC Flow Logs when destination type is cloud-watch-logs. | `string` | `null` | no | +| [flow\_log\_destination\_arn](#input\_flow\_log\_destination\_arn) | ARN of the flow log destination. Required when flow logs are enabled. For cloud-watch-logs, set to a CloudWatch log group ARN. For s3, set to an S3 bucket ARN. | `string` | `null` | no | +| [flow\_log\_destination\_type](#input\_flow\_log\_destination\_type) | Destination type for VPC flow logs. Supported values in this module: cloud-watch-logs, s3. | `string` | `"cloud-watch-logs"` | no | | [flow\_log\_max\_aggregation\_interval](#input\_flow\_log\_max\_aggregation\_interval) | The maximum interval of time (seconds) during which a flow of packets is captured. Valid values: 60 (1 min) or 600 (10 min). | `number` | `600` | no | -| [flow\_log\_retention\_in\_days](#input\_flow\_log\_retention\_in\_days) | Number of days to retain VPC flow logs in CloudWatch. | `number` | `365` | no | | [flow\_log\_tags](#input\_flow\_log\_tags) | Additional tags for the VPC flow log. | `map(string)` | `{}` | no | | [flow\_log\_traffic\_type](#input\_flow\_log\_traffic\_type) | The type of traffic to capture. Valid values: ACCEPT, REJECT, ALL. | `string` | `"ALL"` | no | -| [iam\_role\_tags](#input\_iam\_role\_tags) | Additional tags for the IAM role used by the VPC flow log. | `map(string)` | `{}` | no | | [id\_length\_limit](#input\_id\_length\_limit) | Limit `id` to this many characters (minimum 6).
Set to `0` for unlimited length.
Set to `null` for keep the existing setting, which defaults to `0`.
Does not affect `id_full`. | `number` | `null` | no | | [intra\_subnet\_prefix](#input\_intra\_subnet\_prefix) | Prefix length for intra subnets with no internet route (e.g. 23 = /23, 512 IPs each). AWS allows /16 to /28. Must be more specific (larger numerically) than vpc\_cidr when auto-calculating. Used only when intra\_subnets list is empty; when explicit intra\_subnets are provided, this value is ignored. | `number` | `23` | no | | [intra\_subnet\_tags](#input\_intra\_subnet\_tags) | Additional tags for the intra (no-internet) subnets. | `map(string)` | `{}` | no | @@ -298,8 +338,8 @@ module "vpc_public" { | [firewall\_subnet\_ids](#output\_firewall\_subnet\_ids) | List of IDs of the firewall subnets. | | [firewall\_subnets\_cidr\_blocks](#output\_firewall\_subnets\_cidr\_blocks) | List of CIDR blocks of the firewall subnets. | | [flow\_log\_arn](#output\_flow\_log\_arn) | The ARN of the VPC Flow Log. | -| [flow\_log\_cloudwatch\_log\_group\_arn](#output\_flow\_log\_cloudwatch\_log\_group\_arn) | The ARN of the CloudWatch Log Group for VPC flow logs. | -| [flow\_log\_iam\_role\_arn](#output\_flow\_log\_iam\_role\_arn) | The ARN of the IAM role used by VPC flow logs. | +| [flow\_log\_cloudwatch\_log\_group\_arn](#output\_flow\_log\_cloudwatch\_log\_group\_arn) | The CloudWatch log group ARN returned by the flow-log submodule. May be null for s3 destinations or when external resources are used. | +| [flow\_log\_iam\_role\_arn](#output\_flow\_log\_iam\_role\_arn) | The IAM role ARN returned by the flow-log submodule. May be null for s3 destinations or when external resources are used. | | [flow\_log\_id](#output\_flow\_log\_id) | The ID of the VPC Flow Log. | | [igw\_arn](#output\_igw\_arn) | The ARN of the Internet Gateway. | | [igw\_id](#output\_igw\_id) | The ID of the Internet Gateway. | diff --git a/infrastructure/modules/vpc/main.tf b/infrastructure/modules/vpc/main.tf index db9d5ecc..9f3e1aa8 100644 --- a/infrastructure/modules/vpc/main.tf +++ b/infrastructure/modules/vpc/main.tf @@ -191,10 +191,10 @@ resource "aws_route_table_association" "edge" { # terraform-aws-modules/vpc/aws (the root module's built-in # flow log support is deprecated in v6.x, removed in v7.0.0). # -# The submodule creates: -# - CloudWatch Log Group -# - IAM Role with scoped trust policy -# - VPC Flow Log resource +# Flow log destination and IAM role are managed by the consumer. +# Supported destination types in this wrapper: +# - cloud-watch-logs +# - s3 ################################################################ module "flow_log" { @@ -206,24 +206,17 @@ module "flow_log" { name = "${module.this.id}-flow-log" vpc_id = module.vpc.vpc_id - # CloudWatch destination - log_destination_type = "cloud-watch-logs" - cloudwatch_log_group_name = "/vpc/${module.this.id}/flow-logs" - cloudwatch_log_group_use_name_prefix = false - cloudwatch_log_group_retention_in_days = var.flow_log_retention_in_days - cloudwatch_log_group_kms_key_id = var.flow_log_kms_key_id + # Destination configuration + log_destination_type = var.flow_log_destination_type + log_destination = var.flow_log_destination_arn - # IAM role (created by the submodule with scoped trust policy) - create_iam_role = true - iam_role_name = "${module.this.id}-flow-logs" - iam_role_use_name_prefix = false + # CloudWatch destinations require a consumer-managed IAM role. + iam_role_arn = var.flow_log_cloudwatch_iam_role_arn traffic_type = var.flow_log_traffic_type max_aggregation_interval = var.flow_log_max_aggregation_interval - cloudwatch_log_group_tags = var.cloudwatch_log_group_tags - flow_log_tags = var.flow_log_tags - iam_role_tags = var.iam_role_tags + flow_log_tags = var.flow_log_tags tags = module.this.tags } diff --git a/infrastructure/modules/vpc/outputs.tf b/infrastructure/modules/vpc/outputs.tf index f8407110..caaff8ce 100644 --- a/infrastructure/modules/vpc/outputs.tf +++ b/infrastructure/modules/vpc/outputs.tf @@ -154,12 +154,12 @@ output "flow_log_arn" { } output "flow_log_cloudwatch_log_group_arn" { - description = "The ARN of the CloudWatch Log Group for VPC flow logs." + description = "The CloudWatch log group ARN returned by the flow-log submodule. May be null for s3 destinations or when external resources are used." value = module.flow_log.cloudwatch_log_group_arn } output "flow_log_iam_role_arn" { - description = "The ARN of the IAM role used by VPC flow logs." + description = "The IAM role ARN returned by the flow-log submodule. May be null for s3 destinations or when external resources are used." value = module.flow_log.iam_role_arn } diff --git a/infrastructure/modules/vpc/validations.tf b/infrastructure/modules/vpc/validations.tf index df93ece3..3519b253 100644 --- a/infrastructure/modules/vpc/validations.tf +++ b/infrastructure/modules/vpc/validations.tf @@ -11,6 +11,7 @@ # * enable_network_firewall requires firewall subnets to be created # * single_nat_gateway requires private subnets to be created # * Explicit subnet CIDR lists must have correct length (equal to az_count) +# * Flow log destination-specific inputs are set correctly # ################################################################ @@ -99,5 +100,42 @@ resource "terraform_data" "validations" { condition = length(var.intra_subnets) == 0 || length(var.intra_subnets) == local.az_count error_message = "intra_subnets must be empty or have exactly ${local.az_count} entries (one per AZ); found ${length(var.intra_subnets)}." } + + precondition { + condition = !var.enable_flow_log || contains(["cloud-watch-logs", "s3"], var.flow_log_destination_type) + error_message = "When enable_flow_log is true, flow_log_destination_type must be cloud-watch-logs or s3." + } + + precondition { + condition = !( + var.enable_flow_log && + var.flow_log_destination_type == "cloud-watch-logs" && + (var.flow_log_destination_arn == null || trim(var.flow_log_destination_arn) == "") + ) + error_message = "flow_log_destination_arn must be set when flow_log_destination_type is cloud-watch-logs." + } + + precondition { + condition = !( + var.enable_flow_log && + var.flow_log_destination_type == "cloud-watch-logs" && + (var.flow_log_cloudwatch_iam_role_arn == null || trim(var.flow_log_cloudwatch_iam_role_arn) == "") + ) + error_message = "flow_log_cloudwatch_iam_role_arn must be set when flow_log_destination_type is cloud-watch-logs." + } + + precondition { + condition = !( + var.enable_flow_log && + var.flow_log_destination_type == "s3" && + (var.flow_log_destination_arn == null || trim(var.flow_log_destination_arn) == "") + ) + error_message = "flow_log_destination_arn must be set when flow_log_destination_type is s3." + } + + precondition { + condition = !(var.enable_flow_log && var.flow_log_destination_type == "s3" && var.flow_log_cloudwatch_iam_role_arn != null && trim(var.flow_log_cloudwatch_iam_role_arn) != "") + error_message = "flow_log_cloudwatch_iam_role_arn must be null when flow_log_destination_type is s3." + } } } diff --git a/infrastructure/modules/vpc/variables.tf b/infrastructure/modules/vpc/variables.tf index 936ca933..18a01387 100644 --- a/infrastructure/modules/vpc/variables.tf +++ b/infrastructure/modules/vpc/variables.tf @@ -304,15 +304,32 @@ variable "firewall_subnet_tags" { ################################################################ variable "enable_flow_log" { - description = "Enable VPC flow logs to CloudWatch Logs." + description = "Enable VPC flow logs. Destination is controlled by flow_log_destination_type." type = bool default = true } -variable "flow_log_retention_in_days" { - description = "Number of days to retain VPC flow logs in CloudWatch." - type = number - default = 365 +variable "flow_log_destination_type" { + description = "Destination type for VPC flow logs. Supported values in this module: cloud-watch-logs, s3." + type = string + default = "cloud-watch-logs" + + validation { + condition = contains(["cloud-watch-logs", "s3"], var.flow_log_destination_type) + error_message = "flow_log_destination_type must be one of cloud-watch-logs or s3." + } +} + +variable "flow_log_destination_arn" { + description = "ARN of the flow log destination. Required when flow logs are enabled. For cloud-watch-logs, set to a CloudWatch log group ARN. For s3, set to an S3 bucket ARN." + type = string + default = null +} + +variable "flow_log_cloudwatch_iam_role_arn" { + description = "Existing IAM role ARN used by VPC Flow Logs when destination type is cloud-watch-logs." + type = string + default = null } variable "flow_log_traffic_type" { @@ -326,12 +343,6 @@ variable "flow_log_traffic_type" { } } -variable "flow_log_kms_key_id" { - description = "ARN of a KMS key to encrypt the CloudWatch log group. Leave null for no encryption." - type = string - default = null -} - variable "flow_log_max_aggregation_interval" { description = "The maximum interval of time (seconds) during which a flow of packets is captured. Valid values: 60 (1 min) or 600 (10 min)." type = number @@ -343,23 +354,11 @@ variable "flow_log_max_aggregation_interval" { } } -variable "cloudwatch_log_group_tags" { - description = "Additional tags for the CloudWatch log group." - type = map(string) - default = {} -} - variable "flow_log_tags" { description = "Additional tags for the VPC flow log." type = map(string) default = {} } -variable "iam_role_tags" { - description = "Additional tags for the IAM role used by the VPC flow log." - type = map(string) - default = {} -} - ################################################################ # VPC Endpoints From 54af0931367d1885c004c29733bcc2b40e21902e Mon Sep 17 00:00:00 2001 From: Oliver Slater Date: Wed, 29 Jul 2026 17:35:47 +0100 Subject: [PATCH 02/10] fix(efs): remove IP restrictions and clarify TLS enforcement in documentation and policy --- infrastructure/modules/efs/README.md | 24 ++-- infrastructure/modules/efs/locals.tf | 166 ++++++++++++++---------- infrastructure/modules/efs/variables.tf | 13 -- 3 files changed, 111 insertions(+), 92 deletions(-) diff --git a/infrastructure/modules/efs/README.md b/infrastructure/modules/efs/README.md index a89581d0..ef65c061 100644 --- a/infrastructure/modules/efs/README.md +++ b/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 statements are assembled from conditional `aws_iam_policy_document` data sources and merged into `local.default_policy_statement` before attachment. +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" } ] }) diff --git a/infrastructure/modules/efs/locals.tf b/infrastructure/modules/efs/locals.tf index 45d35821..8c9d37ee 100644 --- a/infrastructure/modules/efs/locals.tf +++ b/infrastructure/modules/efs/locals.tf @@ -6,82 +6,114 @@ ################################################################ 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 + # Scope default statements to this file system only. + file_system_arn = module.efs.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 = ["elasticfilesystem:*"] + 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 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 = "*" - } - ] : [] + default_policy_statement = concat( + try(jsondecode(data.aws_iam_policy_document.deny_unsecure_transport[0].json).Statement, []), + try(jsondecode(data.aws_iam_policy_document.require_tls_version[0].json).Statement, []), + try(jsondecode(data.aws_iam_policy_document.deny_destructive_operations[0].json).Statement, []) ) # 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({ + file_system_policy_doc = length(local.default_policy_statement) > 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 + local.default_policy_statement ) }) : null } diff --git a/infrastructure/modules/efs/variables.tf b/infrastructure/modules/efs/variables.tf index d7c07fc9..5270b8be 100644 --- a/infrastructure/modules/efs/variables.tf +++ b/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 From b0723825f41c0ecdefde5492d040d278b678ff0a Mon Sep 17 00:00:00 2001 From: Oliver Slater Date: Wed, 29 Jul 2026 17:36:03 +0100 Subject: [PATCH 03/10] fix(efs): update policy document handling for improved security statement management --- infrastructure/modules/efs/locals.tf | 31 ++++++++++++++++------------ 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/infrastructure/modules/efs/locals.tf b/infrastructure/modules/efs/locals.tf index 8c9d37ee..c74e6b6a 100644 --- a/infrastructure/modules/efs/locals.tf +++ b/infrastructure/modules/efs/locals.tf @@ -101,19 +101,24 @@ data "aws_iam_policy_document" "deny_destructive_operations" { } locals { - # Build list of default security statements to add to the policy - default_policy_statement = concat( - try(jsondecode(data.aws_iam_policy_document.deny_unsecure_transport[0].json).Statement, []), - try(jsondecode(data.aws_iam_policy_document.require_tls_version[0].json).Statement, []), - try(jsondecode(data.aws_iam_policy_document.deny_destructive_operations[0].json).Statement, []) + # 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_statement) > 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_statement - ) - }) : 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) } From c7ec5a6392dda51ea7846ca19405e4d0e20926d0 Mon Sep 17 00:00:00 2001 From: Oliver Slater Date: Wed, 29 Jul 2026 17:38:09 +0100 Subject: [PATCH 04/10] docs(efs): update policy document assembly method for clarity and accuracy --- infrastructure/modules/efs/README.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/infrastructure/modules/efs/README.md b/infrastructure/modules/efs/README.md index ef65c061..3d9f06e0 100644 --- a/infrastructure/modules/efs/README.md +++ b/infrastructure/modules/efs/README.md @@ -423,7 +423,7 @@ This module automatically adds security-focused policy statements to the EFS fil | `DenyOldTLSVersion` | Disabled | Denies operations using TLS versions older than specified via `var.require_tls_version` | | `DenyDestructiveOperations` | Enabled | Denies `DeleteFileSystem`, `DeleteAccessPoint`, etc. by default (callers must explicitly allow via custom policy) | -All default policy statements are assembled from conditional `aws_iam_policy_document` data sources and merged into `local.default_policy_statement` before attachment. +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 `*`. @@ -523,6 +523,10 @@ 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 @@ -530,7 +534,6 @@ The following cross-variable constraints are enforced in `validations.tf`: | ---- | ----------- | ---- | ------- | :------: | | [access\_points](#input\_access\_points) | Map of EFS Access Point configurations for application-level mount points.
Access Points enforce POSIX user identities and enforce a file system root.
Leave as {} to create no access points.

Example:
access\_points = {
"app-root" = {
enforced\_user\_id = "1000"
root\_directory\_path = "/app"
permissions\_mode = "755"
}
"db-root" = {
enforced\_user\_id = "1001"
root\_directory\_path = "/data"
permissions\_mode = "700"
}
} | `any` | `{}` | no | | [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`.
This is for some rare cases where resources want additional configuration of tags
and therefore take a list of maps with tag key, value, and additional configuration. | `map(string)` | `{}` | no | -| [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 | | [application\_role](#input\_application\_role) | The role the application is performing | `string` | `"General"` | no | | [attributes](#input\_attributes) | ID element. Additional attributes (e.g. `workers` or `cluster`) to add to `id`,
in the order they appear in the list. New attributes are appended to the
end of the list. The elements of the list are joined by the `delimiter`
and treated as a single ID element. | `list(string)` | `[]` | no | | [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 | From 50e67b8bfb518c68546da78f06a8d71d686408a6 Mon Sep 17 00:00:00 2001 From: Oliver Slater Date: Wed, 29 Jul 2026 18:01:11 +0100 Subject: [PATCH 05/10] fix(efs): update deny_unsecure_transport policy actions to deny all actions --- infrastructure/modules/efs/locals.tf | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/infrastructure/modules/efs/locals.tf b/infrastructure/modules/efs/locals.tf index c74e6b6a..630169e5 100644 --- a/infrastructure/modules/efs/locals.tf +++ b/infrastructure/modules/efs/locals.tf @@ -19,7 +19,7 @@ data "aws_iam_policy_document" "deny_unsecure_transport" { statement { sid = "DenyUnsecureTransport" effect = "Deny" - actions = ["elasticfilesystem:*"] + actions = ["*"] resources = [local.file_system_arn] principals { From 9686abda4f81f8ccdd199655e1dee2b7646c7708 Mon Sep 17 00:00:00 2001 From: Oliver Slater Date: Tue, 4 Aug 2026 20:04:54 +0100 Subject: [PATCH 06/10] fix(ecs): correct precondition logic for S3 bucket encryption requirement --- infrastructure/modules/ecs-cluster/validations.tf | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/infrastructure/modules/ecs-cluster/validations.tf b/infrastructure/modules/ecs-cluster/validations.tf index c7b0bdfd..336a268e 100644 --- a/infrastructure/modules/ecs-cluster/validations.tf +++ b/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." } } From df19430e7dcd253cfce12cb4468c566af8fad5c5 Mon Sep 17 00:00:00 2001 From: Oliver Slater Date: Wed, 5 Aug 2026 00:52:06 +0100 Subject: [PATCH 07/10] fix(alb): enhance subnet handling and validation logic for load balancer configuration --- infrastructure/modules/alb/locals.tf | 7 +++++++ infrastructure/modules/alb/main.tf | 3 ++- infrastructure/modules/alb/validations.tf | 5 +++++ infrastructure/modules/alb/variables.tf | 9 ++++++++- 4 files changed, 22 insertions(+), 2 deletions(-) diff --git a/infrastructure/modules/alb/locals.tf b/infrastructure/modules/alb/locals.tf index e762afe3..5c7fa4c1 100644 --- a/infrastructure/modules/alb/locals.tf +++ b/infrastructure/modules/alb/locals.tf @@ -1,5 +1,12 @@ locals { 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 diff --git a/infrastructure/modules/alb/main.tf b/infrastructure/modules/alb/main.tf index e9470e4a..ebb2210d 100644 --- a/infrastructure/modules/alb/main.tf +++ b/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. diff --git a/infrastructure/modules/alb/validations.tf b/infrastructure/modules/alb/validations.tf index 36c2d576..1ce267cb 100644 --- a/infrastructure/modules/alb/validations.tf +++ b/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." diff --git a/infrastructure/modules/alb/variables.tf b/infrastructure/modules/alb/variables.tf index b90242bc..11ab5197 100644 --- a/infrastructure/modules/alb/variables.tf +++ b/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" { From 0d78fd6308c17604fa171a26cf11200175ff2ad3 Mon Sep 17 00:00:00 2001 From: Oliver Slater Date: Wed, 5 Aug 2026 09:53:41 +0100 Subject: [PATCH 08/10] fix(alb): update README and locals for subnet mapping configuration clarity --- infrastructure/modules/alb/README.md | 3 ++- infrastructure/modules/alb/locals.tf | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/infrastructure/modules/alb/README.md b/infrastructure/modules/alb/README.md index 238a6775..493518d1 100644 --- a/infrastructure/modules/alb/README.md +++ b/infrastructure/modules/alb/README.md @@ -660,7 +660,8 @@ For **NLB (network load balancer):** | [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 | | [service\_category](#input\_service\_category) | The tag service\_category | `string` | `"n/a"` | no | | [stack](#input\_stack) | ID element. The name of the stack/component, e.g. `database`, `web`, `waf`, `eks` | `string` | `null` | no | -| [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 | +| [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 | +| [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 | | [tag\_version](#input\_tag\_version) | Used to identify the tagging version in use | `string` | `"1.0"` | no | | [tags](#input\_tags) | Additional tags (e.g. `{'BusinessUnit': 'XYZ'}`).
Neither the tag keys nor the tag values will be modified by this module. | `map(string)` | `{}` | no | | [target\_groups](#input\_target\_groups) | Map of target group configurations to create. Passed directly to the upstream module.
See https://registry.terraform.io/modules/terraform-aws-modules/alb/aws/latest
for full schema documentation. | `any` | `{}` | no | diff --git a/infrastructure/modules/alb/locals.tf b/infrastructure/modules/alb/locals.tf index 5c7fa4c1..a8e87ac3 100644 --- a/infrastructure/modules/alb/locals.tf +++ b/infrastructure/modules/alb/locals.tf @@ -1,5 +1,5 @@ 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 From 00e544ff239bda027e737e1e8a54b4b998e86e0a Mon Sep 17 00:00:00 2001 From: Oliver Slater Date: Wed, 5 Aug 2026 10:02:59 +0100 Subject: [PATCH 09/10] fix(s3-bucket): add optional log delivery policy attachments and related variables --- infrastructure/modules/s3-bucket/README.md | 38 +++++++++---- infrastructure/modules/s3-bucket/main.tf | 13 +++++ infrastructure/modules/s3-bucket/variables.tf | 54 +++++++++++++++++++ 3 files changed, 95 insertions(+), 10 deletions(-) diff --git a/infrastructure/modules/s3-bucket/README.md b/infrastructure/modules/s3-bucket/README.md index 4ffa0240..af083af4 100644 --- a/infrastructure/modules/s3-bucket/README.md +++ b/infrastructure/modules/s3-bucket/README.md @@ -7,16 +7,17 @@ the shared `context.tf` for naming and tagging. ## What this module enforces -| Control | How it is enforced | -| ------------------------ | --------------------------------------------------------------------------------- | -| Ownership | `object_ownership = "BucketOwnerEnforced"` (ACLs disabled) | -| Transport (TLS) | `attach_deny_insecure_transport_policy` + `attach_require_latest_tls_policy` | -| Encryption at rest | SSE-S3 by default; SSE-KMS when `kms_master_key_arn` is set | -| Encryption on PUT | Denies unencrypted, incorrect-header, SSEC and wrong-KMS-key PutObject calls | -| Public access | All four S3 public-access-block toggles set to true | -| Versioning | Enabled by default; opt out with `versioning_enabled = false` | -| Globally unique name | Default name is `-` | -| Logging | Optional, delivered to a caller-supplied target bucket via `var.logging` | +|Control|How it is enforced| +|---|---| +|Ownership|`object_ownership = "BucketOwnerEnforced"` (ACLs disabled)| +|Transport (TLS)|`attach_deny_insecure_transport_policy` + `attach_require_latest_tls_policy`| +|Encryption at rest|SSE-S3 by default; SSE-KMS when `kms_master_key_arn` is set| +|Encryption on PUT|Denies unencrypted, incorrect-header, SSEC and wrong-KMS-key PutObject calls| +|Public access|All four S3 public-access-block toggles set to true| +|Versioning|Enabled by default; opt out with `versioning_enabled = false`| +|Globally unique name|Default name is `-`| +|Logging|Optional, delivered to a caller-supplied target bucket via `var.logging`| +|Service log sink policy|Optional attachments for S3/ELB/ALB-NLB/CloudTrail/WAF log delivery| ## Usage @@ -65,6 +66,12 @@ module "log_bucket" { name = "s3-access-logs" versioning_enabled = false + + attach_access_log_delivery_policy = true + access_log_delivery_policy_source_accounts = ["123456789012"] + + attach_lb_log_delivery_policy = true + lb_log_delivery_policy_source_organizations = ["o-example1234"] } ``` @@ -75,6 +82,8 @@ module "log_bucket" { intervention. * `force_destroy` defaults to `false`. Only set it to `true` for short-lived buckets that will never hold business data. +* Log-delivery policy attachments are opt-in (`false` by default) so standard + buckets do not receive unnecessary policy statements. * Custom bucket policies provided via `var.policy` are merged by the upstream module with the platform's deny-non-TLS and deny-unencrypted statements; you do not need to restate those rules. @@ -118,9 +127,17 @@ No resources. | Name | Description | Type | Default | Required | | ---- | ----------- | ---- | ------- | :------: | +| [access\_log\_delivery\_policy\_source\_accounts](#input\_access\_log\_delivery\_policy\_source\_accounts) | Optional list of AWS account IDs that are allowed to deliver S3 access logs to this bucket. | `list(string)` | `[]` | no | +| [access\_log\_delivery\_policy\_source\_buckets](#input\_access\_log\_delivery\_policy\_source\_buckets) | Optional list of S3 bucket ARNs that are allowed to deliver S3 access logs to this bucket. | `list(string)` | `[]` | no | +| [access\_log\_delivery\_policy\_source\_organizations](#input\_access\_log\_delivery\_policy\_source\_organizations) | Optional list of AWS Organisation IDs that are allowed to deliver S3 access logs to this bucket. | `list(string)` | `[]` | no | | [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`.
This is for some rare cases where resources want additional configuration of tags
and therefore take a list of maps with tag key, value, and additional configuration. | `map(string)` | `{}` | no | | [application\_role](#input\_application\_role) | The role the application is performing | `string` | `"General"` | no | +| [attach\_access\_log\_delivery\_policy](#input\_attach\_access\_log\_delivery\_policy) | Whether to attach the S3 access log delivery policy for this bucket. | `bool` | `false` | no | +| [attach\_cloudtrail\_log\_delivery\_policy](#input\_attach\_cloudtrail\_log\_delivery\_policy) | Whether to attach the CloudTrail log delivery policy for this bucket. | `bool` | `false` | no | | [attach\_deny\_incorrect\_kms\_key\_sse](#input\_attach\_deny\_incorrect\_kms\_key\_sse) | Whether to attach a bucket policy statement denying PutObject calls that reference a KMS key other than `var.kms_master_key_arn` | `bool` | `null` | no | +| [attach\_elb\_log\_delivery\_policy](#input\_attach\_elb\_log\_delivery\_policy) | Whether to attach the ELB log delivery policy for this bucket. | `bool` | `false` | no | +| [attach\_lb\_log\_delivery\_policy](#input\_attach\_lb\_log\_delivery\_policy) | Whether to attach the ALB/NLB log delivery policy for this bucket. | `bool` | `false` | no | +| [attach\_waf\_log\_delivery\_policy](#input\_attach\_waf\_log\_delivery\_policy) | Whether to attach the WAF log delivery policy for this bucket. | `bool` | `false` | no | | [attributes](#input\_attributes) | ID element. Additional attributes (e.g. `workers` or `cluster`) to add to `id`,
in the order they appear in the list. New attributes are appended to the
end of the list. The elements of the list are joined by the `delimiter`
and treated as a single ID element. | `list(string)` | `[]` | no | | [aws\_region](#input\_aws\_region) | The AWS region | `string` | `"eu-west-2"` | no | | [bucket\_name](#input\_bucket\_name) | Optional explicit bucket name. When null, the bucket is named `-` to keep S3's global namespace collision-free. | `string` | `null` | no | @@ -139,6 +156,7 @@ No resources. | [label\_order](#input\_label\_order) | The order in which the labels (ID elements) appear in the `id`.
Defaults to ["namespace", "environment", "stage", "name", "attributes"].
You can omit any of the 6 labels ("tenant" is the 6th), but at least one must be present. | `list(string)` | `null` | no | | [label\_value\_case](#input\_label\_value\_case) | Controls the letter case of ID elements (labels) as included in `id`,
set as tag values, and output by this module individually.
Does not affect values of tags passed in via the `tags` input.
Possible values: `lower`, `title`, `upper` and `none` (no transformation).
Set this to `title` and set `delimiter` to `""` to yield Pascal Case IDs.
Default value: `lower`. | `string` | `null` | no | | [labels\_as\_tags](#input\_labels\_as\_tags) | Set of labels (ID elements) to include as tags in the `tags` output.
Default is to include all labels.
Tags with empty values will not be included in the `tags` output.
Set to `[]` to suppress all generated tags.
**Notes:**
The value of the `name` tag, if included, will be the `id`, not the `name`.
Unlike other `null-label` inputs, the initial setting of `labels_as_tags` cannot be
changed in later chained modules. Attempts to change it will be silently ignored. | `set(string)` | 
[
  "default"
]
| no | +| [lb\_log\_delivery\_policy\_source\_organizations](#input\_lb\_log\_delivery\_policy\_source\_organizations) | Optional list of AWS Organisation IDs that are allowed to deliver ALB/NLB logs to this bucket. | `list(string)` | `[]` | no | | [lifecycle\_rule](#input\_lifecycle\_rule) | List of lifecycle rules forwarded to the upstream module. | `any` | `[]` | no | | [logging](#input\_logging) | Map describing access-log delivery to a target bucket. Leave as
`{}` to disable logging. Example:
logging = {
target\_bucket = "my-log-bucket"
target\_prefix = "s3/access-logs/"
} | `any` | `{}` | no | | [name](#input\_name) | ID element. Usually the component or solution name, e.g. 'app' or 'jenkins'.
This is the only ID element not also included as a `tag`.
The "name" tag is set to the full `id` string. There is no tag with the value of the `name` input. | `string` | `null` | no | diff --git a/infrastructure/modules/s3-bucket/main.tf b/infrastructure/modules/s3-bucket/main.tf index 709ac064..28dedb9e 100644 --- a/infrastructure/modules/s3-bucket/main.tf +++ b/infrastructure/modules/s3-bucket/main.tf @@ -75,6 +75,19 @@ module "s3_bucket" { # ---------------------------------------------------------------- logging = var.logging + # Optional log-delivery policy attachments for buckets that + # receive service logs. + attach_access_log_delivery_policy = var.attach_access_log_delivery_policy + attach_elb_log_delivery_policy = var.attach_elb_log_delivery_policy + attach_lb_log_delivery_policy = var.attach_lb_log_delivery_policy + attach_cloudtrail_log_delivery_policy = var.attach_cloudtrail_log_delivery_policy + attach_waf_log_delivery_policy = var.attach_waf_log_delivery_policy + + access_log_delivery_policy_source_buckets = var.access_log_delivery_policy_source_buckets + access_log_delivery_policy_source_accounts = var.access_log_delivery_policy_source_accounts + access_log_delivery_policy_source_organizations = var.access_log_delivery_policy_source_organizations + lb_log_delivery_policy_source_organizations = var.lb_log_delivery_policy_source_organizations + # ---------------------------------------------------------------- # Optional pass-throughs. # ---------------------------------------------------------------- diff --git a/infrastructure/modules/s3-bucket/variables.tf b/infrastructure/modules/s3-bucket/variables.tf index 77ec4417..72e26e86 100644 --- a/infrastructure/modules/s3-bucket/variables.tf +++ b/infrastructure/modules/s3-bucket/variables.tf @@ -54,6 +54,60 @@ variable "logging" { default = {} } +variable "attach_access_log_delivery_policy" { + description = "Whether to attach the S3 access log delivery policy for this bucket." + type = bool + default = false +} + +variable "attach_elb_log_delivery_policy" { + description = "Whether to attach the ELB log delivery policy for this bucket." + type = bool + default = false +} + +variable "attach_lb_log_delivery_policy" { + description = "Whether to attach the ALB/NLB log delivery policy for this bucket." + type = bool + default = false +} + +variable "attach_cloudtrail_log_delivery_policy" { + description = "Whether to attach the CloudTrail log delivery policy for this bucket." + type = bool + default = false +} + +variable "attach_waf_log_delivery_policy" { + description = "Whether to attach the WAF log delivery policy for this bucket." + type = bool + default = false +} + +variable "access_log_delivery_policy_source_buckets" { + description = "Optional list of S3 bucket ARNs that are allowed to deliver S3 access logs to this bucket." + type = list(string) + default = [] +} + +variable "access_log_delivery_policy_source_accounts" { + description = "Optional list of AWS account IDs that are allowed to deliver S3 access logs to this bucket." + type = list(string) + default = [] +} + +variable "access_log_delivery_policy_source_organizations" { + description = "Optional list of AWS Organisation IDs that are allowed to deliver S3 access logs to this bucket." + type = list(string) + default = [] +} + +variable "lb_log_delivery_policy_source_organizations" { + description = "Optional list of AWS Organisation IDs that are allowed to deliver ALB/NLB logs to this bucket." + type = list(string) + default = [] +} + variable "policy" { description = "Optional custom bucket policy JSON document. The upstream module merges this with the deny-non-TLS and deny-unencrypted statements generated above." type = string From c92fd1be51e47cba7bde7bbf5d241d68558d2c4c Mon Sep 17 00:00:00 2001 From: Uzair Haroon Date: Wed, 5 Aug 2026 11:35:18 +0100 Subject: [PATCH 10/10] fix(efs): use wildcard resource in file system policy to fix first-time deploys --- infrastructure/modules/efs/locals.tf | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/infrastructure/modules/efs/locals.tf b/infrastructure/modules/efs/locals.tf index 630169e5..4211bc22 100644 --- a/infrastructure/modules/efs/locals.tf +++ b/infrastructure/modules/efs/locals.tf @@ -9,8 +9,10 @@ locals { # Naming logic - derive from context, allow caller override efs_name = var.custom_name != null ? var.custom_name : module.this.id - # Scope default statements to this file system only. - file_system_arn = module.efs.arn + # 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" {