From e4c552cd193cbf67e3ef1457d8ab6d7ad73ec67e Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 1 May 2026 23:24:09 +0200 Subject: [PATCH] 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) --- infra/terraform/README.md | 2 +- .../nextcloud-mcp-deployer-role/README.md | 30 +++-- .../nextcloud-mcp-deployer-role/main.tf | 121 ++++++++++++------ .../nextcloud-mcp-deployer-role/vars.tf | 8 +- .../terraform/nextcloud-mcp-server/README.md | 14 +- infra/terraform/nextcloud-mcp-server/alb.tf | 6 + infra/terraform/nextcloud-mcp-server/dns.tf | 5 +- infra/terraform/nextcloud-mcp-server/main.tf | 1 + .../terraform/nextcloud-mcp-server/outputs.tf | 12 +- infra/terraform/nextcloud-mcp-server/vars.tf | 14 +- 10 files changed, 151 insertions(+), 62 deletions(-) diff --git a/infra/terraform/README.md b/infra/terraform/README.md index d6ceca2b..370a7f1e 100644 --- a/infra/terraform/README.md +++ b/infra/terraform/README.md @@ -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. } ``` diff --git a/infra/terraform/nextcloud-mcp-deployer-role/README.md b/infra/terraform/nextcloud-mcp-deployer-role/README.md index f4baf22b..480b85e7 100644 --- a/infra/terraform/nextcloud-mcp-deployer-role/README.md +++ b/infra/terraform/nextcloud-mcp-deployer-role/README.md @@ -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 | | ---- | ------- | +| [terraform](#requirement\_terraform) | >= 1.9 | | [aws](#requirement\_aws) | ~> 6.0 | ## Providers | Name | Version | | ---- | ------- | -| [aws](#provider\_aws) | 6.43.0 | +| [aws](#provider\_aws) | ~> 6.0 | ## Modules @@ -77,7 +89,7 @@ No modules. | [module\_name\_prefix](#input\_module\_name\_prefix) | The `var.name` value passed to the nextcloud-mcp-server module. Used to
scope IAM/logs/secrets ARNs. Defaults match the module default; change
only if the module is instantiated with a non-default name. | `string` | `"nextcloud-mcp-server"` | no | | [role\_name](#input\_role\_name) | Name of the deployer IAM role. | `string` | `"nextcloud-mcp-deployer"` | no | | [role\_path](#input\_role\_path) | IAM path for the deployer role and its policy. | `string` | `"/clients/"` | no | -| [route53\_zone\_ids](#input\_route53\_zone\_ids) | 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. | `list(string)` | `[]` | no | +| [route53\_zone\_ids](#input\_route53\_zone\_ids) | 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. | `list(string)` | `[]` | no | | [secret\_name\_prefix](#input\_secret\_name\_prefix) | Secrets Manager name prefix the deployer can read (and optionally
create, see `allow_secret_create`). The module accepts a secret ARN as
input; this prefix scopes the deployer's access to secrets matching
that name pattern. | `string` | `"nextcloud-mcp"` | no | | [trusted\_principal\_arns](#input\_trusted\_principal\_arns) | Principal ARNs allowed to assume this role. For testing in your own
account: the user/role you want to assume from. For client deployments:
typically a single root-account ARN of the deploying party (e.g.
"arn:aws:iam:::root"), with MFA or external-id
conditions added at the trust-policy level if required. | `list(string)` | n/a | yes | diff --git a/infra/terraform/nextcloud-mcp-deployer-role/main.tf b/infra/terraform/nextcloud-mcp-deployer-role/main.tf index 1d7e90af..8a679362 100644 --- a/infra/terraform/nextcloud-mcp-deployer-role/main.tf +++ b/infra/terraform/nextcloud-mcp-deployer-role/main.tf @@ -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,44 +263,74 @@ 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" - actions = [ - "route53:ChangeResourceRecordSets", - "route53:ListResourceRecordSets", - "route53:GetHostedZone", - ] - resources = [ - for zone_id in var.route53_zone_ids : - "arn:${local.partition}:route53:::hostedzone/${zone_id}" - ] - } + # 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:CreateHostedZone", + "route53:GetHostedZone", + "route53:DeleteHostedZone", + "route53:ListHostedZones", + "route53:ListHostedZonesByVPC", + "route53:AssociateVPCWithHostedZone", + "route53:DisassociateVPCFromHostedZone", + "route53:ChangeTagsForResource", + "route53:ListTagsForResource", + "route53:ListResourceRecordSets", + ] + 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 { - sid = "Route53GetChange" - actions = ["route53:GetChange"] - resources = ["*"] - } + statement { + sid = "Route53GetChange" + actions = ["route53:GetChange"] + resources = ["*"] } - dynamic "statement" { - for_each = length(var.route53_zone_ids) > 0 ? [1] : [] - content { - sid = "AcmService" - actions = ["acm:*"] - resources = ["*"] - } + # --- 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) --- diff --git a/infra/terraform/nextcloud-mcp-deployer-role/vars.tf b/infra/terraform/nextcloud-mcp-deployer-role/vars.tf index e69b477f..7b0a1881 100644 --- a/infra/terraform/nextcloud-mcp-deployer-role/vars.tf +++ b/infra/terraform/nextcloud-mcp-deployer-role/vars.tf @@ -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 = [] diff --git a/infra/terraform/nextcloud-mcp-server/README.md b/infra/terraform/nextcloud-mcp-server/README.md index 7bc2bc3c..7778e534 100644 --- a/infra/terraform/nextcloud-mcp-server/README.md +++ b/infra/terraform/nextcloud-mcp-server/README.md @@ -3,6 +3,7 @@ | Name | Version | | ---- | ------- | +| [terraform](#requirement\_terraform) | >= 1.9 | | [aws](#requirement\_aws) | ~> 6.0 | | [random](#requirement\_random) | ~> 3.6 | @@ -10,8 +11,8 @@ | Name | Version | | ---- | ------- | -| [aws](#provider\_aws) | 6.43.0 | -| [random](#provider\_random) | 3.8.1 | +| [aws](#provider\_aws) | ~> 6.0 | +| [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. | [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 | | [qdrant\_cpu](#input\_qdrant\_cpu) | Qdrant Fargate task vCPU units (1024 = 1 vCPU) | `number` | `512` | no | | [qdrant\_image](#input\_qdrant\_image) | Qdrant container image (without tag) | `string` | `"qdrant/qdrant"` | no | -| [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 | +| [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 | | [qdrant\_memory](#input\_qdrant\_memory) | Qdrant Fargate task memory (MiB) | `number` | `1024` | no | | [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 | | [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. | [alb\_dns\_name](#output\_alb\_dns\_name) | n/a | | [ecs\_cluster\_name](#output\_ecs\_cluster\_name) | n/a | | [ecs\_service\_name](#output\_ecs\_service\_name) | n/a | -| [efs\_id](#output\_efs\_id) | n/a | +| [efs\_id](#output\_efs\_id) | EFS file-system ID. Marked sensitive — surfacing it in CI logs invites enumeration of mount targets. | | [fqdn](#output\_fqdn) | Fully-qualified domain name | | [log\_group\_name](#output\_log\_group\_name) | n/a | -| [qdrant\_dns\_name](#output\_qdrant\_dns\_name) | Internal DNS name where mcp-server reaches qdrant | +| [qdrant\_dns\_name](#output\_qdrant\_dns\_name) | Internal DNS name where mcp-server reaches qdrant (null when use\_external\_qdrant = true). | | [qdrant\_service\_name](#output\_qdrant\_service\_name) | Qdrant ECS service name (null when use\_external\_qdrant = true) | | [subdomain](#output\_subdomain) | Generated random subdomain (label only, without the zone) | -| [task\_role\_arn](#output\_task\_role\_arn) | n/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. | | [url](#output\_url) | Public HTTPS URL of the MCP server | \ No newline at end of file diff --git a/infra/terraform/nextcloud-mcp-server/alb.tf b/infra/terraform/nextcloud-mcp-server/alb.tf index 94dbac51..943cf774 100644 --- a/infra/terraform/nextcloud-mcp-server/alb.tf +++ b/infra/terraform/nextcloud-mcp-server/alb.tf @@ -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" diff --git a/infra/terraform/nextcloud-mcp-server/dns.tf b/infra/terraform/nextcloud-mcp-server/dns.tf index 32a39181..deded904 100644 --- a/infra/terraform/nextcloud-mcp-server/dns.tf +++ b/infra/terraform/nextcloud-mcp-server/dns.tf @@ -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_id = var.zone_id + zone_name = var.zone_name } } diff --git a/infra/terraform/nextcloud-mcp-server/main.tf b/infra/terraform/nextcloud-mcp-server/main.tf index ee8a1009..df66335a 100644 --- a/infra/terraform/nextcloud-mcp-server/main.tf +++ b/infra/terraform/nextcloud-mcp-server/main.tf @@ -1,4 +1,5 @@ terraform { + required_version = ">= 1.9" required_providers { aws = { source = "hashicorp/aws" diff --git a/infra/terraform/nextcloud-mcp-server/outputs.tf b/infra/terraform/nextcloud-mcp-server/outputs.tf index f1486f0e..d8153e24 100644 --- a/infra/terraform/nextcloud-mcp-server/outputs.tf +++ b/infra/terraform/nextcloud-mcp-server/outputs.tf @@ -22,7 +22,9 @@ output "ecs_service_name" { } output "efs_id" { - value = aws_efs_file_system.this.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" { - value = aws_iam_role.task.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}" } diff --git a/infra/terraform/nextcloud-mcp-server/vars.tf b/infra/terraform/nextcloud-mcp-server/vars.tf index 2364a8bf..85dd4289 100644 --- a/infra/terraform/nextcloud-mcp-server/vars.tf +++ b/infra/terraform/nextcloud-mcp-server/vars.tf @@ -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" {