bootstrap: set ReadHeaderTimeout on the HTTP server#421
Open
randomizedcoder wants to merge 1 commit into
Open
Conversation
randomizedcoder
force-pushed
the
bootstrap-read-header-timeout
branch
from
July 20, 2026 21:52
b4a5428 to
950fed8
Compare
randomizedcoder
marked this pull request as ready for review
July 20, 2026 21:52
|
|
||
| // serverReadHeaderTimeout bounds request-header reads to mitigate Slowloris | ||
| // (gosec G112). Package var so tests can shorten it. | ||
| var serverReadHeaderTimeout = time.Minute |
Contributor
There was a problem hiding this comment.
suggestion: Rather than a package-level var (only overridable by this package's own tests), I'd consider adding a ReadHeaderTimeout time.Duration field to Config, defaulting to time.Minute when unset, that matches how the rest of Config is configured (plain struct fields, not functional options) and lets downstream exporters tune it too.
randomizedcoder
force-pushed
the
bootstrap-read-header-timeout
branch
3 times, most recently
from
July 23, 2026 15:52
317d519 to
cdc4a37
Compare
The *http.Server built by Runner.newServer left ReadHeaderTimeout unset, so request-header reads were unbounded -- a Slowloris-style connection exhaustion vector (gosec G112). The exporter-toolkit web package does not set server timeouts either, so every bootstrap-based exporter inherited the gap. Add a ReadHeaderTimeout field to bootstrap.Config, defaulting to one minute when unset. It bounds only header reading, not the metrics handler, so it never affects legitimate scrapes, and -- like every other Config field -- downstream exporters can tune it to their own needs. Follow-up to prometheus/blackbox_exporter#1626, which made the same change on the caller side; setting it here fixes it once for all toolkit users. go build, go vet, and go test ./... (incl. -race) pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: randomizedcoder dave.seddon.ca@gmail.com <dave.seddon.ca@gmail.com>
randomizedcoder
force-pushed
the
bootstrap-read-header-timeout
branch
from
July 23, 2026 15:58
cdc4a37 to
59d9579
Compare
nicolastakashi
approved these changes
Jul 23, 2026
Contributor
|
@SuperQ LGTM, waiting for your final review. |
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
2 participants
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.
Hi again 👋 — a small hardening follow-up, as mentioned in #420.
Why
Runner.newServerinbootstrap/returns&http.Server{Handler: mux}withReadHeaderTimeoutunset, so request-header reads are unbounded — a Slowloris-style connection-exhaustion vector (gosec G112). Because thewebpackage does not set server timeouts either, every bootstrap-based exporter inherits the gap.This is the toolkit-side counterpart to prometheus/blackbox_exporter#1626, which set the same timeout on the caller side. Fixing it here addresses it once for all toolkit users.
What
Add a
ReadHeaderTimeout time.Durationfield tobootstrap.Config, defaulting to one minute when unset. It bounds only header reading, not the metrics handler, so it never affects legitimate scrapes (which can run for the full scrape timeout), and — like every otherConfigfield — downstream exporters can tune it to their own needs.Table-driven tests assert the field maps onto the server (default when unset, configured value otherwise, non-positive treated as unset); a behavioral test confirms the server closes a connection whose headers never complete while a well-formed request still succeeds.
Thanks to @nicolastakashi for the excellent suggestion to make this a
Configfield rather than a package-level var — it's a much better fit for how the rest ofConfigis configured (plain struct fields, not functional options) and lets downstream exporters tune it too.go build,go vet,go test ./...(incl.-race) pass; gosec G112 clears. Happy to adjust the value if a different bound is preferred.🤖 Generated with Claude Code