hackforla / hackforla/devops-security

Move the Terraform workflows off static AWS credentials onto OIDC

Đang mở
#182 0 bình luận 0 reaction 1 người được giao Được @ale210 nhận Xem trên GitHub
complexity: medium feature: security role: DevOps Engineer size: 5pt
Ngôn ngữ chính
HCL
Star
1
Fork
14
Merge trung bình
1 giờ 3 phút
Pull request đã merge (30 ngày)
21

Mô tả

### Overview

We need `terraform-plan.yaml` and `terraform-apply.yaml` to authenticate to AWS by assuming a role through GitHub's OIDC provider instead of passing a static access key and secret, because `devops-security` is the repo that builds the org's GitHub Actions OIDC and is the last repo still using long-lived credentials to reach AWS.

### Action Items

- [x] Read the current position before changing anything. Both workflows pass `aws-access-key-id: ${{ secrets.AWS_ACCESS_KEY_ID }}` and `aws-secret-access-key: ${{ secrets.AWS_SECRET_ACCESS_KEY }}` to `aws-actions/configure-aws-credentials@v4`, and neither declares `permissions: id-token: write` at the job or workflow level. Line numbers drift — find the step by its name, `Configure AWS Credentials`.
- [x] Confirm what the secrets actually are. They are the access key of IAM user **`devops-iam-github-action`** (key `AKIAQQWOSJEPUH74UTOJ`, created 2024-02-19, never rotated, still in active use). Verify that nothing outside these two workflows uses that user before planning to delete it — `aws iam get-access-key-last-used` and CloudTrail on the username are the two places to look.
- [x] **Do not create the roles with `module "aws-gha-oidc-providers"`.** That module creates an `aws_iam_openid_connect_provider`, and AWS permits exactly one provider per URL per account. `arn:aws:iam::035866691871:oidc-provider/token.actions.githubusercontent.com` already exists — the incubator instantiation created it — so a second instantiation fails with `EntityAlreadyExists`. The module is the right *shape* to copy and the wrong thing to call.
- [x] Design two roles, not one. The plan/apply privilege split is the part of the pattern that static keys have no equivalent for, and it is a security win here alongside retiring the keys.
- `devops-security-tf-plan` — read-only, `sub` scoped to `repo:hackforla/devops-security:ref:refs/heads/*` and `repo:hackforla/devops-security:pull_request`.
- `devops-security-tf-apply` — `arn:aws:iam::aws:policy/AdministratorAccess`, `sub` scoped to `repo:hackforla/devops-security:ref:refs/heads/main` **only**. The branch scoping is what keeps this safe: a branch or pull request cannot assume this role at all.
- Both trust `arn:aws:iam::035866691871:oidc-provider/token.actions.githubusercontent.com` with `token.actions.githubusercontent.com:aud = sts.amazonaws.com`.
- `AdministratorAccess` on the apply role is a decision, not a derivation, and is worth recording as one: the IAM user CI runs as today has a narrower set — managed `TerraformIAM`, `TerraformDynamoDBAccess`, `DevopsSecurityTerraformBucketAccess`, `AmazonS3ReadOnlyAccess`, plus inline `GitHubActionsOIDCThumbprintUpdatePolicy` — so this widens the apply path rather than matching it one-for-one. It matches `incubator-tf-apply`, which is the same shape one repo over. When hackforla/incubator#138 reports back on narrowing incubator's apply privileges, its conclusions apply here too.
- [x] **Work out what the plan role needs beyond `ReadOnlyAccess` before creating it, because a plan is not a read-only operation against this backend.** `terraform/prod.backend.tfvars` sets `dynamodb_table = "hfla_ops_terraform_table"`, so every plan acquires and releases a state lock, which needs `dynamodb:PutItem` and `dynamodb:DeleteItem` — neither of which `ReadOnlyAccess` grants. The existing `TerraformDynamoDBAccess` policy grants exactly this set and can be attached to the plan role. Note `incubator-tf-plan` carries only `ReadOnlyAccess` plus a secrets-read policy and appears to lack lock-write permission entirely, so **check how incubator's plan actually behaves rather than copying it** — either it runs with locking disabled or something else is going on, and the answer decides whether this role needs the DynamoDB policy or the workflow needs `-lock=false`.
- [x] Settle the two tagging questions before creating the roles, and record the answers on this issue. A tag is hard to change once an audit depends on it.
- The exact tag key and value marking these roles as deliberately outside Terraform. No `immune` or `exempt` convention exists in any of the three repos today. The only tagging precedent is `user_tags` on IAM users in `terraform/aws-users.tf`, which uses quoted title-case keys (`"Project"`, `"Access Level"`) — match that shape or diverge on purpose.
- Which audit the tag exempts the roles from, **by name**. Decision record "Use it or lose it policy for IAM User Accounts" is about IAM *users* and these are *roles*, so the tag may be pre-empting an audit that does not exist. If it cannot be named, the tag documents nothing.
- [x] Create the two roles by hand in account `035866691871`, and **capture each role's trust policy, attached policies and tags before and after the change**. This is an AWS write with no PR trail, so it follows the same discipline as the org-account work: one change at a time, verified after each. These roles are created manually on purpose — it dissolves the bootstrap problem where the Terraform that creates the role is run by the workflow that needs it — which is what makes the switch below a single PR.
- [x] Write down, in `terraform/aws-gha-oidc-providers.tf` itself, that these two roles exist outside Terraform and why. A reader of that file will otherwise see incubator's roles declared and devops-security's absent and "fix" it, which re-creates the bootstrap problem this decision removed. The note belongs where they will be standing, not only in this issue.
- [x] Switch both workflows in one PR. Replace the two `secrets.AWS_*` inputs with `role-to-assume` / `role-session-name` / `aws-region: us-west-2`, and add `permissions: id-token: write` alongside the existing `contents: read`. `hackforla/incubator`'s `.github/workflows/terraform-plan.yaml` is the working example to copy.
- [x] **Verification does not happen by itself here, and this is the trap.** Both workflows filter on `paths: ['**/*.tf']`, so a PR that changes only workflow files triggers neither one — the switch would sit unexercised until someone else's Terraform change, possibly weeks later. Include a no-op `.tf` change in the same PR (a comment line is enough) so the plan runs on the PR and the apply runs on merge.
- [x] **After the PR merges**, confirm the apply run assumed `devops-security-tf-apply` and succeeded, and that the plan comment appeared on the PR before merge. Both are observable only after the fact, which is why they are listed here rather than as branch checks.
- [x] **After both runs are confirmed green**, retire the credentials: delete access key `AKIAQQWOSJEPUH74UTOJ`, delete the repository secrets `AWS_ACCESS_KEY_ID` and `AWS_SECRET_ACCESS_KEY`, and decide whether the IAM user `devops-iam-github-action` itself should go. Note the user is **not** currently declared in Terraform — verified after hackforla/devops-security#180 merged — so deleting it also removes an unmanaged resource; if it is kept instead, it needs to be brought into `terraform/aws-users.tf`. Do not do any of this before the runs are green — it is the rollback path.

### Resources/Instructions

- `.github/workflows/terraform-plan.yaml` and `.github/workflows/terraform-apply.yaml` — the two files that change. The credential step is identical in both.
- `terraform/aws-gha-oidc-providers.tf` — where the incubator roles are declared, and where the note about these roles living outside Terraform belongs.
- `terraform/modules/aws-gha-oidc-providers/` — the module to read for the trust-policy shape. Do not instantiate it; see the action item above.
- `terraform/prod.backend.tfvars` — the S3 backend and the DynamoDB lock table the plan role has to be able to write to.
- `hackforla/incubator` `.github/workflows/terraform-plan.yaml` and `terraform-apply.yaml` — the working OIDC example, assuming `arn:aws:iam::035866691871:role/incubator-tf-plan`.
- hackforla/devops-security#170 — bumps `aws-actions/configure-aws-credentials` v4→v6 and `dflook/terraform-*` v1→v3 in these same two files, and rewrites the very step this issue rewrites. **Whichever starts first, check the other**: if this issue lands first, #170 should close itself as covered for the `configure-aws-credentials` half.
- hackforla/devops-security#187 — stops the merge-triggered apply auto-approving itself, and adds a `workflow_dispatch` trigger to `terraform-apply.yaml`. It is **blocked on this issue**: that dispatch path is a deliberately unreviewed apply, and it is only safe once `devops-security-tf-apply` scopes `sub` to `refs/heads/main`, because the static keys it replaces carry no branch scoping at all. #187 also hits the same `paths: ['**/*.tf']` verification trap described above. Nothing in this issue needs to change for it — but if the apply role here ends up *not* branch-scoped, #187 needs revisiting before it is worked.
- hackforla/incubator#139 (Terraform plan via Identity Center) and hackforla/incubator#138 (restrict incubator apply privileges) — separate work with the same goal of retiring long-lived AWS credentials. #138 is researching the plan/apply privilege split this issue applies.
- AWS account `035866691871`, region `us-west-2`.

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Đánh giá

Issue này chưa được đánh giá.

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.