feat: add enclave network log - #52
Conversation
|
🚀 Deployed preview to https://eclipse-enclave.github.io/enclave-website-previews/pr-previews/pr-52/ |
a997879 to
a4579f8
Compare
The gateway already wrote audit events that nothing could read. Add internal/netlog as the single source of truth for the JSONL contract, an `enclave network log` subcommand with row, follow, summary and JSON modes, and fix the three format defects the reader would otherwise have to compensate for: raw dnsmasq text appended by a shell tail, no session identity in a file shared by concurrent sessions, and unbounded growth. DNS denials now come from internal/gateway/dnsaudit, which runs as its own process so they are recorded even when the proxy is disabled. Written against captured dnsmasq 2.91 output, which showed NODATA answers are not denials: dnsmasq returns NODATA-IPv6 for every AAAA lookup of an allowlisted host without an IPv6 record, so the shell filter's NODATA case reported allowed domains as denied. It is dropped; NXDOMAIN, SERVFAIL and REFUSED are kept, with the rule distinguishing a policy blackhole from an upstream failure.
Concurrent sessions of one project and tool share a log file, which the reader treated as one stream. --session and --since session now bound the read by the event session field, and gateway paths are deduplicated so a shared file is read once instead of reporting every event twice. Rotation copies and truncates instead of renaming. The gateway bind-mounts the log as a single file, so a rename left an already running session appending to network.log.1, where --follow never looks. Concurrent session starts serialize on a lock file so neither discards the generation the other just wrote. Line splitting moves to a shared splitter with a bound on one line, so a torn write can no longer grow a session-long follower's buffer without limit, and the scanner reports the offset it stopped at. --follow resumes from there rather than re-measuring the file, which closes the window where an event appended while the backlog printed was lost or shown twice. Also: reject --json with --summary, normalize domains through domainpattern everywhere so the filter and the aggregate agree, and move ParseSize to internal/util so internal/config no longer imports the log viewer.
The filter flags had no completion function, so shells fell back to filename completion for --verdict, --type, --since, --domain, and --session.
Coarse mode was documented as "pass/deny events", which reads as one event per request. It is one event per TLS connection, and successful DNS lookups are never recorded, so absence of events was easy to misread as absence of traffic. The website also claimed DNS queries were logged.
a4579f8 to
33daf73
Compare
Both were more machinery than the feature earned. The configurable network_log_max_size dragged in a size parser, a project-scope guardrail and its generated option plumbing to express one number that nobody had asked to change; rotation now uses a netlog.MaxLogBytes constant of 32MB. The tab-separated machine form was a second consumer-facing column contract next to --json, which is the documented one; --summary --json now emits the aggregate as a single object and --plain is gone. Also drops the redundant separator scans in the DNS translator and the SplitHostPort allocation tweak in domainpattern, which optimized a path that was never measured.
EclipseSourceAI
left a comment
EclipseSourceAI
left a comment
There was a problem hiding this comment.
Note
Autonomous AI review.
This review was done by an AI agent and therefore may contain mistakes. Feel free to ignore any comment you disagree with. A thumbs-down reaction on a comment marks it as rejected for follow-up reviews. Noting why in a reply helps, since replies are read too.
Resolving all AI comments does not lead to an automatic approval. A maintainer still needs to review and sign off on the overall architecture and design.
To get an updated review after pushing changes, a maintainer may re-request a review from this account.
Running in Eclipse Enclave, submitted via review-guard-mcp
Adds enclave network log, a reader for the gateway audit log, plus the format fixes that make it readable: a new internal/netlog package owning the JSONL contract (event schema, append, scan, filter, aggregate, follow, rotate), a dnsaudit translator that replaces the shell tail | grep with a real process, session stamping on every event, and copy-and-truncate rotation at 32 MB. The docs work correcting what coarse mode actually records is the most valuable part of the change and is accurate.
The design holds up well. Rotation by copy-and-truncate under a lock is the right call given the bind-mount, the scanner offset handed to --follow genuinely closes the lost/duplicated-event window, and dropping NODATA from the DNS translation fixes a real false positive in the old shell filter. go build ./... and the tests for the touched packages pass here.
Points worth a maintainer's attention:
Aggregatedrops events whose domain fails to normalize, which silently removes the proxy's domainless deny events (tls-clienthello,invalid-host) from--summarytotals.--summaryand--verdict denythen disagree about how much was blocked.- The DNS audit translator is started as root in
gateway-entrypoint.shwhile dnsmasq and the proxy next to it are dropped to unprivileged users. --sessionrequires Docker and a running gateway, even though the events on disk carry the session name and the default scope is explicitly readable after exit.internal/util.FormatBytesduplicates the existingformatBytesininternal/app/cleanup.go.internal/netlogis linked into the gateway proxy build inputs wholesale, including the terminal rendering and reader side the sidecar never uses.
Scope is otherwise tight; the only stray change is a docs/DEV.md paragraph left over from the config option that was dropped in 6a6c15f.
| ) | ||
|
|
||
| // FormatBytes renders a byte count for human output. | ||
| func FormatBytes(bytes int64) string { |
There was a problem hiding this comment.
This duplicates formatBytes in internal/app/cleanup.go (link), same units, same output for everything below 10 units. Since you are adding the shared one anyway, drop the app copy and point its callers here.
| // one log cannot disagree about what counts as the same host. | ||
| domain, err := domainpattern.NormalizeHost(event.Domain) | ||
| if err != nil { | ||
| continue |
There was a problem hiding this comment.
Events with no domain are dropped from the aggregate entirely, including their verdict counts. The proxy writes deny events with an empty domain (tls-clienthello and invalid-host), so --summary reports total_deny: 0 for a log where --verdict deny prints rows. Either count them under a placeholder domain or at least fold them into the totals.
| done & | ||
| DNSMASQ_TAIL_PID="$!" | ||
| log "Starting DNS audit translator" | ||
| enclave-gateway-proxy -dns-audit "$DNSMASQ_LOG_FILE" >>"$DNS_AUDIT_LOG_FILE" 2>&1 & |
There was a problem hiding this comment.
| }, nil | ||
| } | ||
|
|
||
| if err := checkDocker(); err != nil { |
There was a problem hiding this comment.
--session needs Docker and a live gateway, so a session that has exited cannot be selected even though its events are on disk and carry the session name. The whole point of the default scope is that exited sessions stay readable. Falling back to filter.Session = sessionName against the current project's log when no gateway matches would close that gap.
| } | ||
|
|
||
| if pattern := strings.TrimSpace(f.Domain); pattern != "" { | ||
| domain, err := domainpattern.Normalize(pattern) |
There was a problem hiding this comment.
domainpattern.Normalize enforces allowlist safety rules that make no sense for a read-only filter: --domain '*.com' is rejected with "wildcard suffix must include at least two labels" (link). Broadening a query is not a policy decision.
| } | ||
| if asJSON { | ||
| writeErr = writeNetworkLogJSON(out, event) | ||
| } else if _, writeErr = out.WriteString(netlog.RenderEvent(event, render)); writeErr == nil { |
There was a problem hiding this comment.
A session marker in follow mode renders without the blank-line separation and without the pass/deny counts that WriteEvents gives it, so the boundary looks different depending on whether it came from the backlog or the live stream. Reusing the same separator logic for a marker here would keep one output format.
| internal/git | ||
| internal/logx | ||
| internal/model | ||
| internal/netlog |
There was a problem hiding this comment.
The gateway proxy only needs Event and Appender (plus Follower for dnsaudit), but this pulls in render.go, aggregate.go, filter.go and rotate.go along with internal/logx. Splitting the reader side into its own package would keep the sidecar's build inputs and the embedded asset tree to what it actually uses.
| and use `Apply: ApplyNone` in `options_def.go` (for example: | ||
| `--force-base-image` and `--no-rebuild`). | ||
|
|
||
| For config-only options, omit `CLIFlags` instead (for example: |
There was a problem hiding this comment.
This paragraph is left over from the network_log_max_size option that 6a6c15f dropped, and has nothing to do with the network log reader. Better as its own docs commit.
2 participants
What it does
Adds
enclave network log, a reader for the gateway audit log, and fixes the log format it reads.The gateway already wrote audit events, but nothing could read them and the format had three defects a reader would otherwise have to work around: raw dnsmasq text appended by a shell tail, no session identity in a file shared by concurrent sessions, and unbounded growth.
internal/netlogis the single source of truth for the JSONL contract (event schema, append, scan, filter, aggregate, follow, rotate).enclave network logsupports row,--follow,--summary, and--jsonmodes, with--since,--verdict,--domain,--type,--tool,--session, and--all-runningfilters. Terminal output is the aligned human form;--jsonis the machine contract, emitting the raw JSONL event stream or, with--summary, the aggregate as a single object.internal/gateway/dnsaudit, running as its own process so they are recorded even when the proxy is disabled. Written against captured dnsmasq 2.91 output, which showed the shell filter's NODATA case reported allowed domains as denied: dnsmasq returns NODATA-IPv6 for every AAAA lookup of an allowlisted host without an IPv6 record. NODATA is dropped; NXDOMAIN, SERVFAIL, and REFUSED are kept, distinguishing a policy blackhole from an upstream failure.netlog.MaxLogBytesof 32MB. The gateway bind-mounts the log as a single file, so a rename left a running session appending tonetwork.log.1, where--follownever looks. Concurrent session starts serialize on a lock file.--followresumes from there, closing the window where an event appended while the backlog printed was lost or shown twice.--network-log coarserecords one event per TLS connection, not one per request, and successful DNS lookups are never recorded, so absence of events was easy to misread as absence of traffic. The website also claimed DNS queries were logged.Also: normalize domains through
domainpatterneverywhere so the filter and the aggregate agree, and add shell completion for the filter flags.On scope
The first version of this branch also shipped a configurable
network_log_max_sizeand a tab-separated machine output form. Both were dropped in 6a6c15f as more machinery than the feature earned. The size option needed a size parser, a project-scope guardrail and generated option plumbing to express one number nobody had asked to change, so it is a constant now. The TSV form was a second consumer-facing column contract sitting next to--json, which is the documented one, so--plainis gone and--summary --jsoncovers the machine-readable aggregate. That removed about 620 lines.How to test
make buildandmake testpass.make lintwas not run:golangci-lintis not installed in this environment.go vet ./...is clean.Manually, in a project directory:
enclave network logshows pass and deny rows for that project and tool, including after the session exits.enclave network log --summaryaggregates per domain;--jsonemits the raw JSONL and--summary --jsona single aggregate object.enclave network log -fin a second terminal streams new events live while the session runs.--verdict deny,--domain '*.example.com',--type dns,--since 10m,--since session.--session <container>and--all-runningscope correctly and do not duplicate events.--network-log requestsand confirm per-request HTTP events appear.network.logpast 32MB (truncate -s 33Mworks, only the size is checked), start a session, and confirm the previous generation lands innetwork.log.1while a running session keeps appending to the same file and--followkeeps working.Follow-ups
--projectscope for network mutation commands is still unsupported, unchanged by this PR.--network-log requests, which forces MITM for all allowlisted hosts. Documented under Coverage and granularity.Breaking changes
The gateway audit log format changed (raw dnsmasq lines are gone, events carry session identity). The log was not readable before, so no consumer should exist, but it is a format change to a file on disk.
Review checklist