feat(sentinel): searchable Umbrella identities and the cloud firewall, plus three correctness fixes - #104
Conversation
Every server called load_dotenv(".env.<platform>") with a bare relative path,
which resolves against whatever working directory the MCP client happened to
have. Launching opencode from a subdirectory of the checkout (its shipped
wiring is `uv run --directory .`) therefore started all nine servers with no
credentials at all, and each failed with "Missing required environment
variables" — an error that points at the credentials rather than at the launch
context, so the failure reads as misconfiguration rather than as a path bug.
core/auth/env.py resolves the file by searching instead: $F0_SECTOOLS_ENV_DIR,
then the working directory and its parents, then the installed package's
checkout. python-dotenv's override=False semantics are preserved, so an
exported variable still beats the file and container/systemd deployments that
supply credentials without any file are unaffected.
The five duplicated "missing variables" raises in auth/config.py now share one
helper that also reports which of the two problems occurred — file present but
missing a key, or no file found — with the search hint. Values are never
included; a test asserts the error cannot leak one.
Verified live: the read-only Sentinel smoke run from skills/ now reaches the
workspace and all seven tools return findings. A drift guard fails if a future
server reintroduces the bare relative load.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XYTy7da8Z5ZHhkwcCpjojZ
…lt queue, honest truncation The 2026-07-25 read-tool staleness audit answered its four questions for every read tool across the then-eight servers. Sentinel shipped as server #9 on 2026-08-11 and was never put through it. Two of the four answers were wrong. Q2 (can already-handled records come back as current?) — list_sentinel_incidents defaulted to status="any", so the tool described as "the SOC incident queue" returned closed incidents as current work. On the validation workspace the default call returned 23 Closed and 2 New while exactly 2 incidents were open: the queue was 92% already-handled work with the two that mattered buried in it. It now defaults to status="open", expressed as an exclusion (Status !~ "Closed") rather than an allow-list of ("New","Active"), so a Status value Sentinel adds later is treated as open work instead of silently disappearing. status="any" and status="closed" remain available. Q4 (is truncation disclosed?) — three of seven tools cut results silently: list_sentinel_incidents (25 of 55 deduped incidents dropped), search_office_activity, and run_kql. All three now emit core's truncation finding. The four tools that already disclosed used `len(rows) >= limit`, which cannot tell an exactly-full page from a truncated one and so over-reports; every row-returning tool now fetches limit + 1 and reports what it actually observed. run_kql discloses only when it added the bound itself — when the caller supplied their own `take`, neither answer is knowable. The detection-coverage skill now passes status="any" explicitly at the step that compares incident volume against rule count: that comparison is about the whole population a rule set produced, and the new default would have quietly reduced it to open work only. Q1 (server-side filtering) and Q3 (relevant page, not an arbitrary one) were already correct — severity and status filter inside the KQL before the bound, and every row query orders before it takes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XYTy7da8Z5ZHhkwcCpjojZ
Each server maps the errors it expects — auth, permission, licensing, rate limit — into posture findings. Everything else propagated out of the tool, past `_render`, and reached the MCP client as a raw exception string. Reproduced on sentinel: an httpx.ConnectError surfaced its message verbatim, having passed through no redaction at all. That breaks Critical Rule 3 (redaction covers error paths) and Critical Rule 4 (every tool returns the findings schema), and it does so on all 9 servers and all 58 tools — which is why it is fixed once in core rather than per server. `core/redaction/boundary.py` adds `guarded_tool`, applied directly beneath `@mcp.tool()` on every registered tool. It is the only seam wide enough to catch a failure in client construction, in the tool body, and in the mapping code alike. An unclaimed exception becomes one posture finding carrying the exception type and its message — truncated to 300 characters, because an exception can carry an entire HTTP response body, and routed through the same redaction pass as any other output rather than discarded, since a caller with no detail cannot act. The title carries "temporarily unavailable", one of core/reports' DEGRADATION_MARKERS, so a generated report files an infrastructure failure under coverage instead of counting it as a security finding. `Exception`, deliberately not `BaseException`: cancellation and interrupts must keep propagating or a shutting-down server does not shut down. `functools.wraps` keeps the wrapped signature intact — verified by comparing the MCP-generated tool schema with and without the decorator, since a decorator visible to schema generation would silently rewrite all 58 tool contracts. An AST drift guard fails if any registered tool lacks the decorator, or if one server reports more than one source. CONTRIBUTING's recipe step 6 gains the requirement (and loses two stale references: FastMCP, redact_obj). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XYTy7da8Z5ZHhkwcCpjojZ
… returned
Asked "which host or user is behind these IPs", a local model concluded the
Umbrella logs carry no such field and spent six tool calls hunting the mapping
across LimaCharlie, Tenable, Entra and Office 365. The conclusion was wrong.
Identities_s is 100% populated on Cisco_Umbrella_dns_CL (1,245 distinct in 24h;
identity types "AD Users" and "Anyconnect Roaming Client"), and the tool was
already returning it — it simply could not be searched for, and nothing told the
model it was there.
The dns surface matched `indicator` against Domain_s alone, while its own help
text advertised "a domain, URL fragment, or IP". An IP indicator passed
validation and then matched nothing, so the tool answered "no activity" to a
question it had never actually asked — the same advertised-but-not-honoured
shape as the $orderby trap in the read-tool audit.
Rather than add a `hostname` parameter, the existing `indicator` now covers
identity and address: dns gains InternalIp_s, ExternalIp_s and Identities_s; web
gains Internal_IP_s and Identities_s; vpn gains Device_ID_s. One argument that
means "the thing you are looking for" beats a sixth argument on a tool that
already has five — tool-selection and argument-filling accuracy is the whole
premise of this repo. `has` needed no change: a token drawn from a row matched
its own Identities_s on 359,304 of 359,304 rows. validate_indicator now also
accepts UPN_RE, widening the charset by exactly one character ("@") so an AD
user is usable, still with no quote, backslash or whitespace.
Verified live: hunt_dns_web(surface="dns", indicator="tailscale") returns 8
distinct identities across 8 internal IPs in a single call — the whole six-call
cross-platform hunt, answered from the row the tool already had.
The skill gains the routing rule and a pitfall the same transcript earned: the
model reported "IP to User Mapping Found" for an external IP it had itself just
shown serving five internal hosts. That address is a site's NAT egress; naming
one user behind it is a false attribution stated with confidence.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XYTy7da8Z5ZHhkwcCpjojZ
|
Claude finished @ubercylon8's task in 4m 37s —— View job Security-focused review of PR #104Todo list
SummaryThis is a well-scoped, well-tested fix set. The three cross-cutting fixes ( Two findings worth a look, both correctness/design gaps rather than exploitable holes today: 1. result = await run() # the live write already happened
self._audit(target, actor, token, method) # if THIS raises...
2. The no-indicator (aggregate) branch of parts.append(f"| summarize Events=count() by {spec.action_field}, {spec.indicator_fields[0]}")
parts.append(f"| top {limit} by Events desc") # NOT _fetch_bound(limit)Unlike the indicator/row-mode path (which correctly uses Not a problem, but worth a note
|
…rewall surface Cisco_Umbrella_firewall_CL (10.6M rows/7d) was the last table in the workspace no tool could reach. It is not a duplicate of the CEF table behind hunt_firewall: those are on-prem appliances seeing traffic that crosses the office network, while this is Umbrella's cloud-delivered firewall seeing roaming and remote clients that never touch the perimeter at all. The measured difference is what makes it worth reaching. CommonSecurityLog carries 108M rows/7d with a named user on 0.14% of them; Cisco_Umbrella_firewall_CL carries 10.6M with one on 100% (Identity_Type_s "AD Users" on 3,629,629 of 3,630,796 rows in 24h, 286 distinct). Ten times smaller and fully attributed — it answers "which user opened this connection", which the perimeter firewall structurally cannot. SourceIP, destination IP/port and byte counts are 100% populated too, so volume questions become answerable. Added as a second surface rather than a new tool: it mirrors hunt_dns_web's shape, which the model already drives, and keeps the server at seven tools under the ~8 ceiling. The Surface dataclass already carried per-surface action maps, indicator fields and junk filters, so the differing vocabulary needed no new machinery. Identity leads indicator_fields, making it both the primary search field and the aggregate group-by, so a bare call answers "which users generated this traffic, allowed vs blocked". Ports are string-typed here, so unlike CEF's int DestinationPort they need no port_field special case. FQDNS_s (2.5% populated), Destination_Country_s (1%) and App_ID_s (0.8%) are deliberately absent from indicator_fields: searching them would answer "no such traffic" to questions that were really "that column is mostly empty" — the same defect this branch just removed from the dns surface. The connector's ingested CSV header row appears here as verdict_s == "Action" (~1,152/24h), filtered by the existing hygiene clause. Verified live: a bare cloud call returns the top users by verdict with no junk rows; action="blocked" finds the 9 BLOCK flows in 24h; the perimeter default is unchanged. Worth noting operationally — 3,629,635 ALLOW against 9 BLOCK means that firewall is effectively in monitor mode, which is a posture finding for the tenant rather than anything about this code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XYTy7da8Z5ZHhkwcCpjojZ
|
Claude finished @ubercylon8's task in 2m 36s —— View job Security-focused review of PR #104Todo list
SummaryThis review covers the diff currently on the branch ( New since that review is 1. Aggregate (no-
parts.append(f"| top {limit} by Events desc") # not _fetch_bound(limit)Compare This commit's own numbers make the gap concrete: Fix: 2. (Carried forward, unchanged by this commit)
Not a problem, minor note
Testing note: I was not able to run |
ubercylon8
changed the title
**Aggregate mode never disclosed truncation.** `_run_surface`'s no-indicator
branch asked for `| top {limit}`, which can never return more than `limit` rows,
so `_split_page`'s `len(rows) > limit` was structurally always False. The row
path was fixed in 0a0a389 and this branch was missed, making that commit's
claim — "every row-returning tool now fetches limit + 1" — untrue for every
hunt_* aggregate. It bites hardest on the surface added in 194aead:
cloud_firewall aggregates by identity across 286 distinct users against a
default limit of 25, so a bare call hid 261 of them silently.
The first test written for this passed before the fix. The fake client returns
its canned rows whatever the query says, so asserting on returned rows proved
only that the fake ignores KQL. The assertion is now on the emitted query,
which is the actual contract with the platform.
**An audited write could report as though it had not happened.** A gated action
runs the platform call and then records the audit entry; the write must come
first, or the record could not describe its result. If that record throws, the
new guarded_tool caught it and rendered "temporarily unavailable" — a
degradation, which says the opposite of what occurred. In chat-confirm mode the
confirmation is deliberately not single-use, so a model reading that and
retrying would execute the action a second time. `AuditWriteFailed` now carries
`action_executed = True`, and the boundary renders it as a high-severity action
finding that says the action took effect and must not be retried. Duck-typed,
so the redaction layer keeps no dependency on the gating layer.
**A credential file was injected wholesale.** `load_platform_env` loaded every
variable in the file it found. Critical Rule 7 is per-platform isolation, so a
`.env.defender` was never meant to be able to set `ENTRA_CLIENT_SECRET` — and
since the search added in dace53e walks parent directories, a file placed in a
shared ancestor could also set process-wide knobs. `HTTPS_PROXY` is the sharp
one: every client here builds `httpx.AsyncClient` with the default
`trust_env=True`, so it would be honoured on calls carrying a live token. Only
`<PLATFORM>_*` keys are injected now, via `setdefault` so an exported variable
still wins. Every documented variable across all nine `.env.*.example` files is
already prefixed, so nothing supported changes; the user guide notes it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XYTy7da8Z5ZHhkwcCpjojZ
|
Both review findings were real and are fixed in 1. Aggregate mode truncating silently — confirmed, fixed. The review is right that Worth recording: the first test I wrote for this passed before the fix. The fake client returns canned rows regardless of the query, so asserting on returned rows only proved the fake ignores KQL. The assertion is now on the emitted query, which is the real contract with the platform. 2. Audit-write failure masked as a degradation — confirmed, fixed. 3. The Not fixed, and I agree it is a follow-up: 4 mutations run against the new tests, all caught. 1070 passing. |
|
Claude finished @ubercylon8's task in 3m 44s —— View job Security-focused review of PR #104 (re-run after
|
…d it The previous commit claimed every documented variable across the .env.*.example files was platform-prefixed. That was wrong, and the check behind it was wrong: it matched `^[A-Z0-9_]+=` and so skipped commented lines, which in an example file are exactly the instructions operators copy. Both the defender and projectachilles examples carried `# F0_GATING_DIR=...`, deliberately un-prefixed because servers and scripts/confirm_action.py share one gating root. After the prefix filter that line is silently ignored, so an operator who uncommented it would get the default directory, no warning, and a watcher CLI looking somewhere else. Fixed by removing the line rather than allowlisting the variable. Allowlisting would let any discovered .env file relocate where approvals, tokens and the audit log live — and since these files are now found by walking parent directories, that is a strictly worse version of the vector the prefix filter just closed. Rule 8 wants the audit trail somewhere a credential file cannot move it. Both examples now say to export it in the environment instead, which was already the only fully-correct way to set it: the confirm CLI is a separate process that never reads these files. A test now enforces it, since the manual check is what failed: no .env.*.example may document an assignment — commented or not — that load_platform_env would not inject. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XYTy7da8Z5ZHhkwcCpjojZ
|
Third finding confirmed and fixed in The check behind that claim was the actual defect: I matched Took option (2), and I'd argue (1) is not merely the weaker option but an unsafe one. Allowlisting A test now enforces the property, because the hand check is precisely what failed: no 1071 passing. |
|
Claude finished @ubercylon8's task in 1m 37s —— View job Security-focused review of PR #104 (re-run after
|
Asked for a table of hostnames, a local model checked the Umbrella output, found no field named anything like "hostname", and went looking in Tenable and LimaCharlie instead. The hostname was in the rows it already had. `Identities_s` is a JSON array of two identities on 98% of dns rows (356,571 of 362,944 in 1h): element 0 the Anyconnect roaming client — a machine name, 739 distinct, none containing "@" — and element 1 the AD user, 724 distinct, all UPNs, ordered to match Identity_Types_s. PR #104 made that column searchable but left it shaped as a JSON array inside a single evidence value. That shape is the defect. The findings schema is flat by contract precisely so a small model never has to parse a value, and `list_sentinel_incidents` already learned this lesson the same way — live validation caught it returning `Owner` and the tactics inside `AdditionalData` as raw JSON strings, and both were parsed into readable evidence. The Umbrella surfaces never got the same treatment, so "who" was answerable and "which machine" was not. Rows now carry `identity_host` and `identity_user` as separate fields, with `Identity_Types_s` consumed as the classifier rather than echoed back as another array to parse. Classification is driven by the type array, not by position: the ordering is this connector's convention, not a guarantee. Where types are absent or do not line up, it falls back to shape ("@" means a user). The ~1% of rows carrying a third AD Groups element surface it as `identity_other` — calling a group a hostname would be the same class of error this split removes — and a truncated or non-JSON value is kept unparsed rather than dropped. Verified live: the seven blocked Tailscale hosts now report machine names (SBL8773, SB11199, SBL3878, …) alongside their users, in one call, with no raw array left in the output. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XYTy7da8Z5ZHhkwcCpjojZ
1 participant
Shaking down the sentinel server against the live workspace turned up five things worth changing. Each was reproduced before it was fixed, verified live after, and mutation-tested (25 mutations across the five changes, all caught).
1. Umbrella identities were returned but not searchable —
522faf8Asked "which host or user is behind these IPs", a local model concluded the Umbrella logs carry no such field and spent six tool calls hunting the mapping across LimaCharlie, Tenable, Entra and Office 365. The conclusion was wrong:
Identities_sis 100% populated (1,245 distinct in 24h; types"AD Users"and"Anyconnect Roaming Client") andhunt_dns_webwas already returning it in every row. It just wasn't inindicator_fields, and nothing told the model it was there.Worse, the dns surface matched
indicatoragainstDomain_salone while its own help text advertised "a domain, URL fragment, or IP". An IP indicator passed validation and then matched nothing, so the tool answered "no activity" to a question it had never asked — the same advertised-but-not-honoured shape as the$orderbytrap from the read-tool audit.Fixed by widening the existing
indicatorrather than adding ahostnameargument: one argument meaning "the thing you're looking for" beats a sixth argument on a five-argument tool, and argument-filling accuracy is this repo's premise.hasneeded no change — a token drawn from a row matched its ownIdentities_son 359,304 of 359,304 rows.validate_indicatoralso acceptsUPN_REnow, widening the charset by exactly one character (@), still with no quote, backslash or whitespace.Live:
hunt_dns_web(surface="dns", indicator="tailscale")returns 8 distinct identities across 8 internal IPs in one call — the whole six-call cross-platform hunt, answered from the row the tool already had.The skill gains the routing rule and a pitfall the same transcript earned: the model reported "IP to User Mapping Found" for an external IP it had itself just shown serving five internal hosts. That address is a site's NAT egress; naming one user behind it is a false attribution stated with confidence.
2. The Umbrella cloud firewall was unreachable —
194aeadCisco_Umbrella_firewall_CL(10.6M rows/7d) was the last table in the workspace no tool could reach, and it is not a duplicate of the CEF table behindhunt_firewall. Those are on-prem appliances seeing traffic that crosses the office network; this is Umbrella's cloud-delivered firewall, seeing roaming and remote clients that never touch the perimeter.The measured difference is the point:
CommonSecurityLog(perimeter)Cisco_Umbrella_firewall_CL(cloud)Ten times smaller and fully attributed — it answers "which user opened this connection", which the perimeter firewall structurally cannot. Source IP, destination IP/port and byte counts are 100% populated too, so volume questions become answerable.
Added as a second surface rather than a new tool: it mirrors
hunt_dns_web's shape, keeps the server at seven tools under the ~8 ceiling, and theSurfacedataclass already carried per-surface action maps, indicator fields and junk filters. Identity leadsindicator_fields, so it is both the primary search field and the aggregate group-by — a bare call answers "which users generated this traffic, allowed vs blocked".FQDNS_s(2.5% populated),Destination_Country_s(1%) andApp_ID_s(0.8%) are deliberately not searchable: querying them would answer "no such traffic" to questions that really mean "that column is mostly empty" — the same defect removed from the dns surface in change 1.Worth flagging operationally, independent of this code: 24h verdicts are 3,629,635 ALLOW against 9 BLOCK. That firewall is effectively in monitor mode.
3. Credentials were resolved by working directory —
dace53eEvery server called
load_dotenv(".env.<platform>")with a bare relative path. opencode's shipped wiring isuv run --directory ., so launching it from a subdirectory of the checkout started all nine servers with no credentials at all. Each then failed with "Missing required environment variables" — an error that points at the credentials rather than at the launch context, so it reads as misconfiguration. A capable local model burned ten minutes re-exporting variables into shells that each spawned a fresh process.core/auth/env.pyresolves by search instead:$F0_SECTOOLS_ENV_DIR, then the working directory and its parents, then the installed package's checkout.override=Falseis preserved, so an exported variable still wins and container/systemd deployments are unaffected. The five duplicated "missing variables" raises now share one helper that reports which problem occurred — file present but missing a key, or no file found — and a test asserts the message cannot leak a value.4. Sentinel never went through the read-tool audit —
0a0a389The 2026-07-25 staleness audit answered its four questions for every read tool across the then-eight servers. Sentinel shipped as #9 and was never run through it. Two of four answers were wrong.
Q2 — already-handled records returned as current.
list_sentinel_incidentsdefaulted tostatus="any", so the tool described as "the SOC incident queue" returned closed incidents as current work. Live: the default call returned 23 Closed and 2 New while exactly 2 incidents were open — 92% already-handled work with the two that mattered buried in it. Now defaults tostatus="open", expressed as an exclusion (Status !~ "Closed") rather than an allow-list, so a status Sentinel adds later is treated as open work instead of vanishing.Q4 — truncation not disclosed. Three of seven tools cut results silently (25 of 55 deduped incidents dropped). The four that did disclose used
len(rows) >= limit, which cannot tell an exactly-full page from a truncated one. Every row-returning tool now fetcheslimit + 1and reports what it observed.run_kqldiscloses only when it added the bound itself.Q1 and Q3 were already correct — filters apply inside the KQL before the bound, and every row query orders before it takes.
5. Unmapped exceptions escaped the redaction boundary —
1823335Servers map the errors they expect; everything else propagated past
_renderand reached the MCP client as a raw exception string. Reproduced: anhttpx.ConnectErrorsurfaced its message verbatim, through no redaction. That breaks Critical Rule 3 (redaction covers error paths) and Rule 4 (every tool returns the findings schema), on all 9 servers and all 58 tools — hence fixed once incore, not per server.core/redaction/boundary.pyaddsguarded_tool, applied beneath@mcp.tool()on every registered tool — the only seam wide enough to catch failures in client construction, tool body and mapping code alike. An unclaimed exception becomes one posture finding carrying the exception type and its message, truncated to 300 characters and routed through the standard redaction pass rather than discarded. The title carries"temporarily unavailable", aDEGRADATION_MARKER, so reports file an infrastructure failure under coverage instead of counting it as a security finding.Exception, deliberately notBaseException: cancellation must keep propagating.functools.wrapskeeps the signature intact — verified by diffing the MCP-generated tool schema with and without the decorator, since a decorator visible to schema generation would silently rewrite all 58 tool contracts.Drift guards added
Completeness is a property of a moment, not of the repo — an audit "complete across 8 servers" was inherited by none of the 9th. Three static guards make these durable: no server may call
load_dotenvby a bare relative path; every registered tool must carry@guarded_toolwith one source per server; and the existinggen_docsguard covers the regenerated reference.Verification
1065 tests passing (+53), ruff and mypy clean, no
gen_docsdrift. Live read-only smoke against the validation workspace passes all seven sentinel tools, run from a subdirectory. No live write was performed at any point.🤖 Generated with Claude Code
https://claude.ai/code/session_01XYTy7da8Z5ZHhkwcCpjojZ