diff --git a/test/s3/versioning/s3_conditional_reads_test.go b/test/s3/versioning/s3_conditional_reads_test.go new file mode 100644 index 000000000..83dab5366 --- /dev/null +++ b/test/s3/versioning/s3_conditional_reads_test.go @@ -0,0 +1,130 @@ +package s3api + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/s3" + "github.com/aws/smithy-go" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func requireAPIErrorCode(t *testing.T, err error, expected string) { + t.Helper() + require.Error(t, err) + var apiErr smithy.APIError + require.True(t, errors.As(err, &apiErr), "expected a smithy.APIError, got %T: %v", err, err) + assert.Equal(t, expected, apiErr.ErrorCode()) +} + +// TestConditionalReadsOfMissingObject verifies that a missing key stays a missing key +// under If-Match and If-Unmodified-Since instead of surfacing as 412. +// reproduces issue #10984 +func TestConditionalReadsOfMissingObject(t *testing.T) { + client := getS3Client(t) + bucketName := getNewBucketName() + + createBucket(t, client, bucketName) + defer deleteBucket(t, client, bucketName) + + existing := putObject(t, client, bucketName, "etag-source", "content") + require.NotNil(t, existing.ETag) + future := aws.Time(time.Now().Add(24 * time.Hour)) + missing := aws.String("conditional-missing") + + t.Run("HeadObject If-Match", func(t *testing.T) { + _, err := client.HeadObject(context.TODO(), &s3.HeadObjectInput{ + Bucket: aws.String(bucketName), Key: missing, IfMatch: existing.ETag, + }) + requireAPIErrorCode(t, err, "NotFound") + }) + + t.Run("HeadObject If-Unmodified-Since", func(t *testing.T) { + _, err := client.HeadObject(context.TODO(), &s3.HeadObjectInput{ + Bucket: aws.String(bucketName), Key: missing, IfUnmodifiedSince: future, + }) + requireAPIErrorCode(t, err, "NotFound") + }) + + t.Run("GetObject If-Match", func(t *testing.T) { + _, err := client.GetObject(context.TODO(), &s3.GetObjectInput{ + Bucket: aws.String(bucketName), Key: missing, IfMatch: existing.ETag, + }) + requireAPIErrorCode(t, err, "NoSuchKey") + }) + + t.Run("GetObject If-Unmodified-Since", func(t *testing.T) { + _, err := client.GetObject(context.TODO(), &s3.GetObjectInput{ + Bucket: aws.String(bucketName), Key: missing, IfUnmodifiedSince: future, + }) + requireAPIErrorCode(t, err, "NoSuchKey") + }) + + t.Run("GetObject stale If-Match on a live object stays 412", func(t *testing.T) { + _, err := client.GetObject(context.TODO(), &s3.GetObjectInput{ + Bucket: aws.String(bucketName), Key: aws.String("etag-source"), + IfMatch: aws.String(`"0000000000000000000000000000dead"`), + }) + requireAPIErrorCode(t, err, "PreconditionFailed") + }) +} + +// TestConditionalReadsOfNamedVersion verifies that a conditional GET or HEAD of an +// explicit versionId is evaluated against that version rather than the latest one, +// including when the latest version is a delete marker. +func TestConditionalReadsOfNamedVersion(t *testing.T) { + client := getS3Client(t) + bucketName := getNewBucketName() + + createBucket(t, client, bucketName) + defer deleteBucket(t, client, bucketName) + enableVersioning(t, client, bucketName) + + key := "conditional-read-version" + v1 := putObject(t, client, bucketName, key, "content-v1") + require.NotNil(t, v1.ETag) + require.NotNil(t, v1.VersionId) + v2 := putObject(t, client, bucketName, key, "content-v2") + require.NotNil(t, v2.ETag) + require.NotEqual(t, *v1.ETag, *v2.ETag) + + t.Run("If-Match matches the named version, not the latest", func(t *testing.T) { + _, err := client.GetObject(context.TODO(), &s3.GetObjectInput{ + Bucket: aws.String(bucketName), Key: aws.String(key), + VersionId: v1.VersionId, IfMatch: v1.ETag, + }) + require.NoError(t, err) + }) + + t.Run("If-Match against the latest ETag fails on the named version", func(t *testing.T) { + _, err := client.GetObject(context.TODO(), &s3.GetObjectInput{ + Bucket: aws.String(bucketName), Key: aws.String(key), + VersionId: v1.VersionId, IfMatch: v2.ETag, + }) + requireAPIErrorCode(t, err, "PreconditionFailed") + }) + + _, err := client.DeleteObject(context.TODO(), &s3.DeleteObjectInput{ + Bucket: aws.String(bucketName), Key: aws.String(key), + }) + require.NoError(t, err) + + t.Run("named version survives a delete marker on the latest", func(t *testing.T) { + _, err := client.HeadObject(context.TODO(), &s3.HeadObjectInput{ + Bucket: aws.String(bucketName), Key: aws.String(key), + VersionId: v1.VersionId, IfMatch: v1.ETag, + }) + require.NoError(t, err) + }) + + t.Run("delete marker latest is a missing object", func(t *testing.T) { + _, err := client.GetObject(context.TODO(), &s3.GetObjectInput{ + Bucket: aws.String(bucketName), Key: aws.String(key), IfMatch: v1.ETag, + }) + requireAPIErrorCode(t, err, "NoSuchKey") + }) +} diff --git a/weed/s3api/filer_multipart.go b/weed/s3api/filer_multipart.go index 38b73111c..4752c904e 100644 --- a/weed/s3api/filer_multipart.go +++ b/weed/s3api/filer_multipart.go @@ -376,7 +376,7 @@ func applyMultipartSSES3HeadersFromUploadEntry(dst *filer_pb.Entry, sses3Info *m } func (s3a *S3ApiServer) prepareMultipartCompletionState(r *http.Request, input *s3.CompleteMultipartUploadInput, uploadDirectory, entryName, dirName string, completedPartNumbers []int, completedPartMap map[int][]string, maxPartNo int) (*multipartCompletionState, *CompleteMultipartUploadResult, s3err.ErrorCode) { - if entry, err := s3a.resolveObjectEntry(*input.Bucket, *input.Key); err == nil && entry != nil && entry.Extended != nil { + if entry, err := s3a.resolveObjectEntry(*input.Bucket, *input.Key, ""); err == nil && entry != nil && entry.Extended != nil { if uploadId, ok := entry.Extended[s3_constants.SeaweedFSUploadId]; ok && *input.UploadId == string(uploadId) { cleanupEntries, _, cleanupErr := s3a.list(uploadDirectory, "", "", false, s3_constants.MaxS3MultipartParts+1) if cleanupErr != nil && !errors.Is(cleanupErr, filer_pb.ErrNotFound) { diff --git a/weed/s3api/s3api_object_handlers.go b/weed/s3api/s3api_object_handlers.go index 0338af5ad..f45f4166c 100644 --- a/weed/s3api/s3api_object_handlers.go +++ b/weed/s3api/s3api_object_handlers.go @@ -423,9 +423,10 @@ func (s3a *S3ApiServer) checkDirectoryObject(bucket, object string) (*filer_pb.E return dirEntry, true, nil } -// resolveObjectEntry resolves the object entry for conditional checks, -// handling versioned buckets by resolving the latest version. -func (s3a *S3ApiServer) resolveObjectEntry(bucket, object string) (*filer_pb.Entry, error) { +// resolveObjectEntry resolves the object entry for conditional checks: the version the +// request names when the bucket is versioned, otherwise the latest version. Callers +// with no version to target pass an empty versionId. +func (s3a *S3ApiServer) resolveObjectEntry(bucket, object, versionId string) (*filer_pb.Entry, error) { // Check if versioning is configured versioningConfigured, err := s3a.isVersioningConfigured(bucket) if err != nil && !errors.Is(err, filer_pb.ErrNotFound) { @@ -434,6 +435,9 @@ func (s3a *S3ApiServer) resolveObjectEntry(bucket, object string) (*filer_pb.Ent } if versioningConfigured { + if versionId != "" { + return s3a.getSpecificObjectVersion(bucket, object, versionId) + } // For versioned buckets, we must use getLatestObjectVersion to correctly // find the latest versioned object (in .versions/) or null version. // Standard getEntry would fail to find objects moved to .versions/. diff --git a/weed/s3api/s3api_object_handlers_conditional_read_test.go b/weed/s3api/s3api_object_handlers_conditional_read_test.go new file mode 100644 index 000000000..7946e1a9a --- /dev/null +++ b/weed/s3api/s3api_object_handlers_conditional_read_test.go @@ -0,0 +1,60 @@ +package s3api + +import ( + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/seaweedfs/seaweedfs/weed/pb/filer_pb" + "github.com/seaweedfs/seaweedfs/weed/s3api/s3_constants" + "github.com/seaweedfs/seaweedfs/weed/s3api/s3err" +) + +// A precondition can only fail against an object that exists. GET/HEAD of a missing +// key must stay a missing-key answer even when If-Match or If-Unmodified-Since is sent. +func TestValidateConditionalHeadersForReadsMissingObject(t *testing.T) { + s3a := &S3ApiServer{} + + existing := &filer_pb.Entry{ + Attributes: &filer_pb.FuseAttributes{Mtime: time.Now().Unix()}, + Extended: map[string][]byte{s3_constants.ExtETagKey: []byte("d41d8cd98f00b204e9800998ecf8427e")}, + } + deleteMarker := &filer_pb.Entry{ + Attributes: &filer_pb.FuseAttributes{Mtime: time.Now().Unix()}, + Extended: map[string][]byte{s3_constants.ExtDeleteMarkerKey: []byte("true")}, + } + future := time.Now().Add(24 * time.Hour).UTC().Format(http.TimeFormat) + + testCases := []struct { + name string + header string + value string + entry *filer_pb.Entry + want s3err.ErrorCode + }{ + {"if-match on missing object", s3_constants.IfMatch, "0000", nil, s3err.ErrNoSuchKey}, + {"if-match star on missing object", s3_constants.IfMatch, "*", nil, s3err.ErrNoSuchKey}, + {"if-unmodified-since on missing object", s3_constants.IfUnmodifiedSince, future, nil, s3err.ErrNoSuchKey}, + {"if-match on delete marker", s3_constants.IfMatch, "0000", deleteMarker, s3err.ErrNoSuchKey}, + {"if-none-match on missing object", s3_constants.IfNoneMatch, "*", nil, s3err.ErrNone}, + {"if-modified-since on missing object", s3_constants.IfModifiedSince, future, nil, s3err.ErrNone}, + {"if-match mismatch on existing object", s3_constants.IfMatch, "0000", existing, s3err.ErrPreconditionFailed}, + {"if-match hit on existing object", s3_constants.IfMatch, "d41d8cd98f00b204e9800998ecf8427e", existing, s3err.ErrNone}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + r := httptest.NewRequest(http.MethodGet, "/bucket/object", nil) + r.Header.Set(tc.header, tc.value) + headers, errCode := parseConditionalHeaders(r) + if errCode != s3err.ErrNone { + t.Fatalf("parseConditionalHeaders: %v", errCode) + } + result := s3a.validateConditionalHeadersForReads(r, headers, tc.entry, "bucket", "object") + if result.ErrorCode != tc.want { + t.Errorf("got %v, want %v", result.ErrorCode, tc.want) + } + }) + } +} diff --git a/weed/s3api/s3api_object_handlers_delete.go b/weed/s3api/s3api_object_handlers_delete.go index 78e7794c7..853c08e9f 100644 --- a/weed/s3api/s3api_object_handlers_delete.go +++ b/weed/s3api/s3api_object_handlers_delete.go @@ -78,7 +78,7 @@ func (s3a *S3ApiServer) resolveDeleteConditionalEntry(bucket, object, versionId, } return normalizeConditionalTargetEntry(entry), nil default: - entry, err := s3a.resolveObjectEntry(bucket, normalizedObject) + entry, err := s3a.resolveObjectEntry(bucket, normalizedObject, "") if err != nil { return nil, err } diff --git a/weed/s3api/s3api_object_handlers_put.go b/weed/s3api/s3api_object_handlers_put.go index 67aead988..2862f1935 100644 --- a/weed/s3api/s3api_object_handlers_put.go +++ b/weed/s3api/s3api_object_handlers_put.go @@ -2277,7 +2277,7 @@ func (s3a *S3ApiServer) checkConditionalHeaders(r *http.Request, bucket, object // Use resolveObjectEntry to correctly handle versioned objects. // This ensures we check conditions against the LATEST version, not a null version. - entry, err := s3a.resolveObjectEntry(bucket, object) + entry, err := s3a.resolveObjectEntry(bucket, object, "") if err != nil { if errors.Is(err, filer_pb.ErrNotFound) || errors.Is(err, ErrDeleteMarker) { entry = nil @@ -2298,18 +2298,14 @@ func (s3a *S3ApiServer) validateConditionalHeadersForReads(r *http.Request, head entry = normalizeConditionalTargetEntry(entry) objectExists := entry != nil - // If object doesn't exist, fail for If-Match and If-Unmodified-Since + // A precondition only fails against an object that exists: AWS keeps GET/HEAD of a + // missing key a missing-key answer, so a condition never turns absence into 412. + // If-None-Match and If-Modified-Since pass here and the handler answers 404 itself. if !objectExists { - if headers.ifMatch != "" { - glog.V(3).Infof("validateConditionalHeadersForReads: If-Match failed - object %s/%s does not exist", bucket, object) - return ConditionalHeaderResult{ErrorCode: s3err.ErrPreconditionFailed, Entry: nil} + if headers.ifMatch != "" || !headers.ifUnmodifiedSince.IsZero() { + glog.V(3).Infof("validateConditionalHeadersForReads: object %s/%s does not exist", bucket, object) + return ConditionalHeaderResult{ErrorCode: s3err.ErrNoSuchKey, Entry: nil} } - if !headers.ifUnmodifiedSince.IsZero() { - glog.V(3).Infof("validateConditionalHeadersForReads: If-Unmodified-Since failed - object %s/%s does not exist", bucket, object) - return ConditionalHeaderResult{ErrorCode: s3err.ErrPreconditionFailed, Entry: nil} - } - // If-None-Match and If-Modified-Since succeed when object doesn't exist - // No entry to return since object doesn't exist return ConditionalHeaderResult{ErrorCode: s3err.ErrNone, Entry: nil} } @@ -2396,9 +2392,10 @@ func (s3a *S3ApiServer) checkConditionalHeadersForReads(r *http.Request, bucket, return ConditionalHeaderResult{ErrorCode: s3err.ErrNone, Entry: nil} } - // Use resolveObjectEntry to correctly handle versioned objects. - // This ensures we check conditions against the LATEST version, not a null version. - entry, err := s3a.resolveObjectEntry(bucket, object) + // Use resolveObjectEntry to correctly handle versioned objects: the version the + // request names, or the LATEST version rather than a null version. + versionId := r.URL.Query().Get("versionId") + entry, err := s3a.resolveObjectEntry(bucket, object, versionId) if err != nil { if errors.Is(err, filer_pb.ErrNotFound) || errors.Is(err, ErrDeleteMarker) { entry = nil @@ -2407,6 +2404,11 @@ func (s3a *S3ApiServer) checkConditionalHeadersForReads(r *http.Request, bucket, return ConditionalHeaderResult{ErrorCode: s3err.ErrInternalError, Entry: nil} } } + // A named version that resolves to nothing is the handler's answer to give: only it + // knows whether the bucket is versioned, and so whether that is NoSuchVersion. + if versionId != "" && normalizeConditionalTargetEntry(entry) == nil { + return ConditionalHeaderResult{ErrorCode: s3err.ErrNone, Entry: nil} + } return s3a.validateConditionalHeadersForReads(r, headers, entry, bucket, object) }