bootstrap: set ReadHeaderTimeout on the HTTP server - #421
Conversation
b4a5428 to
950fed8
Compare
|
|
||
| // serverReadHeaderTimeout bounds request-header reads to mitigate Slowloris | ||
| // (gosec G112). Package var so tests can shorten it. | ||
| var serverReadHeaderTimeout = time.Minute |
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.
cdc4a37 to
59d9579
Compare
|
@SuperQ LGTM, waiting for your final review. |
There was a problem hiding this comment.
Pull request overview
Adds configurable HTTP request-header timeouts to harden bootstrap-based exporters against Slowloris attacks.
Changes:
- Defaults
ReadHeaderTimeoutto one minute. - Allows positive timeout overrides through
bootstrap.Config. - Adds configuration and end-to-end timeout tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
bootstrap/bootstrap.go |
Configures the server header-read timeout. |
bootstrap/bootstrap_test.go |
Tests timeout mapping and stalled connections. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // ReadHeaderTimeout bounds request-header reads, mitigating Slowloris | ||
| // (gosec G112). Defaults to one minute when zero. | ||
| ReadHeaderTimeout time.Duration |
ArthurSens
left a comment
ArthurSens
left a comment
There was a problem hiding this comment.
LGTM once the co-pilot nit is fixed :)
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>
59d9579 to
ab92444
Compare
|
@ArthurSens Thanks for the feedback. Addressed I'd also love a review on: #396 |
|
Thanks, I'll try to take a look at the linked issue once I find some time 😭 |
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