Merge commit from fork

[28.x] pkg/authz: Reject requests with body size exceeding 4 MiB

pkg/authorization/authz.go

@@ -16,7 +16,7 @@ import (
 	"github.com/docker/docker/pkg/ioutils"
 )
 
-const maxBodySize = 1048576 // 1MB
+const maxBodySize = 4 * 1024 * 1024 // 4MiB
 
 // NewCtx creates new authZ context, it is used to store authorization information related to a specific docker
 // REST http session
@@ -55,28 +55,31 @@ type Ctx struct {
 	authReq *Request
 }
 
-func isChunked(r *http.Request) bool {
-	// RFC 7230 specifies that content length is to be ignored if Transfer-Encoding is chunked
-	if strings.EqualFold(r.Header.Get("Transfer-Encoding"), "chunked") {
-		return true
-	}
-	for _, v := range r.TransferEncoding {
-		if strings.EqualFold(v, "chunked") {
-			return true
-		}
-	}
-	return false
-}
-
 // AuthZRequest authorized the request to the docker daemon using authZ plugins
 func (ctx *Ctx) AuthZRequest(w http.ResponseWriter, r *http.Request) error {
 	var body []byte
-	if sendBody(ctx.requestURI, r.Header) && (r.ContentLength > 0 || isChunked(r)) && r.ContentLength < maxBodySize {
-		var err error
-		body, r.Body, err = drainBody(r.Body)
-		if err != nil {
+	if sendBody(ctx.requestURI, r.Header) {
+		// Wrap the original request body in a buffered reader so we can inspect
+		// the prefix without consuming bytes from the downstream reader.
+		// `Peek(maxBodySize + 1)` is used as a size check:
+		//   - err == nil means at least maxBodySize+1 bytes are buffered/available,
+		//     so the payload exceeds the plugin limit and is rejected.
+		//   - otherwise, `peeked` contains the complete body bytes currently available
+		//     (for short bodies this is the full payload), and reads from r.Body still
+		//     stream the original body unchanged.
+		bufBody := bufio.NewReaderSize(r.Body, maxBodySize+1)
+		r.Body = ioutils.NewReadCloserWrapper(bufBody, r.Body.Close)
+
+		peeked, err := bufBody.Peek(maxBodySize + 1)
+		if err == nil {
+			// Successfully peeked maxBodySize+1 bytes, so body is too large
+			// TODO: Allows plugin to opt in
+			return fmt.Errorf("request body too large for authorization plugin: size exceeds %d bytes", maxBodySize)
+		} else if err != io.EOF {
 			return err
 		}
+
+		body = peeked
 	}
 
 	var h bytes.Buffer
@@ -142,25 +145,6 @@ func (ctx *Ctx) AuthZResponse(rm ResponseModifier, r *http.Request) error {
 	return nil
 }
 
-// drainBody dump the body (if its length is less than 1MB) without modifying the request state
-func drainBody(body io.ReadCloser) ([]byte, io.ReadCloser, error) {
-	bufReader := bufio.NewReaderSize(body, maxBodySize)
-	newBody := ioutils.NewReadCloserWrapper(bufReader, func() error { return body.Close() })
-
-	data, err := bufReader.Peek(maxBodySize)
-	// Body size exceeds max body size
-	if err == nil {
-		log.G(context.TODO()).Warnf("Request body is larger than: '%d' skipping body", maxBodySize)
-		return nil, newBody, nil
-	}
-	// Body size is less than maximum size
-	if err == io.EOF {
-		return data, newBody, nil
-	}
-	// Unknown error
-	return nil, newBody, err
-}
-
 func isAuthEndpoint(urlPath string) (bool, error) {
 	// eg www.test.com/v1.24/auth/optional?optional1=something&optional2=something (version optional)
 	matched, err := regexp.MatchString(`^[^\/]*\/(v\d[\d\.]*\/)?auth.*`, urlPath)

pkg/authorization/authz_unix_test.go

@@ -140,36 +140,69 @@ func TestResponseModifier(t *testing.T) {
 	}
 }
 
-func TestDrainBody(t *testing.T) {
-	tests := []struct {
-		length             int // length is the message length send to drainBody
-		expectedBodyLength int // expectedBodyLength is the expected body length after drainBody is called
-	}{
-		{10, 10},                           // Small message size
-		{maxBodySize - 1, maxBodySize - 1}, // Max message size
-		{maxBodySize * 2, 0},               // Large message size (skip copying body)
+type recordingPlugin struct {
+	recordedRequest Request
+}
 
+func (p *recordingPlugin) Name() string { return "recording-plugin" }
+
+func (p *recordingPlugin) AuthZRequest(authReq *Request) (*Response, error) {
+	p.recordedRequest = *authReq
+	p.recordedRequest.RequestBody = bytes.Clone(authReq.RequestBody)
+	return &Response{Allow: true}, nil
+}
+
+func (p *recordingPlugin) AuthZResponse(_ *Request) (*Response, error) {
+	return &Response{Allow: true}, nil
+}
+
+func TestAuthZRequestBodyWithinLimit(t *testing.T) {
+	payload := strings.Repeat("a", maxBodySize)
+	plugin := &recordingPlugin{}
+	ctx := NewCtx([]Plugin{plugin}, "user", "tls", http.MethodPost, "/containers/create")
+
+	req := httptest.NewRequest(http.MethodPost, "http://example.com/containers/create", strings.NewReader(payload))
+	req.Header.Set("Content-Type", "application/json")
+
+	if err := ctx.AuthZRequest(httptest.NewRecorder(), req); err != nil {
+		t.Fatalf("AuthZRequest failed: %v", err)
 	}
 
-	for _, test := range tests {
-		msg := strings.Repeat("a", test.length)
-		body, closer, err := drainBody(io.NopCloser(bytes.NewReader([]byte(msg))))
-		if err != nil {
-			t.Fatal(err)
-		}
-		if len(body) != test.expectedBodyLength {
-			t.Fatalf("Body must be copied, actual length: '%d'", len(body))
-		}
-		if closer == nil {
-			t.Fatal("Closer must not be nil")
-		}
-		modified, err := io.ReadAll(closer)
-		if err != nil {
-			t.Fatalf("Error must not be nil: '%v'", err)
-		}
-		if len(modified) != len(msg) {
-			t.Fatalf("Result should not be truncated. Original length: '%d', new length: '%d'", len(msg), len(modified))
-		}
+	if string(plugin.recordedRequest.RequestBody) != payload {
+		t.Fatalf("expected full request body to be sent to plugin, got length %d, expected %d", len(plugin.recordedRequest.RequestBody), len(payload))
+	}
+
+	remaining, err := io.ReadAll(req.Body)
+	if err != nil {
+		t.Fatalf("failed to read request body after authz: %v", err)
+	}
+	if string(remaining) != payload {
+		t.Fatalf("request body should be preserved for downstream readers")
+	}
+}
+
+func TestAuthZRequestBodyOverLimit(t *testing.T) {
+	payload := strings.Repeat("a", maxBodySize+1)
+	plugin := &recordingPlugin{}
+	ctx := NewCtx([]Plugin{plugin}, "user", "tls", http.MethodPost, "/containers/create")
+
+	req := httptest.NewRequest(http.MethodPost, "http://example.com/containers/create", strings.NewReader(payload))
+	req.Header.Set("Content-Type", "application/json")
+
+	err := ctx.AuthZRequest(httptest.NewRecorder(), req)
+	if err == nil {
+		t.Fatal("expected AuthZRequest to reject body over max size")
+	}
+	if !strings.Contains(err.Error(), "request body too large for authorization plugin") {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	remaining, readErr := io.ReadAll(req.Body)
+	if readErr != nil {
+		t.Fatalf("failed to read request body after authz error: %v", readErr)
+	}
+	if string(remaining) != payload {
+		t.Fatalf("request body should still be preserved after over-limit check")
 	}
 }