From 7dbaebb48f33cf707a069c67eed5b2b614e9913b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juraci=20Paix=C3=A3o=20Kr=C3=B6hling?= Date: Mon, 3 Jun 2024 17:10:16 +0200 Subject: [PATCH] [confighttp] Apply MaxRequestBodySize to the result of a decompressed body (#10289) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This change applies a restriction on the size of the decompressed payload. Before this change, this is the result of the test added in this PR: ``` --- FAIL: TestServerWithDecompression (0.03s) /home/jpkroehling/Projects/src/github.com/open-telemetry/opentelemetry-collector/config/confighttp/confighttp_test.go:1327: Error Trace: /home/jpkroehling/Projects/src/github.com/open-telemetry/opentelemetry-collector/config/confighttp/confighttp_test.go:1327 /usr/lib/golang/src/net/http/server.go:2166 /home/jpkroehling/Projects/src/github.com/open-telemetry/opentelemetry-collector/config/confighttp/compression.go:163 /home/jpkroehling/Projects/src/github.com/open-telemetry/opentelemetry-collector/config/confighttp/confighttp.go:455 /usr/lib/golang/src/net/http/server.go:2166 /home/jpkroehling/go/pkg/mod/go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp@v0.52.0/handler.go:212 /home/jpkroehling/go/pkg/mod/go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp@v0.52.0/handler.go:73 /usr/lib/golang/src/net/http/server.go:2166 /home/jpkroehling/Projects/src/github.com/open-telemetry/opentelemetry-collector/config/confighttp/clientinfohandler.go:26 /usr/lib/golang/src/net/http/server.go:3137 /usr/lib/golang/src/net/http/server.go:2039 /usr/lib/golang/src/runtime/asm_amd64.s:1695 Error: An error is expected but got nil. Test: TestServerWithDecompression /home/jpkroehling/Projects/src/github.com/open-telemetry/opentelemetry-collector/config/confighttp/confighttp_test.go:1328: Error Trace: /home/jpkroehling/Projects/src/github.com/open-telemetry/opentelemetry-collector/config/confighttp/confighttp_test.go:1328 /usr/lib/golang/src/net/http/server.go:2166 /home/jpkroehling/Projects/src/github.com/open-telemetry/opentelemetry-collector/config/confighttp/compression.go:163 /home/jpkroehling/Projects/src/github.com/open-telemetry/opentelemetry-collector/config/confighttp/confighttp.go:455 /usr/lib/golang/src/net/http/server.go:2166 /home/jpkroehling/go/pkg/mod/go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp@v0.52.0/handler.go:212 /home/jpkroehling/go/pkg/mod/go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp@v0.52.0/handler.go:73 /usr/lib/golang/src/net/http/server.go:2166 /home/jpkroehling/Projects/src/github.com/open-telemetry/opentelemetry-collector/config/confighttp/clientinfohandler.go:26 /usr/lib/golang/src/net/http/server.go:3137 /usr/lib/golang/src/net/http/server.go:2039 /usr/lib/golang/src/runtime/asm_amd64.s:1695 Error: Test: TestServerWithDecompression /home/jpkroehling/Projects/src/github.com/open-telemetry/opentelemetry-collector/config/confighttp/confighttp_test.go:1357: Error Trace: /home/jpkroehling/Projects/src/github.com/open-telemetry/opentelemetry-collector/config/confighttp/confighttp_test.go:1357 Error: Not equal: expected: 200 actual : 400 Test: TestServerWithDecompression FAIL FAIL go.opentelemetry.io/collector/config/confighttp 0.036s FAIL ``` Signed-off-by: Juraci Paixão Kröhling --------- Signed-off-by: Juraci Paixão Kröhling --- ..._set-maxrequestbodysize-to-compressed.yaml | 22 +++++ config/confighttp/compression.go | 16 ++-- config/confighttp/compression_test.go | 4 +- config/confighttp/confighttp.go | 9 +- config/confighttp/confighttp_test.go | 91 ++++++++++++++++++- 5 files changed, 130 insertions(+), 12 deletions(-) create mode 100644 .chloggen/jpkroehling_set-maxrequestbodysize-to-compressed.yaml diff --git a/.chloggen/jpkroehling_set-maxrequestbodysize-to-compressed.yaml b/.chloggen/jpkroehling_set-maxrequestbodysize-to-compressed.yaml new file mode 100644 index 00000000000..0d2ad5d82c1 --- /dev/null +++ b/.chloggen/jpkroehling_set-maxrequestbodysize-to-compressed.yaml @@ -0,0 +1,22 @@ +# Use this changelog template to create an entry for release notes. + +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: 'breaking' + +# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver) +component: confighttp + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Apply MaxRequestBodySize to the result of a decompressed body + +# One or more tracking issues or pull requests related to the change +issues: [10289] + +# (Optional) One or more lines of additional information to render under the primary note. +# These lines will be padded with 2 spaces and then inserted directly into the document. +# Use pipe (|) for multiline entries. +subtext: | + When using compressed payloads, the Collector would verify only the size of the compressed payload. + This change applies the same restriction to the decompressed content. As a security measure, a limit of 20 MiB was added, which makes this a breaking change. + For most clients, this shouldn't be a problem, but if you often have payloads that decompress to more than 20 MiB, you might want to either configure your + client to send smaller batches (recommended), or increase the limit using the MaxRequestBodySize option. diff --git a/config/confighttp/compression.go b/config/confighttp/compression.go index 88ecafe78da..a700bec845b 100644 --- a/config/confighttp/compression.go +++ b/config/confighttp/compression.go @@ -67,24 +67,26 @@ func (r *compressRoundTripper) RoundTrip(req *http.Request) (*http.Response, err } type decompressor struct { - errHandler func(w http.ResponseWriter, r *http.Request, errorMsg string, statusCode int) - base http.Handler - decoders map[string]func(body io.ReadCloser) (io.ReadCloser, error) + errHandler func(w http.ResponseWriter, r *http.Request, errorMsg string, statusCode int) + base http.Handler + decoders map[string]func(body io.ReadCloser) (io.ReadCloser, error) + maxRequestBodySize int64 } // httpContentDecompressor offloads the task of handling compressed HTTP requests // by identifying the compression format in the "Content-Encoding" header and re-writing // request body so that the handlers further in the chain can work on decompressed data. // It supports gzip and deflate/zlib compression. -func httpContentDecompressor(h http.Handler, eh func(w http.ResponseWriter, r *http.Request, errorMsg string, statusCode int), decoders map[string]func(body io.ReadCloser) (io.ReadCloser, error)) http.Handler { +func httpContentDecompressor(h http.Handler, maxRequestBodySize int64, eh func(w http.ResponseWriter, r *http.Request, errorMsg string, statusCode int), decoders map[string]func(body io.ReadCloser) (io.ReadCloser, error)) http.Handler { errHandler := defaultErrorHandler if eh != nil { errHandler = eh } d := &decompressor{ - errHandler: errHandler, - base: h, + maxRequestBodySize: maxRequestBodySize, + errHandler: errHandler, + base: h, decoders: map[string]func(body io.ReadCloser) (io.ReadCloser, error){ "": func(io.ReadCloser) (io.ReadCloser, error) { // Not a compressed payload. Nothing to do. @@ -155,7 +157,7 @@ func (d *decompressor) ServeHTTP(w http.ResponseWriter, r *http.Request) { // "Content-Length" is set to -1 as the size of the decompressed body is unknown. r.Header.Del("Content-Length") r.ContentLength = -1 - r.Body = newBody + r.Body = http.MaxBytesReader(w, newBody, d.maxRequestBodySize) } d.base.ServeHTTP(w, r) } diff --git a/config/confighttp/compression_test.go b/config/confighttp/compression_test.go index 794adda2ec8..db2f7b3b3c0 100644 --- a/config/confighttp/compression_test.go +++ b/config/confighttp/compression_test.go @@ -134,7 +134,7 @@ func TestHTTPCustomDecompression(t *testing.T) { return io.NopCloser(strings.NewReader("decompressed body")), nil }, } - srv := httptest.NewServer(httpContentDecompressor(handler, defaultErrorHandler, decoders)) + srv := httptest.NewServer(httpContentDecompressor(handler, defaultMaxRequestBodySize, defaultErrorHandler, decoders)) t.Cleanup(srv.Close) @@ -253,7 +253,7 @@ func TestHTTPContentDecompressionHandler(t *testing.T) { require.NoError(t, err, "failed to read request body: %v", err) assert.EqualValues(t, testBody, string(body)) w.WriteHeader(http.StatusOK) - }), defaultErrorHandler, noDecoders)) + }), defaultMaxRequestBodySize, defaultErrorHandler, noDecoders)) t.Cleanup(srv.Close) req, err := http.NewRequest(http.MethodGet, srv.URL, tt.reqBody) diff --git a/config/confighttp/confighttp.go b/config/confighttp/confighttp.go index b210fa0dd8d..71b2f17ee2f 100644 --- a/config/confighttp/confighttp.go +++ b/config/confighttp/confighttp.go @@ -30,6 +30,7 @@ import ( ) const headerContentEncoding = "Content-Encoding" +const defaultMaxRequestBodySize = 20 * 1024 * 1024 // 20MiB // ClientConfig defines settings for creating an HTTP client. type ClientConfig struct { @@ -269,7 +270,7 @@ type ServerConfig struct { // Auth for this receiver Auth *configauth.Authentication `mapstructure:"auth"` - // MaxRequestBodySize sets the maximum request body size in bytes + // MaxRequestBodySize sets the maximum request body size in bytes. Default: 20MiB. MaxRequestBodySize int64 `mapstructure:"max_request_body_size"` // IncludeMetadata propagates the client metadata from the incoming requests to the downstream consumers @@ -340,7 +341,11 @@ func (hss *ServerConfig) ToServer(_ context.Context, host component.Host, settin o(serverOpts) } - handler = httpContentDecompressor(handler, serverOpts.errHandler, serverOpts.decoders) + if hss.MaxRequestBodySize <= 0 { + hss.MaxRequestBodySize = defaultMaxRequestBodySize + } + + handler = httpContentDecompressor(handler, hss.MaxRequestBodySize, serverOpts.errHandler, serverOpts.decoders) if hss.MaxRequestBodySize > 0 { handler = maxRequestBodySizeInterceptor(handler, hss.MaxRequestBodySize) diff --git a/config/confighttp/confighttp_test.go b/config/confighttp/confighttp_test.go index 635f9415a2f..99ac9a51201 100644 --- a/config/confighttp/confighttp_test.go +++ b/config/confighttp/confighttp_test.go @@ -4,6 +4,7 @@ package confighttp import ( + "bytes" "context" "errors" "fmt" @@ -13,6 +14,7 @@ import ( "net/http/httptest" "net/url" "path/filepath" + "strings" "testing" "time" @@ -1300,7 +1302,7 @@ func TestServerWithDecoder(t *testing.T) { // test response := &httptest.ResponseRecorder{} - req, err := http.NewRequest(http.MethodGet, srv.Addr, nil) + req, err := http.NewRequest(http.MethodGet, srv.Addr, bytes.NewBuffer([]byte("something"))) require.NoError(t, err, "Error creating request: %v", err) req.Header.Set("Content-Encoding", "something-else") @@ -1310,6 +1312,93 @@ func TestServerWithDecoder(t *testing.T) { } +func TestServerWithDecompression(t *testing.T) { + // prepare + hss := ServerConfig{ + MaxRequestBodySize: 1000, // 1 KB + } + body := []byte(strings.Repeat("a", 1000*1000)) // 1 MB + + srv, err := hss.ToServer( + context.Background(), + componenttest.NewNopHost(), + componenttest.NewNopTelemetrySettings(), + http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) { + actualBody, err := io.ReadAll(req.Body) + assert.ErrorContains(t, err, "http: request body too large") + assert.Len(t, actualBody, 1000) + + if err != nil { + resp.WriteHeader(http.StatusBadRequest) + } else { + resp.WriteHeader(http.StatusOK) + } + }), + ) + require.NoError(t, err) + + testSrv := httptest.NewServer(srv.Handler) + defer testSrv.Close() + + req, err := http.NewRequest(http.MethodGet, testSrv.URL, compressZstd(t, body)) + require.NoError(t, err, "Error creating request: %v", err) + + req.Header.Set("Content-Encoding", "zstd") + + // test + c := http.Client{} + resp, err := c.Do(req) + require.NoError(t, err, "Error sending request: %v", err) + + _, err = io.ReadAll(resp.Body) + require.NoError(t, err, "Error reading response body: %v", err) + + // verifications is done mostly within the test, but this is only a sanity check + // that we got into the test handler + assert.Equal(t, resp.StatusCode, http.StatusBadRequest) +} + +func TestDefaultMaxRequestBodySize(t *testing.T) { + tests := []struct { + name string + settings ServerConfig + expected int64 + }{ + { + name: "default", + settings: ServerConfig{}, + expected: defaultMaxRequestBodySize, + }, + { + name: "zero", + settings: ServerConfig{MaxRequestBodySize: 0}, + expected: defaultMaxRequestBodySize, + }, + { + name: "negative", + settings: ServerConfig{MaxRequestBodySize: -1}, + expected: defaultMaxRequestBodySize, + }, + { + name: "custom", + settings: ServerConfig{MaxRequestBodySize: 100}, + expected: 100, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := tt.settings.ToServer( + context.Background(), + componenttest.NewNopHost(), + componenttest.NewNopTelemetrySettings(), + http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}), + ) + require.NoError(t, err) + assert.Equal(t, tt.expected, tt.settings.MaxRequestBodySize) + }) + } +} + type mockHost struct { component.Host ext map[component.ID]component.Component