fix: HTTP server read timeouts and scratch directory permissions - #225
Open
peatey wants to merge 2 commits into
Open
fix: HTTP server read timeouts and scratch directory permissions#225peatey wants to merge 2 commits into
peatey wants to merge 2 commits into
Conversation
The server on :9003 was constructed as &http.Server{Addr, Handler} with no
timeouts at all. Without ReadHeaderTimeout a client can hold a connection
open indefinitely by dribbling out header bytes a few at a time, and
enough such connections exhaust the accept capacity -- the Slowloris
pattern (gosec G112).
Sets ReadHeaderTimeout (10s), which is the timeout that actually closes
that hole, plus ReadTimeout (30s) to bound a complete request and
IdleTimeout (120s) to bound retained keep-alive connections. Every route
on this server is a small GET, so these are generous.
WriteTimeout is deliberately left unset. When PPROF_ENABLED is on, this
server exposes /debug/pprof/profile and /debug/pprof/trace, which stream a
response for the entire duration of the requested profile -- 30s by
default, longer with ?seconds=. A WriteTimeout would truncate those
mid-profile, and it would not add anything against slow-client attacks,
which the read-side timeouts already address.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The scratch, upload and per-sample directories were created with os.ModePerm, which is 0777 -- world-readable and world-writable (gosec G301). Those directories hold a full inventory of the cluster's Kubernetes resources between collection and upload, so any other user on the host could read the collected data, or write into the tree that gets tarred and shipped. Replaces os.ModePerm with a named sampleDirPerm constant at 0750 across all three creation sites: createIfNotExists (which covers both the scratch and upload roots), and the two per-sample os.Mkdir calls for the current and next sample paths. Fixing only the first would have left the sample directories themselves at 0777. 0750 rather than 0700 so group access is retained, which some Kubernetes volume setups rely on when a pod sets fsGroup. The agent is the only writer either way. Files inside are still created by os.Create at the usual 0644, but they are no longer reachable by other users because the containing directories are no longer world-traversable. Verified the resulting mode on disk is rwxr-x--- rather than rwxrwxrwx, and the cldy suite -- which creates these directories and round-trips real sample data through them -- passes unchanged. Not touched: cmd/bingen-to-json also trips G301/G306, but it is a local code-generation utility that writes non-sensitive generated JSON, not part of the agent runtime. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.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
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
Two small, independent hardening fixes found during the security review. Two
commits, revertible separately.
Deliberately not accompanied by enabling
gosecin.golangci.yaml.Turning that linter on cold surfaces 15 issues, most of them noise for this
codebase (MD5 used for the S3
Content-MD5header the API requires,G304oninternally-constructed file paths, theoretical
G115overflows), and CodeQLplus Mend already cover this ground. These are just the two findings worth
acting on.
1.
pkg/http/server.go— no timeouts on the agent HTTP serverThe server on
:9003was built as&http.Server{Addr, Handler}with notimeouts at all.
Without
ReadHeaderTimeout, a client can hold a connection open indefinitely bydribbling out header bytes a few at a time; enough such connections exhaust the
accept capacity. That's Slowloris (
gosecG112). This server carries/healthzand
/metrics, so wedging it also blanks liveness probing and metrics scraping.Sets:
ReadHeaderTimeoutReadTimeoutIdleTimeoutEvery route here is a small GET, so these are generous.
WriteTimeoutis deliberately left unset, and this is the part worth areviewer's attention. When
PPROF_ENABLEDis on, this same server exposes/debug/pprof/profileand/debug/pprof/trace, which stream a response for thewhole duration of the requested profile — 30s by default, longer with
?seconds=. AWriteTimeoutwould truncate those mid-profile. It also wouldn'tadd anything against slow-client attacks, which the read-side timeouts already
handle.
2.
cldy/emitter.go— scratch directories created 0777Three sites created directories with
os.ModePerm, which is 0777 —world-readable and world-writable (
gosecG301). These hold a full inventoryof the cluster's Kubernetes resources between collection and upload, so any
other user on the host could read the collected data, or write into the tree
that subsequently gets tarred and shipped to Cloudability.
Replaced with a named
sampleDirPerm = 0o750across all three:createIfNotExists— covers both the scratch and upload rootsos.Mkdir(ce.currentSamplePath, …)os.Mkdir(ce.nextSamplePath, …)Fixing only the first would have left the per-sample directories at 0777, which
is where the data actually lands.
0750rather than0700so group access is retained — some Kubernetes volumesetups rely on it when a pod sets
fsGroup. The agent is the only writer eitherway. Files inside are still created by
os.Createat the usual 0644, but theyare no longer reachable by other users because the containing directories are no
longer world-traversable.
Verification
Checked the resulting mode on disk rather than trusting the constant, since
umask can interfere:
The
cldysuite creates these directories and round-trips real sample datathrough them, and passes unchanged — so 0750 is proven sufficient for the
agent's own read/write path.
Not touched
cmd/bingen-to-jsonalso trips G301/G306, but it is a local code-generationutility writing non-sensitive generated JSON, not part of the agent runtime.
Relationship to other open PRs
Independent of #223 (dependency sweep) and #224 (govulncheck gate). Branched
from
develop, touches neithergo.modnor CI config, so it can merge in anyorder.
🤖 Generated with Claude Code