Ahmed Hassan·
Every Terraform module goes through this before it reaches our registry. Last week it caught an unencrypted log bucket and two IAM wildcards a human review had already approved.
Structured security and reusability review for a Terraform module before it is published to an internal registry. Ordered by blast radius, with corrected HCL for every finding.
Terraform Module Pre-Registry Review
Act as a principal cloud architect doing the final review before a Terraform module is published to our internal registry. Registry modules get reused blindly across teams, so defaults matter more than anything else.
Module source:
{{module_code}}
Our compliance baseline: {{compliance_baseline}}
Review in this exact order and stop at each layer before moving on:
**Layer 1 — Insecure defaults (blocking).** Anything a consumer gets by just calling the module with required inputs only: unencrypted storage, public network exposure, missing bucket/object ownership controls, IAM policies containing `*` in Action or Resource, security groups open to 0.0.0.0/0, logging disabled. These block publication.
**Layer 2 — Footguns (blocking).** `force_destroy = true` defaults, missing `prevent_destroy` on stateful resources, `create_before_destroy` absent where replacement causes downtime, provider version constraints missing or unpinned.
**Layer 3 — Interface quality (non-blocking).** Variables without descriptions or types, outputs leaking secrets, missing `sensitive = true`, tags not propagated via `default_tags` or a tags variable, names hardcoded instead of derived from a `name_prefix`.
For each finding output a table row: layer, resource address, one-line risk, corrected HCL snippet. After the table, write the exact `variables.tf` changes needed so secure behavior is the default and insecure behavior requires an explicit opt-in variable. End with PUBLISH / DO NOT PUBLISH and one sentence of justification.
Ergebnisse
## Review: `terraform-aws-app-logging` v0.3.0
**Verdict: DO NOT PUBLISH — 3 blocking findings in Layer 1–2.**
| Layer | Resource | Risk | Fix |
|---|---|---|---|
| 1 | `aws_s3_bucket.logs` | No server-side encryption configured; consumers get plaintext log storage by default | Add `aws_s3_bucket_server_side_encryption_configuration` with `aws:kms` and a `kms_key_arn` variable defaulting to the account log key |
| 1 | `aws_iam_policy.shipper` | `Action: "s3:*"` on `Resource: "*"` — the log shipper can read and delete every bucket in the account | Scope to `s3:PutObject`, `s3:GetBucketLocation` on `${aws_s3_bucket.logs.arn}/*` |
| 1 | `aws_iam_role.shipper` | Trust policy allows `ec2.amazonaws.com` account-wide with no condition | Add `aws:SourceArn` condition limited to the ASG passed in via variable |
| 2 | `aws_s3_bucket.logs` | `force_destroy = true` hardcoded — a `terraform destroy` deletes all retained logs, violating your 365-day retention baseline | Make it a variable, default `false` |
| 3 | `variable "bucket_name"` | No description, no validation; consumers collide on global namespace | Derive from `name_prefix` + account id, add validation block |
| 3 | `output "shipper_access_key"` | Static credential in outputs, not marked sensitive | Remove entirely — use the instance profile already created; if kept, `sensitive = true` |
### Required `variables.tf` changes
```hcl
variable "kms_key_arn" {
description = "KMS key for log encryption. Defaults to account log key."
type = string
default = null # module falls back to data.aws_kms_alias.log
}
variable "force_destroy" {
description = "Allow bucket deletion with objects present. Keep false in prod."
type = bool
default = false
}
```
Secure-by-default is achievable here with zero breaking changes for existing callers except the removed access-key output, which was unused in the two consuming repos I can see referenced in the README.
**Justification:** the module currently makes plaintext, deletable, account-writable logging the path of least resistance — exactly inverted from what a registry module should do.
Modell: Claude Opus 4.6
27 Likes16 SavesScore: 21
2 Kommentare
Daniel Cohen·
The layered ordering is the right call. Most review prompts mix a missing variable description in with an account-wide wildcard and the report reads flat.
Jonas Weber·
Adopted this for our module repo, added a Layer 0 for state backend config. The 'insecure requires explicit opt-in' framing convinced our platform team instantly.