Feat(aws-security): Add Macie, Inspector, GuardDuty & IAM Analyzer integrations#405
Feat(aws-security): Add Macie, Inspector, GuardDuty & IAM Analyzer integrations#405barbaria888 wants to merge 31 commits into
Conversation
Updated the AWS Security Hub integration documentation to reflect changes in architecture, routing, and security measures. Enhanced clarity on the integration's functionality and setup instructions.
Added an image to enhance the README documentation.
…rnally under /securityhub via aws_routes
# Conflicts: # server/utils/db/db_utils.py
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughEventBridge webhooks post Security Hub findings to a secured Flask endpoint that validates API keys, records Prometheus metrics, and enqueues a Celery task. The task generates AI triage and upserts findings into a new Postgres table with RLS. A proxied Next.js GET exposes findings to a new frontend tab with expandable finding cards. ChangesAWS Security Hub Findings Ingestion & Dashboard
Sequence DiagramsequenceDiagram
participant EventBridge
participant Webhook as POST /webhook/<org_id>
participant Validator as _validate_api_key
participant Metrics as Prometheus
participant Queue as Celery.enqueue(process_securityhub_finding)
participant Task as process_securityhub_finding
participant DB as PostgreSQL
participant Frontend as Next.js GET /api/aws/securityhub/findings
EventBridge->>Webhook: send findings payload
Webhook->>Validator: verify x-api-key for org_id
Validator->>DB: load expected api_key
Webhook->>Metrics: increment received/failure, record latency
Webhook->>Queue: enqueue(process_securityhub_finding(payload, org_id))
Queue->>Task: background processing (AI triage)
Task->>DB: set RLS context, UPSERT aws_security_findings
Frontend->>Webhook: proxied GET /findings (via Next.js)
Webhook->>DB: query aws_security_findings (limit, order)
DB-->>Frontend: JSON findings (datetimes isoformatted)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
barbaria888
left a comment
barbaria888
left a comment
There was a problem hiding this comment.
i have reviewed the code best of my knowledge
![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
♻️ Duplicate comments (1)
server/routes/aws/securityhub_routes.py (1)
130-133:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUse the actual timestamp column from
aws_security_findings.This SELECT references
created_at, but the table created in this PR storesreceived_at. This will fail at runtime and return 500 from/findings.💡 Suggested fix
- SELECT finding_id, source, title, severity_label, - payload, ai_summary, ai_risk_level, ai_suggested_fix, - created_at, updated_at + SELECT finding_id, source, title, severity_label, + payload, ai_summary, ai_risk_level, ai_suggested_fix, + received_at AS created_at, updated_at🤖 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 `@server/routes/aws/securityhub_routes.py` around lines 130 - 133, The SQL SELECT in securityhub_routes.py queries aws_security_findings using a non-existent created_at column; update the SELECT to use the actual timestamp column received_at (and remove or replace any other timestamp columns that don't exist, e.g., updated_at) so the query matches the aws_security_findings schema (locate the SELECT that returns finding_id, source, title, severity_label, payload, ai_summary, ai_risk_level, ai_suggested_fix and swap created_at -> received_at).
🤖 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 `@server/routes/aws/SECURITYHUB_README.md`:
- Line 23: The markdown in server/routes/aws/SECURITYHUB_README.md has
MD022/MD031 spacing violations around headings and fenced code blocks; fix by
adding a single blank line before and after each heading and before and after
each fenced code block (for example the json block shown and the surrounding
list item "- Set the destination..."), ensure fenced code blocks are properly
delimited with ``` and that there is a blank line between the closing ``` and
the following paragraph/list, and apply the same spacing corrections at the
other flagged locations (lines referenced 29, 35, 41, 58-60) so the file is
lint-clean.
- Around line 57-60: Update the EventBridge rule example pattern that currently
only matches by source ("aws.securityhub") to also include the detail-type
filter so it only matches Security Hub findings import events; specifically add
"detail-type": ["Security Hub Findings - Imported"] alongside the existing
"source": ["aws.securityhub"] pattern in the JSON example so the rule conforms
to the ingestion contract and avoids forwarding non-finding Security Hub events.
In `@server/routes/aws/tasks.py`:
- Around line 69-117: The Celery task writes to the RLS-protected table
aws_security_findings without setting the session RLS context; call
set_rls_context(...) from utils.auth.stateless_auth on the DB connection/cursor
(e.g., after obtaining conn and cursor from db_pool.get_admin_connection())
using the org_id used for inserts, ensure the import for set_rls_context is
added, and only then perform the INSERT/UPDATE and commit so the RLS policy sees
myapp.current_org_id correctly.
---
Duplicate comments:
In `@server/routes/aws/securityhub_routes.py`:
- Around line 130-133: The SQL SELECT in securityhub_routes.py queries
aws_security_findings using a non-existent created_at column; update the SELECT
to use the actual timestamp column received_at (and remove or replace any other
timestamp columns that don't exist, e.g., updated_at) so the query matches the
aws_security_findings schema (locate the SELECT that returns finding_id, source,
title, severity_label, payload, ai_summary, ai_risk_level, ai_suggested_fix and
swap created_at -> received_at).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f6add56d-e2b0-4a78-849d-2e85ab1ec5a4
📒 Files selected for processing (10)
client/src/app/api/aws/securityhub/findings/route.tsclient/src/app/monitor/components/security-hub-tab.tsxclient/src/app/monitor/page.tsxrequirements.txtserver/requirements.txtserver/routes/aws/SECURITYHUB_README.mdserver/routes/aws/__init__.pyserver/routes/aws/securityhub_routes.pyserver/routes/aws/tasks.pyserver/utils/db/db_utils.py
…EventBridge pattern to findings-imported events.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
server/routes/aws/tasks.py (1)
73-73:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
user_id(notorg_id) when setting Celery RLS context.Line 73 passes
org_idintoset_rls_context, but this worker path should establish RLS using a tenant user identity first, then apply context with thatuser_id.Suggested direction
with db_pool.get_admin_connection() as conn: with conn.cursor() as cursor: - set_rls_context(cursor, conn, org_id, log_prefix="[SECURITY_HUB]") + cursor.execute( + "SELECT id FROM users WHERE org_id = %s ORDER BY created_at ASC LIMIT 1", + (org_id,), + ) + row = cursor.fetchone() + if not row: + logger.warning("[SECURITY_HUB] No user found for org %s", sanitize(org_id)) + return + set_rls_context(cursor, conn, row[0], log_prefix="[SECURITY_HUB]") for finding in findings:#!/bin/bash # Verify the set_rls_context contract and current call usage. fd -i "stateless_auth.py" server --exec rg -n "def set_rls_context|set_rls_context\(" {} rg -n "set_rls_context\(" server/routes/aws/tasks.py -C2As per coding guidelines: “PostgreSQL RLS-protected tables require
myapp.current_org_idset on the connection; in Celery workers callset_rls_context(cursor, conn, user_id)fromutils.auth.stateless_auth.”🤖 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 `@server/routes/aws/tasks.py` at line 73, The call to set_rls_context is using org_id but Celery workers must set RLS using a tenant user identity first; change the argument passed to set_rls_context(cursor, conn, org_id, ...) to pass the tenant user_id instead (i.e., set_rls_context(cursor, conn, user_id, log_prefix="[SECURITY_HUB]")), ensuring you obtain the correct user_id value in the surrounding task code and remove or stop using org_id for this call so the RLS contract from utils.auth.stateless_auth is satisfied.
🤖 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 `@server/routes/aws/tasks.py`:
- Around line 26-27: The two one-line if statements in
server/routes/aws/tasks.py (checking res.get("Id") and res.get("Type")) cause
Ruff E701; replace each single-line conditional with a proper multi-line if
block so the append calls are on their own indented lines (i.e., convert the `if
res.get("Id"):` and `if res.get("Type"):` one-liners into standard two-line if
blocks that call resource_names.append(res["Id"]) and
service_types.append(res["Type"]) respectively), ensuring no other statements
share the same line as the if.
---
Duplicate comments:
In `@server/routes/aws/tasks.py`:
- Line 73: The call to set_rls_context is using org_id but Celery workers must
set RLS using a tenant user identity first; change the argument passed to
set_rls_context(cursor, conn, org_id, ...) to pass the tenant user_id instead
(i.e., set_rls_context(cursor, conn, user_id, log_prefix="[SECURITY_HUB]")),
ensuring you obtain the correct user_id value in the surrounding task code and
remove or stop using org_id for this call so the RLS contract from
utils.auth.stateless_auth is satisfied.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8581be69-19ae-45c9-a43a-b432d45727e2
📒 Files selected for processing (2)
server/routes/aws/SECURITYHUB_README.mdserver/routes/aws/tasks.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@server/routes/aws/tasks.py`:
- Around line 19-29: Normalize nested finding fields before dereferencing by
treating non-dict/null values safely: when accessing Severity in
_generate_ai_triage() (and the other write loop mentioned), coerce Severity to a
dict (e.g., severity_obj = finding.get("Severity") or {} then severity =
severity_obj.get("Label", "UNKNOWN")), and treat Resources as a list of dicts
(e.g., resources = finding.get("Resources") or []; then in the loop use for res
in resources: if not isinstance(res, dict): continue; if res.get("Id"): append
res["Id"]; if res.get("Type"): append res["Type"]). Apply the same defensive
checks in the write loop referenced at line 92.
- Line 60: The log currently interpolates untrusted payload.get('id') into an
f-string which risks log injection; change the logger call in the background
task (where logger.info is used) to use lazy logging (pass a format string and
arguments instead of an f-string) and wrap the event id with
sanitize(payload.get('id')) so control characters are stripped and formatting is
deferred until emit.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ee708909-1075-4ec2-99af-c4a8e1dc7af2
📒 Files selected for processing (1)
server/routes/aws/tasks.py
…ormalize nested finding fields before dereferencing them.
barbaria888
left a comment
barbaria888
left a comment
There was a problem hiding this comment.
Please reflect upon PR. I have addressed the issues that were flagged by the automated security and quality scans.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@docker-compose.prod-local.yml`:
- Line 59: Remove DEV_SECURITYHUB_API_KEY from the shared x-common-env anchor
and instead add it only to the aurora-server service's environment block; locate
the x-common-env anchor definition (x-common-env) and delete the
DEV_SECURITYHUB_API_KEY entry there, then open the aurora-server service
definition (service name "aurora-server") and add DEV_SECURITYHUB_API_KEY:
${DEV_SECURITYHUB_API_KEY} under its environment so the secret is not injected
into other containers.
In `@docker-compose.yaml`:
- Line 56: Remove DEV_SECURITYHUB_API_KEY from the shared x-common-env block so
it is not propagated to all services; instead add DEV_SECURITYHUB_API_KEY only
to the specific service(s) that require it (e.g., the server/webhook validation
service) by placing the variable under that service's environment: section.
Update any references to x-common-env to ensure no other services inherit
DEV_SECURITYHUB_API_KEY and validate that the intended service(s) can read the
variable at runtime.
In `@server/routes/aws/__init__.py`:
- Line 11: Import the cloudwatch_routes module and register its blueprint
instead of referencing an undefined name: add an import for cloudwatch_routes
(alongside aws_routes, auth, onboarding, securityhub_routes) and change the
registration to use cloudwatch_routes.cloudwatch_bp when calling
bp.register_blueprint; do not add any url_prefix since the CloudWatch route
decorators already include /aws/cloudwatch.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5d6ae8a0-e815-46bf-9baf-4df7f41fb9fb
📒 Files selected for processing (6)
.env.exampledocker-compose.prod-local.ymldocker-compose.yamlserver/routes/aws/__init__.pyserver/routes/aws/tasks.pyserver/utils/db/db_utils.py
…d register cloudwatch_routes blueprint
|
|
Hi! Thanks for your contribution. Before we can merge this, we need you to sign our Contributor License Agreement (CLA) for legal purposes. This is a one-time requirement for external contributors — it ensures that contributions are properly licensed and that both parties are protected. I'll send the document separately. Once signed, we're good to go on this and any future PRs. |
Hi @beng360 , Thanks for the update. I would be happy to sign the CLA. Please send it over whenever you get a chance, and I'll complete it as soon as possible. I genuinely enjoyed working on this contribution and appreciate the opportunity to be part of the project. Looking forward to contributing more in the future! |
![arvo-ai-staging[bot]](https://avatars.githubusercontent.com/in/4088260?s=60&v=4){kind=small}
There was a problem hiding this comment.
Aurora Risk Review
Verdict: RISKY
This PR has three confirmed deployment-blocking issues: (1) a duplicate import statement in page.tsx that will cause a TypeScript compile error and break the entire Monitor page on deploy, (2) a schema/query column name mismatch where the table DDL creates received_at but the route queries created_at, causing 500 errors on every findings fetch, and (3) the Validate Environment Variables CI gate is already failing (action_required) on this PR's exact head SHA. The feature itself is architecturally sound and additive, but these three defects will cause immediate user-visible failures on deploy.
Findings
| # | Severity | File | Finding |
|---|---|---|---|
| 1 | HIGH | client/src/app/monitor/page.tsx:3 |
Duplicate lucide-react imports cause TypeScript compile error — Monitor page will not render |
| 2 | HIGH | server/routes/aws/securityhub_routes.py:107 |
Column name mismatch: table has `received_at`, query selects `created_at` — every findings GET returns 500 |
| 3 | MEDIUM | docker-compose.yaml:11 |
FLASK_ENV defaults to `development` in prod-local compose — enables dev-only webhook auth bypass in production containers |
| 4 | MEDIUM | server/routes/aws/securityhub_routes.py:28 |
Prometheus metric registration at import time will crash server startup if `prometheus-client` is absent from the installed environment |
Aurora reviews PRs for incident prevention. This is advisory only and does not block merge.
| @@ -1,12 +1,15 @@ | |||
| 'use client'; | |||
|
|
|||
| import { useState } from 'react'; | |||
There was a problem hiding this comment.
[HIGH] Duplicate lucide-react imports cause TypeScript compile error — Monitor page will not render
The file contains three separate import ... from 'lucide-react' lines (lines 3–5 in the PR head), all importing Radar, HeartPulse, Timer, and ShieldCheck. TypeScript treats duplicate identifier bindings as a compile error (error TS2300: Duplicate identifier). Next.js will fail to build this page, meaning the entire /monitor route returns a 500 or blank page immediately after deploy. This is confirmed by reading the actual file at SHA 32b7828a.
| try: | ||
| # Enqueue background task to process and parse the findings | ||
| process_securityhub_finding.delay(payload, org_id) | ||
| except Exception: |
There was a problem hiding this comment.
[HIGH] Column name mismatch: table has received_at, query selects created_at — every findings GET returns 500
The aws_security_findings DDL in db_utils.py defines the timestamp column as received_at (not created_at), but the SELECT in get_findings() explicitly requests created_at. On the first request to GET /securityhub/findings after deploy, psycopg2 will raise UndefinedColumn: column "created_at" does not exist, returning a 500 to every user who opens the AWS Security tab. The frontend security-hub-tab.tsx also references finding.created_at in its SecurityFinding interface, compounding the failure.
| x-common-env: &common-env | ||
| # Global | ||
| AURORA_ENV: ${AURORA_ENV} | ||
| FLASK_ENV: ${FLASK_ENV:-development} |
There was a problem hiding this comment.
[MEDIUM] FLASK_ENV defaults to development in prod-local compose — enables dev-only webhook auth bypass in production containers
Both docker-compose.yaml and docker-compose.prod-local.yml set FLASK_ENV: ${FLASK_ENV:-development}. The webhook auth logic in securityhub_routes.py (_validate_api_key) explicitly checks os.getenv('FLASK_ENV') == 'development' to allow the DEV_SECURITYHUB_API_KEY to bypass the database token lookup. Any operator who runs docker compose up without explicitly setting FLASK_ENV=production in their .env will have the dev bypass active, allowing unauthenticated webhook ingestion with only the dev key. The Helm chart values.yaml does not set FLASK_ENV at all, so production Kubernetes deployments are unaffected — but the prod-local compose path is directly exposed.
| "Total EventBridge Security Hub events received" | ||
| ) | ||
| EVENTBRIDGE_EVENTS_FAILED = Counter( | ||
| "aws_securityhub_events_failed_total", |
There was a problem hiding this comment.
[MEDIUM] Prometheus metric registration at import time will crash server startup if prometheus-client is absent from the installed environment
Three prometheus_client objects (Counter, Counter, Histogram) are instantiated at module level in securityhub_routes.py. The __init__.py unconditionally imports this module. If prometheus-client==0.20.0 is not present in the container image (e.g., a cached layer from before this PR, or a deployment that skips pip install), the server pod will fail to start with ModuleNotFoundError: No module named 'prometheus_client', taking down the entire Flask server. The dependency is new — it did not exist in server/requirements.txt on main prior to this PR.
|
3 participants
Overview
This PR extends Aurora's application vulnerability detection and remediation capabilities by integrating four additional AWS security services: Amazon Macie, Amazon Inspector, Amazon GuardDuty, and IAM Access Analyzer.
The primary goal is to give Aurora full visibility into the application vulnerability surface across AWS environments. This ranges from CVEs in running workloads (Inspector) to exposed credentials and access paths (GuardDuty, IAM Analyzer) to sensitive data leakage vectors (Macie). This implementation closes the loop with AI-generated, step-by-step remediation guidance surfaced directly in the UI.
Each service follows the exact architectural pattern established in the Security Hub PR: a tenant-isolated backend GET endpoint, a dedicated Monitor tab component, and a standalone findings page with severity-coded cards and collapsible AI remediation details. No new patterns are introduced; this PR is a consistent, additive extension of existing conventions.
Services Added
Security & Architecture Model
Identical to the existing Security Hub implementation. All queries are restricted to the authenticated tenant's
org_id— no cross-tenant data leakage is possible.get_org_id_from_request(). Theaws_security_findingstable explicitly enrolled into Row Level Security (RLS) policies withininitialize_tables().org_idvalues anywhere in this diff. Returns 401 for missing or invalid JWT.org_id) from label tracking, migrating them strictly to context traces and application logs.🚨 CI/CD & Pipeline Fixes (Latest Commits)
_abort_webhook), dropping duplication below the 3.0% threshold.OPTIONS(safe) andPOST(unsafe) webhook handling.logger.exception()and removed user-controlled payload elements from warning logs to prevent log-injection.@require_permission("connectors", "read")structure, satisfying the architectural pipeline linter.client/package-lock.jsonback to upstreammainstandards, clearing blocking vulnerability warnings.upstream/mainand successfully resolved database migration conflicts inserver/utils/db/db_utils.pyto ensure RLS policies cover both the newly added RCA tables and theaws_security_findingstable.Testing & Stability
make devagainst demo payloads for each service. Confirmed zero findings returned from a separate tenant'sorg_id(cross-tenant isolation verified).Frontend Checklist
@/..., typed props, SWR polling).NextRequestdirectly through the fetch payload down to the server API boundary.Notes for Reviewers
This PR is intentionally additive and pattern-conformant. Reviewers who approved the original Security Hub PR will find all four services follow the same conventions exactly — no new architectural decisions were made.
The remediation detail panels use the same AI triage pipeline introduced in the Security Hub PR (
server/routes/aws/tasks.py). No changes were made to the core pipeline itself, but the output formatting was improved to explicitly aggregate AWS target resource contexts and emit deterministic task checklist sequences for the user logic flow—making sure the UPSERT statements confidently overwrite stale metadata representations.Suggested review focus: Auth guard application (
@require_permission), RLS tracking correctness in SQL policies (db_utils.py), frontend component path resolution alignments, and accuracy of concrete remediation steps generated.