Automate IAM User Role Policies#3996
Open
rudransh-shrivastava wants to merge 16 commits intoOWASP:feature/nest-zappa-migrationfrom
Open
Automate IAM User Role Policies#3996rudransh-shrivastava wants to merge 16 commits intoOWASP:feature/nest-zappa-migrationfrom
rudransh-shrivastava wants to merge 16 commits intoOWASP:feature/nest-zappa-migrationfrom
Conversation
2 tasks
github-actions
bot
added
docs
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a bootstrap Terraform workspace and IAM roles; refactors backend state into per-environment resources; renames staging project identifiers to "nest"/"nest-staging"; updates CI/CD with a bootstrap job and expanded AWS role-assume parameters for staging flows. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
3 issues found across 20 files
Confidence score: 3/5
- Security risk:
infrastructure/bootstrap/main.tfgrants ECR write actions on all repositories, which can allow pushing images to any repo in the account. - Functional risk: KMS permissions in
infrastructure/backend/README.mdandinfrastructure/staging/README.mdare incomplete or ineffective, which can block key creation or Terraform state uploads. - The issues are concrete and user-impacting (security scope and state write failures), so this is a moderate merge risk until addressed.
- Pay close attention to
infrastructure/bootstrap/main.tf,infrastructure/backend/README.md,infrastructure/staging/README.md- scope ECR permissions and fix KMS action coverage.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="infrastructure/backend/README.md">
<violation number="1" location="infrastructure/backend/README.md:85">
P2: `kms:CreateKey` and `kms:ListAliases` are not resource-scoped actions, so tying them to `${AWS_BACKEND_KMS_KEY_ARN}` makes those permissions ineffective and prevents the user from creating keys or listing aliases. Split non-key-specific actions into a separate statement with `"Resource": "*"`, keeping key-specific actions scoped to the key ARN.</violation>
</file>
<file name="infrastructure/staging/README.md">
<violation number="1" location="infrastructure/staging/README.md:35">
P2: KMS permissions are incomplete for an SSE-KMS encrypted Terraform state bucket. `kms:GenerateDataKey` is required for `s3:PutObject` on KMS-encrypted objects; without it, state uploads will fail.</violation>
</file>
<file name="infrastructure/bootstrap/main.tf">
<violation number="1" location="infrastructure/bootstrap/main.tf:51">
P1: Security: ECR data-plane write actions (`PutImage`, `UploadLayerPart`, etc.) are granted on `resources = ["*"]`, allowing image push to **any** ECR repository in the account. Only `ecr:GetAuthorizationToken` requires `*`. Split this into two statements: one for `GetAuthorizationToken` on `*`, and one for the repository-scoped data-plane actions on `arn:aws:ecr:*:${account_id}:repository/${project_name}-${env}-*`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![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: 8
🧹 Nitpick comments (9)
.gitignore (1)
41-44: Consider adding a production-environment pattern.The new patterns cover
nest-devandnest-staging, but if anest-prod*/nest-production*environment is ever set up, its Zappa build artifacts would not be auto-ignored.💡 Suggested addition
backend/*nest-staging*.tar.gz backend/*nest-staging*.zip + backend/*nest-prod*.tar.gz + backend/*nest-prod*.zip🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore around lines 41 - 44, Add ignore patterns for production Zappa artifacts analogous to the existing dev and staging entries: include patterns like "backend/*nest-prod*.tar.gz", "backend/*nest-prod*.zip", "backend/*nest-production*.tar.gz", and "backend/*nest-production*.zip" so future nest-prod or nest-production builds are auto-ignored alongside the existing backend/*nest-dev* and backend/*nest-staging* entries.infrastructure/backend/README.md (1)
64-86: Consider removingkms:DisableKeyRotationGranting the ability to disable key rotation weakens the long-term security posture of KMS keys. If key rotation is a project requirement, removing this permission prevents accidental or malicious disabling. The existing
kms:EnableKeyRotationis sufficient for rotation management.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/backend/README.md` around lines 64 - 86, The IAM policy statement with "Sid": "KMSManagement" currently includes "kms:DisableKeyRotation" in its "Action" array; remove "kms:DisableKeyRotation" from that array (leaving "kms:EnableKeyRotation" and the other KMS actions intact) so the policy no longer grants the ability to disable key rotation for the resource ${AWS_BACKEND_KMS_KEY_ARN}.infrastructure/README.md (1)
58-68:touchbeforecat >is redundant
cat source > destcreates the destination file if it doesn't exist. The precedingtouchstep adds no value and can be removed throughout the file (same pattern repeats at lines 70-74 and in the staging section).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/README.md` around lines 58 - 68, Remove the redundant touch commands that precede the redirection-based file creation (e.g., the "touch terraform.tfvars" before "cat terraform.tfvars.example > terraform.tfvars") — keep only the cat redirection lines to create/populate files; apply the same change for the repeated pattern (the similar touch/cat pair in the local section and the staging section) so the README uses only "cat {source} > {dest}" without prior touch..github/workflows/run-ci-cd.yaml (1)
701-704:scan-staging-imagesinplan-staging-nest.needsis redundant
bootstrap-staging-nestalready listsscan-staging-imagesin itsneeds, soplan-staging-nest(which depends onbootstrap-staging-nest) implicitly waits forscan-staging-images. The explicit entry is harmless but adds noise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/run-ci-cd.yaml around lines 701 - 704, The job plan-staging-nest currently lists scan-staging-images in its needs array even though bootstrap-staging-nest already depends on scan-staging-images, so remove scan-staging-images from the needs of plan-staging-nest to avoid redundant noise; edit the needs block for plan-staging-nest to only include bootstrap-staging-nest and set-release-version (retain set-release-version) and keep bootstrap-staging-nest as-is so the dependency on scan-staging-images remains transitively satisfied.infrastructure/bootstrap/terraform.tfbackend.example (1)
1-3: Consider addingencrypt = trueto the backend exampleThe backend config example omits
encrypt = true. While bucket-level SSE handles encryption regardless, explicitly setting it in the backend config is a defense-in-depth practice and is consistent with how other Terraform S3 backends recommend configuration. If the staging backend example includes it, add it here for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/bootstrap/terraform.tfbackend.example` around lines 1 - 3, Add the missing encrypt flag to the Terraform S3 backend example by adding encrypt = true to the backend configuration (alongside bucket, dynamodb_table, and region) so the backend explicitly requests server-side encryption for state files for defense-in-depth and consistency with other examples.infrastructure/bootstrap/variables.tf (1)
7-11: Consider adding a validation block for consistency.
infrastructure/backend/variables.tfvalidatesstate_environmentsentries against an allowed list. Thisenvironmentsvariable lacks similar validation, which means typos (e.g.,"stagin") would silently create misnamed IAM resources rather than failing early.Proposed validation
variable "environments" { description = "The environments to create Terraform roles for." type = list(string) default = ["staging"] + + validation { + condition = alltrue([ + for env in var.environments : contains(["staging"], env) + ]) + error_message = "Each environment must be 'staging'." + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/bootstrap/variables.tf` around lines 7 - 11, The variable "environments" lacks input validation so typos can create incorrect IAM resources; add a Terraform validation block to the variable "environments" that checks every item is a member of your allowed environments set (the same allowed list used by state_environments in infrastructure/backend/variables.tf) using an expression like all(var.environments, contains(local.allowed_environments, ...)) and provide a clear error_message describing allowed values; update the "environments" variable declaration to include that validation and reference the same allowed list to keep both variables consistent.infrastructure/bootstrap/main.tf (2)
424-441:KMSManagementgrantsScheduleKeyDeletionandPutKeyPolicyon all KMS keys.While
kms:CreateKeyrequiresresources = ["*"], several actions in this statement are restrictable by key ARN — notablyScheduleKeyDeletionandPutKeyPolicy, which are high-impact operations. Consider splittingCreateKey(andListAliaseswhich also requires"*") into a separate statement, and scoping the remaining actions to project-specific key ARNs or using akms:ResourceAliasescondition like theKMSKeyUsageAndPolicystatement does.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/bootstrap/main.tf` around lines 424 - 441, The KMSManagement statement currently grants high-impact actions (ScheduleKeyDeletion, PutKeyPolicy) on resources = ["*"]; split this into two statements: keep kms:CreateKey and kms:ListAliases in a new statement with resources = ["*"] (these require wildcard), and move the remaining actions (e.g., kms:DisableKeyRotation, kms:EnableKeyRotation, kms:GetKeyPolicy, kms:GetKeyRotationStatus, kms:PutKeyPolicy, kms:ScheduleKeyDeletion, kms:TagResource, kms:UntagResource) into a scoped statement that targets project-specific KMS key ARNs or applies a kms:ResourceAliases / condition like the existing KMSKeyUsageAndPolicy statement to restrict allowed keys.
70-82: Checkov CKV_AWS_356: several statements use"*"for restrictable actions.Beyond the ECR/KMS issues flagged above, two additional statements use
resources = ["*"]with partially restrictable actions:
AppAutoscalingManagement(Line 82): Mutative actions likePutScalingPolicy,RegisterScalableTargetare restrictable but the ARN format for app-autoscaling is complex (service namespace dependent).ELBManagement(Line 344): Actions likeCreateLoadBalancer,DeleteLoadBalancersupport resource-level constraints.These are lower priority since ELB resource-level restrictions require knowing the ARN upfront (often not possible for
Create*), and app-autoscaling ARNs are unwieldy. Worth revisiting as a hardening pass.Also applies to: 321-344
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/bootstrap/main.tf` around lines 70 - 82, The IAM policy statements AppAutoscalingManagement and ELBManagement currently use resources = ["*"]; change them to enforce resource-level restrictions where possible by replacing "*" with explicit ARNs for restrictable actions (e.g., for PutScalingPolicy, RegisterScalableTarget use app-autoscaling ARNs built from aws_partition/region/account/id or module outputs) and keep a separate minimal "*" statement only for unavoidable create/delete actions that cannot be constrained (e.g., CreateLoadBalancer). Split each statement into two if needed (one with specific ARNs for mutative, restrictable actions and another limited-wildcard for non-constrainable actions), and use Terraform interpolations/data lookups (account_id, aws_region, aws_partition, resource ids) or variables to construct those ARNs so Checkov CKV_AWS_356 is addressed while preserving necessary functionality.infrastructure/backend/main.tf (1)
50-74: Per-environment HTTPS-only policy looks correct.The
for_eachandeach.keyreferences toaws_s3_bucket.state[each.key].arnare consistent.Consider adding a similar HTTPS-only deny statement to the logs bucket policy (
data.aws_iam_policy_document.logs) for parity — currently only the state buckets enforce HTTPS.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/backend/main.tf` around lines 50 - 74, Add the same per-environment HTTPS-only deny statement used in data.aws_iam_policy_document.state_https_only to the logs policy (data.aws_iam_policy_document.logs): create a statement block in data.aws_iam_policy_document.logs that uses the same for_each iteration (matching the same environment keys), sets actions = ["s3:*"], effect = "Deny", sid = "HTTPSOnly", resources to aws_s3_bucket.logs[each.key].arn and "${aws_s3_bucket.logs[each.key].arn}/*", and the same condition (test = "Bool", variable = "aws:SecureTransport", values = ["false"]) and principals (identifiers = ["*"], type = "AWS") so logs buckets enforce HTTPS parity with state buckets.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/run-ci-cd.yaml:
- Around line 479-482: The workflow currently hardcodes role-external-id
("nest-staging-terraform") which is predictable; replace that value with a
GitHub Actions secret (e.g., use ${{ secrets.AWS_ROLE_EXTERNAL_ID }}) wherever
role-external-id appears (the same block with role-to-assume and the other
similar blocks mentioned), ensure you create a strong randomly generated
external ID and store it in the repository/org secrets, and update the
corresponding IAM role trust policy to require that secret value so the
external-id check remains effective.
- Around line 687-689: The CI job currently runs a raw "Terraform Apply" step
(working-directory: infrastructure/bootstrap) with "terraform apply
-auto-approve" which applies IAM changes without producing or surfacing a plan;
update the job to first run "terraform plan -out=tfplan" and append "terraform
show -no-color tfplan" to $GITHUB_STEP_SUMMARY (so the plan diff is visible in
the job summary), then change the apply command to "terraform apply
-auto-approve tfplan" to ensure the exact reviewed plan is applied.
In @.gitignore:
- Around line 41-44: The .gitignore changes removed coverage for legacy
artifacts named like nest-backend-*, so re-add ignore patterns to prevent
accidental tracking: include entries matching backend/*nest-backend-*.tar.gz and
backend/*nest-backend-*.zip (mirror the existing backend/*nest-dev*.tar.gz
lines) and then verify no such files remain locally or in CI by running a
filesystem check (e.g., find backend/ for "*nest-backend-*.tar.gz" and "*.zip")
before merging.
In `@infrastructure/backend/main.tf`:
- Around line 114-160: The lifecycle rule in
aws_s3_bucket_lifecycle_configuration.logs (rule id "expire-logs") only sets
expiration for current objects and doesn't clean up versioned objects; update
that lifecycle rule to include a noncurrent_version_expiration block (e.g.,
noncurrent_version_expiration { days = var.expire_noncurrent_version_days }) so
noncurrent versions are deleted after a defined retention, and ensure you add or
reuse an appropriate variable (such as var.expire_noncurrent_version_days) for
the retention period rather than leaving it hardcoded.
In `@infrastructure/bootstrap/main.tf`:
- Around line 277-293: The ECSOrchestration IAM statement currently uses
unscoped ARNs like "${var.project_name}-*" which allows cross-environment
access; update each ARN in the statement with sid "ECSOrchestration" to include
the environment key by replacing "${var.project_name}-*" with
"${var.project_name}-${each.key}-*" (e.g., task-definition, service, cluster,
task ARNs) so the role is scoped per environment consistent with
ECSClusterManagement and ECSServiceManagement.
- Around line 209-225: The "ECRAuth" policy statement currently mixes the
resource-global action ecr:GetAuthorizationToken (which requires resources =
["*"]) with repository-scoped data-plane actions, effectively granting those
data-plane actions on all repos; split this into two statements: keep a new
statement with sid "ECRAuth" (or similar) that contains only
"ecr:GetAuthorizationToken" and resources = ["*"], and move the remaining
actions (BatchCheckLayerAvailability, BatchGetImage, CompleteLayerUpload,
GetDownloadUrlForLayer, GetRepositoryPolicy, InitiateLayerUpload, ListImages,
PutImage, UploadLayerPart) into the existing ECRManagement scoped statement so
they use repository ARNs instead of "*". Ensure the original ECRAuth actions
list and the ECRManagement block are updated accordingly.
In `@infrastructure/README.md`:
- Around line 82-84: Clarify which bucket to use by updating the README text to
instruct operators to use the specific Terraform output for the bootstrap
environment (for example, the backend module output name like
outputs.state_bucket_name["bootstrap"]) when filling terraform.tfbackend;
mention the exact output key (state_bucket_name["bootstrap"]) and show it as the
value to copy so users pick the per-environment bootstrap bucket rather than a
generic name.
In `@infrastructure/staging/README.md`:
- Around line 32-38: The IAM policy statement with "Sid": "KMSStateManagement"
currently grants only "kms:Decrypt", which prevents S3 SSE-KMS writes; update
the policy for the KMSStateManagement statement (the Action array) to include
"kms:GenerateDataKey" (and optionally "kms:Encrypt" if you prefer explicit
encrypt permissions) so Terraform/state uploads can generate data keys and
succeed.
---
Nitpick comments:
In @.github/workflows/run-ci-cd.yaml:
- Around line 701-704: The job plan-staging-nest currently lists
scan-staging-images in its needs array even though bootstrap-staging-nest
already depends on scan-staging-images, so remove scan-staging-images from the
needs of plan-staging-nest to avoid redundant noise; edit the needs block for
plan-staging-nest to only include bootstrap-staging-nest and set-release-version
(retain set-release-version) and keep bootstrap-staging-nest as-is so the
dependency on scan-staging-images remains transitively satisfied.
In @.gitignore:
- Around line 41-44: Add ignore patterns for production Zappa artifacts
analogous to the existing dev and staging entries: include patterns like
"backend/*nest-prod*.tar.gz", "backend/*nest-prod*.zip",
"backend/*nest-production*.tar.gz", and "backend/*nest-production*.zip" so
future nest-prod or nest-production builds are auto-ignored alongside the
existing backend/*nest-dev* and backend/*nest-staging* entries.
In `@infrastructure/backend/main.tf`:
- Around line 50-74: Add the same per-environment HTTPS-only deny statement used
in data.aws_iam_policy_document.state_https_only to the logs policy
(data.aws_iam_policy_document.logs): create a statement block in
data.aws_iam_policy_document.logs that uses the same for_each iteration
(matching the same environment keys), sets actions = ["s3:*"], effect = "Deny",
sid = "HTTPSOnly", resources to aws_s3_bucket.logs[each.key].arn and
"${aws_s3_bucket.logs[each.key].arn}/*", and the same condition (test = "Bool",
variable = "aws:SecureTransport", values = ["false"]) and principals
(identifiers = ["*"], type = "AWS") so logs buckets enforce HTTPS parity with
state buckets.
In `@infrastructure/backend/README.md`:
- Around line 64-86: The IAM policy statement with "Sid": "KMSManagement"
currently includes "kms:DisableKeyRotation" in its "Action" array; remove
"kms:DisableKeyRotation" from that array (leaving "kms:EnableKeyRotation" and
the other KMS actions intact) so the policy no longer grants the ability to
disable key rotation for the resource ${AWS_BACKEND_KMS_KEY_ARN}.
In `@infrastructure/bootstrap/main.tf`:
- Around line 424-441: The KMSManagement statement currently grants high-impact
actions (ScheduleKeyDeletion, PutKeyPolicy) on resources = ["*"]; split this
into two statements: keep kms:CreateKey and kms:ListAliases in a new statement
with resources = ["*"] (these require wildcard), and move the remaining actions
(e.g., kms:DisableKeyRotation, kms:EnableKeyRotation, kms:GetKeyPolicy,
kms:GetKeyRotationStatus, kms:PutKeyPolicy, kms:ScheduleKeyDeletion,
kms:TagResource, kms:UntagResource) into a scoped statement that targets
project-specific KMS key ARNs or applies a kms:ResourceAliases / condition like
the existing KMSKeyUsageAndPolicy statement to restrict allowed keys.
- Around line 70-82: The IAM policy statements AppAutoscalingManagement and
ELBManagement currently use resources = ["*"]; change them to enforce
resource-level restrictions where possible by replacing "*" with explicit ARNs
for restrictable actions (e.g., for PutScalingPolicy, RegisterScalableTarget use
app-autoscaling ARNs built from aws_partition/region/account/id or module
outputs) and keep a separate minimal "*" statement only for unavoidable
create/delete actions that cannot be constrained (e.g., CreateLoadBalancer).
Split each statement into two if needed (one with specific ARNs for mutative,
restrictable actions and another limited-wildcard for non-constrainable
actions), and use Terraform interpolations/data lookups (account_id, aws_region,
aws_partition, resource ids) or variables to construct those ARNs so Checkov
CKV_AWS_356 is addressed while preserving necessary functionality.
In `@infrastructure/bootstrap/terraform.tfbackend.example`:
- Around line 1-3: Add the missing encrypt flag to the Terraform S3 backend
example by adding encrypt = true to the backend configuration (alongside bucket,
dynamodb_table, and region) so the backend explicitly requests server-side
encryption for state files for defense-in-depth and consistency with other
examples.
In `@infrastructure/bootstrap/variables.tf`:
- Around line 7-11: The variable "environments" lacks input validation so typos
can create incorrect IAM resources; add a Terraform validation block to the
variable "environments" that checks every item is a member of your allowed
environments set (the same allowed list used by state_environments in
infrastructure/backend/variables.tf) using an expression like
all(var.environments, contains(local.allowed_environments, ...)) and provide a
clear error_message describing allowed values; update the "environments"
variable declaration to include that validation and reference the same allowed
list to keep both variables consistent.
In `@infrastructure/README.md`:
- Around line 58-68: Remove the redundant touch commands that precede the
redirection-based file creation (e.g., the "touch terraform.tfvars" before "cat
terraform.tfvars.example > terraform.tfvars") — keep only the cat redirection
lines to create/populate files; apply the same change for the repeated pattern
(the similar touch/cat pair in the local section and the staging section) so the
README uses only "cat {source} > {dest}" without prior touch.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feature/nest-zappa-migration #3996 +/- ##
=============================================================
Coverage 93.32% 93.32%
=============================================================
Files 513 513
Lines 15827 15827
Branches 2178 2134 -44
=============================================================
Hits 14770 14770
Misses 695 695
Partials 362 362
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="infrastructure/bootstrap/terraform.tfvars.example">
<violation number="1" location="infrastructure/bootstrap/terraform.tfvars.example:2">
P2: This tfvars example won’t parse: `${AWS_ROLE_EXTERNAL_ID}` is interpolation syntax and must be inside quotes or replaced with a literal placeholder. As written, Terraform will error when reading the file.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
infrastructure/bootstrap/main.tf (2)
302-326:ELBManagementgrants mutating actions on all ELB resources across all environments.Unlike
ECSClusterManagement,ECRManagement, and other environment-scoped statements,ELBManagementusesresources = ["*"]for actions likeCreateLoadBalancer,DeleteLoadBalancer,DeleteTargetGroup, etc. These actions support resource-level ARN restrictions. A staging role could modify or delete load balancers belonging to other environments.Consider scoping to environment-specific ARNs consistent with the rest of the policy, e.g.:
resources = [ "arn:aws:elasticloadbalancing:${var.aws_region}:${data.aws_caller_identity.current.account_id}:loadbalancer/app/${var.project_name}-${each.key}-*/*", "arn:aws:elasticloadbalancing:${var.aws_region}:${data.aws_caller_identity.current.account_id}:targetgroup/${var.project_name}-${each.key}-*/*", "arn:aws:elasticloadbalancing:${var.aws_region}:${data.aws_caller_identity.current.account_id}:listener/app/${var.project_name}-${each.key}-*/*", "arn:aws:elasticloadbalancing:${var.aws_region}:${data.aws_caller_identity.current.account_id}:listener-rule/app/${var.project_name}-${each.key}-*/*", ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/bootstrap/main.tf` around lines 302 - 326, The ELBManagement IAM statement currently uses resources = ["*"] giving broad mutating rights; replace the wildcard with environment-scoped ELB ARNs (instead of "*") so only load balancers, target groups, listeners and listener-rules for the given environment can be modified—use patterns built from var.aws_region, data.aws_caller_identity.current.account_id, var.project_name and each.key (e.g., loadbalancer/app/..., targetgroup/..., listener/app/..., listener-rule/...) to mirror the scoping used by ECSClusterManagement/ECRManagement; update the ELBManagement statement's resources array accordingly so actions like CreateLoadBalancer, DeleteLoadBalancer, DeleteTargetGroup, CreateListener, and related actions are limited to those ARNs.
139-187:EC2Managementgrants broad mutating permissions on all EC2 resources.This statement allows creating/deleting VPCs, subnets, security groups, NAT gateways, etc. across the entire account without environment scoping. While many EC2 actions don't support resource-level permissions, some do (e.g., security group, subnet, and VPC operations). If cross-environment isolation is a concern, consider adding tag-based conditions (
aws:RequestTag/aws:ResourceTag) to limit mutations to environment-tagged resources.This is noted as informational given the complexity of EC2 resource-level permissions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/bootstrap/main.tf` around lines 139 - 187, The EC2Management IAM statement grants broad mutating EC2 permissions on all resources (resources = ["*"])—narrow this by adding tag-based conditions so mutations are limited to environment-scoped resources: update the statement with a Condition using aws:RequestTag/environment and aws:ResourceTag/environment (e.g., StringEquals -> "environment": "dev" or a variable) and where possible replace resources="*" with the appropriate ARNs for actions that support resource-level scoping; target the statement identified by sid "EC2Management" when applying these changes.infrastructure/backend/main.tf (1)
146-155: Logs bucket uses S3-managed encryption (AES256) while state lock uses KMS — document the rationale.The DynamoDB state lock table (line 99) uses a customer-managed KMS key, but the logs bucket (line 152) and state buckets (line 244) use
AES256(S3-managed SSE). The#trivy:ignoresuppression indicates this is intentional. Consider adding a brief comment explaining the choice (e.g., cost or because logs/state don't contain sensitive data beyond what's in the state file), so future maintainers understand the divergence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/backend/main.tf` around lines 146 - 155, Add a short inline comment above the aws_s3_bucket_server_side_encryption_configuration "logs" resource explaining why SSE is set to AES256 (S3-managed) while the DynamoDB state lock uses a customer-managed KMS key: note the intentional decision (referencing aws_s3_bucket_server_side_encryption_configuration.logs and the DynamoDB state lock KMS usage) and briefly state the rationale (cost, lack of sensitive data in logs/state, or reduced key management burden) and reference the `#trivy`:ignore tag so future maintainers understand this divergence..github/workflows/run-ci-cd.yaml (1)
703-706:scan-staging-imagesis a redundant direct dependency inplan-staging-nest.
bootstrap-staging-nestalready declaresscan-staging-imagesas its own dependency, soplan-staging-nesttransitively waits for it. The explicit entry is harmless but adds noise to the DAG.♻️ Suggested simplification
needs: - bootstrap-staging-nest - - scan-staging-images - set-release-version🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/run-ci-cd.yaml around lines 703 - 706, Remove the redundant direct dependency on scan-staging-images from the needs list of the plan-staging-nest job; since bootstrap-staging-nest already declares scan-staging-images as a dependency, keep needs: - bootstrap-staging-nest and - set-release-version (or other required entries) but delete the explicit - scan-staging-images entry to simplify the DAG while preserving correctness.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/run-ci-cd.yaml:
- Around line 821-824: The role-duration-seconds setting for the GitHub Actions
jobs (notably the deploy-staging-nest job’s role-duration-seconds currently set
to 1200) is too short and causes STS credentials to expire mid-run; update
role-duration-seconds for deploy-staging-nest (and mirror the change for
build-staging-images and plan-staging-nest) to at least 3600 (or to the maximum
allowed by the nest-staging-terraform role) so the assumed-role session spans
the full CI job runtime; ensure you change the role-duration-seconds key value
for those job blocks (deploy-staging-nest, build-staging-images,
plan-staging-nest).
In `@infrastructure/bootstrap/main.tf`:
- Around line 211-212: The IAM policy actions list contains a duplicated action
string "ecr:GetRepositoryPolicy"; remove the redundant entry so each action
appears only once (locate the actions array that includes
"ecr:GetRepositoryPolicy" and delete the duplicate occurrence).
---
Duplicate comments:
In @.github/workflows/run-ci-cd.yaml:
- Around line 689-691: The "Terraform Apply" step currently runs "terraform
apply -auto-approve" with no preceding plan; change the workflow to first run a
"Terraform Plan" step in the same working-directory ("infrastructure/bootstrap")
that runs "terraform init" then "terraform plan -out=tfplan" and saves/exports
the plan (e.g., "terraform show -json tfplan > plan.json"), upload that
plan.json as a job artifact and/or print a human-readable plan to the job
summary, and make the existing "Terraform Apply" step consume the saved plan
(e.g., "terraform apply tfplan") instead of applying directly from workspace to
ensure IAM and other diffs are reviewed and surfaced.
In `@infrastructure/bootstrap/main.tf`:
- Around line 258-274: The ECSOrchestration statement (sid "ECSOrchestration")
grants ARNs using "${var.project_name}-*" without environment scoping; update
the resource ARNs to include the environment/stage variable (e.g., incorporate
var.environment or var.stage into the pattern) so they target
"${var.project_name}-${var.environment}-*" (or equivalent) for task-definition,
service, cluster, and task ARNs; ensure you update all four resource entries in
that statement block so policies are environment-scoped and maintain the
existing region/account interpolation.
---
Nitpick comments:
In @.github/workflows/run-ci-cd.yaml:
- Around line 703-706: Remove the redundant direct dependency on
scan-staging-images from the needs list of the plan-staging-nest job; since
bootstrap-staging-nest already declares scan-staging-images as a dependency,
keep needs: - bootstrap-staging-nest and - set-release-version (or other
required entries) but delete the explicit - scan-staging-images entry to
simplify the DAG while preserving correctness.
In `@infrastructure/backend/main.tf`:
- Around line 146-155: Add a short inline comment above the
aws_s3_bucket_server_side_encryption_configuration "logs" resource explaining
why SSE is set to AES256 (S3-managed) while the DynamoDB state lock uses a
customer-managed KMS key: note the intentional decision (referencing
aws_s3_bucket_server_side_encryption_configuration.logs and the DynamoDB state
lock KMS usage) and briefly state the rationale (cost, lack of sensitive data in
logs/state, or reduced key management burden) and reference the `#trivy`:ignore
tag so future maintainers understand this divergence.
In `@infrastructure/bootstrap/main.tf`:
- Around line 302-326: The ELBManagement IAM statement currently uses resources
= ["*"] giving broad mutating rights; replace the wildcard with
environment-scoped ELB ARNs (instead of "*") so only load balancers, target
groups, listeners and listener-rules for the given environment can be
modified—use patterns built from var.aws_region,
data.aws_caller_identity.current.account_id, var.project_name and each.key
(e.g., loadbalancer/app/..., targetgroup/..., listener/app/...,
listener-rule/...) to mirror the scoping used by
ECSClusterManagement/ECRManagement; update the ELBManagement statement's
resources array accordingly so actions like CreateLoadBalancer,
DeleteLoadBalancer, DeleteTargetGroup, CreateListener, and related actions are
limited to those ARNs.
- Around line 139-187: The EC2Management IAM statement grants broad mutating EC2
permissions on all resources (resources = ["*"])—narrow this by adding tag-based
conditions so mutations are limited to environment-scoped resources: update the
statement with a Condition using aws:RequestTag/environment and
aws:ResourceTag/environment (e.g., StringEquals -> "environment": "dev" or a
variable) and where possible replace resources="*" with the appropriate ARNs for
actions that support resource-level scoping; target the statement identified by
sid "EC2Management" when applying these changes.
rudransh-shrivastava
had a problem deploying
to
staging
GitHub Actions
Failure
rudransh-shrivastava
had a problem deploying
to
staging
GitHub Actions
Failure
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
infrastructure/bootstrap/main.tf (1)
83-84: Checkov CKV_AWS_356 —resources = ["*"]for partially restrictable services.Checkov flags four statements where at least some actions support resource-level ARN restrictions:
Statement Key actions that ARE restrictable AppAutoscalingManagement(line 83)PutScalingPolicy,DeleteScalingPolicycan be scoped to scalable target ARNsEC2Management(line 187)VPC, subnet, security group, route table, NAT gateway, VPC endpoint all have ARN formats ELBManagement(line 324)Load balancer, listener, rule, target group each have elasticloadbalancing:ARN formatsKMSManagement(line 420)PutKeyPolicy,ScheduleKeyDeletion,GetKeyPolicy, etc. can be scoped toarn:aws:kms:...:key/*(onlyCreateKeygenuinely requires*)For the testing environment, this is acceptable. If/when this config is hardened, split
kms:CreateKeyinto its ownresources = ["*"]statement and scope the remaining KMS mutation actions toarn:aws:kms:${var.aws_region}:${data.aws_caller_identity.current.account_id}:key/*; similarly scope ELB management actions to the project's load balancer ARN patterns.Also applies to: 187-188, 324-325, 420-421
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/bootstrap/main.tf` around lines 83 - 84, The policy statements currently using resources = ["*"] (notably the AppAutoscalingManagement, EC2Management, ELBManagement and KMSManagement statements) should be split so only truly global actions keep "*" and all other actions are scoped to resource ARNs; specifically: for KMSManagement move kms:CreateKey into its own statement with resources = ["*"] and scope the remaining KMS actions to arn:aws:kms:${var.aws_region}:${data.aws_caller_identity.current.account_id}:key/*, scope ELBManagement actions to the project's ELB ARNs (elasticloadbalancing:… target-group/listener/loadbalancer ARNs), scope EC2Management actions to VPC/subnet/sg/route/nat/vpc-endpoint ARNs, and scope AppAutoscalingManagement PutScalingPolicy/DeleteScalingPolicy to scalable target ARNs—update the statements (by name) accordingly so only necessary actions remain wildcarded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/run-ci-cd.yaml:
- Around line 444-452: Restore the removed branch guards and test-job
dependencies so staging jobs only run on the canonical main branch and after
tests: re-add the exact if condition "github.repository == 'OWASP/Nest' &&
github.ref == 'refs/heads/main'" to the jobs build-staging-images,
bootstrap-staging-nest, plan-staging-nest, and deploy-staging-nest, and
re-enable the needs list for build-staging-images to include run-backend-tests,
run-frontend-a11y-tests, run-frontend-e2e-tests, run-frontend-unit-tests, and
run-infrastructure-tests so image builds and Terraform applies are gated by
upstream test jobs.
- Around line 503-505: The CI currently stopped pushing Docker Hub tags
(owasp/nest:backend-staging and owasp/nest:frontend-staging) in the
build-staging-images job but scan-staging-images still expects those Docker Hub
names (via BACKEND_IMAGE_NAME=owasp/nest:backend-staging), so either restore the
Docker Hub push tags in build-staging-images (re-add the
owasp/nest:backend-staging and owasp/nest:frontend-staging tags to the tags
block so Docker Hub images are available for scan-staging-images) or change
scan-staging-images to pull from ECR by replacing
BACKEND_IMAGE_NAME/FRONTEND_IMAGE_NAME values with the ECR names and adding AWS
credential setup (aws-actions/configure-aws-credentials) and login-to-ecr steps
in the scan-staging-images job; update references to build-staging-images,
scan-staging-images, BACKEND_IMAGE_NAME and FRONTEND_IMAGE_NAME accordingly.
In `@infrastructure/bootstrap/main.tf`:
- Around line 551-554: The S3 policy's resources array uses a broad wildcard
("${var.project_name}-*") allowing cross-environment access; replace the bucket
ARNs to scope them to the environment key (use
"${var.project_name}-${each.key}-*" and "${var.project_name}-${each.key}-*/*" in
the resources list) so S3Management follows the same per-environment scoping
pattern as other policies (references: resources, var.project_name, each.key);
if there are intentional shared buckets, add those specific ARNs as additional
entries rather than using a widened wildcard.
---
Nitpick comments:
In `@infrastructure/bootstrap/main.tf`:
- Around line 83-84: The policy statements currently using resources = ["*"]
(notably the AppAutoscalingManagement, EC2Management, ELBManagement and
KMSManagement statements) should be split so only truly global actions keep "*"
and all other actions are scoped to resource ARNs; specifically: for
KMSManagement move kms:CreateKey into its own statement with resources = ["*"]
and scope the remaining KMS actions to
arn:aws:kms:${var.aws_region}:${data.aws_caller_identity.current.account_id}:key/*,
scope ELBManagement actions to the project's ELB ARNs (elasticloadbalancing:…
target-group/listener/loadbalancer ARNs), scope EC2Management actions to
VPC/subnet/sg/route/nat/vpc-endpoint ARNs, and scope AppAutoscalingManagement
PutScalingPolicy/DeleteScalingPolicy to scalable target ARNs—update the
statements (by name) accordingly so only necessary actions remain wildcarded.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
infrastructure/bootstrap/main.tf (1)
244-255: Optional: scopeecs:TagResourceinstead of granting it on*inECSGlobal.
ecs:RegisterTaskDefinitionandecs:DeregisterTaskDefinitiongenuinely require*, butecs:TagResourcesupports resource-level restrictions. Keeping it inECSGlobalalso makes the scopedecs:TagResourceentry inECSClusterManagement(line 237) redundant and allows tagging any ECS resource in the account.Consider moving it to the per-environment scoped blocks instead:
♻️ Proposed refactor
statement { sid = "ECSGlobal" effect = "Allow" actions = [ "ecs:DeregisterTaskDefinition", "ecs:ListClusters", "ecs:ListTaskDefinitions", "ecs:RegisterTaskDefinition", - "ecs:TagResource", ] resources = ["*"] }Then add
ecs:TagResourcefor task-definition ARNs in theECSTaskDefinitionblock (line 287) and keep it inECSClusterManagement/ECSServiceManagementas already scoped.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/bootstrap/main.tf` around lines 244 - 255, The ECSGlobal policy block currently grants ecs:TagResource on "*" which is overly broad; remove ecs:TagResource from the ECSGlobal statement and instead add scoped ecs:TagResource permissions to the per-environment blocks: add ecs:TagResource for task definition ARNs in the ECSTaskDefinition policy (so task-definition tagging is limited to arn:aws:ecs:...:task-definition/*), and ensure ECSClusterManagement and ECSServiceManagement keep their existing scoped ecs:TagResource entries—this eliminates the redundant global tag permission while preserving required broad permissions for ecs:RegisterTaskDefinition and ecs:DeregisterTaskDefinition in ECSGlobal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/run-ci-cd.yaml:
- Around line 694-700: The variable declaration for aws_role_external_id in
variables.tf is missing sensitive = true; update the aws_role_external_id
variable block to include sensitive = true so Terraform redacts its value in
plan output (modify the aws_role_external_id variable declaration in
variables.tf to add the sensitive attribute and re-run the plan to confirm the
value is redacted).
---
Duplicate comments:
In @.github/workflows/run-ci-cd.yaml:
- Around line 444-452: Several staging CI jobs have their conditional `if:`
gates and upstream `needs:` dependencies commented out (e.g., the commented
`#if:` block and `needs:` list around the staging job definitions); restore the
guarded execution by uncommenting the `if:` condition and the `needs:` entries
so these jobs run only when the repository/ref matches and after upstream test
jobs complete—specifically remove the leading "#" from the `if:` line and the
required `needs:` lines in the same job blocks (the blocks shown plus the other
occurrences noted at the ranges referenced) so the workflow validates upstream
gates and dependencies.
- Around line 604-615: The workflow step is using old Docker Hub image names;
update the invocations of the Make targets (security-scan-backend-image,
security-scan-frontend-image, sbom-backend-image, sbom-frontend-image) to pass
the correct, currently published image names (or CI image variables) instead of
BACKEND_IMAGE_NAME=owasp/nest:backend-staging and
FRONTEND_IMAGE_NAME=owasp/nest:frontend-staging; change those arguments to the
new registry/image tags or to existing CI variables (e.g., use your build
artifact variables or GHCR/ECR names) so the Make targets reference images that
are actually pushed.
In `@infrastructure/bootstrap/main.tf`:
- Around line 257-273: The IAM policy statement with sid "ECSOrchestration" uses
generic resource ARNs referencing "${var.project_name}-*" which doesn't include
the per-item key; update each ARN in the resources block to interpolate the loop
key (e.g., include "${each.key}" alongside "${var.project_name}") so ARNs are
scoped per item when this module is iterated (adjust the task-definition,
service, cluster, and task ARNs to include ${each.key} where appropriate).
- Around line 551-554: The S3Management IAM policy still grants
cross-environment access by using "arn:aws:s3:::${var.project_name}-*" and
should be limited to the current environment; update the resources array in the
S3Management policy (the entries currently using ${var.project_name}-*) to
reference the environment-scoped bucket naming (e.g., include var.env or
var.deployment_stage so ARNs become ${var.project_name}-${var.env}-* and
${var.project_name}-${var.env}-*/*) or otherwise use the exact bucket
names/variables for this deployment, removing the broad wildcard that allows
cross-environment access.
---
Nitpick comments:
In `@infrastructure/bootstrap/main.tf`:
- Around line 244-255: The ECSGlobal policy block currently grants
ecs:TagResource on "*" which is overly broad; remove ecs:TagResource from the
ECSGlobal statement and instead add scoped ecs:TagResource permissions to the
per-environment blocks: add ecs:TagResource for task definition ARNs in the
ECSTaskDefinition policy (so task-definition tagging is limited to
arn:aws:ecs:...:task-definition/*), and ensure ECSClusterManagement and
ECSServiceManagement keep their existing scoped ecs:TagResource entries—this
eliminates the redundant global tag permission while preserving required broad
permissions for ecs:RegisterTaskDefinition and ecs:DeregisterTaskDefinition in
ECSGlobal.
Contributor
There was a problem hiding this comment.
3 issues found across 8 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="infrastructure/staging/README.md">
<violation number="1" location="infrastructure/staging/README.md:18">
P2: Use the specific Terraform state bucket placeholder instead of a wildcard prefix so the policy only grants access to the actual state bucket.</violation>
<violation number="2" location="infrastructure/staging/README.md:30">
P2: Keep the DynamoDB ARN parameterized for region and table name so the IAM policy matches the configured Terraform backend settings.</violation>
</file>
<file name="infrastructure/bootstrap/main.tf">
<violation number="1" location="infrastructure/bootstrap/main.tf:553">
P2: Object ARN pattern is broader than the bucket ARN and grants access to buckets outside the intended `${var.project_name}-${each.key}-*` scope. Align the object ARN with the bucket naming pattern to keep least-privilege.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
infrastructure/bootstrap/main.tf (3)
26-53:ec2:DescribeFlowLogsandec2:DescribeNetworkAclsare redundant withec2:Describe*.Lines 34–35 explicitly list
ec2:DescribeFlowLogsandec2:DescribeNetworkAcls, butec2:Describe*on line 33 already covers all EC2 Describe actions. These duplicates are harmless but add noise.Proposed cleanup
actions = [ "acm:DescribeCertificate", "application-autoscaling:DescribeScalableTargets", "application-autoscaling:DescribeScalingPolicies", "ec2:Describe*", - "ec2:DescribeFlowLogs", - "ec2:DescribeNetworkAcls", "ecr:DescribeRepositories",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/bootstrap/main.tf` around lines 26 - 53, In the IAM policy statement with sid "GlobalDiscovery" remove the redundant actions "ec2:DescribeFlowLogs" and "ec2:DescribeNetworkAcls" from the actions list because they are already covered by "ec2:Describe*"; update the actions array in that statement to eliminate those two explicit entries so the policy is cleaner while keeping "ec2:Describe*" intact.
86-101:CloudWatchLogsManagementgrants write access to all log groups in the region.The resource pattern
arn:aws:logs:${region}:${account}:*allows creating/deleting/tagging any log group in the account, not just environment-scoped ones. Other statements in this policy (ECS, ECR, ElastiCache, etc.) use${each.key}for environment scoping.If your log groups follow a naming convention like
${project_name}-${environment}-*, you could tighten this:resources = ["arn:aws:logs:${var.aws_region}:${data.aws_caller_identity.current.account_id}:log-group:/aws/*/${var.project_name}-${each.key}-*"]However, CloudWatch Log group ARNs can vary based on the source service (Lambda, ECS, VPC Flow Logs all use different prefixes), so this may require multiple ARN patterns. Noting as a hardening opportunity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/bootstrap/main.tf` around lines 86 - 101, The CloudWatchLogsManagement policy currently allows write access to all log groups via the resources pattern "arn:aws:logs:${var.aws_region}:${data.aws_caller_identity.current.account_id}:*"; restrict this by scoping the resource ARNs to your project and environment (use variables like var.project_name and each.key) instead of a global wildcard and add additional ARN patterns if needed for service-specific prefixes (e.g., log-group:/aws/*/...). Update the statement with sid "CloudWatchLogsManagement" to replace the broad resources entry with a tightened list of ARN patterns matching your naming convention (and include multiple patterns to cover Lambda/ECS/VPC Flow Logs where applicable).
301-325:ELBManagementusesresources = ["*"]— consider scoping if ALB naming is consistent.ELBv2 supports resource-level permissions. If your ALBs and target groups follow
${project_name}-${environment}-*, you could scope these. However, given that this infrastructure code is for testing purposes, the current approach is pragmatic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infrastructure/bootstrap/main.tf` around lines 301 - 325, The IAM statement with sid "ELBManagement" currently grants actions to resources = ["*"]; replace this with scoped ARNs for ALBs and target groups using your naming convention (e.g., arn:aws:elasticloadbalancing:${region}:${account}:loadbalancer/${project_name}-${environment}-* and arn:aws:elasticloadbalancing:${region}:${account}:targetgroup/${project_name}-${environment}-*) or build those ARNs from existing vars/locals (project_name, environment, region, account_id) so permissions are resource-scoped; optionally add a boolean var (e.g., allow_wide_elb_permission) to fall back to "*" for test runs and wrap the resources value with a conditional so tests keep current behavior while production is scoped.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/run-ci-cd.yaml:
- Around line 626-637: The bootstrap job ordering creates a first-run
dependency: update the workflow so the bootstrap-staging-nest job (which
creates/updates the nest-staging-terraform role) runs before or in parallel with
build-staging-images rather than after scan-staging-images; specifically, remove
or change the needs dependency chain that makes bootstrap-staging-nest depend on
scan-staging-images and ensure build-staging-images either depends on
bootstrap-staging-nest or both run in the correct order/parallel, or
alternatively add a README note documenting that the nest-staging-terraform role
must be pre-created manually before first pipeline run.
In `@infrastructure/bootstrap/main.tf`:
- Around line 551-554: The S3 object ARN in the resources list is missing the
hyphen after ${each.key}, making the pattern too broad; update the second ARN
string in the resources array from
"arn:aws:s3:::${var.project_name}-${each.key}*/*" to include the hyphen so it
reads "arn:aws:s3:::${var.project_name}-${each.key}-*/*" (refer to the resources
array, the ARN literals and variables var.project_name and each.key).
---
Duplicate comments:
In @.github/workflows/run-ci-cd.yaml:
- Around line 694-700: The CI step "Show plan in summary" exposes sensitive plan
contents; mark the Terraform variable aws_role_external_id as sensitive by
adding sensitive = true to the variable "aws_role_external_id" in
infrastructure/bootstrap/variables.tf and remove or redact the raw terraform
show -no-color tfplan output in the workflow (the step named "Show plan in
summary") — either stop appending the plan to $GITHUB_STEP_SUMMARY or replace
the show with a filtered/exported view that omits assume-role/external_id
details (e.g., produce a sanitized summary or use terraform show -json and strip
the external_id field before writing).
---
Nitpick comments:
In `@infrastructure/bootstrap/main.tf`:
- Around line 26-53: In the IAM policy statement with sid "GlobalDiscovery"
remove the redundant actions "ec2:DescribeFlowLogs" and
"ec2:DescribeNetworkAcls" from the actions list because they are already covered
by "ec2:Describe*"; update the actions array in that statement to eliminate
those two explicit entries so the policy is cleaner while keeping
"ec2:Describe*" intact.
- Around line 86-101: The CloudWatchLogsManagement policy currently allows write
access to all log groups via the resources pattern
"arn:aws:logs:${var.aws_region}:${data.aws_caller_identity.current.account_id}:*";
restrict this by scoping the resource ARNs to your project and environment (use
variables like var.project_name and each.key) instead of a global wildcard and
add additional ARN patterns if needed for service-specific prefixes (e.g.,
log-group:/aws/*/...). Update the statement with sid "CloudWatchLogsManagement"
to replace the broad resources entry with a tightened list of ARN patterns
matching your naming convention (and include multiple patterns to cover
Lambda/ECS/VPC Flow Logs where applicable).
- Around line 301-325: The IAM statement with sid "ELBManagement" currently
grants actions to resources = ["*"]; replace this with scoped ARNs for ALBs and
target groups using your naming convention (e.g.,
arn:aws:elasticloadbalancing:${region}:${account}:loadbalancer/${project_name}-${environment}-*
and
arn:aws:elasticloadbalancing:${region}:${account}:targetgroup/${project_name}-${environment}-*)
or build those ARNs from existing vars/locals (project_name, environment,
region, account_id) so permissions are resource-scoped; optionally add a boolean
var (e.g., allow_wide_elb_permission) to fall back to "*" for test runs and wrap
the resources value with a conditional so tests keep current behavior while
production is scoped.
This reverts commit d43fc1c.
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/run-ci-cd.yaml">
<violation number="1" location=".github/workflows/run-ci-cd.yaml:632">
P1: Commenting out the `if` guard on the staging bootstrap job allows infrastructure bootstrapping to run on any branch/repo trigger. This is risky for staging environments and can apply unreviewed changes from non-main branches. Restore the main-branch guard (and scan dependency if required) so bootstrap only runs in the intended context.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
rudransh-shrivastava
requested review from
arkid15r and
kasya
as code owners
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.
Proposed change
Resolves #3976
Manual Steps
Please read the entire description before starting. Thank you.
Note: The state S3 buckets were renamed to follow the
nest-${environment}-terraform-state-<hash>pattern.Please backup the
stagingstate BEFORE applying any changes:terraform state pull > terraform.tfstate.backupAn easier alternative is to completely destroy
staging/and thenbackend/and starting from scratch.Create
nest-backenduser with the policies defined ininfrastructure/backend/README.md.Apply
backend/changesCreate
nest-bootstrapuser with the policies defined ininfrastructure/bootstrap/README.md.Apply the changes (bootstrap) to create a role
nest-staginguser can assume.Create
nest-staginguser with the policies defined ininfrastructure/README.md.Generate a secret ID / UUID and use it as
external_idwhile assuming above role to use the role locally.For GitHub actions: The same ID needs to be set as
AWS_ROLE_EXTERNAL_IDGitHub secret and ininfrastructure/bootstrap/terraform.tfvars.Restore the state (if required) and apply the changes. Terraform may complain that it can't access the old buckets (nest-fixtures*, nest-zappa-deployments*, etc). You may temporarily add these bucket's ARNs to
bootstrap/main.tfand apply the changes to delete these buckets properly (starting from scratch prevents this step).Added GitHub secrets:
BOOTSTRAP_AWS_ACCESS_KEY_IDBOOTSTRAP_AWS_SECRET_ACCESS_KEYBOOTSTRAP_TF_STATE_BUCKET_NAMEBOOTSTRAP_TF_STATE_DYNAMODB_TABLE_NAMEAWS_ROLE_EXTERNAL_ID- previously generated ID/UUID.Removed GitHub secrets:
ZAPPA_LAMBDA_FUNCTION_NAMEFollowing GitHub secrets need to be updated:
TF_STATE_DYNAMODB_TABLE_NAME- table is renamedTF_STATE_BUCKET_NAME- bucket is renamedChecklist
make check-testlocally: all warnings addressed, tests passed