s3: honor If-Match on DELETE

adapter/s3.go

@@ -1175,18 +1175,12 @@ func (s *S3Server) deleteObject(w http.ResponseWriter, r *http.Request, bucket s
 		}
 		if !found {
 			cleanupManifest = nil
-			return nil
+			return validateS3AbsentObjectDelete(r, bucket, objectKey)
 		}
-		dispatchCtx := readTimestamp.WithDispatchVoucher(r.Context())
-		_, err = kv.DispatchWithReadTimestamp(dispatchCtx, s.coordinator, &kv.OperationGroup[kv.OP]{
-			IsTxn:   true,
-			StartTS: startTS,
-			Elems: []*kv.Elem[kv.OP]{
-				{Op: kv.Del, Key: headKey},
-			},
-		})
-		if err != nil {
-			return errors.WithStack(err)
+		if err := s.dispatchConditionalObjectDelete(
+			r, readTimestamp, startTS, headKey, manifest, bucket, objectKey,
+		); err != nil {
+			return err
 		}
 		cleanupManifest = manifest
 		generation = meta.Generation
@@ -1202,6 +1196,14 @@ func (s *S3Server) deleteObject(w http.ResponseWriter, r *http.Request, bucket s
 	w.WriteHeader(http.StatusNoContent)
 }
 
+func validateS3AbsentObjectDelete(r *http.Request, bucket, objectKey string) error {
+	if _, conditional := r.Header[http.CanonicalHeaderKey("If-Match")]; conditional {
+		return newS3ResponseError(http.StatusNotFound, "NoSuchKey",
+			"object not found", bucket, objectKey)
+	}
+	return nil
+}
+
 func (s *S3Server) createMultipartUpload(w http.ResponseWriter, r *http.Request, bucket string, objectKey string) {
 	readTS := s.readTS()
 	readTimestamp, err := s.beginTxnReadTimestamp(r.Context(), readTS, "s3 create multipart upload: begin read timestamp")
@@ -2661,6 +2663,89 @@ func validateS3PutPreconditions(ifMatch, ifNoneMatch string, previous *s3ObjectM
 	return nil
 }
 
+// dispatchConditionalObjectDelete evaluates If-Match against the manifest the
+// surrounding transaction already loaded, then removes the head key.
+//
+// DELETE honours If-Match, like PUT does via validateS3PutPreconditions.
+// Accepting the header and deleting anyway is worse than not supporting it: a
+// caller that reads an object, decides it is reclaimable, and sends a
+// conditional delete believes the condition protects it against a concurrent
+// rewrite. It did not -- the object was removed whatever its current ETag -- so
+// the rewrite was silently lost.
+func (s *S3Server) dispatchConditionalObjectDelete(
+	r *http.Request,
+	readTimestamp kv.ReadTimestamp,
+	startTS uint64,
+	headKey []byte,
+	manifest *s3ObjectManifest,
+	bucket, objectKey string,
+) error {
+	if err := validateS3DeletePreconditions(r, manifest); err != nil {
+		return newS3ResponseError(http.StatusPreconditionFailed, "PreconditionFailed",
+			err.Error(), bucket, objectKey)
+	}
+	dispatchCtx := readTimestamp.WithDispatchVoucher(r.Context())
+	if _, err := kv.DispatchWithReadTimestamp(dispatchCtx, s.coordinator, &kv.OperationGroup[kv.OP]{
+		IsTxn:   true,
+		StartTS: startTS,
+		Elems: []*kv.Elem[kv.OP]{
+			{Op: kv.Del, Key: headKey},
+		},
+	}); err != nil {
+		return errors.WithStack(err)
+	}
+	return nil
+}
+
+// validateS3DeletePreconditions enforces If-Match on DELETE.
+//
+// Separate from validateS3PutPreconditions because the two headers mean
+// different things on a delete: If-Match is a guard against removing a
+// version the caller has not seen, while If-None-Match: * ("only if
+// absent") is meaningless for an operation whose whole purpose is to
+// remove something that exists, so it is not honoured here.
+//
+// previous is the manifest the surrounding transaction already loaded at
+// its read timestamp, so the comparison is against the same version the
+// delete will remove -- not a separately-read one that could have moved
+// in between.
+func validateS3DeletePreconditions(r *http.Request, previous *s3ObjectManifest) error {
+	if r == nil {
+		return nil
+	}
+	values, present := r.Header[http.CanonicalHeaderKey("If-Match")]
+	if !present {
+		return nil
+	}
+	if previous == nil {
+		return errors.New("etag precondition failed")
+	}
+	for _, value := range values {
+		// S3 ETags contain no commas; multiple header lines and HTTP's
+		// comma-separated validator list have the same any-match rule.
+		for _, candidate := range strings.Split(value, ",") {
+			if s3DeleteIfMatchCandidateMatches(candidate, previous.ETag) {
+				return nil
+			}
+		}
+	}
+	return errors.New("etag precondition failed")
+}
+
+func s3DeleteIfMatchCandidateMatches(candidate, etag string) bool {
+	candidate = strings.TrimSpace(candidate)
+	if candidate == "*" {
+		return true
+	}
+	if strings.HasPrefix(candidate, "W/") {
+		return false // If-Match requires a strong comparison.
+	}
+	if strings.HasPrefix(candidate, `"`) && strings.HasSuffix(candidate, `"`) && len(candidate) >= 2 {
+		candidate = candidate[1 : len(candidate)-1]
+	}
+	return candidate != "" && candidate == etag
+}
+
 func cloneLeaderAddrMap(src map[string]string) map[string]string {
 	if len(src) == 0 {
 		return nil

adapter/s3_conditional_delete_test.go

@@ -0,0 +1,240 @@
+package adapter
+
+import (
+	"context"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+
+	"github.com/bootjp/elastickv/store"
+	"github.com/stretchr/testify/require"
+)
+
+// Every case here uses one bucket and one object key; only the ETag and the
+// presence of the header vary.
+const (
+	conditionalDeleteBucket = "retention"
+	conditionalDeleteKey    = "payload.bin"
+)
+
+// putS3TestObject stores body at the test bucket/key through the real handler
+// and returns the ETag the server assigned.
+func putS3TestObject(t *testing.T, server *S3Server, body string) string {
+	t.Helper()
+
+	rec := httptest.NewRecorder()
+	req := newS3TestRequest(http.MethodPut,
+		"/"+conditionalDeleteBucket+"/"+conditionalDeleteKey, strings.NewReader(body))
+	req.Header.Set("Content-Type", "text/plain")
+	server.handle(rec, req)
+	require.Equal(t, http.StatusOK, rec.Code, "PUT %s/%s", conditionalDeleteBucket, conditionalDeleteKey)
+
+	etag := strings.Trim(rec.Header().Get("ETag"), `"`)
+	require.NotEmpty(t, etag, "the server must assign an ETag")
+	return etag
+}
+
+func newConditionalDeleteS3Server(t *testing.T) *S3Server {
+	t.Helper()
+
+	st := store.NewMVCCStore()
+	server := NewS3Server(nil, "", st, newLocalAdapterCoordinator(st), nil)
+	_, err := server.AdminCreateBucket(context.Background(),
+		fullAdminBucketsPrincipal(), conditionalDeleteBucket, s3AclPrivate)
+	require.NoError(t, err)
+	return server
+}
+
+func deleteS3TestObject(t *testing.T, server *S3Server, key, ifMatch string) *httptest.ResponseRecorder {
+	t.Helper()
+
+	rec := httptest.NewRecorder()
+	req := newS3TestRequest(http.MethodDelete, "/"+conditionalDeleteBucket+"/"+key, nil)
+	if ifMatch != "" {
+		req.Header.Set("If-Match", ifMatch)
+	}
+	server.handle(rec, req)
+	return rec
+}
+
+func s3TestObjectExists(t *testing.T, server *S3Server) bool {
+	t.Helper()
+
+	rec := httptest.NewRecorder()
+	server.handle(rec, newS3TestRequest(http.MethodHead, "/"+conditionalDeleteBucket+"/"+conditionalDeleteKey, nil))
+	return rec.Code == http.StatusOK
+}
+
+// TestS3ConditionalDeleteRefusesAStaleETag is the load-bearing case.
+//
+// The handler loaded the manifest -- so it had the current ETag -- and then
+// dispatched kv.Del without looking at If-Match. A caller that read an object,
+// decided it was reclaimable, and sent a conditional delete believed the
+// condition protected it from a concurrent rewrite. It did not: the object was
+// removed whatever its ETag, so the rewrite was silently lost. Snapshot-offload
+// retention relies on exactly this precondition and maps the 412 to
+// ErrObjectModified.
+func TestS3ConditionalDeleteRefusesAStaleETag(t *testing.T) {
+	t.Parallel()
+
+	server := newConditionalDeleteS3Server(t)
+	staleETag := putS3TestObject(t, server, "first")
+
+	// A concurrent rewrite changes the ETag after the caller observed it.
+	newETag := putS3TestObject(t, server, "rewritten")
+	require.NotEqual(t, staleETag, newETag, "the rewrite must change the ETag")
+
+	rec := deleteS3TestObject(t, server, conditionalDeleteKey, staleETag)
+	require.Equal(t, http.StatusPreconditionFailed, rec.Code,
+		"a delete conditioned on a superseded ETag must fail with 412")
+	require.Contains(t, rec.Body.String(), "PreconditionFailed")
+	require.True(t, s3TestObjectExists(t, server),
+		"the rewritten object must survive a refused conditional delete")
+}
+
+// A matching ETag must still delete, or retention could never reclaim anything.
+func TestS3ConditionalDeleteAcceptsAMatchingETag(t *testing.T) {
+	t.Parallel()
+
+	server := newConditionalDeleteS3Server(t)
+	etag := putS3TestObject(t, server, "first")
+
+	rec := deleteS3TestObject(t, server, conditionalDeleteKey, etag)
+	require.Equal(t, http.StatusNoContent, rec.Code)
+	require.False(t, s3TestObjectExists(t, server))
+}
+
+// Quoted ETags are the wire form AWS uses, so both spellings must match.
+func TestS3ConditionalDeleteAcceptsAQuotedETag(t *testing.T) {
+	t.Parallel()
+
+	server := newConditionalDeleteS3Server(t)
+	etag := putS3TestObject(t, server, "first")
+
+	rec := deleteS3TestObject(t, server, conditionalDeleteKey, `"`+etag+`"`)
+	require.Equal(t, http.StatusNoContent, rec.Code)
+}
+
+func TestS3ConditionalDeleteAcceptsWildcardAndETagList(t *testing.T) {
+	t.Parallel()
+
+	for _, ifMatch := range []string{"*", `"stale", "PLACEHOLDER"`} {
+		t.Run(ifMatch, func(t *testing.T) {
+			t.Parallel()
+			server := newConditionalDeleteS3Server(t)
+			etag := putS3TestObject(t, server, "first")
+			condition := strings.ReplaceAll(ifMatch, "PLACEHOLDER", etag)
+
+			rec := deleteS3TestObject(t, server, conditionalDeleteKey, condition)
+			require.Equal(t, http.StatusNoContent, rec.Code)
+			require.False(t, s3TestObjectExists(t, server))
+		})
+	}
+}
+
+func TestS3ConditionalDeleteRejectsBlankIfMatch(t *testing.T) {
+	t.Parallel()
+
+	server := newConditionalDeleteS3Server(t)
+	putS3TestObject(t, server, "first")
+	rec := httptest.NewRecorder()
+	req := newS3TestRequest(http.MethodDelete, "/"+conditionalDeleteBucket+"/"+conditionalDeleteKey, nil)
+	req.Header.Set("If-Match", "  ")
+	server.handle(rec, req)
+
+	require.Equal(t, http.StatusPreconditionFailed, rec.Code)
+	require.True(t, s3TestObjectExists(t, server))
+}
+
+// An unconditional delete must be unaffected: adding the header check must not
+// change the behaviour of every existing caller that does not send one.
+func TestS3UnconditionalDeleteIsUnchanged(t *testing.T) {
+	t.Parallel()
+
+	server := newConditionalDeleteS3Server(t)
+	putS3TestObject(t, server, "first")
+
+	rec := deleteS3TestObject(t, server, conditionalDeleteKey, "")
+	require.Equal(t, http.StatusNoContent, rec.Code)
+	require.False(t, s3TestObjectExists(t, server))
+}
+
+// An unconditional delete remains idempotent, while a conditional delete
+// reports that the object whose ETag was to be checked is missing.
+func TestS3ConditionalDeleteOnAnAbsentKey(t *testing.T) {
+	t.Parallel()
+
+	server := newConditionalDeleteS3Server(t)
+
+	rec := deleteS3TestObject(t, server, "never-existed", "")
+	require.Equal(t, http.StatusNoContent, rec.Code)
+
+	rec = deleteS3TestObject(t, server, "never-existed", "deadbeef")
+	require.Equal(t, http.StatusNotFound, rec.Code)
+	require.Contains(t, rec.Body.String(), "NoSuchKey")
+
+	rec = deleteS3TestObject(t, server, "never-existed", "*")
+	require.Equal(t, http.StatusNotFound, rec.Code)
+	require.Contains(t, rec.Body.String(), "NoSuchKey")
+}
+
+// TestValidateS3DeletePreconditions covers the predicate directly, including
+// that If-None-Match is deliberately NOT honoured on a delete.
+func TestValidateS3DeletePreconditions(t *testing.T) {
+	t.Parallel()
+
+	previous := &s3ObjectManifest{ETag: "abc123"}
+
+	t.Run("no header passes", func(t *testing.T) {
+		t.Parallel()
+		req := newS3TestRequest(http.MethodDelete, "/b/k", nil)
+		require.NoError(t, validateS3DeletePreconditions(req, previous))
+	})
+
+	t.Run("matching etag passes", func(t *testing.T) {
+		t.Parallel()
+		req := newS3TestRequest(http.MethodDelete, "/b/k", nil)
+		req.Header.Set("If-Match", "abc123")
+		require.NoError(t, validateS3DeletePreconditions(req, previous))
+	})
+
+	t.Run("stale etag fails", func(t *testing.T) {
+		t.Parallel()
+		req := newS3TestRequest(http.MethodDelete, "/b/k", nil)
+		req.Header.Set("If-Match", "stale")
+		require.Error(t, validateS3DeletePreconditions(req, previous))
+	})
+
+	t.Run("multiple header lines match any strong etag", func(t *testing.T) {
+		t.Parallel()
+		req := newS3TestRequest(http.MethodDelete, "/b/k", nil)
+		req.Header.Add("If-Match", `"stale"`)
+		req.Header.Add("If-Match", `"abc123"`)
+		require.NoError(t, validateS3DeletePreconditions(req, previous))
+	})
+
+	t.Run("weak etag does not match", func(t *testing.T) {
+		t.Parallel()
+		req := newS3TestRequest(http.MethodDelete, "/b/k", nil)
+		req.Header.Set("If-Match", `W/"abc123"`)
+		require.Error(t, validateS3DeletePreconditions(req, previous))
+	})
+
+	t.Run("If-None-Match is not honoured on delete", func(t *testing.T) {
+		t.Parallel()
+		// "only if absent" is meaningless for an operation whose purpose is
+		// to remove something that exists; honouring it would refuse every
+		// delete of a present object.
+		req := newS3TestRequest(http.MethodDelete, "/b/k", nil)
+		req.Header.Set("If-None-Match", "*")
+		require.NoError(t, validateS3DeletePreconditions(req, previous))
+	})
+
+	t.Run("nil manifest fails with If-Match", func(t *testing.T) {
+		t.Parallel()
+		req := newS3TestRequest(http.MethodDelete, "/b/k", nil)
+		req.Header.Set("If-Match", "abc123")
+		require.Error(t, validateS3DeletePreconditions(req, nil))
+	})
+}