serve s3: make the ETag returned on upload agree with HEAD for any --etag-hash

With an --etag-hash other than MD5, including "" to turn hashing off,
PutObject returned the MD5 of the data and CompleteMultipartUpload an
S3 style multipart ETag, neither of which matched the ETag HeadObject
and listings returned for the object afterwards.

These now return the ETag HeadObject does. Listings now quote an empty
ETag as HeadObject does.

See #10082

cmd/serve/s3/backend.go

@@ -21,6 +21,7 @@ import (
 	"github.com/ncw/swift/v2"
 	"github.com/rclone/gofakes3"
 	"github.com/rclone/rclone/fs"
+	fshash "github.com/rclone/rclone/fs/hash"
 	"github.com/rclone/rclone/fs/operations"
 	"github.com/rclone/rclone/vfs"
 	"github.com/rclone/rclone/vfs/vfscommon"
@@ -560,6 +561,9 @@ func (b *s3Backend) PutObject(
 		if err != nil {
 			return result, err
 		}
+		if b.s.etagHash(_vfs) != fshash.MD5 {
+			result.ETag = quoteETag(emptyHash(b.s.etagHash(_vfs)))
+		}
 		return result, putDirMarker(_vfs, fp, objectName, input)
 	}
 	fp, err := bucketObjectPath(bucketName, objectName)
@@ -639,12 +643,26 @@ func (b *s3Backend) PutObject(
 		}
 	}
 
-	_, err = _vfs.Stat(fp)
+	node, err := _vfs.Stat(fp)
 	if err != nil {
 		return result, err
 	}
 
-	return result, b.tenant(ctx).storeMeta(_vfs, fp, meta)
+	if err := b.tenant(ctx).storeMeta(_vfs, fp, meta); err != nil {
+		return result, err
+	}
+
+	// gofakes3 returns the MD5 of the data as the ETag, so another
+	// hash must be given to match what HeadObject returns.
+	if b.s.etagHash(_vfs) != fshash.MD5 {
+		result.ETag = quoteETag(b.objectHash(ctx, _vfs, fp, node))
+	}
+	return result, nil
+}
+
+// quoteETag returns hash as an ETag.
+func quoteETag(hash []byte) string {
+	return `"` + hex.EncodeToString(hash) + `"`
 }
 
 // commitObject moves the object written to tmpFp into place at fp,
@@ -842,7 +860,7 @@ func (b *s3Backend) CopyObject(ctx context.Context, srcBucket, srcKey, dstBucket
 	}
 
 	return gofakes3.CopyObjectResult{
-		ETag:         `"` + hex.EncodeToString(c.Hash) + `"`,
+		ETag:         quoteETag(c.Hash),
 		LastModified: gofakes3.NewContentTime(cStat.ModTime()),
 	}, nil
 }

cmd/serve/s3/list.go

@@ -119,14 +119,10 @@ func (l *lister) addObject(key string, entry vfs.Node) error {
 	if l.keys >= l.max {
 		return l.truncate()
 	}
-	etag := getFileHash(entry, l.hashType)
-	if etag != "" {
-		etag = `"` + etag + `"`
-	}
 	l.response.Add(&gofakes3.Content{
 		Key:          key,
 		LastModified: gofakes3.NewContentTime(entry.ModTime()),
-		ETag:         etag,
+		ETag:         quoteETag(getFileHashByte(entry, l.hashType)),
 		Size:         entry.Size(),
 		StorageClass: gofakes3.StorageStandard,
 	})

cmd/serve/s3/multipart.go

@@ -27,7 +27,6 @@ import (
 	"io"
 	"math"
 	"path"
-	"sort"
 	"strings"
 	"sync"
 	"time"
@@ -926,23 +925,27 @@ func (b *s3Backend) CompleteMultipartUpload(ctx context.Context, bucketName, obj
 	}
 	b.deleteUpload(uploadID, up)
 
-	// The ETag is the MD5 of the object when that is the hash used
-	// for ETags, so it is the same as HEAD returns for it, otherwise
-	// the S3 multipart ETag. It is stored, with the part sizes, so
-	// HEAD returns it while the object is unchanged.
-	etag := up.multipartETag(input)
+	// The ETag is the one HEAD returns for the object. With MD5
+	// ETags the MD5 of the object, which the backend may not have, is
+	// stored with the part sizes so HEAD returns it while the object
+	// is unchanged.
 	var etagHash []byte
 	if b.s.etagHash(up.vfs) == fshash.MD5 {
 		etagHash = up.objectMD5.Sum(nil)
-		etag = fmt.Sprintf("%q", hex.EncodeToString(etagHash))
 	}
 	_ = up.tenant.storeObjectMeta(up.vfs, up.fp, objectMeta{
 		meta:      up.meta,
 		hash:      etagHash,
 		partSizes: up.listedPartSizes(input),
 	})
+	if etagHash == nil {
+		// The upload is complete, so isn't failed if the object can't be read
+		if node, err := up.vfs.Stat(up.fp); err == nil {
+			etagHash = b.objectHash(ctx, up.vfs, up.fp, node)
+		}
+	}
 
-	return "", etag, nil
+	return "", quoteETag(etagHash), nil
 }
 
 // listedPartSizes returns the sizes of the parts listed in input, in
@@ -1210,24 +1213,3 @@ func (up *multipartUpload) abort() error {
 	}
 	return fh.Close()
 }
-
-// multipartETag computes the S3 multipart ETag for the assembled object:
-//
-//	hex(md5(concat(part_md5s_in_order))) + "-" + N
-func (up *multipartUpload) multipartETag(input *gofakes3.CompleteMultipartUploadRequest) string {
-	partNumbers := make([]int, 0, len(input.Parts))
-	for _, p := range input.Parts {
-		partNumbers = append(partNumbers, p.PartNumber)
-	}
-	sort.Ints(partNumbers)
-
-	up.mu.Lock()
-	concat := make([]byte, 0, len(partNumbers)*md5.Size)
-	for _, n := range partNumbers {
-		concat = append(concat, up.partMD5s[n]...)
-	}
-	up.mu.Unlock()
-
-	sum := md5.Sum(concat)
-	return fmt.Sprintf("%q", fmt.Sprintf("%s-%d", hex.EncodeToString(sum[:]), len(partNumbers)))
-}

cmd/serve/s3/multipart_test.go

@@ -6,6 +6,7 @@ import (
 	"bytes"
 	"context"
 	"crypto/md5"
+	"crypto/sha1"
 	"encoding/base64"
 	"encoding/hex"
 	"fmt"
@@ -13,6 +14,7 @@ import (
 	"math"
 	"net/url"
 	"path"
+	"strings"
 	"sync"
 	"sync/atomic"
 	"testing"
@@ -24,6 +26,7 @@ import (
 	_ "github.com/rclone/rclone/backend/memory"
 	"github.com/rclone/rclone/cmd/serve/proxy"
 	"github.com/rclone/rclone/fs"
+	"github.com/rclone/rclone/fs/hash"
 	"github.com/rclone/rclone/fs/object"
 	"github.com/rclone/rclone/fs/operations"
 	"github.com/rclone/rclone/fstest"
@@ -1956,3 +1959,84 @@ func TestMultipartETagAndParts(t *testing.T) {
 		})
 	}
 }
+
+// TestETagHashConsistent checks the ETags returned when objects are
+// made are the same as HEAD and listings return for them, whatever
+// --etag-hash is.
+func TestETagHashConsistent(t *testing.T) {
+	for _, etagHash := range []string{"", "SHA1"} {
+		for _, tc := range testRemotes {
+			t.Run(fmt.Sprintf("%s/%q", tc.name, etagHash), func(t *testing.T) {
+				// A directory marker on a memory remote is a virtual
+				// directory, which stops the VFS shared by the servers
+				// of plain ":memory:" seeing new buckets, so this needs
+				// its own.
+				backing := tc.backing
+				if backing == ":memory:" {
+					backing = fmt.Sprintf(":memory,description=etag%d:", testBackingCounter.Add(1))
+				}
+				core, f, bucket := newMultipartTestServerOpt(t, backing, false, func(opt *Options) {
+					opt.EtagHash = etagHash
+				})
+				ctx := context.Background()
+				sha1Hex := func(data []byte) string {
+					if etagHash == "" {
+						return ""
+					}
+					sum := sha1.Sum(data)
+					return hex.EncodeToString(sum[:])
+				}
+				// The backend gives the hashes of objects, but not of
+				// directory markers
+				wantETag := func(data []byte) string {
+					if !f.Hashes().Contains(hash.SHA1) {
+						return ""
+					}
+					return sha1Hex(data)
+				}
+				etags := map[string]string{}
+
+				data := []byte("put object")
+				info, err := core.PutObject(ctx, bucket, "put.txt", bytes.NewReader(data), int64(len(data)), "", "", minio.PutObjectOptions{})
+				require.NoError(t, err)
+				assert.Equal(t, wantETag(data), info.ETag, "put")
+				etags["put.txt"] = info.ETag
+
+				info, err = core.PutObject(ctx, bucket, "dir/", bytes.NewReader(nil), 0, "", "", minio.PutObjectOptions{})
+				require.NoError(t, err)
+				assert.Equal(t, sha1Hex(nil), info.ETag, "directory marker")
+				etags["dir/"] = info.ETag
+
+				uploadID, err := core.NewMultipartUpload(ctx, bucket, "multipart.bin", minio.PutObjectOptions{})
+				require.NoError(t, err)
+				var all []byte
+				var parts []minio.CompletePart
+				for i, size := range []int{100 * 1024, 30 * 1024} {
+					data := []byte(random.String(size))
+					all = append(all, data...)
+					p, err := core.PutObjectPart(ctx, bucket, "multipart.bin", uploadID, i+1, bytes.NewReader(data), int64(size), minio.PutObjectPartOptions{})
+					require.NoError(t, err)
+					parts = append(parts, minio.CompletePart{PartNumber: i + 1, ETag: p.ETag})
+				}
+				complete, err := core.CompleteMultipartUpload(ctx, bucket, "multipart.bin", uploadID, parts, minio.PutObjectOptions{})
+				require.NoError(t, err)
+				assert.Equal(t, wantETag(all), complete.ETag, "multipart")
+				etags["multipart.bin"] = complete.ETag
+
+				for key, etag := range etags {
+					stat, err := core.StatObject(ctx, bucket, key, minio.StatObjectOptions{})
+					require.NoError(t, err, key)
+					assert.Equal(t, etag, stat.ETag, "HEAD %s", key)
+				}
+				listed := map[string]string{}
+				result, err := core.ListObjects(bucket, "", "", "", 1000)
+				require.NoError(t, err)
+				for _, obj := range result.Contents {
+					listed[obj.Key] = strings.Trim(obj.ETag, `"`)
+				}
+				delete(etags, "dir/")
+				assert.Equal(t, etags, listed)
+			})
+		}
+	}
+}

cmd/serve/s3/serve_s3.md

@@ -65,11 +65,11 @@ cause problems for S3 clients which rely on the Etag being the MD5.
 Use `--etag-hash auto` to use the best hash the backend supports - with
 `--auth-proxy` that of each user's backend.
 
-With `--etag-hash MD5`, the `ETag` of an object made by a multipart
-upload is the MD5 of the whole object, like any other object, rather
-than the `-N` suffixed `ETag` AWS S3 gives these. With another hash
-it is the AWS S3 style multipart `ETag`, but `HeadObject` only
-returns that while the object's metadata is kept (see below).
+The `ETag` of an object made by a multipart upload is its hash, like
+any other object, rather than the `-N` suffixed `ETag` AWS S3 gives
+these. If the remote doesn't have the MD5 of objects, `HeadObject`
+only returns the MD5 of these while the object's metadata is kept (see
+below).
 
 ### Quickstart