validate correct ETag for the parts sent during CompleteMultipart (#15751)

.github/workflows/go.yml

@@ -41,10 +41,7 @@ jobs:
         env:
           CGO_ENABLED: 0
           GO111MODULE: on
-          MINIO_KMS_KES_CERT_FILE: /home/runner/work/minio/minio/.github/workflows/root.cert
+          MINIO_KMS_SECRET_KEY: "my-minio-key:OSMM+vkKUTCvQs9YL/CVMIMt43HFhkUpqJxTmGl6rYw="
-          MINIO_KMS_KES_KEY_FILE: /home/runner/work/minio/minio/.github/workflows/root.key
-          MINIO_KMS_KES_ENDPOINT: "https://play.min.io:7373"
-          MINIO_KMS_KES_KEY_NAME: "my-minio-key"
           MINIO_KMS_AUTO_ENCRYPTION: on
         run: |
           sudo sysctl net.ipv6.conf.all.disable_ipv6=0

cmd/disk-cache-backend.go

@@ -706,7 +706,7 @@ func (c *diskCache) updateMetadata(ctx context.Context, bucket, object, etag str
 
 	if globalCacheKMS != nil {
 		// Calculating object encryption key
-		key, err = decryptObjectInfo(key, bucket, object, m.Meta)
+		key, err = decryptObjectMeta(key, bucket, object, m.Meta)
 		if err != nil {
 			return err
 		}
@@ -1397,7 +1397,7 @@ func (c *diskCache) SavePartMetadata(ctx context.Context, bucket, object, upload
 	var objectEncryptionKey crypto.ObjectKey
 	if globalCacheKMS != nil {
 		// Calculating object encryption key
-		key, err = decryptObjectInfo(key, bucket, object, m.Meta)
+		key, err = decryptObjectMeta(key, bucket, object, m.Meta)
 		if err != nil {
 			return err
 		}
@@ -1427,7 +1427,7 @@ func newCachePartEncryptReader(ctx context.Context, bucket, object string, partI
 	var objectEncryptionKey, partEncryptionKey crypto.ObjectKey
 
 	// Calculating object encryption key
-	key, err = decryptObjectInfo(key, bucket, object, metadata)
+	key, err = decryptObjectMeta(key, bucket, object, metadata)
 	if err != nil {
 		return nil, err
 	}

cmd/encryption-v1.go

@@ -473,7 +473,7 @@ func EncryptRequest(content io.Reader, r *http.Request, bucket, object string, m
 	return newEncryptReader(r.Context(), content, kind, keyID, key, bucket, object, metadata, ctx)
 }
 
-func decryptObjectInfo(key []byte, bucket, object string, metadata map[string]string) ([]byte, error) {
+func decryptObjectMeta(key []byte, bucket, object string, metadata map[string]string) ([]byte, error) {
 	switch kind, _ := crypto.IsEncrypted(metadata); kind {
 	case crypto.S3:
 		var KMS kms.KMS = GlobalKMS
@@ -544,7 +544,7 @@ func DecryptCopyRequestR(client io.Reader, h http.Header, bucket, object string,
 }
 
 func newDecryptReader(client io.Reader, key []byte, bucket, object string, seqNumber uint32, metadata map[string]string) (io.Reader, error) {
-	objectEncryptionKey, err := decryptObjectInfo(key, bucket, object, metadata)
+	objectEncryptionKey, err := decryptObjectMeta(key, bucket, object, metadata)
 	if err != nil {
 		return nil, err
 	}
@@ -656,7 +656,7 @@ func (d *DecryptBlocksReader) buildDecrypter(partID int) error {
 		return err
 	}
 
-	objectEncryptionKey, err := decryptObjectInfo(key, d.bucket, d.object, m)
+	objectEncryptionKey, err := decryptObjectMeta(key, d.bucket, d.object, m)
 	if err != nil {
 		return err
 	}
@@ -822,7 +822,7 @@ func getDecryptedETag(headers http.Header, objInfo ObjectInfo, copySource bool)
 		return objInfo.ETag[len(objInfo.ETag)-32:]
 	}
 
-	objectEncryptionKey, err := decryptObjectInfo(key[:], objInfo.Bucket, objInfo.Name, objInfo.UserDefined)
+	objectEncryptionKey, err := decryptObjectMeta(key[:], objInfo.Bucket, objInfo.Name, objInfo.UserDefined)
 	if err != nil {
 		return objInfo.ETag
 	}
@@ -1085,7 +1085,7 @@ func (o *ObjectInfo) metadataDecrypter() objectMetaDecryptFn {
 			return input, nil
 		}
 
-		key, err := decryptObjectInfo(nil, o.Bucket, o.Name, o.UserDefined)
+		key, err := decryptObjectMeta(nil, o.Bucket, o.Name, o.UserDefined)
 		if err != nil {
 			return nil, err
 		}

cmd/encryption-v1_test.go

@@ -79,7 +79,7 @@ func TestEncryptRequest(t *testing.T) {
 	}
 }
 
-var decryptObjectInfoTests = []struct {
+var decryptObjectMetaTests = []struct {
 	info    ObjectInfo
 	request *http.Request
 	expErr  error
@@ -122,7 +122,7 @@ var decryptObjectInfoTests = []struct {
 }
 
 func TestDecryptObjectInfo(t *testing.T) {
-	for i, test := range decryptObjectInfoTests {
+	for i, test := range decryptObjectMetaTests {
 		if encrypted, err := DecryptObjectInfo(&test.info, test.request); err != test.expErr {
 			t.Errorf("Test %d: Decryption returned wrong error code: got %d , want %d", i, err, test.expErr)
 		} else if _, enc := crypto.IsEncrypted(test.info.UserDefined); encrypted && enc != encrypted {

cmd/erasure-multipart.go

@@ -32,6 +32,7 @@ import (
 
 	"github.com/klauspost/readahead"
 	"github.com/minio/minio-go/v7/pkg/set"
+	"github.com/minio/minio/internal/crypto"
 	"github.com/minio/minio/internal/hash"
 	xhttp "github.com/minio/minio/internal/http"
 	"github.com/minio/minio/internal/logger"
@@ -982,8 +983,28 @@ func (er erasureObjects) CompleteMultipartUpload(ctx context.Context, bucket str
 			}
 		}
 	}
+
 	var checksumCombined []byte
 
+	// However, in case of encryption, the persisted part ETags don't match
+	// what we have sent to the client during PutObjectPart. The reason is
+	// that ETags are encrypted. Hence, the client will send a list of complete
+	// part ETags of which non can match the ETag of any part. For example
+	//   ETag (client):          30902184f4e62dd8f98f0aaff810c626
+	//   ETag (server-internal): 20000f00ce5dc16e3f3b124f586ae1d88e9caa1c598415c2759bbb50e84a59f630902184f4e62dd8f98f0aaff810c626
+	//
+	// Therefore, we adjust all ETags sent by the client to match what is stored
+	// on the backend.
+	kind, isEncrypted := crypto.IsEncrypted(fi.Metadata)
+
+	var objectEncryptionKey []byte
+	if isEncrypted && kind == crypto.S3 {
+		objectEncryptionKey, err = decryptObjectMeta(nil, bucket, object, fi.Metadata)
+		if err != nil {
+			return oi, err
+		}
+	}
+
 	for i, part := range partInfoFiles {
 		partID := parts[i].PartNumber
 		if part.Error != "" || !part.Exists {
@@ -1042,21 +1063,22 @@ func (er erasureObjects) CompleteMultipartUpload(ctx context.Context, bucket str
 			}
 			return oi, invp
 		}
-		gotPart := currentFI.Parts[partIdx]
+		expPart := currentFI.Parts[partIdx]
 
 		// ensure that part ETag is canonicalized to strip off extraneous quotes
 		part.ETag = canonicalizeETag(part.ETag)
-		if gotPart.ETag != part.ETag {
+		expETag := tryDecryptETag(objectEncryptionKey, expPart.ETag, kind != crypto.S3)
+		if expETag != part.ETag {
 			invp := InvalidPart{
 				PartNumber: part.PartNumber,
-				ExpETag:    gotPart.ETag,
+				ExpETag:    expETag,
 				GotETag:    part.ETag,
 			}
 			return oi, invp
 		}
 
 		if checksumType.IsSet() {
-			crc := gotPart.Checksums[checksumType.String()]
+			crc := expPart.Checksums[checksumType.String()]
 			if crc == "" {
 				return oi, InvalidPart{
 					PartNumber: part.PartNumber,
@@ -1088,24 +1110,24 @@ func (er erasureObjects) CompleteMultipartUpload(ctx context.Context, bucket str
 		if (i < len(parts)-1) && !isMinAllowedPartSize(currentFI.Parts[partIdx].ActualSize) {
 			return oi, PartTooSmall{
 				PartNumber: part.PartNumber,
-				PartSize:   gotPart.ActualSize,
+				PartSize:   expPart.ActualSize,
 				PartETag:   part.ETag,
 			}
 		}
 
 		// Save for total object size.
-		objectSize += gotPart.Size
+		objectSize += expPart.Size
 
 		// Save the consolidated actual size.
-		objectActualSize += gotPart.ActualSize
+		objectActualSize += expPart.ActualSize
 
 		// Add incoming parts.
 		fi.Parts[i] = ObjectPartInfo{
 			Number:     part.PartNumber,
-			Size:       gotPart.Size,
+			Size:       expPart.Size,
-			ActualSize: gotPart.ActualSize,
+			ActualSize: expPart.ActualSize,
-			ModTime:    gotPart.ModTime,
+			ModTime:    expPart.ModTime,
-			Index:      gotPart.Index,
+			Index:      expPart.Index,
 			Checksums:  nil, // Not transferred since we do not need it.
 		}
 	}

cmd/erasure-single-drive.go

@@ -43,6 +43,7 @@ import (
 	"github.com/minio/minio/internal/bucket/lifecycle"
 	"github.com/minio/minio/internal/bucket/object/lock"
 	"github.com/minio/minio/internal/bucket/replication"
+	"github.com/minio/minio/internal/crypto"
 	"github.com/minio/minio/internal/event"
 	"github.com/minio/minio/internal/hash"
 	xhttp "github.com/minio/minio/internal/http"
@@ -2677,6 +2678,25 @@ func (es *erasureSingle) CompleteMultipartUpload(ctx context.Context, bucket str
 		return oi, err
 	}
 
+	// However, in case of encryption, the persisted part ETags don't match
+	// what we have sent to the client during PutObjectPart. The reason is
+	// that ETags are encrypted. Hence, the client will send a list of complete
+	// part ETags of which non can match the ETag of any part. For example
+	//   ETag (client):          30902184f4e62dd8f98f0aaff810c626
+	//   ETag (server-internal): 20000f00ce5dc16e3f3b124f586ae1d88e9caa1c598415c2759bbb50e84a59f630902184f4e62dd8f98f0aaff810c626
+	//
+	// Therefore, we adjust all ETags sent by the client to match what is stored
+	// on the backend.
+	kind, isEncrypted := crypto.IsEncrypted(fi.Metadata)
+
+	var objectEncryptionKey []byte
+	if isEncrypted && kind == crypto.S3 {
+		objectEncryptionKey, err = decryptObjectMeta(nil, bucket, object, fi.Metadata)
+		if err != nil {
+			return oi, err
+		}
+	}
+
 	// Calculate full object size.
 	var objectSize int64
 
@@ -2707,10 +2727,11 @@ func (es *erasureSingle) CompleteMultipartUpload(ctx context.Context, bucket str
 
 		// ensure that part ETag is canonicalized to strip off extraneous quotes
 		part.ETag = canonicalizeETag(part.ETag)
-		if currentFI.Parts[partIdx].ETag != part.ETag {
+		expETag := tryDecryptETag(objectEncryptionKey, currentFI.Parts[partIdx].ETag, kind != crypto.S3)
+		if expETag != part.ETag {
 			invp := InvalidPart{
 				PartNumber: part.PartNumber,
-				ExpETag:    currentFI.Parts[partIdx].ETag,
+				ExpETag:    expETag,
 				GotETag:    part.ETag,
 			}
 			return oi, invp

cmd/object-handlers.go

@@ -2534,7 +2534,7 @@ func (api objectAPIHandlers) CopyObjectPartHandler(w http.ResponseWriter, r *htt
 				return
 			}
 		}
-		key, err = decryptObjectInfo(key, dstBucket, dstObject, mi.UserDefined)
+		key, err = decryptObjectMeta(key, dstBucket, dstObject, mi.UserDefined)
 		if err != nil {
 			writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL)
 			return

cmd/object-multipart-handlers.go

@@ -427,7 +427,7 @@ func (api objectAPIHandlers) PutObjectPartHandler(w http.ResponseWriter, r *http
 		}
 
 		// Calculating object encryption key
-		key, err = decryptObjectInfo(key, bucket, object, mi.UserDefined)
+		key, err = decryptObjectMeta(key, bucket, object, mi.UserDefined)
 		if err != nil {
 			writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL)
 			return
@@ -646,47 +646,6 @@ func (api objectAPIHandlers) CompleteMultipartUploadHandler(w http.ResponseWrite
 	multipartETag := etag.Multipart(completeETags...)
 	opts.UserDefined["etag"] = multipartETag.String()
 
-	// However, in case of encryption, the persisted part ETags don't match
-	// what we have sent to the client during PutObjectPart. The reason is
-	// that ETags are encrypted. Hence, the client will send a list of complete
-	// part ETags of which non can match the ETag of any part. For example
-	//   ETag (client):          30902184f4e62dd8f98f0aaff810c626
-	//   ETag (server-internal): 20000f00ce5dc16e3f3b124f586ae1d88e9caa1c598415c2759bbb50e84a59f630902184f4e62dd8f98f0aaff810c626
-	//
-	// Therefore, we adjust all ETags sent by the client to match what is stored
-	// on the backend.
-	// TODO(klauspost): This should be done while object is finalized instead of fetching the data twice
-	if objectAPI.IsEncryptionSupported() {
-		mi, err := objectAPI.GetMultipartInfo(ctx, bucket, object, uploadID, ObjectOptions{})
-		if err != nil {
-			writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL)
-			return
-		}
-
-		if _, ok := crypto.IsEncrypted(mi.UserDefined); ok {
-			// Only fetch parts in between first and last.
-			// We already checked if we have at least one part.
-			start := complMultipartUpload.Parts[0].PartNumber
-			maxParts := complMultipartUpload.Parts[len(complMultipartUpload.Parts)-1].PartNumber - start + 1
-			listPartsInfo, err := objectAPI.ListObjectParts(ctx, bucket, object, uploadID, start-1, maxParts, ObjectOptions{})
-			if err != nil {
-				writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL)
-				return
-			}
-			sort.Slice(listPartsInfo.Parts, func(i, j int) bool {
-				return listPartsInfo.Parts[i].PartNumber < listPartsInfo.Parts[j].PartNumber
-			})
-			for i := range listPartsInfo.Parts {
-				for j := range complMultipartUpload.Parts {
-					if listPartsInfo.Parts[i].PartNumber == complMultipartUpload.Parts[j].PartNumber {
-						complMultipartUpload.Parts[j].ETag = listPartsInfo.Parts[i].ETag
-						continue
-					}
-				}
-			}
-		}
-	}
-
 	w = &whiteSpaceWriter{ResponseWriter: w, Flusher: w.(http.Flusher)}
 	completeDoneCh := sendWhiteSpace(ctx, w)
 	objInfo, err := completeMultiPartUpload(ctx, bucket, object, uploadID, complMultipartUpload.Parts, opts)
@@ -854,7 +813,7 @@ func (api objectAPIHandlers) ListObjectPartsHandler(w http.ResponseWriter, r *ht
 	if kind, ok := crypto.IsEncrypted(listPartsInfo.UserDefined); ok && objectAPI.IsEncryptionSupported() {
 		var objectEncryptionKey []byte
 		if kind == crypto.S3 {
-			objectEncryptionKey, err = decryptObjectInfo(nil, bucket, object, listPartsInfo.UserDefined)
+			objectEncryptionKey, err = decryptObjectMeta(nil, bucket, object, listPartsInfo.UserDefined)
 			if err != nil {
 				writeErrorResponse(ctx, w, toAPIError(ctx, err), r.URL)
 				return