Require content-length in POST & Upload requests (#3671)

Avoid passing size = -1 to PutObject API by requiring content-length
header in POST request (as AWS S3 does) and in Upload web handler.
Post handler is modified to completely store multipart file to know
its size before sending it to PutObject().

cmd/bucket-handlers.go

@@ -393,6 +393,13 @@ func (api objectAPIHandlers) PostPolicyBucketHandler(w http.ResponseWriter, r *h
 		return
 	}
 
+	// Require Content-Length to be set in the request
+	size := r.ContentLength
+	if size < 0 {
+		writeErrorResponse(w, ErrMissingContentLength, r.URL)
+		return
+	}
+
 	// Here the parameter is the size of the form data that should
 	// be loaded in memory, the remaining being put in temporary files.
 	reader, err := r.MultipartReader()
@@ -402,12 +409,28 @@ func (api objectAPIHandlers) PostPolicyBucketHandler(w http.ResponseWriter, r *h
 		return
 	}
 
-	fileBody, fileName, formValues, err := extractPostPolicyFormValues(reader)
+	// Read multipart data and save in memory and in the disk if needed
+	form, err := reader.ReadForm(maxFormMemory)
+	if err != nil {
+		errorIf(err, "Unable to initialize multipart reader.")
+		writeErrorResponse(w, ErrMalformedPOSTRequest, r.URL)
+		return
+	}
+
+	// Remove all tmp files creating during multipart upload
+	defer form.RemoveAll()
+
+	// Extract all form fields
+	fileBody, fileName, fileSize, formValues, err := extractPostPolicyFormValues(form)
 	if err != nil {
 		errorIf(err, "Unable to parse form values.")
 		writeErrorResponse(w, ErrMalformedPOSTRequest, r.URL)
 		return
 	}
+
+	// Close multipart file
+	defer fileBody.Close()
+
 	bucket := mux.Vars(r)["bucket"]
 	formValues["Bucket"] = bucket
 
@@ -443,21 +466,20 @@ func (api objectAPIHandlers) PostPolicyBucketHandler(w http.ResponseWriter, r *h
 		return
 	}
 
-	// Use rangeReader to ensure that object size is within expected range.
+	// Ensure that the object size is within expected range, also the file size
+	// should not exceed the maximum single Put size (5 GiB)
 	lengthRange := postPolicyForm.Conditions.ContentLengthRange
 	if lengthRange.Valid {
-		// If policy restricted the size of the object.
-		fileBody = &rangeReader{
-			Reader: fileBody,
-			Min:    lengthRange.Min,
-			Max:    lengthRange.Max,
+		if fileSize < lengthRange.Min {
+			errorIf(err, "Unable to create object.")
+			writeErrorResponse(w, toAPIErrorCode(errDataTooSmall), r.URL)
+			return
 		}
-	} else {
-		// Default values of min/max size of the object.
-		fileBody = &rangeReader{
-			Reader: fileBody,
-			Min:    0,
-			Max:    maxObjectSize,
+
+		if fileSize > lengthRange.Max || fileSize > maxObjectSize {
+			errorIf(err, "Unable to create object.")
+			writeErrorResponse(w, toAPIErrorCode(errDataTooLarge), r.URL)
+			return
 		}
 	}
 
@@ -470,7 +492,7 @@ func (api objectAPIHandlers) PostPolicyBucketHandler(w http.ResponseWriter, r *h
 	objectLock.Lock()
 	defer objectLock.Unlock()
 
-	objInfo, err := objectAPI.PutObject(bucket, object, -1, fileBody, metadata, sha256sum)
+	objInfo, err := objectAPI.PutObject(bucket, object, fileSize, fileBody, metadata, sha256sum)
 	if err != nil {
 		errorIf(err, "Unable to create object.")
 		writeErrorResponse(w, toAPIErrorCode(err), r.URL)

cmd/globals.go

@@ -59,6 +59,9 @@ const (
 	// can reach that size according to https://aws.amazon.com/articles/1434
 	maxFormFieldSize = int64(1 * humanize.MiByte)
 
+	// Limit memory allocation to store multipart data
+	maxFormMemory = int64(5 * humanize.MiByte)
+
 	// The maximum allowed difference between the request generation time and the server processing time
 	globalMaxSkewTime = 15 * time.Minute
 )

cmd/handler-utils.go

@@ -18,7 +18,6 @@ package cmd
 
 import (
 	"io"
-	"io/ioutil"
 	"mime/multipart"
 	"net/http"
 	"strings"
@@ -158,33 +157,49 @@ func extractMetadataFromForm(formValues map[string]string) map[string]string {
 }
 
 // Extract form fields and file data from a HTTP POST Policy
-func extractPostPolicyFormValues(reader *multipart.Reader) (filePart io.Reader, fileName string, formValues map[string]string, err error) {
+func extractPostPolicyFormValues(form *multipart.Form) (filePart io.ReadCloser, fileName string, fileSize int64, formValues map[string]string, err error) {
 	/// HTML Form values
 	formValues = make(map[string]string)
 	fileName = ""
-	for err == nil {
-		var part *multipart.Part
-		part, err = reader.NextPart()
-		if part != nil {
-			canonicalFormName := http.CanonicalHeaderKey(part.FormName())
-			if canonicalFormName != "File" {
-				var buffer []byte
-				limitReader := io.LimitReader(part, maxFormFieldSize+1)
-				buffer, err = ioutil.ReadAll(limitReader)
-				if err != nil {
-					return nil, "", nil, err
-				}
-				if int64(len(buffer)) > maxFormFieldSize {
-					return nil, "", nil, errSizeUnexpected
-				}
-				formValues[canonicalFormName] = string(buffer)
-			} else {
-				filePart = part
-				fileName = part.FileName()
-				// As described in S3 spec, we expect file to be the last form field
-				break
+
+	// Iterate over form values
+	for k, v := range form.Value {
+		canonicalFormName := http.CanonicalHeaderKey(k)
+		// Check if value's field exceeds S3 limit
+		if int64(len(v[0])) > maxFormFieldSize {
+			return nil, "", 0, nil, errSizeUnexpected
+		}
+		// Set the form value
+		formValues[canonicalFormName] = v[0]
+	}
+
+	// Iterator until we find a valid File field and break
+	for k, v := range form.File {
+		canonicalFormName := http.CanonicalHeaderKey(k)
+		if canonicalFormName == "File" {
+			if len(v) == 0 {
+				return nil, "", 0, nil, errInvalidArgument
 			}
+			// Fetch fileHeader which has the uploaded file information
+			fileHeader := v[0]
+			// Set filename
+			fileName = fileHeader.Filename
+			// Open the uploaded part
+			filePart, err = fileHeader.Open()
+			// Compute file size
+			fileSize, err = filePart.(io.Seeker).Seek(0, 2)
+			if err != nil {
+				return nil, "", 0, nil, err
+			}
+			// Reset Seek to the beginning
+			_, err = filePart.(io.Seeker).Seek(0, 0)
+			if err != nil {
+				return nil, "", 0, nil, err
+			}
+			// File found and ready for reading
+			break
 		}
 	}
-	return filePart, fileName, formValues, nil
+
+	return filePart, fileName, fileSize, formValues, nil
 }

cmd/jwt.go

@@ -106,13 +106,13 @@ func isAuthTokenValid(tokenString string) bool {
 }
 
 func isHTTPRequestValid(req *http.Request) bool {
-	return webReqestAuthenticate(req) == nil
+	return webRequestAuthenticate(req) == nil
 }
 
 // Check if the request is authenticated.
 // Returns nil if the request is authenticated. errNoAuthToken if token missing.
 // Returns errAuthentication for all other errors.
-func webReqestAuthenticate(req *http.Request) error {
+func webRequestAuthenticate(req *http.Request) error {
 	jwtToken, err := jwtreq.ParseFromRequest(req, jwtreq.AuthorizationHeaderExtractor, keyFuncCallback)
 	if err != nil {
 		if err == jwtreq.ErrNoTokenInRequest {

cmd/post-policy_test.go

@@ -342,46 +342,66 @@ func testPostPolicyBucketHandler(obj ObjectLayer, instanceType string, t TestErr
 	}
 
 	testCases2 := []struct {
-		objectName         string
-		data               []byte
-		expectedRespStatus int
-		accessKey          string
-		secretKey          string
-		malformedBody      bool
+		objectName          string
+		data                []byte
+		expectedRespStatus  int
+		accessKey           string
+		secretKey           string
+		malformedBody       bool
+		ignoreContentLength bool
 	}{
 		// Success case.
 		{
-			objectName:         "test",
-			data:               bytes.Repeat([]byte("a"), 1025),
-			expectedRespStatus: http.StatusNoContent,
-			accessKey:          credentials.AccessKey,
-			secretKey:          credentials.SecretKey,
-			malformedBody:      false,
+			objectName:          "test",
+			data:                bytes.Repeat([]byte("a"), 1025),
+			expectedRespStatus:  http.StatusNoContent,
+			accessKey:           credentials.AccessKey,
+			secretKey:           credentials.SecretKey,
+			malformedBody:       false,
+			ignoreContentLength: false,
+		},
+		// Failed with Content-Length not specified.
+		{
+			objectName:          "test",
+			data:                bytes.Repeat([]byte("a"), 1025),
+			expectedRespStatus:  http.StatusNoContent,
+			accessKey:           credentials.AccessKey,
+			secretKey:           credentials.SecretKey,
+			malformedBody:       false,
+			ignoreContentLength: true,
 		},
 		// Failed with entity too small.
 		{
-			objectName:         "test",
-			data:               bytes.Repeat([]byte("a"), 1023),
-			expectedRespStatus: http.StatusBadRequest,
-			accessKey:          credentials.AccessKey,
-			secretKey:          credentials.SecretKey,
-			malformedBody:      false,
+			objectName:          "test",
+			data:                bytes.Repeat([]byte("a"), 1023),
+			expectedRespStatus:  http.StatusBadRequest,
+			accessKey:           credentials.AccessKey,
+			secretKey:           credentials.SecretKey,
+			malformedBody:       false,
+			ignoreContentLength: false,
 		},
 		// Failed with entity too large.
 		{
-			objectName:         "test",
-			data:               bytes.Repeat([]byte("a"), (1*humanize.MiByte)+1),
-			expectedRespStatus: http.StatusBadRequest,
-			accessKey:          credentials.AccessKey,
-			secretKey:          credentials.SecretKey,
-			malformedBody:      false,
+			objectName:          "test",
+			data:                bytes.Repeat([]byte("a"), (1*humanize.MiByte)+1),
+			expectedRespStatus:  http.StatusBadRequest,
+			accessKey:           credentials.AccessKey,
+			secretKey:           credentials.SecretKey,
+			malformedBody:       false,
+			ignoreContentLength: false,
 		},
 	}
 
 	for i, testCase := range testCases2 {
 		// initialize HTTP NewRecorder, this records any mutations to response writer inside the handler.
 		rec := httptest.NewRecorder()
-		req, perr := newPostRequestV4WithContentLength("", bucketName, testCase.objectName, testCase.data, testCase.accessKey, testCase.secretKey)
+		var req *http.Request
+		var perr error
+		if testCase.ignoreContentLength {
+			req, perr = newPostRequestV4("", bucketName, testCase.objectName, testCase.data, testCase.accessKey, testCase.secretKey)
+		} else {
+			req, perr = newPostRequestV4WithContentLength("", bucketName, testCase.objectName, testCase.data, testCase.accessKey, testCase.secretKey)
+		}
 		if perr != nil {
 			t.Fatalf("Test %d: %s: Failed to create HTTP request for PostPolicyHandler: <ERROR> %v", i+1, instanceType, perr)
 		}

cmd/typed-errors.go

@@ -33,6 +33,9 @@ var errContentSHA256Mismatch = errors.New("Content checksum SHA256 mismatch")
 // used when we deal with data larger than expected
 var errSizeUnexpected = errors.New("Data size larger than expected")
 
+// used when we deal with data with unknown size
+var errSizeUnspecified = errors.New("Data size is unspecified")
+
 // When upload object size is greater than 5G in a single PUT/POST operation.
 var errDataTooLarge = errors.New("Object size larger than allowed limit")
 

cmd/web-handlers.go

@@ -151,7 +151,7 @@ func (web *webAPIHandlers) ListBuckets(r *http.Request, args *WebGenericArgs, re
 	if objectAPI == nil {
 		return toJSONError(errServerNotInitialized)
 	}
-	authErr := webReqestAuthenticate(r)
+	authErr := webRequestAuthenticate(r)
 	if authErr != nil {
 		return toJSONError(authErr)
 	}
@@ -208,7 +208,7 @@ func (web *webAPIHandlers) ListObjects(r *http.Request, args *ListObjectsArgs, r
 	prefix := args.Prefix + "test" // To test if GetObject/PutObject with the specified prefix is allowed.
 	readable := isBucketActionAllowed("s3:GetObject", args.BucketName, prefix)
 	writable := isBucketActionAllowed("s3:PutObject", args.BucketName, prefix)
-	authErr := webReqestAuthenticate(r)
+	authErr := webRequestAuthenticate(r)
 	switch {
 	case authErr == errAuthentication:
 		return toJSONError(authErr)
@@ -446,7 +446,7 @@ func (web *webAPIHandlers) Upload(w http.ResponseWriter, r *http.Request) {
 	bucket := vars["bucket"]
 	object := vars["object"]
 
-	authErr := webReqestAuthenticate(r)
+	authErr := webRequestAuthenticate(r)
 	if authErr == errAuthentication {
 		writeWebErrorResponse(w, errAuthentication)
 		return
@@ -456,6 +456,13 @@ func (web *webAPIHandlers) Upload(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	// Require Content-Length to be set in the request
+	size := r.ContentLength
+	if size < 0 {
+		writeWebErrorResponse(w, errSizeUnspecified)
+		return
+	}
+
 	// Extract incoming metadata if any.
 	metadata := extractMetadataFromHeader(r.Header)
 
@@ -465,7 +472,7 @@ func (web *webAPIHandlers) Upload(w http.ResponseWriter, r *http.Request) {
 	defer objectLock.Unlock()
 
 	sha256sum := ""
-	objInfo, err := objectAPI.PutObject(bucket, object, -1, r.Body, metadata, sha256sum)
+	objInfo, err := objectAPI.PutObject(bucket, object, size, r.Body, metadata, sha256sum)
 	if err != nil {
 		writeWebErrorResponse(w, err)
 		return
@@ -816,6 +823,12 @@ func toWebAPIError(err error) APIError {
 			HTTPStatusCode: http.StatusForbidden,
 			Description:    err.Error(),
 		}
+	} else if err == errSizeUnspecified {
+		return APIError{
+			Code:           "InvalidRequest",
+			HTTPStatusCode: http.StatusBadRequest,
+			Description:    err.Error(),
+		}
 	}
 
 	// Convert error type to api error code.

cmd/web-handlers_test.go

@@ -738,19 +738,24 @@ func testUploadWebHandler(obj ObjectLayer, instanceType string, t TestErrHandler
 	objectName := "test.file"
 	bucketName := getRandomBucketName()
 
-	test := func(token string) int {
+	test := func(token string, sendContentLength bool) int {
 		rec := httptest.NewRecorder()
-		var req *http.Request
-		req, err = http.NewRequest("PUT", "/minio/upload/"+bucketName+"/"+objectName, nil)
-		if err != nil {
-			t.Fatalf("Cannot create upload request, %v", err)
+		req, rErr := http.NewRequest("PUT", "/minio/upload/"+bucketName+"/"+objectName, nil)
+		if rErr != nil {
+			t.Fatalf("Cannot create upload request, %v", rErr)
 		}
 
-		req.Header.Set("Content-Length", strconv.Itoa(len(content)))
 		req.Header.Set("x-amz-date", "20160814T114029Z")
 		req.Header.Set("Accept", "*/*")
+
 		req.Body = ioutil.NopCloser(bytes.NewReader(content))
 
+		if !sendContentLength {
+			req.ContentLength = -1
+		} else {
+			req.ContentLength = int64(len(content))
+		}
+
 		if token != "" {
 			req.Header.Set("Authorization", "Bearer "+authorization)
 		}
@@ -765,7 +770,7 @@ func testUploadWebHandler(obj ObjectLayer, instanceType string, t TestErrHandler
 	}
 
 	// Authenticated upload should succeed.
-	code := test(authorization)
+	code := test(authorization, true)
 	if code != http.StatusOK {
 		t.Fatalf("Expected the response status to be 200, but instead found `%d`", code)
 	}
@@ -780,8 +785,14 @@ func testUploadWebHandler(obj ObjectLayer, instanceType string, t TestErrHandler
 		t.Fatalf("The upload file is different from the download file")
 	}
 
+	// Authenticated upload without content-length should fail
+	code = test(authorization, false)
+	if code != http.StatusBadRequest {
+		t.Fatalf("Expected the response status to be 200, but instead found `%d`", code)
+	}
+
 	// Unauthenticated upload should fail.
-	code = test("")
+	code = test("", true)
 	if code != http.StatusForbidden {
 		t.Fatalf("Expected the response status to be 403, but instead found `%d`", code)
 	}
@@ -794,13 +805,13 @@ func testUploadWebHandler(obj ObjectLayer, instanceType string, t TestErrHandler
 	globalBucketPolicies.SetBucketPolicy(bucketName, policyChange{false, &policy})
 
 	// Unauthenticated upload with WRITE policy should succeed.
-	code = test("")
+	code = test("", true)
 	if code != http.StatusOK {
 		t.Fatalf("Expected the response status to be 200, but instead found `%d`", code)
 	}
 }
 
-// Wrapper for calling Upload Handler
+// Wrapper for calling Download Handler
 func TestWebHandlerDownload(t *testing.T) {
 	ExecObjectLayerTest(t, testDownloadWebHandler)
 }