fix(infra): address PR review feedback on tf modules

Deployer role: - Add servicediscovery actions; module always creates Cloud Map namespace and service so the policy must grant CreatePrivateDnsNamespace etc. - Make Route53 + ACM permissions unconditional. The server module always issues an ACM cert and writes Route53 records (no CloudFront default-cert path exists), so gating these on route53_zone_ids was broken. Split Route53 into hosted-zone management (always) plus record-set mutation (scoped to caller-supplied zones, falls back to *). - Remove unused cloudfront:* statement; no CloudFront resources in module. - Replace acm:* wildcard with explicit cert-management action set. Server module: - qdrant_image_tag is now nullable with default null and validated against use_external_qdrant, so external-qdrant callers can omit it instead of passing a sentinel "unused" value. - task_role_arn and efs_id outputs marked sensitive; qdrant_dns_name returns null when use_external_qdrant = true. - ALB SG now has matching IPv6 egress rule (was v4-only). - nextcloud_url validates the https:// scheme. - random_pet.subdomain keeper includes zone_name so a zone migration that preserves zone_id still triggers regeneration. - Pin required_version >= 1.9 on both modules. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-01 23:24:09 +02:00
parent ccf4b91bf9
commit e4c552cd19
10 changed files with 151 additions and 62 deletions
@@ -228,8 +228,8 @@ module "nextcloud_mcp_server" {
  secret_arn = "arn:aws:secretsmanager:eu-west-1:123456789012:secret:nextcloud-mcp-aws-env-XXXXXX"

  use_external_qdrant = true
-  qdrant_image_tag    = "unused" # required input but ignored when use_external_qdrant=true
  qdrant_collection   = "nextcloud-mcp"
+  # qdrant_image_tag intentionally omitted — only required when use_external_qdrant = false.
 }
 ```

@@ -15,15 +15,26 @@ IAM role and least-privilege policy scoped to deploy the
  client will use — so any "works for me, breaks for them" gap surfaces in
  testing rather than at the client.

-## Modes
+## What the policy grants

-The role's permissions are mode-aware via inputs:
+Always granted (the server module always creates these resources):

-| Mode | Inputs | What gets granted |
-|---|---|---|
-| **CloudFront default cert** (recommended) | (defaults) | ECS, ALB, EFS, CloudFront, scoped IAM/logs/secrets, EC2 SG + describe |
-| **Custom domain** | `route53_zone_ids = [...]` | + Route53 (scoped to listed zones) and ACM |
-| **Secret managed in same TF** | `allow_secret_create = true` | + Secrets Manager create/update/delete (scoped to `secret_name_prefix`) |
+- ECS, ALB, EFS, Cloud Map (servicediscovery), Bedrock describe
+- ACM cert management (scoped action set, resources `*` since cert ARNs
+  aren't known at policy-write time)
+- Route53 hosted-zone reads + private-zone CRUD (Cloud Map needs the
+  latter); record-set mutation is scoped to caller-supplied zones via
+  `route53_zone_ids`
+- IAM (scoped to `role/ecs/${module_name_prefix}-*`), CloudWatch Logs
+  (scoped to `/ecs/${module_name_prefix}*`), Secrets Manager read (scoped
+  to `${secret_name_prefix}*`), EC2 SG + describe
+
+Conditional via inputs:
+
+| Input | Effect |
+|---|---|
+| `route53_zone_ids = [...]` | scopes `route53:ChangeResourceRecordSets` to the listed zones (otherwise falls back to `*`) |
+| `allow_secret_create = true` | adds Secrets Manager create/update/delete (scoped to `secret_name_prefix`) |

 ## Cross-account assume from your account

@@ -45,13 +56,14 @@ provider "aws" {

 | Name | Version |
 | ---- | ------- |
+| <a name="requirement_terraform"></a> [terraform](#requirement\_terraform) | >= 1.9 |
 | <a name="requirement_aws"></a> [aws](#requirement\_aws) | ~> 6.0 |

 ## Providers

 | Name | Version |
 | ---- | ------- |
-| <a name="provider_aws"></a> [aws](#provider\_aws) | 6.43.0 |
+| <a name="provider_aws"></a> [aws](#provider\_aws) | ~> 6.0 |

 ## Modules

@@ -77,7 +89,7 @@ No modules.
 | <a name="input_module_name_prefix"></a> [module\_name\_prefix](#input\_module\_name\_prefix) | The `var.name` value passed to the nextcloud-mcp-server module. Used to<br/>scope IAM/logs/secrets ARNs. Defaults match the module default; change<br/>only if the module is instantiated with a non-default name. | `string` | `"nextcloud-mcp-server"` | no |
 | <a name="input_role_name"></a> [role\_name](#input\_role\_name) | Name of the deployer IAM role. | `string` | `"nextcloud-mcp-deployer"` | no |
 | <a name="input_role_path"></a> [role\_path](#input\_role\_path) | IAM path for the deployer role and its policy. | `string` | `"/clients/"` | no |
-| <a name="input_route53_zone_ids"></a> [route53\_zone\_ids](#input\_route53\_zone\_ids) | Route53 hosted zone IDs the deployer is allowed to mutate. Only needed<br/>in the module's custom-domain mode. Leave empty (the default) for the<br/>CloudFront-default-cert path, which requires no DNS or ACM permissions. | `list(string)` | `[]` | no |
+| <a name="input_route53_zone_ids"></a> [route53\_zone\_ids](#input\_route53\_zone\_ids) | Route53 public hosted zone IDs the deployer is allowed to mutate. The<br/>server module always creates Route53 records (ALB alias + ACM DNS-01<br/>validation), so this should be set to the zone(s) the module's<br/>`zone_id` input points at. Leaving it empty falls back to `*` as a<br/>convenience but is not recommended in production — scope it. | `list(string)` | `[]` | no |
 | <a name="input_secret_name_prefix"></a> [secret\_name\_prefix](#input\_secret\_name\_prefix) | Secrets Manager name prefix the deployer can read (and optionally<br/>create, see `allow_secret_create`). The module accepts a secret ARN as<br/>input; this prefix scopes the deployer's access to secrets matching<br/>that name pattern. | `string` | `"nextcloud-mcp"` | no |
 | <a name="input_trusted_principal_arns"></a> [trusted\_principal\_arns](#input\_trusted\_principal\_arns) | Principal ARNs allowed to assume this role. For testing in your own<br/>account: the user/role you want to assume from. For client deployments:<br/>typically a single root-account ARN of the deploying party (e.g.<br/>"arn:aws:iam::<your-account-id>:root"), with MFA or external-id<br/>conditions added at the trust-policy level if required. | `list(string)` | n/a | yes |

@@ -1,4 +1,5 @@
 terraform {
+  required_version = ">= 1.9"
  required_providers {
    aws = {
      source  = "hashicorp/aws"
@@ -71,13 +72,29 @@ data "aws_iam_policy_document" "deployer" {
    resources = ["*"]
  }

-  # --- Edge: CloudFront ---
+  # --- Service Discovery / Cloud Map ---
  #
-  # CloudFront has no resource-ARN scoping for distribution create. Cache /
-  # origin-request policies the module ships are also account-wide objects.
+  # Module always creates `aws_service_discovery_private_dns_namespace` (in
+  # service_discovery.tf) and `aws_service_discovery_service` (in qdrant.tf).
+  # Cloud Map APIs don't accept resource-ARN scoping at create time; the
+  # blast radius is bounded by the SG/account boundary already.
  statement {
-    sid       = "CloudFrontService"
-    actions   = ["cloudfront:*"]
+    sid = "ServiceDiscoveryService"
+    actions = [
+      "servicediscovery:CreatePrivateDnsNamespace",
+      "servicediscovery:DeleteNamespace",
+      "servicediscovery:GetNamespace",
+      "servicediscovery:ListNamespaces",
+      "servicediscovery:GetOperation",
+      "servicediscovery:CreateService",
+      "servicediscovery:DeleteService",
+      "servicediscovery:GetService",
+      "servicediscovery:UpdateService",
+      "servicediscovery:ListServices",
+      "servicediscovery:TagResource",
+      "servicediscovery:UntagResource",
+      "servicediscovery:ListTagsForResource",
+    ]
    resources = ["*"]
  }

@@ -246,45 +263,75 @@ data "aws_iam_policy_document" "deployer" {
    resources = ["*"]
  }

-  # --- Route53 + ACM (custom-domain mode only) ---
+  # --- Route53 ---
  #
-  # Default mode (CloudFront with `*.cloudfront.net` cert) needs neither.
-  # Opt in by passing `route53_zone_ids` for the zones the deployer is
-  # allowed to mutate.
-  dynamic "statement" {
-    for_each = length(var.route53_zone_ids) > 0 ? [1] : []
-    content {
-      sid = "Route53RecordsForZones"
+  # The server module always creates ACM cert + Route53 records (alias to ALB
+  # and DNS-01 validation), so these statements are unconditional. Cloud Map's
+  # CreatePrivateDnsNamespace also creates a Route53 private hosted zone
+  # under the caller's identity, which needs the hosted-zone management
+  # actions below.
+
+  # Hosted-zone CRUD (private zones for Cloud Map, public-zone reads for the
+  # caller-supplied zone). ChangeResourceRecordSets is the destructive action
+  # and is split into a separate statement scoped to caller-supplied zones.
+  statement {
+    sid = "Route53HostedZoneManage"
    actions = [
-        "route53:ChangeResourceRecordSets",
-        "route53:ListResourceRecordSets",
+      "route53:CreateHostedZone",
      "route53:GetHostedZone",
+      "route53:DeleteHostedZone",
+      "route53:ListHostedZones",
+      "route53:ListHostedZonesByVPC",
+      "route53:AssociateVPCWithHostedZone",
+      "route53:DisassociateVPCFromHostedZone",
+      "route53:ChangeTagsForResource",
+      "route53:ListTagsForResource",
+      "route53:ListResourceRecordSets",
    ]
-      resources = [
-        for zone_id in var.route53_zone_ids :
-        "arn:${local.partition}:route53:::hostedzone/${zone_id}"
-      ]
+    resources = ["*"]
  }
+
+  # ChangeResourceRecordSets is scoped to caller-supplied public zones when
+  # `route53_zone_ids` is set. Falls back to `*` only as a convenience for
+  # callers who haven't enumerated their zones — strongly recommend setting
+  # the variable.
+  statement {
+    sid     = "Route53RecordsForZones"
+    actions = ["route53:ChangeResourceRecordSets"]
+    resources = (
+      length(var.route53_zone_ids) > 0
+      ? [for zone_id in var.route53_zone_ids : "arn:${local.partition}:route53:::hostedzone/${zone_id}"]
+      : ["*"]
+    )
  }

  # GetChange takes a change-id, not a zone ARN — must be `*`.
-  dynamic "statement" {
-    for_each = length(var.route53_zone_ids) > 0 ? [1] : []
-    content {
+  statement {
    sid       = "Route53GetChange"
    actions   = ["route53:GetChange"]
    resources = ["*"]
  }
-  }

-  dynamic "statement" {
-    for_each = length(var.route53_zone_ids) > 0 ? [1] : []
-    content {
-      sid       = "AcmService"
-      actions   = ["acm:*"]
+  # --- ACM ---
+  #
+  # The server module always issues an ACM cert. ACM cert ARNs are only
+  # known after RequestCertificate, so the destructive actions can't be
+  # scoped further at policy-write time. Restrict the action set instead of
+  # granting `acm:*`.
+  statement {
+    sid = "AcmManageCertificates"
+    actions = [
+      "acm:RequestCertificate",
+      "acm:DescribeCertificate",
+      "acm:DeleteCertificate",
+      "acm:ListCertificates",
+      "acm:ListTagsForCertificate",
+      "acm:AddTagsToCertificate",
+      "acm:RemoveTagsFromCertificate",
+      "acm:UpdateCertificateOptions",
+    ]
    resources = ["*"]
  }
-  }

  # --- KMS describe (AWS-managed keys for default EFS / Secrets encryption) ---
  statement {
@@ -60,9 +60,11 @@ variable "allow_secret_create" {

 variable "route53_zone_ids" {
  description = <<-EOT
-    Route53 hosted zone IDs the deployer is allowed to mutate. Only needed
-    in the module's custom-domain mode. Leave empty (the default) for the
-    CloudFront-default-cert path, which requires no DNS or ACM permissions.
+    Route53 public hosted zone IDs the deployer is allowed to mutate. The
+    server module always creates Route53 records (ALB alias + ACM DNS-01
+    validation), so this should be set to the zone(s) the module's
+    `zone_id` input points at. Leaving it empty falls back to `*` as a
+    convenience but is not recommended in production — scope it.
  EOT
  type        = list(string)
  default     = []
@@ -3,6 +3,7 @@

 | Name | Version |
 | ---- | ------- |
+| <a name="requirement_terraform"></a> [terraform](#requirement\_terraform) | >= 1.9 |
 | <a name="requirement_aws"></a> [aws](#requirement\_aws) | ~> 6.0 |
 | <a name="requirement_random"></a> [random](#requirement\_random) | ~> 3.6 |

@@ -10,8 +11,8 @@

 | Name | Version |
 | ---- | ------- |
-| <a name="provider_aws"></a> [aws](#provider\_aws) | 6.43.0 |
-| <a name="provider_random"></a> [random](#provider\_random) | 3.8.1 |
+| <a name="provider_aws"></a> [aws](#provider\_aws) | ~> 6.0 |
+| <a name="provider_random"></a> [random](#provider\_random) | ~> 3.6 |

 ## Modules

@@ -58,6 +59,7 @@ No modules.
 | [aws_service_discovery_private_dns_namespace.this](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/service_discovery_private_dns_namespace) | resource |
 | [aws_service_discovery_service.qdrant](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/service_discovery_service) | resource |
 | [aws_vpc_security_group_egress_rule.alb_all_v4](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/vpc_security_group_egress_rule) | resource |
+| [aws_vpc_security_group_egress_rule.alb_all_v6](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/vpc_security_group_egress_rule) | resource |
 | [aws_vpc_security_group_egress_rule.qdrant_all_v4](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/vpc_security_group_egress_rule) | resource |
 | [aws_vpc_security_group_egress_rule.qdrant_all_v6](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/vpc_security_group_egress_rule) | resource |
 | [aws_vpc_security_group_egress_rule.task_all_v4](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/vpc_security_group_egress_rule) | resource |
@@ -100,7 +102,7 @@ No modules.
 | <a name="input_qdrant_collection"></a> [qdrant\_collection](#input\_qdrant\_collection) | Qdrant collection name. Set to a stable value (anything other than upstream's default 'nextcloud\_content') so the upstream config doesn't fall through to its hostname-based auto-naming, which churns the collection on every rolling deploy. | `string` | `"nextcloud-mcp"` | no |
 | <a name="input_qdrant_cpu"></a> [qdrant\_cpu](#input\_qdrant\_cpu) | Qdrant Fargate task vCPU units (1024 = 1 vCPU) | `number` | `512` | no |
 | <a name="input_qdrant_image"></a> [qdrant\_image](#input\_qdrant\_image) | Qdrant container image (without tag) | `string` | `"qdrant/qdrant"` | no |
-| <a name="input_qdrant_image_tag"></a> [qdrant\_image\_tag](#input\_qdrant\_image\_tag) | Qdrant container image tag (e.g., v1.15.0). Pin to a specific release; avoid :latest. Unused when use\_external\_qdrant = true. | `string` | n/a | yes |
+| <a name="input_qdrant_image_tag"></a> [qdrant\_image\_tag](#input\_qdrant\_image\_tag) | Qdrant container image tag (e.g., v1.15.0). Pin to a specific release; avoid :latest. Required only when use\_external\_qdrant = false; omit (or pass null) when use\_external\_qdrant = true. | `string` | `null` | no |
 | <a name="input_qdrant_memory"></a> [qdrant\_memory](#input\_qdrant\_memory) | Qdrant Fargate task memory (MiB) | `number` | `1024` | no |
 | <a name="input_secret_arn"></a> [secret\_arn](#input\_secret\_arn) | ARN of the Secrets Manager secret holding JSON {host, client\_id, client\_secret, token\_encryption\_key, webhook\_secret} | `string` | n/a | yes |
 | <a name="input_use_external_qdrant"></a> [use\_external\_qdrant](#input\_use\_external\_qdrant) | When true, skip the in-AWS Qdrant ECS task and source QDRANT\_URL/QDRANT\_API\_KEY from the Secrets Manager secret (keys: qdrant\_url, qdrant\_api\_key). When false, run an in-AWS Qdrant Fargate task and point the MCP server at it via Cloud Map DNS. | `bool` | `false` | no |
@@ -117,12 +119,12 @@ No modules.
 | <a name="output_alb_dns_name"></a> [alb\_dns\_name](#output\_alb\_dns\_name) | n/a |
 | <a name="output_ecs_cluster_name"></a> [ecs\_cluster\_name](#output\_ecs\_cluster\_name) | n/a |
 | <a name="output_ecs_service_name"></a> [ecs\_service\_name](#output\_ecs\_service\_name) | n/a |
-| <a name="output_efs_id"></a> [efs\_id](#output\_efs\_id) | n/a |
+| <a name="output_efs_id"></a> [efs\_id](#output\_efs\_id) | EFS file-system ID. Marked sensitive — surfacing it in CI logs invites enumeration of mount targets. |
 | <a name="output_fqdn"></a> [fqdn](#output\_fqdn) | Fully-qualified domain name |
 | <a name="output_log_group_name"></a> [log\_group\_name](#output\_log\_group\_name) | n/a |
-| <a name="output_qdrant_dns_name"></a> [qdrant\_dns\_name](#output\_qdrant\_dns\_name) | Internal DNS name where mcp-server reaches qdrant |
+| <a name="output_qdrant_dns_name"></a> [qdrant\_dns\_name](#output\_qdrant\_dns\_name) | Internal DNS name where mcp-server reaches qdrant (null when use\_external\_qdrant = true). |
 | <a name="output_qdrant_service_name"></a> [qdrant\_service\_name](#output\_qdrant\_service\_name) | Qdrant ECS service name (null when use\_external\_qdrant = true) |
 | <a name="output_subdomain"></a> [subdomain](#output\_subdomain) | Generated random subdomain (label only, without the zone) |
-| <a name="output_task_role_arn"></a> [task\_role\_arn](#output\_task\_role\_arn) | n/a |
+| <a name="output_task_role_arn"></a> [task\_role\_arn](#output\_task\_role\_arn) | Task role ARN. Marked sensitive — knowing the ARN is the first step to abusing it via SSRF/role-confusion. |
 | <a name="output_url"></a> [url](#output\_url) | Public HTTPS URL of the MCP server |
 <!-- END_TF_DOCS -->
@@ -46,6 +46,12 @@ resource "aws_vpc_security_group_egress_rule" "alb_all_v4" {
  ip_protocol       = "-1"
 }

+resource "aws_vpc_security_group_egress_rule" "alb_all_v6" {
+  security_group_id = aws_security_group.alb.id
+  cidr_ipv6         = "::/0"
+  ip_protocol       = "-1"
+}
+
 resource "aws_lb" "this" {
  name               = var.name
  load_balancer_type = "application"
@@ -3,8 +3,11 @@ resource "random_pet" "subdomain" {
  separator = "-"

  # Stable across applies; regenerate only if we point at a different zone.
+  # `zone_name` is in the keeper too so a zone migration that keeps the same
+  # zone_id (rare but possible across providers) still triggers regeneration.
  keepers = {
    zone_id   = var.zone_id
+    zone_name = var.zone_name
  }
 }

@@ -1,4 +1,5 @@
 terraform {
+  required_version = ">= 1.9"
  required_providers {
    aws = {
      source  = "hashicorp/aws"
@@ -22,7 +22,9 @@ output "ecs_service_name" {
 }

 output "efs_id" {
+  description = "EFS file-system ID. Marked sensitive — surfacing it in CI logs invites enumeration of mount targets."
  value       = aws_efs_file_system.this.id
+  sensitive   = true
 }

 output "log_group_name" {
@@ -30,7 +32,9 @@ output "log_group_name" {
 }

 output "task_role_arn" {
+  description = "Task role ARN. Marked sensitive — knowing the ARN is the first step to abusing it via SSRF/role-confusion."
  value       = aws_iam_role.task.arn
+  sensitive   = true
 }

 output "alb_dns_name" {
@@ -43,6 +47,6 @@ output "qdrant_service_name" {
 }

 output "qdrant_dns_name" {
-  description = "Internal DNS name where mcp-server reaches qdrant"
-  value       = "qdrant.${aws_service_discovery_private_dns_namespace.this.name}"
+  description = "Internal DNS name where mcp-server reaches qdrant (null when use_external_qdrant = true)."
+  value       = var.use_external_qdrant ? null : "qdrant.${aws_service_discovery_private_dns_namespace.this.name}"
 }
@@ -22,6 +22,11 @@ variable "private_subnet_ids" {
 variable "nextcloud_url" {
  description = "Public URL of the Nextcloud instance the MCP server pairs with (e.g., https://cloud.example.com). Used to advertise the OIDC discovery endpoint via /api/v1/status so the astrolabe Nextcloud app can discover Nextcloud's oidc_provider as the IdP instead of falling back to http://localhost."
  type        = string
+
+  validation {
+    condition     = startswith(var.nextcloud_url, "https://")
+    error_message = "nextcloud_url must include the https:// scheme; OIDC discovery is composed by appending /.well-known/openid-configuration."
+  }
 }

 variable "zone_id" {
@@ -117,8 +122,15 @@ variable "qdrant_collection" {
 }

 variable "qdrant_image_tag" {
-  description = "Qdrant container image tag (e.g., v1.15.0). Pin to a specific release; avoid :latest. Unused when use_external_qdrant = true."
+  description = "Qdrant container image tag (e.g., v1.15.0). Pin to a specific release; avoid :latest. Required only when use_external_qdrant = false; omit (or pass null) when use_external_qdrant = true."
  type        = string
+  nullable    = true
+  default     = null
+
+  validation {
+    condition     = var.use_external_qdrant || var.qdrant_image_tag != null
+    error_message = "qdrant_image_tag is required when use_external_qdrant = false."
+  }
 }

 variable "use_external_qdrant" {