Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to split GitHub Actions OIDC permissions for the hackforla/incubator Terraform workflows into separate Plan vs Apply IAM roles, reducing Plan privileges while keeping Apply capable of administering infrastructure.
Changes:
- Added distinct IAM roles for Terraform plan (
incubator-tf-plan) and apply (incubator-tf-apply) with different OIDC trust conditions and policy attachments. - Added a new custom IAM policy (
IncubatorTfPlanSecretsRead) and registered it in the custom policy module. - Introduced a new JSON policy document granting Terraform plan access to specific Secrets Manager secrets.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| terraform/aws-gha-oidc-providers.tf | Adds separate plan/apply OIDC-assumable roles and attaches policies. |
| terraform/aws-custom-policies.tf | Registers the new custom policy in the custom policies module. |
| terraform/aws-custom-policies/incubator-tf-plan-secrets-read-policy.json | Defines Secrets Manager read permissions for Terraform plan. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| resource "aws_iam_role_policy_attachment" "incubator_tf_plan_readonly" { | ||
| role = aws_iam_role.incubator_tf_plan.name | ||
| policy_arn = "arn:aws:iam::aws:policy/ReadOnlyAccess" | ||
| } |
There was a problem hiding this comment.
Attaching only the AWS managed ReadOnlyAccess policy to the Terraform plan role is likely insufficient for a remote S3 backend with DynamoDB state locking (this repo configures dynamodb_table in prod.backend.tfvars). Terraform plan/init typically needs write permissions to the lock table (e.g., dynamodb:PutItem, DeleteItem, UpdateItem) and appropriate S3 backend access; otherwise CI plans will fail when locking the state. Consider adding a minimal backend-access policy (S3 state bucket + DynamoDB lock table) to this role, instead of (or in addition to) ReadOnlyAccess.
| "IncubatorTfPlanSecretsRead" = { | ||
| description = "Allows incubator tf plan role to read specific Secrets Manager secrets needed for terraform plan" | ||
| filename = "incubator-tf-plan-secrets-read-policy.json" | ||
| } |
There was a problem hiding this comment.
PR description mentions a new policy file incubator-tf-plan-secrets-read-policy.tf, but the change here references incubator-tf-plan-secrets-read-policy.json. If the PR description is outdated/typo, consider updating it to match the actual file name/type to avoid confusion for reviewers and future maintainers.
|
Terraform plan in terraform Plan: 2 to add, 0 to change, 0 to destroy.Terraform used the selected providers to generate the following execution
plan. Resource actions are indicated with the following symbols:
+ create
Terraform will perform the following actions:
# aws_iam_role_policy_attachment.incubator_tf_plan_secrets_read will be created
+ resource "aws_iam_role_policy_attachment" "incubator_tf_plan_secrets_read" {
+ id = (known after apply)
+ policy_arn = (known after apply)
+ role = "incubator-tf-plan"
}
# module.aws_custom_policies.aws_iam_policy.custom_policy["IncubatorTfPlanSecretsRead"] will be created
+ resource "aws_iam_policy" "custom_policy" {
+ arn = (known after apply)
+ attachment_count = (known after apply)
+ description = "Allows incubator tf plan role to read specific Secrets Manager secrets needed for terraform plan"
+ id = (known after apply)
+ name = "IncubatorTfPlanSecretsRead"
+ name_prefix = (known after apply)
+ path = "/"
+ policy = jsonencode(
{
+ Statement = [
+ {
+ Action = [
+ "secretsmanager:GetSecretValue",
]
+ Effect = "Allow"
+ Resource = [
+ "arn:aws:secretsmanager:us-west-2:035866691871:secret:home-unite-us-cognito-client*",
+ "arn:aws:secretsmanager:us-west-2:035866691871:secret:home-unite-us-google-clientid*",
+ "arn:aws:secretsmanager:us-west-2:035866691871:secret:home-unite-us-google-secret*",
]
+ Sid = "AllowReadSpecificSecretsForTerraformPlan"
},
]
+ Version = "2012-10-17"
}
)
+ policy_id = (known after apply)
+ tags_all = (known after apply)
}
Plan: 2 to add, 0 to change, 0 to destroy.
Warning: Argument is deprecated
with module.iam_oidc_gha_incubator.aws_iam_role.github_actions_oidc,
on modules/aws-gha-oidc-providers/main.tf line 54, in resource "aws_iam_role" "github_actions_oidc":
54: managed_policy_arns = var.policy_arns
managed_policy_arns is deprecated. Use the aws_iam_role_policy_attachment
resource instead. If Terraform should exclusively manage all managed policy
attachments (the current behavior of this argument), use the
aws_iam_role_policy_attachments_exclusive resource as well.📝 Plan generated in Write Terraform Plan to Pull Request #99 |
| "secretsmanager:GetSecretValue" | ||
| ], | ||
| "Resource": [ | ||
| "arn:aws:secretsmanager:us-west-2:035866691871:secret:home-unite-us-cognito-client*", |
There was a problem hiding this comment.
should be:
arn:aws:secretsmanager:us-west-2:035866691871:secret:*
There was a problem hiding this comment.
the name of the file should not be specific to secrets, but all of the 'other' things that TF plan needs other than read only
Addresses Issue 146
What changes did you make?
aws-gha-oidc-providers.tfIncubatorTfPlanSecretsRead inaws-custom-policies.tf`incubator-tf-plan-secrets-read-policy.tfWhy did you make the changes (we will use this info to test)?