Repository navigation
fix(iac/aws): grant deploy role KMS key-lifecycle for OIDC signing-key replace - #1496
Merged
Merged
Conversation
…y replace The cudly-terraform-deploy GitHub Actions role holds KMS data-plane and alias actions but no key-lifecycle actions, so the OIDC issuer signing key was bootstrap-created. PR #1480 migrated aws_kms_key.signing from RSA_2048 to ECC_NIST_P256 (ES256); KMS cannot change a key spec in place, so Terraform must replace the key. The deploy then failed on: AccessDenied: not authorized to perform kms:ScheduleKeyDeletion on arn:aws:kms:us-east-1:...:key/a8e28eac-... Add a tag-gated key-lifecycle grant so the replace can complete: - ScheduleKeyDeletion + management reads/mutates gated to Project=CUDly keys (deploy SA can never schedule deletion of an unrelated account CMK). - TagResource/UntagResource gated on aws:RequestTag/Project=CUDly (a new key is untagged at create, so the request tag is used). - CreateKey on "*" (cannot be resource/tag-scoped; benign - creating a CMK grants no data access; deletion stays tag-gated). Requires the ci-cd-permissions bootstrap apply to take effect before the runtime deploy can complete.
cristim
added
triaged
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 19 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe compute_b managed policy adds three KMS permission statements for tagged key lifecycle operations, request-tag-gated tagging, and KMS key creation. ChangesKMS permissions
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@terraform/environments/aws/ci-cd-permissions/policy_compute_b.tf`:
- Around line 116-134: Update the KMSTagResourceRequestGated statement to
restrict tagging to CMKs owned by the deployment scope, rather than allowing
Resource = "*" based only on aws:RequestTag/Project. Add an appropriate
resource-side condition or explicit deny that prevents adopting foreign keys
while preserving tagging of newly created, untagged CMKs; do not rely on
aws:TagKeys alone.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 089afc7a-f2b9-4f78-8ae7-7a6d8eb70a12
📒 Files selected for processing (1)
terraform/environments/aws/ci-cd-permissions/policy_compute_b.tf
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
…rants The prior fix added kms:ScheduleKeyDeletion and other lifecycle actions gated on aws:ResourceTag/Project=CUDly in policy_compute_b.tf, but those actions were already granted under an identical condition by KMSMutateTaggedOnly in policy_compute.tf (IAM unions statements, so the new grants were no-ops). The real blocker is a tag-value case mismatch: the signing key's Project tag comes from var.project_name, which defaults to lowercase "cudly", overriding the provider's default_tags value of "CUDly" for that resource. StringEquals is case-sensitive, so the existing tag-gated grant never matched the signing key. Switch KMSMutateTaggedOnly's condition to StringEqualsIgnoreCase so it matches regardless of tag casing, add the one genuinely new action (kms:CancelKeyDeletion) to that statement, and remove the duplicate KMSKeyLifecycleTaggedOnly/KMSTagResourceRequestGated/KMSCreateKey statements from policy_compute_b.tf (their actions were already granted elsewhere; the RequestTag-gated UntagResource was additionally a dead grant since UntagResource evaluates aws:TagKeys, not aws:RequestTag).
cristim
force-pushed
the
fix/cicd-kms-key-lifecycle
branch
from
July 23, 2026 14:08
302c34a to
c96e856
Compare
Member
Author
|
Adversarial review found this PR's original fix does not address the reported failure. Filing the finding and the corrected fix here. Why the original fix is a no-op The new statements added in
IAM unions statements across policies attached to the same role, so duplicating an existing grant under the same condition changes nothing. It cannot fix the reported Real cause: tag-value case mismatch The signing key's
Every KMS condition in this policy uses Fix applied
Net effect: same set of actions as before this PR, minus the case-sensitivity bug, on a single statement instead of three redundant ones. Recommend the maintainer verify the actual tag casing post-apply with: to confirm the live tag is @coderabbitai review |
Contributor
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
for file in \
terraform/environments/aws/ci-cd-permissions/policy_compute.tf \
terraform/environments/aws/ci-cd-permissions/policy_compute_b.tf \
terraform/environments/aws/main.tf \
terraform/environments/aws/variables.tf \
modules/compute/aws/lambda/signing-key.tf
do
printf '\n===== OUTLINE: %s =====\n' "$file"
ast-grep outline "$file" --items all || true
done
printf '\n===== RELEVANT POLICY/TAG DECLARATIONS =====\n'
rg -n -C 12 \
-e 'KMSMutateTaggedOnly' \
-e 'KMSCreateAndRead' \
-e 'KMSKeyLifecycleTaggedOnly' \
-e 'KMSTagResourceRequestGated' \
-e 'KMSCreateKey' \
-e 'common_tags' \
-e 'project_name' \
-e 'aws_kms_key' \
-e 'tags\s*=' \
terraform/environments/aws/ci-cd-permissions/policy_compute.tf \
terraform/environments/aws/ci-cd-permissions/policy_compute_b.tf \
terraform/environments/aws/main.tf \
terraform/environments/aws/variables.tf \
modules/compute/aws/lambda/signing-key.tf
printf '\n===== CURRENT PR DIFF (relevant files) =====\n'
git diff "$(git merge-base HEAD origin/main)"...HEAD -- \
terraform/environments/aws/ci-cd-permissions/policy_compute.tf \
terraform/environments/aws/ci-cd-permissions/policy_compute_b.tf \
terraform/environments/aws/main.tf \
terraform/environments/aws/variables.tf \
modules/compute/aws/lambda/signing-key.tf || trueLength of output: 24118 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '\n===== SIGNING-KEY RESOURCE / MODULE LOCATIONS =====\n'
fd -HI -t f -e tf . | while IFS= read -r file; do
if rg -n -q -e 'resource\s+"aws_kms_key"' -e 'signing[_-]?key' -e 'kms_key' "$file"; then
printf '%s\n' "$file"
rg -n -C 10 \
-e 'resource\s+"aws_kms_key"' \
-e 'signing[_-]?key' \
-e 'kms_key' \
-e 'tags\s*=' \
-e 'policy\s*=' \
"$file" || true
fi
done
printf '\n===== MODULE TAG WIRING =====\n'
rg -n -C 8 \
-e 'module\s+"compute' \
-e 'tags\s*=\s*local\.common_tags' \
-e 'tags\s*=\s*var\.tags' \
terraform/environments/aws modules || true
printf '\n===== COMPLETE CURRENT KMS POLICY BLOCKS =====\n'
sed -n '285,375p' terraform/environments/aws/ci-cd-permissions/policy_compute.tf
sed -n '60,95p' terraform/environments/aws/ci-cd-permissions/policy_compute_b.tf
printf '\n===== PR DIFF =====\n'
base="$(git merge-base HEAD origin/main 2>/dev/null || git merge-base HEAD main)"
git diff --unified=40 "$base"...HEAD -- \
terraform/environments/aws/ci-cd-permissions/policy_compute.tf \
terraform/environments/aws/ci-cd-permissions/policy_compute_b.tf
printf '\n===== KMS POLICY ACTION REFERENCES =====\n'
rg -n -C 4 \
-e 'GetKeyPolicy' \
-e 'PutKeyPolicy' \
-e 'ScheduleKeyDeletion' \
-e 'CancelKeyDeletion' \
terraform modules || trueLength of output: 74002
No new blocking finding on the corrected deletion path. One non-blocking consistency note: Live AWS verification remains appropriate after the bootstrap apply: aws kms list-resource-tags --key-id <signing-key-id>✏️ Learnings added
✅ Action performedReview finished.
|
cristim
added a commit
that referenced
this pull request
Jul 27, 2026
…rant (#1514) Follow-up to #1496. That PR fixed KMSMutateTaggedOnly in policy_compute.tf to use StringEqualsIgnoreCase after discovering the OIDC signing key is tagged Project=cudly (lowercase, from local.common_tags / var.project_name), which overrides the provider default_tags value CUDly and never matched a case-sensitive StringEquals "CUDly". KMSReadTaggedOnly in policy_compute_b.tf gates the deploy role's only grant for kms:GetKeyPolicy on the same tag with case-sensitive StringEquals, so it does not match the signing key either. The AWS provider reads the key policy on every aws_kms_key refresh, so the deploy role hits AccessDenied on kms:GetKeyPolicy when refreshing or replacing the signing key, blocking the RSA to ECC replace that #1496/#1512 exist to support. Switch KMSReadTaggedOnly to StringEqualsIgnoreCase for consistency with KMSMutateTaggedOnly. terraform validate passes.
cristim
added a commit
that referenced
this pull request
Aug 3, 2026
Second slice of the #1698 audit, covering the compute, container, edge and networking resources. Same three defect classes as the RDS fix: an action that is missing, an action gated on a condition that cannot hold, and an action whose grant can never match the request. Blocking on the deploy path: - ec2:CreateLaunchTemplateVersion, ec2:ModifyLaunchTemplate and ec2:DeleteLaunchTemplateVersions. The fck-nat launch template sources its AMI from a most_recent = true lookup, so every upstream AMI republish takes the launch template's update path rather than create. Only Create/DeleteLaunchTemplate were granted, so the first AMI rotation after the ASG exists would have failed the apply. - EC2RunInstancesFckNAT used StringEquals on ec2:InstanceType. AWS authorizes one RunInstances call against every resource type it touches, and ec2:InstanceType is only in the request context for the instance leg, so the condition evaluated false on the volume, network-interface, security-group, subnet, image and launch-template legs and denied the call as a whole. Switched to StringEqualsIfExists, which keeps the t4g.nano restriction exactly where the key exists. This is the operator the IAM condition-operators reference documents for this call. - iam:CreateServiceLinkedRole for autoscaling.amazonaws.com, elasticloadbalancing.amazonaws.com and ecs.amazonaws.com. Service-linked roles are created lazily on first use of a service in an account/region, so these are invisible where the role already exists and a hard failure in a fresh region. Note ecs.application-autoscaling was already granted and is a different principal that does not cover plain ecs. Latent, fixed while the file is open: - ec2:DeleteTags, the other half of ec2:CreateTags. The provider removes dropped keys before adding new ones, so dropping any key from common_tags or default_tags on an existing EC2 resource failed the apply. - ec2:ReplaceRouteTableAssociation, which the association's update path calls instead of Disassociate plus Associate. - ecr:PutImageScanningConfiguration, whose sibling PutImageTagMutability was already granted. - The elasticloadbalancing Set* family plus ModifyListenerAttributes. Changing a load balancer's subnets or security groups, which is what bumping az_count does, goes through Set* rather than Modify*. - CloudFrontMutateTaggedOnly was gated on a case-sensitive StringEquals against "CUDly" while the distribution's Project tag resolves to the lowercase "cudly" via local.common_tags, so all four of its actions were silently denied. This is the same defect #1496 fixed on the KMS statements, left unfixed here because enable_cdn is false in every tfvars so nothing exercised it. It would also have killed the destroy path, since deleting a distribution requires UpdateDistribution to disable it first. The ECR and ELB statements go in policy_compute_b.tf rather than policy_compute.tf, which is the file's stated purpose: policy_compute is at 5933 of 6144 characters and would not hold them. Sizes after this change: compute 5933, compute-b 1290, data 4588, networking 2831. Refs #1698
cristim
added a commit
that referenced
this pull request
Aug 3, 2026
…ited grant gaps (#1699) * fix(iac/aws): unblock deploy on rds:DescribeDBInstances and close audited grant gaps terraform plan on main fails with AccessDenied on rds:DescribeDBInstances against arn:aws:rds:us-east-1:*:db:*, even though the action was already granted on arn:aws:rds:*:*:db:cudly-* and the instance name matches that prefix. The Terraform id of aws_db_instance is the DbiResourceId (db-<opaque>), not the DB identifier. provider v5.100.0's findDBInstanceByID branches on the id shape and, for a DbiResourceId, sends DescribeDBInstances with Filters=[dbi-resource-id] and no DBInstanceIdentifier. RDS authorizes an identifier-less DescribeDBInstances against the wildcard ARN db:*, which no name-scoped grant can match. It is unconditional on every read: the provider's retry with the plain identifier only fires on NotFound, and AccessDenied is not NotFound, so the plan hard-fails. Rather than add the one action that 403'd, this audits every resource and data block under terraform/environments/aws/ and its modules against the union of the four deploy policies, and fixes the gaps that block the deploy path. It also names a third failure mode the previous four patches did not account for: a grant can be present and still unusable because the request carries no identifier, or because the real ARN uses a different segment or a server-assigned opaque id. Fixed here: - Move rds:DescribeDBInstances and the DB proxy reads to a read-only Resource = "*" statement; every mutating RDS action stays ARN-scoped. Return rds:DescribeDBSubnetGroups to the scoped statement, since that call does carry a name and needs no widening. - Add rds:ModifyDBSubnetGroup, which is required whenever az_count or the private subnet set changes. - Extend the iam:PassedToService allowlist with ec2.amazonaws.com (fck-nat instance profile, on the deploy path in every environment), vpc-flow-logs.amazonaws.com (staging and prod) and events.amazonaws.com (scheduled ECS tasks). Each was a granted-but-unsatisfiable condition of the same shape as the kms:TagResource bug. - Add iam:UpdateAssumeRolePolicy, iam:UpdateRole and iam:UpdateRoleDescription for trust-policy and description drift on cudly-* roles. - Add secretsmanager:UpdateSecretVersionStage and secretsmanager:ListSecretVersionIds for secret versions carrying a stage other than a lone AWSCURRENT. - Drop secretsmanager:ListSecrets, which has no resource type and so could never be satisfied inside an ARN-scoped statement. Nothing on the deploy path calls it. - Add IAMDenyModifyDeployRoleAndPolicies. iam:UpdateAssumeRolePolicy on role/cudly-* also matches cudly-terraform-deploy, so granting it would let the deploy role rewrite its own trust policy. The Deny closes that along with two pre-existing doors into the same escalation: AttachRolePolicy / PutRolePolicy on the deploy role, and CreatePolicyVersion on the cudly-deploy-* policies. Denying only the role side would have looked complete without being complete. - Document, for every Resource = "*", which of the three reasons applies (no resource type, identifier-less request, or opaque id) so a reviewer can tell a justified wildcard from a lazy one, and mark the known-dead RDS proxy ARN scopes in place so nothing new is added to them. Deliberately not fixed here, enumerated in the issue and tracked separately: the RDS proxy ARN scopes (proxy:/target-group: are unmatchable but the path is gated off by enable_rds_proxy = false, and scoping an opaque id needs a tag condition whose satisfiability has to be verified rather than assumed), and the permissions for the monitoring module, cleanup-lambda and CloudFront, none of which are instantiated on any deploy path today. All four managed policies stay under the 6144-character limit; cudly-deploy-data goes from 3548 to 4219 characters. This root is bootstrap-only and is applied manually by a privileged human, so merging does not by itself unblock deploys. Closes #1698 * fix(iac/aws): close audited deploy-role gaps in compute and networking Second slice of the #1698 audit, covering the compute, container, edge and networking resources. Same three defect classes as the RDS fix: an action that is missing, an action gated on a condition that cannot hold, and an action whose grant can never match the request. Blocking on the deploy path: - ec2:CreateLaunchTemplateVersion, ec2:ModifyLaunchTemplate and ec2:DeleteLaunchTemplateVersions. The fck-nat launch template sources its AMI from a most_recent = true lookup, so every upstream AMI republish takes the launch template's update path rather than create. Only Create/DeleteLaunchTemplate were granted, so the first AMI rotation after the ASG exists would have failed the apply. - EC2RunInstancesFckNAT used StringEquals on ec2:InstanceType. AWS authorizes one RunInstances call against every resource type it touches, and ec2:InstanceType is only in the request context for the instance leg, so the condition evaluated false on the volume, network-interface, security-group, subnet, image and launch-template legs and denied the call as a whole. Switched to StringEqualsIfExists, which keeps the t4g.nano restriction exactly where the key exists. This is the operator the IAM condition-operators reference documents for this call. - iam:CreateServiceLinkedRole for autoscaling.amazonaws.com, elasticloadbalancing.amazonaws.com and ecs.amazonaws.com. Service-linked roles are created lazily on first use of a service in an account/region, so these are invisible where the role already exists and a hard failure in a fresh region. Note ecs.application-autoscaling was already granted and is a different principal that does not cover plain ecs. Latent, fixed while the file is open: - ec2:DeleteTags, the other half of ec2:CreateTags. The provider removes dropped keys before adding new ones, so dropping any key from common_tags or default_tags on an existing EC2 resource failed the apply. - ec2:ReplaceRouteTableAssociation, which the association's update path calls instead of Disassociate plus Associate. - ecr:PutImageScanningConfiguration, whose sibling PutImageTagMutability was already granted. - The elasticloadbalancing Set* family plus ModifyListenerAttributes. Changing a load balancer's subnets or security groups, which is what bumping az_count does, goes through Set* rather than Modify*. - CloudFrontMutateTaggedOnly was gated on a case-sensitive StringEquals against "CUDly" while the distribution's Project tag resolves to the lowercase "cudly" via local.common_tags, so all four of its actions were silently denied. This is the same defect #1496 fixed on the KMS statements, left unfixed here because enable_cdn is false in every tfvars so nothing exercised it. It would also have killed the destroy path, since deleting a distribution requires UpdateDistribution to disable it first. The ECR and ELB statements go in policy_compute_b.tf rather than policy_compute.tf, which is the file's stated purpose: policy_compute is at 5933 of 6144 characters and would not hold them. Sizes after this change: compute 5933, compute-b 1290, data 4588, networking 2831. Refs #1698 * fix(iac/aws): move rds:DescribeDBSubnetGroups to the account-wide read statement rds:DescribeDBSubnetGroups was the only rds:Describe* left in the ARN-scoped RDSResourceScoped statement, and it is in exactly the same position as the rds:DescribeDBInstances read that has been failing the deploy: aws_db_subnet_group.main (terraform/modules/database/aws/main.tf:73) is created in every environment, and a resource-scoped grant on a Describe authorizes only the form that targets one named resource, while the enumerate form carries no identifier and is authorized against the wildcard ARN. Worth recording why this is not the simpler claim that ARN-scoped Describes are always dead, because that claim is false and would have justified the wrong fix elsewhere. AWS's Service Authorization Reference confirms both actions DO support resource types (db and subgrp), and the real names DO match the patterns (cudly-<env>-<hex>-postgres against db:cudly-*, cudly-<env>-<hex>-db-subnet against subgrp:cudly-*). The scoped grants were syntactically valid; only the request shape differs. The production 403 on DescribeDBInstances is the empirical proof that the scoped form did not suffice. Also records two facts established during review, so they are not rediscovered: - enable_rds_proxy is a literal false at terraform/environments/aws/database.tf:28, not a variable, so no tfvars can override it. All three proxy resources are count = 0 in every environment, which makes the proxy Describes here belt-and-braces rather than load-bearing. - The RDSResourceScoped comment now says no rds:Describe* lives there at all, rather than naming only DescribeDBInstances. Policy size is unchanged at 4588 of 6144 characters: the action moved between statements rather than being added. Refs #1698
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The post-merge deploy of #1480 (OIDC ES256 migration) fails:
aws_kms_key.signingchanged specRSA_2048->ECC_NIST_P256(#1480). KMScannot change a key spec in place, so Terraform must replace the key. The
deploy role has KMS data-plane + alias actions but no key-lifecycle actions
(the signing key was bootstrap-created), so the replace is denied.
Change
Adds tag-gated KMS key-lifecycle grants to
cudly-deploy-compute-b:KMSKeyLifecycleTaggedOnly(ResourceTagProject=CUDly):ScheduleKeyDeletion,CancelKeyDeletion,PutKeyPolicy,EnableKeyRotation/DisableKeyRotation,GetKeyRotationStatus,ListResourceTags. Destructive deletion is scoped so the deploy SA can never schedule deletion of an unrelated account CMK.KMSTagResourceRequestGated(RequestTagProject=CUDly):TagResource/UntagResource(a new key is untagged at create, so gate on the request tag).KMSCreateKey(Resource="*"):CreateKeycannot be resource/tag-scoped; benign (creating a CMK grants no data access; deletion stays tag-gated).terraform fmt+validateclean.Operational notes
aws_kms_key.signinghas nocreate_before_destroy, so the replace schedules the old signing key for deletion before the new EC key exists -> a brief OIDC-signing gap on deploy. Considerlifecycle { create_before_destroy = true }on the signing key.Summary by CodeRabbit