From 83fe70f71049c9b60e89bf8413f23073abd75137 Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Tue, 27 Nov 2018 13:23:32 -0800 Subject: [PATCH] Fix CopyObject regression calculating md5sum (#6868) CopyObject() failed to calculate proper md5sum when without encryption headers. This is a regression fix perhaps introduced in commit 5f6d717b7a Fixes https://github.com/minio/minio-go/issues/1044 --- cmd/object-api-utils.go | 14 ++++++++------ cmd/object-handlers.go | 10 +++++++--- cmd/object-handlers_test.go | 29 +++++++++++++++++++++++++---- 3 files changed, 40 insertions(+), 13 deletions(-) diff --git a/cmd/object-api-utils.go b/cmd/object-api-utils.go index 50d98f647..c36877a7e 100644 --- a/cmd/object-api-utils.go +++ b/cmd/object-api-utils.go @@ -635,12 +635,13 @@ func (p *PutObjReader) MD5CurrentHexString() string { func NewPutObjReader(rawReader *hash.Reader, encReader *hash.Reader, encKey []byte) *PutObjReader { p := PutObjReader{Reader: rawReader, rawReader: rawReader} - var objKey crypto.ObjectKey - copy(objKey[:], encKey) - p.sealMD5Fn = sealETagFn(objKey) - if encReader != nil { + if len(encKey) != 0 && encReader != nil { + var objKey crypto.ObjectKey + copy(objKey[:], encKey) + p.sealMD5Fn = sealETagFn(objKey) p.Reader = encReader } + return &p } @@ -651,9 +652,10 @@ func sealETag(encKey crypto.ObjectKey, md5CurrSum []byte) []byte { } return encKey.SealETag(md5CurrSum) } + func sealETagFn(key crypto.ObjectKey) SealMD5CurrFn { - fn1 := func(md5sumcurr []byte) []byte { + fn := func(md5sumcurr []byte) []byte { return sealETag(key, md5sumcurr) } - return fn1 + return fn } diff --git a/cmd/object-handlers.go b/cmd/object-handlers.go index b88e6694b..6f188fc85 100644 --- a/cmd/object-handlers.go +++ b/cmd/object-handlers.go @@ -774,6 +774,7 @@ func (api objectAPIHandlers) CopyObjectHandler(w http.ResponseWriter, r *http.Re writeErrorResponse(w, toAPIErrorCode(ctx, err), r.URL, guessIsBrowserReq(r)) return } + length = actualSize } // No need to compress for remote etcd calls @@ -813,8 +814,10 @@ func (api objectAPIHandlers) CopyObjectHandler(w http.ResponseWriter, r *http.Re writeErrorResponse(w, toAPIErrorCode(ctx, err), r.URL, guessIsBrowserReq(r)) return } + rawReader := srcInfo.Reader - pReader := NewPutObjReader(srcInfo.Reader, srcInfo.Reader, nil) + pReader := NewPutObjReader(srcInfo.Reader, nil, nil) + var encMetadata = make(map[string]string) if objectAPI.IsEncryptionSupported() && !isCompressed { // Encryption parameters not applicable for this object. @@ -882,15 +885,15 @@ func (api objectAPIHandlers) CopyObjectHandler(w http.ResponseWriter, r *http.Re switch { case !isSourceEncrypted && !isTargetEncrypted: - fallthrough + targetSize = srcInfo.Size case isSourceEncrypted && isTargetEncrypted: targetSize = srcInfo.Size - // Source not encrypted and target encrypted case !isSourceEncrypted && isTargetEncrypted: targetSize = srcInfo.EncryptedSize() case isSourceEncrypted && !isTargetEncrypted: targetSize, _ = srcInfo.DecryptedSize() } + if isTargetEncrypted { reader, objEncKey, err = newEncryptReader(srcInfo.Reader, newKey, dstBucket, dstObject, encMetadata, sseS3) if err != nil { @@ -910,6 +913,7 @@ func (api objectAPIHandlers) CopyObjectHandler(w http.ResponseWriter, r *http.Re writeErrorResponse(w, toAPIErrorCode(ctx, err), r.URL, guessIsBrowserReq(r)) return } + pReader = NewPutObjReader(rawReader, srcInfo.Reader, objEncKey) } } diff --git a/cmd/object-handlers_test.go b/cmd/object-handlers_test.go index f74d942e6..5fc2f193b 100644 --- a/cmd/object-handlers_test.go +++ b/cmd/object-handlers_test.go @@ -21,6 +21,7 @@ import ( "context" "crypto/md5" "encoding/base64" + "encoding/hex" "encoding/xml" "fmt" "io" @@ -1811,9 +1812,13 @@ func testAPICopyObjectHandler(obj ObjectLayer, instanceType, bucketName string, // this is required even to assert the copied object, bytesData := []struct { byteData []byte + md5sum string }{ - {generateBytesData(6 * humanize.KiByte)}, + {byteData: generateBytesData(6 * humanize.KiByte)}, } + h := md5.New() + h.Write(bytesData[0].byteData) + bytesData[0].md5sum = hex.EncodeToString(h.Sum(nil)) buffers := []*bytes.Buffer{ new(bytes.Buffer), @@ -1826,23 +1831,29 @@ func testAPICopyObjectHandler(obj ObjectLayer, instanceType, bucketName string, objectName string contentLength int64 textData []byte + md5sum string metaData map[string]string }{ // case - 1. - {bucketName, objectName, int64(len(bytesData[0].byteData)), bytesData[0].byteData, make(map[string]string)}, + {bucketName, objectName, int64(len(bytesData[0].byteData)), bytesData[0].byteData, bytesData[0].md5sum, make(map[string]string)}, // case - 2. // used for anonymous HTTP request test. - {bucketName, anonObject, int64(len(bytesData[0].byteData)), bytesData[0].byteData, make(map[string]string)}, + {bucketName, anonObject, int64(len(bytesData[0].byteData)), bytesData[0].byteData, bytesData[0].md5sum, make(map[string]string)}, } + // iterate through the above set of inputs and upload the object. for i, input := range putObjectInputs { // uploading the object. - _, err = obj.PutObject(context.Background(), input.bucketName, input.objectName, mustGetPutObjReader(t, bytes.NewBuffer(input.textData), input.contentLength, input.metaData[""], ""), input.metaData, opts) + var objInfo ObjectInfo + objInfo, err = obj.PutObject(context.Background(), input.bucketName, input.objectName, mustGetPutObjReader(t, bytes.NewBuffer(input.textData), input.contentLength, input.metaData[""], ""), input.metaData, opts) // if object upload fails stop the test. if err != nil { t.Fatalf("Put Object case %d: Error uploading object: %v", i+1, err) } + if objInfo.ETag != input.md5sum { + t.Fatalf("Put Object case %d: Checksum mismatched: got %s, expected %s", i+1, input.md5sum, objInfo.ETag) + } } // test cases with inputs and expected result for Copy Object. @@ -2105,6 +2116,16 @@ func testAPICopyObjectHandler(obj ObjectLayer, instanceType, bucketName string, t.Fatalf("Test %d: %s: Expected the response status to be `%d`, but instead found `%d`", i+1, instanceType, testCase.expectedRespStatus, rec.Code) } if rec.Code == http.StatusOK { + var cpObjResp CopyObjectResponse + if err = xml.Unmarshal(rec.Body.Bytes(), &cpObjResp); err != nil { + t.Fatalf("Test %d: %s: Failed to parse the CopyObjectResult response: %s", i+1, instanceType, err) + } + + retag := canonicalizeETag(cpObjResp.ETag) + if retag != bytesData[0].md5sum { + t.Fatalf("Test %d: %s: Failed to CopyObject: got %s, expected %s", i+1, instanceType, retag, bytesData[0].md5sum) + } + // See if the new object is formed. // testing whether the copy was successful. err = obj.GetObject(context.Background(), testCase.bucketName, testCase.newObjectName, 0, int64(len(bytesData[0].byteData)), buffers[0], "", opts)