Repository navigation
fix: make CI's gosec scan the agent, and close what it finds - #43
Merged
Merged
Conversation
The agent names a log source's checkpoint after the id the server sends, filepath.Join(-log-state-dir, id+".ckpt"), and writes it as its own user, which is root on most nodes, after a MkdirAll of the parent. Nothing checked the id, so an id like "../../etc/cron.d/x" made the agent create directories and write a checkpoint file outside the state directory, and read one back from there on the next start. The server only mints plain ids (logsrc_<base32>, agent-debug-<hex>, singbox-<hex>), so this needed a compromised or faulty control plane, which is the case node-side checks exist for. An id that is not a plain file name now gets no checkpoint, the mode the agent already has without -log-state-dir: the source still tails from the end, and the agent logs why. Found by gosec G304 on the checkpoint read. Tested: TestLogCheckpointPathStaysInStateDir failed before the change (nine ids escaped or misnamed the file) and passes after.
…ition
Resume took the checkpoint offset as uint64 and cast it to int64, so an
offset above MaxInt64, from a damaged checkpoint file, became negative,
and every later Poll failed on the seek: the source stopped shipping
lines until someone deleted the checkpoint. Such an offset is past the
end of any file, so Resume now starts the file over, which is what Poll
already does for a checkpoint past EOF.
Found by gosec G115 on the conversion.
Tested: TestResumePastAnyOffsetStartsOver failed before the change
("seek ...: invalid argument") and passes after.
…viour CI's gosec step never scanned this tree (see the next commit). Run inside the module, gosec v2.29.0 and v2.30.0 report the same 74 findings; two were real and are fixed in the previous commits. The rest: Every dropped error on a cleanup path that already returns the primary error (G104) is an explicit discard. Every finding that is guarded or intended carries a "#nosec <rule> -- <guard>" comment in the form the tree already used, naming the guard: commands run fixed binaries, or the agent's own flags, with no shell, and terminals only under -allow-terminal (G204); paths are the agent's own flags, /proc, the validated line-chain artifact or prefetch digest, or files opened with O_NOFOLLOW and checked on the descriptor (G304); conversions are counts, POSIX ids, bounded sizes or window sizes (G115). The four G103 findings are protoc-gen-go output (stats.pb.go); CI now passes -exclude-generated rather than editing a generated file. Not-tested: the annotations themselves; the gosec runs are the check.
The gosec step ran "gosec ./lattice-node-agent/..." from the parent of the checkout, where gosec v2.29.0 loads no package: run 40086050635, like every run before it, printed "Files: 0" and passed. The step now runs in lattice-node-agent like vet and test, and fails when the report shows no file or any package load error; gosec alone exits 0 in both cases. It skips generated files, so protoc output does not need annotations. Tested: the step's commands locally with GOTOOLCHAIN=go1.26.10. v2.29.0 and v2.30.0 both exit 0 with 65 files, 0 issues, no load error. The guard was proven on the server's identical step: exit 1 on a 0-file report, on a type error and on an empty directory.
gosec recognises the MaxInt64 branch in Resume, so the comment on the conversion suppressed nothing.
A terminal session names its shell, and the agent ran whatever path the server sent once it existed, as the agent's user. The server maps every request to one of five shells (normalizeTerminalShell) and always has, but a compromised server could name any executable on the node, which then ran with no input the transcript would show. The agent now holds the shell to the same five, on both transports, and fails the session before anything runs otherwise. -allow-terminal still gates terminals as before. Found while checking gosec G204 on the shell exec. Tested: TestTerminalRefusesAShellOutsideTheAllowlist failed before the change on both transports (the stand-in executable ran) and passes after; TestTerminalShellDefaultsAndAllowlist covers the five and the default.
A checkpoint held only "offset:incarnation". When the server changed a source's path under the same id, the agent resumed the new file at the old file's offset; a smaller new file then read as truncated and was restarted at 0 and shipped whole at once. For a legitimate edit that re-ships or skips lines; for a compromised server it turned "tail this log" into "send this file now". A checkpoint now also names its file (a hash of the path), and one that names another file is ignored, so the source starts from the end like a new one. The format stays readable both ways: a checkpoint an older agent wrote has no binding and still resumes, and an older agent reading the new line ignores the third field, as fmt.Sscanf already did. Also reads the state directory through snapshotConfig, as the rest of the manager does, instead of the unlocked field. Tested: TestLogCheckpointIsBoundToItsPath (same path resumes, another path does not, an old checkpoint still resumes).
CI runs gosec on Linux, where the task sandbox (taskexec_linux.go), the statfs readers and the cgroup writers compile; the macOS scans the triage started from never saw these twelve. Each is guarded: the task timeout is bounded by resolveTaskTimeout before it becomes a CPU limit, the shim re-execs this agent's own binary with an allowlisted interpreter, the cgroup root is the agent's own -task-cgroup, and the cgroupfs control files already exist, so the 0644 mode is never applied. Tested: the gosec step on GOOS=linux with v2.29.0 and v2.30.0: both exit 0 with 65 files and 0 issues. GOOS=linux go vet ./... is clean.
The agent tailed whatever LogSource.Path the server sent and shipped the lines back, as its own user, usually root. Only the server checked the path (validateLogPath), so a compromised control plane could read appended lines of any root-readable file on every node, and whole files after a rewrite, with no node opt-in. Node-side vetoes exist for that case, so the node now holds its own allowlist: - It tails only regular files under /var/log/, the server's default, plus the absolute directories in -log-path-allow (LATTICE_LOG_PATH_ALLOW in the agent env file, which the installer now carries through the sudo re-exec, keeps across reinstalls and writes for systemd and launchd). Nothing the server sends widens it. - A path is judged after symlinks and ".." are resolved, and again on every open, rotations included: the tailer opens through the policy, with O_NOFOLLOW and O_NONBLOCK, refuses a non-regular file, and where /proc names the opened descriptor, judges that name too. - A path that is not an absolute file path is refused like one outside the allowlist. That covers singbox:// and agent-debug://, which the server never sends to the tailer (it filters "://" paths out of /api/agent/log-sources; sing-box lines reach the server through the trace collector, unchanged), and any scheme a server might invent. - A refused source is not tailed, is logged on the node, and is the log_sources step's error, which every heartbeat carries as loop health; the server shows that step and derives no status from it. The text reads "refused by the node: outside its log allowlist: <id> <path>". A source retargeted under the same id already starts at the end of its new file (5e89e7f), so retargeting cannot ship a file whole. Compatibility: the server mints file sources only under its own allowlist, /var/log/ unless LATTICE_LOG_PATH_ALLOW widens it, and its managed sources are virtual. A fleet that widened the server's list must set the same LATTICE_LOG_PATH_ALLOW on its nodes, or those sources show as refused on the node's loop health. Tested: five tests in logtail_veto_test.go, written before the code. With the veto disabled, the refusal, rotation and reporting tests fail; with the checkpoint binding disabled, the retarget test ships the new file's content. The installer contract check fails on the old install.sh. test-install-integrity.sh and check-release-workflow.sh pass.
… allowlist -no-log-tail (LATTICE_NO_LOG_TAIL, failing closed like LATTICE_NO_BEACON) turns log tailing off on the node: the agent still reads its assigned sources, tails none, and reports each as refused. The installer carries it like the allowlist. The refusal now reads as an instruction, "outside this node's log allowlist; widen it with LATTICE_LOG_PATH_ALLOW on the node", and refusals are grouped under their reason, reason first, so the sentence survives the 200-rune cut loop health applies. Tested: TestNoLogTailRefusesEverySource and TestLogRefusalsGroupByReason were written first and failed against the previous behaviour, as did the updated refusal tests; all pass now. The installer contract check covers both variables.
…g text Follow-ups from the security review of the log path veto: - A checkpoint an older agent wrote names no file, and 5e89e7f still resumed it for whatever path the source names. A source idle since before the upgrade and retargeted under the same id therefore resumed at a stale offset, and a smaller new file restarted at 0 and shipped whole. An unbound checkpoint is now ignored, so the source starts at the end; the cost is the lines written during the upgrade restart, once. - The source id comes from the server, so node log lines and the log_sources step error now quote it with %q, as they already quoted the path. - A tailer that reconcile has stopped no longer records an open-time refusal after it was stopped, which could flap the step error. - The agent logs its effective allowlist at start, and warns when it holds "/", which lets the server name any file the agent can read. Tested: TestLogCheckpointIsBoundToItsPath, the new older-agent case of TestRetargetedLogSourceStartsAtTheEnd and TestLogRefusalsGroupByReason failed first (the retarget case shipped the new file's line) and pass now.
The node's built-in /var/log/ allowlist let a compromised control plane tail new lines of auth.log, secure, the audit log or sudo's session logs on every node. The default now leaves those out: auth.log*, secure*, btmp*, wtmp*, lastlog, faillog, sudo.log* directly under /var/log/, and everything under audit/ and sudo-io/. The names are judged on the path as written, after symlinks and ".." are resolved, and on the opened file's real name, so a symlink to auth.log is refused from inside /var/log/ or from a listed directory. A directory the node lists in LATTICE_LOG_PATH_ALLOW is taken literally and tailed whole, which is how a node opts in to one of those files. The setting still adds to /var/log/ rather than replacing it, matching the server's variable of the same name. Rejected: an empty default, which would make log tailing unusable until every node is configured. Rejected: dropping the denylist whenever the variable is set, which would open auth.log for a node that only listed an application directory. Not-tested: per-host logs a syslog collector keeps in subdirectories of /var/log/, and the systemd journal, are not on the list.
…logs On a filesystem that folds case (APFS, ext4 casefold, vfat) the kernel opens /var/log/auth.log when asked for /var/log/AUTH.log, and nothing in path resolution turns the name back, so the denylist compared exact case and let it through. Entries are now lowercase and matched without case. The list also takes in files a standard install leaves under /var/log/ that can carry a password or user-data: tallylog, cloud-init*, and the installer/, anaconda/ and private/ directories. A listed parent of /var/log/, such as /var/, takes in all of it under the rule that a listed directory is taken literally. That stays, since only the node can set it, but the agent now warns at start when it happens.
…ates /var/log/ A node could only opt in to auth.log by listing /var/log/, because every entry was a directory prefix, and listing /var/log/ lifted the whole denylist. That is the setting an operator writes to restate the default, and it opened every credential and audit log without saying so. An entry now also names the file of that name, so LATTICE_LOG_PATH_ALLOW=/var/log/auth.log allows that file and nothing else. Under /var/log/, a denied name passes only through an entry strictly inside it: the file itself or a directory such as /var/log/audit. Listing /var/log/ or a parent of it, /var/ or /, keeps the denylist. The refusal now tells the operator to list the file, and the start-up warning for a listed parent is gone with the case it described.
A listed parent such as /var/ no longer lifts the default denylist, but it still lets the server name any other file under it, /var/lib/ and the rest. The agent now says so at start, as it already does for "/".
1 of 3 tasks
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
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.
Summary
CI's gosec step has never scanned this repository. It ran
gosec ./lattice-node-agent/...from the parent of the checkout, where gosec v2.29.0 loads no package: run 40086050635 printsFiles: 0. Run inside the module, v2.29.0 and v2.30.0 both report the same 74 findings. All were read and handled, and the tree now reports 0 under both versions.The agent runs as root on every node, so real findings are release blockers.
Release blocker, fixed: a server-sent log source id escaped the state directory (
cmd/lattice-agent/logtail.gocheckpointPath, found by G304). The agent wrotefilepath.Join(-log-state-dir, id+".ckpt")as root after aMkdirAll, with no check on the id, so../../etc/cron.d/xcreated directories and wrote a file outside the state directory. An id that is not a plain file name now gets no checkpoint, and the source still tails from the end. The server only mints plain ids, so nothing legitimate changes.Also fixed:
terminal.go,terminal_stream.go; G204). The agent now holds the shell to the five the server already allows, on both transports, and fails the session before anything runs otherwise.Everything else is behaviour-neutral:
_ =discards on cleanup paths.// #nosec <rule> -- <guard>comments naming each guard.-exclude-generatedin CI for the protoc output, whose 4 G103 findings are protobuf's own.CI: the gosec step runs inside the module, and fails when the report shows zero files or any package load error. gosec v2.29.0 alone exits 0 in both cases.
Node-side log path veto (ships as v0.3.10-alpha.9). Until now the agent tailed whatever
LogSource.Paththe server sent, as root, and only the server checked it. A compromised control plane could read root-readable files on every node, which the security review rated HIGH. The agent now decides for itself:/var/log/by default, widened by-log-path-allow(LATTICE_LOG_PATH_ALLOWin the agent env file) and never by anything the server sends...and symlinks, and again on every open, rotations included. It opens withO_NOFOLLOW|O_NONBLOCK, refuses a non-regular descriptor, and on Linux checks the opened file's real name through/proc/self/fd./var/log/leaves out the files that hold credentials or audit trails:auth.log*,secure*,btmp*,wtmp*,lastlog,faillog,tallylog,sudo.log*,cloud-init*directly under it, and everything underaudit/,sudo-io/,installer/,anaconda/andprivate/. Those names are matched without case (a case-folding filesystem opensauth.logasAUTH.log) and judged as written, after resolution, and on the opened file's real name, so a symlink toauth.logis refused wherever the link sits. The refusal readsa credential or audit log this node's default leaves out; allow it by listing the file in LATTICE_LOG_PATH_ALLOW on the node.LATTICE_LOG_PATH_ALLOWis taken whole and may name a file or a directory. The setting adds to/var/log/rather than replacing it, matching the server's variable of the same name./var/log/that holds it (/var/log/auth.log,/var/log/audit), and gets exactly that. Listing/var/log/itself or a parent such as/var/restates the default and keeps the denylist, since that is the setting an operator could write without meaningauth.log.-helpand the README state both rules.singbox://...,singbox://../../etc/shadow, other or unknown schemes, relative paths. The server already filters its virtualsingbox://andagent-debug://sources out before the agent sees them, so nothing it mints today is affected.-no-log-tail(LATTICE_NO_LOG_TAIL, fails closed) tails nothing.log_sourcesstep error in the loop health every heartbeat carries, with the instruction first:refused by the node: outside this node's log allowlist; widen it with LATTICE_LOG_PATH_ALLOW on the node: "<id>" "<path>". The agent has no per-source status channel; loop health is the one it already uses for a failing step.LATTICE_LOG_PATH_ALLOWthrough the sudo re-exec, keeps it across reinstalls, and writes it for systemd and launchd.Compatibility: a fleet that widened the server's
LATTICE_LOG_PATH_ALLOWmust set the same value on its nodes, or those sources show as refused. No production fleet has.Not covered by the denylist: per-host logs a syslog collector keeps in subdirectories of
/var/log/, the systemd journal,sssd/debug logs, and Alpine's busyboxsyslogd, which writes sshd, su and sudo lines into/var/log/messages. macOS has no/proc, so the post-open real-path check runs on Linux only (review LOW).Test plan
gofmt -l .andgo vet ./...: clean.go test -race -cover ./...: every package ok, 0DATA RACE.TestLogCheckpointPathStaysInStateDirTestResumePastAnyOffsetStartsOverTestTerminalRefusesAShellOutsideTheAllowlist(both transports)TestLogCheckpointIsBoundToItsPathcovers the new checkpoint format.TestLogPathAllowlistDefaultsAndWideningTestLogPathPolicyRefusesWhatItCannotVouchFor(schemes, relative,.., symlink out, directory, FIFO, NUL)TestLogTailerRefusesARotationOutOfTheAllowlistTestRetargetedLogSourceStartsAtTheEnd(bound and older-agent checkpoints)TestRefusedLogSourceIsReportedAndNotTailed,TestNoLogTailRefusesEverySource,TestLogRefusalsGroupByReasonTestDefaultLogAllowlistLeavesOutCredentialAndAuditLogs,TestNodeLogAllowlistIsTakenLiterally(254c7cf, d330b4e, d3d5cbc): listingauth.logallows it and notauth.log.1; listing/var/log/,/var/logor its parent keeps the denylistinstall.shand passes on the new one./var/log/lifting the denylist, is closed by d3d5cbc, which was not re-reviewed; it only narrows what an entry can lift.go vet(darwin and linux) clean;go test -race -count=1 ./...all 25 packages ok, 0DATA RACE; the gosec step with v2.29.0 and v2.30.0 on darwin and linux, 66 files, 0 issues, no load error in all four.sh scripts/check-release-workflow.shandsh scripts/test-install-integrity.sh: ok.