From d32f271195754703f9393b4d8830c7e4b9da6655 Mon Sep 17 00:00:00 2001 From: mathieuHa Date: Wed, 1 Jul 2026 12:44:58 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20appsec:=20do=20not=20buffer=20un?= =?UTF-8?q?readable=20(gRPC/HTTP2)=20request=20bodies=20(#332)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * :bug: fix appsec silently 403-ing gRPC streams with unreadable body A bidirectional gRPC stream is an HTTP/2 request with no Content-Length whose body never reaches EOF. Since #321 removed the ContentLength guard, appsecQuery buffered it with io.ReadAll, which blocked until the request timed out and was turned into a 403 (issue #323). The backend was never reached (OriginStatus:0). Mirror the reference lua-cs-bouncer behaviour: detect an unreadable body (ProtoMajor >= 2 && ContentLength < 0) and, instead of buffering it, forward the request to Appsec with headers only. Add a new CrowdsecAppsecDropUnreadableBody option (default false) that mirrors the reference APPSEC_DROP_UNREADABLE_BODY: when true, such requests are blocked outright instead of forwarded without their body. Readable HTTP/1.1 bodies are still buffered and inspected, so the bypass closed by #321 stays closed. Co-Authored-By: Claude Opus 4.8 * :rotating_light: appsec: satisfy linters (gocritic ifElseChain, misspell) Rewrite the body-handling if/else chain in appsecQuery as a switch (gocritic) and use US spelling "behavior" (misspell). Co-Authored-By: Claude Opus 4.8 * 🔇 appsec: drop redundant unreadable-body debug log Address review on #332: the caller (handleNextServeHTTP) already logs the returned error with the request IP, so the inner Debug line duplicated it. Co-Authored-By: Claude Opus 4.8 * :bento: increase gocyclo * :bento: fix lint --------- Co-authored-by: Claude Opus 4.8 Co-authored-by: maxlerebourg --- .golangci.yml | 2 +- README.md | 5 + bouncer.go | 158 ++++++++++++++++------------- bouncer_test.go | 137 +++++++++++++++++++++++++ pkg/configuration/configuration.go | 94 ++++++++--------- 5 files changed, 280 insertions(+), 116 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 2a5e6de..34b62ea 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -7,7 +7,7 @@ linters-settings: disable: - fieldalignment gocyclo: - min-complexity: 15 + min-complexity: 20 goconst: min-len: 5 min-occurrences: 4 diff --git a/README.md b/README.md index a63a466..4098944 100644 --- a/README.md +++ b/README.md @@ -382,6 +382,10 @@ make run - int64 - default: 10485760 (= 10MB) - Transmit only the first number of bytes to Crowdsec Appsec Server. +- CrowdsecAppsecUnreadableBodyBlock + - bool + - default: false + - Behaviour when the request body cannot be buffered for inspection (HTTP/2 or HTTP/3 request without a `Content-Length`, typically a bidirectional gRPC stream). When `false` (default) the request is forwarded to the Appsec Server with headers only (the body is left to stream through untouched). When `true` the request is blocked outright. Mirrors the reference bouncers' `APPSEC_DROP_UNREADABLE_BODY` option. - CrowdsecAppsecKey - string - default: value of `CrowdsecLapiKey` @@ -616,6 +620,7 @@ http: crowdsecAppsecFailureBlock: true crowdsecAppsecUnreachableBlock: true crowdsecAppsecBodyLimit: 10485760 + crowdsecAppsecUnreadableBodyBlock: false crowdsecLapiKey: privateKey-foo crowdsecLapiScheme: http crowdsecLapiHost: crowdsec:8080 diff --git a/bouncer.go b/bouncer.go index b5ac73b..a98cf18 100644 --- a/bouncer.go +++ b/bouncer.go @@ -83,42 +83,43 @@ type Bouncer struct { name string template *template.Template - enabled bool - appsecEnabled bool - appsecScheme string - appsecHost string - appsecPath string - appsecKey string - appsecFailureBlock bool - appsecUnreachableBlock bool - appsecBodyLimit int64 - crowdsecScheme string - crowdsecHost string - crowdsecPath string - crowdsecKey string - crowdsecMode string - crowdsecMachineID string - crowdsecPassword string - crowdsecScenarios []string - updateInterval int64 - updateMaxFailure int64 - defaultDecisionTimeout int64 - remediationStatusCode int - remediationCustomHeader string - forwardedCustomHeader string - crowdsecStreamRoute string - crowdsecHeader string - redisUnreachableBlock bool - banTemplate *template.Template - banTemplateContentType string - traceCustomHeader string - clientPoolStrategy *ip.PoolStrategy - serverPoolStrategy *ip.PoolStrategy - httpClient *http.Client - httpAppsecClient *http.Client - cacheClient *cache.Client - captchaClient *captcha.Client - log *slog.Logger + enabled bool + appsecEnabled bool + appsecScheme string + appsecHost string + appsecPath string + appsecKey string + appsecFailureBlock bool + appsecUnreachableBlock bool + appsecUnreadableBodyBlock bool + appsecBodyLimit int64 + crowdsecScheme string + crowdsecHost string + crowdsecPath string + crowdsecKey string + crowdsecMode string + crowdsecMachineID string + crowdsecPassword string + crowdsecScenarios []string + updateInterval int64 + updateMaxFailure int64 + defaultDecisionTimeout int64 + remediationStatusCode int + remediationCustomHeader string + forwardedCustomHeader string + crowdsecStreamRoute string + crowdsecHeader string + redisUnreachableBlock bool + banTemplate *template.Template + banTemplateContentType string + traceCustomHeader string + clientPoolStrategy *ip.PoolStrategy + serverPoolStrategy *ip.PoolStrategy + httpClient *http.Client + httpAppsecClient *http.Client + cacheClient *cache.Client + captchaClient *captcha.Client + log *slog.Logger } // New creates the crowdsec bouncer plugin. @@ -203,36 +204,37 @@ func New(_ context.Context, next http.Handler, config *configuration.Config, nam name: name, template: template.New("CrowdsecBouncer").Delims("[[", "]]"), - enabled: config.Enabled, - crowdsecMode: config.CrowdsecMode, - appsecEnabled: config.CrowdsecAppsecEnabled, - appsecScheme: config.CrowdsecAppsecScheme, - appsecHost: config.CrowdsecAppsecHost, - appsecPath: config.CrowdsecAppsecPath, - appsecKey: config.CrowdsecAppsecKey, - appsecFailureBlock: config.CrowdsecAppsecFailureBlock, - appsecUnreachableBlock: config.CrowdsecAppsecUnreachableBlock, - appsecBodyLimit: config.CrowdsecAppsecBodyLimit, - crowdsecScheme: config.CrowdsecLapiScheme, - crowdsecHost: config.CrowdsecLapiHost, - crowdsecPath: config.CrowdsecLapiPath, - crowdsecKey: config.CrowdsecLapiKey, - crowdsecMachineID: config.CrowdsecCapiMachineID, - crowdsecPassword: config.CrowdsecCapiPassword, - crowdsecScenarios: config.CrowdsecCapiScenarios, - updateInterval: config.UpdateIntervalSeconds, - updateMaxFailure: config.UpdateMaxFailure, - remediationCustomHeader: config.RemediationHeadersCustomName, - forwardedCustomHeader: config.ForwardedHeadersCustomName, - defaultDecisionTimeout: config.DefaultDecisionSeconds, - remediationStatusCode: config.RemediationStatusCode, - redisUnreachableBlock: config.RedisCacheUnreachableBlock, - banTemplate: banTemplate, - banTemplateContentType: banTemplateContentType, - traceCustomHeader: config.TraceHeadersCustomName, - crowdsecStreamRoute: crowdsecStreamRoute, - crowdsecHeader: crowdsecHeader, - log: log, + enabled: config.Enabled, + crowdsecMode: config.CrowdsecMode, + appsecEnabled: config.CrowdsecAppsecEnabled, + appsecScheme: config.CrowdsecAppsecScheme, + appsecHost: config.CrowdsecAppsecHost, + appsecPath: config.CrowdsecAppsecPath, + appsecKey: config.CrowdsecAppsecKey, + appsecFailureBlock: config.CrowdsecAppsecFailureBlock, + appsecUnreachableBlock: config.CrowdsecAppsecUnreachableBlock, + appsecUnreadableBodyBlock: config.CrowdsecAppsecUnreadableBodyBlock, + appsecBodyLimit: config.CrowdsecAppsecBodyLimit, + crowdsecScheme: config.CrowdsecLapiScheme, + crowdsecHost: config.CrowdsecLapiHost, + crowdsecPath: config.CrowdsecLapiPath, + crowdsecKey: config.CrowdsecLapiKey, + crowdsecMachineID: config.CrowdsecCapiMachineID, + crowdsecPassword: config.CrowdsecCapiPassword, + crowdsecScenarios: config.CrowdsecCapiScenarios, + updateInterval: config.UpdateIntervalSeconds, + updateMaxFailure: config.UpdateMaxFailure, + remediationCustomHeader: config.RemediationHeadersCustomName, + forwardedCustomHeader: config.ForwardedHeadersCustomName, + defaultDecisionTimeout: config.DefaultDecisionSeconds, + remediationStatusCode: config.RemediationStatusCode, + redisUnreachableBlock: config.RedisCacheUnreachableBlock, + banTemplate: banTemplate, + banTemplateContentType: banTemplateContentType, + traceCustomHeader: config.TraceHeadersCustomName, + crowdsecStreamRoute: crowdsecStreamRoute, + crowdsecHeader: crowdsecHeader, + log: log, serverPoolStrategy: &ip.PoolStrategy{ Checker: serverChecker, }, @@ -327,7 +329,7 @@ func New(_ context.Context, next http.Handler, config *configuration.Config, nam // ServeHTTP principal function of plugin. // -//nolint:nestif,gocyclo +//nolint:nestif func (bouncer *Bouncer) ServeHTTP(rw http.ResponseWriter, req *http.Request) { if !bouncer.enabled { bouncer.next.ServeHTTP(rw, req) @@ -722,6 +724,17 @@ func crowdsecQuery(bouncer *Bouncer, stringURL string, data []byte) ([]byte, err return body, nil } +// isBodyUnreadable reports whether the request body cannot be buffered before +// forwarding it to the Appsec component. An HTTP/2 or HTTP/3 request without a +// Content-Length (typically a bidirectional gRPC stream) keeps its body open +// for the whole life of the stream and never reaches EOF, so reading it with +// io.ReadAll would block until the request times out and is wrongly turned into +// a 403. This mirrors the reference lua-cs-bouncer behavior, which refuses to +// read the body of an HTTP/2+ request that has no Content-Length. +func isBodyUnreadable(httpReq *http.Request) bool { + return httpReq.Body != nil && httpReq.ProtoMajor >= 2 && httpReq.ContentLength < 0 +} + func appsecQuery(bouncer *Bouncer, ip string, httpReq *http.Request) error { routeURL := url.URL{ Scheme: bouncer.appsecScheme, @@ -729,7 +742,14 @@ func appsecQuery(bouncer *Bouncer, ip string, httpReq *http.Request) error { Path: bouncer.appsecPath, } var req *http.Request - if bouncer.appsecBodyLimit > 0 && httpReq.Body != nil { + switch { + case isBodyUnreadable(httpReq): + if bouncer.appsecUnreadableBodyBlock { + // The caller (handleNextServeHTTP) logs this returned error with the IP. + return errors.New("appsecQuery:unreadableBody dropped") + } + req, _ = http.NewRequest(http.MethodGet, routeURL.String(), nil) + case bouncer.appsecBodyLimit > 0 && httpReq.Body != nil: var bodyBuffer bytes.Buffer limitedReader := io.LimitReader(httpReq.Body, bouncer.appsecBodyLimit) teeReader := io.TeeReader(limitedReader, &bodyBuffer) @@ -740,7 +760,7 @@ func appsecQuery(bouncer *Bouncer, ip string, httpReq *http.Request) error { // Conserve body intact after reading it for other middlewares and service httpReq.Body = io.NopCloser(io.MultiReader(&bodyBuffer, httpReq.Body)) req, _ = http.NewRequest(http.MethodPost, routeURL.String(), bytes.NewBuffer(bodyBytes)) - } else { + default: req, _ = http.NewRequest(http.MethodGet, routeURL.String(), nil) } diff --git a/bouncer_test.go b/bouncer_test.go index f4f369e..483d640 100644 --- a/bouncer_test.go +++ b/bouncer_test.go @@ -2,15 +2,19 @@ package crowdsec_bouncer_traefik_plugin //nolint:revive,stylecheck import ( "context" + "io" "net/http" "net/http/httptest" + "net/url" "reflect" "testing" "text/template" + "time" cache "github.com/maxlerebourg/crowdsec-bouncer-traefik-plugin/pkg/cache" configuration "github.com/maxlerebourg/crowdsec-bouncer-traefik-plugin/pkg/configuration" ip "github.com/maxlerebourg/crowdsec-bouncer-traefik-plugin/pkg/ip" + logger "github.com/maxlerebourg/crowdsec-bouncer-traefik-plugin/pkg/logger" ) func TestServeHTTP(t *testing.T) { @@ -376,3 +380,136 @@ func TestCaptchaMethodBasedLogic(t *testing.T) { }) } } + +// blockingBody simulates a request body that never reaches EOF, like a +// bidirectional gRPC stream that keeps its body open for the whole life of +// the connection. Reading from it blocks until the test is done. +type blockingBody struct { + done <-chan struct{} +} + +func (b blockingBody) Read(_ []byte) (int, error) { + <-b.done + return 0, io.EOF +} + +func (blockingBody) Close() error { return nil } + +func Test_isBodyUnreadable(t *testing.T) { + tests := []struct { + name string + protoMajor int + contentLength int64 + hasBody bool + want bool + }{ + {name: "http2 grpc stream without content-length", protoMajor: 2, contentLength: -1, hasBody: true, want: true}, + {name: "http3 stream without content-length", protoMajor: 3, contentLength: -1, hasBody: true, want: true}, + {name: "http2 with content-length", protoMajor: 2, contentLength: 42, hasBody: true, want: false}, + {name: "http1.1 chunked without content-length", protoMajor: 1, contentLength: -1, hasBody: true, want: false}, + {name: "http2 without body", protoMajor: 2, contentLength: -1, hasBody: false, want: false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + req, _ := http.NewRequest(http.MethodPost, "http://localhost", nil) + req.ProtoMajor = tt.protoMajor + req.ContentLength = tt.contentLength + if tt.hasBody { + req.Body = http.NoBody + } else { + req.Body = nil + } + if got := isBodyUnreadable(req); got != tt.want { + t.Errorf("isBodyUnreadable() = %v, want %v", got, tt.want) + } + }) + } +} + +// newStreamingRequest builds an HTTP/2 request whose body never reaches EOF, +// like a bidirectional gRPC stream (issue #323). +func newStreamingRequest(done <-chan struct{}) *http.Request { + req, _ := http.NewRequest(http.MethodPost, "http://localhost/signalexchange.SignalExchange/ConnectStream", blockingBody{done: done}) + req.Header.Set("Content-Type", "application/grpc") + req.ProtoMajor = 2 + req.ContentLength = -1 + return req +} + +// Test_appsecQuery_streamingDoesNotBlock is a regression test for issue #323: +// a gRPC streaming request whose body never reaches EOF must not be buffered +// (io.ReadAll would block until timeout and wrongly produce a 403). The appsec +// query must complete promptly, inspecting headers only. +func Test_appsecQuery_streamingDoesNotBlock(t *testing.T) { + appsecServer := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, _ *http.Request) { + rw.WriteHeader(http.StatusOK) + })) + defer appsecServer.Close() + + appsecURL, _ := url.Parse(appsecServer.URL) + bouncer := &Bouncer{ + appsecScheme: appsecURL.Scheme, + appsecHost: appsecURL.Host, + appsecPath: "/", + appsecBodyLimit: 10485760, + appsecUnreachableBlock: true, + appsecFailureBlock: true, + httpAppsecClient: appsecServer.Client(), + log: logger.New("INFO", ""), + } + + done := make(chan struct{}) + defer close(done) + + finished := make(chan error, 1) + go func() { + finished <- appsecQuery(bouncer, "1.2.3.4", newStreamingRequest(done)) + }() + + select { + case err := <-finished: + if err != nil { + t.Errorf("appsecQuery() on streaming request returned error: %v", err) + } + case <-time.After(2 * time.Second): + t.Fatal("appsecQuery() blocked on a streaming request body (issue #323 regression)") + } +} + +// Test_appsecQuery_dropUnreadableBody verifies that, when configured to do so, +// a request with an unreadable body is dropped (blocked) instead of forwarded +// without its body, mirroring the reference APPSEC_DROP_UNREADABLE_BODY option. +func Test_appsecQuery_dropUnreadableBody(t *testing.T) { + appsecServer := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, _ *http.Request) { + rw.WriteHeader(http.StatusOK) + })) + defer appsecServer.Close() + + appsecURL, _ := url.Parse(appsecServer.URL) + bouncer := &Bouncer{ + appsecScheme: appsecURL.Scheme, + appsecHost: appsecURL.Host, + appsecPath: "/", + appsecBodyLimit: 10485760, + appsecUnreadableBodyBlock: true, + httpAppsecClient: appsecServer.Client(), + log: logger.New("INFO", ""), + } + + done := make(chan struct{}) + defer close(done) + + finished := make(chan error, 1) + go func() { + finished <- appsecQuery(bouncer, "1.2.3.4", newStreamingRequest(done)) + }() + + select { + case err := <-finished: + if err == nil { + t.Error("appsecQuery() expected an error to block the request, got nil") + } + case <-time.After(2 * time.Second): + t.Fatal("appsecQuery() blocked on a streaming request body (issue #323 regression)") + } +} diff --git a/pkg/configuration/configuration.go b/pkg/configuration/configuration.go index c787bdd..fda49d7 100644 --- a/pkg/configuration/configuration.go +++ b/pkg/configuration/configuration.go @@ -63,6 +63,7 @@ type Config struct { CrowdsecAppsecTLSCertificateBouncerKeyFile string `json:"crowdsecAppsecTlsCertificateBouncerKeyFile,omitempty"` CrowdsecAppsecFailureBlock bool `json:"crowdsecAppsecFailureBlock,omitempty"` CrowdsecAppsecUnreachableBlock bool `json:"crowdsecAppsecUnreachableBlock,omitempty"` + CrowdsecAppsecUnreadableBodyBlock bool `json:"crowdsecAppsecUnreadableBodyBlock,omitempty"` CrowdsecAppsecBodyLimit int64 `json:"crowdsecAppsecBodyLimit,omitempty"` CrowdsecLapiScheme string `json:"crowdsecLapiScheme,omitempty"` CrowdsecLapiHost string `json:"crowdsecLapiHost,omitempty"` @@ -127,52 +128,53 @@ func contains(source []string, target string) bool { // New creates the default plugin configuration. func New() *Config { return &Config{ - Enabled: false, - LogLevel: LogINFO, - LogFormat: "common", - LogFilePath: "", - CrowdsecMode: LiveMode, - CrowdsecAppsecEnabled: false, - CrowdsecAppsecFailureBlock: true, - CrowdsecAppsecUnreachableBlock: true, - CrowdsecAppsecBodyLimit: 10485760, - CrowdsecAppsecScheme: "", - CrowdsecAppsecHost: "crowdsec:7422", - CrowdsecAppsecPath: "/", - CrowdsecAppsecKey: "", - CrowdsecAppsecTLSInsecureVerify: false, - CrowdsecLapiScheme: HTTP, - CrowdsecLapiHost: "crowdsec:8080", - CrowdsecLapiPath: "/", - CrowdsecLapiKey: "", - CrowdsecLapiTLSInsecureVerify: false, - UpdateIntervalSeconds: 60, - MetricsUpdateIntervalSeconds: 600, - UpdateMaxFailure: 0, - StreamStartupBlock: true, - DefaultDecisionSeconds: 60, - RemediationStatusCode: http.StatusForbidden, - HTTPTimeoutSeconds: 10, - CaptchaProvider: "", - CaptchaCustomJsURL: "", - CaptchaCustomValidateURL: "", - CaptchaCustomKey: "", - CaptchaCustomResponse: "", - CaptchaSiteKey: "", - CaptchaSecretKey: "", - CaptchaGracePeriodSeconds: 1800, - CaptchaFilePath: "/captcha.html", - BanFilePath: "", - TraceHeadersCustomName: "", - RemediationHeadersCustomName: "", - ForwardedHeadersCustomName: "X-Forwarded-For", - ForwardedHeadersTrustedIPs: []string{}, - ClientTrustedIPs: []string{}, - RedisCacheEnabled: false, - RedisCacheHost: "redis:6379", - RedisCachePassword: "", - RedisCacheDatabase: "", - RedisCacheUnreachableBlock: true, + Enabled: false, + LogLevel: LogINFO, + LogFormat: "common", + LogFilePath: "", + CrowdsecMode: LiveMode, + CrowdsecAppsecEnabled: false, + CrowdsecAppsecFailureBlock: true, + CrowdsecAppsecUnreachableBlock: true, + CrowdsecAppsecUnreadableBodyBlock: true, + CrowdsecAppsecBodyLimit: 10485760, + CrowdsecAppsecScheme: "", + CrowdsecAppsecHost: "crowdsec:7422", + CrowdsecAppsecPath: "/", + CrowdsecAppsecKey: "", + CrowdsecAppsecTLSInsecureVerify: false, + CrowdsecLapiScheme: HTTP, + CrowdsecLapiHost: "crowdsec:8080", + CrowdsecLapiPath: "/", + CrowdsecLapiKey: "", + CrowdsecLapiTLSInsecureVerify: false, + UpdateIntervalSeconds: 60, + MetricsUpdateIntervalSeconds: 600, + UpdateMaxFailure: 0, + StreamStartupBlock: true, + DefaultDecisionSeconds: 60, + RemediationStatusCode: http.StatusForbidden, + HTTPTimeoutSeconds: 10, + CaptchaProvider: "", + CaptchaCustomJsURL: "", + CaptchaCustomValidateURL: "", + CaptchaCustomKey: "", + CaptchaCustomResponse: "", + CaptchaSiteKey: "", + CaptchaSecretKey: "", + CaptchaGracePeriodSeconds: 1800, + CaptchaFilePath: "/captcha.html", + BanFilePath: "", + TraceHeadersCustomName: "", + RemediationHeadersCustomName: "", + ForwardedHeadersCustomName: "X-Forwarded-For", + ForwardedHeadersTrustedIPs: []string{}, + ClientTrustedIPs: []string{}, + RedisCacheEnabled: false, + RedisCacheHost: "redis:6379", + RedisCachePassword: "", + RedisCacheDatabase: "", + RedisCacheUnreachableBlock: true, } }