feat(diagnose): add omac diagnose — explain why a sandboxed run failed#128
Open
nhuelstng wants to merge 8 commits into
Open
feat(diagnose): add omac diagnose — explain why a sandboxed run failed#128nhuelstng wants to merge 8 commits into
omac diagnose — explain why a sandboxed run failed#128nhuelstng wants to merge 8 commits into
Conversation
What
- New `omac diagnose` command: the reactive counterpart to `omac doctor`.
It reads the (previously write-only) audit trail back and correlates the
network decisions the sandbox actually made against the effective policy,
surfacing blocked hosts and non-obvious config clashes.
- blocked hosts grouped by host, scoped to the last run (`--run all`).
- clash hints: a host in allow_domain yet DENIED (names the shadow:
deny_domain / learned / hard-deny); allow_domain entries that matched no
traffic (typos); DNS failures; prompt fallbacks; proxy/env notes.
- `--probe host[:port]`: static ALLOW/DENY/PROMPT verdict via the real
netproxy filter (no DNS, no sandbox launch). `--json` for both modes.
- `omac doctor` now runs the existing profileaudit lint as advisory and
points users to `omac diagnose` and the log paths.
Why
- Every net decision is already recorded (audit.jsonl + sandbox.log) but no
command read either back, so when gradle/npm/etc. fails in the sandbox the
user cannot see what was blocked or which rule blocked it. The doctor can
never cover every external tool; surfacing evidence + generic hints lets a
user debug an unknown tool themselves. Reuses provenance (effective config)
and profileaudit (security lint) rather than reimplementing them.
How
- internal/diagnose: pure, stdlib-only analysis (DTOs + hint engine). Zero
omac deps so it is insulated from omac's frequent schema churn.
- Volatile concerns are injected/single-sourced: audit owns read+write of its
format (new audit.ReadLog/ReadFile); netproxy.MatchDomainList is exported
and injected (no re-implemented matching); path resolution reuses
audit.EffectivePath. internal/cli/diagnose.go is the thin composition root
that adapts concrete types into the diagnose DTOs.
Verification
- go test ./... green; gofmt/vet clean.
- New behavioral tests: audit reader (round-trip + malformed-line tolerance),
every hint rule incl. the allow+deny overlap clash, CLI wiring + all three
probe outcomes. `go list -deps ./internal/diagnose` shows no omac deps.
- Ran the binary against a fixture trail: clash, dead-rule, and probe outputs
render correctly.
Signed-off-by: Niclas Hülsmann <niclas.huelsmann@tngtech.com>
What - `omac diagnose --probe <host> --live` performs a real reachability test: it launches the actual sandbox (via sandboxrun.Run) running omac's own hidden `sandbox probe-connect` inner command, which connects to the host through the injected HTTP(S)_PROXY and reports reached/blocked/dns/refused/ timeout/tls. Result is captured via a temp file granted into the sandbox. Why - The static probe answers "would a rule admit this host?"; the live probe answers "does traffic actually get out right now?" — catching proxy-down, broken upstream proxy, DNS, and destination-unreachable failures that the static view cannot see (the gradle/npm failure class). Safety (bounded so a diagnostic cannot run amok) - Opt-in: only with --live; plain --probe stays static/offline. - Gated on static==ALLOW: a denied or unlisted host is never dialed, so the probe can only contact a host the profile's own allowlist already permits. It cannot scan, fan out, or reach anything policy forbids. - Single host, single HTTP HEAD, no retry/loop. - Child HTTP timeout (12s) + a 30s wall-clock watchdog so the command can never hang; the child self-terminates regardless. - No InsecureSkipVerify (a destination TLS error already counts as reached). - No persisted state and no audit-trail pollution (probe proxy uses audit.Nop()); temp result file is unique and defer-removed. How - internal/cli/probe_connect.go: the hidden inner command + pure classifyProbeError. Wired as the hidden `sandbox probe-connect` verb. - internal/cli/diagnose.go: --live flag; runLiveProbe orchestrates the gated sandbox launch, watchdog, and result read-back. Verification - go test ./... green; gofmt/vet clean. New tests: classifyProbeError table, inner command writes result file + keeps stdout clean, and a safety-gate regression asserting a non-ALLOW host is never dialed. - Manual (Linux, bwrap, Landlock ABI 9): allow-listed host -> reached HTTP 200; unlisted host -> skipped without dialing; --json nests the live block. Signed-off-by: Niclas Hülsmann <niclas.huelsmann@tngtech.com>
What
- --probe now understands loopback targets (127.0.0.0/8, ::1, localhost):
their reachability is governed by network.open_port, not the proxy, and a
kernel (Landlock) denial leaves NO trace in the audit trail — so this
static check is the only way to diagnose it (e.g. a sandboxed tool reaching
a local Ollama/DB, or the bridge port). Verified empirically: without
open_port the connection is kernel-blocked and unlogged; with it, it works.
- Reframe hints toward least privilege instead of "allow everything":
- a blocked host recommends allowing the EXACT host and explicitly
legitimizes leaving it denied (the sandbox blocking unexpected egress is
a valid outcome, not always a misconfig);
- unused allow_domain entries are flagged as removal candidates;
- whole-TLD wildcards (e.g. "*.com") are flagged as over-broad.
Why
- Experiments against real failures showed the loopback/open_port class is
invisible to the audit-based view; --probe closes that prospectively.
- A sandbox diagnostic must not train users to widen the allowlist; it should
push the profile toward minimal, specific grants.
How
- internal/cli/diagnose.go: isLoopbackHost + loopbackProbe branch in the
probe path; --live is a no-op for loopback (kernel-enforced).
- internal/diagnose: reworded blocked-host/dead-rule hints; new
overBroadAllowHints for whole-TLD wildcards.
Verification
- go test ./... green; gofmt/vet clean. New tests: over-broad wildcard
flagged (scoped wildcard not), blocked-host hint asserts exact-host +
leave-denied wording and no wildcard steering.
- Manual: loopback probe DENY/ALLOW flips with open_port; over-permissive
profile surfaces both the broad-wildcard warning and the unused-rule note.
Signed-off-by: Niclas Hülsmann <niclas.huelsmann@tngtech.com>
What
- Add an info hint that fires in filtered mode when nothing was blocked:
a proxy-unaware tool (JVM/gradle, some native clients) connects directly
instead of via HTTP(S)_PROXY, the kernel blocks that with NO audit entry,
so the report looks empty while the tool fails on an already-allowed host.
Why
- Reproduced the gradle failure mode: `curl --noproxy '*'` to an allow-listed
host fails ("could not connect") and records nothing — the audit-based view
is blind to it. This is the exact case proxy_injection exists to fix; the
diagnoser must name it instead of leaving the user staring at "0 denied".
How
- internal/diagnose: proxyUnawareHint(p, decisions) — gated on mode=filtered
and zero denials so it never competes with a concrete blocked-host finding;
steers to routing through the proxy (JAVA_TOOL_OPTIONS / proxy_injection),
explicitly not to widening the network policy.
Verification
- go test ./... green; gofmt/vet clean. New tests: hint fires when nothing
blocked + points at the proxy + does not suggest widening; suppressed when
a concrete block exists. Manual: gradle-style direct-connect run now yields
the proxy-unaware hint despite an empty decision list.
Signed-off-by: Niclas Hülsmann <niclas.huelsmann@tngtech.com>
…opback blind spots
What
- Generalize the filtered-mode "nothing was blocked" hint to name BOTH classes
of failure that leave no entry in the network report:
(1) proxy-unaware tools connecting directly (JVM/gradle) and
(2) filesystem/socket or loopback access denials (e.g. docker.sock, a
127.0.0.1 service). Cross-references `omac provenance` and
`omac diagnose --probe 127.0.0.1:<port>` for the non-network case.
Why
- Experiments with Docker (CLI -> daemon over a Unix socket) and Kubernetes
(kubectl -> API server over HTTPS) showed the split: kubectl is proxy-aware
network and already diagnosed cleanly (endpoint:port surfaced); the Docker
socket path produces zero net.decision events, so the old hint would have
misattributed it to proxy-unawareness. The blind-spot note must point at the
socket/filesystem grant (provenance) too, not just the proxy.
How
- internal/diagnose: rename proxyUnawareHint -> invisibleFailureHint; same
firing condition (filtered mode, zero denials) and least-privilege stance
(narrowest grant; do not widen the policy).
Verification
- go test ./... green; gofmt/vet clean. Tests updated to assert both the
proxy-injection and docker.sock/provenance guidance and the no-widen stance.
- Manual: docker-socket blind-spot run now shows both branches of the hint.
Signed-off-by: Niclas Hülsmann <niclas.huelsmann@tngtech.com>
What
- Redesign the default view to stay focused: a one-line status (run/exit ·
mode · N blocked), the blocked hosts (capped), the PROBLEMS to act on with
their fixes, and a single collapsed line for least-privilege/context
ADVISORIES. `-v/--verbose` expands all advisories, the effective config, and
log paths. Blocked list is capped (+K more) unless -v.
- Add failure modes, folded into that model so they add signal, not noise:
* raw-TCP / SSH: `--probe host:port` now reports BOTH paths — HTTP(S) via
proxy (allow_domain) and raw TCP (allow_tcp_connect) — so git@host (SSH:22)
vs git https is diagnosable; the "nothing blocked" hint names allow_tcp_connect.
* network.mode=blocked: a single clear problem instead of a confusing empty
report.
* corporate-CA/TLS: the upstream-proxy advisory now covers re-signed TLS and
the per-ecosystem CA knobs (NODE_EXTRA_CA_CERTS, pip --cert, truststore).
* doctor: bridge/facade loopback ports checked for in-use (unprivileged-userns
is already covered by CheckPlatform's bwrap smoke test).
* exit-code correlation: diagnose leads with "last run exited N" (session.stop).
Why
- Adding failure modes to a flat list would overwhelm users. Separating
"why did my thing fail" (problems, shown) from "hygiene/context" (advisories,
collapsed) keeps the output actionable and non-scary while covering more.
How
- internal/diagnose: Hint gains a Kind (problem|advisory); Report.Problems()/
Advisories(); mode=blocked short-circuit; wording updates.
- internal/cli/diagnose.go: rewritten renderer with -v; rawTCPProbe + two-path
probe output; runExitCode from session.stop.
- internal/cli/doctor.go: doctorBridgePorts.
Verification
- go test ./... green; gofmt/vet clean. New/updated tests: Kind split &
collapse, -v expansion, raw-TCP probe path, over-broad/dead-rule advisories,
invisible-failure hint (proxy + docker.sock + allow_tcp_connect).
- Manual: focused vs -v views; SSH-vs-HTTPS probe flips with allow_tcp_connect;
bridge-port-in-use warning.
Signed-off-by: Niclas Hülsmann <niclas.huelsmann@tngtech.com>
What - Remove the redundant Severity (warn/info) axis: after Kind (problem/advisory) it was a second, ~90%-correlated axis the renderer never printed. Order hints by Kind alone. - Remove dead Decision.When / BlockedHost.LastSeen (computed, never rendered; dropped the time dependency from the pure package). - Collapse profileSummaryV + profileSummary() into the existing diagnose.Policy (one profile->struct mapping instead of two near-duplicates). - Split cli/diagnose.go (592 LOC) into diagnose.go (retrospective analysis + render, 357) and diagnose_probe.go (all probe variants, 254) so the two concerns review independently. - Make --json consistent: add snake_case tags to diagnose.Report/Policy/Hint/ BlockedHost (previously untagged → mixed-case output; tests passed only via Go's case-insensitive unmarshal). Why - Self-review round: cut redundant abstraction and dead code, right-size files for a human reviewer, and fix a half-tagged public JSON contract. Verification - go test ./... green; gofmt/vet clean; behavior unchanged (focused view, --json now uniform snake_case). Tests updated to assert on Kind, not Severity. Signed-off-by: Niclas Hülsmann <niclas.huelsmann@tngtech.com>
What: - Remove the opt-in `--probe --live` machinery (sandbox spawn, temp-file IPC, watchdog) and its hidden `omac sandbox probe-connect` inner command. - Remove doctor's bridge/facade port-in-use check. - Fix Decision.Source doc (actual audit source is "blocklist", not "deny_domain"); suppress the single-run exit code under `--run all`. Why: - The static `--probe` already reuses the real netproxy filter, so its verdict is authoritative; --live added a real sandbox launch + IPC for marginal gain and was the PR's riskiest surface. - The port nudge is scope creep on this PR and adds a live net.Listen side-effect to doctor; the substance is covered elsewhere. How: - Delete probe_connect.go/_test.go; strip live handling and probeResult from diagnose_probe.go; drop the --live flag and sandbox dispatch. - Keep the cheap profileaudit lint gap-fill in doctor. Verification: - go build/vet/test ./... green; ~330 net LOC removed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Niclas Hülsmann <niclas.huelsmann@tngtech.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds
omac diagnose, the reactive counterpart toomac doctor(doctor = "is my setup correct?"; diagnose = "why did my run fail?"). It reads the previously write-only audit trail back and correlates the sandbox's actual network decisions against the effective policy. It is pure, read-only inspection — it never launches a sandbox or contacts the network.net.decisiondenials fromaudit.jsonl(structured, persistent, CI-safe), scoped to the last run.allow_domainyet DENIED (names the shadowing deny/learned/hard-deny rule); unused/typoallow_domainentries; DNS failures.allow_tcp_connect), and filesystem/socket/loopback denials (docker.sock, local services) that leave no trace in the network log.--probe host[:port]— static admission check reusing the real filter (no DNS, no dialing, no sandbox launch): reports the HTTP(S)-via-proxy path and the raw-TCP path; loopback-aware. The verdict cannot drift from runtime behavior because it evaluates the samenetproxyfilter the sandbox enforces.-vexpands everything;--jsonfor a shareable bundle.omac doctorgains the existingprofileauditsecurity lint (advisory) and points atdiagnose.Why
Every network decision omac makes is already recorded, but no command read it back — so when gradle/npm/kubectl/etc. failed inside the sandbox, users had no way to see what was blocked or which rule did it. The doctor can't cover every external tool; the design surfaces evidence + generic, least-privilege hints so a user can debug an unknown tool themselves. It reuses
provenance(effective config) andprofileaudit(security lint) rather than reimplementing them.Least-privilege by design: hints recommend the exact host/port, legitimize leaving a host denied, and flag over-broad/unused rules for removal — never "allow everything".
How
internal/diagnose— pure, stdlib-only analysis (DTOs + hint engine); zero omac deps, insulated from schema churn.auditowns read+write of its format (audit.ReadLog);netproxy.MatchDomainListexported and injected (no re-implemented matching); path resolution reusesaudit.EffectivePath.internal/cli/diagnose.go(analysis + render) anddiagnose_probe.go(static probe) are thin composition roots that adapt concrete types into the DTOs.Verification
go test ./...green;gofmt/go vetclean.-vrendering, and all static probe outcomes (proxy allow/deny/prompt, raw-TCP, loopback, hard-deny).:6443, docker socket, git-over-SSH, loopback services.Notes for review
internal/diagnose/diagnose.go(the model) →internal/audit/read.go→internal/cli/diagnose.go→diagnose_probe.go.--probe --livesandboxed-connection check (spawned a real sandbox + temp-file IPC + watchdog) and adoctorbridge-port-in-use check. Both were dropped to keep the PR to a single, clearly-defined value: read-only diagnosis with no side effects. The static--probealready gives an authoritative verdict by reusing the real filter, so--liveadded machinery for marginal gain.🤖 Generated with Claude Code