fix: fixes error handling for unsigned streaming upload malformed encoding

Fixes #1666
Fixes #1660

Unsigned streaming payload trailers have strict encoding rules for the request body. If the body isn’t encoded correctly, the expected `IncompleteBody` API error is now returned. Incorrect encoding includes things like invalid chunk sizes, missing delimiters, or malformed `\r\n` sequences.

s3api/utils/unsigned-chunk-reader.go

@@ -111,6 +111,12 @@ func (ucr *UnsignedChunkReader) Read(p []byte) (int, error) {
 		// Read and cache the payload
 		_, err = io.ReadFull(rdr, payload)
 		if err != nil {
+			// the chunk size is not 0 and if io.EOF is returned
+			// it means the body is incomplete
+			if errors.Is(err, io.EOF) {
+				debuglogger.Logf("unexpected EOF when reading chunk data")
+				return 0, s3err.GetAPIError(s3err.ErrIncompleteBody)
+			}
 			debuglogger.Logf("failed to read chunk data: %v", err)
 			return 0, err
 		}
@@ -149,14 +155,11 @@ func (ucr *UnsignedChunkReader) readAndSkip(data ...byte) error {
 	for _, d := range data {
 		b, err := ucr.reader.ReadByte()
 		if err != nil {
-			if err == io.EOF {
-				return io.ErrUnexpectedEOF
-			}
-			return err
+			return s3err.GetAPIError(s3err.ErrIncompleteBody)
 		}
 
 		if b != d {
-			return errMalformedEncoding
+			return s3err.GetAPIError(s3err.ErrIncompleteBody)
 		}
 	}
 
@@ -165,17 +168,24 @@ func (ucr *UnsignedChunkReader) readAndSkip(data ...byte) error {
 
 // Extracts the chunk size from the payload
 func (ucr *UnsignedChunkReader) extractChunkSize() (int64, error) {
-	line, err := ucr.reader.ReadString('\n')
+	line, err := ucr.reader.ReadString('\r')
 	if err != nil {
 		debuglogger.Logf("failed to parse chunk size: %v", err)
-		return 0, errMalformedEncoding
+		return 0, s3err.GetAPIError(s3err.ErrIncompleteBody)
 	}
+
+	err = ucr.readAndSkip('\n')
+	if err != nil {
+		debuglogger.Logf("failed to read the second byte (\\n) after chunk size")
+		return 0, err
+	}
+
 	line = strings.TrimSpace(line)
 
 	chunkSize, err := strconv.ParseInt(line, 16, 64)
 	if err != nil {
 		debuglogger.Logf("failed to convert chunk size: %v", err)
-		return 0, errMalformedEncoding
+		return 0, s3err.GetAPIError(s3err.ErrIncompleteBody)
 	}
 
 	debuglogger.Infof("chunk size extracted: %v", chunkSize)
@@ -191,11 +201,7 @@ func (ucr *UnsignedChunkReader) readTrailer() error {
 	for {
 		v, err := ucr.reader.ReadByte()
 		if err != nil {
-			debuglogger.Logf("failed to read byte: %v", err)
-			if err == io.EOF {
-				return io.ErrUnexpectedEOF
-			}
-			return err
+			return s3err.GetAPIError(s3err.ErrIncompleteBody)
 		}
 		if v != '\r' {
 			hasChecksum = true
@@ -210,10 +216,7 @@ func (ucr *UnsignedChunkReader) readTrailer() error {
 			err := ucr.readAndSkip('\n')
 			if err != nil {
 				debuglogger.Logf("failed to read chunk last byte: \\n: %v", err)
-				if err == io.EOF {
-					return io.ErrUnexpectedEOF
-				}
-				return err
+				return s3err.GetAPIError(s3err.ErrIncompleteBody)
 			}
 
 			break
@@ -223,14 +226,11 @@ func (ucr *UnsignedChunkReader) readTrailer() error {
 		_, err = io.ReadFull(ucr.reader, tmp[:])
 		if err != nil {
 			debuglogger.Logf("failed to read chunk ending: \\n\\r\\n: %v", err)
-			if err == io.EOF {
-				return io.ErrUnexpectedEOF
-			}
-			return err
+			return s3err.GetAPIError(s3err.ErrIncompleteBody)
 		}
 		if !bytes.Equal(tmp[:], trailerDelim) {
 			debuglogger.Logf("incorrect trailer delimiter: (expected): \\n\\r\\n, (got): %q", tmp[:])
-			return errMalformedEncoding
+			return s3err.GetAPIError(s3err.ErrIncompleteBody)
 		}
 		break
 	}

s3err/s3err.go

@@ -88,6 +88,7 @@ const (
 	ErrInvalidCompleteMpPartNumber
 	ErrInternalError
 	ErrNonEmptyRequestBody
+	ErrIncompleteBody
 	ErrInvalidCopyDest
 	ErrInvalidCopySourceRange
 	ErrInvalidCopySourceBucket
@@ -328,6 +329,11 @@ var errorCodeResponse = map[ErrorCode]APIError{
 		Description:    "The request included a body. Requests of this type must not include a non-empty body.",
 		HTTPStatusCode: http.StatusBadRequest,
 	},
+	ErrIncompleteBody: {
+		Code:           "IncompleteBody",
+		Description:    "The request body terminated unexpectedly",
+		HTTPStatusCode: http.StatusBadRequest,
+	},
 	ErrInvalidPart: {
 		Code:           "InvalidPart",
 		Description:    "One or more of the specified parts could not be found.  The part may not have been uploaded, or the specified entity tag may not match the part's entity tag.",

tests/integration/group-tests.go

@@ -1100,6 +1100,7 @@ func TestUnsignedStreaminPayloadTrailer(ts *TestState) {
 		ts.Run(UnsignedStreamingPayloadTrailer_incorrect_trailing_checksum)
 		ts.Run(UnsignedStreamingPayloadTrailer_multiple_checksum_headers)
 		ts.Run(UnsignedStreamingPayloadTrailer_sdk_algo_and_trailer_mismatch)
+		ts.Run(UnsignedStreamingPayloadTrailer_incomplete_body)
 		ts.Run(UnsignedStreamingPayloadTrailer_no_trailer_should_calculate_crc64nvme)
 		ts.Run(UnsignedStreamingPayloadTrailer_no_payload_trailer_only_headers)
 		ts.Run(UnsignedStreamingPayloadTrailer_success_both_sdk_algo_and_trailer)
@@ -1752,6 +1753,7 @@ func GetIntTests() IntTests {
 		"UnsignedStreamingPayloadTrailer_incorrect_trailing_checksum":              UnsignedStreamingPayloadTrailer_incorrect_trailing_checksum,
 		"UnsignedStreamingPayloadTrailer_multiple_checksum_headers":                UnsignedStreamingPayloadTrailer_multiple_checksum_headers,
 		"UnsignedStreamingPayloadTrailer_sdk_algo_and_trailer_mismatch":            UnsignedStreamingPayloadTrailer_sdk_algo_and_trailer_mismatch,
+		"UnsignedStreamingPayloadTrailer_incomplete_body":                          UnsignedStreamingPayloadTrailer_incomplete_body,
 		"UnsignedStreamingPayloadTrailer_no_trailer_should_calculate_crc64nvme":    UnsignedStreamingPayloadTrailer_no_trailer_should_calculate_crc64nvme,
 		"UnsignedStreamingPayloadTrailer_no_payload_trailer_only_headers":          UnsignedStreamingPayloadTrailer_no_payload_trailer_only_headers,
 		"UnsignedStreamingPayloadTrailer_success_both_sdk_algo_and_trailer":        UnsignedStreamingPayloadTrailer_success_both_sdk_algo_and_trailer,

tests/integration/unsigned_streaming_payload_trailer.go

@@ -175,6 +175,42 @@ func UnsignedStreamingPayloadTrailer_sdk_algo_and_trailer_mismatch(s *S3Conf) er
 	})
 }
 
+func UnsignedStreamingPayloadTrailer_incomplete_body(s *S3Conf) error {
+	testName := "UnsignedStreamingPayloadTrailer_incomplete_body"
+	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
+		object := "my-object"
+		for i, body := range [][]byte{
+			[]byte("A\ndummy data\r\n0\r\n\r\n"),
+			[]byte("A\r\ndummy data0\r\n\r\n"),
+			[]byte("A\r\nXYZ\r\ndummy data\r\n0\r\n\r\n"),
+			[]byte("B\r\ndummy data\r\n0\r\n\r\n"),
+			[]byte("A\r\ndummy data\r\n0\r\n"),
+			[]byte("A\r\ndummy data\r\n0\r\nx-amz-checksum-crc64nvme:dPVWc2vU1+Q=\r\n"),
+			[]byte("A\r\n"),
+			[]byte("A\r\nA\r\ndummy data\r\n0\r\n\r\n"),
+			// invalid chunk size
+			[]byte("invalid_chunk_size\r\ndummy data\r\n0\r\nx-amz-checksum-crc64nvme:dPVWc2vU1+Q=\r\n\r\n"),
+			[]byte("A\r\ndummy data\r\nJ\r\nx-amz-checksum-crc64nvme:dPVWc2vU1+Q=\r\n\r\n"),
+		} {
+			reqHeaders := map[string]string{
+				"x-amz-decoded-content-length": "10",
+				"x-amz-trailer":                "x-amz-checksum-crc64nvme",
+			}
+
+			_, apiErr, err := testUnsignedStreamingPayloadTrailerObjectPut(s, bucket, object, body, reqHeaders)
+			if err != nil {
+				return fmt.Errorf("test %v failed: %w", i+1, err)
+			}
+
+			if err := compareS3ApiError(s3err.GetAPIError(s3err.ErrIncompleteBody), apiErr); err != nil {
+				return fmt.Errorf("test %v failed: %w", i+1, err)
+			}
+		}
+
+		return nil
+	})
+}
+
 func UnsignedStreamingPayloadTrailer_no_trailer_should_calculate_crc64nvme(s *S3Conf) error {
 	testName := "UnsignedStreamingPayloadTrailer_no_trailer_should_calculate_crc64nvme"
 	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {