Merge pull request #20602 from twistlock/20508_authz_plugin_corrupt_body

Fix #20508 - Authz plugin enabled with large text/JSON POST payload corrupts body

authorization/authz.go

@@ -54,13 +54,11 @@ type Ctx struct {
 // 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) {
-		if r.ContentLength < maxBodySize {
-			var err error
-			body, r.Body, err = drainBody(r.Body)
-			if err != nil {
-				return err
-			}
+	if sendBody(ctx.requestURI, r.Header) && r.ContentLength > 0 && r.ContentLength < maxBodySize {
+		var err error
+		body, r.Body, err = drainBody(r.Body)
+		if err != nil {
+			return err
 		}
 	}
 
@@ -121,23 +119,23 @@ func (ctx *Ctx) AuthZResponse(rm ResponseModifier, r *http.Request) error {
 	return nil
 }
 
-// drainBody dump the body, it reads the body data into memory and
-// see go sources /go/src/net/http/httputil/dump.go
+// drainBody dump the body (if it's 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)
-	if err != io.EOF {
-		// This means the request is larger than our max
-		if err == bufio.ErrBufferFull {
-			return nil, newBody, nil
-		}
-		// This means we had an error reading
-		return nil, nil, err
+	// Body size exceeds max body size
+	if err == nil {
+		logrus.Warnf("Request body is larger than: '%d' skipping body", maxBodySize)
+		return nil, newBody, nil
 	}
-
-	return data, newBody, nil
+	// Body size is less than maximum size
+	if err == io.EOF {
+		return data, newBody, nil
+	}
+	// Unknown error
+	return nil, newBody, err
 }
 
 // sendBody returns true when request/response body should be sent to AuthZPlugin
@@ -148,8 +146,7 @@ func sendBody(url string, header http.Header) bool {
 	}
 
 	// body is sent only for text or json messages
-	v := header.Get("Content-Type")
-	return strings.HasPrefix(v, "text/") || v == "application/json"
+	return header.Get("Content-Type") == "application/json"
 }
 
 // headers returns flatten version of the http headers excluding authorization

authorization/authz_unix_test.go

@@ -17,9 +17,11 @@ import (
 	"reflect"
 	"testing"
 
+	"bytes"
 	"github.com/docker/docker/pkg/plugins"
 	"github.com/docker/go-connections/tlsconfig"
 	"github.com/gorilla/mux"
+	"strings"
 )
 
 const pluginAddress = "authzplugin.sock"
@@ -135,6 +137,41 @@ 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)
+
+	}
+
+	for _, test := range tests {
+
+		msg := strings.Repeat("a", test.length)
+		body, closer, err := drainBody(ioutil.NopCloser(bytes.NewReader([]byte(msg))))
+		if len(body) != test.expectedBodyLength {
+			t.Fatalf("Body must be copied, actual length: '%d'", len(body))
+		}
+		if closer == nil {
+			t.Fatalf("Closer must not be nil")
+		}
+		if err != nil {
+			t.Fatalf("Error must not be nil: '%v'", err)
+		}
+		modified, err := ioutil.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))
+		}
+	}
+}
+
 func TestResponseModifierOverride(t *testing.T) {
 	r := httptest.NewRecorder()
 	m := NewResponseModifier(r)