fix(security): narrow IAM Resource="*" on ECS/ECR/CloudFront/KMS/SES#103
Conversation
Closes 5 HIGH findings (H4–H8) from the security review. Each Resource="*"
on the deploy SA / runtime Lambda role now uses the tightest scope AWS IAM
allows for the action, falling back to a Project=CUDly tag condition where
the API doesn't accept a resource ARN at create time.
H4 — ECS (policy_compute.tf):
Split the previous all-* statement into three:
1. Resource-scoped: Create/Update/Delete on cluster/cudly-*,
service/cudly-*/*, task/cudly-*/*, task-definition/cudly-*:*.
2. Account-wide read: RegisterTaskDefinition (no resource ARN at
registration time) + ListTagsForResource (read-only).
3. Task-definition-scoped: Deregister/Describe limited to
task-definition/cudly-*:*.
The deploy SA can no longer touch unrelated ECS clusters/services in
the same account.
H5 — SES SendEmail (modules/compute/aws/lambda/main.tf):
Added `ses:FromAddress` StringLike condition restricting send actions
to *@${var.email_from_domain}. The Resource stays "*" because SES walks
the identity hierarchy (see existing comment), but a compromised Lambda
can no longer spoof from arbitrary verified identities — only from
CUDly's configured domain. Read-only ses:GetAccount/GetEmailIdentity
split into a separate statement (no spoofing risk).
H6 — ECR (policy_compute.tf):
Repository-scoped actions now require resource ARN
arn:aws:ecr:*:*:repository/cudly-*. ecr:GetAuthorizationToken split
into its own statement because it's an account-wide token endpoint
with no resource ARN.
H7 — CloudFront (policy_compute.tf):
Split into Create+Read (Resource=*) and Mutate (Resource=* +
StringEquals aws:ResourceTag/Project=CUDly). The Terraform
aws_cloudfront_distribution sets the Project tag at creation time, so
CUDly distributions remain manageable while attempts to update or
delete unrelated distributions in the same account are denied.
H8 — KMS (policy_compute.tf):
Same pattern as CloudFront: read/create stay broad (key ARN unknown
at create time, terraform plan needs to enumerate), but mutating
actions (ScheduleKeyDeletion, DisableKey, PutKeyPolicy, etc.) now
require aws:ResourceTag/Project=CUDly. Closes the path where the
deploy SA could schedule deletion of unrelated KMS keys.
Operator action required: this is a bootstrap module per CLAUDE.md's
CI/CD IAM split — `terraform/environments/aws/ci-cd-permissions/` is
applied manually by a privileged human, not by the CI workflow itself.
After merging, re-apply that directory out of band. Until then, the
deploy SA still has the old (broader) grants and nothing breaks.
Validation:
- terraform fmt clean
- terraform validate green
- No runtime code paths touched
|
@coderabbitai review |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughSplit and narrowed CI/CD compute IAM policies: ECR, CloudFront, ECS, SES, and KMS permissions were re-scoped into ARN- and tag-restricted statements; a new managed policy was added and attached to the deploy role; Lambda SES send permissions were constrained to the project email domain and SES read actions were added. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
✅ Actions performedReview triggered.
|
cristim
added
triaged
|
P1 — closes H4–H8: narrows 5 IAM |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
terraform/environments/aws/ci-cd-permissions/policy_compute.tf (1)
280-318:⚠️ Potential issue | 🟠 MajorSplit KMS alias actions out from the key-tagged statement.
AWS KMS requires permissions on both the alias and the associated key for
kms:DeleteAliasandkms:UpdateAliasoperations. The condition keyaws:ResourceTag/Projectapplies only to key resources, not alias resources. Withkms:CreateAliasunrestricted in the broad bucket andkms:DeleteAlias/kms:UpdateAliasgated only by key tags, alias management is both broader than intended and inconsistently scoped. A separate statement targeting alias ARNs for alias operations would properly gate alias management independently from key resource tagging. (docs.aws.amazon.com/kms)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terraform/environments/aws/ci-cd-permissions/policy_compute.tf` around lines 280 - 318, The KMS alias actions are mixed into the "KMSMutateTaggedOnly" statement which uses a key-only condition ("aws:ResourceTag/Project") and thus does not correctly constrain alias operations; split alias-specific actions into their own statement: remove "kms:DeleteAlias" and "kms:UpdateAlias" from the Sid "KMSMutateTaggedOnly" Action list and add a new statement (e.g., Sid "KMSAliasMutate") that grants "kms:CreateAlias", "kms:DeleteAlias", and "kms:UpdateAlias" (and any other alias-only actions) with Resource set to alias ARNs (arn:aws:kms:*:*:alias/*) and appropriate conditions (aliases cannot be conditioned by aws:ResourceTag/Project), so alias permission scope is explicit and separate from key-tag gated key mutations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@terraform/environments/aws/ci-cd-permissions/policy_compute.tf`:
- Around line 137-153: The CloudFrontMutateTaggedOnly statement mixes
distribution mutations with function actions that do not support the
aws:ResourceTag condition; remove "cloudfront:DeleteFunction",
"cloudfront:PublishFunction", and "cloudfront:UpdateFunction" from the
CloudFrontMutateTaggedOnly statement and keep the StringEquals
"aws:ResourceTag/Project" = "CUDly" condition only for distribution-related
actions (e.g., DeleteDistribution, UpdateDistribution) so tag scoping applies;
then add a new separate statement granting the function actions and scope them
to explicit CloudFront Function ARNs (use the
arn:aws:cloudfront::ACCOUNT:function/function-name pattern or a narrower
ARN/wildcard) with no aws:ResourceTag condition; reference the existing
statement name CloudFrontMutateTaggedOnly for removal and create a new statement
(e.g., CloudFrontFunctionMutateScoped) for the function actions.
- Around line 172-194: The policy block with Sid "ECSFargateResourceScoped"
improperly scopes the action ecs:ListTasks to specific ARNs; create a new IAM
statement that contains only Action = ["ecs:ListTasks"] with Resource = ["*"]
(keeping the existing ECSFargateResourceScoped statement for the other actions),
and if you need to limit it further add an appropriate Condition using the
ecs:cluster key to restrict to clusters like "arn:aws:ecs:*:*:cluster/cudly-*".
---
Outside diff comments:
In `@terraform/environments/aws/ci-cd-permissions/policy_compute.tf`:
- Around line 280-318: The KMS alias actions are mixed into the
"KMSMutateTaggedOnly" statement which uses a key-only condition
("aws:ResourceTag/Project") and thus does not correctly constrain alias
operations; split alias-specific actions into their own statement: remove
"kms:DeleteAlias" and "kms:UpdateAlias" from the Sid "KMSMutateTaggedOnly"
Action list and add a new statement (e.g., Sid "KMSAliasMutate") that grants
"kms:CreateAlias", "kms:DeleteAlias", and "kms:UpdateAlias" (and any other
alias-only actions) with Resource set to alias ARNs (arn:aws:kms:*:*:alias/*)
and appropriate conditions (aliases cannot be conditioned by
aws:ResourceTag/Project), so alias permission scope is explicit and separate
from key-tag gated key mutations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f359648f-5659-4a4f-9541-ebf889bcdc61
📒 Files selected for processing (2)
terraform/environments/aws/ci-cd-permissions/policy_compute.tfterraform/modules/compute/aws/lambda/main.tf
| Sid = "CloudFrontMutateTaggedOnly" | ||
| Effect = "Allow" | ||
| Action = [ | ||
| "cloudfront:DeleteDistribution", | ||
| "cloudfront:DeleteFunction", | ||
| "cloudfront:PublishFunction", | ||
| "cloudfront:TagResource", | ||
| "cloudfront:UntagResource", | ||
| "cloudfront:UpdateDistribution", | ||
| "cloudfront:UpdateFunction", | ||
| ] | ||
| Resource = "*" | ||
| Condition = { | ||
| StringEquals = { | ||
| "aws:ResourceTag/Project" = "CUDly" | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
AWS CloudFront service authorization reference function resource type ResourceTag
💡 Result:
In AWS CloudFront's Service Authorization Reference, ResourceTag refers to the resource condition key #amazoncloudfront:aws:ResourceTag/${TagKey}, available for all taggable CloudFront resource types. This key allows attribute-based access control (ABAC) in IAM policies by evaluating tags on CloudFront resources like distributions, streaming-distributions, anycast-ip-list, vpcorigin, distribution-tenant, connection-group, trust-store, and connection-function. Example ARN format for distribution: arn:${Partition}:cloudfront::${Account}:distribution/${DistributionId} Condition keys column lists #amazoncloudfront:aws:ResourceTag/${TagKey} for each, enabling policies like: { "Condition": { "StringEquals": { "cloudfront:aws:ResourceTag/environment": "production" } } } This supports tag-based permissions on supported resources. CloudFront partially supports ABAC for taggable resources. Tagging is managed via console, API, CLI for distributions and other listed types (not origin access identities or invalidations).
Citations:
- 1: https://docs.aws.amazon.com/service-authorization/latest/reference/list_amazoncloudfront.html
- 2: https://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/tagging.html
- 3: https://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/security_iam_service-with-iam.html
🌐 Web query:
CloudFront DeleteFunction PublishFunction UpdateFunction IAM resource types
💡 Result:
The IAM actions cloudfront:DeleteFunction, cloudfront:PublishFunction, and cloudfront:UpdateFunction all use the CloudFront function resource type. Resource type: function ARN format: arn:${Partition}:cloudfront::${Account}:function/${Name} These actions support resource-level permissions, so specify the function ARN in the IAM policy Resource element. No condition keys are defined for the function resource. Example IAM policy statement: { "Version": "2012-10-17", "Statement": [ { "Effect": "Allow", "Action": [ "cloudfront:DeleteFunction", "cloudfront:PublishFunction", "cloudfront:UpdateFunction" ], "Resource": "arn:aws:cloudfront::123456789012:function/my-function" } ] }
Citations:
🌐 Web query:
CloudFront function resource type tagging support aws:ResourceTag condition
💡 Result:
CloudFront Functions (resource type AWS::CloudFront::Function, ARN arn:aws:cloudfront::account:function/name) do not support aws:ResourceTag condition keys in IAM policies. The official AWS Service Authorization Reference lists no condition keys (empty column) for this resource type, unlike distributions and other CloudFront resources which explicitly support #aws:ResourceTag/${TagKey}. CloudFront provides general TagResource/TagFunction APIs that accept function ARNs, indicating functions can be tagged. However, the IAM service authorization documentation confirms no support for tag-based conditions (aws:ResourceTag) specifically for CloudFront Functions. This means you cannot use conditions like "StringEquals": {"aws:ResourceTag/MyTag": "MyValue"} to restrict CloudFront function actions (e.g., cloudfront:CreateFunction, cloudfront:PublishFunction, cloudfront:UpdateFunction) based on resource tags.
Citations:
Split function permissions from distribution mutations—CloudFront Functions don't support the aws:ResourceTag condition.
This statement mixes distribution mutations with cloudfront:DeleteFunction, cloudfront:PublishFunction, and cloudfront:UpdateFunction. The CloudFront function resource type does not support aws:ResourceTag as a condition key in IAM policies. This means the Project=CUDly tag condition will not apply to or restrict function actions—leaving function permissions unscoped by tags. Move function actions into a separate statement with explicit ARN-based resource scoping (e.g., arn:aws:cloudfront::ACCOUNT:function/function-name), and keep tag-based conditions only for resource types that support them (distributions, streaming distributions, etc.).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@terraform/environments/aws/ci-cd-permissions/policy_compute.tf` around lines
137 - 153, The CloudFrontMutateTaggedOnly statement mixes distribution mutations
with function actions that do not support the aws:ResourceTag condition; remove
"cloudfront:DeleteFunction", "cloudfront:PublishFunction", and
"cloudfront:UpdateFunction" from the CloudFrontMutateTaggedOnly statement and
keep the StringEquals "aws:ResourceTag/Project" = "CUDly" condition only for
distribution-related actions (e.g., DeleteDistribution, UpdateDistribution) so
tag scoping applies; then add a new separate statement granting the function
actions and scope them to explicit CloudFront Function ARNs (use the
arn:aws:cloudfront::ACCOUNT:function/function-name pattern or a narrower
ARN/wildcard) with no aws:ResourceTag condition; reference the existing
statement name CloudFrontMutateTaggedOnly for removal and create a new statement
(e.g., CloudFrontFunctionMutateScoped) for the function actions.
| Sid = "ECSFargateResourceScoped" | ||
| Effect = "Allow" | ||
| Action = [ | ||
| "ecs:CreateCluster", | ||
| "ecs:CreateService", | ||
| "ecs:DeleteCluster", | ||
| "ecs:DeleteService", | ||
| "ecs:DeregisterTaskDefinition", | ||
| "ecs:DescribeClusters", | ||
| "ecs:DescribeServices", | ||
| "ecs:DescribeTaskDefinition", | ||
| "ecs:ListTagsForResource", | ||
| "ecs:ListTasks", | ||
| "ecs:PutClusterCapacityProviders", | ||
| "ecs:StopTask", | ||
| "ecs:RegisterTaskDefinition", | ||
| "ecs:TagResource", | ||
| "ecs:UntagResource", | ||
| "ecs:UpdateCluster", | ||
| "ecs:UpdateService", | ||
| ] | ||
| Resource = [ | ||
| "arn:aws:ecs:*:*:cluster/cudly-*", | ||
| "arn:aws:ecs:*:*:service/cudly-*/*", | ||
| "arn:aws:ecs:*:*:task/cudly-*/*", | ||
| "arn:aws:ecs:*:*:task-definition/cudly-*:*", | ||
| ] |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
ecs:ListTasks AWS resource type supported container-instance documentation
💡 Result:
Yes, the AWS ECS ListTasks API supports filtering by container-instance. The 'containerInstance' parameter accepts the container instance ID or full ARN and limits results to tasks belonging to that container instance. This is documented in the official AWS ECS API reference.
Citations:
- 1: https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_ListTasks.html
- 2: https://docs.aws.amazon.com/code-library/latest/ug/ecs_example_ecs_ListTasks_section.html
- 3: https://botocore.amazonaws.com/v1/documentation/api/latest/reference/services/ecs/client/list_tasks.html
- 4: https://docs.aws.amazon.com/cli/latest/reference/ecs/list-tasks.html
🌐 Web query:
AWS service authorization reference ecs:ListTasks resource types supported
💡 Result:
The AWS ecs:ListTasks action does not support resource-level permissions. In IAM policies, you must specify "Resource": "" (all resources) to grant permission for this action. This is indicated in the official AWS Service Authorization Reference for Amazon ECS, where List actions like ecs:ListTasks have no specific resource types listed in the "Resource types" column of the Actions table. Per AWS documentation: "If there is no value for this column [Resource types], you must specify all resources ("") to which the policy applies in the Resource element of your policy statement." Examples in AWS ECS IAM policy documentation consistently use "Resource": "" for ecs:ListTasks and ecs:List actions, often combined with conditions like "ArnEquals": {"ecs:cluster": "arn:..."} to restrict to specific clusters. ECS defines resource types such as cluster, service, task, container-instance, task-definition, etc., but ecs:ListTasks (a List access level action) is not scoped to any of them.
Citations:
- 1: https://docs.aws.amazon.com/service-authorization/latest/reference/list_amazonelasticcontainerservice.html
- 2: https://docs.aws.amazon.com/AmazonECS/latest/developerguide/security_iam_id-based-policy-examples.html
- 3: https://docs.aws.amazon.com/AmazonECS/latest/developerguide/security_iam_service-with-iam.html
ecs:ListTasks does not support resource-level permissions and must use "Resource": "*".
According to AWS service authorization documentation, List actions like ecs:ListTasks cannot be scoped to specific resource types. Move ecs:ListTasks to its own statement with "Resource": "*" and optionally use the ecs:cluster condition key to restrict it to specific clusters if needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@terraform/environments/aws/ci-cd-permissions/policy_compute.tf` around lines
172 - 194, The policy block with Sid "ECSFargateResourceScoped" improperly
scopes the action ecs:ListTasks to specific ARNs; create a new IAM statement
that contains only Action = ["ecs:ListTasks"] with Resource = ["*"] (keeping the
existing ECSFargateResourceScoped statement for the other actions), and if you
need to limit it further add an appropriate Condition using the ecs:cluster key
to restrict to clusters like "arn:aws:ecs:*:*:cluster/cudly-*".
CodeRabbit flagged three statements in policy_compute.tf where the
intended scoping is silently broken because the AWS service does not
support the condition key or resource type used. In all three cases
the policy LOOKED narrow but actually granted account-wide access for
the affected actions:
1. CloudFront functions (DeleteFunction/PublishFunction/
UpdateFunction) operate on the `function` resource type, which
does not support the aws:ResourceTag condition key per the AWS
Service Authorization Reference. They were grouped with
distribution mutations under CloudFrontMutateTaggedOnly with a
`aws:ResourceTag/Project=CUDly` condition that silently
evaluates to no-match for functions, so the deploy SA could
mutate any CloudFront function in the account.
2. ecs:ListTasks does not support resource-level permissions per
the AWS Service Authorization Reference (no resource types are
listed for the action). It was bundled with cluster/service/task
ARNs under ECSFargateResourceScoped where AWS silently ignores
the Resource list, so the deploy SA could list tasks in any
cluster in the account.
3. KMS alias actions (CreateAlias/DeleteAlias/UpdateAlias) act on
the `alias` resource type, but the aws:ResourceTag/Project gate
applies only to keys (aliases inherit no IAM-visible tags).
CreateAlias was in the unrestricted KMSCreateAndRead bucket and
DeleteAlias/UpdateAlias in the key-tag-gated bucket, so the
deploy SA could create or rename aliases on any KMS key in the
account.
Each fix replaces the silently-no-op tag/ARN scoping with explicit
ARN-prefix scoping that AWS will actually enforce. Net effect tightens
permissions; no new grants are introduced.
Structural note — split into a second managed policy:
The existing `aws_iam_policy.compute` is at AWS's 6144-char default
managed-policy limit. The three new ARN-scoped statements wouldn't fit
without evicting unrelated existing grants, so they live in a new
`aws_iam_policy.compute_b` (file `policy_compute_b.tf`) attached to
the same role. Three statements moved out, three statements added in.
Verification:
- terraform validate, terraform fmt, tflint all pass.
- tflint's aws_iam_policy_too_long_policy rule confirms the main
policy is back under 6144 chars after the alias/ListTasks
deletions.
- Each new statement uses an AWS-supported condition key or
resource type per the Service Authorization Reference (verified
against docs cited in CR's review).
CR triage — round 1 (commit
|
| # | Finding | Status | Fix |
|---|---|---|---|
| 1 | CloudFrontMutateTaggedOnly mixed cloudfront:DeleteFunction/PublishFunction/UpdateFunction with distribution mutations under an aws:ResourceTag/Project=CUDly condition. The CloudFront function resource type does not support aws:ResourceTag per the AWS Service Authorization Reference, so the gate silently no-ops. |
Fixed | Removed function actions from CloudFrontMutateTaggedOnly; added CloudFrontFnMutate with Resource = "arn:aws:cloudfront::*:function/cudly-*" (CF function ARNs are global, no region segment). |
| 2 | ECSFargateResourceScoped listed ecs:ListTasks alongside cluster/service/task ARNs. Per the AWS Service Authorization Reference, ecs:ListTasks is a List action with no resource types, so AWS silently ignores Resource and the policy admits ListTasks against any cluster in the account. |
Fixed | Removed ecs:ListTasks from ECSFargateResourceScoped; added ECSListTasks with Resource="*" and Condition.ArnLike.ecs:cluster = "arn:aws:ecs:*:*:cluster/cudly-*" (the ecs:cluster key IS supported for ListTasks). |
| 3 | KMSCreateAndRead granted kms:CreateAlias unrestricted, and KMSMutateTaggedOnly gated kms:DeleteAlias/UpdateAlias with aws:ResourceTag/Project=CUDly — but that tag condition matches only key resources, not aliases (aliases inherit no IAM-visible tags). Result: deploy SA could create/rename aliases on any KMS key in the account. |
Fixed (outside-diff comment) | Removed alias actions from both buckets; added KMSAliasMutate with Resource = "arn:aws:kms:*:*:alias/cudly-*". |
Structural note — split into a second managed policy
aws_iam_policy.compute was at AWS's 6144-char default managed-policy limit. Adding the three new ARN-scoped statements wouldn't fit without evicting unrelated existing grants, so they live in a new aws_iam_policy.compute_b (file policy_compute_b.tf) attached to the same role via role.tf. Three statements moved out of the main policy, three statements added to the new one — net deploy-SA permissions tighten in every case, no new grants.
Verification
terraform validate,terraform fmt -check, andtflint(with the AWS plugin'saws_iam_policy_too_long_policyrule) all pass locally.- Each new statement uses an AWS-supported condition key or resource type per the Service Authorization Reference.
- Pre-commit hooks ran cleanly.
@coderabbitai review
🧩 Analysis chain🏁 Script executed: Length of output: 452 🏁 Script executed: Length of output: 41 🏁 Script executed: Length of output: 31701
Round-2 findings on
|
| Statement | Verdict |
|---|---|
CloudFrontFnMutate — ARN-scoped to arn:aws:cloudfront::*:function/cudly-* |
✅ Correct. CF function resource type does not support aws:ResourceTag; ARN scope is the right gate. |
ECSListTasks — Resource="*" + ArnLike ecs:cluster condition |
✅ Correct. ecs:ListTasks has no resource-level permissions; ecs:cluster condition key is a supported context key for ListTasks per the AWS SAR. |
KMSAliasMutate (alias ARN scope, removed from tag-gated bucket) |
✅ Conceptually correct split; see the blocking issue above about the missing key ARN. |
✅ Actions performed
Review triggered.
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@terraform/environments/aws/ci-cd-permissions/policy_compute_b.tf`:
- Around line 53-60: The KMSAliasMutate statement currently only allows alias
ARNs which is insufficient for kms:CreateAlias and kms:UpdateAlias because AWS
evaluates both the alias resource and the target key resource; update the
statement with Sid "KMSAliasMutate" to grant the same actions
("kms:CreateAlias","kms:DeleteAlias","kms:UpdateAlias") on both the alias ARNs
and the corresponding key ARNs (e.g. include "arn:aws:kms:*:*:key/*" or the
specific key ARNs) so Create/Update flows have permission on the target key as
well.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3f879d72-668f-46fa-a827-28e5529d5725
📒 Files selected for processing (3)
terraform/environments/aws/ci-cd-permissions/policy_compute.tfterraform/environments/aws/ci-cd-permissions/policy_compute_b.tfterraform/environments/aws/ci-cd-permissions/role.tf
🚧 Files skipped from review as they are similar to previous changes (1)
- terraform/environments/aws/ci-cd-permissions/policy_compute.tf
| Sid = "KMSAliasMutate" | ||
| Effect = "Allow" | ||
| Action = [ | ||
| "kms:CreateAlias", | ||
| "kms:DeleteAlias", | ||
| "kms:UpdateAlias", | ||
| ] | ||
| Resource = "arn:aws:kms:*:*:alias/cudly-*" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In AWS IAM Service Authorization Reference for KMS, what resource types are required for kms:CreateAlias, kms:UpdateAlias, and kms:DeleteAlias? Confirm whether CreateAlias/UpdateAlias require both alias and key resources.
💡 Result:
In the AWS IAM Service Authorization Reference for KMS, the resource types for kms:CreateAlias, kms:UpdateAlias, and kms:DeleteAlias are both "alias" and "key". KMS defines two resource types: alias (arn:aws:kms:::alias/) and key (arn:aws:kms:::key/). These actions require permissions on both resource types: - kms:CreateAlias requires kms:CreateAlias permission on the alias (IAM policy) and on the target key (key policy). - kms:DeleteAlias requires kms:DeleteAlias permission on the alias (IAM policy) and on the associated key (key policy). - kms:UpdateAlias requires kms:UpdateAlias permission on the alias (IAM policy), on the current key (key policy), and on the new target key (key policy). Yes, CreateAlias and UpdateAlias require both alias and key resources (permissions on both types). DeleteAlias also requires both.
Citations:
- 1: https://docs.aws.amazon.com/service-authorization/latest/reference/list_awskeymanagementservice.html
- 2: https://docs.aws.amazon.com/kms/latest/developerguide/kms-api-permissions-reference.html
- 3: https://docs.aws.amazon.com/kms/latest/developerguide/alias-access.html
- 4: https://docs.aws.amazon.com/kms/latest/APIReference/API_CreateAlias.html
- 5: https://docs.aws.amazon.com/kms/latest/APIReference/API_UpdateAlias.html
- 6: https://docs.aws.amazon.com/kms/latest/APIReference/API_DeleteAlias.html
🏁 Script executed:
# First, find the file and check if it exists
fd -t f "policy_compute_b.tf" -x wc -l {}
# Then read the specific lines mentioned
cat -n terraform/environments/aws/ci-cd-permissions/policy_compute_b.tf | sed -n '50,65p'Repository: LeanerCloud/CUDly
Length of output: 528
🏁 Script executed:
# Search for other KMS policies in the same directory
grep -r "kms:CreateAlias\|kms:UpdateAlias\|kms:DeleteAlias" terraform/environments/aws/ci-cd-permissions/ -A 5 -B 5
# Also check if there are other policy files that might have similar issues
fd -t f "policy_.*\.tf" terraform/environments/aws/ci-cd-permissions/Repository: LeanerCloud/CUDly
Length of output: 3871
KMS alias mutations are under-authorized for Create/Update flows.
Line 60 scopes only alias ARNs. For kms:CreateAlias and kms:UpdateAlias, AWS KMS authorization evaluates both the alias resource and the target key resource. Without key permissions, these operations will fail at deploy time with AccessDenied.
🔧 Proposed fix
{
Sid = "KMSAliasMutate"
Effect = "Allow"
Action = [
"kms:CreateAlias",
"kms:DeleteAlias",
"kms:UpdateAlias",
]
- Resource = "arn:aws:kms:*:*:alias/cudly-*"
+ Resource = [
+ "arn:aws:kms:*:*:alias/cudly-*",
+ "arn:aws:kms:*:*:key/*",
+ ]
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Sid = "KMSAliasMutate" | |
| Effect = "Allow" | |
| Action = [ | |
| "kms:CreateAlias", | |
| "kms:DeleteAlias", | |
| "kms:UpdateAlias", | |
| ] | |
| Resource = "arn:aws:kms:*:*:alias/cudly-*" | |
| Sid = "KMSAliasMutate" | |
| Effect = "Allow" | |
| Action = [ | |
| "kms:CreateAlias", | |
| "kms:DeleteAlias", | |
| "kms:UpdateAlias", | |
| ] | |
| Resource = [ | |
| "arn:aws:kms:*:*:alias/cudly-*", | |
| "arn:aws:kms:*:*:key/*", | |
| ] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@terraform/environments/aws/ci-cd-permissions/policy_compute_b.tf` around
lines 53 - 60, The KMSAliasMutate statement currently only allows alias ARNs
which is insufficient for kms:CreateAlias and kms:UpdateAlias because AWS
evaluates both the alias resource and the target key resource; update the
statement with Sid "KMSAliasMutate" to grant the same actions
("kms:CreateAlias","kms:DeleteAlias","kms:UpdateAlias") on both the alias ARNs
and the corresponding key ARNs (e.g. include "arn:aws:kms:*:*:key/*" or the
specific key ARNs) so Create/Update flows have permission on the target key as
well.
…eck (CR r2)
CodeRabbit flagged that KMSAliasMutate in policy_compute_b.tf only
allows alias ARNs, which is insufficient: AWS KMS evaluates
kms:CreateAlias / kms:DeleteAlias / kms:UpdateAlias against BOTH the
alias resource AND the target key resource. Without permissions on
the target key, every CreateAlias/UpdateAlias call (and DeleteAlias)
would 403 at deploy time.
The CR-proposed fix added "arn:aws:kms:*:*:key/*" to the alias
statement's Resource list with no condition, which would allow alias
mutations against ANY key in the account. Tighter alternative: add
the three alias actions to the existing KMSMutateTaggedOnly statement
in policy_compute.tf, which is already tag-gated to keys carrying
Project=CUDly. The deploy SA can now operate aliases targeting the
CUDly OIDC signing key (and any future CUDly keys) but not other
keys in the same account.
The dual-check pattern is now:
- Alias side: policy_compute_b.tf KMSAliasMutate, ARN-scoped to
arn:aws:kms:*:*:alias/cudly-*.
- Key side: policy_compute.tf KMSMutateTaggedOnly, Resource="*"
with aws:ResourceTag/Project=CUDly.
Both must allow for the API call to succeed, which is exactly the
authorization model AWS uses for these actions per the AWS KMS
authorization docs.
CR triage — round 2 (commit
|
| # | Finding | Status | Fix |
|---|---|---|---|
| 1 | KMSAliasMutate scoped only to alias ARNs; missing the key-side check that KMS requires for CreateAlias/UpdateAlias/DeleteAlias. | Fixed | Added kms:CreateAlias, kms:DeleteAlias, kms:UpdateAlias back to KMSMutateTaggedOnly in policy_compute.tf (existing tag-gated key statement). The alias-side check stays in policy_compute_b.tf KMSAliasMutate (ARN-scoped). Both must allow for the API call to succeed, matching AWS's dual-resource authorization model. |
Note: tightened beyond CR's proposed fix
CR's suggested patch added arn:aws:kms:*:*:key/* to the alias statement's Resource list with no condition — that would have allowed alias mutations against ANY key in the account. Instead, the key-side check piggybacks on the existing KMSMutateTaggedOnly statement, which is already tag-gated to Project=CUDly. So the deploy SA can mutate aliases pointing at CUDly-owned keys (the OIDC signing key, etc.) but not at unrelated keys in the same account.
Verification: terraform fmt, terraform validate, and tflint (incl. AWS-plugin policy-too-long check) all pass. Pre-commit hooks ran cleanly.
@coderabbitai review
🧩 Analysis chain🏁 Script executed: Length of output: 41 🏁 Script executed: Length of output: 14840
Round-2 triage — verified ✅KMS dual-resource fix — confirmed correct and tighter than suggestedThe implementation is exactly right. KMS's dual-resource evaluation is now satisfied by two independent statements:
Both 🟡 Still-open: deploy SA
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
terraform/environments/aws/ci-cd-permissions/policy_compute.tf (1)
263-276: Consider removingses:SendEmailor addingses:FromAddresscondition.The deploy SA includes
ses:SendEmailwithResource = "*"and noses:FromAddresscondition. Per PR discussion, the CI deploy role likely doesn't need email-sending capability. If it does, adding the sameses:FromAddressrestriction used by the Lambda runtime would prevent abuse:Condition = { StringLike = { "ses:FromAddress" = "*@${var.email_from_domain}" } }Alternatively, split identity management actions from
SendEmailand remove the latter from this policy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terraform/environments/aws/ci-cd-permissions/policy_compute.tf` around lines 263 - 276, The SES statement currently grants "ses:SendEmail" on Resource="*" (the SES block with Sid = "SES" and Action including "ses:SendEmail"); either remove "ses:SendEmail" from that Action list if the CI deploy role does not need to send mail, or add a condition restricting the sender (mirror the Lambda runtime rule) by adding a Condition with StringLike for "ses:FromAddress" = "*@${var.email_from_domain}" to the same SES statement; alternatively split identity-management actions (CreateEmailIdentity, DeleteEmailIdentity, GetEmailIdentity, PutEmailIdentityDkimSigningAttributes, TagResource, UntagResource) into a separate statement and omit "ses:SendEmail" from the CI policy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@terraform/environments/aws/ci-cd-permissions/policy_compute.tf`:
- Around line 263-276: The SES statement currently grants "ses:SendEmail" on
Resource="*" (the SES block with Sid = "SES" and Action including
"ses:SendEmail"); either remove "ses:SendEmail" from that Action list if the CI
deploy role does not need to send mail, or add a condition restricting the
sender (mirror the Lambda runtime rule) by adding a Condition with StringLike
for "ses:FromAddress" = "*@${var.email_from_domain}" to the same SES statement;
alternatively split identity-management actions (CreateEmailIdentity,
DeleteEmailIdentity, GetEmailIdentity, PutEmailIdentityDkimSigningAttributes,
TagResource, UntagResource) into a separate statement and omit "ses:SendEmail"
from the CI policy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e1b3bad3-6ba9-4b71-9dc2-027b976aba29
📒 Files selected for processing (2)
terraform/environments/aws/ci-cd-permissions/policy_compute.tfterraform/environments/aws/ci-cd-permissions/policy_compute_b.tf
CR pass-3 nitpick on policy_compute.tf:271. The deploy SA's SES statement listed the identity-management actions (Create/Delete/Get EmailIdentity, PutDkimSigningAttributes, Tag/Untag) plus SendEmail — the latter on Resource = "*" with no FromAddress condition. Splitting actions is the principled fix: CI deploy configures email identities (verify domain, rotate DKIM, tag); it doesn't send mail. The runtime Lambda role in terraform/modules/compute/aws/lambda/main.tf keeps its own ses:SendEmail grant (already restricted) for the actual sending path. Dropping the CI grant means a leaked deploy SA can no longer exfiltrate via mail. A comment on the SES statement explains the omission so a future reader doesn't add SendEmail back as a "missing action". Closes the last open CR finding on PR #103.
CR triage — round 3 (commit
|
🧩 Analysis chain🏁 Script executed: Length of output: 30808
Round-3 review — all findings resolved ✅🟢
|
| Round | Finding | Statement | Status |
|---|---|---|---|
| 1 | CloudFront function actions not scopeable via aws:ResourceTag |
CloudFrontFnMutate → arn:aws:cloudfront::*:function/cudly-* |
✅ |
| 1 | ecs:ListTasks ignores resource-level ARN scoping |
ECSListTasks with ArnLike ecs:cluster condition |
✅ |
| 1 | KMS alias tag-gate is a no-op (aliases have no IAM-visible tags) | KMSAliasMutate ARN-scoped to alias/cudly-* |
✅ |
| 2 | KMS alias operations require dual-resource authorization (alias + key) | Key-side added to KMSMutateTaggedOnly (tag-gated); alias-side in KMSAliasMutate (ARN-scoped) |
✅ |
| 3 | Deploy SA ses:SendEmail with Resource="*", no FromAddress condition |
Action removed from CI SA entirely | ✅ |
No open findings remain. The PR is clean and ready to merge — pending the manual terraform apply of terraform/environments/aws/ci-cd-permissions/ by a privileged human post-merge, as noted.
✅ Actions performed
Review triggered.
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
terraform/environments/aws/ci-cd-permissions/policy_compute.tf (1)
117-128: Split CloudFront function reads out of the wildcard statement.Line 122 and Line 125 do not need to stay under
Resource = "*"just because Line 121 does. In the CloudFront authorization reference,DescribeFunctionandGetFunctionare resource-scopeable onfunction*, whileCreateFunctionhas no resource type and therefore must remain wildcarded. Keeping all three together still lets the deploy role read arbitrary CloudFront function config/code in the account. Move just the two read actions into their own statement scoped toarn:aws:cloudfront::*:function/cudly-*. (docs.aws.amazon.com)Proposed change
{ Sid = "CloudFrontCreateAndRead" Effect = "Allow" Action = [ "cloudfront:CreateDistribution", "cloudfront:CreateFunction", - "cloudfront:DescribeFunction", "cloudfront:GetDistribution", "cloudfront:GetDistributionConfig", - "cloudfront:GetFunction", "cloudfront:ListTagsForResource", ] Resource = "*" }, + { + Sid = "CloudFrontFnRead" + Effect = "Allow" + Action = [ + "cloudfront:DescribeFunction", + "cloudfront:GetFunction", + ] + Resource = "arn:aws:cloudfront::*:function/cudly-*" + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terraform/environments/aws/ci-cd-permissions/policy_compute.tf` around lines 117 - 128, The CloudFront statement "CloudFrontCreateAndRead" currently groups CreateFunction (requires Resource="*") with DescribeFunction and GetFunction (which are resource-scoped); split this into two statements: keep a statement containing "cloudfront:CreateFunction" with Resource="*", and create a new statement containing "cloudfront:DescribeFunction" and "cloudfront:GetFunction" scoped to Resource "arn:aws:cloudfront::*:function/cudly-*"; update the Sid values (e.g., "CloudFrontCreate" and "CloudFrontReadFunctions") so the policy clearly separates the wildcard create permission from the scoped read permissions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@terraform/environments/aws/ci-cd-permissions/policy_compute.tf`:
- Around line 117-128: The CloudFront statement "CloudFrontCreateAndRead"
currently groups CreateFunction (requires Resource="*") with DescribeFunction
and GetFunction (which are resource-scoped); split this into two statements:
keep a statement containing "cloudfront:CreateFunction" with Resource="*", and
create a new statement containing "cloudfront:DescribeFunction" and
"cloudfront:GetFunction" scoped to Resource
"arn:aws:cloudfront::*:function/cudly-*"; update the Sid values (e.g.,
"CloudFrontCreate" and "CloudFrontReadFunctions") so the policy clearly
separates the wildcard create permission from the scoped read permissions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0933f105-e9e2-4be8-9134-7fd5f8236e15
📒 Files selected for processing (1)
terraform/environments/aws/ci-cd-permissions/policy_compute.tf
|
@coderabbitai create a commit to address this nitpick for terraform/environments/aws/ci-cd-permissions/policy_compute.tf (1) |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me read the current state of the file first. 🏁 Script executed: Length of output: 12715 I can see the issue: Let me read 🏁 Script executed: Length of output: 2456 Got everything I need. Here's the plan:
|
DescribeFunction and GetFunction support the function resource type per the AWS Service Authorization Reference. Move them out of the wildcard CloudFrontCreateAndRead statement into a new CloudFrontFnRead statement scoped to arn:aws:cloudfront::*:function/cudly-*, matching the ARN scoping already used by CloudFrontFnMutate in policy_compute_b.tf. CreateFunction has no resource type so stays under Resource="*".
|
Addressed CR r4 nitpick in 2a67339:
@coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me read the files at the new commit to verify. 🏁 Script executed: Length of output: 15861 Let me verify one detail before finalizing the review. 🌐 Web query: 💡 Result: In the Amazon CloudFront – Service Authorization Reference, the IAM action
CloudFront does define a
Sources: [1] [2]
Round-4 nitpick — confirmed ✅
My earlier plan incorrectly suggested The Full closure — all 5 rounds resolved ✅
No open findings remain. The PR is clean and ready to merge — pending the manual |
1 participant
Summary
Closes 5 HIGH findings from the security review (H4–H8). Each
Resource = "*"on the deploy SA / runtime Lambda role now uses the tightest scope AWS IAM allows for the action, with aProject=CUDlytag condition where the API doesn't accept a resource ARN at create time.cluster/cudly-*,service/cudly-*/*,task-definition/cudly-*:*), account-wide read (RegisterTaskDefinition + tags), and task-def-scoped Describe/Deregister.*(SES walks identity hierarchy — see existing comment), but newses:FromAddressStringLikecondition restricts send to*@${var.email_from_domain}. Read-only actions split into their own statement.arn:aws:ecr:*:*:repository/cudly-*.GetAuthorizationTokensplit (account-wide token endpoint, no resource ARN).*(ARN unknown at create), but mutate (Update/Delete/Tag) requiresaws:ResourceTag/Project = "CUDly".aws:ResourceTag/Project = "CUDly".⚠ Operator action required
terraform/environments/aws/ci-cd-permissions/is a bootstrap module per the CLAUDE.md CI/CD IAM split — applied manually by a privileged human, not by the CI workflow itself. After merging this PR, re-apply that directory out of band:cd terraform/environments/aws/ci-cd-permissions terraform plan terraform applyUntil that re-apply happens, the deploy SA still has the old (broader) grants and nothing breaks. After re-apply, any drift detection on third-party resources sharing the same account that the deploy SA happens to have been touching becomes visible — that's the fix taking effect.
Test plan
terraform fmt -checkclean.terraform validategreen forenvironments/aws/ci-cd-permissions/.terraform planagainst the bootstrap module in a sandbox account; confirm no live CUDly resources hit "access denied"; then re-apply in prod.Follow-up PR
PR5 (supply chain) — independent of this PR.
🤖 Generated with claude-flow
Summary by CodeRabbit