Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
parthrohit22
left a comment
There was a problem hiding this comment.
The BFS itself is clean and easy to reason about. The hop cap and the tenant filter on every endpoint query are good, and moving tenant resolution off the query param onto the verified principal was the right fix.
I'm requesting changes, mostly because of what's upstream. With the #352/#353 issues this step always has nothing to work on: no snapshot in real scans, and even with one, finding_graph_nodes is empty because linking runs before save_scan. So compute_attack_paths returns 0 on every scan, and the API only ever serves empty lists. Once those are fixed, a few things here are worth sorting before merge:
1. Edge direction doesn't match "attack path from a finding"
BFS follows edges source → target, but the edge directions from #353 are mixed: EXPOSES is PublicIP→NIC, HAS_IDENTITY is identity→resource, REACHABLE_VIA is storage→PE, MEMBER_OF is resource→subnet. Starting from a flagged storage account you reach its private endpoint. But starting from a flagged VM/NIC you can never reach the public IP that exposes it, and that's the path people will actually care about. Could you either define edge direction as "attacker can move from A to B" consistently, or traverse with per-relationship direction rules?
2. Row growth
Every scan writes one row per (finding node × reachable node), and nothing ever deletes old scans' paths. On a subscription with a few hundred findings in a connected VNet this grows quickly. At minimum, keep only the latest scan per subscription, or add a retention step. Also, _write_paths returns len(rows) even when ON CONFLICT DO NOTHING skips them, so the logged count can overstate what was written.
3. Tenant resolution in shared-secret mode
In shared-secret mode tenant is always None, so every non-admin caller gets 400 tenant_id not available. Admins can pass any X-Tenant-Id. If that's intentional for now, please mention it in the API docs. Otherwise a 400 for a normal viewer feels like the wrong status (403, or scoping by subscription like the rest of the API).
Small
/api/attack-graphreturns edges where only one end is in the page of nodes, so the client gets danglingnode_ids. Filtering withANDinstead ofOR, or returning the other endpoints too, would keep the payload consistent._conn()opens a raw connection per request. The other routes go throughDatabaseManager, which shares the pool metrics inapi/observability.py. Reusing it would keep/metricsaccurate.
Signed-off-by: Tanvir Farhad <tamimtarafder12@gmail.com>
…oints Signed-off-by: Tanvir Farhad <tamimtarafder12@gmail.com>
…nant IDOR Viewer-role tokens and tokens without a tenant claim can no longer access another tenant's graph data by supplying tenant_id as a query parameter. Tenant resolution now uses the OIDC tid claim (user["tenant"]) or, for shared-secret admin tokens only, the X-Tenant-Id request header. Signed-off-by: Tanvir Farhad <tamimtarafder12@gmail.com>
Signed-off-by: Tanvir Farhad <tamimtarafder12@gmail.com>
…ph edge query Signed-off-by: Tanvir Farhad <tamimtarafder12@gmail.com>
f966799 to
63c223c
Compare
|
Both items addressed:
|
parthrohit22
left a comment
There was a problem hiding this comment.
Re-reviewed 63c223c. The AND filter on /api/attack-graph edges is right: no more dangling node IDs.
This head brings back the bug #353 just fixed, though.
1. Regression: populate_graph() runs inside run_scan() again
Commit 830ca7a ("wire post-scan node and edge population into ScanEngine") sits on top of #353's fix. It re-adds the in-engine call at scanner/engine.py:205-211, while scanner/worker.py:219-226 still calls it after save_scan(). On this head every scan therefore populates the graph twice:
- first inside
run_scan(), before findings are saved, solink_findings_to_nodeslinks 0 findings andcompute_attack_pathshas no sources; - then again from the worker.
It looks like a rebase artefact. Please drop 830ca7a (or remove that block) so the worker is the only caller.
2. Fully bidirectional BFS turns the graph into an undirected one
Adding a _REV edge for every edge answers "the VM can't reach the public IP that exposes it". But it also lets BFS walk any relationship backwards. For example, MEMBER_OF reversed goes subnet → every other member, so every resource sharing a subnet with a flagged resource becomes reachable within two hops. On a real VNet the path count becomes roughly findings × subnet size, which is also why the row growth below matters. What I asked for was per-relationship direction ("attacker can move from A to B"):
- Reverse:
EXPOSES(reach the exposing public IP) andHAS_IDENTITY(reach the identity a resource runs as). - Forward only:
MEMBER_OFandPROTECTS.
A small {relationship: (forward, reverse)} table in path_traversal.py does it. Please add a test with two unrelated VMs in one subnet, asserting that a finding on one does not produce a path to the other.
3. Still open from my first review
- Row growth: paths are written per scan and never pruned, and
_write_pathsstill returnslen(rows)even whenON CONFLICT DO NOTHINGskips rows. With 2 in place this is worse. Keep only the latest scan's paths per subscription, or add retention, and returncur.rowcount. - Shared-secret mode: every non-admin caller still gets
400 tenant_id not available. If that's intended until OIDC is the only mode, say so indocs/api-reference.md. Otherwise 403, or scoping by subscription like the rest of the API, is the right response. _conn(): it still opens a rawpsycopg2connection per request instead of going throughDatabaseManager. Those connections are invisible to the pool metrics in/metrics.
CI
- Lint fails with the same unused
MagicMockimport intests/test_graph_engine_post_scan.py. - Backend Tests fails with the same 5 failures and 1 error inherited from #352's
down_revision.
A second migration (f2a3b4c5d6e7_attack_paths.py) stacks on e1f2a3b4c5d6. Once #352 changes to a single parent, check that alembic heads is still one head across the stack.
…h count, use DatabaseManager - Remove populate_graph call from ScanEngine.run_scan(); worker.py is the authoritative post-save call site. Running it inside run_scan() fires before findings are persisted and duplicates the population on every scan. - Restrict _REV adjacency edges to EXPOSES and HAS_IDENTITY only. Reversing MEMBER_OF would connect any two VMs sharing a subnet through the subnet node, producing spurious lateral-movement paths with no real attack vector. - _write_paths: return cur.rowcount instead of len(rows). ON CONFLICT DO NOTHING silently drops duplicate insertions; len(rows) overcounts whereas rowcount reflects actual rows written. - Replace raw psycopg2.connect in attack_graph routes with DatabaseManager to use the shared connection pool and match the pattern used by all other routes. - Remove unused MagicMock import (ruff F401). Signed-off-by: Tanvir Farhad <tamimtarafder12@gmail.com>
…llers, document retention - tests/test_graph_path_traversal.py: add test asserting MEMBER_OF edges are not reversed (two VMs in one subnet must not be reachable from each other through the subnet node). Add test for _load_adjacency verifying EXPOSES gets a _REV edge but MEMBER_OF does not. - api/routes/attack_graph.py: return 403 instead of 400 when tenant_id is absent in shared-secret mode. The request is well-formed; the auth method is insufficient. Error message now explains OIDC is required. - docs/api-reference.md: document the OIDC-only requirement for attack graph endpoints and note the attack_paths retention follow-up (issue #333). Signed-off-by: Tanvir Farhad <tamimtarafder12@gmail.com>
| limit = _MAX_LIMIT | ||
| subscription_id = request.args.get("subscription_id") | ||
| except ValidationError as exc: | ||
| return jsonify({"error": str(exc)}), 400 |
| if limit > _MAX_LIMIT: | ||
| limit = _MAX_LIMIT | ||
| except ValidationError as exc: | ||
| return jsonify({"error": str(exc)}), 400 |
| try: | ||
| path_id = uuid_string(path_id, "path_id") | ||
| except ValidationError as exc: | ||
| return jsonify({"error": str(exc)}), 400 |
…ix stacked admission test - scanner/engine.py: remove 'import os' now unused after populate_graph was removed from run_scan(). - tests/test_attack_graph_api.py: update viewer-token test to expect 403 (auth method insufficient) instead of 400 (bad request) to match the updated response for shared-secret callers without a tenant claim. - tests/test_scan_admission_migration_postgres.py: resolve _HEAD dynamically via ScriptDirectory.get_current_head() so stacked migration tests do not pin a hardcoded revision. Signed-off-by: Tanvir Farhad <tamimtarafder12@gmail.com>
Summary
PR 3/3 of the attack graph series. Depends on #353 (node and edge population).
After nodes and edges are populated, this PR computes shortest attack paths from every finding-linked node via BFS and exposes them through three new API endpoints.
New modules
alembic/versions/f2a3b4c5d6e7_attack_paths.pyattack_pathstable:path_idUUID PK,tenant_id,scan_id,source_node_idFK,target_node_idFK,path_node_ids UUID[],path_length,min_confidence,relationship_types text[],computed_at(source_node_id, target_node_id, scan_id)for idempotent re-runsscanner/graph/path_traversal.pycompute_attack_paths(scan_id, tenant_id, dsn) -> int— BFS from each finding-linked node; paths capped at 8 hops; writes toattack_pathsviaON CONFLICT DO NOTHING; returns 0 and logs on any DB error (non-fatal)_bfs_from(start, adj)— pure BFS returning shortest path metadata per reachable nodescanner/graph/graph_populator.py(extended)compute_attack_pathsafter finding-link step; failure is non-fatalapi/routes/attack_graph.pyGET /api/attack-graph— nodes and edges for caller's tenant (filtered bytenant_idJWT claim or query param)GET /api/attack-paths?scan_id=<uuid>— pre-computed paths ordered by length and confidenceGET /api/attack-paths/<path_id>— single path with full hop node detailapi/app.pyasattack_graph_bpSecurity
All three endpoints gate on
tenant_id(from JWTtenant_idclaim or explicit query param) before any DB query. No cross-tenant rows are ever returned.Test coverage
tests/test_graph_path_traversal.py— 6 tests: isolated node, single hop, two hops, no-revisit, zero-paths, DB errortests/test_attack_graph_api.py— 5 tests: empty graph response, missing scan_id, invalid UUID (paths list), invalid UUID (single path), 404 not foundMerge order
Closes #333. Depends on #353.