From cbfd024bbe2beb79e9a79c31cc673dc652b66272 Mon Sep 17 00:00:00 2001 From: Nick Craig-Wood Date: Thu, 15 Oct 2026 17:07:35 +0100 Subject: [PATCH] serve s3: keep the MD5 ETag of objects on remotes without MD5 With --etag-hash MD5, the default, on a remote which can't give the MD5 of objects, such as crypt or sftp, PutObject returned the MD5 of the data as the ETag but HeadObject and listings returned "". Listings also ignored the MD5 kept for multipart uploads, and copying an object onto itself to replace its metadata lost it, along with the sizes of its parts. The MD5 of a PutObject is now kept with the object's metadata, as for multipart uploads, listings return it, and replacing the metadata of an object keeps it. See #10082 --- cmd/serve/s3/backend.go | 26 ++++++++++++-- cmd/serve/s3/list.go | 15 +++++--- cmd/serve/s3/list_test.go | 2 +- cmd/serve/s3/multipart_test.go | 64 ++++++++++++++++++++++++++++++++++ cmd/serve/s3/serve_s3.md | 4 +-- 5 files changed, 100 insertions(+), 11 deletions(-) diff --git a/cmd/serve/s3/backend.go b/cmd/serve/s3/backend.go index 9fc9a0fc1..81605ce28 100644 --- a/cmd/serve/s3/backend.go +++ b/cmd/serve/s3/backend.go @@ -3,8 +3,10 @@ package s3 import ( "context" + "crypto/md5" "encoding/hex" "errors" + "hash" "io" "maps" "math" @@ -289,7 +291,7 @@ func (b *s3Backend) ListBucket(ctx context.Context, bucket string, prefix *gofak return response, nil } - err = b.listPage(_vfs, bucket, prefix.Prefix, prefix.Delimiter, page, response) + err = b.listPage(ctx, _vfs, bucket, prefix.Prefix, prefix.Delimiter, page, response) if err == gofakes3.ErrNoSuchKey { // AWS just returns an empty list response = gofakes3.NewObjectList() @@ -613,6 +615,15 @@ func (b *s3Backend) PutObject( removeMadeDirs(objectDir, madeDir, _vfs) } + // With MD5 ETags gofakes3 returns the MD5 of the data, which is + // kept with the metadata if the remote can't give it, so HEAD + // returns it too. + var md5Hasher hash.Hash + if b.s.etagHash(_vfs) == fshash.MD5 && !fsys.Hashes().Contains(fshash.MD5) { + md5Hasher = md5.New() + input = io.TeeReader(input, md5Hasher) + } + f, err := _vfs.Create(tmpFp) if err != nil { cleanup() @@ -655,7 +666,11 @@ func (b *s3Backend) PutObject( return result, err } - if err := b.tenant(ctx).storeMeta(_vfs, fp, meta); err != nil { + om := objectMeta{meta: meta} + if md5Hasher != nil { + om.hash = md5Hasher.Sum(nil) + } + if err := b.tenant(ctx).storeObjectMeta(_vfs, fp, om); err != nil { return result, err } @@ -847,7 +862,12 @@ func (b *s3Backend) CopyObject(ctx context.Context, srcBucket, srcKey, dstBucket // Directory markers have no metadata to replace return result, nil } - return result, b.tenant(ctx).storeMeta(_vfs, fp, meta) + // The data is unchanged, so its stored ETag and part sizes + // still apply + t := b.tenant(ctx) + om, _ := t.getMeta(ctx, fp, cStat) + om.meta = meta + return result, t.storeObjectMeta(_vfs, fp, om) } c, err := b.GetObject(ctx, srcBucket, srcKey, nil) diff --git a/cmd/serve/s3/list.go b/cmd/serve/s3/list.go index 0bc3ba7ba..1fc393f5a 100644 --- a/cmd/serve/s3/list.go +++ b/cmd/serve/s3/list.go @@ -1,6 +1,7 @@ package s3 import ( + "context" "errors" "path" "slices" @@ -29,7 +30,8 @@ var errPageFull = errors.New("listing page full") // the size of the subtree below the prefix. type lister struct { vfs *vfs.VFS - hashType hash.Type // hash used for ETags + hashType hash.Type // hash used for ETags + etag func(fp string, entry vfs.Node) []byte // returns the ETag of the object at fp bucket string marker string // keys at or before this one have already been returned hasMarker bool @@ -49,7 +51,7 @@ type lister struct { // newLister makes a lister which fills response with at most one page of the // listing of bucket, as described by page. -func newLister(b *s3Backend, _vfs *vfs.VFS, bucket string, page gofakes3.ListBucketPage, response *gofakes3.ObjectList) *lister { +func newLister(ctx context.Context, b *s3Backend, _vfs *vfs.VFS, bucket string, page gofakes3.ListBucketPage, response *gofakes3.ObjectList) *lister { max := int(page.MaxKeys) if max <= 0 { // A missing or zero max-keys means the client didn't ask for a @@ -64,6 +66,9 @@ func newLister(b *s3Backend, _vfs *vfs.VFS, bucket string, page gofakes3.ListBuc hasMarker: page.HasMarker, max: max, response: response, + etag: func(fp string, entry vfs.Node) []byte { + return b.objectHash(ctx, _vfs, fp, entry) + }, } } @@ -122,7 +127,7 @@ func (l *lister) addObject(key string, entry vfs.Node) error { l.response.Add(&gofakes3.Content{ Key: key, LastModified: gofakes3.NewContentTime(entry.ModTime()), - ETag: quoteETag(getFileHashByte(entry, l.hashType)), + ETag: quoteETag(l.etag(path.Join(l.bucket, key), entry)), Size: entry.Size(), StorageClass: gofakes3.StorageStandard, }) @@ -325,8 +330,8 @@ func (l *lister) list(fdPath, name string, addPrefix bool) error { // // It returns gofakes3.ErrNoSuchKey if the directory the prefix is in // doesn't exist. -func (b *s3Backend) listPage(_vfs *vfs.VFS, bucket, prefix, delimiter string, page gofakes3.ListBucketPage, response *gofakes3.ObjectList) error { - l := newLister(b, _vfs, bucket, page, response) +func (b *s3Backend) listPage(ctx context.Context, _vfs *vfs.VFS, bucket, prefix, delimiter string, page gofakes3.ListBucketPage, response *gofakes3.ObjectList) error { + l := newLister(ctx, b, _vfs, bucket, page, response) // The prefix is split into the directory to list and the start of // the names in it to list. fdPath, name := "", prefix diff --git a/cmd/serve/s3/list_test.go b/cmd/serve/s3/list_test.go index 7a08e20f1..47d9711d1 100644 --- a/cmd/serve/s3/list_test.go +++ b/cmd/serve/s3/list_test.go @@ -170,7 +170,7 @@ func countDirs(t *testing.T, b *s3Backend, page gofakes3.ListBucketPage) (dirsRe _vfs, err := b.s.getVFS(context.Background()) require.NoError(t, err) list = gofakes3.NewObjectList() - l := newLister(b, _vfs, "bucket", page, list) + l := newLister(context.Background(), b, _vfs, "bucket", page, list) err = l.list("", "", false) if err != nil && !errors.Is(err, errPageFull) { require.NoError(t, err) diff --git a/cmd/serve/s3/multipart_test.go b/cmd/serve/s3/multipart_test.go index 8fc710b1c..b1f0626ac 100644 --- a/cmd/serve/s3/multipart_test.go +++ b/cmd/serve/s3/multipart_test.go @@ -26,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/config/obscure" "github.com/rclone/rclone/fs/hash" "github.com/rclone/rclone/fs/object" "github.com/rclone/rclone/fs/operations" @@ -2039,3 +2040,66 @@ func TestETagHashConsistent(t *testing.T) { } } } + +// TestETagMD5WithoutRemoteMD5 checks that with MD5 ETags on a remote +// which can't give the MD5 of objects, the ETags returned when objects +// are made are the same as HEAD and listings return for them, and are +// kept when an object is copied onto itself. +func TestETagMD5WithoutRemoteMD5(t *testing.T) { + backing := ":crypt,remote='" + t.TempDir() + "',password=" + obscure.MustObscure("password") + ":" + core, f, bucket := newMultipartTestServerBacking(t, backing, false) + require.False(t, f.Hashes().Contains(hash.MD5)) + ctx := context.Background() + md5Hex := func(data []byte) string { + sum := md5.Sum(data) + return hex.EncodeToString(sum[:]) + } + 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, md5Hex(data), info.ETag, "put") + etags["put.txt"] = 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, md5Hex(all), complete.ETag, "multipart") + etags["multipart.bin"] = complete.ETag + + // Replacing the metadata of an object leaves its data and so its + // ETag unchanged + _, err = core.CopyObject(ctx, bucket, "multipart.bin", bucket, "multipart.bin", map[string]string{ + "x-amz-metadata-directive": "REPLACE", + "x-amz-meta-potato": "jersey royal", + }, minio.CopySrcOptions{}, minio.PutObjectOptions{}) + require.NoError(t, err) + + 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) + } + stat, err := core.StatObject(ctx, bucket, "multipart.bin", minio.StatObjectOptions{PartNumber: 2}) + require.NoError(t, err) + assert.Equal(t, int64(30*1024), stat.Size, "part 2 size after copy") + + 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, `"`) + } + assert.Equal(t, etags, listed) +} diff --git a/cmd/serve/s3/serve_s3.md b/cmd/serve/s3/serve_s3.md index 201615351..e486d4b3f 100644 --- a/cmd/serve/s3/serve_s3.md +++ b/cmd/serve/s3/serve_s3.md @@ -68,8 +68,8 @@ Use `--etag-hash auto` to use the best hash the backend supports - with 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). +and listings only return the MD5 of objects uploaded through `serve +s3` while the object's metadata is kept (see below). ### Quickstart