Skip to content

feat(graph): BFS attack path traversal and API endpoints - #354

Open
TFT444 wants to merge 8 commits into
feat/332-evidence-graph-node-edge-populationfrom
feat/333-evidence-graph-path-api
Open

TFT444 wants to merge 8 commits into
feat/332-evidence-graph-node-edge-populationfrom
feat/333-evidence-graph-path-api

Conversation

@TFT444

@TFT444 TFT444 commented Sep 24, 2026

Copy link
Copy Markdown
Collaborator

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.py

  • attack_paths table: path_id UUID PK, tenant_id, scan_id, source_node_id FK, target_node_id FK, path_node_ids UUID[], path_length, min_confidence, relationship_types text[], computed_at
  • Unique index on (source_node_id, target_node_id, scan_id) for idempotent re-runs

scanner/graph/path_traversal.py

  • compute_attack_paths(scan_id, tenant_id, dsn) -> int — BFS from each finding-linked node; paths capped at 8 hops; writes to attack_paths via ON 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 node

scanner/graph/graph_populator.py (extended)

  • Adds step 4: call compute_attack_paths after finding-link step; failure is non-fatal

api/routes/attack_graph.py

  • GET /api/attack-graph — nodes and edges for caller's tenant (filtered by tenant_id JWT claim or query param)
  • GET /api/attack-paths?scan_id=<uuid> — pre-computed paths ordered by length and confidence
  • GET /api/attack-paths/<path_id> — single path with full hop node detail
  • Registered in api/app.py as attack_graph_bp

Security

All three endpoints gate on tenant_id (from JWT tenant_id claim 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 error
  • tests/test_attack_graph_api.py — 5 tests: empty graph response, missing scan_id, invalid UUID (paths list), invalid UUID (single path), 404 not found

Merge order

  1. feat(graph): evidence graph foundation - resource inventory schema and scan wiring #352 — foundation (graph tables + engine snapshot wiring)
  2. feat(graph): evidence graph node and edge population #353 — node and edge population
  3. This PR — path traversal and API endpoints

Closes #333. Depends on #353.

@TFT444
TFT444 added this pull request to stack #356 September 24, 2026 23:38
@github-actions

Copy link
Copy Markdown

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

@TFT444 TFT444 self-assigned this Sep 24, 2026
@TFT444
TFT444 requested a review from m-khan-97 September 24, 2026 23:39
Comment thread api/routes/attack_graph.py Fixed
Comment thread api/routes/attack_graph.py Fixed
Comment thread api/routes/attack_graph.py Fixed

@parthrohit22 parthrohit22 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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-graph returns edges where only one end is in the page of nodes, so the client gets dangling node_ids. Filtering with AND instead of OR, or returning the other endpoints too, would keep the payload consistent.
  • _conn() opens a raw connection per request. The other routes go through DatabaseManager, which shares the pool metrics in api/observability.py. Reusing it would keep /metrics accurate.

Happy to re-review once #352 and #353 settle.

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>
@TFT444
TFT444 force-pushed the feat/333-evidence-graph-path-api branch from f966799 to 63c223c Compare September 27, 2026 10:37
@TFT444

TFT444 commented Sep 27, 2026

Copy link
Copy Markdown
Collaborator Author

Both items addressed:

  1. BFS bidirectional traversal: _load_adjacency() now adds reverse edges to the adjacency map (with _REV suffix on the relationship type). BFS from a flagged resource can now reach nodes that point TO it, e.g. a VM with a finding can reach the PublicIP that EXPOSES it.
  2. AND filter on /api/attack-graph edges: Changed OR to AND in the edge query so only edges where both endpoints appear in the returned node page are included. No more dangling node references on the client.

@parthrohit22 parthrohit22 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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, so link_findings_to_nodes links 0 findings and compute_attack_paths has 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) and HAS_IDENTITY (reach the identity a resource runs as).
  • Forward only: MEMBER_OF and PROTECTS.

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_paths still returns len(rows) even when ON CONFLICT DO NOTHING skips rows. With 2 in place this is worse. Keep only the latest scan's paths per subscription, or add retention, and return cur.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 in docs/api-reference.md. Otherwise 403, or scoping by subscription like the rest of the API, is the right response.
  • _conn(): it still opens a raw psycopg2 connection per request instead of going through DatabaseManager. Those connections are invisible to the pool metrics in /metrics.

CI

  • Lint fails with the same unused MagicMock import in tests/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>

This branch has not been deployed

No deployments
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.

feat(graph): [3/3] path traversal, scoring, API endpoints, and tests

3 participants