From c42e311c1ce48226b3bca18cf1bfcea3c3ce0184 Mon Sep 17 00:00:00 2001 From: Nick Craig-Wood Date: Thu, 15 Oct 2026 15:33:01 +0100 Subject: [PATCH] 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 | 24 ++++++++-- cmd/serve/s3/list.go | 6 +-- cmd/serve/s3/multipart.go | 40 +++++----------- cmd/serve/s3/multipart_test.go | 84 ++++++++++++++++++++++++++++++++++ cmd/serve/s3/serve_s3.md | 10 ++-- 5 files changed, 122 insertions(+), 42 deletions(-) diff --git a/cmd/serve/s3/backend.go b/cmd/serve/s3/backend.go index fa1218e06..0f1bf608e 100644 --- a/cmd/serve/s3/backend.go +++ b/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 } diff --git a/cmd/serve/s3/list.go b/cmd/serve/s3/list.go index f174d0037..b238c2b99 100644 --- a/cmd/serve/s3/list.go +++ b/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, }) diff --git a/cmd/serve/s3/multipart.go b/cmd/serve/s3/multipart.go index 3a356efcd..bb14846b5 100644 --- a/cmd/serve/s3/multipart.go +++ b/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))) -} diff --git a/cmd/serve/s3/multipart_test.go b/cmd/serve/s3/multipart_test.go index 759aedf03..94246f160 100644 --- a/cmd/serve/s3/multipart_test.go +++ b/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) + }) + } + } +} diff --git a/cmd/serve/s3/serve_s3.md b/cmd/serve/s3/serve_s3.md index 6a2fcfe6c..037a3b244 100644 --- a/cmd/serve/s3/serve_s3.md +++ b/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