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

@@ -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)

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

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)

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)
+}

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