Allow non-ASCII values in S3 PutObject user metadata (#9604)

* Test S3 PutObject with UserMetadata

It verifies both ASCII and non-ASCII values.

* Check metadata uploaded with S3 API is preserved in lakeFS API

This verifies that Unicode actually is treated corrected.

* Strip "X-Amz-Meta-" prefix before testing with lakeFS API

See #9089 for why it is there in the first place.

* Apply RFC 2047 coding to metadata headers

This matches the S3 spec.

* [CR] [bug] Add missing "return" after EncodeError calls

I _also_ asked Claude to look for other missing returns of this fashion; it
found these but no new ones.

Author: arielshaqed

esti/s3_gateway_test.go

@@ -3,6 +3,7 @@ package esti
 import (
 	"bytes"
 	"context"
+	"errors"
 	"fmt"
 	"io"
 	"math/rand"
@@ -20,6 +21,7 @@ import (
 	"github.com/aws/smithy-go/middleware"
 	smithyhttp "github.com/aws/smithy-go/transport/http"
 	"github.com/go-openapi/swag"
+	"github.com/go-test/deep"
 	"github.com/minio/minio-go/v7"
 	"github.com/minio/minio-go/v7/pkg/credentials"
 	"github.com/minio/minio-go/v7/pkg/tags"
@@ -902,6 +904,90 @@ func TestS3CopyObject(t *testing.T) {
 	})
 }
 
+var errMissingPrefix = errors.New("missing prefix")
+
+// stripKeyPrefix strips prefix from all keys of m.  It returns an error along with a stripped
+// map if not all keys have the prefix.
+func stripKeyPrefix[T any](prefix string, m map[string]T) (map[string]T, error) {
+	ret := make(map[string]T, len(m))
+	var err error
+	for key, value := range m {
+		stripped, ok := strings.CutPrefix(key, prefix)
+		if !ok {
+			err = errors.Join(err, fmt.Errorf("%w in %s", errMissingPrefix, key))
+		}
+		ret[stripped] = value
+	}
+	return ret, err
+}
+
+func TestS3PutObjectUserMetadata(t *testing.T) {
+	t.Parallel()
+	ctx, _, repo := setupTest(t)
+	defer tearDownTest(repo)
+
+	const contents = "the quick brown fox jumps over the lazy dog."
+	// metadata are the metadata we want on the object.
+	metadata := map[string]string{
+		"Ascii":     "value",
+		"Non-Ascii": "והארץ היתה תהו ובהו", // "without form and void"
+	}
+	// encodedMetadata are the metadata a conforming client sends, with
+	// values Q-word encoded according to RFC 2047.
+	encodedMetadata := map[string]string{
+		"Ascii":     "value",
+		"Non-Ascii": "=?utf-8?q?=D7=95=D7=94=D7=90=D7=A8=D7=A5_=D7=94=D7=99=D7=AA=D7=94_=D7=AA?= =?utf-8?q?=D7=94=D7=95_=D7=95=D7=91=D7=94=D7=95?=",
+	}
+
+	// Headers are _supposed_ to be RFC 2047 encoded on upload.  But this should actually
+	// _always_ work, "non" is just illegal encoding.
+	for _, encode := range []string{"rfc2047", "none"} {
+		t.Run(encode, func(t *testing.T) {
+			file := fmt.Sprintf("data/file.%s", encode)
+			path := fmt.Sprintf("%sfile.%s", gatewayTestPrefix, encode)
+			s3lakefsClient := newMinioClient(t, credentials.NewStaticV4)
+
+			metadataToSend := metadata
+			if encode == "rfc2047" {
+				metadataToSend = encodedMetadata
+			}
+			_, err := s3lakefsClient.PutObject(ctx, repo, path, strings.NewReader(contents), int64(len(contents)),
+				minio.PutObjectOptions{
+					UserMetadata: metadataToSend,
+				})
+			require.NoError(t, err, "putObject")
+
+			info, err := s3lakefsClient.StatObject(ctx, repo, path, minio.StatObjectOptions{})
+			require.NoError(t, err, "statObject")
+
+			// The AWS golang SDK does _not_ decode RFC 2047 headers.  The check
+			// here is actually a minor bug because the gateway could choose a
+			// different encoding for the same headers.  But of course it uses the
+			// same encoder we used, so this should never happen.
+			if diffs := deep.Equal(info.UserMetadata, minio.StringMap(encodedMetadata)); diffs != nil {
+				t.Errorf("Different user metadata from S3 gateway: %s", diffs)
+			}
+
+			// The lakeFS API does not need to perform header encoding.  Use it to
+			// check that lakeFS sees the expected values.
+			statsResp, err := client.StatObjectWithResponse(ctx, repo, mainBranch, &apigen.StatObjectParams{
+				Path:         file,
+				UserMetadata: aws.Bool(true),
+			})
+			require.NoError(t, err, "Call statObject using lakeFS API")
+			require.NoError(t, VerifyResponse(statsResp.HTTPResponse, statsResp.Body), "statObject using lakeFS API")
+
+			// Because of #9089, any user metadata uploaded through the S3 gateway
+			// has a x-aws-meta- prefix.
+			strippedMetadata, err := stripKeyPrefix("X-Amz-Meta-", statsResp.JSON200.Metadata.AdditionalProperties)
+			assert.NoErrorf(t, err, "Failed to strip prefix from metadata keys in %+v", statsResp.JSON200.Metadata.AdditionalProperties)
+			if diffs := deep.Equal(strippedMetadata, metadata); diffs != nil {
+				t.Errorf("Different user metadata from API: %s", diffs)
+			}
+		})
+	}
+}
+
 func TestS3PutObjectTagging(t *testing.T) {
 	t.Parallel()
 	ctx, _, repo := setupTest(t)

pkg/gateway/errors/errors.go

@@ -69,6 +69,7 @@ const (
 	ErrInvalidMaxUploads
 	ErrInvalidMaxParts
 	ErrInvalidPartNumberMarker
+	ErrInvalidHeaderValue
 	ErrInvalidRequestBody
 	ErrInvalidCopySource
 	ErrInvalidMetadataDirective
@@ -249,6 +250,13 @@ var Codes = errorCodeMap{
 		Description:    "Argument partNumberMarker must be an integer.",
 		HTTPStatusCode: http.StatusBadRequest,
 	},
+	ErrInvalidHeaderValue: {
+		// TODO(ariels): S3 itself encodes the bad header in XML tags ArgumentName,
+		// ArgumentValue.
+		Code:           "InvalidArgument",
+		Description:    "Invalid header value.",
+		HTTPStatusCode: http.StatusBadRequest,
+	},
 	ErrInvalidPolicyDocument: {
 		Code:           "InvalidPolicyDocument",
 		Description:    "The content of the form does not meet the conditions specified in the policy document.",

pkg/gateway/operations/operation_utils.go

@@ -1,6 +1,8 @@
 package operations
 
 import (
+	"errors"
+	"mime"
 	"net/http"
 	"strings"
 	"time"
@@ -12,23 +14,33 @@ import (
 
 const amzMetaHeaderPrefix = "X-Amz-Meta-"
 
+var (
+	rfc2047Decoder = new(mime.WordDecoder)
+)
+
 // amzMetaAsMetadata prepare metadata based on amazon user metadata request headers
-func amzMetaAsMetadata(req *http.Request) catalog.Metadata {
+func amzMetaAsMetadata(req *http.Request) (catalog.Metadata, error) {
 	metadata := make(catalog.Metadata)
+	var err error
 	for k := range req.Header {
 		if strings.HasPrefix(k, amzMetaHeaderPrefix) {
-			metadata[k] = req.Header.Get(k)
+			value, decodeErr := rfc2047Decoder.DecodeHeader(req.Header.Get(k))
+			if decodeErr != nil {
+				err = errors.Join(err, decodeErr)
+				continue
+			}
+			metadata[k] = value
 		}
 	}
-	return metadata
+	return metadata, err
 }
 
 // amzMetaWriteHeaders set amazon user metadata on http response
 func amzMetaWriteHeaders(w http.ResponseWriter, metadata catalog.Metadata) {
 	h := w.Header()
 	for k, v := range metadata {
 		if strings.HasPrefix(k, amzMetaHeaderPrefix) {
-			h.Set(k, v)
+			h.Set(k, mime.QEncoding.Encode("utf-8", v))
 		}
 	}
 }

pkg/gateway/operations/postobject.go

@@ -65,12 +65,18 @@ func (controller *PostObject) HandleCreateMultipartUpload(w http.ResponseWriter,
 		_ = o.EncodeError(w, req, err, gatewayErrors.Codes.ToAPIErr(gatewayErrors.ErrInternalError))
 		return
 	}
+	metadata, err := amzMetaAsMetadata(req)
+	if err != nil {
+		o.Log(req).WithError(err).Error("failed to decode user metadata")
+		_ = o.EncodeError(w, req, err, gatewayErrors.Codes.ToAPIErr(gatewayErrors.ErrInvalidHeaderValue))
+		return
+	}
 	mpu := multipart.Upload{
 		UploadID:        resp.UploadID,
 		Path:            o.Path,
 		CreationDate:    time.Now(),
 		PhysicalAddress: address,
-		Metadata:        map[string]string(amzMetaAsMetadata(req)),
+		Metadata:        map[string]string(metadata),
 		ContentType:     req.Header.Get("Content-Type"),
 	}
 	err = o.MultipartTracker.Create(req.Context(), mpu)

pkg/gateway/operations/putobject.go

@@ -112,7 +112,12 @@ func handleCopy(w http.ResponseWriter, req *http.Request, o *PathOperation, copy
 
 	ctx := req.Context()
 
-	metadata := amzMetaAsMetadata(req)
+	metadata, err := amzMetaAsMetadata(req)
+	if err != nil {
+		o.Log(req).WithError(err).Error("failed to decode user metadata")
+		_ = o.EncodeError(w, req, err, gatewayErrors.Codes.ToAPIErr(gatewayErrors.ErrInvalidHeaderValue))
+		return
+	}
 	replaceMetadata := shouldReplaceMetadata(req)
 
 	entry, err := o.Catalog.CopyEntry(ctx, srcPath.Repo, srcPath.Reference, srcPath.Path, repository, branch, o.Path, replaceMetadata, metadata)
@@ -360,7 +365,12 @@ func handlePut(w http.ResponseWriter, req *http.Request, o *PathOperation) {
 	}
 
 	// write metadata
-	metadata := amzMetaAsMetadata(req)
+	metadata, err := amzMetaAsMetadata(req)
+	if err != nil {
+		o.Log(req).WithError(err).Error("failed to decode user metadata")
+		_ = o.EncodeError(w, req, err, gatewayErrors.Codes.ToAPIErr(gatewayErrors.ErrInvalidHeaderValue))
+		return
+	}
 	contentType := req.Header.Get("Content-Type")
 	err = o.finishUpload(req, &blob.CreationDate, blob.Checksum, blob.PhysicalAddress, blob.Size, true, metadata, contentType, gravelerOpts)
 	if errors.Is(err, graveler.ErrPreconditionFailed) {