merge nested hash readers (#9582)

The `ioutil.NopCloser(reader)` was hiding nested hash readers.

We make it an `io.Closer` so it can be attached without wrapping 
and allows for nesting, by merging the requests.

cmd/auth-handler.go

@@ -423,7 +423,7 @@ func isReqAuthenticated(ctx context.Context, r *http.Request, region string, sty
 	if err != nil {
 		return toAPIErrorCode(ctx, err)
 	}
-	r.Body = ioutil.NopCloser(reader)
+	r.Body = reader
 	return ErrNone
 }
 

cmd/object-api-putobject_test.go

@@ -172,22 +172,27 @@ func testObjectAPIPutObject(obj ObjectLayer, instanceType string, t TestErrHandl
 	}
 
 	for i, testCase := range testCases {
-		objInfo, actualErr := obj.PutObject(context.Background(), testCase.bucketName, testCase.objName, mustGetPutObjReader(t, bytes.NewReader(testCase.inputData), testCase.intputDataSize, testCase.inputMeta["etag"], testCase.inputSHA256), ObjectOptions{UserDefined: testCase.inputMeta})
+		in := mustGetPutObjReader(t, bytes.NewReader(testCase.inputData), testCase.intputDataSize, testCase.inputMeta["etag"], testCase.inputSHA256)
+		objInfo, actualErr := obj.PutObject(context.Background(), testCase.bucketName, testCase.objName, in, ObjectOptions{UserDefined: testCase.inputMeta})
 		if actualErr != nil && testCase.expectedError == nil {
 			t.Errorf("Test %d: %s: Expected to pass, but failed with: error %s.", i+1, instanceType, actualErr.Error())
+			continue
 		}
 		if actualErr == nil && testCase.expectedError != nil {
 			t.Errorf("Test %d: %s: Expected to fail with error \"%s\", but passed instead.", i+1, instanceType, testCase.expectedError.Error())
+			continue
 		}
 		// Failed as expected, but does it fail for the expected reason.
 		if actualErr != nil && actualErr != testCase.expectedError {
 			t.Errorf("Test %d: %s: Expected to fail with error \"%v\", but instead failed with error \"%v\" instead.", i+1, instanceType, testCase.expectedError, actualErr)
+			continue
 		}
 		// Test passes as expected, but the output values are verified for correctness here.
 		if actualErr == nil {
 			// Asserting whether the md5 output is correct.
 			if expectedMD5, ok := testCase.inputMeta["etag"]; ok && expectedMD5 != objInfo.ETag {
 				t.Errorf("Test %d: %s: Calculated Md5 different from the actual one %s.", i+1, instanceType, objInfo.ETag)
+				continue
 			}
 		}
 	}

cmd/test-utils_test.go

@@ -222,6 +222,8 @@ func initFSObjects(disk string, t *testing.T) (obj ObjectLayer) {
 // This makes it easy to run the TestServer from any of the tests.
 // Using this interface, functionalities to be used in tests can be made generalized, and can be integrated in benchmarks/unit tests/go check suite tests.
 type TestErrHandler interface {
+	Log(args ...interface{})
+	Logf(format string, args ...interface{})
 	Error(args ...interface{})
 	Errorf(format string, args ...interface{})
 	Failed() bool

pkg/hash/errors.go

@@ -16,6 +16,8 @@
 
 package hash
 
+import "fmt"
+
 // SHA256Mismatch - when content sha256 does not match with what was sent from client.
 type SHA256Mismatch struct {
 	ExpectedSHA256   string
@@ -23,7 +25,7 @@ type SHA256Mismatch struct {
 }
 
 func (e SHA256Mismatch) Error() string {
-	return "Bad sha256: Expected " + e.ExpectedSHA256 + " is not valid with what we calculated " + e.CalculatedSHA256
+	return "Bad sha256: Expected " + e.ExpectedSHA256 + " does not match calculated " + e.CalculatedSHA256
 }
 
 // BadDigest - Content-MD5 you specified did not match what we received.
@@ -33,5 +35,14 @@ type BadDigest struct {
 }
 
 func (e BadDigest) Error() string {
-	return "Bad digest: Expected " + e.ExpectedMD5 + " is not valid with what we calculated " + e.CalculatedMD5
+	return "Bad digest: Expected " + e.ExpectedMD5 + " does not match calculated " + e.CalculatedMD5
+}
+
+type ErrSizeMismatch struct {
+	Want int64
+	Got  int64
+}
+
+func (e ErrSizeMismatch) Error() string {
+	return fmt.Sprintf("Size mismatch: got %d, want %d", e.Got, e.Want)
 }

pkg/hash/reader.go

@@ -28,14 +28,13 @@ import (
 	sha256 "github.com/minio/sha256-simd"
 )
 
-var errNestedReader = errors.New("Nesting of Reader detected, not allowed")
-
 // Reader writes what it reads from an io.Reader to an MD5 and SHA256 hash.Hash.
 // Reader verifies that the content of the io.Reader matches the expected checksums.
 type Reader struct {
 	src        io.Reader
 	size       int64
 	actualSize int64
+	bytesRead  int64
 
 	md5sum, sha256sum   []byte // Byte values of md5sum, sha256sum of client sent values.
 	md5Hash, sha256Hash hash.Hash
@@ -44,44 +43,14 @@ type Reader struct {
 // NewReader returns a new hash Reader which computes the MD5 sum and
 // SHA256 sum (if set) of the provided io.Reader at EOF.
 func NewReader(src io.Reader, size int64, md5Hex, sha256Hex string, actualSize int64, strictCompat bool) (*Reader, error) {
-	if _, ok := src.(*Reader); ok {
-		return nil, errNestedReader
-	}
-
-	sha256sum, err := hex.DecodeString(sha256Hex)
-	if err != nil {
-		return nil, SHA256Mismatch{}
-	}
-
-	md5sum, err := hex.DecodeString(md5Hex)
-	if err != nil {
-		return nil, BadDigest{}
+	if r, ok := src.(*Reader); ok {
+		// Merge expectations and return parent.
+		return r.merge(size, md5Hex, sha256Hex, actualSize, strictCompat)
 	}
 
-	var sha256Hash hash.Hash
-	if len(sha256sum) != 0 {
-		sha256Hash = sha256.New()
-	}
-	var md5Hash hash.Hash
-	if strictCompat {
-		// Strict compatibility is set then we should
-		// calculate md5sum always.
-		md5Hash = md5.New()
-	} else if len(md5sum) != 0 {
-		md5Hash = md5.New()
-	}
-	if size >= 0 {
-		src = io.LimitReader(src, size)
-	}
-	return &Reader{
-		md5sum:     md5sum,
-		sha256sum:  sha256sum,
-		src:        src,
-		size:       size,
-		md5Hash:    md5Hash,
-		sha256Hash: sha256Hash,
-		actualSize: actualSize,
-	}, nil
+	// Create empty reader and merge into that.
+	r := Reader{src: src, size: -1, actualSize: -1}
+	return r.merge(size, md5Hex, sha256Hex, actualSize, strictCompat)
 }
 
 func (r *Reader) Read(p []byte) (n int, err error) {
@@ -94,10 +63,11 @@ func (r *Reader) Read(p []byte) (n int, err error) {
 			r.sha256Hash.Write(p[:n])
 		}
 	}
+	r.bytesRead += int64(n)
 
 	// At io.EOF verify if the checksums are right.
 	if err == io.EOF {
-		if cerr := r.Verify(); cerr != nil {
+		if cerr := r.verify(); cerr != nil {
 			return 0, cerr
 		}
 	}
@@ -150,9 +120,9 @@ func (r *Reader) SHA256HexString() string {
 	return hex.EncodeToString(r.sha256sum)
 }
 
-// Verify verifies if the computed MD5 sum and SHA256 sum are
+// verify verifies if the computed MD5 sum and SHA256 sum are
 // equal to the ones specified when creating the Reader.
-func (r *Reader) Verify() error {
+func (r *Reader) verify() error {
 	if r.sha256Hash != nil && len(r.sha256sum) > 0 {
 		if sum := r.sha256Hash.Sum(nil); !bytes.Equal(r.sha256sum, sum) {
 			return SHA256Mismatch{hex.EncodeToString(r.sha256sum), hex.EncodeToString(sum)}
@@ -165,3 +135,63 @@ func (r *Reader) Verify() error {
 	}
 	return nil
 }
+
+// merge another hash into this one.
+// There cannot be conflicting information given.
+func (r *Reader) merge(size int64, md5Hex, sha256Hex string, actualSize int64, strictCompat bool) (*Reader, error) {
+	if r.bytesRead > 0 {
+		return nil, errors.New("internal error: Already read from hash reader")
+	}
+	// Merge sizes.
+	// If not set before, just add it.
+	if r.size < 0 && size >= 0 {
+		r.src = io.LimitReader(r.src, size)
+		r.size = size
+	}
+	// If set before and set now they must match.
+	if r.size >= 0 && size >= 0 && r.size != size {
+		return nil, ErrSizeMismatch{Want: r.size, Got: size}
+	}
+
+	if r.actualSize <= 0 && actualSize >= 0 {
+		r.actualSize = actualSize
+	}
+
+	// Merge SHA256.
+	sha256sum, err := hex.DecodeString(sha256Hex)
+	if err != nil {
+		return nil, SHA256Mismatch{}
+	}
+
+	// If both are set, they must be the same.
+	if r.sha256Hash != nil && len(sha256sum) > 0 {
+		if !bytes.Equal(r.sha256sum, sha256sum) {
+			return nil, SHA256Mismatch{}
+		}
+	} else if len(sha256sum) > 0 {
+		r.sha256Hash = sha256.New()
+		r.sha256sum = sha256sum
+	}
+
+	// Merge MD5 Sum.
+	md5sum, err := hex.DecodeString(md5Hex)
+	if err != nil {
+		return nil, BadDigest{}
+	}
+	// If both are set, they must expect the same.
+	if r.md5Hash != nil && len(md5sum) > 0 {
+		if !bytes.Equal(r.md5sum, md5sum) {
+			return nil, BadDigest{}
+		}
+	} else if len(md5sum) > 0 || (r.md5Hash == nil && strictCompat) {
+		r.md5Hash = md5.New()
+		r.md5sum = md5sum
+	}
+	return r, nil
+}
+
+// Close and release resources.
+func (r *Reader) Close() error {
+	// Support the io.Closer interface.
+	return nil
+}

pkg/hash/reader_test.go

@@ -19,6 +19,7 @@ package hash
 import (
 	"bytes"
 	"encoding/hex"
+	"fmt"
 	"io"
 	"io/ioutil"
 	"testing"
@@ -71,20 +72,21 @@ func TestHashReaderHelperMethods(t *testing.T) {
 // Tests hash reader checksum verification.
 func TestHashReaderVerification(t *testing.T) {
 	testCases := []struct {
+		desc              string
 		src               io.Reader
 		size              int64
 		actualSize        int64
 		md5hex, sha256hex string
 		err               error
 	}{
-		// Success, no checksum verification provided.
 		{
+			desc:       "Success, no checksum verification provided.",
 			src:        bytes.NewReader([]byte("abcd")),
 			size:       4,
 			actualSize: 4,
 		},
-		// Failure md5 mismatch.
 		{
+			desc:       "Failure md5 mismatch.",
 			src:        bytes.NewReader([]byte("abcd")),
 			size:       4,
 			actualSize: 4,
@@ -94,8 +96,8 @@ func TestHashReaderVerification(t *testing.T) {
 				"e2fc714c4727ee9395f324cd2e7f331f",
 			},
 		},
-		// Failure sha256 mismatch.
 		{
+			desc:       "Failure sha256 mismatch.",
 			src:        bytes.NewReader([]byte("abcd")),
 			size:       4,
 			actualSize: 4,
@@ -105,33 +107,123 @@ func TestHashReaderVerification(t *testing.T) {
 				"88d4266fd4e6338d13b845fcf289579d209c897823b9217da3e161936f031589",
 			},
 		},
+		{
+			desc:       "Nested hash reader NewReader() should merge.",
+			src:        mustReader(t, bytes.NewReader([]byte("abcd")), 4, "", "", 4, false),
+			size:       4,
+			actualSize: 4,
+		},
+		{
+			desc:       "Incorrect sha256, nested",
+			src:        mustReader(t, bytes.NewReader([]byte("abcd")), 4, "", "", 4, false),
+			size:       4,
+			actualSize: 4,
+			sha256hex:  "50d858e0985ecc7f60418aaf0cc5ab587f42c2570a884095a9e8ccacd0f6545c",
+			err: SHA256Mismatch{
+				ExpectedSHA256:   "50d858e0985ecc7f60418aaf0cc5ab587f42c2570a884095a9e8ccacd0f6545c",
+				CalculatedSHA256: "88d4266fd4e6338d13b845fcf289579d209c897823b9217da3e161936f031589",
+			},
+		},
+		{
+			desc:       "Correct sha256, nested",
+			src:        mustReader(t, bytes.NewReader([]byte("abcd")), 4, "", "", 4, false),
+			size:       4,
+			actualSize: 4,
+			sha256hex:  "88d4266fd4e6338d13b845fcf289579d209c897823b9217da3e161936f031589",
+		},
+		{
+			desc:       "Correct sha256, nested, truncated",
+			src:        mustReader(t, bytes.NewReader([]byte("abcd-more-stuff-to-be ignored")), 4, "", "", 4, false),
+			size:       4,
+			actualSize: -1,
+			sha256hex:  "88d4266fd4e6338d13b845fcf289579d209c897823b9217da3e161936f031589",
+		},
+		{
+			desc:       "Correct sha256, nested, truncated, swapped",
+			src:        mustReader(t, bytes.NewReader([]byte("abcd-more-stuff-to-be ignored")), 4, "", "", -1, false),
+			size:       4,
+			actualSize: -1,
+			sha256hex:  "88d4266fd4e6338d13b845fcf289579d209c897823b9217da3e161936f031589",
+		},
+		{
+			desc:       "Incorrect MD5, nested",
+			src:        mustReader(t, bytes.NewReader([]byte("abcd")), 4, "", "", 4, false),
+			size:       4,
+			actualSize: 4,
+			md5hex:     "0773da587b322af3a8718cb418a715ce",
+			err: BadDigest{
+				ExpectedMD5:   "0773da587b322af3a8718cb418a715ce",
+				CalculatedMD5: "e2fc714c4727ee9395f324cd2e7f331f",
+			},
+		},
+		{
+			desc:       "Correct sha256, truncated",
+			src:        bytes.NewReader([]byte("abcd-morethan-4-bytes")),
+			size:       4,
+			actualSize: 4,
+			sha256hex:  "88d4266fd4e6338d13b845fcf289579d209c897823b9217da3e161936f031589",
+		},
+		{
+			desc:       "Correct MD5, nested",
+			src:        mustReader(t, bytes.NewReader([]byte("abcd")), 4, "", "", 4, false),
+			size:       4,
+			actualSize: 4,
+			md5hex:     "e2fc714c4727ee9395f324cd2e7f331f",
+		},
+		{
+			desc:       "Correct MD5, truncated",
+			src:        bytes.NewReader([]byte("abcd-morethan-4-bytes")),
+			size:       4,
+			actualSize: 4,
+			sha256hex:  "",
+			md5hex:     "e2fc714c4727ee9395f324cd2e7f331f",
+		},
+		{
+			desc:       "Correct MD5, nested, truncated",
+			src:        mustReader(t, bytes.NewReader([]byte("abcd-morestuff")), -1, "", "", -1, false),
+			size:       4,
+			actualSize: 4,
+			md5hex:     "e2fc714c4727ee9395f324cd2e7f331f",
+		},
 	}
 	for i, testCase := range testCases {
-		r, err := NewReader(testCase.src, testCase.size, testCase.md5hex, testCase.sha256hex, testCase.actualSize, false)
-		if err != nil {
-			t.Fatalf("Test %d: Initializing reader failed %s", i+1, err)
-		}
-		_, err = io.Copy(ioutil.Discard, r)
-		if err != nil {
-			if err.Error() != testCase.err.Error() {
-				t.Errorf("Test %d: Expected error %s, got error %s", i+1, testCase.err, err)
+		t.Run(fmt.Sprintf("case-%d", i+1), func(t *testing.T) {
+			r, err := NewReader(testCase.src, testCase.size, testCase.md5hex, testCase.sha256hex, testCase.actualSize, false)
+			if err != nil {
+				t.Fatalf("Test %q: Initializing reader failed %s", testCase.desc, err)
+			}
+			_, err = io.Copy(ioutil.Discard, r)
+			if err != nil {
+				if err.Error() != testCase.err.Error() {
+					t.Errorf("Test %q: Expected error %s, got error %s", testCase.desc, testCase.err, err)
+				}
 			}
-		}
+		})
+	}
+}
+
+func mustReader(t *testing.T, src io.Reader, size int64, md5Hex, sha256Hex string, actualSize int64, strictCompat bool) *Reader {
+	r, err := NewReader(src, size, md5Hex, sha256Hex, actualSize, strictCompat)
+	if err != nil {
+		t.Fatal(err)
 	}
+	return r
 }
 
 // Tests NewReader() constructor with invalid arguments.
 func TestHashReaderInvalidArguments(t *testing.T) {
 	testCases := []struct {
+		desc              string
 		src               io.Reader
 		size              int64
 		actualSize        int64
 		md5hex, sha256hex string
 		success           bool
 		expectedErr       error
+		strict            bool
 	}{
-		// Invalid md5sum NewReader() will fail.
 		{
+			desc:        "Invalid md5sum NewReader() will fail.",
 			src:         bytes.NewReader([]byte("abcd")),
 			size:        4,
 			actualSize:  4,
@@ -139,8 +231,8 @@ func TestHashReaderInvalidArguments(t *testing.T) {
 			success:     false,
 			expectedErr: BadDigest{},
 		},
-		// Invalid sha256 NewReader() will fail.
 		{
+			desc:        "Invalid sha256 NewReader() will fail.",
 			src:         bytes.NewReader([]byte("abcd")),
 			size:        4,
 			actualSize:  4,
@@ -148,33 +240,78 @@ func TestHashReaderInvalidArguments(t *testing.T) {
 			success:     false,
 			expectedErr: SHA256Mismatch{},
 		},
-		// Nested hash reader NewReader() will fail.
 		{
-			src:         &Reader{src: bytes.NewReader([]byte("abcd"))},
+			desc:       "Nested hash reader NewReader() should merge.",
+			src:        mustReader(t, bytes.NewReader([]byte("abcd")), 4, "", "", 4, false),
+			size:       4,
+			actualSize: 4,
+			success:    true,
+		},
+		{
+			desc:        "Mismatching sha256",
+			src:         mustReader(t, bytes.NewReader([]byte("abcd")), 4, "", "88d4266fd4e6338d13b845fcf289579d209c897823b9217da3e161936f031589", 4, false),
+			size:        4,
+			actualSize:  4,
+			sha256hex:   "50d858e0985ecc7f60418aaf0cc5ab587f42c2570a884095a9e8ccacd0f6545c",
+			success:     false,
+			expectedErr: SHA256Mismatch{},
+		},
+		{
+			desc:       "Correct sha256",
+			src:        mustReader(t, bytes.NewReader([]byte("abcd")), 4, "", "88d4266fd4e6338d13b845fcf289579d209c897823b9217da3e161936f031589", 4, false),
+			size:       4,
+			actualSize: 4,
+			sha256hex:  "88d4266fd4e6338d13b845fcf289579d209c897823b9217da3e161936f031589",
+			success:    true,
+		},
+		{
+			desc:        "Mismatching MD5",
+			src:         mustReader(t, bytes.NewReader([]byte("abcd")), 4, "e2fc714c4727ee9395f324cd2e7f331f", "", 4, false),
 			size:        4,
 			actualSize:  4,
+			md5hex:      "0773da587b322af3a8718cb418a715ce",
 			success:     false,
-			expectedErr: errNestedReader,
+			expectedErr: BadDigest{},
 		},
-		// Expected inputs, NewReader() will succeed.
 		{
+			desc:       "Correct MD5",
+			src:        mustReader(t, bytes.NewReader([]byte("abcd")), 4, "e2fc714c4727ee9395f324cd2e7f331f", "", 4, false),
+			size:       4,
+			actualSize: 4,
+			md5hex:     "e2fc714c4727ee9395f324cd2e7f331f",
+			success:    true,
+		},
+		{
+			desc:       "Nothing, all ok",
 			src:        bytes.NewReader([]byte("abcd")),
 			size:       4,
 			actualSize: 4,
 			success:    true,
 		},
+		{
+			desc:        "Nested, size mismatch",
+			src:         mustReader(t, bytes.NewReader([]byte("abcd-morestuff")), 4, "", "", -1, false),
+			size:        2,
+			actualSize:  -1,
+			success:     false,
+			expectedErr: ErrSizeMismatch{Want: 4, Got: 2},
+		},
 	}
 
 	for i, testCase := range testCases {
-		_, err := NewReader(testCase.src, testCase.size, testCase.md5hex, testCase.sha256hex, testCase.actualSize, false)
-		if err != nil && testCase.success {
-			t.Errorf("Test %d: Expected success, but got error %s instead", i+1, err)
-		}
-		if err == nil && !testCase.success {
-			t.Errorf("Test %d: Expected error, but got success", i+1)
-		}
-		if err != testCase.expectedErr {
-			t.Errorf("Test %d: Expected error %v, but got %v", i+1, testCase.expectedErr, err)
-		}
+		t.Run(fmt.Sprintf("case-%d", i+1), func(t *testing.T) {
+			_, err := NewReader(testCase.src, testCase.size, testCase.md5hex, testCase.sha256hex, testCase.actualSize, testCase.strict)
+			if err != nil && testCase.success {
+				t.Errorf("Test %q: Expected success, but got error %s instead", testCase.desc, err)
+			}
+			if err == nil && !testCase.success {
+				t.Errorf("Test %q: Expected error, but got success", testCase.desc)
+			}
+			if !testCase.success {
+				if err != testCase.expectedErr {
+					t.Errorf("Test %q: Expected error %v, but got %v", testCase.desc, testCase.expectedErr, err)
+				}
+			}
+		})
 	}
 }