From 60d6d57625e7b1316e513a732287eb8cd1802cb4 Mon Sep 17 00:00:00 2001 From: mhx Date: Thu, 3 Sep 2026 08:52:51 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20drain=20the=20appsec=20response?= =?UTF-8?q?=20body=20so=20connections=20are=20reused?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix #384. appsecQuery never read the appsec response body. net/http only returns a connection to the idle pool once its body has reached EOF, so closing early discarded it: every request opened a fresh TCP connection to the appsec host and left it in TIME_WAIT. crowdsecQuery, two functions above, does call io.ReadAll, which is why the LAPI path in the same process pools correctly. Measured with an httptest appsec server over 10 calls: empty response body -> 1 connection (already at EOF, reuse worked) non-empty response body -> 10 connections (one per request) The drain goes in the defer rather than before the final return, because the 500 and non-200 paths return earlier. Draining only at the end fixes 200 and leaves the other two leaking: status 200 -> 1 connection status 403 -> 10 status 500 -> 10 which is the wrong half to fix: 403 is what appsec produces for a site under attack, and 500 is what a wedged appsec produces. Also removes the unreachable `if err != nil { ... appsecQuery:readBody }` left over from a version that did read the body -- err is always nil there -- and sets MaxIdleConnsPerHost on the three transports. They each talk to a single host, so the unset default of 2 (DefaultMaxIdleConnsPerHost) capped the pool well below the configured MaxIdleConns of 10. Adds Test_appsecQuery_reusesConnection, which asserts one connection for ten calls across 200/403/500 and fails on the previous code. --- bouncer.go | 26 +++++++++++++++---------- bouncer_test.go | 50 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 10 deletions(-) diff --git a/bouncer.go b/bouncer.go index 5f02e27..379a4e3 100644 --- a/bouncer.go +++ b/bouncer.go @@ -243,17 +243,19 @@ func New(_ context.Context, next http.Handler, config *configuration.Config, nam }, httpClient: &http.Client{ Transport: &http.Transport{ - MaxIdleConns: 10, - IdleConnTimeout: 30 * time.Second, - TLSClientConfig: tlsConfig, + MaxIdleConns: 10, + MaxIdleConnsPerHost: 10, + IdleConnTimeout: 30 * time.Second, + TLSClientConfig: tlsConfig, }, Timeout: time.Duration(config.HTTPTimeoutSeconds) * time.Second, }, httpAppsecClient: &http.Client{ Transport: &http.Transport{ - MaxIdleConns: 10, - IdleConnTimeout: 30 * time.Second, - TLSClientConfig: tlsAppsecConfig, + MaxIdleConns: 10, + MaxIdleConnsPerHost: 10, + IdleConnTimeout: 30 * time.Second, + TLSClientConfig: tlsAppsecConfig, }, Timeout: time.Duration(config.HTTPTimeoutSeconds) * time.Second, }, @@ -278,7 +280,7 @@ func New(_ context.Context, next http.Handler, config *configuration.Config, nam log, bouncer.cacheClient, &http.Client{ - Transport: &http.Transport{MaxIdleConns: 10, IdleConnTimeout: 30 * time.Second}, + Transport: &http.Transport{MaxIdleConns: 10, MaxIdleConnsPerHost: 10, IdleConnTimeout: 30 * time.Second}, Timeout: time.Duration(config.HTTPTimeoutSeconds) * time.Second, }, config.CaptchaProvider, @@ -801,6 +803,13 @@ func appsecQuery(bouncer *Bouncer, ip string, httpReq *http.Request) error { return nil } defer func() { + // net/http only returns a connection to the idle pool once its body has + // been read to EOF; closing early discards it. Drain here rather than at + // the end of the function so the 500 and non-200 paths, which return + // earlier, keep their connections too. + if _, errDrain := io.Copy(io.Discard, res.Body); errDrain != nil { + bouncer.log.Debug("appsecQuery:drainBody " + errDrain.Error()) + } if err = res.Body.Close(); err != nil { bouncer.log.Error("appsecQuery:closeBody " + err.Error()) } @@ -816,9 +825,6 @@ func appsecQuery(bouncer *Bouncer, ip string, httpReq *http.Request) error { return fmt.Errorf("appsecQuery statusCode:%d", res.StatusCode) } - if err != nil { - return fmt.Errorf("appsecQuery:readBody %w", err) - } return nil } diff --git a/bouncer_test.go b/bouncer_test.go index 8eb03ed..7694f5c 100644 --- a/bouncer_test.go +++ b/bouncer_test.go @@ -2,12 +2,14 @@ package crowdsec_bouncer_traefik_plugin //nolint:revive,stylecheck import ( "context" + "fmt" "io" "net/http" "net/http/httptest" "net/url" "reflect" "strings" + "sync" "testing" "text/template" "time" @@ -555,3 +557,51 @@ func Test_appsecQuery_unreadableBodyGetNotDropped(t *testing.T) { t.Fatal("appsecQuery() blocked on an HTTP/3 GET request body (issue #351 regression)") } } + +// Test_appsecQuery_reusesConnection is a regression test for issue #384: the +// appsec response body must be drained so net/http can return the connection to +// the idle pool. Without it every request opened a fresh TCP connection and left +// it in TIME_WAIT. The 403 and 500 cases matter as much as 200: they return +// before the end of the function, and they are what a site under attack or a +// wedged appsec produce in volume. +func Test_appsecQuery_reusesConnection(t *testing.T) { + for _, status := range []int{http.StatusOK, http.StatusForbidden, http.StatusInternalServerError} { + t.Run(http.StatusText(status), func(t *testing.T) { + var mu sync.Mutex + conns := map[string]bool{} + appsecServer := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + mu.Lock() + conns[r.RemoteAddr] = true + mu.Unlock() + rw.WriteHeader(status) + // a non-empty body is what makes the connection unusable when it + // is not drained; an empty one is already at EOF + fmt.Fprint(rw, `{"action":"allow"}`) + })) + defer appsecServer.Close() + + appsecURL, _ := url.Parse(appsecServer.URL) + bouncer := &Bouncer{ + appsecScheme: appsecURL.Scheme, + appsecHost: appsecURL.Host, + appsecPath: "/", + appsecBodyLimit: 10485760, + appsecFailureBlock: false, + httpAppsecClient: appsecServer.Client(), + log: logger.New("INFO", ""), + } + + const calls = 10 + for range calls { + req, _ := http.NewRequest(http.MethodGet, "http://localhost/", nil) + _ = appsecQuery(bouncer, "1.2.3.4", req) + } + + mu.Lock() + defer mu.Unlock() + if len(conns) != 1 { + t.Errorf("appsecQuery() opened %d connections for %d calls, want 1 (response body not drained?)", len(conns), calls) + } + }) + } +}