From 47959f5ca659cccb5616cc768478b0905317bee1 Mon Sep 17 00:00:00 2001 From: bootjp Date: Sat, 12 Sep 2026 14:52:17 +0900 Subject: [PATCH] s3: honor If-Match on DELETE The deleteObject handler loaded the object manifest -- so it had the current ETag -- and then dispatched kv.Del without looking at If-Match. PUT already enforces the header via validateS3PutPreconditions; DELETE accepted it and deleted anyway. Accepting a precondition and ignoring it 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 from a concurrent rewrite. It did not: the object was removed whatever its current ETag, so the rewrite was silently lost. snapshotoffload retention depends on exactly this -- DeleteObjectIfUnmodified sends If-Match and maps the 412 to ErrObjectModified -- so against the bundled endpoint its precondition was inert and it could reclaim a payload that had been republished after it was observed. The check runs against the manifest the surrounding transaction already loaded at its read timestamp, so the comparison is against the same version the delete will remove rather than a separately-read one that could have moved in between. If-None-Match is deliberately not honoured on DELETE: "only if absent" is meaningless for an operation whose purpose is to remove something that exists, and honouring it would refuse every delete of a present object. Claude-Session: https://claude.ai/code/session_013rNHooj7NF3giihWVba8QE --- adapter/s3.go | 74 ++++++++-- adapter/s3_conditional_delete_test.go | 190 ++++++++++++++++++++++++++ 2 files changed, 254 insertions(+), 10 deletions(-) create mode 100644 adapter/s3_conditional_delete_test.go diff --git a/adapter/s3.go b/adapter/s3.go index 47524d753..b28300c54 100644 --- a/adapter/s3.go +++ b/adapter/s3.go @@ -1179,16 +1179,10 @@ func (s *S3Server) deleteObject(w http.ResponseWriter, r *http.Request, bucket s cleanupManifest = nil return nil } - 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 @@ -2742,6 +2736,66 @@ func validateS3PutPreconditions(r *http.Request, previous *s3ObjectManifest) err 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 || previous == nil { + return nil + } + ifMatch := strings.TrimSpace(r.Header.Get("If-Match")) + if ifMatch == "" { + return nil + } + if strings.Trim(ifMatch, `"`) != previous.ETag { + return errors.New("etag precondition failed") + } + return nil +} + func cloneLeaderAddrMap(src map[string]string) map[string]string { if len(src) == 0 { return nil diff --git a/adapter/s3_conditional_delete_test.go b/adapter/s3_conditional_delete_test.go new file mode 100644 index 000000000..07116d82a --- /dev/null +++ b/adapter/s3_conditional_delete_test.go @@ -0,0 +1,190 @@ +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, key string) bool { + t.Helper() + + rec := httptest.NewRecorder() + server.handle(rec, newS3TestRequest(http.MethodHead, "/"+conditionalDeleteBucket+"/"+key, 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, conditionalDeleteKey), + "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, conditionalDeleteKey)) +} + +// 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) +} + +// 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, conditionalDeleteKey)) +} + +// Delete stays idempotent for an absent key, with or without the header: S3 +// delete is not an error on a missing object, and a precondition cannot be +// evaluated against something that does not exist. +func TestS3ConditionalDeleteOnAnAbsentKeyStaysIdempotent(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.StatusNoContent, rec.Code) +} + +// 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("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 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, nil)) + }) +}