Improve Security Groups for Infrastructure #2685
Conversation
github-actions
bot
added
docs
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughReplaces Lambda security-group usage with a dedicated ECS security group across Terraform modules, adds ECS/RDS/RDS-proxy/Redis security groups and rules, updates module inputs/outputs and README; also removes a single in-code comment in a backend management command. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
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 |
rudransh-shrivastava
changed the base branch from
main
to
feature/nest-zappa-migration
rudransh-shrivastava
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
backend/docker/Dockerfile (1)
46-47: Symlink workaround is functional but consider refactoring paths in the future.The symlink enables make targets to reference
backend/*paths correctly. This is a pragmatic solution, though a cleaner long-term approach might involve refactoring the Makefile includes to avoid needing the symlink.infrastructure/modules/database/outputs.tf (1)
7-10: Consider renaming output for clarity.The output
db_proxy_endpointnow conditionally returns either the RDS Proxy endpoint or the direct database address. Consider renaming todb_endpointto better reflect its dual purpose and avoid confusion.Apply this diff:
-output "db_proxy_endpoint" { - description = "The endpoint of the RDS proxy" +output "db_endpoint" { + description = "The database endpoint (RDS Proxy if enabled, otherwise direct DB address)" value = var.create_rds_proxy ? aws_db_proxy.main[0].endpoint : aws_db_instance.main.address }infrastructure/README.md (2)
106-109: Document the critical deployment dependency for secrets.The warning about invalid DJANGO_SLACK_BOT_TOKEN causing silent deployment failures is valuable. Consider adding similar warnings for other critical secrets (e.g., DJANGO_SECRET_KEY, DJANGO_ALLOWED_HOSTS) to prevent deployment issues from incomplete parameter setup.
161-167: ECS task security group guidance is correct but could be more explicit.The instruction to "select the ECS security group (e.g.
owasp-nest-staging-ecs-sg)" is correct, but should emphasize that selecting the wrong security group will break database/Redis connectivity for tasks. Consider adding a note: "⚠️ Ensure the ECS security group is selected; using the Lambda security group will cause task failures."infrastructure/modules/ecs/main.tf (1)
209-237: S3 fixture copy with inline AWS CLI installation could be optimized.Installing awscli at task runtime (lines 218-219) adds ~30-60s per execution. Consider pre-installing in the Docker image or using a lighter alternative (e.g., AWS Lambda Web Adapter for S3 access). For now, this is acceptable for infrequent fixture loads but flag for future optimization.
infrastructure/modules/security/main.tf (1)
85-108: Minor: Inconsistent Redis ingress rule definition style.Redis ingress for Lambda is defined inline within the security group (lines 102-107), while Redis ingress for ECS is defined as a separate
security_group_ruleresource (lines 165-172). Both are functionally correct, but using a consistent style (all inline or all separate resources) would improve maintainability.Consider refactoring both to use separate
security_group_ruleresources for consistency:# In the redis security group (remove inline rule): - ingress { - description = "Redis from Lambda" - from_port = var.redis_port - protocol = "tcp" - security_groups = [aws_security_group.lambda.id] - to_port = var.redis_port - }Then both Lambda and ECS Redis access use separate security_group_rule resources.
infrastructure/modules/parameters/variables.tf (1)
35-38: Minor: db_port variable should be type number, not string.Line 36 declares
db_portastype = string, but ports are numeric and should betype = number. This will require conversion downstream when the port is used in SSM parameter values or Docker environment variables.While Terraform will coerce between types, declaring the correct type prevents confusion and enables validation. Consider changing to:
variable "db_port" { description = "The port of the database." - type = string + type = number }Then convert to string when serializing to SSM parameters (e.g.,
tostring(var.db_port)).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
backend/Makefile(1 hunks)backend/apps/common/management/commands/load_data.py(1 hunks)backend/docker/Dockerfile(1 hunks)backend/settings/staging.py(0 hunks)backend/tests/apps/common/management/commands/load_data_test.py(4 hunks)backend/wsgi.py(1 hunks)backend/zappa_settings.example.json(1 hunks)infrastructure/README.md(4 hunks)infrastructure/main.tf(4 hunks)infrastructure/modules/cache/main.tf(2 hunks)infrastructure/modules/cache/variables.tf(0 hunks)infrastructure/modules/database/main.tf(4 hunks)infrastructure/modules/database/outputs.tf(1 hunks)infrastructure/modules/database/variables.tf(2 hunks)infrastructure/modules/ecs/main.tf(11 hunks)infrastructure/modules/ecs/modules/task/main.tf(1 hunks)infrastructure/modules/ecs/modules/task/variables.tf(1 hunks)infrastructure/modules/ecs/variables.tf(6 hunks)infrastructure/modules/parameters/main.tf(1 hunks)infrastructure/modules/parameters/outputs.tf(1 hunks)infrastructure/modules/parameters/variables.tf(1 hunks)infrastructure/modules/security/main.tf(4 hunks)infrastructure/modules/security/outputs.tf(1 hunks)infrastructure/modules/security/variables.tf(1 hunks)infrastructure/outputs.tf(0 hunks)infrastructure/terraform.tfvars.example(1 hunks)infrastructure/variables.tf(2 hunks)
💤 Files with no reviewable changes (3)
- backend/settings/staging.py
- infrastructure/outputs.tf
- infrastructure/modules/cache/variables.tf
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2551
File: infrastructure/modules/parameters/main.tf:1-191
Timestamp: 2025-11-08T11:16:25.725Z
Learning: The parameters module in infrastructure/modules/parameters/ is currently configured for staging environment only. The `configuration` and `settings_module` variables default to "Staging" and "settings.staging" respectively, and users can update parameter values via the AWS Parameter Store console. The lifecycle.ignore_changes blocks on these parameters support manual console updates without Terraform reverting them.
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2551
File: infrastructure/modules/parameters/main.tf:16-26
Timestamp: 2025-11-08T11:43:19.276Z
Learning: KMS CMK encryption for SSM SecureString parameters in infrastructure/modules/parameters/ is planned to be implemented after S3 state management is completed. Currently using AWS-managed keys for the testing infrastructure.
📚 Learning: 2025-10-26T12:50:50.512Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 2429
File: backend/Makefile:30-32
Timestamp: 2025-10-26T12:50:50.512Z
Learning: The `exec-backend-e2e-command` and `exec-db-e2e-command` Makefile targets in the backend/Makefile are intended for local development and debugging only, not for CI/CD execution, so the `-it` flags are appropriate.
Applied to files:
backend/Makefile
📚 Learning: 2025-10-23T19:22:23.811Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2431
File: infrastructure/main.tf:0-0
Timestamp: 2025-10-23T19:22:23.811Z
Learning: In Zappa-based serverless deployments, Lambda functions and IAM execution roles are managed by Zappa at application deployment time (via `zappa deploy`/`zappa update`), not via Terraform. Terraform provisions the supporting infrastructure (VPC, RDS, S3, security groups, RDS Proxy, Secrets Manager), while Zappa handles the Lambda orchestration layer.
Applied to files:
backend/zappa_settings.example.jsoninfrastructure/README.mdinfrastructure/modules/security/main.tf
📚 Learning: 2025-11-08T11:16:25.725Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2551
File: infrastructure/modules/parameters/main.tf:1-191
Timestamp: 2025-11-08T11:16:25.725Z
Learning: The parameters module in infrastructure/modules/parameters/ is currently configured for staging environment only. The `configuration` and `settings_module` variables default to "Staging" and "settings.staging" respectively, and users can update parameter values via the AWS Parameter Store console. The lifecycle.ignore_changes blocks on these parameters support manual console updates without Terraform reverting them.
Applied to files:
backend/zappa_settings.example.jsoninfrastructure/modules/parameters/main.tfinfrastructure/main.tfinfrastructure/modules/parameters/variables.tf
📚 Learning: 2025-10-17T15:25:34.963Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2431
File: infrastructure/modules/cache/main.tf:30-30
Timestamp: 2025-10-17T15:25:34.963Z
Learning: The infrastructure/Terraform code in the OWASP Nest repository under the `infrastructure/` directory is intended for quick testing purposes only, not for production deployment.
Applied to files:
infrastructure/terraform.tfvars.example
📚 Learning: 2025-10-17T15:25:53.713Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2431
File: infrastructure/modules/database/main.tf:22-60
Timestamp: 2025-10-17T15:25:53.713Z
Learning: The infrastructure code in the `infrastructure/` directory is intended for quick testing purposes only, not production-grade deployment. Production-grade security hardening controls (such as IAM database authentication, deletion protection, Performance Insights KMS encryption) are not required for this testing infrastructure.
Applied to files:
infrastructure/terraform.tfvars.exampleinfrastructure/README.md
📚 Learning: 2025-11-08T11:43:19.276Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2551
File: infrastructure/modules/parameters/main.tf:16-26
Timestamp: 2025-11-08T11:43:19.276Z
Learning: KMS CMK encryption for SSM SecureString parameters in infrastructure/modules/parameters/ is planned to be implemented after S3 state management is completed. Currently using AWS-managed keys for the testing infrastructure.
Applied to files:
infrastructure/modules/parameters/main.tf
🧬 Code graph analysis (1)
backend/apps/common/management/commands/load_data.py (3)
backend/apps/github/management/commands/github_update_related_organizations.py (2)
add_arguments(20-32)handle(34-86)backend/apps/common/management/commands/restore_backup.py (1)
handle(13-17)backend/apps/core/utils/index.py (1)
disable_indexing(74-92)
🪛 Checkov (3.2.334)
infrastructure/modules/parameters/main.tf
[high] 16-26: Ensure SSM parameters are using KMS CMK
(CKV_AWS_337)
[high] 28-38: Ensure SSM parameters are using KMS CMK
(CKV_AWS_337)
[high] 80-86: Ensure SSM parameters are using KMS CMK
(CKV_AWS_337)
[high] 104-114: Ensure SSM parameters are using KMS CMK
(CKV_AWS_337)
[high] 124-130: Ensure SSM parameters are using KMS CMK
(CKV_AWS_337)
[high] 132-138: Ensure SSM parameters are using KMS CMK
(CKV_AWS_337)
[high] 152-162: Ensure SSM parameters are using KMS CMK
(CKV_AWS_337)
[high] 164-174: Ensure SSM parameters are using KMS CMK
(CKV_AWS_337)
[high] 176-186: Ensure SSM parameters are using KMS CMK
(CKV_AWS_337)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
🔇 Additional comments (39)
backend/Makefile (1)
24-36: LGTM! Clean conditional execution support.The conditional execution paths for Docker and direct modes are well-implemented and consistent across both targets. This enables flexible command execution for different deployment contexts (local Docker vs. ECS/production).
backend/docker/Dockerfile (2)
41-41: LGTM!The Makefile copy is correctly positioned and necessary to support make target execution within the container.
54-54: LGTM! Proper make installation with cache optimization.The
--no-cacheflag prevents unnecessary cache buildup while installing the required make utility.infrastructure/modules/database/variables.tf (2)
7-11: LGTM! Well-defined conditional feature flag.The
create_rds_proxyvariable with a safe default offalseenables opt-in RDS Proxy provisioning, aligning with the PR's infrastructure improvements.
87-90: Variable rename is complete and properly propagated.The rename from
db_usernametodb_userhas been successfully implemented across the infrastructure. No references to the old variable name remain in the codebase, and all parent module usages ininfrastructure/main.tfcorrectly reference the new variable name.backend/apps/common/management/commands/load_data.py (2)
13-26: LGTM! Well-implemented CLI argument.The
--fixture-pathargument follows Django command conventions and maintains backward compatibility with the default value. The implementation is clean and well-documented.
28-34: LGTM! Clean integration of fixture path parameter.The parameter is correctly extracted from options and passed to the
loaddatacommand while preserving the transactional and indexing behavior.backend/wsgi.py (1)
24-24: LGTM! Correct placement for environment setup.The function is correctly invoked before Django settings initialization, ensuring SSM parameters are available when settings are loaded.
backend/zappa_settings.example.json (1)
4-6: LGTM! SSM Parameter Store path correctly configured.The
AWS_SYSTEMS_MANAGER_PARAM_STORE_PATHenvironment variable aligns with the wsgi.py implementation and uses the correct SSM parameter path format.backend/tests/apps/common/management/commands/load_data_test.py (3)
8-36: LGTM! Clean migration to call_command.The test correctly uses
call_commandand validates the default fixture path behavior with proper mocking of transaction and indexing contexts.
38-68: LGTM! Comprehensive test for custom fixture path.The new test correctly validates that the
--fixture-pathargument is properly passed through to theloaddatacommand, with appropriate mocking of transaction and indexing behavior.
70-94: LGTM! Exception handling test preserved.The test correctly validates that indexing is re-enabled even when
call_commandraises an exception, maintaining proper cleanup behavior.infrastructure/modules/ecs/modules/task/variables.tf (1)
17-21: Variable rename verified and properly implemented across the module hierarchy.The change from
container_environmenttocontainer_parameters_arnsis complete. No references to the old variable name remain, and the new variable is correctly threaded through the module hierarchy (root main.tf → ecs module → task module) and actively used in the task definition's secrets block on line 46, where it iterates over the SSM parameter ARNs map. The security improvement through AWS SSM Parameter Store integration is properly implemented.infrastructure/modules/parameters/main.tf (3)
1-14: LGTM!The Terraform and provider version constraints are appropriate.
16-186: KMS CMK encryption planned for future implementation.The static analysis correctly identifies that SecureString parameters are using AWS-managed keys instead of customer-managed KMS keys. This is acceptable for the current testing infrastructure, with KMS CMK encryption planned for implementation after S3 state management is completed.
Based on learnings.
188-191: LGTM!The random string generation for Django secret key is configured appropriately with sufficient length and special characters enabled.
infrastructure/modules/security/variables.tf (1)
7-11: LGTM!The
create_rds_proxyvariable is well-defined with an appropriate default value offalse, enabling optional RDS Proxy support as intended by the PR objectives.infrastructure/modules/security/outputs.tf (2)
1-4: LGTM!The new
ecs_sg_idoutput correctly exposes the ECS security group ID, aligning with the PR objective to create a separate security group for ECS tasks.
11-14: LGTM!The conditional logic for
rds_proxy_sg_idcorrectly returns the security group ID when RDS Proxy is enabled, ornullotherwise. The array index notation is appropriate for the count-based resource.infrastructure/terraform.tfvars.example (1)
1-10: LGTM!The migration from Django-specific variables to infrastructure configuration variables aligns with the Parameter Store integration. The removed Django variables are now managed through SSM parameters, improving secret management and supporting least-privilege IAM policies as per the PR objectives.
The default values are appropriate for testing infrastructure:
create_rds_proxy = falseprovides optional RDS Proxy supportdb_backup_retention_period = 0is acceptable for testing (not production)force_destroy_bucket = truefacilitates testing cleanupBased on learnings.
infrastructure/modules/cache/main.tf (1)
16-36: LGTM!The change to unconditional Redis auth token generation simplifies the logic and improves security by always requiring authentication. The addition of
special = truewith the existingoverride_specialconfiguration ensures strong token entropy while meeting Redis-specific requirements.infrastructure/modules/parameters/outputs.tf (1)
1-22: LGTM!The
ssm_parameter_arnsoutput provides a clean interface for consuming modules to reference SSM parameters. The map structure with environment variable names as keys aligns well with the ECS secrets configuration pattern.infrastructure/modules/ecs/modules/task/main.tf (1)
46-49: Execution role SSM permissions verified and correctly configured.The infrastructure code properly implements SSM parameter access for ECS tasks. The parent ECS module creates
aws_iam_role.ecs_tasks_execution_rolewith an attached policy grantingssm:GetParametersaction on the resource patternarn:aws:ssm:${var.aws_region}:${data.aws_caller_identity.current.account_id}:parameter/${var.project_name}/${var.environment}/*. This role ARN is passed to all task modules via theecs_tasks_execution_role_arnparameter and used in the task definitions. The wildcard pattern covers environment-specific SSM parameters, supporting the migration from environment variables to secrets.The security improvement is correctly implemented with appropriate IAM permissions in place.
infrastructure/README.md (1)
56-60: Parameter Store setup instructions are clear.The new steps correctly direct users to populate DJANGO_* secrets via AWS Console after terraform apply, which aligns with the SSM-based secrets architecture.
infrastructure/modules/ecs/main.tf (3)
119-138: ECS security group integration is correct; verify cross-module wiring.Task modules correctly reference
var.ecs_sg_idinstead of the previouslambda_sg_id, implementing proper separation of concerns. Ensure thatinfrastructure/main.tfpassesmodule.security.ecs_sg_idto this module and that the security module outputs this ID.
140-167: Multi-line shell command in owasp_update_project_health_metrics_task is well-structured.Using heredoc syntax with
set -eensures early exit on command failure, which is appropriate for critical health update tasks. Verify that both make targets exist in the backend Makefile.
46-61: No changes needed — SSM policy is correct as written.ECS reads SSM parameters referenced via containerDefinition.secrets using ssm:GetParameters (not GetParameter). The policy in lines 46-61 correctly uses
ssm:GetParameterswith appropriate resource ARN scoping for the project/environment parameters. The batch API is the correct and only required action for this use case.infrastructure/modules/ecs/variables.tf (2)
12-21: New variables properly implement SSM-based secrets and separate ECS security group.
container_parameters_arns(map format) correctly replaces inline environment variables, enabling secure parameter injection via ECS secrets block.ecs_sg_idrequired parameter ensures explicit wiring to correct security group.
70-120: Justify memory reductions before merging.Four task memory defaults were reduced from 2048 MiB to 1024 MiB (migrate, sync-data, health metrics, health scores tasks). While this reduces costs, it may impact throughput or cause OOM errors if task workloads increase.
Please clarify:
- Was this change load-tested or is it based on actual observed usage?
- What is the OOM behavior for these tasks if they exceed 1024 MiB?
- Should index_data_task also be reduced from 2048 MiB for consistency?
infrastructure/main.tf (2)
42-70: Module invocations correctly implement conditional RDS Proxy and ECS security group separation.The refactoring properly passes
create_rds_proxyto database and security modules, and wiresecs_sg_idto the ECS module. Variable renames (db_username -> db_user) are consistent across modules.Ensure that:
infrastructure/modules/security/outputs.tfexportsecs_sg_idinfrastructure/modules/cache/outputs.tfexportsredis_auth_token(referenced on line 96)infrastructure/modules/database/outputs.tfexportsdb_proxy_endpointwith conditional null handling
84-97: Parameter module wiring is correctly implemented. Verification confirms:
- The
db_proxy_endpointreference properly handles bothcreate_rds_proxy = true(proxy endpoint) andcreate_rds_proxy = false(direct RDS address) cases via conditional logic in the database module output- All SSM parameter names follow the pattern
/${var.project_name}/${var.environment}/*, matching the ARN pattern required by the ECS SSM policy (arn:aws:ssm:${var.aws_region}:${data.aws_caller_identity.current.account_id}:parameter/${var.project_name}/${var.environment}/*)- The
ssm_parameter_arnsoutput correctly maps parameter names to their ARNs, all of which will satisfy the ECS policy resource constraintinfrastructure/modules/database/main.tf (1)
79-158: Conditional RDS Proxy implementation with count is correct and follows best practices.All proxy resources properly gate creation with
count = var.create_rds_proxy ? 1 : 0, and dependent resources use index-based access (e.g.,aws_iam_role.rds_proxy[0].arn). The IAM policy correctly limits Secrets Manager access to only the db_credentials secret, andrequire_tls = trueon the proxy enforces encryption.Verify that
infrastructure/modules/database/outputs.tfexportsdb_proxy_endpointwith proper null-coalescing logic (e.g.,db_proxy_endpoint = var.create_rds_proxy ? aws_db_proxy.main[0].endpoint : aws_db_instance.main.endpoint) to support both proxy and direct modes in the parameters module.infrastructure/modules/security/main.tf (2)
16-48: ECS and Lambda security groups are now properly separated.The creation of a distinct ECS security group (ecs-sg) separate from Lambda's security group (lambda-sg) correctly implements the PR objective. Both groups have identical egress policies (allow all outbound), which is appropriate for workloads that need outbound access to multiple services.
110-163: Conditional RDS access rules properly implement direct-vs-proxy access patterns.When
create_rds_proxy = false: ECS and Lambda connect directly to RDS (lines 110-130).
Whencreate_rds_proxy = true: Both connect to RDS Proxy instead (lines 132-163), which then connects to RDS.This logic correctly prevents direct RDS access when a proxy is configured, which is a best practice for connection pooling and audit trails.
infrastructure/variables.tf (3)
13-17: New create_rds_proxy variable correctly defaults to false for backward compatibility.The optional RDS Proxy flag allows existing deployments to work without changes while enabling proxy-based configurations for new deployments. Default false is appropriate for incremental adoption.
68-82: Environment validation and db_user rename improve consistency and prevent configuration errors.The validation block (lines 78-80) prevents typos like
environment = "test"from silently creating misnamed resources. Renamingdb_usernametodb_useraligns with AWS RDS terminology and matches the parameters module variable naming.
1-148: All verifications passed—no issues identified.The reference to
module.cache.redis_auth_tokenon line 96 is correctly implemented:
- Cache module properly outputs this value via
infrastructure/modules/cache/outputs.tf- No dangling references to removed
var.django_*variables exist- Django-specific variables have been successfully migrated to SSM Parameter Store via the parameters module
The refactoring correctly redirects secret management away from tfvars (which could be accidentally committed) to SSM Parameter Store.
infrastructure/modules/parameters/variables.tf (2)
13-17: Configuration defaults correctly align with staging environment focus.The
configuration = "Staging"andsettings_module = "settings.staging"defaults match the learned behavior that the parameters module is currently configured for staging only. This is appropriate for the current infrastructure scope.
1-70: Parameter module variables comprehensively cover Django configuration needs.All essential database, cache, and Django settings are captured as inputs. Sensitive fields are properly marked. The module follows good practice of accepting derived values (db_host from RDS Proxy endpoint, redis_host from cache endpoint) rather than requiring hardcoded IPs.
backend/wsgi.py
Outdated
| def _populate_environ_from_ssm(): | ||
| ssm_param_path = os.getenv("AWS_SYSTEMS_MANAGER_PARAM_STORE_PATH") | ||
| if not ssm_param_path: | ||
| return | ||
|
|
||
| from pathlib import Path | ||
|
|
||
| import boto3 | ||
|
|
||
| client = boto3.client("ssm") | ||
| paginator = client.get_paginator("get_parameters_by_path") | ||
| response_iterator = paginator.paginate(Path=ssm_param_path, WithDecryption=True) | ||
|
|
||
| for page in response_iterator: | ||
| for param in page["Parameters"]: | ||
| os.environ[Path(param["Name"]).name] = param["Value"] | ||
|
|
There was a problem hiding this comment.
Add error handling and clarify boto3 region configuration.
The SSM parameter retrieval lacks error handling, which could cause application startup failures if AWS API calls fail. Additionally, the boto3 client is created without an explicit region, relying on the default AWS SDK resolution chain (environment variables, credentials file, or EC2 instance metadata).
Consider adding error handling and region specification:
def _populate_environ_from_ssm():
ssm_param_path = os.getenv("AWS_SYSTEMS_MANAGER_PARAM_STORE_PATH")
if not ssm_param_path:
return
from pathlib import Path
import boto3
- client = boto3.client("ssm")
+ # Region will be resolved from AWS_DEFAULT_REGION env var or EC2 instance metadata
+ try:
+ client = boto3.client("ssm")
+ except Exception as e:
+ # Log but don't crash on boto3 initialization failure
+ print(f"Warning: Failed to initialize SSM client: {e}")
+ return
+
paginator = client.get_paginator("get_parameters_by_path")
- response_iterator = paginator.paginate(Path=ssm_param_path, WithDecryption=True)
+ try:
+ response_iterator = paginator.paginate(Path=ssm_param_path, WithDecryption=True)
+ except Exception as e:
+ # Log but don't crash on SSM API failure
+ print(f"Warning: Failed to fetch SSM parameters from {ssm_param_path}: {e}")
+ return
for page in response_iterator:
for param in page["Parameters"]:
os.environ[Path(param["Name"]).name] = param["Value"]🤖 Prompt for AI Agents
In backend/wsgi.py around lines 6 to 22, the SSM parameter fetch lacks error
handling and doesn't set a boto3 region; wrap boto3 client creation and
pagination in a try/except, determine region_name from os.getenv("AWS_REGION")
or os.getenv("AWS_DEFAULT_REGION") (fallback to boto3's default if neither
present) and pass it into boto3.client("ssm", region_name=region_name), catch
and handle relevant exceptions (botocore.exceptions.ClientError, NoRegionError,
EndpointConnectionError, generic Exception), log a clear error message with
exception details via the existing logger, and on failure either return early
(so startup continues) or re-raise only if this must be fatal; also validate
each parameter before writing into os.environ (skip or log malformed names) to
avoid crashing on unexpected API responses.
| "extra_permissions": [ | ||
| { | ||
| "Effect": "Allow", | ||
| "Action": [ | ||
| "ssm:GetParametersByPath", | ||
| "ssm:GetParameter" | ||
| ], | ||
| "Resource": [ | ||
| "arn:aws:ssm:${AWS_REGION}:${AWS_ACCOUNT_ID}:parameter/owasp-nest/staging", | ||
| "arn:aws:ssm:${AWS_REGION}:${AWS_ACCOUNT_ID}:parameter/owasp-nest/staging/*" | ||
| ] | ||
| } | ||
| ], |
There was a problem hiding this comment.
🧩 Analysis chain
Verify Zappa supports environment variable placeholder substitution in extra_permissions.
The IAM permissions correctly grant least-privilege SSM access, but the ARN Resource strings use ${AWS_REGION} and ${AWS_ACCOUNT_ID} placeholders. Ensure Zappa substitutes these at deployment time, as raw placeholders in IAM policy resources will not match actual ARN patterns.
🌐 Web query:
Does Zappa support ${AWS_REGION} and ${AWS_ACCOUNT_ID} placeholder substitution in extra_permissions IAM resource ARNs?
💡 Result:
Short answer: No — Zappa does not perform substitution of ${AWS_REGION} or ${AWS_ACCOUNT_ID} inside extra_permissions ARNs. Use concrete ARNs (or generate them yourself) — the docs only show example placeholders like and <account_id> in snippets, they are not automatic runtime substitutions. [1][2]
Sources:
- Zappa README / settings docs (shows example ARN placeholders like arn:aws:lambda::<account_id>:function:...) — examples only. [1]
- PyPI Zappa docs / extra_permissions example. [2]
Remove placeholder syntax from IAM Resource ARNs — Zappa does not substitute ${AWS_REGION} and ${AWS_ACCOUNT_ID}.
Zappa does not perform substitution of ${AWS_REGION} or ${AWS_ACCOUNT_ID} inside extra_permissions ARNs. The documentation examples showing these placeholders are illustrative only. Lines 16-17 will be interpreted as literal strings, causing the IAM policy to fail silently — the Lambda execution role will not actually grant SSM parameter access.
Replace with concrete ARN values, environment-specific configuration, or generate ARNs dynamically outside of the Zappa settings file.
🤖 Prompt for AI Agents
In backend/zappa_settings.example.json around lines 8 to 20, the IAM resource
ARNs use placeholder syntax (${AWS_REGION}, ${AWS_ACCOUNT_ID}) that Zappa will
not substitute, so the policy will be treated as literal strings and fail to
grant SSM access; fix by replacing those placeholder ARNs with concrete ARNs
(e.g., arn:aws:ssm:<region>:<account-id>:parameter/owasp-nest/staging and
arn:aws:ssm:<region>:<account-id>:parameter/owasp-nest/staging/*), or instead
remove hardcoded ARNs and generate/populate correct ARNs from your deployment
pipeline or environment-specific config (CloudFormation/CI variables) before
writing zappa_settings, ensuring the final file contains real region and account
values rather than ${...} placeholders.
| variable "allowed_hosts" { | ||
| description = "The Django allowed hosts." | ||
| type = string | ||
| default = "*" | ||
| } |
There was a problem hiding this comment.
Setting allowed_hosts to "*" disables Host header validation in Django, which is vulnerable to Host-header attacks and cache poisoning. This default is acceptable for development/staging but dangerous for production.
Recommend:
- Change default to an empty string or require explicit specification
- Add validation warning in documentation
- For production deployments, override with specific hostnames
variable "allowed_hosts" {
description = "The Django allowed hosts."
type = string
- default = "*"
+ default = ""
}Add a comment warning users to configure this properly for production.
📝 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.
| variable "allowed_hosts" { | |
| description = "The Django allowed hosts." | |
| type = string | |
| default = "*" | |
| } | |
| variable "allowed_hosts" { | |
| description = "The Django allowed hosts." | |
| type = string | |
| default = "" | |
| } |
🤖 Prompt for AI Agents
in infrastructure/modules/parameters/variables.tf lines 1-5, the variable
"allowed_hosts" currently defaults to "*" which is unsafe for production; update
the variable to require explicit configuration (remove the "*" default or change
default to an empty string) and add an inline comment above the variable warning
that "*" disables Django host header validation and must be overridden with
explicit hostnames in production; also ensure documentation/README is updated to
call out this requirement and suggest production override examples.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/wsgi.py (1)
3-23: Add error handling, logging, and explicit region handling around SSM access.As written, any boto3/SSM issue (missing region/credentials, network, IAM denial, malformed response) will raise during module import and take down WSGI startup, with no logging for diagnosis; the region is also implicit. Wrapping client creation and pagination in targeted try/except, resolving region from environment, and logging failures would make this far more robust.
A concrete direction:
-import os -from pathlib import Path - -import boto3 - - -def populate_environ_from_ssm(): - """Populate environment variables from AWS Systems Manager Parameter Store.""" - if not (ssm_param_path := os.getenv("AWS_SYSTEMS_MANAGER_PARAM_STORE_PATH")): - return - - client = boto3.client("ssm") - paginator = client.get_paginator("get_parameters_by_path") - response_iterator = paginator.paginate(Path=ssm_param_path, WithDecryption=True) - - for page in response_iterator: - for param in page["Parameters"]: - os.environ[Path(param["Name"]).name] = param["Value"] +import os +from pathlib import Path +import logging + +import boto3 +from botocore.exceptions import ClientError, NoRegionError, EndpointConnectionError + +logger = logging.getLogger(__name__) + + +def populate_environ_from_ssm(): + """Populate environment variables from AWS Systems Manager Parameter Store.""" + ssm_param_path = os.getenv("AWS_SYSTEMS_MANAGER_PARAM_STORE_PATH") + if not ssm_param_path: + return + + region_name = os.getenv("AWS_REGION") or os.getenv("AWS_DEFAULT_REGION") or None + + try: + client = boto3.client("ssm", region_name=region_name) if region_name else boto3.client("ssm") + paginator = client.get_paginator("get_parameters_by_path") + response_iterator = paginator.paginate(Path=ssm_param_path, WithDecryption=True) + except (NoRegionError, EndpointConnectionError, ClientError) as exc: + logger.error("Failed to initialize or call SSM for path %s: %s", ssm_param_path, exc) + # Decide if this should be fatal for your deployment; currently we fail closed: + raise + except Exception as exc: # defensive catch‑all + logger.error("Unexpected error while preparing SSM client for path %s: %s", ssm_param_path, exc) + raise + + for page in response_iterator: + for param in page.get("Parameters", []): + name = param.get("Name") + value = param.get("Value") + if not name or value is None: + logger.warning("Skipping malformed SSM parameter entry in path %s: %r", ssm_param_path, param) + continue + os.environ[Path(name).name] = valueIf you prefer non‑fatal behavior in some environments, you can
returninstead ofraisein the exception handlers; just be explicit about that policy.
🧹 Nitpick comments (1)
backend/wsgi.py (1)
23-30: Late import ofget_wsgi_applicationwith# noqa: E402is appropriate.Importing
get_wsgi_applicationonly after SSM‑backed env population and Django settings defaults is correct here; the# noqa: E402suppression is justified and keeps lint noise down.If you later add more WSGI‑time initialization, consider grouping it into a small helper (e.g.,
_bootstrap_settings_from_env_and_ssm()) above this import to keep the module‑level flow easy to scan.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/apps/common/management/commands/load_data.py(1 hunks)backend/wsgi.py(1 hunks)infrastructure/README.md(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- infrastructure/README.md
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2551
File: infrastructure/modules/parameters/main.tf:1-191
Timestamp: 2025-11-08T11:16:25.725Z
Learning: The parameters module in infrastructure/modules/parameters/ is currently configured for staging environment only. The `configuration` and `settings_module` variables default to "Staging" and "settings.staging" respectively, and users can update parameter values via the AWS Parameter Store console. The lifecycle.ignore_changes blocks on these parameters support manual console updates without Terraform reverting them.
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2699
File: backend/wsgi.py:13-13
Timestamp: 2025-11-23T11:52:15.447Z
Learning: In the OWASP Nest project, the SSM parameter store setup in backend/wsgi.py (using boto3 to fetch parameters from AWS Systems Manager) is designed for staging and production environments, not just for testing purposes.
📚 Learning: 2025-11-23T11:52:15.447Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2699
File: backend/wsgi.py:13-13
Timestamp: 2025-11-23T11:52:15.447Z
Learning: In the OWASP Nest project, the SSM parameter store setup in backend/wsgi.py (using boto3 to fetch parameters from AWS Systems Manager) is designed for staging and production environments, not just for testing purposes.
Applied to files:
backend/wsgi.py
🧬 Code graph analysis (1)
backend/apps/common/management/commands/load_data.py (2)
backend/apps/owasp/management/commands/owasp_enrich_events.py (2)
add_arguments(17-24)handle(26-60)backend/apps/common/management/commands/restore_backup.py (1)
handle(13-17)
🔇 Additional comments (1)
backend/apps/common/management/commands/load_data.py (1)
13-27: Dynamic fixture path wiring looks correct and backward compatible
add_argumentscorrectly registers--fixture-pathwith a sensible default, and using_options["fixture_path"]inhandlematches Django’s option naming (hyphens → underscores). The updatedcall_command("loaddata", _options["fixture_path"], "-v", "3")preserves existing behavior while allowing overrides, and stays consistent with the pattern inrestore_backup.py. I don’t see any correctness or transactional issues here.Also applies to: 31-31
* Use AWS SSM Parameter Store to handle environment variables * Use focused policy for read access * Update documentation * Add flag for create_rds_proxy * set default value of create_rds_proxy to false * Populate Zappa/Lambda environment variables from ssm/parameter store * Update documentation * Update example * add default configurations * add security group db from lambda * fix load-data task by adding a --fixture-path flag * fix ecs tasks by introducing ecs-* make targets * change ecs run steps * remove ecs-* and clean code * add --no-cache * use call_command * add test for --fixture-path * Update code * Update backend/wsgi.py --------- Co-authored-by: Arkadii Yakovets <[email protected]> Co-authored-by: Arkadii Yakovets <[email protected]>
arkid15r
force-pushed
the
feature/nest-zappa-migration-security-groups
branch
from
c520124 to
d56de4d
Compare
|
arkid15r
merged commit 1020b29
into
OWASP:feature/nest-zappa-migration
2 participants
Resolves #2568
Proposed change
Note PR depends on another PR, will be rebased once it's merged.
Checklist
make check-testlocally; all checks and tests passed.