feat: add CloudFormation IAM activity alerts#1
Conversation
…and README setup docs Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…down script reference Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… group cleanup Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
solaris007
left a comment
There was a problem hiding this comment.
Hey @ssilare-adobe,
Thanks for the v2 - good job addressing the feedback from the prior review on adobe/mysticat-ai-native-guidelines#4. The eval injection fix, NoEcho documentation, Adobe-specific defaults removal, doc consolidation, error handling, email template fix, presigned URLs, and standalone repo scope are all addressed. A few things remain before this is ready.
Strengths
- Clean conditional architecture -
UseEmail/UseSlackconditions with!If+AWS::NoValueon EventBridge targets is the correct CloudFormation pattern for optional resources, avoiding dead infrastructure - Eval injection properly eliminated - bash array (
PARAMS=(...) + "${PARAMS[@]}") in deploy.sh is the right fix - Least-privilege IAM role -
SlackNotificationFunctionRoleonly attachesAWSLambdaBasicExecutionRole, no over-scoping - SNS topic policy correctly scoped to
events.amazonaws.comwith specific resource ARN - Honest NoEcho documentation - both the template parameter description and README Security Notes call out the
lambda:GetFunctionConfigurationlimitation with concrete SSM/Secrets Manager recommendations - Teardown with orphan detection - scanning for Lambda-created CloudWatch Log Groups after stack deletion addresses a real operational gap
- Region validation - regex guard on
aws configure get regionprevents corrupted config values from being silently used - Lambda uses defensive
.get()throughout with fallback defaults
Issues
Critical (Must Fix)
1. Lambda silently drops failed Slack notifications - alerts permanently lost
iam-alerts-cloudformation.yaml:187
When the Slack webhook returns a non-200 status (rate-limited 429, server error 500, revoked webhook 403), the Lambda logs the error but returns a success response. EventBridge considers this a successful invocation (Lambda didn't throw), so it won't retry. The security alert is permanently lost with no indication to the operator. For a security monitoring tool, silent alert loss is the worst failure mode.
Fix: Raise an exception on non-200 responses so EventBridge retries (up to 185 times over 24 hours by default):
if response.status != 200:
msg = f'Slack webhook returned HTTP {response.status}'
logger.error(msg)
raise RuntimeError(msg)Important (Should Fix)
2. EventBridge rule deploys with zero targets if both channels are empty
iam-alerts-cloudformation.yaml:28
HasNotifications condition is defined (!Or [UseEmail, UseSlack]) but never referenced. If someone deploys the template directly (bypassing deploy.sh) with both NotificationEmail and SlackWebhookUrl empty, both !If branches resolve to AWS::NoValue. The EventBridge rule is created with an empty Targets list - events match but go nowhere. No error, no warning. The operator believes they have alerting, but they don't.
Fix: Add a CloudFormation validation rule that fails fast at deploy time:
Rules:
AtLeastOneNotification:
Assertions:
- Assert: !Or
- !Not [!Equals [!Ref NotificationEmail, '']]
- !Not [!Equals [!Ref SlackWebhookUrl, '']]
AssertDescription: At least one notification method (email or Slack) must be configured3. deploy.sh || true swallows stack update failures
deploy.sh:259
If the update fails (bad Lambda code, IAM error), the script prints "Stack deployment completed!" and exits 0. The operator sees success when the stack is in UPDATE_ROLLBACK_COMPLETE.
Fix: Check actual stack status after the wait:
aws cloudformation wait stack-update-complete ... 2>/dev/null
STATUS=$(aws cloudformation describe-stacks --stack-name "$STACK_NAME" --region "$REGION" \
--query 'Stacks[0].StackStatus' --output text)
if [[ "$STATUS" != "UPDATE_COMPLETE" ]]; then
print_error "Stack update failed. Status: ${STATUS}"
exit 1
fi4. deploy.sh overwrites the default AWS CLI profile
deploy.sh:86
aws configure set without --profile silently overwrites whatever credentials exist in the user's default profile. For security teams who likely have multiple profiles, this could replace production credentials with temporary session creds. If the new credentials are also invalid, the old working ones are already gone.
Fix: Use environment variables instead (session-scoped, no disk persistence):
export AWS_ACCESS_KEY_ID="$aws_access_key_id"
export AWS_SECRET_ACCESS_KEY="$aws_secret_access_key"
export AWS_SESSION_TOKEN="$aws_session_token"5. Teardown log group scan is overly broad
teardown.sh:129
The JMESPath query contains(logGroupName, 'iam-') matches ANY log group with "iam-" in the name - including unrelated ones like /aws/lambda/iam-rotation-handler. A hasty "yes" at the interactive prompt deletes someone else's operational logs.
Fix: Remove the || contains(logGroupName, 'iam-') clause. Match the specific Lambda function name pattern instead: /aws/lambda/${ALERT_RULE_NAME}-slack-notifier.
6. Lambda should validate webhook URL scheme
iam-alerts-cloudformation.yaml (inline Lambda)
The Lambda POSTs to whatever URL is in the env var with no validation. A non-HTTPS URL makes it an SSRF vector, leaking event data (source IPs, principal ARNs, account IDs) to any reachable endpoint.
Fix:
if not webhook_url.startswith('https://hooks.slack.com/'):
logger.error('Invalid webhook URL - must start with https://hooks.slack.com/')
return {'statusCode': 400, 'body': 'Invalid webhook URL'}Minor (Nice to Have)
7. Cross-stack exports create deletion friction - All three outputs use Export. If any other stack imports these values, this stack becomes undeletable. For a self-contained alerting tool, remove the Export blocks (keep the outputs).
8. No Slack webhook URL validation in deploy.sh - Email gets validated with a regex, but the Slack webhook URL is accepted as-is. Add a basic format check (^https://hooks\.slack\.com/services/).
9. Lambda FunctionName / SNS TopicName collision risk - Explicit names prevent CloudFormation replacement during updates and cause conflicts if two stacks use the same AlertRuleName. Consider omitting physical names.
10. Region validation regex misses GovCloud - ^[a-z]{2}-[a-z]+-[0-9]+$ doesn't match us-gov-west-1. Change to ^[a-z]{2}(-[a-z]+)+-[0-9]+$.
Recommendations
- Add a DLQ - An SQS dead-letter queue on the EventBridge target catches permanent failures after retries are exhausted. For a security alerting pipeline, this is the safety net.
- Add CloudFormation parameter validation -
AllowedPatternon the webhook URL parameter catches misconfiguration at deploy time. - Add
cfn-lintandshellcheckto CI - catches template and script issues statically. - Consider
AWS::CloudFormation::Interfacemetadata - groups parameters logically in the AWS Console.
Assessment
Ready to merge? No - fix Critical #1 first, Important #2-6 strongly recommended.
The overall design is sound - EventBridge + conditional SNS/Lambda is the right pattern, and the prior review feedback has been well addressed. The critical issue is that the Lambda's silent failure on non-200 Slack responses means security alerts can be permanently lost. That's a one-line fix (raise instead of return). The important issues are all straightforward fixes that prevent real operational surprises.
Next Steps
- Fix Critical #1 (Lambda raise on non-200) - one-line change
- Address Important #2-6 - all small, targeted fixes
- Minor items and recommendations can be follow-up work
| response.status, | ||
| response.data.decode('utf-8') | ||
| ) | ||
| return { |
There was a problem hiding this comment.
Critical: This returns success to EventBridge on non-200 Slack responses, so EventBridge won't retry and the security alert is silently lost. Raise an exception instead:
if response.status != 200:
msg = f'Slack webhook returned HTTP {response.status}'
logger.error(msg)
raise RuntimeError(msg)| if [ "$OPERATION" == "create-stack" ]; then | ||
| aws cloudformation wait stack-create-complete --stack-name "$STACK_NAME" --region "$REGION" | ||
| else | ||
| aws cloudformation wait stack-update-complete --stack-name "$STACK_NAME" --region "$REGION" 2>/dev/null || true |
There was a problem hiding this comment.
Important: || true swallows real update failures - if the stack rolls back, the script still prints 'completed' and exits 0. Check actual stack status after the wait instead.
| read -r -p "AWS Region [${REGION}]: " input | ||
| REGION=${input:-$REGION} | ||
|
|
||
| aws configure set aws_access_key_id "$aws_access_key_id" |
There was a problem hiding this comment.
Important: aws configure set without --profile silently overwrites the user's default AWS profile credentials. Consider using environment variables (export AWS_ACCESS_KEY_ID=...) instead - they're session-scoped and don't persist to disk.
| log_groups=$(aws logs describe-log-groups \ | ||
| --log-group-name-prefix "/aws/lambda/" \ | ||
| --region "$REGION" \ | ||
| --query "logGroups[?contains(logGroupName, '${STACK_NAME}') || contains(logGroupName, 'iam-')].logGroupName" \ |
There was a problem hiding this comment.
Important: contains(logGroupName, 'iam-') is too broad - matches unrelated log groups like /aws/lambda/iam-rotation-handler. Match against the specific Lambda function name pattern (${ALERT_RULE_NAME}-slack-notifier) instead.
…dential handling, fix log group scope, add webhook validation
|
Thanks for the thorough review @solaris007! I've addressed all Critical and Important issues in 548f616, plus the clear-cut Minor items. Here's what was done: Critical #1 — Lambda now raises on non-200 Slack responses Important #2 — CloudFormation Important #3 — Stack update failure no longer swallowed Important #4 — Credentials via env vars, not Important #5 — Teardown log group scan scoped to exact Lambda name Important #6 — Lambda validates webhook URL scheme Minor #7 — Removed Minor #8 — Slack webhook URL validated in deploy.sh Minor #10 — GovCloud region regex fixed Minor #9 (explicit resource names) and the recommendations (DLQ, |
solaris007
left a comment
There was a problem hiding this comment.
Hey @ssilare-adobe,
Excellent turnaround. All six Critical/Important findings from the prior review are resolved, plus three of the four Minor items. The fixes are clean and match the suggested approaches.
Strengths
Previously flagged issues now addressed:
- Lambda now raises
RuntimeErroron non-200 Slack responses (iam-alerts-cloudformation.yaml:193-195), so EventBridge will retry failed alerts instead of silently dropping them - CloudFormation
AtLeastOneNotificationassertion fails fast at deploy time if neither email nor Slack is configured (iam-alerts-cloudformation.yaml:36-42) - a cleaner approach than gating the rule onHasNotificationsbecause the operator gets an immediate, clear error deploy.shstack update now verifies actualStackStatusafter the wait and exits 1 on rollback (deploy.sh:271-276) - no more silent success on failed updates- Credential handling switched to session-scoped environment variables (
deploy.sh:86-90) - no more overwriting the user's default AWS profile - Teardown captures the Lambda function name before stack deletion (
teardown.sh:89-94) and scopes the orphaned log group scan to/aws/lambda/${LAMBDA_FUNCTION_NAME}*, with a safe stack-name-only fallback when the lookup returns empty - Lambda validates the webhook URL scheme before processing events (
iam-alerts-cloudformation.yaml:101-103) - defense in depth against SSRF via misconfiguration - Cross-stack exports removed from all three outputs - the stack is now cleanly deletable
- Webhook URL validation added to
deploy.shprompts - catches typos at deploy time with a clear error message - Region regex updated to
^[a-z]{2}(-[a-z]+)+-[0-9]+$in both scripts, now handles GovCloud regions likeus-gov-west-1
Assessment
Ready to merge? Yes.
Reasoning: The original design was sound and the prior review's findings are all resolved. The remaining unaddressed item (explicit FunctionName/TopicName collision risk) is legitimately minor - it only surfaces if two stacks use the same AlertRuleName in the same account/region, which is unlikely for a security alerting tool typically deployed once per account. Ship it.
Summary
Migrates the CloudFormation IAM activity alerts from adobe/mysticat-ai-native-guidelines#4 into this standalone repo, with all issues from the code review addressed.
Review feedback addressed
evalcommand injection indeploy.sh— replaced with a bash array (PARAMS=(...)+"${PARAMS[@]}") so user-supplied input never passes throughevalportalsvc1,klam-sts-userfromExcludedUserNamesdefault everywhere (template, script, parameters file); replaced with generic placeholders or emptytry/except; logs HTTP errors and non-200 responses vialogger.errorbefore returning, and re-raises unexpected exceptionsInputTemplateblock with a single JSON string using\nescapes, so email bodies arrive as plain text without literal quote characters--acl public-read— replaced with presigned URL instructions and StackSets as alternatives; no public ACL recommendationNoEcholimitation — added an explicit note in both the CloudFormation parameter description and README explaining thatNoEchodoesn't preventlambda:GetFunctionConfigurationaccess, and recommending SSM SecureString / Secrets Manager for higher-security environmentsREADME.mdappropriate for a standalone repoChanges in this update
teardown.sh— new interactive teardown script that lists stack resources, confirms before deleting, and waits for completionteardown.shpost-deletion verification — after stack deletion, verifies the stack is truly gone and scans for leftover CloudWatch Log Groups (which Lambda creates at runtime outside of CloudFormation); offers to delete them interactivelydeploy.shregion validation —aws configure get regionnow validated against the AWS region pattern before use; falls back tous-east-1if the stored value is corrupted (e.g. accidentally set to a session token)README.mdsetup docs — expanded Prerequisites and Quick Start sections with step-by-step AWS CLI install, credential setup (including temporary/SSO credentials), CloudTrail verification, and a numbered walkthrough of everydeploy.shpromptdeploy.shpost-deployment instructions — expanded next steps to cover all three monitored events (CreateUser,CreateAccessKey,CreateLoginProfile) with ready-to-run test commands, cleanup steps, CloudWatch log tailing, and a reference toteardown.shfor stack removalFiles
README.md— consolidated documentation covering setup, configuration, testing, sharing, security notes, and troubleshootingiam-alerts-cloudformation.yaml— EventBridge rule, SNS topic, Lambda (Slack), IAM rolesdeploy.sh— interactive deployment script with region validation and comprehensive post-deployment instructionsteardown.sh— interactive stack teardown script with post-deletion verificationparameters-example.json— sample parameters with generic placeholders