Add CloudTrail S3 admin audit helper#90
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR introduces a complete offline CloudTrail S3 admin event audit tool. The implementation reads CloudTrail exports in JSON, JSONL, or CSV format, detects suspicious S3 bucket-admin events from a curated event name allowlist, assigns severity based on error codes and event types, and generates Markdown or CSV reports with credential redaction. Documentation and end-to-end tests verify the tool's input handling, event filtering, and output formatting. ChangesCloudTrail Audit Implementation
Documentation and Verification
Sequence DiagramsequenceDiagram
participant CLI as CLI Arguments
participant Parser as Input Parser
participant Normalizer as Event Normalizer
participant Detector as Event Detector
participant Reporter as Report Generator
CLI->>Parser: File paths, format, watched users
Parser->>Parser: Auto-detect JSON/JSONL/CSV
Parser->>Normalizer: Raw CloudTrail events
Normalizer->>Normalizer: Extract fields, infer buckets
Normalizer->>Detector: Normalized event shape
Detector->>Detector: Filter suspicious names
Detector->>Detector: Assign severity, build notes
Detector->>Reporter: Classified events
Reporter->>Reporter: Aggregate counts
Reporter->>Reporter: Redact credentials
Reporter->>CLI: Markdown or CSV report
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Pushed a small follow-up in Verification run locally:
The repository wrapper |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tools/check-cloudtrail-s3-admin-audit.mjs (1)
16-97: ⚡ Quick winExpand fixture/assertion coverage to the full risky-event matrix.
Current checks exercise only a subset of flagged event names, and CSV-output mode is validated only with JSON input. Please add synthetic cases for the remaining risky event names and include at least one
--format csvrun that ingests CSV input too, so allowlist and parser regressions are caught early.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/check-cloudtrail-s3-admin-audit.mjs` around lines 16 - 97, Add synthetic events covering every risky event name to the test fixtures and add a CSV-format invocation that uses the csvInput fixture: extend the JSON written by writeFileSync(jsonInput, ...) to include records for the remaining flagged eventNames (e.g., PutBucketAcl, PutBucketPolicy, PutBucketCors, PutObjectAcl, etc.) and ensure corresponding assertions (using assert.match/assert.doesNotMatch) check counts and severity labels for those names; also add a second execFileSync call that runs script with '--format', 'csv' and '--input', csvInput (not just JSON) and assert the CSV output contains expected headers and rows and redacted access key suffixes, mirroring the existing JSON CSV assertions so parser/allowlist regressions are caught.tools/audit-cloudtrail-s3-admin-events.mjs (1)
104-109: ⚖️ Poor tradeoffConsider narrowing the JSONL fallback condition.
The broad
catchon line 104 treats any JSON parse error as a JSONL file, but the nestedJSON.parse(line)on line 108 is not wrapped in a try-catch. If the initial parse failed due to malformed JSON rather than JSONL format, the line-by-line parse will likely throw an uncaught error with a less helpful message.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/audit-cloudtrail-s3-admin-events.mjs` around lines 104 - 109, The catch is too broad: when JSON.parse(raw) fails we should only treat the input as JSONL if the error is a SyntaxError and raw contains newlines, and we must guard the per-line JSON.parse, e.g. inside the JSONL fallback wrap JSON.parse(line) with a try/catch (or use a safeParse helper) and either skip/bubble malformed lines or collect/log parse errors before calling normalizeEvent; update the block handling raw, JSON.parse, normalizeEvent, and the JSONL mapping so malformed individual lines don't throw uncaught exceptions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/cloudtrail-s3-admin-audit.md`:
- Line 3: Replace the ambiguous incident range "Apr 17-19" used in Issue `#55`
Phase 7 with an explicit date-time range including year and timezone (for
example "2026-04-17T00:00:00Z to 2026-04-19T23:59:59Z UTC"); update the
CloudTrail export instructions that mention "export CloudTrail events for the
suspicious Apr 17-19 window" so they reference the full ISO-8601 timestamps and
specified timezone to ensure operators export the correct window when reviewing
whether service IAM users performed bucket-admin operations.
In `@tools/audit-cloudtrail-s3-admin-events.mjs`:
- Around line 57-59: The conditional that checks if args.watchUsers ===
DEFAULT_USERS and reassigns args.watchUsers to new Set(DEFAULT_USERS) is dead
code because args.watchUsers is already initialized as new Set(DEFAULT_USERS);
remove the entire if block referencing args.watchUsers and DEFAULT_USERS (the
identity comparison and reassignment) to eliminate dead code and keep the
already-safe clone behavior from the earlier initialization of args.watchUsers.
- Line 19: The ACCESS_KEY_PATTERN constant currently uses an incorrect regex
(/AKIA[0-9A-Z]{12,20}/g); update it to match AWS access key format by
restricting the character set to base32 and the length to the standard
20-character key (AKIA + 16 chars) or the full AWS range if desired — for
example replace ACCESS_KEY_PATTERN with a regex like /AKIA[2-7A-Z]{16}/g for
standard keys or /AKIA[2-7A-Z]{16,128}/g to allow the full documented range.
---
Nitpick comments:
In `@tools/audit-cloudtrail-s3-admin-events.mjs`:
- Around line 104-109: The catch is too broad: when JSON.parse(raw) fails we
should only treat the input as JSONL if the error is a SyntaxError and raw
contains newlines, and we must guard the per-line JSON.parse, e.g. inside the
JSONL fallback wrap JSON.parse(line) with a try/catch (or use a safeParse
helper) and either skip/bubble malformed lines or collect/log parse errors
before calling normalizeEvent; update the block handling raw, JSON.parse,
normalizeEvent, and the JSONL mapping so malformed individual lines don't throw
uncaught exceptions.
In `@tools/check-cloudtrail-s3-admin-audit.mjs`:
- Around line 16-97: Add synthetic events covering every risky event name to the
test fixtures and add a CSV-format invocation that uses the csvInput fixture:
extend the JSON written by writeFileSync(jsonInput, ...) to include records for
the remaining flagged eventNames (e.g., PutBucketAcl, PutBucketPolicy,
PutBucketCors, PutObjectAcl, etc.) and ensure corresponding assertions (using
assert.match/assert.doesNotMatch) check counts and severity labels for those
names; also add a second execFileSync call that runs script with '--format',
'csv' and '--input', csvInput (not just JSON) and assert the CSV output contains
expected headers and rows and redacted access key suffixes, mirroring the
existing JSON CSV assertions so parser/allowlist regressions are caught.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bd77db9d-9e17-4526-8638-68d4ef83d5e3
📒 Files selected for processing (3)
docs/cloudtrail-s3-admin-audit.mdtools/audit-cloudtrail-s3-admin-events.mjstools/check-cloudtrail-s3-admin-audit.mjs
|
Pushed follow-up Changes:
Verification:
The repo-level |
|
Pushed follow-up Changes:
Verification:
Repo-level |
1 participant
/claim #55
Summary
Scope
This is a separate Phase 7 CloudTrail audit slice. It does not overlap with the existing Civil ID backend/frontend fixes, S3 env-var credential PRs, n8n workflow audit, bucket guardrail audit, or secret-scanning PRs.
No live AWS/IAM/S3 calls were made. No CloudTrail exports, key rotation/deletion, bucket policy changes, candidate records, or credential material are included.
Verification
node tools/check-cloudtrail-s3-admin-audit.mjsnode --check tools/audit-cloudtrail-s3-admin-events.mjsnode --check tools/check-cloudtrail-s3-admin-audit.mjsgit diff --checkThe repo's documented full suite command currently fails before test bootstrap because
run-tests.shinvokesdocker-composewithout a compose file, while this checkout only includes named compose files such asdocker-compose-local.yml,docker-compose-dev.yml, anddocker-compose-prod.yml.Summary by CodeRabbit
New Features
Documentation
Tests