[james-nesbitt]

Ready for review.

Updated 2026-11-06 after the user's review of this PR (a8c4860). Two things this PR originally did were rejected as breaking existing external usecases, and have been reverted to a non-breaking shape at his instruction: admin_cidrs was a required variable with no default, and the generated launchpad.yaml referenced credentials only by environment variable. Both are fixed below, and the claims in this body have been rewritten to match what the branch now does rather than what it did. The firewall tuning, the CIDR ingress scoping and the password generation/handoff he named as good are unchanged.

This PR was draft until its test-side companion could land, because the suites that provision these examples are in test/smoke/*.go, test/integration/ and test/test_manager.go, which PR #686 also edited. #686 merged, the companion landed at 259de1d, and the branch was brought up to date with main by a merge commit (a85b720).

What

• aws-simple/launchpad.tf: external ingress scoped to 22 and 5986 from var.admin_cidrs, and 443/6443 from var.admin_cidrs plus the VPC CIDR. Node-to-node traffic inside the group unchanged.

• aws-complete/sg.tf: 22 and WinRM from var.admin_cidrs; 5985 removed, not narrowed. aws-complete/launchpad.tf: manager 443/6443 scoped the same way.

• var.admin_cidrs added to both examples, defaulting to ["0.0.0.0/0"] and documented as open. See "Why admin_cidrs defaults open" below; this is deliberate and at the user's instruction, not an oversight. The non-empty validation is kept, so an explicit empty list is still rejected. Documented at the declaration, in both terraform.tfvars.template files and in each example's README.md, with the one-line change that narrows it.

• var.windows_password declared sensitive = true in both examples, and its empty default replaced by default = null plus a validation that fires only when a nodegroup uses a windows_ platform. Each example derives local.windows_password, substituting "" for null, because consumers take a string.

• aws-complete: separate manager and worker IAM roles and instance profiles, assigned by nodegroup role. CCM control-plane policy attached to managers only; worker (ECR) policy and the EBS CSI policy attached to both.

• modules/aws-ccm/iam.tf: single Resource: * statement split into discovery, creation, tagging and tag-constrained mutation. ec2:CreateTags and elasticloadbalancing:AddTags are conditioned on ec2:CreateAction / elasticloadbalancing:CreateAction, so a tag can only be applied as part of a create call this policy grants; an unconditioned tagging grant would let the role tag an unrelated resource as cluster-owned and then mutate it under the condition meant to constrain it.

  Not verifiable here: whether the cloud controller manager only ever tags during creation. If it tags an already-created resource in a separate call, that call is now denied. aws-complete has no smoke coverage, so this has no gate behind it.

• Both examples: the generated launchpad.yaml is written from the terraform variables that carry the credentials — var.launchpad.mke_connect.password and var.windows_password — so the document is usable with nothing exported into the environment. Supplying them from the environment is still available, through Terraform's own mechanism rather than a launchpad-specific one: TF_VAR_launchpad carries the object as JSON because the MKE password is nested inside it, and TF_VAR_windows_password carries the Windows credential. Documented in both templates, both READMEs and above each emission block. The environment augments the variables; it is not the only route.

• aws-complete/provision.tf: a caller-supplied per-nodegroup user_data is no longer discarded. The platform data was merged over the nodegroup definition, so the platform default always won. The composition and its rationale are copied verbatim from aws-simple/provision.tf: a caller's user_data replaces the platform default on ssh platforms and is appended after it on winrm platforms, where the platform default resets the Administrator password and stands up the WinRM HTTPS listener launchpad needs to connect at all. This is Finding A from the post-merge review of Adopt provision-aws 0.3.0 in the terraform examples #680, found after that PR merged and authorised into this ticket; it is not part of PRODENG-3929's original text.

Why

The aws-simple rule the file itself labelled [BAD RULE] accepted every protocol and port from 0.0.0.0/0 on every node, and that module is what every CI smoke run provisions. aws-complete opened 22 and 5985-5986 from anywhere, 5985 being cleartext WinRM. One IAM role for all nodes meant workers held cloud-controller-manager permissions — security-group, route, volume and load-balancer mutation at Resource: * — that only the control plane uses.

Why admin_cidrs defaults open. It defaults to ["0.0.0.0/0"] on purpose, at the user's instruction. An earlier revision of this PR made it required with no default, on the reasoning that a default would reintroduce the open rule silently; that broke every existing external terraform.tfvars, which failed at variable resolution before any provider call. The ruling is that defaulting to 0.0.0.0/0 does produce a real risk, but the risk is documented and easy to handle, whereas a required variable makes the example harder to use and breaks callers. So the exposure is kept as the default and stated plainly wherever a user will look. The VPC CIDR is required on 443/6443 because the MKE and MSR load balancers reach nodes from inside the VPC, and health checks would fail if only admin_cidrs were allowed.

The inline-credential change is withdrawn. It came from PRODENG-3935's review rather than this ticket's text: 3935 documented sourcing credentials from the environment while the repo's own examples inlined them. The shape this PR used to close that finding — referencing LAUNCHPAD_MKE_ADMIN_PASSWORD and LAUNCHPAD_WINRM_PASSWORD from the generated YAML, so the environment was the only route — is the shape the user ruled against, because it hides an important input and makes the example harder to run. That finding is therefore open again and is not closed by this PR. TF_VAR_<name> is offered as the documented way to keep a secret off disk, but it is an option, not a requirement, so it does not close a finding about credentials being inlined.

Known consequence of the ingress scoping — NodePort and the swarm ports

Stated here because a reader will otherwise reconstruct it wrongly, and because defaulting admin_cidrs to 0.0.0.0/0 does not hide it.

main's aws-simple rule was protocol -1, ports 0-0, from 0.0.0.0/0 — every port on every node. Replacing it with four scoped rules changes which ports exist, not only who may reach them. So NodePort 32768-35535, swarm 2377 and docker 2376 now have no rule at all, at any CIDR. Defaulting admin_cidrs to 0.0.0.0/0 restores the breadth of the sources allowed to reach 22, 5986, 443 and 6443; it does not restore those other ports, and no tfvars value can.

This example declares --nodeport-range=32768-35535 in its MKE installFlags (aws-simple/launchpad.tf:234) while no longer admitting traffic to that range. A user whose workloads are reached by NodePort will find them unreachable after re-applying.

This is recorded as a known consequence of the scoping, not as a defect being fixed here. Widening the rules back is a decision for the user, and no fifth rule is added in this PR.

What this remediates, and what it does not

Everything here changes what a future terraform apply produces. It does not alter infrastructure that already exists:

• A running cluster keeps its 0.0.0.0/0 ingress until something re-applies these rules against it.
• sensitive = true changes what terraform prints from now on. It cannot retract output already written by past runs, wherever that output was captured, and it does not change a running instance's launch template.
• The Administrator password already written into the user data of running instances stays there regardless of what the examples emit for new stacks.

terraform validate on a scratch copy is structurally a creation-path proof: it says the configuration is valid to apply, not that any deployed stack has been remediated. Operators of existing clusters need a re-apply, and for the user-data item a replacement of the Windows instances.

AC-2 — specified, not implemented, pending a decision

Base note: main has moved to 51e98a6 ("Adopt provision-aws 0.3.0 in the terraform examples", #680) since this branch was cut. That commit deleted examples/terraform/aws-simple/userdata_windows.tpl and moved the Windows user-data rendering into the upstream module. It relocated this exposure rather than removing it: both examples still pass var.windows_password to modules/platform (aws-simple/platform.tf:34, aws-complete/platform.tf:16), and provision-aws v0.3.0 generates the Administrator-password-reset user data itself from that variable — see the comment #680 added at aws-simple/platform.tf:60-66. The password still reaches EC2 user data; only the code that writes it moved out of this repository. This branch does not touch that file, which is why it remains MERGEABLE across the deletion.

PRODENG-3929's ticket text cites aws-simple/userdata_windows.tpl:11-13 as the rendering site. That citation is stale on main as of 51e98a6. The ticket is not edited here.

The deciding fact is coverage asymmetry. aws-simple is the only example any smoke suite exercises — every test/smoke/*.go points at it — so implementing the password-data path there puts an unverifiable timing dependency on the single path that can break a smoke run. aws-complete has no smoke coverage at all, its key module is ed25519, which cannot decrypt EC2 password data, and it does not currently terraform validate (see Testing).

Two ways to close it, and only one is inside this repository's control:

1. Upstream stops writing it. provision-aws modules/platform would have to stop deriving user data from windows_password, or accept a mode that does not. Not actionable here; it is a different repository.
2. The example stops passing windows_password at all and sources the credential from the EC2 password-data API instead. This is the in-repo option.

Option 2 is implementable, verified from source rather than assumed:

• provision-aws exposes nodegroups[k].nodes[*].instance_id (nodes.tf:60-74).
• The AWS provider's data source aws_instance carries password_data and accepts get_password_data (confirmed from terraform providers schema -json).
• aws-simple/key.tf already generates RSA-4096 when ssh_key_algorithm = "rsa", which the Windows smoke tests set.

So the path is: stop passing windows_password into module.platform, let EC2Launch keep the random Administrator password it sets, read it with get_password_data, rsadecrypt it with tls_private_key.this.private_key_pem, and feed that into the generated YAML. Since 51e98a6 this is the whole of the change on this side — there is no longer a local template whose password-setting block could simply be deleted.

The unresolved risk: password_data is only available once the Windows agent posts it, typically minutes after boot, and a data source does not retry the way the resource's get_password_data wait does. Read too early it is empty and WinRM auth fails. In-repo mitigations exist — depends_on against the nodegroup, or time_sleep — but whether the timing holds on a real Windows 2019/2022/2025 boot is only measurable with a live apply, which is gated.

sensitive = true is applied in both examples because AC-2 requires it separately. It does not satisfy AC-2. The value still reaches aws_launch_template.user_data, which the provider schema does not mark sensitive. That is a schema and module-source reading, not an observation of any run: it establishes that nothing in that chain suppresses the value, so an apply's output can carry the base64 user data and a reader who has it can base64 -d it. Whether any specific stored log contains it has not been checked. IMDS separately serves instance user data to processes on the instance.

AC-3 — why aws-simple keeps a single role

The role split is applied to aws-complete only. In aws-simple, iam.tf creates common_role and common_profile and no policy anywhere references that role — grep -rn 'common_role\|iam_role_name' examples/terraform/aws-simple returns only iam.tf:1 (the role) and iam.tf:22 (the profile's reference to it); the only consumer is provision.tf:46, which assigns the instance profile. AC-3 asks for separate roles so the CCM policy attaches to managers only; with no policy attached there is nothing to misattach, and splitting an unused role would add surface without changing any permission.

• test/terraform.go, test/smoke/*.go, test/integration/integration_test.go: the suites pass admin_cidrs (test.AdminCIDRs, from SMOKE_ADMIN_CIDRS, falling back to 0.0.0.0/0 with a log line). The ExportGeneratedCredentials helper, the two credential env constants and their four call sites are removed, because the generated YAML no longer refers to the environment. TestGeneratedCredentialReferencesMatchExports is deleted rather than re-pinned, and coverage was not quietly dropped: it asserted that the .tf files emit $${NAME} references matching the Go exports, and with the emission reverted that contract no longer exists. What would be left is an assertion that a .tf file does not contain a $${} pattern — source text, guarding a mechanism this commit deletes, and it would misfire the first time anyone uses $${} legitimately. TestAdminCIDRs and the SMOKE_ADMIN_CIDRS override stay.

Where each change came from

This PR carries three things and only the first is in PRODENG-3929's text.

1. PRODENG-3929 — the ingress scoping (AC-1), the IAM role split and CCM policy scoping (AC-3), and sensitive = true (part of AC-2). The credential-inlining change carried in from PRODENG-3935's review was part of this PR and has been withdrawn; see "The inline-credential change is withdrawn" above.
2. Adopt provision-aws 0.3.0 in the terraform examples #680 Finding A — caller user_data discarded in aws-complete, found in the post-merge review of Adopt provision-aws 0.3.0 in the terraform examples #680 and authorised into this ticket. Landed at c3b75b1.
3. windows_password empty default — authorised by user ruling, off a measurement made while validating the empty-password fold in [PRODENG-3935] Warn on insecure WinRM with an inline password #682. Landed at e4df886.

On item 3, the literal ruling was remove the default, and this does not do that. Measured, an unconditional requirement fails at plan on stacks with no Windows node: test/smoke/smoke_test.go:144-158 passes the variable only when a platform starts with windows_, so the three Linux-only smoke tests pass nothing; swarm_addr_pool_test.go, upgrade_test.go, integration_test.go and test_manager.go reference it zero times; and both terraform.tfvars.template files ship it commented out. Five call paths and both documented starting points would break. The conditional validation delivers the ruled intent — failure at plan time, naming the variable, for exactly the configuration measured — without that. The divergence is deliberate.

The matching validator half lands in #682, which rejects a loaded config carrying an empty WinRM password. This PR stops that config being generated in the first place; #682 stops it being accepted. Neither is sufficient alone.

Cross-variable references in validation require Terraform 1.9 or later. CI pins no version — hashicorp/setup-terraform@v4 with no terraform_version — so it runs the current release. Recorded as a latent fragility, not addressed here.

Upgrade hazard for existing stacks

The IAM role split destroys common_role, common_profile and their policy attachments. The new profiles reach the launch templates only, and provision-aws does not refresh running instances on a launch-template change, so nodes provisioned before this change keep an instance profile whose role no longer exists and lose their CCM and EBS CSI permissions until they are replaced. aws-complete/iam.tf opens with that statement. Replace the nodes, or recreate the stack; do not re-apply in place against a live cluster. Enabling instance_refresh is deliberately not done here — it would roll every node when an upstream AMI is republished, since the examples select AMIs with most_recent = true — and is tracked as its own decision under PRODENG-4062.

Testing

• terraform fmt -check -recursive examples/terraform: clean.

• windows_password plan matrix against aws-simple, counting Invalid value for variable: Linux-only unset 0; Linux-only with windows_password = "" 0; Windows nodegroup unset 1; Windows with windows_password = "" 1; Windows with a password 0.

• go test -v --tags 'testing' ./test: RC 0 — TestTerraformOptionsKeepsCredentialsOutOfTheCommandLine, TestSecretVarsRoundTripThroughTerraform and TestAdminCIDRs all pass. go vet ./test/... RC 0, so test/smoke and test/integration still compile. make lint RC 0, 0 issues. The full ./test/... pattern is deliberately not run: test/smoke and test/integration have a TestMain that provisions real AWS stacks.

• An existing main-era terraform.tfvars — reconstructed verbatim from git show origin/main:…/terraform.tfvars.template, with only name uncommented and the MKE password filled — now plans. Before/after, same file, both examples:

  before: Error: No value for required variable
            on variables.tf line 71: variable "admin_cidrs" {   (aws-simple; line 65 in aws-complete)
  after:  Error: No valid credential sources found
            on system.tf line 1, in provider "aws"
  

  The after-plan reaches resource generation (+ create for tls_private_key.this, local_sensitive_file.ssh_private_key, time_static.now) and stops only at the AWS provider, which has no credentials here by design. Variable resolution and validation are past; that pair is the proof the breakage is gone.

• Credential emission verified by rendering, not only by reading: a throwaway module reproducing the heredoc with the same escaping emits adminPassword: Sup3rS3cret! when the values come from a tfvars with nothing exported, and adminPassword: FromEnvP@ss when they come from TF_VAR_launchpad with no -var arguments. Both routes produce a usable document.

• AC-4 is claimed at the plan level only: the suites supply admin_cidrs. A smoke run was not executed — it needs live AWS and is gated — so it is not shown that a provisioned cluster comes up under the narrowed rules.

• windows_password validation exercised by terraform plan against aws-simple (no AWS credentials, so the plan stops at the provider, after variable validation): a Linux-only config raises no variable error; a config whose nodegroup platform is windows_2022 and which passes no password fails with Invalid value for variable ... windows_password must be set to a non-empty value when any nodegroup uses a windows_ platform; the same config passing windows_password = "" fails identically; a Linux-only config that does pass a password raises no error.

• aws-simple: terraform init + terraform validate on a scratch copy — Success! The configuration is valid.

• aws-complete does not validate today, before this change. On a git archive of untouched main @ ce4a91d, terraform validate fails with:

  Error: Unsupported block type
    on k8s.tf line 12, in provider "helm":
    12:   kubernetes {
  Blocks of type "kubernetes" are not expected here.
  

  This is the helm provider v3 syntax change, pre-existing and out of scope here. With the helm provider pinned to ~> 2.17 in the scratch copy only, validation of this branch succeeds.

• No apply, no AWS. Nothing was provisioned and no credential profile was used.

• AC-4 is not demonstrated: it needs the held test-side change plus a smoke run. The argument available now is structural — which ports are reachable from which sources, and that the load-balanced ports retain a VPC-sourced rule. It does not cover a port MKE needs that nothing documents.

• aws-complete changes carry more risk than aws-simple ones for three independent reasons: no smoke suite exercises it, it does not currently validate at all, and its ed25519 key cannot decrypt EC2 password data. That is where review attention is worth spending.

• Aikido scanning is parked by user ruling and was not run; no other check here covers it.

Links

https://mirantis.jira.com/browse/PRODENG-3929 — findings secf_1d9be1096e3191dee288119a, secf_59e5c583a76665a8ae369d7f, secf_1d7359175d2e08a1777bb478.

This PR does not complete PRODENG-3929, to the same extent as before the 2026-11-06 rework — that rework changed the examples' input surface, not which acceptance criteria are met. AC-1 and AC-3 are implemented; AC-2 is partially implemented (sensitive = true) with the remainder specified above pending a decision; AC-4's test-side companion has landed, demonstrated at plan level rather than by a smoke run. Nothing in PRODENG-3929's acceptance criteria required admin_cidrs to be a required variable: the ticket asks for the ingress to be scoped, and it still is — by port in both examples, and by CIDR for any operator who narrows the default. What changed is the out-of-the-box posture, not the mechanism. The one item that genuinely goes out is the carried-in PRODENG-3935 finding, which is open again.

Checklist

• AC-1 ingress scoped by port in both examples; 5985 closed. admin_cidrs defaults to 0.0.0.0/0 by user ruling, documented as open
• AC-2 sensitive = true applied; password-data path specified, not implemented
• AC-3 manager/worker roles split in aws-complete; aws-simple excluded with evidence
• AC-4 suites supply admin_cidrs; not demonstrated by a smoke run
• Commit signed off

Written by AI: claude-opus-5

[Copilot]

🟡 Changes recommended

Unrestricted tagging allows ownership-check bypasses that expose unrelated AWS resources to modification or deletion, while an incorrect Classic ELB tag condition breaks CCM reconciliation.

2 open issues

• Unrestricted tag permissions allow cluster ownership-tag forgery · New
• Incorrect tag condition blocks Classic ELB reconciliation · New

What changed in this PR

This PR hardens Launchpad’s AWS Terraform examples by narrowing administrative ingress, separating node permissions, and replacing inline credentials in generated YAML. It remains a draft pending companion test changes after #686; Windows password removal from user data is deferred.

Changes:

• Require explicit admin_cidrs, restrict external ingress, and remove cleartext WinRM ingress.
• Separate manager and worker IAM roles in aws-complete and introduce tag-based CCM mutation restrictions.
• Mark Windows passwords sensitive and reference environment variables in generated YAML.

File	Description
examples/​terraform/​aws-simple/​variables.tf	Require administrative CIDRs and mark Windows passwords sensitive.
examples/​terraform/​aws-simple/​terraform.tfvars.template	Add an administrative CIDR example.
examples/​terraform/​aws-simple/​launchpad.tf	Restrict ingress and emit credential environment references.
examples/​terraform/​aws-complete/​variables.tf	Require administrative CIDRs and mark Windows passwords sensitive.
examples/​terraform/​aws-complete/​terraform.tfvars.template	Add an administrative CIDR example.
examples/​terraform/​aws-complete/​sg.tf	Restrict SSH and HTTPS WinRM; remove port 5985.
examples/​terraform/​aws-complete/​provision.tf	Select instance profiles by node role.
examples/​terraform/​aws-complete/​modules/​ebs-csi-driver/​variables.tf	Accept multiple IAM role names.
examples/​terraform/​aws-complete/​modules/​ebs-csi-driver/​iam.tf	Attach the CSI policy to each supplied role.
examples/​terraform/​aws-complete/​modules/​aws-ccm/​variables.tf	Separate control-plane and workload role inputs.
examples/​terraform/​aws-complete/​modules/​aws-ccm/​iam.tf	Split permissions and add ownership-tag conditions.
examples/​terraform/​aws-complete/​main.tf	Wire manager and worker roles into policy modules.
examples/​terraform/​aws-complete/​launchpad.tf	Restrict manager API ingress and emit credential environment references.
examples/​terraform/​aws-complete/​iam.tf	Create separate manager and worker roles and profiles.

_{Review effort: Balanced}

[Copilot]

🟡 Changes recommended

Unscoped ELB listener and policy permissions bypass cluster ownership boundaries, while reapplying existing stacks can remove running nodes’ AWS permissions without migrating their instance profiles.

4 open issues

• Restrict listener and policy changes to cluster-owned load balancers · New
• Preserve node permissions during instance-profile migration · New
• Allow tagging existing cluster-owned load balancers · New
• Require Windows passwords only when Windows nodegroups exist · New

2 resolved since last review

• Unrestricted tag permissions allow cluster ownership-tag forgery
• Incorrect tag condition blocks Classic ELB reconciliation

_{Review effort: Balanced}

[james-nesbitt]

most of this PR is good , but you have changed the examples input significantly in ways that will break existing external usecases and make it harder to run.

Things that are good:

1. firewall tuning
2. better ingress control (CIDR tuning)
3. better passowrd generation and handoff

But in your attempt to push for top level security the following are unacceptable:

1. defaulting to an unknown IP for admin access (is that IP a leak)? - yes defaulting to 0.0.0.0/0 produces a risk, but the risk is documented and easy to handle.
2. using only ENV variables hides an important input - ENV variables should augment to allow security improvements, but not make it harder to use. Doesn't Terraform already have a pattern for injecting inputs as ENV variables?