Skip to content

Conversation

@bspeagle
Copy link
Collaborator

@bspeagle bspeagle commented Dec 23, 2025

🔄 Fully Automated IAM Key Rotation System with Terragrunt Multi-Environment Deployment

Overview

This PR implements a complete automated IAM access key rotation system that eliminates manual key rotation workflows. The system automatically creates new keys, stores them encrypted in S3, tracks downloads via CloudTrail, sends reminder emails, and cleans up old keys after 14 days.

🚀 Major Features

Automated Key Rotation System (4 Lambda Functions)

  1. Enforcement Lambda - Daily monitoring and rotation initiation

    • Generates AWS IAM credential reports
    • Detects keys reaching age thresholds (configurable: 0/1/2 days for testing, 75/85/90 for production)
    • Creates new IAM access keys automatically
    • Encrypts and stores credentials in S3 with AES-256
    • Generates 7-day pre-signed download URLs
    • Creates DynamoDB tracking records
    • Sends HTML email notifications via SES with download links
  2. Download Tracker Lambda - Real-time download monitoring

    • Triggered by CloudTrail S3 Data Events (GetObject)
    • Marks credentials as downloaded in DynamoDB
    • Deletes S3 credential file immediately after download (one-time use)
    • Tracks download IP address and timestamp
  3. URL Regenerator Lambda - Day 7 reminder system

    • Queries DynamoDB for credentials approaching expiration
    • Checks S3 file existence before sending reminders
    • Generates new 7-day pre-signed URLs
    • Sends reminder emails if credentials not downloaded
  4. Cleanup Lambda - Day 14 old key deletion

    • Scans DynamoDB for records reaching deletion date
    • Deletes original (old) IAM access keys
    • Updates DynamoDB status to completed
    • Marks old_key_deleted: true

Infrastructure Components

  • 2 S3 Buckets: Encrypted credentials storage + CloudTrail audit logs
  • 1 DynamoDB Table: Complete rotation lifecycle tracking with composite keys (PK/SK) and GSI indexes
  • CloudTrail: S3 Data Events trail for download detection
  • 3 EventBridge Rules: Daily enforcement, day 7 reminders, day 14 cleanup
  • CloudWatch: Log groups for all 4 Lambdas, custom metrics, and alarms
  • SES: HTML email templates with branded styling
  • IAM Roles: Least-privilege policies for all Lambda functions

Terragrunt Multi-Environment Deployment

  • Hierarchical configuration structure: account → region → environment → service
  • Remote state management: S3 backend with DynamoDB locking
  • Environment isolation: Separate state files per environment
  • DRY configuration: Shared settings via account.hcl, region.hcl, env.hcl
  • Test environment: Ultra-fast thresholds (0/1/2 days) for immediate validation
  • Absolute path resolution: Fixed Lambda packaging with get_repo_root()

🔧 Technical Implementation

Lambda Functions

  • Python 3.11 runtime
  • Boto3 for AWS SDK operations
  • Environment variables encrypted with KMS
  • Comprehensive error handling and logging
  • CloudWatch metrics publishing

DynamoDB Schema

Primary Key: PK (username) + SK (old_key_id)
Attributes:
- rotation_initiated (timestamp)
- new_key_id
- new_key_secret (encrypted)
- current_url_expires (date string for day 7 queries)
- url_expires_at (timestamp)
- downloaded (boolean)
- download_timestamp
- old_key_deletion_date (timestamp for day 14 scans)
- old_key_deleted (boolean)
- status (rotation_initiated | completed)

GSI: status-index (status + rotation_initiated)

CloudTrail Integration

  • S3 Data Events for GetObject operations
  • EventBridge rule filters for credentials bucket
  • Lambda invocation via EventBridge (not S3 notifications)
  • Separate S3 bucket for CloudTrail logs with proper IAM policies

Email System

  • HTML templates with CSS styling (blue/orange/red severity levels)
  • Plain text fallback for email clients
  • Pre-signed URL download links
  • One-time use warnings
  • 14-day retention timeline

🐛 Bug Fixes & Iterations

  1. S3 Notification → CloudTrail Migration

    • S3 bucket notifications don't support GetObject events
    • Implemented CloudTrail S3 Data Events with EventBridge trigger
    • Updated download tracker Lambda to parse CloudTrail event format
  2. Lambda Environment Variable Encryption

    • Added KMS decrypt permissions to all Lambda IAM policies
    • Handles AWS-managed encryption keys
  3. DynamoDB Query Optimization

    • Fixed cleanup Lambda to use Scan instead of incorrect GSI query
    • URL regenerator queries by current_url_expires date string
    • Added composite key support (PK/SK) for efficient lookups
  4. IAM Permissions Fine-Tuning

    • Added dynamodb:Query to download tracker
    • Added dynamodb:Scan to cleanup Lambda
    • Added s3:HeadObject to URL regenerator for file existence checks
    • Added iam:DeleteAccessKey and iam:ListAccessKeys to cleanup
  5. S3 File Existence Checks

    • URL regenerator now checks if S3 file exists before sending reminders
    • Prevents reminder emails for already-downloaded credentials
  6. Terragrunt Path Issues

    • Fixed Lambda source paths with absolute get_repo_root() references
    • Resolved cache path issues causing Lambda packaging failures

📚 Documentation Updates

Main README.md

  • Updated features section for fully automated system
  • Replaced architecture diagram with complete mermaid workflow
  • Added day-by-day timeline (Day 0, 1-7, 7, 14)
  • Expanded core components table
  • Updated deployment instructions for Terragrunt
  • Enhanced monitoring section with DynamoDB tracking

terraform/iam/README.md

  • Updated overview and features for automation
  • Complete architecture diagram with all components
  • Organized resources by category (Lambda, Storage, Events, IAM, Monitoring)
  • Expanded inputs/outputs tables
  • Updated usage examples

terragrunt/README.md

  • FIXED critical threshold values: 0/1/2 days (was incorrectly documented as 30/45/60)
  • Added complete system components section
  • Documented all 4 CloudWatch log groups
  • Expanded monitoring with DynamoDB structure
  • Enhanced validation section with commands for all Lambdas

tests/README_WORKFLOW_TESTING.md

  • New comprehensive testing guide
  • Day 7 and day 14 workflow simulation instructions
  • Prerequisites including infrastructure deployment
  • Important notes about AWS credential report caching
  • System architecture context

🧪 Testing & Validation

Dev Environment Configuration

warning_threshold  = 0  # Immediate rotation
urgent_threshold   = 1  # 1 day old
disable_threshold  = 2  # 2 days old (old key deleted)
auto_disable       = false  # Manual deletion for safety
schedule_expression = "rate(6 hours)"

Test Users

  • iam-test-user-dev-1 - Normal test user
  • iam-test-user-dev-2 - Test user (not exempt)
  • iam-test-exempt-user-dev - Exemption testing

Validation Commands

# Test enforcement Lambda
aws lambda invoke --function-name iam-access-key-enforcement

# Run workflow tests
python3 tests/test_rotation_workflow.py

# Check DynamoDB records
aws dynamodb scan --table-name iam-key-rotation-tracking

# Verify S3 credentials
aws s3 ls s3://iam-credentials-{account-id}/credentials/

📊 Deployment Stats

  • 20 files changed: 3,055 insertions(+), 240 deletions(-)
  • 4 new Lambda functions: enforcement, download_tracker, url_regenerator, cleanup
  • 12 new Terraform resources: S3 buckets, DynamoDB table, CloudTrail, EventBridge rules
  • 3 new test scripts: workflow testing, interactive validation
  • 4 comprehensive READMEs: main, terraform, terragrunt, testing

🔒 Security Features

  • AES-256 S3 bucket encryption
  • Pre-signed URLs with 7-day expiration
  • One-time credential file deletion after download
  • KMS encryption for Lambda environment variables
  • Least-privilege IAM policies
  • CloudTrail audit logging
  • Tag-based user exemption system

🎯 Production Readiness

  • Fully automated rotation workflow
  • Complete DynamoDB lifecycle tracking
  • CloudTrail download monitoring
  • Email notification system
  • CloudWatch metrics and alarms
  • Comprehensive documentation
  • Test environment validated
  • Multi-environment deployment structure
  • AWS credential report caching tested (requires time)
  • Production threshold configuration (75/85/90 days)

⚠️ Known Limitations

  • AWS IAM credential reports cache for several hours (affects new user detection)
  • Testing timing: credential reports refresh faster during evening/off-peak hours
  • Test users require waiting for credential report regeneration

🚢 Deployment Instructions

cd terragrunt/mvw-dw-nonprod/us-east-1/dev/iam-key-rotation
terragrunt init
terragrunt plan
terragrunt apply

Branch: feature/terragrunt-multi-environment-deployment
Target: main
Status: Ready for review
Testing: Dev environment fully operational

Summary by CodeRabbit

Release Notes

  • New Features

    • Fully automated IAM key rotation system with secure credential distribution via pre-signed download links
    • Email notifications at key lifecycle stages (initial notification and day-7 reminders)
    • Automatic credential cleanup after 14-day retention period
    • Download tracking and one-time credential access enforcement
  • Documentation

    • Added comprehensive deployment guide with step-by-step infrastructure setup and configuration
    • New email template examples for rotation notifications
    • Added workflow testing documentation and interactive test scripts
    • Updated system architecture documentation with new infrastructure components
  • Chores

    • Updated gitignore for Terragrunt configuration files
    • Added Terragrunt multi-environment deployment templates and configurations

✏️ Tip: You can customize this high-level summary in your review settings.

Brian Speagle added 2 commits December 23, 2025 00:20
…t setup

Major Changes:
- Add Terragrunt hierarchical configuration for multi-account/region deployment
- Configure mvw-dw-nonprod dev environment in us-east-1
- Deploy Lambda enforcement function with 6-hour schedule
- Set up test IAM users with ultra-fast thresholds (0/1/2 days)
- Configure Gmail sender ([email protected]) for SES email delivery

Infrastructure:
- Terragrunt root configuration with S3 backend and DynamoDB locking
- Account/region/environment hierarchy structure
- Lambda deployment with EventBridge trigger and CloudWatch monitoring
- IAM roles with least-privilege policies for SES, CloudWatch, IAM access

Configuration:
- Warning threshold: 0 days (immediate testing)
- Urgent threshold: 1 day
- Disable threshold: 2 days
- Auto-disable: false (safer for dev)
- Schedule: rate(6 hours)

Testing Validated:
- Lambda execution: 2.7s, 91MB memory usage
- Email delivery: Successfully sending via SES
- Metrics publishing: All 5 metrics to IAM/KeyRotation namespace
- Exemption system: Tag-based exclusions working
- CloudWatch alarms: Configured for expired and urgent keys

Documentation:
- Complete DEPLOYMENT.md with step-by-step instructions
- Terraform module README with architecture diagrams
- Terragrunt README with hierarchy explanation
- Template documentation for standards compliance

Deployment Stats:
- 27 total keys monitored
- 2 test users receiving notifications
- 1 exempt user properly excluded
- 25 production users discovered (need email tags)

Fixes:
- Resolved SES sandbox mode email delivery issues
- Updated sender from corporate to Gmail for testing
- Removed hardcoded provider.tf (now Terragrunt-managed)

Tested in: mvw-dw-nonprod/us-east-1/dev
Status: Fully operational and validated
Updated all README files to accurately reflect the fully automated IAM key rotation system with complete 14-day lifecycle workflow.

Main README.md:
- Updated features to describe automated key creation, S3 storage, download tracking
- Replaced architecture diagram with complete mermaid showing 4 Lambdas, S3, DynamoDB, CloudTrail, EventBridge
- Added detailed day-by-day workflow timeline (Day 0, 1-7, 7, 14)
- Expanded core components table with all infrastructure
- Updated deployment section for Terragrunt with complete system
- Enhanced monitoring section with DynamoDB tracking details

terraform/iam/README.md:
- Updated overview and features for fully automated system
- Replaced simple architecture diagram with complete workflow
- Organized resources by category (Lambda Functions, Storage & Tracking, Event Processing, IAM, Monitoring)
- Expanded outputs table with all system components organized by category

terragrunt/README.md:
- FIXED critical threshold values: corrected from incorrect 30/45/60 to actual 0/1/2 days for testing
- Added complete system components section (4 Lambdas, 2 S3 buckets, DynamoDB, CloudTrail)
- Documented all 4 CloudWatch log groups with specific Lambda names
- Expanded monitoring with DynamoDB structure and email workflow details
- Enhanced validation section with commands for all 4 Lambdas, DynamoDB queries, S3 checks

tests/README_WORKFLOW_TESTING.md:
- Added overview explaining 14-day lifecycle simulation purpose
- Expanded prerequisites with infrastructure deployment and rotation trigger requirements
- Added important notes about AWS credential report caching and timing
- Included system architecture context showing which Lambdas are tested

All diagrams, infrastructure sections, and directory structures now accurately document the complete automated rotation system with S3 encrypted storage, pre-signed URLs, CloudTrail download tracking, day 7 reminders, and day 14 cleanup.
Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @bspeagle, your pull request is larger than the review limit of 150000 diff characters

@coderabbitai
Copy link

coderabbitai bot commented Dec 23, 2025

Walkthrough

This PR transforms a basic IAM key enforcement system into a fully automated key rotation solution featuring secure credential distribution via pre-signed URLs, DynamoDB lifecycle tracking, scheduled URL regeneration, and automated cleanup operations across a 14-day rotation window.

Changes

Cohort / File(s) Summary
Configuration & Ignores
.gitignore
Added Terragrunt-related ignore patterns (cache, terraform.tfvars, backend.tf, provider.tf, .terraform.lock.hcl, plan files) and sensitive config paths (account.hcl, config.hcl).
Core Lambda: Key Enforcement
lambda/access_key_enforcement/access_key_enforcement.py
Refactored key rotation flow to create new keys and store credentials in S3 with pre-signed URLs instead of disabling old keys. Added S3/DynamoDB client initialization, create_and_store_new_key() function, and updated notification payloads with download URL/expiry metadata.
Core Lambda: Download Tracking
lambda/download_tracker/download_tracker.py
New Lambda triggered by CloudTrail S3 GetObject events. Validates credentials, updates DynamoDB tracking record with download time/IP, deletes S3 object to enforce one-time access.
Core Lambda: URL Regeneration
lambda/url_regenerator/url_regenerator.py
New Lambda for daily regeneration of pre-signed URLs. Queries DynamoDB for expiring URLs, regenerates 7-day URLs for undownloaded credentials, sends reminder emails via SES, and updates tracking.
Core Lambda: Cleanup
lambda/cleanup/cleanup.py
New Lambda for daily cleanup of old keys. Scans DynamoDB for keys past deletion date, deletes IAM keys, updates tracking status, and publishes CloudWatch metrics.
Terraform IAM Module Core
terraform/iam/lambda.tf, terraform/iam/users.tf, terraform/iam/variables.tf, terraform/iam/outputs.tf
Renamed execution role to lambda_exec, expanded IAM policy with S3/DynamoDB/KMS permissions. Added environment variables for credentials bucket and tracking table. Added 6 new Terraform variables for Lambda source directories. Added outputs for test keys, bucket/table ARNs, and Lambda function ARNs.
Terraform IAM: New Infrastructure
terraform/iam/dynamodb.tf, terraform/iam/s3.tf, terraform/iam/cleanup_lambda.tf, terraform/iam/download_tracker_lambda.tf, terraform/iam/url_regenerator_lambda.tf
Created DynamoDB tracking table with composite key (PK/SK) and GSIs for status/expiration queries. Configured S3 credentials bucket with versioning, encryption, lifecycle rules. Added CloudTrail/EventBridge integration for S3 event tracking. Defined three new Lambda functions with execution roles, policies, logging, and scheduled triggers (daily at 2-3 AM UTC).
Terraform IAM: Provider Config
terraform/iam/provider.tf, terraform/iam/README.md
Removed provider version constraint block. Added comprehensive module README with architecture diagram, usage examples, and troubleshooting.
Terragrunt Root & Structure
terragrunt/root.terragrunt.hcl, terragrunt/README.md
New root Terragrunt config generating provider.tf and backend.tf, configuring S3 state backend with locking. Added comprehensive deployment guide and directory structure.
Terragrunt Environment Configuration
terragrunt/mvw-dw-nonprod/account.hcl.example, terragrunt/mvw-dw-nonprod/us-east-1/region.hcl, terragrunt/mvw-dw-nonprod/us-east-1/env.hcl, terragrunt/mvw-dw-nonprod/us-east-1/dev/iam-key-rotation/config.hcl.example, terragrunt/mvw-dw-nonprod/us-east-1/dev/iam-key-rotation/terragrunt.hcl
Added Terragrunt hierarchical configuration with account, region, environment, and service-specific layers. Defined IAM policy thresholds, sender email, exemption tags, test user configuration. Service-level terragrunt.hcl wires module source, environment variables, and input validation.
Documentation & Guides
README.md, DEPLOYMENT.md, docs/AUTOMATED_ROTATION_IMPLEMENTATION.md, docs/email-templates.md, docs/templates/readme-template.md, docs/templates/terraform-module-readme-template.md
Completely rewrote README with new feature set, architecture diagrams, and 14-day rotation workflow. Added DEPLOYMENT.md with end-to-end deployment steps and troubleshooting. New docs for implementation details, email templates with placeholders and examples, and reusable documentation templates.
Testing Infrastructure
tests/test_rotation_workflow.py, tests/README_WORKFLOW_TESTING.md
New test script for simulating 14-day rotation lifecycle by backdating DynamoDB timestamps and invoking Lambda functions. Includes interactive menu, record display, Day 7 reminder and Day 14 cleanup test scenarios.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant IAM as IAM Service
    participant Enforcement as Enforcement Lambda
    participant S3 as S3 Credentials
    participant DynamoDB as DynamoDB Tracking
    participant Email as Email/SES
    
    rect rgb(200, 220, 255)
    Note over Enforcement,Email: Day 0: Key Rotation Initiation
    User->>Enforcement: Key age threshold crossed
    Enforcement->>IAM: CreateAccessKey()
    IAM-->>Enforcement: new_key_id
    Enforcement->>S3: Store credentials JSON
    S3-->>Enforcement: upload_success
    Enforcement->>Enforcement: Generate 7-day pre-signed URL
    Enforcement->>DynamoDB: Create tracking record
    DynamoDB-->>Enforcement: record_created
    Enforcement->>Email: Send with download URL + old key info
    Email-->>User: Rotation notification
    end
    
    rect rgb(220, 240, 220)
    Note over User,S3: Day 1-7: Download Window
    User->>S3: GET credentials (pre-signed URL)
    S3->>S3: Track via CloudTrail
    S3-->>User: credentials.json
    end
    
    rect rgb(255, 240, 200)
    Note over Enforcement,Email: Day 7: URL Regeneration (no download)
    Note over Enforcement,Email: Automated daily check at 2 AM
    Enforcement->>DynamoDB: Query URL expirations
    DynamoDB-->>Enforcement: items with expired_urls
    loop For each user without download
        Enforcement->>S3: Verify object exists
        S3-->>Enforcement: object_exists
        Enforcement->>Enforcement: Generate new 7-day URL
        Enforcement->>DynamoDB: Update presigned_url + expiry
        Enforcement->>Email: Send Day 7 reminder
        Email-->>User: URL refresh notification
    end
    end
    
    rect rgb(255, 200, 200)
    Note over Enforcement,IAM: Day 14: Old Key Cleanup
    Note over Enforcement,IAM: Automated daily check at 3 AM
    Enforcement->>DynamoDB: Query past_deletion_dates
    DynamoDB-->>Enforcement: items_ready_for_cleanup
    loop For each old key
        Enforcement->>IAM: DeleteAccessKey(old_key_id)
        IAM-->>Enforcement: deleted
        Enforcement->>DynamoDB: Mark as cleaned_up
    end
    end
Loading

Estimated Code Review Effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly Related PRs

  • Fix outdated tests #7: Updates to test mocks align with the PR's refactoring of AWS client caching and initialization patterns in the enforcement Lambda module.
  • Add IAM credential rotation script #8: Implements overlapping key creation, S3 storage, and pre-signed URL generation functionality as the core rotation-and-store workflow in this PR.

Suggested Labels

codex

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The pull request title clearly summarizes the main change: a fully automated IAM key rotation system with Terragrunt multi-environment deployment, which accurately reflects the substantial feature additions and infrastructure changes throughout the changeset.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feature/terragrunt-multi-environment-deployment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 16

🧹 Nitpick comments (22)
.gitignore (1)

249-249: Remove redundant **/terraform.tfvars pattern.

Line 224 already ignores *.tfvars, making the **/terraform.tfvars pattern on line 249 redundant. Remove line 249 to keep the ignore rules clean.

🔎 Proposed removal
 # Terragrunt
 **/.terragrunt-cache/
-**/terraform.tfvars
 **/backend.tf
DEPLOYMENT.md (1)

113-115: Add language specifier to fenced code block.

The DNS TXT record example lacks a language specifier. While this is a minor formatting issue, adding txt or plaintext improves consistency.

🔎 Proposed fix
-```
+```txt
 _amazonses.yourcompany.com TXT "generated-verification-token"
</details>

</blockquote></details>
<details>
<summary>terragrunt/README.md (1)</summary><blockquote>

`7-18`: **Add language specifier to directory structure code block.**

The directory tree lacks a language specifier. Use `text` or `plaintext` for consistency with markdown linting.

<details>
<summary>🔎 Proposed fix</summary>

```diff
-```
+```text
 terragrunt/
 ├── root.terragrunt.hcl                    # Root configuration with remote state
lambda/download_tracker/download_tracker.py (4)

88-91: Use logging.exception for better error diagnostics.

When catching exceptions, logging.exception automatically includes the stack trace, which aids debugging in CloudWatch logs. The bare Exception catch is acceptable for Lambda resilience but consider using logging.exception.

🔎 Proposed fix
     except Exception as e:
-        error_msg = f"Error processing CloudTrail event: {str(e)}"
-        logger.error(error_msg)
+        error_msg = f"Error processing CloudTrail event: {e}"
+        logger.exception(error_msg)
         errors.append(error_msg)

159-161: Use logging.exception to capture stack traces.

Replace logger.error with logger.exception when handling exceptions to automatically include the traceback in logs.

🔎 Proposed fix
     except ClientError as e:
-        logger.error(f"DynamoDB error updating record for {username}: {e}")
+        logger.exception(f"DynamoDB error updating record for {username}: {e}")
         raise

176-178: Use logging.exception for S3 error logging.

Consistent with the other exception handlers, use logger.exception to capture the full traceback.

🔎 Proposed fix
     except ClientError as e:
-        logger.error(f"Error deleting S3 file {s3_key}: {e}")
+        logger.exception(f"Error deleting S3 file {s3_key}: {e}")
         raise

152-152: Consider using datetime.now(timezone.utc) for Python 3.12+ compatibility.

datetime.utcnow() is deprecated in Python 3.12. Since the Lambda runtime is Python 3.11, this works now but may cause issues when upgrading.

🔎 Proposed fix
-from datetime import datetime
+from datetime import datetime, timezone

# In update_tracking_record:
-                ":del_ts": datetime.utcnow().isoformat(),
+                ":del_ts": datetime.now(timezone.utc).isoformat(),
lambda/access_key_enforcement/access_key_enforcement.py (3)

305-398: Consider using logging.exception for better error diagnostics.

When catching exceptions in create_and_store_new_key, using logging.exception instead of logging.error will automatically include the stack trace, making debugging easier.

🔎 Proposed change
     except ClientError as e:
-        logger.error(f"Error creating new key for {username}: {e}")
+        logger.exception(f"Error creating new key for {username}: {e}")
         return None

110-117: Clarify timing comment for credential report polling.

Line 111 comments "keep short to avoid slow tests" with time.sleep(0.1), but Line 115 logs success after {attempt * 2} seconds, suggesting 2-second intervals were intended. The actual sleep is 0.1 seconds (100ms).

🔎 Proposed clarification
-            # Sleep briefly before retrying; keep short to avoid slow tests
-            time.sleep(0.1)
+            # Sleep briefly before retrying (0.1s for fast tests, increase if needed in production)
+            time.sleep(0.1)
             response = iam.get_credential_report()
             if "Content" in response:
                 logger.info(
-                    f"Credential report generated successfully after {attempt * 2} seconds"
+                    f"Credential report generated successfully after {attempt * 0.1:.1f} seconds"
                 )
                 break

354-355: Simplify URL expiration calculation.

The URL expiration is calculated as datetime.now().timestamp() + 604800 and then converted back to datetime for formatting. This can be simplified using timedelta.

🔎 Proposed simplification
+from datetime import datetime, timedelta
+
         # Calculate URL expiration time
-        url_expires = (datetime.now().timestamp() + 604800)
-        url_expires_str = datetime.fromtimestamp(url_expires).strftime("%Y-%m-%d %H:%M:%S UTC")
+        url_expires_dt = datetime.now() + timedelta(days=7)
+        url_expires = int(url_expires_dt.timestamp())
+        url_expires_str = url_expires_dt.strftime("%Y-%m-%d %H:%M:%S UTC")
terraform/iam/variables.tf (1)

59-94: New variables properly support multi-Lambda deployment.

The source directory variables enable flexible Lambda packaging across different execution contexts. The defaults with relative paths work for local development, and Terragrunt properly overrides these with absolute paths via TF_VAR_* environment variables.

Consider adding validation blocks for credential_retention_days to ensure it's a positive integer within expected bounds (e.g., 7-90 days).

🔎 Optional validation example
variable "credential_retention_days" {
  description = "Number of days to retain credentials before deleting old IAM key"
  type        = number
  default     = 14

  validation {
    condition     = var.credential_retention_days >= 7 && var.credential_retention_days <= 90
    error_message = "credential_retention_days must be between 7 and 90 days."
  }
}
terragrunt/root.terragrunt.hcl (1)

39-42: Consider relaxing AWS provider version constraint to allow minor version updates.

The constraint ~> 5.16.2 allows only patch updates within 5.16.x. Relaxing to ~> 5.16 would allow any 5.x release from 5.16 onwards (e.g., 5.17.0, 5.18.0) while still preventing major version jumps. Note that AWS provider v6.25.0 is now available; if compatibility allows, consider ~> 6.0 for access to the latest features and fixes.

lambda/url_regenerator/url_regenerator.py (8)

28-30: Validate required environment variables at startup.

S3_BUCKET and SENDER_EMAIL lack default values and will cause runtime errors if missing. Consider adding validation at module initialization to fail fast with a clear error message.

🔎 Proposed validation
 # Configuration from environment
 DYNAMODB_TABLE = os.environ.get("DYNAMODB_TABLE", "iam-key-rotation-tracking")
-S3_BUCKET = os.environ.get("S3_BUCKET")
-SENDER_EMAIL = os.environ.get("SENDER_EMAIL")
+S3_BUCKET = os.environ["S3_BUCKET"]  # Required, fail fast if missing
+SENDER_EMAIL = os.environ["SENDER_EMAIL"]  # Required, fail fast if missing

35-95: Consider using logging.exception for better error context.

Lines 66 and 81 use logging.error which omits the stack trace. Using logging.exception within exception handlers provides valuable debugging information.

🔎 Proposed improvement
     except ClientError as e:
-        logger.error(f"Error querying DynamoDB: {e}")
+        logger.exception("Error querying DynamoDB")
         return {
             "statusCode": 500,
             "body": json.dumps({"error": str(e)})
         }
     
     regenerated = 0
     errors = []
     
     for item in items:
         try:
             regenerate_url_and_notify(item)
             regenerated += 1
         except Exception as e:
             error_msg = f"Error regenerating URL for {item.get('username')}: {str(e)}"
-            logger.error(error_msg)
+            logger.exception(f"Error regenerating URL for {item.get('username')}")
             errors.append(error_msg)

109-109: Remove unused variable.

rotation_initiated is assigned but never used. Remove it to reduce cognitive load.

🔎 Proposed fix
     username = item.get("username")
     email = item.get("email")
     s3_key = item.get("s3_key")
     old_key_id = item.get("old_key_id")
-    rotation_initiated = item.get("rotation_initiated")
     old_key_deletion_date = item.get("old_key_deletion_date")

130-145: Use logging.exception for complete error context.

Lines 130 and 144 use logging.error, which omits stack traces. Consider using logging.exception within exception handlers.


147-174: Consider idempotency for URL regeneration.

If the Lambda is invoked multiple times (e.g., retry), the email_sent_count will increment and a new email record will be appended each time. Consider adding a condition to check if regeneration already occurred today to prevent duplicate emails.


189-189: Remove unused parameter.

The old_key_id parameter is not used in the email templates. Consider removing it from the function signature.


203-262: Consider parameterizing the support email address.

Line 254 hard-codes [email protected]. Consider making this configurable via an environment variable for better flexibility across environments.


288-306: Use logging.exception within exception handler.

Line 305 uses logging.error, which omits the stack trace. Using logging.exception provides better debugging context.

lambda/cleanup/cleanup.py (1)

32-97: Consider using logging.exception for better debugging.

Lines 62 and 78 use logging.error, which omits stack traces. Using logging.exception within exception handlers provides valuable debugging context, especially when troubleshooting production issues.

🔎 Proposed improvement
     except ClientError as e:
-        logger.error(f"Error querying DynamoDB: {e}")
+        logger.exception("Error querying DynamoDB")
         return {
             "statusCode": 500,
             "body": json.dumps({"error": str(e)})
         }
     
     deleted = 0
     failed = 0
     errors = []
     
     for item in items:
         try:
             delete_old_key(item)
             deleted += 1
         except Exception as e:
             error_msg = f"Error deleting key for {item.get('username')}: {str(e)}"
-            logger.error(error_msg)
+            logger.exception(f"Error deleting key for {item.get('username')}")
             errors.append(error_msg)
             failed += 1
terraform/iam/s3.tf (1)

75-120: Add lifecycle configuration for CloudTrail logs bucket.

The CloudTrail logs bucket lacks lifecycle management, which will cause logs to accumulate indefinitely and increase storage costs. Consider adding a lifecycle policy to expire or transition old logs to cheaper storage.

🔎 Proposed lifecycle configuration
+# Lifecycle policy for CloudTrail logs
+resource "aws_s3_bucket_lifecycle_configuration" "cloudtrail_logs" {
+  bucket = aws_s3_bucket.cloudtrail_logs.id
+
+  rule {
+    id     = "expire-old-logs"
+    status = "Enabled"
+
+    expiration {
+      days = 90  # Adjust based on retention requirements
+    }
+  }
+
+  rule {
+    id     = "transition-to-glacier"
+    status = "Enabled"
+
+    transition {
+      days          = 30
+      storage_class = "GLACIER"
+    }
+  }
+}

Based on learnings, ensure Terraform validation, formatting, linting, and security checks pass.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 94ab934 and 3522658.

📒 Files selected for processing (31)
  • .gitignore
  • DEPLOYMENT.md
  • README.md
  • docs/AUTOMATED_ROTATION_IMPLEMENTATION.md
  • docs/email-templates.md
  • docs/templates/readme-template.md
  • docs/templates/terraform-module-readme-template.md
  • lambda/access_key_enforcement/access_key_enforcement.py
  • lambda/cleanup/cleanup.py
  • lambda/download_tracker/download_tracker.py
  • lambda/url_regenerator/url_regenerator.py
  • terraform/iam/README.md
  • terraform/iam/cleanup_lambda.tf
  • terraform/iam/download_tracker_lambda.tf
  • terraform/iam/dynamodb.tf
  • terraform/iam/lambda.tf
  • terraform/iam/outputs.tf
  • terraform/iam/provider.tf
  • terraform/iam/s3.tf
  • terraform/iam/url_regenerator_lambda.tf
  • terraform/iam/users.tf
  • terraform/iam/variables.tf
  • terragrunt/README.md
  • terragrunt/mvw-dw-nonprod/account.hcl.example
  • terragrunt/mvw-dw-nonprod/us-east-1/dev/env.hcl
  • terragrunt/mvw-dw-nonprod/us-east-1/dev/iam-key-rotation/config.hcl.example
  • terragrunt/mvw-dw-nonprod/us-east-1/dev/iam-key-rotation/terragrunt.hcl
  • terragrunt/mvw-dw-nonprod/us-east-1/region.hcl
  • terragrunt/root.terragrunt.hcl
  • tests/README_WORKFLOW_TESTING.md
  • tests/test_rotation_workflow.py
💤 Files with no reviewable changes (1)
  • terraform/iam/provider.tf
🧰 Additional context used
📓 Path-based instructions (7)
**/*.md

📄 CodeRabbit inference engine (AGENTS.md)

**/*.md: Docs-only changes (Markdown) may skip tests/quality gates if no executable code is modified
Before review, update any relevant documentation

Files:

  • terraform/iam/README.md
  • DEPLOYMENT.md
  • tests/README_WORKFLOW_TESTING.md
  • terragrunt/README.md
  • docs/AUTOMATED_ROTATION_IMPLEMENTATION.md
  • docs/templates/terraform-module-readme-template.md
  • README.md
  • docs/templates/readme-template.md
  • docs/email-templates.md
**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.py: Always use Flake8 for linting the application. E501 errors can be ignored up to 120 lines.
Always use Black for formatting the application.
Always use Pyright or mypy for type checking the application.

Files:

  • lambda/url_regenerator/url_regenerator.py
  • lambda/cleanup/cleanup.py
  • lambda/access_key_enforcement/access_key_enforcement.py
  • tests/test_rotation_workflow.py
  • lambda/download_tracker/download_tracker.py
{scripts,lambda,tests}/**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

{scripts,lambda,tests}/**/*.py: Format Python code with Black before merging
Run Flake8 with max line length 120 and ignore E501,W503,E203
Use descriptive variables and function names in Python code
Write docstrings for all Python functions and modules

Files:

  • lambda/url_regenerator/url_regenerator.py
  • lambda/cleanup/cleanup.py
  • lambda/access_key_enforcement/access_key_enforcement.py
  • tests/test_rotation_workflow.py
  • lambda/download_tracker/download_tracker.py
{scripts,lambda}/**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

{scripts,lambda}/**/*.py: Run mypy (or pyright) for type checking on Python application code
Run Bandit security scanning on Python code
Before review, ensure type checking passes for Python application code

Files:

  • lambda/url_regenerator/url_regenerator.py
  • lambda/cleanup/cleanup.py
  • lambda/access_key_enforcement/access_key_enforcement.py
  • lambda/download_tracker/download_tracker.py
**/*.tf

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.tf: Format Terraform files using terraform fmt -recursive.
Validate Terraform configuration syntax using terraform validate.

Files:

  • terraform/iam/variables.tf
  • terraform/iam/cleanup_lambda.tf
  • terraform/iam/lambda.tf
  • terraform/iam/dynamodb.tf
  • terraform/iam/download_tracker_lambda.tf
  • terraform/iam/url_regenerator_lambda.tf
  • terraform/iam/s3.tf
  • terraform/iam/outputs.tf
  • terraform/iam/users.tf
terraform/iam/**/*.tf

📄 CodeRabbit inference engine (AGENTS.md)

terraform/iam/**/*.tf: Initialize and validate Terraform in terraform/iam before changes
Ensure Terraform files are formatted with terraform fmt -recursive
Run tflint on the Terraform directory
Run tfsec or checkov for Terraform security scanning
Generate and review a terraform plan before applying changes
Before review, ensure Terraform validation, formatting, linting, and security checks pass

Files:

  • terraform/iam/variables.tf
  • terraform/iam/cleanup_lambda.tf
  • terraform/iam/lambda.tf
  • terraform/iam/dynamodb.tf
  • terraform/iam/download_tracker_lambda.tf
  • terraform/iam/url_regenerator_lambda.tf
  • terraform/iam/s3.tf
  • terraform/iam/outputs.tf
  • terraform/iam/users.tf
tests/**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

tests/**/*.py: Run unit tests with pytest
Before review, ensure unit and integration tests pass

Files:

  • tests/test_rotation_workflow.py
🧠 Learnings (11)
📓 Common learnings
Learnt from: CR
Repo: tblakex01/iam-key-rotation PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-05T19:42:23.187Z
Learning: Applies to scripts/aws_iam_self_service_key_rotation.py : Scripts follow AWS best practices for key rotation.
Learnt from: CR
Repo: tblakex01/iam-key-rotation PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-18T22:45:45.046Z
Learning: Applies to terraform/iam/**/*.tf : Before review, ensure Terraform validation, formatting, linting, and security checks pass
Learnt from: CR
Repo: tblakex01/iam-key-rotation PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-18T22:45:45.046Z
Learning: Applies to terraform/iam/**/*.tf : Generate and review a terraform plan before applying changes
📚 Learning: 2025-09-18T22:45:45.046Z
Learnt from: CR
Repo: tblakex01/iam-key-rotation PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-18T22:45:45.046Z
Learning: Applies to terraform/iam/**/*.tf : Generate and review a terraform plan before applying changes

Applied to files:

  • terraform/iam/README.md
  • DEPLOYMENT.md
  • terragrunt/README.md
  • terraform/iam/variables.tf
  • terraform/iam/cleanup_lambda.tf
  • terragrunt/mvw-dw-nonprod/us-east-1/dev/iam-key-rotation/config.hcl.example
  • README.md
  • terragrunt/root.terragrunt.hcl
  • terraform/iam/lambda.tf
  • terraform/iam/dynamodb.tf
  • terraform/iam/download_tracker_lambda.tf
  • terragrunt/mvw-dw-nonprod/us-east-1/dev/iam-key-rotation/terragrunt.hcl
  • terraform/iam/url_regenerator_lambda.tf
  • terraform/iam/s3.tf
  • terraform/iam/outputs.tf
  • terraform/iam/users.tf
📚 Learning: 2025-09-18T22:45:45.046Z
Learnt from: CR
Repo: tblakex01/iam-key-rotation PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-18T22:45:45.046Z
Learning: Applies to terraform/iam/**/*.tf : Before review, ensure Terraform validation, formatting, linting, and security checks pass

Applied to files:

  • terraform/iam/README.md
  • terragrunt/README.md
  • terraform/iam/variables.tf
  • terraform/iam/cleanup_lambda.tf
  • terragrunt/mvw-dw-nonprod/us-east-1/dev/iam-key-rotation/config.hcl.example
  • README.md
  • terraform/iam/lambda.tf
  • terraform/iam/dynamodb.tf
  • terragrunt/mvw-dw-nonprod/us-east-1/dev/iam-key-rotation/terragrunt.hcl
  • terraform/iam/url_regenerator_lambda.tf
  • terraform/iam/s3.tf
  • terraform/iam/outputs.tf
  • terraform/iam/users.tf
📚 Learning: 2025-09-18T22:45:45.046Z
Learnt from: CR
Repo: tblakex01/iam-key-rotation PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-18T22:45:45.046Z
Learning: Applies to terraform/iam/**/*.tf : Initialize and validate Terraform in terraform/iam before changes

Applied to files:

  • terraform/iam/README.md
  • terraform/iam/variables.tf
  • terraform/iam/cleanup_lambda.tf
  • terragrunt/mvw-dw-nonprod/us-east-1/dev/iam-key-rotation/config.hcl.example
  • terraform/iam/lambda.tf
  • terraform/iam/dynamodb.tf
  • terragrunt/mvw-dw-nonprod/us-east-1/dev/iam-key-rotation/terragrunt.hcl
  • terraform/iam/url_regenerator_lambda.tf
  • terraform/iam/s3.tf
  • terraform/iam/outputs.tf
  • terraform/iam/users.tf
📚 Learning: 2025-08-05T19:42:23.187Z
Learnt from: CR
Repo: tblakex01/iam-key-rotation PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-05T19:42:23.187Z
Learning: Applies to scripts/aws_iam_self_service_key_rotation.py : Scripts follow AWS best practices for key rotation.

Applied to files:

  • terraform/iam/README.md
  • DEPLOYMENT.md
  • tests/README_WORKFLOW_TESTING.md
  • terragrunt/README.md
  • docs/AUTOMATED_ROTATION_IMPLEMENTATION.md
  • terraform/iam/cleanup_lambda.tf
  • terragrunt/mvw-dw-nonprod/us-east-1/dev/iam-key-rotation/config.hcl.example
  • README.md
  • docs/email-templates.md
  • lambda/cleanup/cleanup.py
  • terraform/iam/dynamodb.tf
  • lambda/access_key_enforcement/access_key_enforcement.py
  • terragrunt/mvw-dw-nonprod/us-east-1/dev/iam-key-rotation/terragrunt.hcl
  • tests/test_rotation_workflow.py
📚 Learning: 2025-09-18T22:45:45.046Z
Learnt from: CR
Repo: tblakex01/iam-key-rotation PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-18T22:45:45.046Z
Learning: Applies to terraform/iam/**/*.tf : Run tfsec or checkov for Terraform security scanning

Applied to files:

  • terragrunt/README.md
  • terraform/iam/variables.tf
  • terraform/iam/cleanup_lambda.tf
  • README.md
  • terraform/iam/lambda.tf
  • terraform/iam/dynamodb.tf
  • terraform/iam/s3.tf
  • terraform/iam/outputs.tf
📚 Learning: 2025-09-18T22:45:45.046Z
Learnt from: CR
Repo: tblakex01/iam-key-rotation PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-18T22:45:45.046Z
Learning: Applies to terraform/iam/**/*.tf : Ensure Terraform files are formatted with terraform fmt -recursive

Applied to files:

  • terraform/iam/cleanup_lambda.tf
  • terraform/iam/s3.tf
  • .gitignore
📚 Learning: 2025-09-18T22:45:45.046Z
Learnt from: CR
Repo: tblakex01/iam-key-rotation PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-18T22:45:45.046Z
Learning: Applies to terraform/iam/**/*.tf : Run tflint on the Terraform directory

Applied to files:

  • terraform/iam/cleanup_lambda.tf
📚 Learning: 2025-08-05T19:42:23.187Z
Learnt from: CR
Repo: tblakex01/iam-key-rotation PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-05T19:42:23.187Z
Learning: Applies to scripts/aws_iam_{self_service_password_reset,user_password_reset}.py : Password generation uses Python's `secrets` module for cryptographic security.

Applied to files:

  • README.md
📚 Learning: 2025-08-05T19:42:23.187Z
Learnt from: CR
Repo: tblakex01/iam-key-rotation PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-05T19:42:23.187Z
Learning: Applies to **/*.tf : Validate Terraform configuration syntax using `terraform validate`.

Applied to files:

  • .gitignore
📚 Learning: 2025-08-05T19:42:23.187Z
Learnt from: CR
Repo: tblakex01/iam-key-rotation PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-08-05T19:42:23.187Z
Learning: Applies to **/*.tf : Format Terraform files using `terraform fmt -recursive`.

Applied to files:

  • .gitignore
🧬 Code graph analysis (1)
lambda/url_regenerator/url_regenerator.py (2)
lambda/cleanup/cleanup.py (1)
  • lambda_handler (32-97)
lambda/download_tracker/download_tracker.py (1)
  • lambda_handler (29-103)
🪛 Checkov (3.2.334)
terraform/iam/cleanup_lambda.tf

[high] 82-100: Ensure AWS Lambda function is configured to validate code-signing

(CKV_AWS_272)

terraform/iam/download_tracker_lambda.tf

[high] 72-89: Ensure AWS Lambda function is configured to validate code-signing

(CKV_AWS_272)

terraform/iam/url_regenerator_lambda.tf

[high] 89-108: Ensure AWS Lambda function is configured to validate code-signing

(CKV_AWS_272)

terraform/iam/s3.tf

[medium] 76-86: Ensure that an S3 bucket has a lifecycle configuration

(CKV2_AWS_61)

🪛 GitHub Actions: AWS IAM Key Rotation Tests
tests/test_rotation_workflow.py

[error] 24-24: botocore.exceptions.ProfileNotFound: The config profile (dw-nonprod) could not be found

🪛 LanguageTool
docs/AUTOMATED_ROTATION_IMPLEMENTATION.md

[style] ~369-~369: Some style guides suggest that commas should set off the year in a month-day-year date.
Context: ...channel --- Created: December 23, 2025 Author: beepboop (Windsurf AI) ...

(MISSING_COMMA_AFTER_YEAR)

🪛 markdownlint-cli2 (0.18.1)
terraform/iam/README.md

138-138: Tables should be surrounded by blank lines

(MD058, blanks-around-tables)


146-146: Tables should be surrounded by blank lines

(MD058, blanks-around-tables)


155-155: Tables should be surrounded by blank lines

(MD058, blanks-around-tables)


165-165: Tables should be surrounded by blank lines

(MD058, blanks-around-tables)


172-172: Tables should be surrounded by blank lines

(MD058, blanks-around-tables)


179-179: Tables should be surrounded by blank lines

(MD058, blanks-around-tables)


345-345: Link fragments should be valid

(MD051, link-fragments)

DEPLOYMENT.md

113-113: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

tests/README_WORKFLOW_TESTING.md

10-10: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


15-15: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


135-135: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

terragrunt/README.md

7-7: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

docs/AUTOMATED_ROTATION_IMPLEMENTATION.md

147-147: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


196-196: Bare URL used

(MD034, no-bare-urls)

🪛 Ruff (0.14.10)
lambda/url_regenerator/url_regenerator.py

35-35: Unused function argument: event

(ARG001)


35-35: Unused function argument: context

(ARG001)


66-66: Use logging.exception instead of logging.error

Replace with exception

(TRY400)


79-79: Do not catch blind exception: Exception

(BLE001)


80-80: Use explicit conversion flag

Replace with conversion flag

(RUF010)


81-81: Use logging.exception instead of logging.error

Replace with exception

(TRY400)


109-109: Local variable rotation_initiated is assigned to but never used

Remove assignment to unused variable rotation_initiated

(F841)


130-130: Use logging.exception instead of logging.error

Replace with exception

(TRY400)


144-144: Use logging.exception instead of logging.error

Replace with exception

(TRY400)


173-173: Use logging.exception instead of logging.error

Replace with exception

(TRY400)


189-189: Unused function argument: old_key_id

(ARG001)


305-305: Use logging.exception instead of logging.error

Replace with exception

(TRY400)

lambda/cleanup/cleanup.py

32-32: Unused function argument: event

(ARG001)


32-32: Unused function argument: context

(ARG001)


62-62: Use logging.exception instead of logging.error

Replace with exception

(TRY400)


76-76: Do not catch blind exception: Exception

(BLE001)


77-77: Use explicit conversion flag

Replace with conversion flag

(RUF010)


78-78: Use logging.exception instead of logging.error

Replace with exception

(TRY400)


125-125: Use logging.exception instead of logging.error

Replace with exception

(TRY400)


152-152: Use logging.exception instead of logging.error

Replace with exception

(TRY400)


186-186: Use logging.exception instead of logging.error

Replace with exception

(TRY400)

lambda/access_key_enforcement/access_key_enforcement.py

390-394: Consider moving this statement to an else block

(TRY300)


397-397: Use logging.exception instead of logging.error

Replace with exception

(TRY400)

tests/test_rotation_workflow.py

64-64: Do not use bare except

(E722)


64-65: try-except-pass detected, consider logging the exception

(S110)


113-113: f-string without any placeholders

Remove extraneous f prefix

(F541)


152-152: Do not catch blind exception: Exception

(BLE001)


158-158: f-string without any placeholders

Remove extraneous f prefix

(F541)


180-180: f-string without any placeholders

Remove extraneous f prefix

(F541)


199-199: Do not catch blind exception: Exception

(BLE001)


241-241: Do not catch blind exception: Exception

(BLE001)

lambda/download_tracker/download_tracker.py

29-29: Unused function argument: context

(ARG001)


88-88: Do not catch blind exception: Exception

(BLE001)


89-89: Use explicit conversion flag

Replace with conversion flag

(RUF010)


90-90: Use logging.exception instead of logging.error

Replace with exception

(TRY400)


160-160: Use logging.exception instead of logging.error

Replace with exception

(TRY400)


177-177: Use logging.exception instead of logging.error

Replace with exception

(TRY400)

🔇 Additional comments (38)
.gitignore (1)

252-252: Reconsider ignoring .terraform.lock.hcl for reproducible deployments.

The .terraform.lock.hcl pattern on line 252 conflicts with the PR's stated goal of "reproducible builds." Lock files ensure consistent provider versions across environments and team members. For infrastructure-as-code, committing lock files is a best practice to prevent unexpected provider upgrades. Consider removing this ignore pattern or negating it with !**/.terraform.lock.hcl.

Can you confirm whether ignoring lock files aligns with your deployment strategy, or should these files be committed for reproducibility?

terragrunt/mvw-dw-nonprod/us-east-1/dev/env.hcl (1)

1-8: LGTM!

Simple and clean environment configuration file that correctly defines the dev environment label for the Terragrunt hierarchy.

terragrunt/mvw-dw-nonprod/account.hcl.example (1)

1-11: LGTM!

Well-documented example configuration template with clear placeholder values and helpful inline comments guiding users on what to update.

docs/templates/terraform-module-readme-template.md (1)

1-126: LGTM!

Well-structured template covering all essential sections for Terraform module documentation. The placeholder values and structure provide a good starting point for new modules.

DEPLOYMENT.md (1)

1-379: Comprehensive and well-structured deployment guide.

The phased approach (Infrastructure → Manual Configuration → Testing → Operations) with detailed CLI examples and troubleshooting guidance provides excellent operational documentation.

terragrunt/README.md (1)

1-288: Excellent Terragrunt deployment documentation.

Thorough coverage of multi-environment deployment, test environment setup, configuration management, and troubleshooting. The security notes section appropriately warns against using real user emails in test environments.

lambda/download_tracker/download_tracker.py (1)

29-103: Well-structured Lambda handler with good security practices.

The one-time download enforcement (deleting S3 file after download) and DynamoDB tracking pattern are correctly implemented. The event validation and error aggregation provide good operational visibility.

README.md (2)

176-208: Excellent documentation of the automated rotation workflow.

The day-by-day breakdown (Day 0 initiation → Day 1-7 download window → Day 7 reminder → Day 14 cleanup) clearly explains the lifecycle and integrates well with the DynamoDB tracking model.


296-319: Terragrunt deployment instructions updated appropriately.

The transition from direct Terraform commands to Terragrunt-based deployment aligns with the new multi-environment architecture introduced in this PR.

terragrunt/mvw-dw-nonprod/us-east-1/region.hcl (1)

1-9: LGTM!

Clean region configuration following Terragrunt conventions.

terragrunt/mvw-dw-nonprod/us-east-1/dev/iam-key-rotation/config.hcl.example (2)

19-21: Clarify threshold values in example configuration.

The example shows thresholds of 30/45/60 days, but the PR summary mentions "ultra-fast thresholds (0/1/2 days)" for the test environment.

Questions:

  • Are these intentionally different to show "safer" defaults vs. actual test values?
  • Should the example reflect the actual test configuration mentioned in the PR summary?
  • Consider adding a comment explaining the difference between example values and actual test values.

6-56: Remember to update placeholder values before deployment.

The configuration contains several placeholder values that must be updated:

  • Line 11: Account (currently "your-account-name")
  • Line 23: sender_email (currently "[email protected]")
  • Lines 31, 39, 48: User email addresses

These are clearly marked with comments, which is good practice.

tests/README_WORKFLOW_TESTING.md (1)

1-134: Excellent testing documentation!

The workflow testing guide is comprehensive and well-structured, covering prerequisites, scenarios, expected outcomes, and troubleshooting.

docs/email-templates.md (1)

1-205: Comprehensive email template documentation.

The documentation clearly defines all email variants (Day 0 Initial, Day 7 Reminder, Plain Text), template variables, and email types. The emphasis on one-time download constraints is appropriate for security compliance.

docs/AUTOMATED_ROTATION_IMPLEMENTATION.md (1)

1-371: Outstanding implementation documentation!

This document provides comprehensive coverage of the automated rotation system including:

  • Architecture and infrastructure components
  • Deployment steps and verification procedures
  • Testing scenarios and troubleshooting
  • Security features and cost estimates
  • Migration notes and support contacts

The documentation will be invaluable for deployment, maintenance, and onboarding.

terraform/iam/users.tf (1)

5-5: Update README.md and verify Terragrunt configuration provides user_tags for all users.

The code currently defines user_tags as map(string) (required), but the README.md documents it as optional(map(string)). Additionally, the variable-level defaults on line 10 only apply to the hardcoded userA and userB entries—if a custom user_info is provided via Terragrunt without user_tags for any user, Terraform will error since the field is required.

Ensure the actual config.hcl (currently missing; only config.hcl.example exists) includes user_tags for all users, and update the README.md to reflect that user_tags is required, not optional.

terraform/iam/dynamodb.tf (1)

6-78: Well-structured DynamoDB table with good security defaults.

The table configuration includes essential security features: PAY_PER_REQUEST billing for cost efficiency, point-in-time recovery, server-side encryption, and TTL for automatic record cleanup. The composite key design (PK/SK) follows single-table design patterns.

Consider using a customer-managed KMS key (kms_key_arn) for enhanced key management control if organizational policies require it.

terragrunt/root.terragrunt.hcl (1)

110-115: After-hook cleanup is functional.

The cache cleanup on destroy is a good practice. Minor optimization: using + instead of ; would batch the rm operations for better performance on large caches, though the current approach works correctly.

terraform/iam/download_tracker_lambda.tf (1)

72-89: Lambda configuration is appropriate for the use case.

The 30-second timeout and 256MB memory are reasonable for download tracking operations. The static analysis hint about code-signing (CKV_AWS_272) is a valid security enhancement for supply chain protection, but may be overkill for internal automation Lambdas. Consider implementing if your organization requires it.

terraform/iam/cleanup_lambda.tf (1)

110-133: EventBridge scheduling is well-configured.

The 3 AM UTC schedule runs after the URL regenerator (2 AM), providing proper sequencing for the rotation workflow. Lambda permissions are correctly configured.

terraform/iam/url_regenerator_lambda.tf (1)

88-108: Lambda configuration looks appropriate.

The environment variables provide necessary context. Consider whether CREDENTIAL_RETENTION_DAYS should be passed here as well (for consistency with cleanup Lambda), or verify the 7-day URL window is acceptable as a hardcoded constant in the Lambda code.

terraform/iam/outputs.tf (2)

66-86: Test user access key outputs are properly secured.

Separating access key IDs (non-sensitive) from secrets (marked sensitive = true) is the correct approach. The secrets won't appear in plan output or logs but remain in state for testing workflows.

⚠️ Ensure the Terraform state backend is properly secured since it contains the actual secret values.


126-139: Comprehensive system summary output is helpful for operational visibility.

This consolidated output makes it easy to verify deployment and reference key resource names. Good practice for complex multi-resource systems.

terragrunt/mvw-dw-nonprod/us-east-1/dev/iam-key-rotation/terragrunt.hcl (3)

1-29: LGTM: Clean Terragrunt configuration structure.

The hierarchical configuration loading and sender_email validation are well-implemented. The use of get_repo_root() ensures proper path resolution across environments.


31-46: LGTM: Proper Lambda source path configuration.

The use of get_repo_root() for Lambda source directories resolves packaging issues in multi-environment deployments by providing absolute paths. This aligns with the PR's goal of fixing Lambda packaging via absolute repo-root paths.


48-65: LGTM: Comprehensive input forwarding.

All necessary configuration parameters are properly forwarded from the service-specific config to the Terraform module without hardcoding values.

tests/test_rotation_workflow.py (4)

31-68: LGTM: Helper functions are well-structured.

The bare except on line 64 is acceptable here as a fallback for timestamp formatting in display logic.


71-125: LGTM: Day 7 reminder test logic is sound.

The test correctly simulates expiring URLs by setting current_url_expires to today's date, then verifies Lambda execution and DynamoDB updates.


128-202: LGTM: Day 14 cleanup test properly validates key deletion.

The test correctly backdates the deletion timestamp and verifies that the old IAM key is removed. Exception handling ensures test resilience when keys don't exist.


205-244: LGTM: Clean interactive test runner.

The menu-driven interface and exception handling make this test script easy to use for manual workflow validation.

lambda/cleanup/cleanup.py (3)

1-30: LGTM: Clean Lambda initialization with sensible defaults.

Environment configuration includes appropriate defaults for both DynamoDB table name and retention period.


100-153: LGTM: Robust key deletion with proper error handling.

The NoSuchEntity handling (line 122-123) correctly treats already-deleted keys as a warning rather than an error. The status logic differentiates between completed downloads and expired credentials.


156-187: Good pattern: Non-critical metrics don't block cleanup.

Line 187 correctly avoids raising exceptions for metrics failures, preventing metrics issues from disrupting the core cleanup operation.

terraform/iam/s3.tf (5)

1-17: LGTM: Properly tagged credentials bucket.

The use of account ID in the bucket name ensures global uniqueness, and the Sensitivity = "High" tag correctly identifies the critical nature of this resource.


19-73: LGTM: Comprehensive security controls on credentials bucket.

The combination of public access blocking, versioning, encryption, and aggressive lifecycle policies (1-day retention) aligns well with the one-time download security model.


122-143: LGTM: CloudTrail properly configured for download tracking.

The ReadOnly event selector correctly captures GetObject events for the credentials path, and the dependency on the bucket policy ensures proper resource ordering.


145-179: LGTM: Complete download detection pipeline.

The EventBridge rule and target correctly wire CloudTrail S3 events to the download tracker Lambda, with proper invocation permissions.


181-235: LGTM: Least-privilege bucket policy with transport security.

The policy correctly restricts access to specific Lambda execution roles and enforces HTTPS-only transport via the aws:SecureTransport condition.

@@ -0,0 +1,19 @@
# Legal AI Virtual Courtroom Documentation Template
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Update template title to match project.

The template title references "Legal AI Virtual Courtroom" but this is an IAM Key Rotation project. Update the title to reflect the correct project.

🔎 Proposed fix
-# Legal AI Virtual Courtroom Documentation Template
+# IAM Key Rotation Documentation Template
📝 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.

Suggested change
# Legal AI Virtual Courtroom Documentation Template
# IAM Key Rotation Documentation Template
🤖 Prompt for AI Agents
In docs/templates/readme-template.md around lines 1 to 1, the top-level title
incorrectly reads "Legal AI Virtual Courtroom Documentation Template"; update
that single header line to the correct project title, e.g., "IAM Key Rotation
Documentation Template" (or another exact project name), ensuring the leading
markdown heading symbol (#) is preserved and no other content is changed.

Comment on lines +77 to +79
S3_BUCKET = os.environ.get("S3_BUCKET")
DYNAMODB_TABLE = os.environ.get("DYNAMODB_TABLE")
CREDENTIAL_RETENTION_DAYS = int(os.environ.get("CREDENTIAL_RETENTION_DAYS", "14"))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add validation for required environment variables.

The new environment variables S3_BUCKET and DYNAMODB_TABLE are required for the rotation workflow but have no validation. If these are not set, the Lambda will fail at runtime when trying to access S3 or DynamoDB.

🔎 Proposed fix to validate required environment variables
 S3_BUCKET = os.environ.get("S3_BUCKET")
 DYNAMODB_TABLE = os.environ.get("DYNAMODB_TABLE")
 CREDENTIAL_RETENTION_DAYS = int(os.environ.get("CREDENTIAL_RETENTION_DAYS", "14"))
+
+# Validate required environment variables
+if not S3_BUCKET:
+    raise ValueError("S3_BUCKET environment variable is required")
+if not DYNAMODB_TABLE:
+    raise ValueError("DYNAMODB_TABLE environment variable is required")
📝 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.

Suggested change
S3_BUCKET = os.environ.get("S3_BUCKET")
DYNAMODB_TABLE = os.environ.get("DYNAMODB_TABLE")
CREDENTIAL_RETENTION_DAYS = int(os.environ.get("CREDENTIAL_RETENTION_DAYS", "14"))
S3_BUCKET = os.environ.get("S3_BUCKET")
DYNAMODB_TABLE = os.environ.get("DYNAMODB_TABLE")
CREDENTIAL_RETENTION_DAYS = int(os.environ.get("CREDENTIAL_RETENTION_DAYS", "14"))
# Validate required environment variables
if not S3_BUCKET:
raise ValueError("S3_BUCKET environment variable is required")
if not DYNAMODB_TABLE:
raise ValueError("DYNAMODB_TABLE environment variable is required")
🤖 Prompt for AI Agents
In lambda/access_key_enforcement/access_key_enforcement.py around lines 77 to
79, the S3_BUCKET and DYNAMODB_TABLE environment variables are used but not
validated; update the top-level initialization to validate required env vars and
fail fast: read S3_BUCKET and DYNAMODB_TABLE from os.environ, and if either is
missing or empty raise a clear exception (RuntimeError or ValueError) with a
descriptive message so the Lambda fails at deployment/init instead of runtime;
also guard parsing CREDENTIAL_RETENTION_DAYS with a try/except or defaulting
logic to ensure non-integer values produce a clear error.

Comment on lines 237 to +289
if key_age >= DISABLE_THRESHOLD:
metrics["expired_keys"] += 1

if AUTO_DISABLE:
disable_key(username, key_id)
metrics["disabled_keys"] += 1

# Create new key and initiate automated rotation
new_key_data = create_and_store_new_key(username, key_id, email)
if new_key_data:
notifications.append(
{
"username": username,
"email": email,
"key_id": key_id,
"old_key_id": key_id,
"age": key_age,
"action": "disabled",
"action": "rotated",
"severity": "critical",
"download_url": new_key_data["download_url"],
"url_expires": new_key_data["url_expires"],
}
)
else:

elif key_age >= URGENT_THRESHOLD:
metrics["urgent_keys"] += 1
# Create new key for urgent rotations
new_key_data = create_and_store_new_key(username, key_id, email)
if new_key_data:
notifications.append(
{
"username": username,
"email": email,
"key_id": key_id,
"old_key_id": key_id,
"age": key_age,
"action": "expired",
"severity": "critical",
"action": "rotated",
"severity": "high",
"download_url": new_key_data["download_url"],
"url_expires": new_key_data["url_expires"],
}
)

elif key_age >= URGENT_THRESHOLD:
metrics["urgent_keys"] += 1
notifications.append(
{
"username": username,
"email": email,
"key_id": key_id,
"age": key_age,
"action": "urgent",
"severity": "high",
}
)

elif key_age >= WARNING_THRESHOLD:
metrics["warning_keys"] += 1
notifications.append(
{
"username": username,
"email": email,
"key_id": key_id,
"age": key_age,
"action": "warning",
"severity": "medium",
}
)
# Create new key for warning rotations
new_key_data = create_and_store_new_key(username, key_id, email)
if new_key_data:
notifications.append(
{
"username": username,
"email": email,
"old_key_id": key_id,
"age": key_age,
"action": "rotated",
"severity": "medium",
"download_url": new_key_data["download_url"],
"url_expires": new_key_data["url_expires"],
}
)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, find the file and examine the threshold constants
rg -n "DISABLE_THRESHOLD|URGENT_THRESHOLD|WARNING_THRESHOLD" lambda/access_key_enforcement/access_key_enforcement.py -B2 -A2

Repository: tblakex01/iam-key-rotation

Length of output: 1000


🏁 Script executed:

# Look for the create_and_store_new_key function implementation
rg -n "def create_and_store_new_key" lambda/access_key_enforcement/ -A30

Repository: tblakex01/iam-key-rotation

Length of output: 3213


🏁 Script executed:

# Search for duplicate key prevention or tracking logic in DynamoDB
rg -n "duplicate|already.*rotated|existing.*rotation|tracking" lambda/access_key_enforcement/access_key_enforcement.py -B2 -A2

Repository: tblakex01/iam-key-rotation

Length of output: 214


🏁 Script executed:

# Check for any rotation policy documentation or comments near the thresholds
rg -n "rotation.*policy|intended.*behavior|early.*rotation" lambda/access_key_enforcement/access_key_enforcement.py -B3 -A3

Repository: tblakex01/iam-key-rotation

Length of output: 484


🏁 Script executed:

# Search for DynamoDB tracking logic in the entire lambda directory
rg -n "dynamodb|put_item|Table\(" lambda/access_key_enforcement/ -B2 -A5

Repository: tblakex01/iam-key-rotation

Length of output: 3049


🏁 Script executed:

# Look for any conditional logic or comments about why keys are created at all thresholds
rg -n "rotation.*status|tracked|previous" lambda/access_key_enforcement/access_key_enforcement.py -B3 -A3

Repository: tblakex01/iam-key-rotation

Length of output: 52


🏁 Script executed:

# Check the actual structure of the notification dictionary to understand what's happening at each level
rg -n "action.*rotated|action.*notification" lambda/access_key_enforcement/access_key_enforcement.py -B5 -A2

Repository: tblakex01/iam-key-rotation

Length of output: 1140


🏁 Script executed:

# Search for any existing key tracking to prevent duplicates within a single run
rg -n "already.*created|key.*exists|check.*existing" lambda/access_key_enforcement/access_key_enforcement.py -B3 -A3

Repository: tblakex01/iam-key-rotation

Length of output: 52


🏁 Script executed:

# Look at the complete DynamoDB tracking schema to understand the data structure
rg -n "TABLE|rotation_timestamp|ROTATION#" lambda/access_key_enforcement/access_key_enforcement.py -B5 -A10

Repository: tblakex01/iam-key-rotation

Length of output: 2329


🏁 Script executed:

# Check if there's any git history or comments explaining why all thresholds create keys
cat lambda/access_key_enforcement/access_key_enforcement.py | head -50

Repository: tblakex01/iam-key-rotation

Length of output: 1210


🏁 Script executed:

# Verify the complete create_and_store_new_key function to see if there's any duplicate prevention
rg -n "def create_and_store_new_key" lambda/access_key_enforcement/access_key_enforcement.py -A60 | head -70

Repository: tblakex01/iam-key-rotation

Length of output: 2636


🏁 Script executed:

# Check if there's any query logic that checks for existing rotations for the same key
rg -n "query|get_item|scan|Query" lambda/access_key_enforcement/access_key_enforcement.py -B3 -A5

Repository: tblakex01/iam-key-rotation

Length of output: 52


🏁 Script executed:

# Look at the complete main loop to see the exact flow of key processing
rg -n "for key_id|process_key|key_metadata" lambda/access_key_enforcement/access_key_enforcement.py -B5 -A8

Repository: tblakex01/iam-key-rotation

Length of output: 1418


🏁 Script executed:

# Check if there's any logic checking if a rotation was already initiated for a key
sed -n '200,300p' lambda/access_key_enforcement/access_key_enforcement.py

Repository: tblakex01/iam-key-rotation

Length of output: 3511


🏁 Script executed:

# Final check: See if there's any other code path that prevents key creation at WARNING level
rg -n "WARNING_THRESHOLD|75" lambda/access_key_enforcement/access_key_enforcement.py

Repository: tblakex01/iam-key-rotation

Length of output: 249


Implementation creates new keys at all three thresholds, contradicting the stated policy.

The module docstring states "Sends notifications at 75 and 85 days, disables keys at 90+ days", but the code calls create_and_store_new_key() at all three thresholds (lines 237, 255, 273). This means users receive new credentials at 75 days (WARNING_THRESHOLD), which is 15 days earlier than the policy indicates.

Additionally, there is no duplicate key prevention logic. If the Lambda executes multiple times before a user downloads their credentials, create_and_store_new_key() will be called repeatedly for the same old key, creating multiple new credentials. The DynamoDB tracking (table.put_item()) records rotations but is never queried to check if a rotation is already in progress for a given key_id.

Required clarification: Determine whether keys should be created at all three thresholds (current behavior) or only at DISABLE_THRESHOLD, with notifications-only logic at WARNING and URGENT thresholds (as documented). If early rotation at WARNING level is intentional, add duplicate prevention by querying DynamoDB before creating new keys, or use a flag in the DynamoDB item to track rotation state within a single execution.

🤖 Prompt for AI Agents
In lambda/access_key_enforcement/access_key_enforcement.py around lines 237 to
289, the code currently calls create_and_store_new_key() at WARNING, URGENT and
DISABLE thresholds which contradicts the module docstring and also lacks
duplicate-rotation prevention; change the logic to only create new keys at
DISABLE_THRESHOLD (leave WARNING and URGENT to send notifications only), and add
idempotency checks before creating a key by querying the DynamoDB rotations
table for an existing in-progress rotation for the same key_id (or a rotation
flag on the item); if no in-progress rotation exists, create the new key and
immediately atomically write a rotation record/flag to DynamoDB (so repeated
Lambda runs don’t create duplicates), and ensure any DynamoDB put_item uses a
conditional write (or transaction) to prevent race conditions.

Comment on lines +114 to +131
# Check if S3 file still exists (may have been downloaded and deleted)
try:
s3.head_object(Bucket=S3_BUCKET, Key=s3_key)
logger.info(f"S3 file exists for {username}: {s3_key}")
except ClientError as e:
if e.response['Error']['Code'] == '404':
logger.warning(f"S3 file already deleted for {username}: {s3_key}. Skipping reminder.")
# Update DynamoDB to reflect file was already downloaded/deleted
table.update_item(
Key={"PK": item["PK"], "SK": item["SK"]},
UpdateExpression="SET #st = :status",
ExpressionAttributeNames={"#st": "status"},
ExpressionAttributeValues={":status": "downloaded"}
)
return
else:
logger.error(f"Error checking S3 file existence: {e}")
raise
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for status value assignments across Lambda functions
rg -n 'status\s*[=:]\s*["\']' lambda/ --type py

Repository: tblakex01/iam-key-rotation

Length of output: 138


🏁 Script executed:

#!/bin/bash
# Also search for downloaded boolean or attribute usage
rg -n 'downloaded' lambda/ --type py -C 2

Repository: tblakex01/iam-key-rotation

Length of output: 6598


🏁 Script executed:

#!/bin/bash
# Check the cleanup.py file specifically at the mentioned lines
cat -n lambda/cleanup/cleanup.py | head -150 | tail -30

Repository: tblakex01/iam-key-rotation

Length of output: 1306


Set the downloaded boolean attribute to True instead of setting status to "downloaded".

Line 126 sets status = "downloaded" when the S3 file is missing, but this is inconsistent with the workflow. The downloaded boolean attribute is used throughout (initialized in access_key_enforcement.py, filtered in url_regenerator.py, and checked in cleanup.py), while final status values in cleanup.py are determined by the boolean: "completed" if downloaded, "expired_no_download" if not. Set downloaded = :true to align with how download_tracker.py marks actual downloads and let cleanup.py determine the final status value.

🧰 Tools
🪛 Ruff (0.14.10)

130-130: Use logging.exception instead of logging.error

Replace with exception

(TRY400)

🤖 Prompt for AI Agents
In lambda/url_regenerator/url_regenerator.py around lines 114 to 131, the code
currently updates DynamoDB with UpdateExpression "SET #st = :status" to set
status = "downloaded" when the S3 object is missing; change this to set the
boolean downloaded attribute to True instead. Replace the UpdateExpression and
ExpressionAttributeValues so the request sets downloaded = :true (e.g.,
UpdateExpression="SET downloaded = :true" and
ExpressionAttributeValues={":true": True}) and remove the status update so
downstream cleanup logic can derive final status from the boolean.

Comment on lines +102 to +118
subgraph "Lambda Functions"
Enforcement[🔧 Enforcement Lambda<br/>Creates New Keys]
DownloadTracker[� Download Tracker<br/>S3 Event Monitoring]
URLRegenerator[� URL Regenerator<br/>Day 7 Reminders]
Cleanup[🗑️ Cleanup Lambda<br/>Day 14 Deletion]
end
subgraph "IAM Resources"
Users[👥 IAM Users]
Keys[🔑 Access Keys]
Policies[📋 Policies]
subgraph "Storage & Data"
S3Creds[�️ S3 Credentials<br/>Encrypted Keys]
S3Trail[�️ S3 CloudTrail<br/>Audit Logs]
DynamoDB[(� DynamoDB<br/>Rotation Tracking)]
end
subgraph "Data & Reports"
CredReport[📄 Credential Report]
S3[🗄️ S3 Bucket<br/>Audit Logs]
subgraph "Event Processing"
EventBridge[⏰ EventBridge Rules<br/>Scheduled Triggers]
CloudTrail[� CloudTrail<br/>S3 Data Events]
end
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

cat -n README.md | sed -n '102,118p'

Repository: tblakex01/iam-key-rotation

Length of output: 906


🏁 Script executed:

# Check the raw bytes of the emoji characters to identify encoding issues
head -n 118 README.md | tail -n 17 | od -c | head -100

Repository: tblakex01/iam-key-rotation

Length of output: 110


🏁 Script executed:

# Use a more targeted approach - extract the exact section and check for valid UTF-8
python3 << 'EOF'
with open('README.md', 'r', encoding='utf-8') as f:
    lines = f.readlines()
    for i, line in enumerate(lines[101:119], start=102):
        # Print line with character codes
        print(f"Line {i}: {repr(line)}")
        # Check for replacement characters (U+FFFD) or broken unicode
        if '\ufffd' in line or '?' in line:
            print(f"  ⚠️ Potential encoding issue detected")
EOF

Repository: tblakex01/iam-key-rotation

Length of output: 156


Fix broken emoji characters in Mermaid diagram.

The emoji characters on lines 104–105, 110–112, and 117 are corrupted in the file (displaying as ), while others like 🔧, 🗑️, and render correctly. Replace the malformed emojis with valid Unicode sequences or use text labels for consistency and cross-platform compatibility.

Comment on lines +150 to +152
S3_BUCKET = "iam-credentials-${data.aws_caller_identity.current.account_id}"
DYNAMODB_TABLE = "iam-key-rotation-tracking"
CREDENTIAL_RETENTION_DAYS = var.credential_retention_days
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Use resource attributes for environment variables.

Other Lambda configs (cleanup, url_regenerator) reference aws_dynamodb_table.key_rotation_tracking.name directly. The enforcement Lambda should follow the same pattern for consistency.

🔎 Proposed fix
       S3_BUCKET              = "iam-credentials-${data.aws_caller_identity.current.account_id}"
-      DYNAMODB_TABLE         = "iam-key-rotation-tracking"
+      S3_BUCKET              = aws_s3_bucket.credentials.id
+      DYNAMODB_TABLE         = aws_dynamodb_table.key_rotation_tracking.name
       CREDENTIAL_RETENTION_DAYS = var.credential_retention_days

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In terraform/iam/lambda.tf around lines 150 to 152, the Lambda env vars use
hardcoded S3 bucket and DynamoDB table names; update them to reference the
actual Terraform resources for consistency (e.g., replace the literal
"iam-credentials-${data.aws_caller_identity.current.account_id}" with the
aws_s3_bucket resource attribute such as aws_s3_bucket.iam_credentials.bucket,
and replace "iam-key-rotation-tracking" with the DynamoDB table resource
attribute aws_dynamodb_table.key_rotation_tracking.name), leaving
CREDENTIAL_RETENTION_DAYS as is.


<div align="center">

**[ [Back to Top](#-iam-key-rotation-enforcement-module) ]**
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Fix broken anchor link.

The link fragment #-iam-key-rotation-enforcement-module doesn't match the actual heading at line 1 (# 🔧 IAM Key Rotation Automation Module). The anchor should be #-iam-key-rotation-automation-module.

🔎 Proposed fix
-**[ [Back to Top](#-iam-key-rotation-enforcement-module) ]**
+**[ [Back to Top](#-iam-key-rotation-automation-module) ]**
📝 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.

Suggested change
**[ [Back to Top](#-iam-key-rotation-enforcement-module) ]**
**[ [Back to Top](#-iam-key-rotation-automation-module) ]**
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)

345-345: Link fragments should be valid

(MD051, link-fragments)

🤖 Prompt for AI Agents
In terraform/iam/README.md around line 345, the "Back to Top" anchor uses the
incorrect fragment `#-iam-key-rotation-enforcement-module`; update the link
fragment to match the actual heading `# 🔧 IAM Key Rotation Automation Module`
by changing it to `#-iam-key-rotation-automation-module` so the anchor correctly
navigates to the top heading.

Comment on lines +33 to +41
{
Effect = "Allow"
Action = [
"logs:CreateLogGroup",
"logs:CreateLogStream",
"logs:PutLogEvents"
]
Resource = "arn:aws:logs:*:*:*"
},
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Scope CloudWatch Logs permissions to specific log group.

Consistent with other Lambda policies, the logs resource should be scoped to /aws/lambda/iam-key-url-regenerator instead of arn:aws:logs:*:*:*.

🤖 Prompt for AI Agents
In terraform/iam/url_regenerator_lambda.tf around lines 33 to 41, the CloudWatch
Logs permission is currently scoped to the wildcard "arn:aws:logs:*:*:*"; change
the Resource to target the specific log group for this Lambda (the
/aws/lambda/iam-key-url-regenerator log group) instead of the global wildcard —
e.g. use the log-group ARN pattern
arn:aws:logs:*:*:log-group:/aws/lambda/iam-key-url-regenerator:* or interpolate
region/account data (data.aws_caller_identity.current.account_id and var.region)
so the policy only grants CreateLogGroup/CreateLogStream/PutLogEvents on
/aws/lambda/iam-key-url-regenerator.

Comment on lines +37 to 43
# Create access keys for test users (needed for testing key rotation enforcement)
resource "aws_iam_access_key" "this" {
for_each = var.user_info
user = aws_iam_user.this[each.key].name

depends_on = [aws_iam_user.this]
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Critical: Access key secrets are not captured or output.

The aws_iam_access_key resource creates access keys for test users, but the SecretAccessKey attribute is not output or stored anywhere. Once Terraform creates the key, the secret is lost and cannot be retrieved, making these keys unusable.

Required actions:

  1. Either output the secret keys to a secure location (e.g., AWS Secrets Manager, SSM Parameter Store), or
  2. Document that these keys are intentionally disposable and will be rotated by the enforcement Lambda, or
  3. Remove this resource if initial keys should be created through a different mechanism
💡 Option 1: Store secrets in SSM Parameter Store
 # Create access keys for test users (needed for testing key rotation enforcement)
 resource "aws_iam_access_key" "this" {
   for_each = var.user_info
   user     = aws_iam_user.this[each.key].name
 
   depends_on = [aws_iam_user.this]
 }
+
+# Store secret keys in SSM Parameter Store (encrypted)
+resource "aws_ssm_parameter" "access_key_secret" {
+  for_each = var.user_info
+  name     = "/iam/test-users/${each.key}/secret-access-key"
+  type     = "SecureString"
+  value    = aws_iam_access_key.this[each.key].secret
+  
+  tags = merge(
+    var.common_tags,
+    {
+      Name = "Secret key for ${each.key}"
+    }
+  )
+}

Option 2: Output to console (less secure, only for testing)

+output "test_user_access_keys" {
+  description = "Access keys for test users (SECRET - handle with care)"
+  sensitive   = true
+  value = {
+    for username, user in var.user_info : username => {
+      access_key_id     = aws_iam_access_key.this[username].id
+      secret_access_key = aws_iam_access_key.this[username].secret
+    }
+  }
+}
📝 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.

Suggested change
# Create access keys for test users (needed for testing key rotation enforcement)
resource "aws_iam_access_key" "this" {
for_each = var.user_info
user = aws_iam_user.this[each.key].name
depends_on = [aws_iam_user.this]
}
# Create access keys for test users (needed for testing key rotation enforcement)
resource "aws_iam_access_key" "this" {
for_each = var.user_info
user = aws_iam_user.this[each.key].name
depends_on = [aws_iam_user.this]
}
# Store secret keys in SSM Parameter Store (encrypted)
resource "aws_ssm_parameter" "access_key_secret" {
for_each = var.user_info
name = "/iam/test-users/${each.key}/secret-access-key"
type = "SecureString"
value = aws_iam_access_key.this[each.key].secret
tags = merge(
var.common_tags,
{
Name = "Secret key for ${each.key}"
}
)
}
Suggested change
# Create access keys for test users (needed for testing key rotation enforcement)
resource "aws_iam_access_key" "this" {
for_each = var.user_info
user = aws_iam_user.this[each.key].name
depends_on = [aws_iam_user.this]
}
# Create access keys for test users (needed for testing key rotation enforcement)
resource "aws_iam_access_key" "this" {
for_each = var.user_info
user = aws_iam_user.this[each.key].name
depends_on = [aws_iam_user.this]
}
output "test_user_access_keys" {
description = "Access keys for test users (SECRET - handle with care)"
sensitive = true
value = {
for username, user in var.user_info : username => {
access_key_id = aws_iam_access_key.this[username].id
secret_access_key = aws_iam_access_key.this[username].secret
}
}
}
🤖 Prompt for AI Agents
In terraform/iam/users.tf around lines 37 to 43 the aws_iam_access_key resource
creates access keys but does not capture or persist the SecretAccessKey, which
means the secret is lost after apply; fix by either (a) creating a secure
storage resource and storing the secret there (e.g., create
aws_secretsmanager_secret and aws_secretsmanager_secret_version or
aws_ssm_parameter with secure_string and reference the access_key.secret), or
(b) remove the access_key resource if keys should not be created here, or (c)
add clear documentation and a warning output that keys are disposable and will
be rotated by the enforcement Lambda; choose one approach and implement the
corresponding Terraform resources/outputs and update variables/permissions so
the module can write secrets securely.

Comment on lines +18 to +21
REGION = "us-east-1"
PROFILE = "dw-nonprod"
DYNAMODB_TABLE = "iam-key-rotation-tracking"
TEST_USERNAME = "iam-test-user-dev-1"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Fix hardcoded AWS profile causing pipeline failure.

The hardcoded profile "dw-nonprod" causes the pipeline failure: ProfileNotFound: The config profile (dw-nonprod) could not be found. Use environment variables or allow the default credential chain to work in CI/CD.

🔎 Proposed fix
 # Configuration
 REGION = "us-east-1"
-PROFILE = "dw-nonprod"
+PROFILE = os.environ.get("AWS_PROFILE")  # None allows default credential chain
 DYNAMODB_TABLE = "iam-key-rotation-tracking"
 TEST_USERNAME = "iam-test-user-dev-1"
 
 # Initialize AWS clients
-session = boto3.Session(profile_name=PROFILE, region_name=REGION)
+session = boto3.Session(profile_name=PROFILE, region_name=REGION) if PROFILE else boto3.Session(region_name=REGION)

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In tests/test_rotation_workflow.py around lines 18 to 21, the AWS profile is
hardcoded to "dw-nonprod" which breaks CI because that profile doesn't exist;
change the code to avoid a fixed profile by reading from an environment variable
(e.g., os.environ.get("AWS_PROFILE") or a test-specific variable like
"TEST_AWS_PROFILE") and fall back to None so the AWS SDK uses the default
credential chain, or remove the PROFILE constant entirely and let boto3 use its
normal credential resolution in CI; update any test setup that references
PROFILE to use this env-based value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants