From a70e576ff52e12ec37f7abb5cded75f185dda393 Mon Sep 17 00:00:00 2001 From: Alex English Date: Wed, 16 Sep 2026 18:31:02 -0700 Subject: [PATCH] Add iam_only variable and remove import blocks --- .github/workflows/terraform-apply.yaml | 1 + .github/workflows/terraform-plan.yaml | 1 + CONTRIBUTING.md | 9 +++ terraform/aws-custom-policies-legacy.tf | 8 +-- terraform/aws-groups-legacy.tf | 5 -- terraform/aws-groups.tf | 4 +- terraform/aws-users-legacy.tf | 10 ---- terraform/cloudtrail.tf | 63 +++++++++++++++++--- terraform/imports.tf | 76 ------------------------- terraform/variables.tf | 12 ++++ 10 files changed, 81 insertions(+), 108 deletions(-) delete mode 100644 terraform/imports.tf create mode 100644 terraform/variables.tf diff --git a/.github/workflows/terraform-apply.yaml b/.github/workflows/terraform-apply.yaml index 2857f8b..9b529c4 100644 --- a/.github/workflows/terraform-apply.yaml +++ b/.github/workflows/terraform-apply.yaml @@ -38,4 +38,5 @@ jobs: with: path: terraform backend_config_file: terraform/prod.backend.tfvars + variables: iam_only = false auto_approve: ${{ github.event_name == 'workflow_dispatch' }} diff --git a/.github/workflows/terraform-plan.yaml b/.github/workflows/terraform-plan.yaml index 6217ed2..1555ced 100644 --- a/.github/workflows/terraform-plan.yaml +++ b/.github/workflows/terraform-plan.yaml @@ -34,3 +34,4 @@ jobs: with: path: terraform backend_config_file: terraform/prod.backend.tfvars + variables: iam_only = false diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f38d7d0..e9796b1 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -260,6 +260,15 @@ Remember to match these values to the ones in your backend state (and replace US ### **Terraform Setup and Execution Instructions** Make the required changes and execute them to see the changes in your own AWS account +**Note:** By default this configuration manages IAM resources only (`iam_only = true`). The CloudTrail trails and log buckets in `cloudtrail.tf` belong to the Hack for LA AWS account and are skipped, so you don't need to set anything for a test run in your own account. Everything else is still created in your account, including: + * every IAM user this repo declares + * the `ops-leads` group, with `AdministratorAccess` attached + * a GitHub Actions OIDC provider. AWS allows only one per account, so `terraform apply` fails with `EntityAlreadyExists` if your account already has one. + +Run `terraform destroy` when you are finished to remove them. + +**Note:** Any plan or apply against the Hack for LA AWS account must pass `-var iam_only=false`. The GitHub Actions workflows already do this. + - Change into `terraform` directory with ```bash diff --git a/terraform/aws-custom-policies-legacy.tf b/terraform/aws-custom-policies-legacy.tf index 8188f37..96c554f 100644 --- a/terraform/aws-custom-policies-legacy.tf +++ b/terraform/aws-custom-policies-legacy.tf @@ -9,7 +9,8 @@ // // ManageAccessKeys is attached to the ops-leads group by // aws_iam_group_policy_attachment.manageAccessKeys in aws-groups.tf, which -// still refers to it by literal ARN. That attachment is unchanged here. +// refers to this resource's arn rather than a literal ARN, so it also resolves +// in a contributor's own AWS account. resource "aws_iam_policy" "manage_access_keys" { name = "ManageAccessKeys" @@ -19,8 +20,3 @@ resource "aws_iam_policy" "manage_access_keys" { description = "Policy for creating, listing, and updating Access Keys" policy = file("${path.module}/aws-custom-policies/existing-policies/manage-access-keys-policy.json") } - -import { - to = aws_iam_policy.manage_access_keys - id = "arn:aws:iam::035866691871:policy/ManageAccessKeys" -} diff --git a/terraform/aws-groups-legacy.tf b/terraform/aws-groups-legacy.tf index e2c54a3..3a08407 100644 --- a/terraform/aws-groups-legacy.tf +++ b/terraform/aws-groups-legacy.tf @@ -13,8 +13,3 @@ resource "aws_iam_group" "project_leads" { name = "project-leads" } - -import { - to = aws_iam_group.project_leads - id = "project-leads" -} diff --git a/terraform/aws-groups.tf b/terraform/aws-groups.tf index 646f677..439b864 100644 --- a/terraform/aws-groups.tf +++ b/terraform/aws-groups.tf @@ -10,7 +10,7 @@ module "iam_read_only_group" { } } -//import ops-leads group +// ops-leads group resource "aws_iam_group" "ops_leads_group" { name = "ops-leads" } @@ -22,7 +22,7 @@ resource "aws_iam_group_policy_attachment" "admin"{ resource "aws_iam_group_policy_attachment" "manageAccessKeys"{ group = aws_iam_group.ops_leads_group.name - policy_arn = "arn:aws:iam::035866691871:policy/ManageAccessKeys" + policy_arn = aws_iam_policy.manage_access_keys.arn } diff --git a/terraform/aws-users-legacy.tf b/terraform/aws-users-legacy.tf index 9c44499..c08a6fc 100644 --- a/terraform/aws-users-legacy.tf +++ b/terraform/aws-users-legacy.tf @@ -21,13 +21,3 @@ resource "aws_iam_user" "fangyiliu" { resource "aws_iam_user" "jack_pashayan" { name = "jack.pashayan" } - -import { - to = aws_iam_user.fangyiliu - id = "fangyiliu" -} - -import { - to = aws_iam_user.jack_pashayan - id = "jack.pashayan" -} diff --git a/terraform/cloudtrail.tf b/terraform/cloudtrail.tf index 233f647..3d73aec 100644 --- a/terraform/cloudtrail.tf +++ b/terraform/cloudtrail.tf @@ -17,6 +17,13 @@ // indefinitely), and the disagreement over log file validation, which is reproduced below // rather than resolved. // +// Everything here is skipped unless var.iam_only is false (see variables.tf). The bucket +// names, bucket policies and the backend bucket selector all belong to the Hack for LA +// account, so none of this can be created in a contributor's own AWS account. The CI +// workflows set iam_only = false. The trails and buckets carry prevent_destroy so that a +// run against the Hack for LA account which forgets to do the same fails at plan time +// instead of planning to destroy the account's audit logging. +// // A bucket is not one resource. Since AWS provider v4 the policy, public access block, // encryption and ownership controls are each their own resource with their own import -- // the same trap terraform/import.tf in hackforla/incubator documents. @@ -26,7 +33,13 @@ // --------------------------------------------------------------------------- resource "aws_s3_bucket" "tf_backend_logs" { + count = var.iam_only ? 0 : 1 + bucket = "aws-cloudtrail-logs-035866691871-4b8654bf" + + lifecycle { + prevent_destroy = true + } } // Reproduced verbatim from the live bucket. These two statements are what let CloudTrail @@ -38,7 +51,9 @@ resource "aws_s3_bucket" "tf_backend_logs" { // equivalent to AWS, and both are left exactly as stored. Normalising them would be a // change to a live policy for no behavioural gain. resource "aws_s3_bucket_policy" "tf_backend_logs" { - bucket = aws_s3_bucket.tf_backend_logs.id + count = var.iam_only ? 0 : 1 + + bucket = aws_s3_bucket.tf_backend_logs[0].id policy = jsonencode({ Version = "2012-10-17" @@ -73,7 +88,9 @@ resource "aws_s3_bucket_policy" "tf_backend_logs" { } resource "aws_s3_bucket_public_access_block" "tf_backend_logs" { - bucket = aws_s3_bucket.tf_backend_logs.id + count = var.iam_only ? 0 : 1 + + bucket = aws_s3_bucket.tf_backend_logs[0].id block_public_acls = true block_public_policy = true @@ -84,7 +101,9 @@ resource "aws_s3_bucket_public_access_block" "tf_backend_logs" { // blocked_encryption_types is deliberately not set. The provider reads it back as // ["SSE-C"] on both buckets but it is computed, so leaving it out produces no diff. resource "aws_s3_bucket_server_side_encryption_configuration" "tf_backend_logs" { - bucket = aws_s3_bucket.tf_backend_logs.id + count = var.iam_only ? 0 : 1 + + bucket = aws_s3_bucket.tf_backend_logs[0].id rule { apply_server_side_encryption_by_default { @@ -98,7 +117,9 @@ resource "aws_s3_bucket_server_side_encryption_configuration" "tf_backend_logs" // equivalent resource is declared for it below -- declaring one would create it rather // than import it, which is a live change. resource "aws_s3_bucket_ownership_controls" "tf_backend_logs" { - bucket = aws_s3_bucket.tf_backend_logs.id + count = var.iam_only ? 0 : 1 + + bucket = aws_s3_bucket.tf_backend_logs[0].id rule { object_ownership = "BucketOwnerEnforced" @@ -109,8 +130,10 @@ resource "aws_s3_bucket_ownership_controls" "tf_backend_logs" { // the provider's default_tags would plan to REMOVE it, which is why this is the one // resource in this file carrying a tags block. resource "aws_cloudtrail" "tf_backend_logs" { + count = var.iam_only ? 0 : 1 + name = "devops-security-tf-backend-logs" - s3_bucket_name = aws_s3_bucket.tf_backend_logs.id + s3_bucket_name = aws_s3_bucket.tf_backend_logs[0].id include_global_service_events = true is_multi_region_trail = true enable_log_file_validation = true @@ -133,6 +156,10 @@ resource "aws_cloudtrail" "tf_backend_logs" { tags = { project = "devops-security" } + + lifecycle { + prevent_destroy = true + } } // --------------------------------------------------------------------------- @@ -140,13 +167,21 @@ resource "aws_cloudtrail" "tf_backend_logs" { // --------------------------------------------------------------------------- resource "aws_s3_bucket" "management_events" { + count = var.iam_only ? 0 : 1 + bucket = "aws-cloudtrail-logs-035866691871-6539ef03" + + lifecycle { + prevent_destroy = true + } } // Verbatim from the live bucket, including the "AWS:SourceArn" spelling -- see the note on // the policy above. resource "aws_s3_bucket_policy" "management_events" { - bucket = aws_s3_bucket.management_events.id + count = var.iam_only ? 0 : 1 + + bucket = aws_s3_bucket.management_events[0].id policy = jsonencode({ Version = "2012-10-17" @@ -181,7 +216,9 @@ resource "aws_s3_bucket_policy" "management_events" { } resource "aws_s3_bucket_public_access_block" "management_events" { - bucket = aws_s3_bucket.management_events.id + count = var.iam_only ? 0 : 1 + + bucket = aws_s3_bucket.management_events[0].id block_public_acls = true block_public_policy = true @@ -190,7 +227,9 @@ resource "aws_s3_bucket_public_access_block" "management_events" { } resource "aws_s3_bucket_server_side_encryption_configuration" "management_events" { - bucket = aws_s3_bucket.management_events.id + count = var.iam_only ? 0 : 1 + + bucket = aws_s3_bucket.management_events[0].id rule { apply_server_side_encryption_by_default { @@ -203,8 +242,10 @@ resource "aws_s3_bucket_server_side_encryption_configuration" "management_events // Log file validation is off on this trail and on for the other. That inconsistency is // reproduced rather than resolved -- turning it on is a live change and its own ticket. resource "aws_cloudtrail" "management_events" { + count = var.iam_only ? 0 : 1 + name = "management-events" - s3_bucket_name = aws_s3_bucket.management_events.id + s3_bucket_name = aws_s3_bucket.management_events[0].id include_global_service_events = true is_multi_region_trail = true enable_log_file_validation = false @@ -217,4 +258,8 @@ resource "aws_cloudtrail" "management_events" { equals = ["Management"] } } + + lifecycle { + prevent_destroy = true + } } diff --git a/terraform/imports.tf b/terraform/imports.tf deleted file mode 100644 index 14b30ef..0000000 --- a/terraform/imports.tf +++ /dev/null @@ -1,76 +0,0 @@ -import { - to = aws_iam_group.ops_leads_group - id = "ops-leads" -} - -import { - to = aws_iam_group_policy_attachment.admin - id = "ops-leads/arn:aws:iam::aws:policy/AdministratorAccess" -} -import { - to = aws_iam_group_policy_attachment.manageAccessKeys - id = "ops-leads/arn:aws:iam::035866691871:policy/ManageAccessKeys" -} - -// Adopts the two CloudTrail trails and their two log buckets. See -// hackforla/devops-security#191 and the commentary in cloudtrail.tf. -// -// A trail imports by its full ARN, NOT by its name. The name fails against the pinned -// provider with `could not parse import ID "management-events" as ARN: arn: invalid -// prefix`. Every S3 resource below imports by bucket name, including the policy, public -// access block, encryption and ownership controls, each of which is its own resource. - -import { - to = aws_cloudtrail.tf_backend_logs - id = "arn:aws:cloudtrail:us-west-2:035866691871:trail/devops-security-tf-backend-logs" -} - -import { - to = aws_s3_bucket.tf_backend_logs - id = "aws-cloudtrail-logs-035866691871-4b8654bf" -} - -import { - to = aws_s3_bucket_policy.tf_backend_logs - id = "aws-cloudtrail-logs-035866691871-4b8654bf" -} - -import { - to = aws_s3_bucket_public_access_block.tf_backend_logs - id = "aws-cloudtrail-logs-035866691871-4b8654bf" -} - -import { - to = aws_s3_bucket_server_side_encryption_configuration.tf_backend_logs - id = "aws-cloudtrail-logs-035866691871-4b8654bf" -} - -import { - to = aws_s3_bucket_ownership_controls.tf_backend_logs - id = "aws-cloudtrail-logs-035866691871-4b8654bf" -} - -import { - to = aws_cloudtrail.management_events - id = "arn:aws:cloudtrail:us-west-2:035866691871:trail/management-events" -} - -import { - to = aws_s3_bucket.management_events - id = "aws-cloudtrail-logs-035866691871-6539ef03" -} - -import { - to = aws_s3_bucket_policy.management_events - id = "aws-cloudtrail-logs-035866691871-6539ef03" -} - -import { - to = aws_s3_bucket_public_access_block.management_events - id = "aws-cloudtrail-logs-035866691871-6539ef03" -} - -import { - to = aws_s3_bucket_server_side_encryption_configuration.management_events - id = "aws-cloudtrail-logs-035866691871-6539ef03" -} diff --git a/terraform/variables.tf b/terraform/variables.tf new file mode 100644 index 0000000..ea5018e --- /dev/null +++ b/terraform/variables.tf @@ -0,0 +1,12 @@ +// Defaults to true so that a contributor running this against their own AWS account, +// which is how changes are tested (see CONTRIBUTING.md), gets IAM resources only. The +// resources in cloudtrail.tf are specific to the Hack for LA account and cannot be +// created anywhere else. +// +// Any plan or apply against the Hack for LA account MUST set this to false. Both CI +// workflows do. See hackforla/devops-security#196. +variable "iam_only" { + description = "Manage IAM resources only, skipping the Hack for LA account's CloudTrail trails and log buckets. Set to false for the Hack for LA account." + type = bool + default = true +}