feat: cloud integration — WAF v2, Secrets Manager, CloudTrail retention, DynamoDB PITR, SQS DLQ alarms#28
Conversation
…DB PITR, SQS DLQ alarms Agent-Logs-Url: https://github.com/Panacota96/master-Project-Phishing/sessions/3af16f29-8959-43b5-a275-793626e09aef Co-authored-by: Panacota96 <118935424+Panacota96@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds baseline AWS security/audit controls and reliability safeguards to the platform’s Terraform infrastructure, plus runtime support for loading secrets from AWS Secrets Manager in the Flask app configuration.
Changes:
- Provision and attach a CloudFront-scoped WAFv2 Web ACL (managed rule groups + per-IP rate limiting) with an enable/disable toggle.
- Move application secrets into AWS Secrets Manager and add CloudTrail + SQS DLQ depth alarming + DynamoDB PITR.
- Update
config.pyto resolveSECRET_KEY/MSAL_CLIENT_SECRETviaSECRET_ARNat startup (with env-var fallback).
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| VERSION | Bumps project version to 1.3.0. |
| phishing-platform-infra/terraform/waf.tf | Defines WAFv2 Web ACL for CloudFront (us-east-1 provider alias) + output. |
| phishing-platform-infra/terraform/variables.tf | Adds feature toggles for WAF and CloudTrail. |
| phishing-platform-infra/terraform/terraform.tfvars.example | Documents the new infra toggles. |
| phishing-platform-infra/terraform/secrets_manager.tf | Creates Secrets Manager secret/version for app secrets + output. |
| phishing-platform-infra/terraform/outputs.tf | Adds outputs for Secrets Manager + WAF. |
| phishing-platform-infra/terraform/lambda.tf | Switches app Lambda from SECRET_KEY to SECRET_ARN env var. |
| phishing-platform-infra/terraform/iam.tf | Grants Lambda secretsmanager:GetSecretValue on the app secret. |
| phishing-platform-infra/terraform/dynamodb.tf | Enables PITR across all DynamoDB tables. |
| phishing-platform-infra/terraform/cloudwatch_monitoring.tf | Adds CloudWatch alarms for registration/campaign DLQ depth. |
| phishing-platform-infra/terraform/cloudtrail.tf | Adds multi-region CloudTrail + dedicated encrypted S3 bucket. |
| phishing-platform-infra/terraform/cloudfront.tf | Attaches WAF Web ACL ARN to CloudFront when enabled. |
| documentation/ARCHITECTURE.md | Updates diagrams to include WAF/Secrets Manager/CloudTrail/PITR/DLQ alarms. |
| config.py | Adds Secrets Manager resolution helpers for SECRET_KEY and MSAL client secret. |
| CHANGELOG.md | Documents new infra/security features for v1.3.0. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| FLASK_ENV = var.environment | ||
| SECRET_KEY = var.secret_key | ||
| SECRET_ARN = aws_secretsmanager_secret.app_secrets.arn | ||
| AWS_REGION_NAME = var.aws_region |
There was a problem hiding this comment.
This switches SECRET_KEY to SECRET_ARN, but the same file still sets MSAL_CLIENT_SECRET = var.msal_client_secret later in the Lambda environment variables, leaving a secret exposed in plaintext env vars. To fully achieve the stated security goal, remove the plaintext MSAL_CLIENT_SECRET env var and fetch it via Secrets Manager using SECRET_ARN (as config.py now supports).
| secret_arn = os.environ.get("SECRET_ARN", "") | ||
| if secret_arn: | ||
| try: | ||
| import boto3 | ||
| from botocore.exceptions import BotoCoreError, ClientError | ||
|
|
||
| region = os.environ.get("AWS_REGION_NAME", "eu-west-3") | ||
| client = boto3.client("secretsmanager", region_name=region) | ||
| resp = client.get_secret_value(SecretId=secret_arn) | ||
| data = json.loads(resp["SecretString"]) | ||
| return data.get("SECRET_KEY", os.environ.get("SECRET_KEY", "dev-secret-key-change-in-production")) | ||
| except (ClientError, BotoCoreError) as exc: | ||
| logger.warning("Secrets Manager fetch failed (%s); falling back to SECRET_KEY env var", exc) | ||
| except (json.JSONDecodeError, KeyError) as exc: | ||
| logger.warning("Secrets Manager secret parse error (%s); falling back to SECRET_KEY env var", exc) | ||
| return os.environ.get("SECRET_KEY", "dev-secret-key-change-in-production") |
There was a problem hiding this comment.
When SECRET_ARN is set but the Secrets Manager call fails, this falls back to os.environ.get("SECRET_KEY", "dev-secret-key-change-in-production"). In the new Terraform setup, SECRET_KEY is no longer provided to the Lambda env, so a transient Secrets Manager/VPC endpoint/IAM issue would silently switch production to the insecure dev default (breaking session integrity). Consider failing fast if SECRET_ARN is set and SECRET_KEY env var is absent, or at least avoid ever using the dev default in that path.
| def _resolve_secret_key() -> str: | ||
| """Return the Flask SECRET_KEY. | ||
|
|
||
| When running on AWS Lambda the ``SECRET_ARN`` environment variable points | ||
| to a Secrets Manager secret that holds the key as a JSON object | ||
| ``{"SECRET_KEY": "..."}`` alongside other credentials. We fetch the value | ||
| at startup so the plaintext key is never stored as a Lambda environment | ||
| variable (where it is visible in the AWS console). | ||
|
|
||
| Falls back to the ``SECRET_KEY`` environment variable (or the insecure | ||
| development default) when ``SECRET_ARN`` is absent — this preserves | ||
| backward-compatibility for local development and unit tests. | ||
| """ | ||
| secret_arn = os.environ.get("SECRET_ARN", "") | ||
| if secret_arn: | ||
| try: | ||
| import boto3 | ||
| from botocore.exceptions import BotoCoreError, ClientError | ||
|
|
||
| region = os.environ.get("AWS_REGION_NAME", "eu-west-3") | ||
| client = boto3.client("secretsmanager", region_name=region) | ||
| resp = client.get_secret_value(SecretId=secret_arn) | ||
| data = json.loads(resp["SecretString"]) | ||
| return data.get("SECRET_KEY", os.environ.get("SECRET_KEY", "dev-secret-key-change-in-production")) | ||
| except (ClientError, BotoCoreError) as exc: | ||
| logger.warning("Secrets Manager fetch failed (%s); falling back to SECRET_KEY env var", exc) | ||
| except (json.JSONDecodeError, KeyError) as exc: | ||
| logger.warning("Secrets Manager secret parse error (%s); falling back to SECRET_KEY env var", exc) | ||
| return os.environ.get("SECRET_KEY", "dev-secret-key-change-in-production") | ||
|
|
||
|
|
||
| def _resolve_msal_client_secret() -> str: | ||
| """Return the MSAL client secret from Secrets Manager or env var.""" | ||
| secret_arn = os.environ.get("SECRET_ARN", "") | ||
| if secret_arn: | ||
| try: | ||
| import boto3 | ||
| from botocore.exceptions import BotoCoreError, ClientError | ||
|
|
||
| region = os.environ.get("AWS_REGION_NAME", "eu-west-3") | ||
| client = boto3.client("secretsmanager", region_name=region) | ||
| resp = client.get_secret_value(SecretId=secret_arn) | ||
| data = json.loads(resp["SecretString"]) | ||
| return data.get("MSAL_CLIENT_SECRET", os.environ.get("MSAL_CLIENT_SECRET", "")) | ||
| except (ClientError, BotoCoreError) as exc: | ||
| logger.warning("Secrets Manager fetch failed (%s); falling back to MSAL_CLIENT_SECRET env var", exc) | ||
| except (json.JSONDecodeError, KeyError) as exc: | ||
| logger.warning("Secrets Manager secret parse error (%s); falling back to MSAL_CLIENT_SECRET env var", exc) | ||
| return os.environ.get("MSAL_CLIENT_SECRET", "") |
There was a problem hiding this comment.
Both _resolve_secret_key() and _resolve_msal_client_secret() call Secrets Manager independently, which results in two network calls during cold start (and repeats JSON parsing). Consider factoring this into a single helper that fetches/parses the secret once and memoizes the result (module-level cache), then have both resolvers read from that cached dict.
| resource "aws_s3_bucket" "cloudtrail" { | ||
| count = var.enable_cloudtrail ? 1 : 0 | ||
| bucket = "${local.prefix}-cloudtrail-${data.aws_caller_identity.current.account_id}" | ||
| force_destroy = true | ||
|
|
There was a problem hiding this comment.
force_destroy = true on the CloudTrail log bucket makes it easy to accidentally delete audit logs (including non-empty buckets) during Terraform destroy/replace, which is usually undesirable for audit/compliance and incident response. Consider removing force_destroy (or making it an explicit opt-in variable for non-prod) and adding a retention/lifecycle policy instead.
|
|
||
| output "secrets_manager_arn" { | ||
| description = "ARN of the Secrets Manager secret holding Flask SECRET_KEY and MSAL credentials" | ||
| value = aws_secretsmanager_secret.app_secrets.arn | ||
| } | ||
|
|
||
| output "waf_arn" { | ||
| description = "ARN of the WAF v2 Web ACL attached to CloudFront (empty if WAF disabled)" | ||
| value = var.enable_waf ? aws_wafv2_web_acl.app[0].arn : "" | ||
| } |
There was a problem hiding this comment.
There are now two different outputs for the same Secrets Manager ARN (app_secrets_arn in secrets_manager.tf and secrets_manager_arn here), and similarly two for the WAF ACL ARN (waf_web_acl_arn vs waf_arn). Keeping duplicate outputs with different names can confuse downstream consumers; consider consolidating to a single canonical output name for each value and removing the redundant ones.
| output "secrets_manager_arn" { | |
| description = "ARN of the Secrets Manager secret holding Flask SECRET_KEY and MSAL credentials" | |
| value = aws_secretsmanager_secret.app_secrets.arn | |
| } | |
| output "waf_arn" { | |
| description = "ARN of the WAF v2 Web ACL attached to CloudFront (empty if WAF disabled)" | |
| value = var.enable_waf ? aws_wafv2_web_acl.app[0].arn : "" | |
| } |
| - **AWS Secrets Manager** (`secrets_manager.tf`): Flask `SECRET_KEY` and `MSAL_CLIENT_SECRET` are now stored in a Secrets Manager secret (`{prefix}/app-secrets`). The Lambda receives `SECRET_ARN` instead of the plaintext values, preventing credentials from appearing in the AWS console environment variables view. | ||
| - **AWS CloudTrail** (`cloudtrail.tf`): Multi-region trail writing to a dedicated AES256-encrypted S3 bucket with log-file validation enabled and S3 data-event logging for the application bucket. Controlled by `enable_cloudtrail` variable (default `true`). | ||
| - **DynamoDB PITR**: Point-in-Time Recovery enabled on all 11 DynamoDB tables for a 35-day recovery window. | ||
| - **SQS DLQ alarms**: Two new CloudWatch alarms (`{prefix}-registration-dlq-depth` and `{prefix}-campaign-dlq-depth`) fire on the SNS alerts topic when either dead-letter queue accumulates ≥ 1 message. | ||
| - **`config.py` Secrets Manager resolver**: `_resolve_secret_key()` and `_resolve_msal_client_secret()` fetch credentials from Secrets Manager when `SECRET_ARN` is set, falling back to the `SECRET_KEY` / `MSAL_CLIENT_SECRET` environment variables for local development and unit tests. | ||
| - **Terraform variables**: `enable_waf` (bool, default `true`) and `enable_cloudtrail` (bool, default `true`) added to `variables.tf`. | ||
| - **Terraform outputs**: `waf_arn`, `app_secrets_arn`, `secrets_manager_arn`, and `cloudtrail_bucket` added to `outputs.tf`. | ||
|
|
||
| ### Changed | ||
| - `lambda.tf`: Lambda env var `SECRET_KEY` replaced with `SECRET_ARN`; `MSAL_CLIENT_SECRET` removed from plaintext env vars (now fetched at runtime from Secrets Manager). | ||
| - `iam.tf`: Lambda execution role now includes `secretsmanager:GetSecretValue` on the application secrets ARN. |
There was a problem hiding this comment.
This entry says MSAL_CLIENT_SECRET was removed from plaintext Lambda env vars and that Lambda receives only SECRET_ARN, but phishing-platform-infra/terraform/lambda.tf still sets MSAL_CLIENT_SECRET = var.msal_client_secret in the environment. Either update the infrastructure to actually remove it, or adjust the changelog to reflect the current behavior.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 357f4ffaf2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| logger.warning("Secrets Manager fetch failed (%s); falling back to SECRET_KEY env var", exc) | ||
| except (json.JSONDecodeError, KeyError) as exc: | ||
| logger.warning("Secrets Manager secret parse error (%s); falling back to SECRET_KEY env var", exc) | ||
| return os.environ.get("SECRET_KEY", "dev-secret-key-change-in-production") |
There was a problem hiding this comment.
When SECRET_ARN is present and Secrets Manager retrieval fails (for example due to IAM, throttling, or regional issues), this path falls back to os.environ.get("SECRET_KEY", "dev-secret-key-change-in-production"). In the same commit lambda.tf replaces SECRET_KEY with SECRET_ARN, so production can start with the known dev key instead of refusing to boot, which weakens session and CSRF signing security.
Useful? React with 👍 / 👎.
| # as plaintext Lambda environment variables (visible in the AWS console). | ||
| # The Lambda receives SECRET_ARN; config.py fetches the value at startup. | ||
|
|
||
| resource "aws_secretsmanager_secret" "app_secrets" { |
There was a problem hiding this comment.
This adds new Terraform-managed resources (starting with aws_secretsmanager_secret here), but the GitHub deploy role policy in phishing-platform-infra/terraform/github_actions_oidc.tf does not include required actions for these changes (e.g., Secrets Manager CRUD, WAFv2, CloudTrail, and dynamodb:UpdateContinuousBackups for PITR). In the CI deploy workflows that assume AWS_DEPLOY_ROLE_ARN, terraform apply will fail with AccessDenied before rollout completes.
Useful? React with 👍 / 👎.
|
@openai-code-agent review the pr |
|
@codex[agent] review the comments on #28 (review) and #28 (review) investigate, an apli a fix on this |
Summary
Testing
|
Co-authored-by: Panacota96 <118935424+Panacota96@users.noreply.github.com>
Addressed the review points in a9a1e47: CloudTrail bucket now retains logs with lifecycle (no force-destroy) and changelog updated; MSAL secret already pulled from Lambda env, secret resolvers cache the bundle, and duplicate outputs removed. |
|
To use Codex here, create a Codex account and connect to github. |
Codex
AI
changed the title
The application lacked several foundational cloud security and reliability controls: no WAF on CloudFront,
SECRET_KEYexposed as a plaintext Lambda env var, no audit trail or retention safeguards, no DynamoDB backups, and no alerting on dead-letter queue buildup.Summary
enable_waf.SECRET_KEYandMSAL_CLIENT_SECRETin Secrets Manager; Lambda now receives onlySECRET_ARN, andconfig.pyfetches a shared cached payload and fails closed when the secret is missing.Validation
make lintmake test(fails locally: Playwright browsers not installed and moto 5.0.28 leaves existing DynamoDB tables, causingResourceInUseduring setup)make docs-checkterraform -chdir=phishing-platform-infra/terraform validateLinked Work
Deployment Notes