diff --git a/CHANGELOG.md b/CHANGELOG.md index 245f185..e746e32 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,76 @@ follow semantic versioning once it reaches 1.0. ## [Unreleased] +## [0.40.0] - 2026-07-20 + +_Security-floor and correctness hardening from an adversarial mining round: three +more secret shapes are redacted, prefix scoping now matches at a path boundary, +ingestion is bounded against oversized/hostile inputs, run executors persist and +degrade truthfully, and provider-credential failures surface cleanly instead of +as opaque 500s._ + +### Security + +- **Prefix scope now matches at a path boundary, not a raw prefix.** A provider + restricted to `allowed_prefixes=["logs"]` previously admitted + `logs-private/secret` via a plain `startswith` — a different top-level path. + `check_scope` now matches only an exact equal or a `/`-delimited child + (`logs` / `logs/…`, never `logs-private/…`), and drops empty-string prefix + entries so a stray `""` can't silently unrestrict the bucket. +- **Three more secret shapes are redacted** in logs/reports/traces/UI: Azure + Blob **SAS `sig=`** HMAC query params, temporary-credential **session tokens** + (`FQoG`/`FwoG`/`IQoJ…`), and `private_key = ` assignments (GCP + service-account / TLS keys) — labels kept, values masked. +- The session agent's appended provider block (name + endpoint URL, added after + `build_session_context` and therefore outside `assert_no_secrets_in_context`) + now routes both fields through `redact_text`, closing the one place a + credential-bearing endpoint string could reach the prompt unredacted. + +### Ingestion bounds + +- `_nonempty_lines` reads via a bounded `readline(_MAX_LINE_CHARS)` loop with a + cumulative-bytes ceiling, so a single multi-GiB line (or a giant file) can no + longer be slurped whole into one Python string and OOM the sidecar. +- Free-text group-by labels in the access-log and inventory analyzers are clipped + to `_LABEL_LEN` before they reach the model context / report prose. +- Finding cells are HTML-escaped in **both** report writers (`analysis_report` + and `report`), so a crafted finding title/detail can't inject markup + (``) into a generated report. +- A **future-dated** object (clock skew or a garbage `9999` date) now lands in + the `unknown` age bucket instead of being mis-bucketed as freshly modified + (`0-7d`). +- The per-run DuckDB session applies a best-effort `memory_limit` / `threads` + ceiling, so an unbounded analytical query can't exhaust host RAM and take the + desktop app down. +- A managed evidence combine now enforces a **cumulative decompressed-output + budget** across all parts (`_COMBINE_MAX_OUT_BYTES`), closing the gap where + thousands of individually-under-cap gzip members could still sum to a + disk-filling total (decompression-bomb defense in depth). + +### Run executors + +- `recent_run_ids_for_provider` joins on `runs.status = 'completed'`, so a + crashed/partial survey's snapshot is never read back as the newest — which had + made "what changed" report un-scanned buckets as removed and posture answers + come from a truncated set. +- Per-bucket persistence in the account-discovery run is wrapped in a + savepoint-style try/except: one bucket's write failure rolls back that bucket + and records a warning finding instead of aborting the whole survey. +- Report generation in the diagnostic, config-review, and account-discovery runs + is wrapped in `require_success`, so a failed report write fails the run loudly + instead of silently completing without an artifact. +- A region-mismatch HeadBucket (`301` / `PermanentRedirect`) surfaces a distinct + `region_mismatch` status instead of a generic access failure. + +### Provider credentials + +- The encrypted-vault temp file is forced to `0600` via `fchmod` before any + ciphertext is written, so a leftover `.tmp` (whose pre-existing perms `O_CREAT` + would otherwise keep) can never be briefly group/world-readable. +- A malformed stored credential reference now raises a clear + `CredentialResolutionError` ("re-enter the credential") instead of a raw + `ValueError` surfacing as an opaque 500. + ## [0.39.0] - 2026-07-17 _Test coverage, not behavior: a frontend test harness (the frontend had zero diff --git a/sidecar/app/agent_runtime/session_agent.py b/sidecar/app/agent_runtime/session_agent.py index e2c988e..856c593 100644 --- a/sidecar/app/agent_runtime/session_agent.py +++ b/sidecar/app/agent_runtime/session_agent.py @@ -665,8 +665,14 @@ def _build_prompt( if conn is not None: try: from ..repositories import cloud_providers as cloud_repo - providers = [{"provider_id": p.id, "name": p.name, "type": p.provider_type, - "region": p.region, "endpoint": p.endpoint_url} + # redact_text the operator-controlled name/endpoint: an endpoint URL + # configured with embedded basic-auth (https://KEY:SECRET@host) would + # otherwise leak into the prompt verbatim — this block is appended + # AFTER build_session_context, so assert_no_secrets_in_context (which + # guards only `context`) does not cover it. + providers = [{"provider_id": p.id, "name": redact_text(p.name or ""), + "type": p.provider_type, "region": p.region, + "endpoint": redact_text(p.endpoint_url or "")} for p in cloud_repo.list_all(conn)] except Exception: # noqa: BLE001 providers = [] diff --git a/sidecar/app/analysis/access_logs.py b/sidecar/app/analysis/access_logs.py index 3a9da93..37b9bab 100644 --- a/sidecar/app/analysis/access_logs.py +++ b/sidecar/app/analysis/access_logs.py @@ -183,10 +183,29 @@ def _open_text(path: str | Path): return open(p, "r", encoding="utf-8", errors="replace") +# A real access-log line is well under a few KB. MAX_INGEST_ROWS bounds the line +# COUNT, but not the length of any one line — so a 2 GiB single-line upload was +# read into one giant str (OOM), and redact_text then ran its regex set over the +# whole thing. Cap bytes per readline AND total bytes so a pathological file +# degrades to bounded junk rows (the parsed-fraction truth guard flags them), +# never an OOM. +_MAX_LINE_CHARS = 1 << 20 # 1 MiB per line +_MAX_TOTAL_CHARS = 3 << 30 # ~3 GiB total (above the 2 GiB upload cap, below OOM) + + def _nonempty_lines(path: str | Path, limit: int | None = None) -> list[str]: out: list[str] = [] + total = 0 with _open_text(path) as fh: - for line in fh: + while True: + # readline(size) reads AT MOST _MAX_LINE_CHARS chars, so a giant single + # line comes back as bounded chunks instead of one huge str. + line = fh.readline(_MAX_LINE_CHARS) + if not line: + break + total += len(line) + if total > _MAX_TOTAL_CHARS: + break s = line.strip() if not s: continue @@ -401,8 +420,19 @@ def import_access_logs(raw_path: str | Path, duckdb_path: str | Path, fmt: str) # --- tool 3: analyze_access_logs -------------------------------------------- +# A single object key / user-agent can be arbitrarily long (bounded only by the +# 2 GiB upload cap). Cap every distribution LABEL so one pathological value can't +# blow up the model context / report prose. Mirrors aggregate._LABEL_LEN. +_LABEL_LEN = 300 + + +def _clip(v: object) -> str: + s = str(v) + return s if len(s) <= _LABEL_LEN else s[:_LABEL_LEN] + "…" + + def _dist(con, sql: str) -> list[dict[str, Any]]: - return [{"value": str(v), "count": int(c)} for v, c in con.execute(sql).fetchall()] + return [{"value": _clip(v), "count": int(c)} for v, c in con.execute(sql).fetchall()] def analyze_access_logs(duckdb_path: str | Path) -> dict[str, Any]: diff --git a/sidecar/app/analysis/duck.py b/sidecar/app/analysis/duck.py index 6573c77..76d7a96 100644 --- a/sidecar/app/analysis/duck.py +++ b/sidecar/app/analysis/duck.py @@ -15,6 +15,16 @@ def _configure(con: duckdb.DuckDBPyConnection) -> duckdb.DuckDBPyConnection: # day when the sidecar runs outside UTC. UTC keeps every comparison consistent # with how the data was stored. con.execute("SET TimeZone='UTC'") + # Defense-in-depth memory ceiling: the analysis runs inside the desktop app's + # sidecar, so an unbounded DuckDB query (a future path using read_csv_auto on a + # crafted file, a huge aggregate) must not be able to exhaust host RAM and take + # the app down. Bounded threads likewise keep a background analysis from + # starving the UI. Both are best-effort — ignored if the build rejects them. + for pragma in ("SET memory_limit='2GB'", "SET threads=4"): + try: + con.execute(pragma) + except duckdb.Error: + pass return con diff --git a/sidecar/app/analysis/inventory.py b/sidecar/app/analysis/inventory.py index 5e9b417..dcc5866 100644 --- a/sidecar/app/analysis/inventory.py +++ b/sidecar/app/analysis/inventory.py @@ -41,6 +41,17 @@ def _norm(name: str) -> str: return name.lower().replace(" ", "").replace("_", "") +# Cap every key/prefix/storage-class LABEL: a single object key is bounded only by +# the 2 GiB upload cap, so one pathological value could blow up the model context +# and the report prose. Mirrors aggregate._LABEL_LEN / access_logs._LABEL_LEN. +_LABEL_LEN = 300 + + +def _clip(v: object) -> str: + s = str(v) + return s if len(s) <= _LABEL_LEN else s[:_LABEL_LEN] + "…" + + # Storage-class tokens used to recognize the storage-class column in a HEADERLESS # inventory CSV (S3 + common S3-compatible names). _KNOWN_STORAGE = { @@ -290,6 +301,10 @@ def series_for(field: str) -> pd.Series: _AGE_CASE = """ CASE WHEN try_cast(last_modified AS TIMESTAMP) IS NULL THEN 'unknown' + -- A future last_modified (clock skew, or a garbage 9999 date) yields a NEGATIVE + -- age; without this guard it satisfies '<= 7' and is mis-bucketed as freshly + -- modified ('0-7d'). Treat future-dated objects as 'unknown', not recent. + WHEN datediff('day', try_cast(last_modified AS TIMESTAMP), current_timestamp) < 0 THEN 'unknown' WHEN datediff('day', try_cast(last_modified AS TIMESTAMP), current_timestamp) <= 7 THEN '0-7d' WHEN datediff('day', try_cast(last_modified AS TIMESTAMP), current_timestamp) <= 30 THEN '8-30d' WHEN datediff('day', try_cast(last_modified AS TIMESTAMP), current_timestamp) <= 90 THEN '31-90d' @@ -329,20 +344,21 @@ def analyze_inventory(duckdb_path: str | Path) -> dict[str, Any]: ).fetchall() ] prefix_dist = [ - {"value": str(p), "count": int(c), "size": int(s or 0)} + {"value": _clip(p), "count": int(c), "size": int(s or 0)} for p, c, s in con.execute( f"SELECT prefix, count(*) c, COALESCE(sum(size), 0) s FROM {TABLE_NAME} " f"GROUP BY prefix ORDER BY s DESC LIMIT {SAMPLE_LIMIT}" ).fetchall() ] storage_dist = [ - {"value": str(sc), "count": int(c)} + {"value": _clip(sc), "count": int(c)} for sc, c in con.execute( f"SELECT storage_class, count(*) c FROM {TABLE_NAME} GROUP BY storage_class ORDER BY c DESC" ).fetchall() ] top_large = [ - {"key": str(k), "size": int(sz or 0), "storage_class": str(sc) if sc is not None else None} + {"key": _clip(k), "size": int(sz or 0), + "storage_class": _clip(sc) if sc is not None else None} for k, sz, sc in con.execute( f"SELECT key, size, storage_class FROM {TABLE_NAME} " f"ORDER BY size DESC NULLS LAST LIMIT {SAMPLE_LIMIT}" diff --git a/sidecar/app/evidence/managed_import.py b/sidecar/app/evidence/managed_import.py index 1e1b5ad..30d6a48 100644 --- a/sidecar/app/evidence/managed_import.py +++ b/sidecar/app/evidence/managed_import.py @@ -46,6 +46,36 @@ # bounds the compressed input, and this ratio bounds the decompressed output. _GUNZIP_MAX_RATIO = 1000 _GUNZIP_MIN_OUT_CAP = 4 * _CHUNK +# Cumulative decompression-bomb backstop for a whole combine. `_gunzip_out_cap` +# bounds ONE gzip member, but a confirmed selection can carry thousands of parts, +# and the per-file 4-MiB floor (`_GUNZIP_MIN_OUT_CAP`) means many small members +# each stay "under cap" while their decompressed SUM fills the disk. This absolute +# ceiling bounds the TOTAL bytes one import writes to the combined file across all +# parts — legitimate inventory/log combines land far below it; a bomb field trips +# it. Set generously above any real analytical import (the DuckDB engine that reads +# the result runs under a 2 GiB memory ceiling). +_COMBINE_MAX_OUT_BYTES = 16 * 1024 * 1024 * 1024 # 16 GiB + + +class _OutBudget: + """Shared cumulative-output budget for one combine (see _COMBINE_MAX_OUT_BYTES). + + Threaded through every part so the running decompressed/copied total is bounded + across the whole selection, not just per-file.""" + + __slots__ = ("remaining",) + + def __init__(self, cap: int = _COMBINE_MAX_OUT_BYTES) -> None: + self.remaining = cap + + def take(self, n: int) -> None: + self.remaining -= n + if self.remaining < 0: + raise LimitExceeded( + "combined evidence exceeds the total decompression budget " + f"({_COMBINE_MAX_OUT_BYTES} bytes across all parts); refusing a " + "possible decompression bomb" + ) class ImportError_(Exception): @@ -427,21 +457,23 @@ def write(self, data: bytes) -> None: self._inner.write(data) -def _append_maybe_gunzip(src: Path, out) -> None: +def _append_maybe_gunzip(src: Path, out, budget: "_OutBudget | None" = None) -> None: """Stream ``src`` into ``out``, transparently gunzipping (bounded). Non-gzip content (and content that merely starts with the gzip magic but is not a valid stream) is copied through verbatim, matching the old best-effort behavior. Decompressed output is capped at ``_GUNZIP_MAX_RATIO`` x the compressed size — beyond that it is treated as - a decompression bomb and the import fails with LimitExceeded. + a decompression bomb and the import fails with LimitExceeded. When ``budget`` + is supplied, every emitted byte also draws down a cumulative cap shared across + all parts of the combine (``_COMBINE_MAX_OUT_BYTES``). """ size = src.stat().st_size with src.open("rb") as fh: magic = fh.read(2) fh.seek(0) if magic != b"\x1f\x8b": - _copy_stream(fh, out) + _copy_stream(fh, out, budget) return out_cap = _gunzip_out_cap(size) d = zlib.decompressobj(wbits=31) # gzip container @@ -464,6 +496,8 @@ def _append_maybe_gunzip(src: Path, out) -> None: f"bound ({out_cap} bytes for {size} compressed bytes); " "refusing a possible decompression bomb" ) + if budget is not None: + budget.take(len(piece)) out.write(piece) data = d.unconsumed_tail if d.eof: @@ -493,26 +527,29 @@ def _append_maybe_gunzip(src: Path, out) -> None: _copy_stream(fh, out) -def _copy_stream(fh: BinaryIO, out) -> None: +def _copy_stream(fh: BinaryIO, out, budget: "_OutBudget | None" = None) -> None: while True: chunk = fh.read(_CHUNK) if not chunk: break + if budget is not None: + budget.take(len(chunk)) out.write(chunk) -def _append_with_newline(src: Path, out: _TrackedWriter) -> None: - _append_maybe_gunzip(src, out) +def _append_with_newline(src: Path, out: _TrackedWriter, budget: "_OutBudget | None" = None) -> None: + _append_maybe_gunzip(src, out, budget) if out.wrote and out.last != b"\n": out.write(b"\n") def _combine_access_logs(parts: list[Path], dest_dir: Path) -> Path: out_path = dest_dir / "combined.log" + budget = _OutBudget() with out_path.open("wb") as fh: for part in parts: writer = _TrackedWriter(fh) - _append_with_newline(part, writer) + _append_with_newline(part, writer, budget) return out_path @@ -523,13 +560,14 @@ def _combine_inventory(parts: list[Path], fmt: str, schema: str | None, dest_dir import duckdb plain: list[Path] = [] + budget = _OutBudget() for i, part in enumerate(parts): with part.open("rb") as fh: is_gz = fh.read(2) == b"\x1f\x8b" if is_gz: unpacked = part.with_name(part.name + ".parquet") with unpacked.open("wb") as fh: - _append_maybe_gunzip(part, fh) + _append_maybe_gunzip(part, fh, budget) plain.append(unpacked) else: plain.append(part) @@ -550,13 +588,14 @@ def _combine_inventory(parts: list[Path], fmt: str, schema: str | None, dest_dir # provides the column names, which we write as the header so the existing # header-based importer can map columns. out_path = dest_dir / "combined.csv" + budget = _OutBudget() with out_path.open("wb") as fh: if schema: header = ",".join(c.strip() for c in schema.split(",")) fh.write(header.encode("utf-8") + b"\n") for part in parts: writer = _TrackedWriter(fh) - _append_with_newline(part, writer) + _append_with_newline(part, writer, budget) else: # No schema (prefix-listing fallback, no manifest). Only skip the # first line of subsequent parts when the parts REALLY carry a @@ -568,7 +607,7 @@ def _combine_inventory(parts: list[Path], fmt: str, schema: str | None, dest_dir for i, part in enumerate(parts): tracked = _TrackedWriter(fh) writer = tracked if (i == 0 or not headered) else _SkipFirstLineWriter(tracked) - _append_maybe_gunzip(part, writer) + _append_maybe_gunzip(part, writer, budget) if writer.wrote and writer.last != b"\n": tracked.write(b"\n") return out_path diff --git a/sidecar/app/repositories/account_discovery.py b/sidecar/app/repositories/account_discovery.py index bb52cbc..681f777 100644 --- a/sidecar/app/repositories/account_discovery.py +++ b/sidecar/app/repositories/account_discovery.py @@ -176,12 +176,22 @@ def get_profile(conn: sqlite3.Connection, run_id: str) -> dict[str, Any] | None: def recent_run_ids_for_provider( conn: sqlite3.Connection, provider_id: str, limit: int = 2 ) -> list[str]: - """The run_ids of a provider's most recent account snapshots, newest first — - across sessions (the account is the same account). Used to diff 'what changed - since last time'.""" + """The run_ids of a provider's most recent COMPLETED account snapshots, newest + first — across sessions (the account is the same account). Used to diff 'what + changed since last time' and to answer cross-bucket posture. + + Only ``runs.status = 'completed'`` snapshots are returned: ``create_snapshot`` + commits the snapshot row (with ``processed_count`` = the full selection) BEFORE + the per-bucket loop, so a run that crashed/was killed mid-survey leaves a + snapshot with only k of N bucket rows. Surfacing that as the newest survey made + ``compare_to_last_survey`` report the un-scanned buckets as 'removed' and + ``query_account_profile`` answer posture from a truncated set. Joining on the + run's terminal status excludes those partials.""" rows = conn.execute( - "SELECT run_id FROM account_snapshots WHERE provider_id = ? " - "ORDER BY created_at DESC, rowid DESC LIMIT ?", + "SELECT s.run_id FROM account_snapshots s " + "JOIN runs r ON r.id = s.run_id " + "WHERE s.provider_id = ? AND r.status = 'completed' " + "ORDER BY s.created_at DESC, s.rowid DESC LIMIT ?", (provider_id, max(1, int(limit))), ).fetchall() return [r["run_id"] for r in rows] diff --git a/sidecar/app/runs/account_discovery_run.py b/sidecar/app/runs/account_discovery_run.py index d883810..be74a32 100644 --- a/sidecar/app/runs/account_discovery_run.py +++ b/sidecar/app/runs/account_discovery_run.py @@ -28,7 +28,7 @@ from ..s3 import account_tools, tools as s3tools from ..s3.scope import check_scope from ..security.redaction import redact_text -from ._common import RunError, run_executor, run_tool_with_events +from ._common import RunError, require_success, run_executor, run_tool_with_events from .analysis_report import render_account_profile, write DEFAULT_MAX_BUCKETS = 100 @@ -198,12 +198,22 @@ def _body(conn: sqlite3.Connection, run_id: str, run: dict[str, Any]) -> str: for name in selected: access_status = _CONFIGURED try: + # Strip `_raw_reads` INSIDE the recorded callable so run_tool_with_events + # persists the cleaned snapshot — popping it after the call (as before) + # left the raw logging/inventory reads in the committed tool_call row, + # contradicting the "never persist" contract and ~doubling the row. + _rr: dict[str, Any] = {} + + def _snapshot(n=name, holder=_rr): + s = account_tools.get_bucket_config_snapshot(conn, provider_id, n) + holder["raw"] = s.pop("_raw_reads", None) + return s + snap = run_tool_with_events( conn, run_id, "get_bucket_config_snapshot", - {"provider_id": provider_id, "bucket": name}, - lambda n=name: account_tools.get_bucket_config_snapshot(conn, provider_id, n), + {"provider_id": provider_id, "bucket": name}, _snapshot, ) - raw_reads = snap.pop("_raw_reads", None) # reuse, never persist + raw_reads = _rr.get("raw") # reuse, never persisted ev = run_tool_with_events( conn, run_id, "discover_evidence_sources", {"provider_id": provider_id, "bucket": name}, @@ -219,6 +229,10 @@ def _body(conn: sqlite3.Connection, run_id: str, run: dict[str, Any]) -> str: # denied buckets were mis-reported as "available".) if head == _DENIED: access_status = _DENIED + elif head == account_tools.REGION_MISMATCH: + # Exists but in another region — reachable with the right region, + # NOT an error; surface distinctly so it isn't dropped as broken. + access_status = account_tools.REGION_MISMATCH elif head == "error": access_status = "error" elif snap.get("access_denied_items"): @@ -240,12 +254,22 @@ def _body(conn: sqlite3.Connection, run_id: str, run: dict[str, Any]) -> str: } per_bucket.append(bucket_entry) - account_repo.add_bucket(conn, snapshot_id, run_id, provider_id, name, - bucket_entry.get("region"), bucket_entry["access_status"]) - account_repo.add_config_snapshot(conn, snapshot_id, run_id, provider_id, name, bucket_entry) - for src in bucket_entry.get("evidence_sources", []): - account_repo.add_evidence_source(conn, snapshot_id, run_id, provider_id, name, src) - conn.commit() + # Persistence is ISOLATED per-bucket too: a redact/json error on one + # bucket's row must not abort the whole survey (the docstring promises it + # continues). Previously these inserts sat outside the try above, so one + # bad row failed the entire run. + try: + account_repo.add_bucket(conn, snapshot_id, run_id, provider_id, name, + bucket_entry.get("region"), bucket_entry["access_status"]) + account_repo.add_config_snapshot(conn, snapshot_id, run_id, provider_id, name, bucket_entry) + for src in bucket_entry.get("evidence_sources", []): + account_repo.add_evidence_source(conn, snapshot_id, run_id, provider_id, name, src) + conn.commit() + except Exception as exc: # noqa: BLE001 - per-bucket persistence isolation + conn.rollback() + bus.publish(run_id, {"type": "finding", "severity": "warning", + "title": f"Bucket {name}: persistence error", + "detail": redact_text(str(exc))}) summary = _build_summary(per_bucket, visible, len(per_bucket), truncated) # Persist the computed summary onto the snapshot row. @@ -304,8 +328,8 @@ def _body(conn: sqlite3.Connection, run_id: str, run: dict[str, Any]) -> str: "summary": summary, "buckets": per_bucket, } content = render_account_profile(run, profile, summary_text) - run_tool_with_events( + require_success(run_tool_with_events( conn, run_id, "generate_markdown_report", {"run_id": run_id}, lambda: {"report_path": config.rel_path(write(run_id, content)), "format": "markdown"}, - ) + )) return summary_text diff --git a/sidecar/app/runs/analysis_report.py b/sidecar/app/runs/analysis_report.py index 5c08266..2aa3daa 100644 --- a/sidecar/app/runs/analysis_report.py +++ b/sidecar/app/runs/analysis_report.py @@ -55,7 +55,16 @@ def _table(headers: list[str], rows: list[list[str]]) -> str: def _findings_md(findings: list[dict[str, str]]) -> str: if not findings: return "- No findings." - return "\n".join(f"- **[{f['severity']}]** {f['title']} — {f['detail']}" for f in findings) + # Escape title/detail like table cells: they interpolate object keys / + # prefixes / storage-class values (attacker-influenceable) into prose, and the + # report renders as HTML — an unescaped `` in a storage_class + # would be stored script. `_cell` was applied to table cells (v0.35) but this + # prose sink was missed. + return "\n".join( + f"- **[{_cell(f.get('severity', ''))}]** {_cell(f.get('title', ''))} — " + f"{_cell(f.get('detail', ''))}" + for f in findings + ) def _cat_findings_md(findings: list[dict[str, str]]) -> str: diff --git a/sidecar/app/runs/config_review_run.py b/sidecar/app/runs/config_review_run.py index d138f21..84386a1 100644 --- a/sidecar/app/runs/config_review_run.py +++ b/sidecar/app/runs/config_review_run.py @@ -16,7 +16,7 @@ from ..repositories import cloud_providers as cloud_repo from ..s3 import config_tools as ct from ..s3.scope import check_scope -from ._common import RunError, run_executor, run_tool_with_events +from ._common import RunError, require_success, run_executor, run_tool_with_events from .analysis_report import render_config_review, write @@ -121,8 +121,8 @@ def call_and_collect(name: str, raw_input: dict[str, Any], executor) -> dict[str "performance": performance, } content = render_config_review(run, summary_out, sections, counts, summary_text) - run_tool_with_events( + require_success(run_tool_with_events( conn, run_id, "generate_markdown_report", {"run_id": run_id}, lambda: {"report_path": config.rel_path(write(run_id, content)), "format": "markdown"}, - ) + )) return summary_text diff --git a/sidecar/app/runs/diagnostic.py b/sidecar/app/runs/diagnostic.py index 962e69b..d7d2d1d 100644 --- a/sidecar/app/runs/diagnostic.py +++ b/sidecar/app/runs/diagnostic.py @@ -19,7 +19,7 @@ from ..repositories import cloud_providers as cloud_repo from ..s3 import tools as s3_tools from ..s3.scope import check_scope -from ._common import RunError, run_executor, run_tool_with_events +from ._common import RunError, require_success, run_executor, run_tool_with_events from .report import write_report # Bounded sample size for the diagnostic listing (never a full scan). @@ -124,9 +124,9 @@ def call(name: str, raw_input: dict[str, Any], executor) -> dict[str, Any]: # Route report generation through run_tool_with_events like every other # executor, so it lands a tool_call + audit row (rule 17: report generation is # auditable) instead of writing report.md with no trail. - run_tool_with_events( + require_success(run_tool_with_events( conn, run_id, "generate_markdown_report", {"run_id": run_id}, lambda: {"report_path": config.rel_path( write_report(run, evidence, findings, summary)[0]), "format": "markdown"}, - ) + )) return summary diff --git a/sidecar/app/runs/report.py b/sidecar/app/runs/report.py index f4a3309..ad2df69 100644 --- a/sidecar/app/runs/report.py +++ b/sidecar/app/runs/report.py @@ -22,6 +22,16 @@ def report_path_for(run_id: str) -> Path: return config.data_dir() / "runs" / run_id / "report.md" +def _esc(v: object) -> str: + """Escape a value interpolated into report PROSE. The report renders as HTML; + a finding detail may carry a bucket name / object key, so neutralize the + markup/table metacharacters (same set as the analysis report's cell escaper). + Whole-document redaction covers credentials, not these.""" + return (str(v).replace("\\", "\\\\").replace("|", "\\|") + .replace("\r", " ").replace("\n", " ").replace("`", "\\`") + .replace("<", "<").replace(">", ">")) + + def _evidence_block(name: str, output: dict[str, Any] | None) -> str: body = json.dumps(output or {"note": "not run"}, indent=2, default=str) return f"### {name}\n\n```json\n{body}\n```\n" @@ -38,7 +48,8 @@ def render( evidence_md = "\n".join(_evidence_block(n, evidence.get(n)) for n in _TOOL_ORDER) if findings: findings_md = "\n".join( - f"- **[{f['severity']}]** {f['title']} — {f['detail']}" for f in findings + f"- **[{_esc(f['severity'])}]** {_esc(f['title'])} — {_esc(f['detail'])}" + for f in findings ) else: findings_md = "- No findings." diff --git a/sidecar/app/s3/account_tools.py b/sidecar/app/s3/account_tools.py index 4ec969d..f0c41b1 100644 --- a/sidecar/app/s3/account_tools.py +++ b/sidecar/app/s3/account_tools.py @@ -26,6 +26,11 @@ SAMPLE_LIMIT = 20 +# A bucket that exists but lives in another region (HeadBucket → 301). Reachable +# with the right region, so it is NOT an error — a distinct status the survey +# reports without dropping the bucket from posture counts. +REGION_MISMATCH = "region_mismatch" + # Evidence source types. INVENTORY = "inventory" SERVER_ACCESS_LOGGING = "server_access_logging" @@ -76,6 +81,12 @@ def get_bucket_config_snapshot( head_status = ct.ACCESS_DENIED elif code in ct._UNSUPPORTED_CODES or http == 501: head_status = ct.PROVIDER_UNSUPPORTED + elif code in ("PermanentRedirect", "301") or http == 301: + # A healthy bucket in ANOTHER region answers HeadBucket with a 301 + # redirect. That is NOT "error" (the bucket exists and is reachable + # with the right region) — brand it distinctly so the survey doesn't + # drop it from posture counts and the user can fix the provider region. + head_status = REGION_MISMATCH location = ct._read(client, "get_bucket_location", Bucket=bucket) versioning = ct._read(client, "get_bucket_versioning", Bucket=bucket) diff --git a/sidecar/app/s3/client_factory.py b/sidecar/app/s3/client_factory.py index 6c1079a..13d201e 100644 --- a/sidecar/app/s3/client_factory.py +++ b/sidecar/app/s3/client_factory.py @@ -93,7 +93,18 @@ def invalidate_provider(provider_id: str) -> None: def _resolve(ref: str | None) -> str | None: if not ref: return None - scope, name = keyring_store.parse_ref(ref) + # A malformed ref (corrupt row, a value that never went through make_ref) + # makes parse_ref raise a bare ValueError that would bubble up as an opaque + # 500. Translate it to the same clear, actionable CredentialResolutionError + # the missing-value path raises, so the operator is told to re-enter the + # credential rather than seeing a raw parse error. + try: + scope, name = keyring_store.parse_ref(ref) + except ValueError as exc: + raise CredentialResolutionError( + "A stored credential reference for this provider is malformed and " + "could not be parsed. Re-enter the credential(s) for this provider." + ) from exc return keyring_store.get_secret(scope, name) diff --git a/sidecar/app/s3/scope.py b/sidecar/app/s3/scope.py index 028581c..eb25a0f 100644 --- a/sidecar/app/s3/scope.py +++ b/sidecar/app/s3/scope.py @@ -16,6 +16,24 @@ from __future__ import annotations +def _prefix_in_scope(target: str, allowed_prefixes: list[str]) -> bool: + """True if ``target`` is within one of ``allowed_prefixes`` at a PATH boundary. + + A plain ``startswith`` let ``allowed_prefixes=["logs"]`` admit + ``logs-private/secret`` — a different top-level path. A prefix ``p`` matches + only an EXACT equal, or ``p`` treated as a directory boundary (``p`` ending in + ``/``, or ``target`` continuing with ``/`` right after ``p``). So ``logs`` + admits ``logs`` and ``logs/...`` but NOT ``logs-private/...``; ``logs/`` admits + ``logs/...``. Callers already strip empty entries.""" + for p in allowed_prefixes: + if target == p: + return True + boundary = p if p.endswith("/") else p + "/" + if target.startswith(boundary): + return True + return False + + def check_scope( allowed_buckets: list[str] | None, allowed_prefixes: list[str] | None, @@ -47,10 +65,13 @@ def check_scope( f"Bucket '{bucket}' is outside this provider's allowed_buckets scope. " f"Allowed: {shown}{more}." ) + # An empty-string entry would make every startswith() match — i.e. silently + # unrestrict the bucket. Drop empties, so a stray "" doesn't widen scope. + allowed_prefixes = [p for p in (allowed_prefixes or []) if p] if allowed_prefixes: target = key if key is not None else prefix if target: - if not any(target.startswith(p) for p in allowed_prefixes): + if not _prefix_in_scope(target, allowed_prefixes): kind = "key" if key is not None else "prefix" return ( f"The {kind} '{target}' is outside this provider's " diff --git a/sidecar/app/security/keyring_store.py b/sidecar/app/security/keyring_store.py index 0184933..a1d6ca8 100644 --- a/sidecar/app/security/keyring_store.py +++ b/sidecar/app/security/keyring_store.py @@ -278,6 +278,15 @@ def _persist(blob: dict[str, str]) -> None: tmp = path.with_suffix(path.suffix + ".tmp") fd = os.open(str(tmp), os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) try: + # O_CREAT's mode is masked by umask AND ignored entirely if the .tmp + # already exists (a prior crashed write) — so a leftover file could carry + # looser perms. fchmod on the open fd forces 0600 unconditionally before + # any ciphertext is written, so the vault is never briefly group/world + # readable. Best-effort on platforms without fchmod (Windows). + try: + os.fchmod(fd, 0o600) + except (AttributeError, OSError): + pass os.write(fd, token) os.fsync(fd) # ciphertext durable before the atomic swap finally: diff --git a/sidecar/app/security/redaction.py b/sidecar/app/security/redaction.py index 1c55b82..dfd823b 100644 --- a/sidecar/app/security/redaction.py +++ b/sidecar/app/security/redaction.py @@ -107,6 +107,11 @@ ), r"\1" + REDACTED, ), + # Azure Blob SAS: the HMAC credential rides in the `sig=` query param (this app + # targets Azure-via-S3-compat too). Anchored to `?`/`&` so a bare word "sig" + # in prose isn't touched. Kept AFTER the AWS rule so an already-masked URL is + # untouched. `se`/`sp`/`sv` etc. are non-secret and intentionally left. + (re.compile(r"(?i)([?&]sig=)([^&\s]+)"), r"\1" + REDACTED), # Bare `token=` / `api_key=` in free text or query strings (e.g. the local # SSE auth `?token=...`, a pasted `api_key=...` config line). Label kept, # value masked. `\b` keeps compound labels like `next_token=` (preceded by a @@ -163,6 +168,18 @@ re.compile(r"(?i)\b(accountkey)(\s*=\s*)[A-Za-z0-9/+=]{8,}"), r"\1\2" + REDACTED, ), + # AWS STS session tokens: a long base64 blob with an UNAMBIGUOUS prefix + # (FQoG/FwoG/IQoJ…). Unlike a bare 40-char SK, this shape can't collide with an + # object key, so it's masked on its own — no AKIA-co-present condition needed. + (re.compile(r"\b(?:FQoG|FwoG|IQoJ)[A-Za-z0-9/+=_\-]{40,}"), REDACTED), + # A `private_key:`/`private-key=` value that is NOT PEM-armored (the PEM rules + # above catch armored blocks; a raw base64/hex value after the label slips + # past them). Label kept, value masked. Requires a longish value so prose + # ("the private key was rotated") isn't touched. + ( + re.compile(r"(?i)\b(private[_-]?key)(\s*[:=]\s*)(['\"]?)[A-Za-z0-9/+=_\-]{20,}"), + r"\1\2\3" + REDACTED, + ), # Basic-auth userinfo in a URL (`scheme://user:pass@host`): mask the # password only, keep the scheme/user so the URL stays diagnosable. ( diff --git a/sidecar/tests/test_v0400_fixes.py b/sidecar/tests/test_v0400_fixes.py new file mode 100644 index 0000000..25ed92c --- /dev/null +++ b/sidecar/tests/test_v0400_fixes.py @@ -0,0 +1,282 @@ +"""v0.40.0 — security redaction/scope, ingestion bounds, run-executor truth, +provider credential robustness. + +Security (SEC): + SEC3 prefix scope matches at a PATH boundary — allowed_prefixes=["logs"] + denies "logs-private/secret" while allowing "logs/app.log"; an empty "" + entry never widens scope. + SEC1 Azure Blob SAS `sig=` HMAC is redacted (non-secret `se=`/`sp=` kept). + SEC5 temporary-credential session tokens (FQoG/FwoG/IQoJ…) are redacted. + SEC6 `private_key = ` secrets are redacted, label kept. + +Ingestion (ING): + ING1 _nonempty_lines reads bounded chunks — a single 50 MiB line can't be + slurped whole into one Python str. + ING3 free-text group-by labels are clipped to _LABEL_LEN in both the + access-log and inventory analyzers. + ING2 finding cells are HTML-escaped in both report writers (no raw ). + ING4 a future-dated object lands in the 'unknown' age bucket, not '0-7d'. + ING5 duck._configure applies the memory/threads pragmas without error. + +Run executors (RUN): + RUN1 recent_run_ids_for_provider returns only COMPLETED surveys (a partial / + crashed run's snapshot is never read back as newest). + RUN4 a region-mismatch (301/PermanentRedirect) HeadBucket surfaces the + REGION_MISMATCH status constant. + +Provider (PROV): + PROV1 the vault .tmp is forced to 0600 before ciphertext is written. + PROV2 a malformed credential ref becomes a CredentialResolutionError, not a + raw ValueError 500. + RUN6 a combine's cumulative decompressed output is bounded across all parts. +""" + +from __future__ import annotations + +import io +import os +import sqlite3 + +from app import config + + +# --- SEC3: prefix scope path-boundary matching ------------------------------- + +def test_scope_prefix_boundary_denies_sibling_path(): + from app.s3.scope import check_scope + + # "logs" must NOT admit "logs-private/..." (a different top-level path). + assert check_scope(None, ["logs"], "b", key="logs-private/secret") is not None + # but DOES admit the exact prefix and anything under it. + assert check_scope(None, ["logs"], "b", key="logs/app.log") is None + assert check_scope(None, ["logs"], "b", key="logs") is None + # a trailing-slash prefix admits children. + assert check_scope(None, ["logs/"], "b", key="logs/app.log") is None + + +def test_scope_empty_prefix_entry_does_not_widen(): + from app.s3.scope import check_scope + + # A stray "" must not turn into "match everything". + assert check_scope(None, ["", "x"], "b", key="other/y") is not None + assert check_scope(None, ["", "x"], "b", key="x/y") is None + + +# --- SEC1/5/6: redaction of non-AWS secrets ---------------------------------- + +def test_redact_azure_sas_signature(): + from app.security.redaction import redact_text + + url = ("https://acct.blob.core.windows.net/c/b?sp=r&st=2024-01-01T00:00:00Z" + "&se=2024-12-31T00:00:00Z&sig=abcDEF123%2Fsecrethmac%3D") + out = redact_text(url) + assert "abcDEF123" not in out and "secrethmac" not in out + # Non-secret SAS params stay (they carry no credential). + assert "se=2024-12-31T00:00:00Z" in out + + +def test_redact_session_token(): + from app.security.redaction import redact_text + + tok = "FQoGZXIvYXdzEBragedyaddagedyaddaBLAHBLAHBLAH0123456789abcdefXYZ=" + out = redact_text(f"aws_session_token={tok}") + assert "agedyaddagedy" not in out + assert tok not in out + + +def test_redact_private_key_value(): + from app.security.redaction import redact_text + + out = redact_text("private_key = AKIAsecretMATERIALvalue0123456789abcd") + assert "secretMATERIALvalue" not in out + # The label survives so the log still says WHAT was redacted. + assert "private_key" in out + + +# --- ING1: bounded line reads ------------------------------------------------ + +def test_nonempty_lines_bounds_a_giant_single_line(tmp_path): + from app.analysis import access_logs + + p = tmp_path / "huge.log" + # One 50 MiB line with no newline. + p.write_bytes(b"a" * (50 * 1024 * 1024)) + lines = access_logs._nonempty_lines(p, limit=5) + # Each returned chunk is bounded by _MAX_LINE_CHARS, never the whole 50 MiB. + assert lines + assert all(len(ln) <= access_logs._MAX_LINE_CHARS for ln in lines) + + +# --- ING3: clipped group-by labels ------------------------------------------- + +def test_clip_bounds_free_text_labels(): + from app.analysis import access_logs as al + from app.analysis import inventory as inv + + long = "x" * 5000 + assert len(al._clip(long)) <= al._LABEL_LEN + 1 # +1 for the ellipsis + assert len(inv._clip(long)) <= inv._LABEL_LEN + 1 + assert al._clip("short") == "short" + + +# --- ING2: HTML-escaped finding cells ---------------------------------------- + +def test_report_findings_are_html_escaped(): + from app.runs import analysis_report, report + + finding = {"severity": "high", "title": "", + "detail": "a & b < c"} + md1 = analysis_report._findings_md([finding]) + assert "") + assert "= 1 # the 2999 object + assert ages.get("0-7d", 0) == 0 # it must NOT be 'freshly modified' + + +# --- ING5: duck pragmas apply cleanly ---------------------------------------- + +def test_duck_configure_applies_pragmas(tmp_path): + from app.analysis import duck + + con = duck.connect(tmp_path / "x.duckdb") + try: + # memory_limit was set (best-effort); it should read back non-null. + val = con.execute("SELECT current_setting('memory_limit')").fetchone()[0] + assert val # some human string like '1.8 GiB' + finally: + con.close() + + +# --- RUN1: only completed surveys are read back ------------------------------ + +def _insert_run(conn: sqlite3.Connection, run_id: str, provider_id: str, + status: str, created: str) -> None: + conn.execute( + "INSERT INTO runs " + "(id, run_type, title, status, provider_id, bucket, prefix, " + " user_prompt, final_summary, report_path, options_json, session_id, " + " origin, created_at, updated_at) " + "VALUES (?, 'account_discovery', 't', ?, ?, NULL, NULL, NULL, NULL, " + " NULL, NULL, NULL, 'agent', ?, ?)", + (run_id, status, provider_id, created, created), + ) + conn.commit() + + +def test_recent_run_ids_excludes_partial_survey(client): + from app.repositories import account_discovery as ad + + conn = sqlite3.connect(str(config.db_path())) + conn.row_factory = sqlite3.Row + pid = "prov-x" + + # Older COMPLETED survey. + _insert_run(conn, "run-done", pid, "completed", "2026-01-01T00:00:00Z") + ad.create_snapshot(conn, "run-done", pid, bucket_count=3, visible_count=3, + processed_count=3, truncated=False, list_status="ok", + summary={}) + # NEWER run that crashed mid-survey (status still 'running'). + _insert_run(conn, "run-partial", pid, "running", "2026-02-01T00:00:00Z") + ad.create_snapshot(conn, "run-partial", pid, bucket_count=3, visible_count=3, + processed_count=3, truncated=False, list_status="ok", + summary={}) + + ids = ad.recent_run_ids_for_provider(conn, pid, limit=2) + assert ids == ["run-done"] # the newer partial is excluded + conn.close() + + +# --- RUN4: region-mismatch status constant ----------------------------------- + +def test_region_mismatch_constant_exists(): + from app.s3 import account_tools + + assert account_tools.REGION_MISMATCH == "region_mismatch" + + +# --- PROV1: vault tmp is 0600 ------------------------------------------------ + +def test_vault_tmp_is_mode_0600(tmp_path, monkeypatch): + monkeypatch.setenv("SAW_DATA_DIR", str(tmp_path / "vault")) + from app.security import keyring_store + + keyring_store._reset_for_tests() + keyring_store.save_secret("scope", "name", "supersecretvalue") + vault = keyring_store._vault_path() + assert vault.exists() + mode = os.stat(vault).st_mode & 0o777 + assert mode == 0o600, oct(mode) + + +# --- PROV2: malformed ref → CredentialResolutionError ------------------------ + +def test_malformed_ref_raises_credential_error(): + from app.s3 import client_factory + + try: + client_factory._resolve("not-a-keyring-ref") + except client_factory.CredentialResolutionError: + pass + else: + raise AssertionError("expected CredentialResolutionError") + # empty/None refs still resolve to None (no credentials configured). + assert client_factory._resolve(None) is None + assert client_factory._resolve("") is None + + +# --- RUN6: cumulative decompression budget ----------------------------------- + +def test_combine_cumulative_output_budget(): + from app.evidence import managed_import as m + + b = m._OutBudget(cap=100) + b.take(60) + try: + b.take(60) # 120 > 100 + except m.LimitExceeded: + pass + else: + raise AssertionError("expected LimitExceeded on cumulative overflow") + + +def test_append_maybe_gunzip_copy_path_honors_budget(tmp_path): + from app.evidence import managed_import as m + + part = tmp_path / "part_00000" + part.write_bytes(b"x" * 200) # plain (non-gzip) content + out = io.BytesIO() + budget = m._OutBudget(cap=100) + try: + m._append_maybe_gunzip(part, out, budget) + except m.LimitExceeded: + pass + else: + raise AssertionError("expected LimitExceeded from cumulative budget") + # Back-compat: no budget → unbounded copy still works. + out2 = io.BytesIO() + m._append_maybe_gunzip(part, out2) + assert out2.getvalue() == b"x" * 200