diff --git a/adapter/s3.go b/adapter/s3.go index 96b54e982..6020843c0 100644 --- a/adapter/s3.go +++ b/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 diff --git a/adapter/s3_conditional_delete_test.go b/adapter/s3_conditional_delete_test.go new file mode 100644 index 000000000..6f63eea84 --- /dev/null +++ b/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)) + }) +}