From bd6cfe64a30b903d8b84c62a063d043c468352c3 Mon Sep 17 00:00:00 2001 From: niksis02 Date: Sat, 19 Sep 2026 22:26:47 +0400 Subject: [PATCH 1/3] fix: stop posix mixing up keys that differ only by a trailing slash In posix a key and the same key with a trailing slash, such as `foo` and `foo/`, map to one path and share one set of attributes: a file there is the object of `foo`, a directory object is the object of `foo/`. Several operations used the entry at that path without checking that it belongs to the requested key. They now check it through the new `isLiveObject`, `statLiveObject` and `objVersionAttrPath` helpers. `DeleteObject` of `foo` with a version id of `foo/` treated the directory as the current version of `foo` and removed it. That left the noncurrent versions of `foo/` unlisted and undeletable, so `DeleteBucket` failed with `BucketNotEmpty`. A delete of `foo` without a version id failed with an internal error while trying to version the directory as a file. Both now succeed without touching `foo/`, as for any object that doesn't exist. `CompleteMultipartUpload` of `foo` cleared the `delete-marker` attribute of `foo/` and then failed with an internal error when linking the object onto the directory, which turned the delete marker back into a live version. It now returns `ExistingObjectIsDirectory` before any attribute is changed, both before the parts are assembled and again under the object publish lock. The idempotent completion path also no longer reports a missing upload as completed just because `foo/` exists. The object tagging, legal hold and retention APIs read and wrote the attributes of the other key. For example, `PutObjectLegalHold` on `foo` could turn off the legal hold of `foo/`, and the object lock check could block a delete because of the other key's retention. When the requested key has no object, they now return `NoSuchKey` for the current version and `NoSuchVersion` for a specific version. They also resolve the `null` version id to a current null version instead of looking for it in the versioning directory. The object lock check treats `NoSuchVersion` like `NoSuchKey`, since a version that doesn't exist has nothing to protect. Conditional writes no longer evaluate `If-Match` and `If-None-Match` against the other key's ETag. `CreateMultipartUpload` now stores the upload's tagging and object lock settings directly on the upload directory, and `PutObject` of a directory object sets its tagging after the directory gets its ETag. --- auth/object_lock.go | 7 +- backend/posix/posix.go | 357 +++++++++++++++---------------- tests/integration/group-tests.go | 21 ++ tests/integration/posix.go | 250 ++++++++++++++++++++++ tests/integration/utils.go | 167 +++++++++++++++ tests/integration/versioning.go | 253 ++++++++++++++++++++++ 6 files changed, 873 insertions(+), 182 deletions(-) diff --git a/auth/object_lock.go b/auth/object_lock.go index 77083cb2..ba502e50 100644 --- a/auth/object_lock.go +++ b/auth/object_lock.go @@ -495,7 +495,9 @@ func (s objectLockState) checkObject(ctx context.Context, be backend.Backend, ia checkRetention := true retentionData, err := be.GetObjectRetention(ctx, bucket, key, versionId) - if errors.Is(err, s3err.GetAPIError(s3err.ErrNoSuchKey)) { + // an object or version that doesn't exist has nothing to protect + if errors.Is(err, s3err.GetAPIError(s3err.ErrNoSuchKey)) || + errors.Is(err, s3err.GetAPIError(s3err.ErrNoSuchVersion)) { return nil } // the object is a delete marker, if a `MethodNotAllowed` error is returned @@ -538,7 +540,8 @@ func (s objectLockState) checkObject(ctx context.Context, be backend.Backend, ia status, err := be.GetObjectLegalHold(ctx, bucket, key, versionId) if err != nil { - if errors.Is(err, s3err.GetAPIError(s3err.ErrNoSuchKey)) { + if errors.Is(err, s3err.GetAPIError(s3err.ErrNoSuchKey)) || + errors.Is(err, s3err.GetAPIError(s3err.ErrNoSuchVersion)) { return nil } if errors.Is(err, s3err.GetAPIError(s3err.ErrNoSuchObjectLockConfiguration)) { diff --git a/backend/posix/posix.go b/backend/posix/posix.go index 018421be..a93ad686 100644 --- a/backend/posix/posix.go +++ b/backend/posix/posix.go @@ -631,21 +631,20 @@ func (p *Posix) doesBucketExist(bucket string) error { return nil } -func (p *Posix) doesBucketAndObjectExist(bucket, object string) error { +// doesBucketAndObjectExist checks that bucket exists and, when versionId is +// empty, that object has a current version. A specific version is looked up +// once versionId is validated. +func (p *Posix) doesBucketAndObjectExist(bucket, object, versionId string) error { err := p.doesBucketExist(bucket) if err != nil { return err } - - _, err = os.Stat(p.ObjectPath(bucket, object)) - if errors.Is(err, fs.ErrNotExist) || isErrNotDir(err) { - return s3err.GetAPIError(s3err.ErrNoSuchKey) - } - if err != nil { - return fmt.Errorf("stat object: %w", err) + if versionId != "" { + return nil } - return nil + _, _, err = p.objVersionAttrPath(bucket, object, "") + return err } func (p *Posix) ListBuckets(ctx context.Context, input s3response.ListBucketsInput) (s3response.ListAllMyBucketsResult, error) { @@ -1300,15 +1299,79 @@ func (p *Posix) isDirObject(bucket, key string) (bool, error) { return true, nil } -// isLiveDirObject reports whether fi, the entry at the path of the -// directory object key, is a directory object -func (p *Posix) isLiveDirObject(fi os.FileInfo, bucket, key string) (bool, error) { - if !fi.IsDir() { +// isLiveObject reports whether fi, the entry at the path of key, is the +// current version of key. A key and the same key with a trailing slash +// share one path and one set of attributes: a file there is the object of +// the key without the slash, a directory object the object of the key with +// it. +func (p *Posix) isLiveObject(fi os.FileInfo, bucket, key string) (bool, error) { + if fi.IsDir() != strings.HasSuffix(key, "/") { return false, nil } + if !fi.IsDir() { + return true, nil + } return p.isDirObject(bucket, key) } +// statLiveObject returns the file info of the current version of key. An +// error matching fs.ErrNotExist is returned when key has no current +// version, including when the entry at its path is another key's object. +func (p *Posix) statLiveObject(bucket, key string) (os.FileInfo, error) { + fi, err := os.Stat(p.ObjectPath(bucket, key)) + if isErrNotDir(err) { + return nil, fs.ErrNotExist + } + if err != nil { + return nil, err + } + isObj, err := p.isLiveObject(fi, bucket, key) + if err != nil { + return nil, err + } + if !isObj { + return nil, fs.ErrNotExist + } + return fi, nil +} + +// objVersionAttrPath returns the bucket and object that the attributes of +// the version versionId of key are stored at: key itself for its current +// version, an entry of the versioning directory for other versions. An +// empty versionId selects the current version, and NoSuchKey is returned if +// key has none. +func (p *Posix) objVersionAttrPath(bucket, key, versionId string) (string, string, error) { + _, err := p.statLiveObject(bucket, key) + if isErrNameTooLong(err) { + return "", "", s3err.GetKeyTooLongErr(int64(len(key)), 1024) + } + if err != nil && !errors.Is(err, fs.ErrNotExist) { + return "", "", fmt.Errorf("stat object: %w", err) + } + isLive := err == nil + + if versionId == "" { + if !isLive { + return "", "", s3err.GetAPIError(s3err.ErrNoSuchKey) + } + return bucket, key, nil + } + + if isLive { + vId, err := p.meta.RetrieveAttribute(nil, bucket, key, versionIdKey) + if errors.Is(err, meta.ErrNoSuchKey) { + vId = []byte(nullVersionId) + } else if err != nil { + return "", "", fmt.Errorf("get obj versionId: %w", err) + } + if string(vId) == versionId { + return bucket, key, nil + } + } + + return filepath.Join(p.versioningDir, bucket), filepath.Join(genObjVersionKey(key), versionId), nil +} + // clearDirObjectAttrs removes the directory object attributes, including // legacy metadata attributes, from the directory at bucket/key. The etag is // kept: it marks the directory as an object, so a failure before the new @@ -1962,7 +2025,7 @@ func (p *Posix) CreateMultipartUpload(ctx context.Context, mpu s3response.Create // set object tagging if tags != nil { - err := p.PutObjectTagging(withCtxNoSlot(ctx), bucket, filepath.Join(objdir, uploadID), "", tags) + err := p.storeObjectTags(bucket, filepath.Join(objdir, uploadID), tags) if err != nil { // cleanup object if returning error os.RemoveAll(filepath.Join(tmppath, uploadID)) @@ -1993,7 +2056,10 @@ func (p *Posix) CreateMultipartUpload(ctx context.Context, mpu s3response.Create // set object legal hold if mpu.ObjectLockLegalHoldStatus == types.ObjectLockLegalHoldStatusOn { - err := p.PutObjectLegalHold(withCtxNoSlot(ctx), bucket, filepath.Join(objdir, uploadID), "", true) + err := p.isBucketObjectLockEnabled(bucket) + if err == nil { + err = p.meta.StoreAttribute(nil, bucket, filepath.Join(objdir, uploadID), objectLegalHoldKey, []byte{1}) + } if err != nil { if errors.Is(err, s3err.GetAPIError(s3err.ErrMissingObjectLockConfiguration)) { err = s3err.GetAPIError(s3err.ErrMissingObjectLockConfigurationNoSpaces) @@ -2020,7 +2086,10 @@ func (p *Posix) CreateMultipartUpload(ctx context.Context, mpu s3response.Create _ = p.meta.DeleteAttributes(bucket, filepath.Join(objdir, uploadID)) return s3response.InitiateMultipartUploadResult{}, fmt.Errorf("parse object lock retention: %w", err) } - err = p.PutObjectRetention(withCtxNoSlot(ctx), bucket, filepath.Join(objdir, uploadID), "", retParsed) + err = p.isBucketObjectLockEnabled(bucket) + if err == nil { + err = p.meta.StoreAttribute(nil, bucket, filepath.Join(objdir, uploadID), objectRetentionKey, retParsed) + } if err != nil { if errors.Is(err, s3err.GetAPIError(s3err.ErrMissingObjectLockConfiguration)) { err = s3err.GetAPIError(s3err.ErrMissingObjectLockConfigurationNoSpaces) @@ -2313,7 +2382,9 @@ func (p *Posix) CompleteMultipartUploadWithCopy(ctx context.Context, input *s3.C }, "", nil } // Directory is gone: the concurrent call already completed and cleaned up. - if _, statErr := os.Stat(p.ObjectPath(bucket, object)); statErr == nil { + // A directory at the object path is the object of the key with a + // trailing slash, not the completed upload. + if fi, statErr := os.Stat(p.ObjectPath(bucket, object)); statErr == nil && !fi.IsDir() { etag := multipartClaimToken if p.dataIntegrityEtag { etagBytes, etagErr := p.meta.RetrieveAttribute(nil, bucket, object, etagkey) @@ -2383,13 +2454,17 @@ func (p *Posix) CompleteMultipartUploadWithCopy(ctx context.Context, input *s3.C defer os.Rename(uploadIDInProgress, uploadIDDir) defer p.meta.RenameObject(bucket, newMetaObj, oldMetaObj) - // Fast-fail precondition check before the parts are assembled. This is - // only advisory: the authoritative check is repeated while holding the - // object publish lock just before the final link. + // Fast-fail precondition and directory checks before the parts are + // assembled. These are only advisory: the authoritative checks are + // repeated while holding the object publish lock just before the final + // link. err = p.checkPutPreconditions(bucket, object, input.IfMatch, input.IfNoneMatch) if err != nil { return res, "", err } + if d, err := os.Stat(p.ObjectPath(bucket, object)); err == nil && d.IsDir() { + return res, "", s3err.GetAPIError(s3err.ErrExistingObjectIsDirectory) + } checksums, err := p.retrieveChecksums(nil, bucket, filepath.Join(objdir, activeUploadName)) if err != nil && !errors.Is(err, meta.ErrNoSuchKey) { @@ -2717,9 +2792,14 @@ func (p *Posix) CompleteMultipartUploadWithCopy(ctx context.Context, input *s3.C vEnabled := p.isBucketVersioningEnabled(vStatus) d, err := os.Stat(objname) + if err == nil && d.IsDir() { + // the directory is the object of the key with a trailing slash, or + // the parent of other objects: its attributes are not this object's + return res, "", s3err.GetAPIError(s3err.ErrExistingObjectIsDirectory) + } // if the versioning is enabled first create the file object version - if p.versioningEnabled() && vEnabled && err == nil && !d.IsDir() { + if p.versioningEnabled() && vEnabled && err == nil { _, err := p.createObjVersion(bucket, object, d.Size(), acct, false) if err != nil { return res, "", fmt.Errorf("create object version: %w", err) @@ -4189,6 +4269,13 @@ func (p *Posix) checkPutPreconditions(bucket, object string, ifMatch, ifNoneMatc return s3err.GetAPIError(s3err.ErrNotImplemented) } + // the etag at the object path may be the one of the key with or + // without the trailing slash + _, err := p.statLiveObject(bucket, object) + if errors.Is(err, fs.ErrNotExist) { + return backend.EvaluateObjectPutPreconditions("", ifMatch, ifNoneMatch, false) + } + etagBytes, err := p.meta.RetrieveAttribute(nil, bucket, object, etagkey) if err == nil || errors.Is(err, fs.ErrNotExist) || errors.Is(err, meta.ErrNoSuchKey) { return backend.EvaluateObjectPutPreconditions(string(etagBytes), ifMatch, ifNoneMatch, err == nil) @@ -4420,14 +4507,6 @@ func (p *Posix) PutObjectWithPostFunc(ctx context.Context, po s3response.PutObje return s3response.PutObjectOutput{}, fmt.Errorf("set object metadata: %w", err) } - // Set object tagging - if tags != nil { - err := p.PutObjectTagging(withCtxNoSlot(ctx), *po.Bucket, *po.Key, "", tags) - if err != nil { - return s3response.PutObjectOutput{}, err - } - } - dirETag := emptyMD5 if p.dataIntegrityEtag { dirETag = fmt.Sprintf("\"%s-%s\"", strings.ToUpper(string(checksumAlgorithm)), expectedSum) @@ -4483,6 +4562,14 @@ func (p *Posix) PutObjectWithPostFunc(ctx context.Context, po s3response.PutObje _ = os.Chtimes(name, now, now) } + // Set object tagging once the etag makes the directory an object + if tags != nil { + err := p.PutObjectTagging(withCtxNoSlot(ctx), *po.Bucket, *po.Key, "", tags) + if err != nil { + return s3response.PutObjectOutput{}, err + } + } + err = p.putObjectLockSettings(ctx, po) if err != nil { return s3response.PutObjectOutput{}, err @@ -4896,15 +4983,15 @@ func (p *Posix) DeleteObject(ctx context.Context, input *s3.DeleteObjectInput) ( if err != nil { return nil, s3err.GetAPIError(s3err.ErrNoSuchKey) } - if isDir { - isObj, err := p.isLiveDirObject(fi, bucket, object) - if err != nil { - return nil, err - } - if !isObj { - // AWS returns success if the object does not exist - return &s3.DeleteObjectOutput{}, nil - } + // the entry at the object path may be the object of the key + // with or without the trailing slash + isObj, err := p.isLiveObject(fi, bucket, object) + if err != nil { + return nil, err + } + if !isObj { + // AWS returns success if the object does not exist + return &s3.DeleteObjectOutput{}, nil } err = evalPreconditions(fi, bucket, object) @@ -4962,28 +5049,19 @@ func (p *Posix) DeleteObject(ctx context.Context, input *s3.DeleteObjectInput) ( } else { versionPath := p.genObjVersionPath(bucket, object) - if isDir { - // the attributes at a directory object path may belong to a - // file or to a directory that isn't an object - fi, err := os.Stat(objpath) - if errors.Is(err, fs.ErrNotExist) || isErrNotDir(err) { - // AWS returns success if the object does not exist - return &s3.DeleteObjectOutput{VersionId: input.VersionId}, nil - } - if isErrNameTooLong(err) { - return nil, s3err.GetKeyTooLongErr(int64(len(object)), 1024) - } - if err != nil { - return nil, fmt.Errorf("stat object: %w", err) - } - isObj, err := p.isLiveDirObject(fi, bucket, object) - if err != nil { - return nil, err - } - if !isObj { - // AWS returns success if the object does not exist - return &s3.DeleteObjectOutput{VersionId: input.VersionId}, nil - } + // the attributes at the object path may belong to the key with + // or without the trailing slash, or to a directory that isn't an + // object + _, err := p.statLiveObject(bucket, object) + if errors.Is(err, fs.ErrNotExist) { + // AWS returns success if the object does not exist + return &s3.DeleteObjectOutput{VersionId: input.VersionId}, nil + } + if isErrNameTooLong(err) { + return nil, s3err.GetKeyTooLongErr(int64(len(object)), 1024) + } + if err != nil { + return nil, fmt.Errorf("stat object: %w", err) } vId, err := p.meta.RetrieveAttribute(nil, bucket, object, versionIdKey) @@ -4997,16 +5075,6 @@ func (p *Posix) DeleteObject(ctx context.Context, input *s3.DeleteObjectInput) ( return nil, fmt.Errorf("get obj versionId: %w", err) } if errors.Is(err, meta.ErrNoSuchKey) { - // With sidecar, ErrNoSuchKey means "attribute absent" regardless of - // whether the data file exists. If the file is absent the object - // does not exist at all → AWS returns success for DeleteObject. - // Also handle ENOTDIR: when a key such as "foo/bar" is requested - // but "foo" is a regular file (not a directory), the path cannot - // contain any object. - _, statErr := os.Stat(p.ObjectPath(bucket, object)) - if errors.Is(statErr, fs.ErrNotExist) || isErrNotDir(statErr) { - return &s3.DeleteObjectOutput{VersionId: input.VersionId}, nil - } vId = []byte(nullVersionId) } @@ -6836,36 +6904,14 @@ func (p *Posix) GetObjectTagging(ctx context.Context, bucket, object, versionId return nil, err } - if versionId == "" { - _, err = os.Stat(p.ObjectPath(bucket, object)) - if errors.Is(err, fs.ErrNotExist) || isErrNotDir(err) { - return nil, s3err.GetAPIError(s3err.ErrNoSuchKey) - } - if isErrNameTooLong(err) { - return nil, s3err.GetAPIError(s3err.ErrKeyTooLong) - } - if err != nil { - return nil, fmt.Errorf("stat object: %w", err) - } + if versionId != "" && !p.versioningEnabled() { + //TODO: Maybe we need to return our custom error here? + return nil, s3err.GetInvalidArgumentErr(s3err.InvalidArgVersionId, versionId) } - if versionId != "" { - if !p.versioningEnabled() { - //TODO: Maybe we need to return our custom error here? - return nil, s3err.GetInvalidArgumentErr(s3err.InvalidArgVersionId, versionId) - } - vId, err := p.meta.RetrieveAttribute(nil, bucket, object, versionIdKey) - if errors.Is(err, fs.ErrNotExist) || isErrNotDir(err) { - return nil, s3err.GetAPIError(s3err.ErrNoSuchKey) - } - if err != nil && !errors.Is(err, meta.ErrNoSuchKey) { - return nil, fmt.Errorf("get obj versionId: %w", err) - } - - if string(vId) != versionId { - bucket = filepath.Join(p.versioningDir, bucket) - object = filepath.Join(genObjVersionKey(object), versionId) - } + bucket, object, err = p.objVersionAttrPath(bucket, object, versionId) + if err != nil { + return nil, err } err = p.ensureNotDeleteMarker(bucket, object, versionId) @@ -6923,36 +6969,14 @@ func (p *Posix) PutObjectTagging(ctx context.Context, bucket, object, versionId return err } - if versionId == "" { - _, err = os.Stat(p.ObjectPath(bucket, object)) - if errors.Is(err, fs.ErrNotExist) || isErrNotDir(err) { - return s3err.GetAPIError(s3err.ErrNoSuchKey) - } - if isErrNameTooLong(err) { - return s3err.GetAPIError(s3err.ErrKeyTooLong) - } - if err != nil { - return fmt.Errorf("stat object: %w", err) - } + if versionId != "" && !p.versioningEnabled() { + //TODO: Maybe we need to return our custom error here? + return s3err.GetInvalidArgumentErr(s3err.InvalidArgVersionId, versionId) } - if versionId != "" { - if !p.versioningEnabled() { - //TODO: Maybe we need to return our custom error here? - return s3err.GetInvalidArgumentErr(s3err.InvalidArgVersionId, versionId) - } - vId, err := p.meta.RetrieveAttribute(nil, bucket, object, versionIdKey) - if errors.Is(err, fs.ErrNotExist) || isErrNotDir(err) { - return s3err.GetAPIError(s3err.ErrNoSuchKey) - } - if err != nil && !errors.Is(err, meta.ErrNoSuchKey) { - return fmt.Errorf("get obj versionId: %w", err) - } - - if string(vId) != versionId { - bucket = filepath.Join(p.versioningDir, bucket) - object = filepath.Join(genObjVersionKey(object), versionId) - } + bucket, object, err = p.objVersionAttrPath(bucket, object, versionId) + if err != nil { + return err } err = p.ensureNotDeleteMarker(bucket, object, versionId) @@ -6977,12 +7001,7 @@ func (p *Posix) PutObjectTagging(ctx context.Context, bucket, object, versionId return nil } - b, err := json.Marshal(tags) - if err != nil { - return fmt.Errorf("marshal tags: %w", err) - } - - err = p.meta.StoreAttribute(nil, bucket, object, tagHdr, b) + err = p.storeObjectTags(bucket, object, tags) if errors.Is(err, fs.ErrNotExist) || isErrNotDir(err) { if versionId != "" { return s3err.GetNoSuchVersionErr(object, versionId) @@ -6996,6 +7015,16 @@ func (p *Posix) PutObjectTagging(ctx context.Context, bucket, object, versionId return nil } +// storeObjectTags stores tags as the tagging attribute of bucket/object +func (p *Posix) storeObjectTags(bucket, object string, tags map[string]string) error { + b, err := json.Marshal(tags) + if err != nil { + return fmt.Errorf("marshal tags: %w", err) + } + + return p.meta.StoreAttribute(nil, bucket, object, tagHdr, b) +} + func (p *Posix) DeleteObjectTagging(ctx context.Context, bucket, object, versionId string) error { if !p.isBucketValid(bucket) { return s3err.GetBucketErr(s3err.ErrInvalidBucketName, bucket) @@ -7317,7 +7346,7 @@ func (p *Posix) PutObjectLegalHold(ctx context.Context, bucket, object, versionI if !p.isBucketValid(bucket) { return s3err.GetBucketErr(s3err.ErrInvalidBucketName, bucket) } - err = p.doesBucketAndObjectExist(bucket, object) + err = p.doesBucketAndObjectExist(bucket, object, versionId) if err != nil { return err } @@ -7342,17 +7371,9 @@ func (p *Posix) PutObjectLegalHold(ctx context.Context, bucket, object, versionI //TODO: Maybe we need to return our custom error here? return s3err.GetInvalidArgumentErr(s3err.InvalidArgVersionId, versionId) } - vId, err := p.meta.RetrieveAttribute(nil, bucket, object, versionIdKey) - if errors.Is(err, fs.ErrNotExist) || isErrNotDir(err) { - return s3err.GetAPIError(s3err.ErrNoSuchKey) - } - if err != nil && !errors.Is(err, meta.ErrNoSuchKey) { - return fmt.Errorf("get obj versionId: %w", err) - } - - if string(vId) != versionId { - bucket = filepath.Join(p.versioningDir, bucket) - object = filepath.Join(genObjVersionKey(object), versionId) + bucket, object, err = p.objVersionAttrPath(bucket, object, versionId) + if err != nil { + return err } } @@ -7385,7 +7406,7 @@ func (p *Posix) GetObjectLegalHold(ctx context.Context, bucket, object, versionI if !p.isBucketValid(bucket) { return nil, s3err.GetBucketErr(s3err.ErrInvalidBucketName, bucket) } - err = p.doesBucketAndObjectExist(bucket, object) + err = p.doesBucketAndObjectExist(bucket, object, versionId) if err != nil { return nil, err } @@ -7403,17 +7424,9 @@ func (p *Posix) GetObjectLegalHold(ctx context.Context, bucket, object, versionI //TODO: Maybe we need to return our custom error here? return nil, s3err.GetInvalidArgumentErr(s3err.InvalidArgVersionId, versionId) } - vId, err := p.meta.RetrieveAttribute(nil, bucket, object, versionIdKey) - if errors.Is(err, fs.ErrNotExist) || isErrNotDir(err) { - return nil, s3err.GetAPIError(s3err.ErrNoSuchKey) - } - if err != nil && !errors.Is(err, meta.ErrNoSuchKey) { - return nil, fmt.Errorf("get obj versionId: %w", err) - } - - if string(vId) != versionId { - bucket = filepath.Join(p.versioningDir, bucket) - object = filepath.Join(genObjVersionKey(object), versionId) + bucket, object, err = p.objVersionAttrPath(bucket, object, versionId) + if err != nil { + return nil, err } } @@ -7451,7 +7464,7 @@ func (p *Posix) PutObjectRetention(ctx context.Context, bucket, object, versionI if !p.isBucketValid(bucket) { return s3err.GetBucketErr(s3err.ErrInvalidBucketName, bucket) } - err = p.doesBucketAndObjectExist(bucket, object) + err = p.doesBucketAndObjectExist(bucket, object, versionId) if err != nil { return err } @@ -7469,17 +7482,9 @@ func (p *Posix) PutObjectRetention(ctx context.Context, bucket, object, versionI //TODO: Maybe we need to return our custom error here? return s3err.GetInvalidArgumentErr(s3err.InvalidArgVersionId, versionId) } - vId, err := p.meta.RetrieveAttribute(nil, bucket, object, versionIdKey) - if errors.Is(err, fs.ErrNotExist) || isErrNotDir(err) { - return s3err.GetAPIError(s3err.ErrNoSuchKey) - } - if err != nil && !errors.Is(err, meta.ErrNoSuchKey) { - return fmt.Errorf("get obj versionId: %w", err) - } - - if string(vId) != versionId { - bucket = filepath.Join(p.versioningDir, bucket) - object = filepath.Join(genObjVersionKey(object), versionId) + bucket, object, err = p.objVersionAttrPath(bucket, object, versionId) + if err != nil { + return err } } @@ -7506,7 +7511,7 @@ func (p *Posix) GetObjectRetention(ctx context.Context, bucket, object, versionI if !p.isBucketValid(bucket) { return nil, s3err.GetBucketErr(s3err.ErrInvalidBucketName, bucket) } - err = p.doesBucketAndObjectExist(bucket, object) + err = p.doesBucketAndObjectExist(bucket, object, versionId) if err != nil { return nil, err } @@ -7524,17 +7529,9 @@ func (p *Posix) GetObjectRetention(ctx context.Context, bucket, object, versionI //TODO: Maybe we need to return our custom error here? return nil, s3err.GetInvalidArgumentErr(s3err.InvalidArgVersionId, versionId) } - vId, err := p.meta.RetrieveAttribute(nil, bucket, object, versionIdKey) - if errors.Is(err, fs.ErrNotExist) || isErrNotDir(err) { - return nil, s3err.GetAPIError(s3err.ErrNoSuchKey) - } - if err != nil && !errors.Is(err, meta.ErrNoSuchKey) { - return nil, fmt.Errorf("get obj versionId: %w", err) - } - - if string(vId) != versionId { - bucket = filepath.Join(p.versioningDir, bucket) - object = filepath.Join(genObjVersionKey(object), versionId) + bucket, object, err = p.objVersionAttrPath(bucket, object, versionId) + if err != nil { + return nil, err } } diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go index 02b9e1a1..c9961db4 100644 --- a/tests/integration/group-tests.go +++ b/tests/integration/group-tests.go @@ -990,6 +990,12 @@ func TestPosix(ts *TestState) { ts.Run(DeleteObject_name_too_long) ts.Run(CopyObject_overwrite_same_dir_object) ts.Run(CopyObject_overwrite_same_file_object) + ts.Run(CompleteMultipartUpload_overwrite_dir_obj) + if ts.conf.versioningEnabled { + ts.Run(CompleteMultipartUpload_overwrite_dir_obj_delete_marker) + } + ts.Run(ObjectTagging_trailing_slash_counterpart) + ts.Run(ObjectLock_trailing_slash_counterpart) ts.Run(DeleteObject_directory_not_empty) if !ts.conf.windowsTests { ts.Run(PutObject_race_with_delete) @@ -1094,6 +1100,9 @@ func TestScoutfs(ts *TestState) { ts.Run(DeleteObject_name_too_long) ts.Run(CopyObject_overwrite_same_dir_object) ts.Run(CopyObject_overwrite_same_file_object) + ts.Run(CompleteMultipartUpload_overwrite_dir_obj) + ts.Run(ObjectTagging_trailing_slash_counterpart) + ts.Run(ObjectLock_trailing_slash_counterpart) ts.Run(DeleteObject_directory_not_empty) } @@ -1977,6 +1986,7 @@ func TestVersioning(ts *TestState) { ts.Run(Versioning_DeleteObjectTagging_invalid_versionId) ts.Run(Versioning_DeleteObjectTagging_non_existing_object_version) ts.Run(Versioning_PutGetDeleteObjectTagging_success) + ts.Run(Versioning_ObjectTagging_trailing_slash_counterpart) // GetObjectAttributes action ts.Run(Versioning_GetObjectAttributes_invalid_versionId) ts.Run(Versioning_GetObjectAttributes_object_version) @@ -1987,6 +1997,7 @@ func TestVersioning(ts *TestState) { ts.Run(Versioning_DeleteObject_dir_object_latest_version) ts.Run(Versioning_DeleteObject_non_existing_object) ts.Run(Versioning_DeleteObject_implicit_dir) + ts.Run(Versioning_DeleteObject_trailing_slash_counterpart) if !ts.conf.windowsTests { ts.Run(Versioning_DeleteObject_delete_a_delete_marker) ts.Run(Versioning_DeleteObject_dir_object_with_children) @@ -2047,6 +2058,8 @@ func TestVersioning(ts *TestState) { if !ts.conf.windowsTests { ts.Run(Versioning_WORM_remove_delete_marker_under_bucket_default_retention) } + ts.Run(Versioning_WORM_trailing_slash_counterpart) + ts.Run(Versioning_WORM_null_version_locked_with_legal_hold) // Concurrent requests // Versioninig_concurrent_upload_object ts.Run(Versioning_AccessControl_GetObjectVersion) @@ -2989,6 +3002,10 @@ func GetIntTests() IntTests { "DeleteObject_name_too_long": DeleteObject_name_too_long, "CopyObject_overwrite_same_dir_object": CopyObject_overwrite_same_dir_object, "CopyObject_overwrite_same_file_object": CopyObject_overwrite_same_file_object, + "CompleteMultipartUpload_overwrite_dir_obj": CompleteMultipartUpload_overwrite_dir_obj, + "CompleteMultipartUpload_overwrite_dir_obj_delete_marker": CompleteMultipartUpload_overwrite_dir_obj_delete_marker, + "ObjectTagging_trailing_slash_counterpart": ObjectTagging_trailing_slash_counterpart, + "ObjectLock_trailing_slash_counterpart": ObjectLock_trailing_slash_counterpart, "DeleteObject_non_existing_dir_object": DeleteObject_non_existing_dir_object, "DeleteObject_directory_object": DeleteObject_directory_object, "DeleteObject_success": DeleteObject_success, @@ -3499,6 +3516,7 @@ func GetIntTests() IntTests { "Versioning_DeleteObjectTagging_invalid_versionId": Versioning_DeleteObjectTagging_invalid_versionId, "Versioning_DeleteObjectTagging_non_existing_object_version": Versioning_DeleteObjectTagging_non_existing_object_version, "Versioning_PutGetDeleteObjectTagging_success": Versioning_PutGetDeleteObjectTagging_success, + "Versioning_ObjectTagging_trailing_slash_counterpart": Versioning_ObjectTagging_trailing_slash_counterpart, "Versioning_GetObjectAttributes_invalid_versionId": Versioning_GetObjectAttributes_invalid_versionId, "Versioning_GetObjectAttributes_object_version": Versioning_GetObjectAttributes_object_version, "Versioning_GetObjectAttributes_delete_marker": Versioning_GetObjectAttributes_delete_marker, @@ -3507,6 +3525,7 @@ func GetIntTests() IntTests { "Versioning_DeleteObject_dir_object_latest_version": Versioning_DeleteObject_dir_object_latest_version, "Versioning_DeleteObject_non_existing_object": Versioning_DeleteObject_non_existing_object, "Versioning_DeleteObject_implicit_dir": Versioning_DeleteObject_implicit_dir, + "Versioning_DeleteObject_trailing_slash_counterpart": Versioning_DeleteObject_trailing_slash_counterpart, "Versioning_DeleteObject_delete_a_delete_marker": Versioning_DeleteObject_delete_a_delete_marker, "Versioning_DeleteObject_dir_object_with_children": Versioning_DeleteObject_dir_object_with_children, "Versioning_Delete_null_versionId_object": Versioning_Delete_null_versionId_object, @@ -3557,6 +3576,8 @@ func GetIntTests() IntTests { "Versioning_WORM_CopyObject_overwrite_locked_object": Versioning_WORM_CopyObject_overwrite_locked_object, "Versioning_WORM_CompleteMultipartUpload_overwrite_locked_object": Versioning_WORM_CompleteMultipartUpload_overwrite_locked_object, "Versioning_WORM_remove_delete_marker_under_bucket_default_retention": Versioning_WORM_remove_delete_marker_under_bucket_default_retention, + "Versioning_WORM_trailing_slash_counterpart": Versioning_WORM_trailing_slash_counterpart, + "Versioning_WORM_null_version_locked_with_legal_hold": Versioning_WORM_null_version_locked_with_legal_hold, "Versioning_AccessControl_GetObjectVersion": Versioning_AccessControl_GetObjectVersion, "Versioning_AccessControl_HeadObjectVersion": Versioning_AccessControl_HeadObjectVersion, "Versioning_AccessControl_object_tagging_policy": Versioning_AccessControl_object_tagging_policy, diff --git a/tests/integration/posix.go b/tests/integration/posix.go index b9401a77..71454f59 100644 --- a/tests/integration/posix.go +++ b/tests/integration/posix.go @@ -17,6 +17,7 @@ package integration import ( "context" "fmt" + "time" "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/service/s3" @@ -222,6 +223,255 @@ func CopyObject_overwrite_same_file_object(s *S3Conf) error { }) } +func CompleteMultipartUpload_overwrite_dir_obj(s *S3Conf) error { + testName := "CompleteMultipartUpload_overwrite_dir_obj" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + dir, obj := "foo/", "foo" + _, err := putObjects(s3client, []string{dir}, bucket) + if err != nil { + return err + } + + mp, err := createMp(s3client, bucket, obj) + if err != nil { + return err + } + + parts, _, err := uploadParts(s3client, 100, 1, bucket, obj, *mp.UploadId) + if err != nil { + return err + } + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err = s3client.CompleteMultipartUpload(ctx, &s3.CompleteMultipartUploadInput{ + Bucket: &bucket, + Key: &obj, + UploadId: mp.UploadId, + MultipartUpload: &types.CompletedMultipartUpload{ + Parts: []types.CompletedPart{ + {ETag: parts[0].ETag, PartNumber: parts[0].PartNumber}, + }, + }, + }) + cancel() + if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrExistingObjectIsDirectory)); err != nil { + return err + } + + // the directory object isn't taken for the object of a completed upload + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + _, err = s3client.CompleteMultipartUpload(ctx, &s3.CompleteMultipartUploadInput{ + Bucket: &bucket, + Key: &obj, + UploadId: getPtr("non-existing-upload-id"), + MultipartUpload: &types.CompletedMultipartUpload{ + Parts: []types.CompletedPart{ + {ETag: parts[0].ETag, PartNumber: parts[0].PartNumber}, + }, + }, + }) + cancel() + if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrNoSuchUpload)); err != nil { + return err + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + _, err = s3client.HeadObject(ctx, &s3.HeadObjectInput{ + Bucket: &bucket, + Key: &dir, + }) + cancel() + if err != nil { + return err + } + + // the failed upload can still be completed or aborted + return checkAndAbortUpload(s3client, bucket, obj, *mp.UploadId) + }) +} + +func CompleteMultipartUpload_overwrite_dir_obj_delete_marker(s *S3Conf) error { + testName := "CompleteMultipartUpload_overwrite_dir_obj_delete_marker" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + dir, obj := "foo/", "foo" + versions, err := createObjVersions(s3client, bucket, dir, 1) + if err != nil { + return err + } + versions[0].IsLatest = getPtr(false) + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + out, err := s3client.DeleteObject(ctx, &s3.DeleteObjectInput{ + Bucket: &bucket, + Key: &dir, + }) + cancel() + if err != nil { + return err + } + delMarkers := []types.DeleteMarkerEntry{ + {Key: &dir, VersionId: out.VersionId, IsLatest: getPtr(true)}, + } + + mp, err := createMp(s3client, bucket, obj) + if err != nil { + return err + } + + parts, _, err := uploadParts(s3client, 100, 1, bucket, obj, *mp.UploadId) + if err != nil { + return err + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + _, err = s3client.CompleteMultipartUpload(ctx, &s3.CompleteMultipartUploadInput{ + Bucket: &bucket, + Key: &obj, + UploadId: mp.UploadId, + MultipartUpload: &types.CompletedMultipartUpload{ + Parts: []types.CompletedPart{ + {ETag: parts[0].ETag, PartNumber: parts[0].PartNumber}, + }, + }, + }) + cancel() + if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrExistingObjectIsDirectory)); err != nil { + return err + } + + // the delete marker of the directory object is left in place + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + res, err := s3client.ListObjectVersions(ctx, &s3.ListObjectVersionsInput{ + Bucket: &bucket, + }) + cancel() + if err != nil { + return err + } + + if !compareVersions(versions, res.Versions) { + return fmt.Errorf("expected the versions to be %v, instead got %v", + versions, res.Versions) + } + if !compareDelMarkers(delMarkers, res.DeleteMarkers) { + return fmt.Errorf("expected the delete markers to be %v, instead got %v", + delMarkers, res.DeleteMarkers) + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + _, err = s3client.HeadObject(ctx, &s3.HeadObjectInput{ + Bucket: &bucket, + Key: &dir, + }) + cancel() + if err := checkSdkApiErr(err, "NotFound"); err != nil { + return err + } + + return checkAndAbortUpload(s3client, bucket, obj, *mp.UploadId) + }, withVersioning(types.BucketVersioningStatusEnabled)) +} + +func ObjectTagging_trailing_slash_counterpart(s *S3Conf) error { + testName := "ObjectTagging_trailing_slash_counterpart" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + tagSet := []types.Tag{{Key: getPtr("key"), Value: getPtr("value")}} + + // a key and the same key with a trailing slash have one path: the + // object of one of them isn't an object of the other + for _, keys := range [][2]string{{"my-dir/", "my-dir"}, {"my-obj", "my-obj/"}} { + obj, other := keys[0], keys[1] + _, err := putObjectWithData(objDataLen(obj, 10), &s3.PutObjectInput{ + Bucket: &bucket, + Key: &obj, + Tagging: getPtr("key=value"), + }, s3client) + if err != nil { + return err + } + + err = checkObjectTaggingErr(s3client, bucket, other, "", s3err.GetAPIError(s3err.ErrNoSuchKey)) + if err != nil { + return fmt.Errorf("%v: %w", other, err) + } + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + res, err := s3client.GetObjectTagging(ctx, &s3.GetObjectTaggingInput{ + Bucket: &bucket, + Key: &obj, + }) + cancel() + if err != nil { + return fmt.Errorf("%v: %w", obj, err) + } + if !areTagsSame(res.TagSet, tagSet) { + return fmt.Errorf("%v: expected the tag set to be %v, instead got %v", + obj, tagSet, res.TagSet) + } + } + + // neither key names the parent directory of an object + _, err := putObjects(s3client, []string{"my-parent/obj"}, bucket) + if err != nil { + return err + } + for _, key := range []string{"my-parent/", "my-parent"} { + err := checkObjectTaggingErr(s3client, bucket, key, "", s3err.GetAPIError(s3err.ErrNoSuchKey)) + if err != nil { + return fmt.Errorf("%v: %w", key, err) + } + } + + return nil + }) +} + +func ObjectLock_trailing_slash_counterpart(s *S3Conf) error { + testName := "ObjectLock_trailing_slash_counterpart" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + rDate := time.Now().Add(time.Hour).UTC().Truncate(time.Second) + lockedObjs := []objToDelete{} + + for _, keys := range [][2]string{{"my-dir/", "my-dir"}, {"my-obj", "my-obj/"}} { + obj, other := keys[0], keys[1] + _, err := putObjectWithData(objDataLen(obj, 10), &s3.PutObjectInput{ + Bucket: &bucket, + Key: &obj, + ObjectLockLegalHoldStatus: types.ObjectLockLegalHoldStatusOn, + ObjectLockMode: types.ObjectLockModeGovernance, + ObjectLockRetainUntilDate: &rDate, + }, s3client) + if err != nil { + return err + } + lockedObjs = append(lockedObjs, objToDelete{key: obj, removeOnlyLeglHold: true}) + + err = checkObjectLockErr(s3client, bucket, other, "", s3err.GetAPIError(s3err.ErrNoSuchKey)) + if err != nil { + return fmt.Errorf("%v: %w", other, err) + } + + // the lock of the object doesn't protect the other key + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err = s3client.DeleteObject(ctx, &s3.DeleteObjectInput{ + Bucket: &bucket, + Key: &other, + }) + cancel() + if err != nil { + return fmt.Errorf("%v: %w", other, err) + } + + err = checkObjectLock(s3client, bucket, obj, "", rDate) + if err != nil { + return fmt.Errorf("%v: %w", obj, err) + } + } + + return cleanupLockedObjects(s3client, bucket, lockedObjs) + }, withLock()) +} + // PutObject_race_with_delete tests the race between PutObject and DeleteObject // in the same subdirectory. // One goroutine sequentially puts "race-dir/0.txt" … "race-dir/N-1.txt". diff --git a/tests/integration/utils.go b/tests/integration/utils.go index e12976bb..7f5e2cbb 100644 --- a/tests/integration/utils.go +++ b/tests/integration/utils.go @@ -3989,3 +3989,170 @@ func checkDeleteObjectsErrsInOrder(got []types.Error, want []keyDenial) error { } return nil } + +// checkObjectTaggingErr checks that getting, putting and deleting the +// tagging of the version versionId of key fail with expected. An empty +// versionId selects the current version. +func checkObjectTaggingErr(client *s3.Client, bucket, key, versionId string, expected s3err.S3Error) error { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err := client.GetObjectTagging(ctx, &s3.GetObjectTaggingInput{ + Bucket: &bucket, + Key: &key, + VersionId: getNonEmptyPtr(versionId), + }) + cancel() + if err := checkApiErr(err, expected); err != nil { + return fmt.Errorf("get object tagging: %w", err) + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + _, err = client.PutObjectTagging(ctx, &s3.PutObjectTaggingInput{ + Bucket: &bucket, + Key: &key, + VersionId: getNonEmptyPtr(versionId), + Tagging: &types.Tagging{ + TagSet: []types.Tag{{Key: getPtr("other-key"), Value: getPtr("other-value")}}, + }, + }) + cancel() + if err := checkApiErr(err, expected); err != nil { + return fmt.Errorf("put object tagging: %w", err) + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + _, err = client.DeleteObjectTagging(ctx, &s3.DeleteObjectTaggingInput{ + Bucket: &bucket, + Key: &key, + VersionId: getNonEmptyPtr(versionId), + }) + cancel() + if err := checkApiErr(err, expected); err != nil { + return fmt.Errorf("delete object tagging: %w", err) + } + + return nil +} + +// checkObjectLockErr checks that getting and putting the legal hold and the +// retention of the version versionId of key fail with expected. An empty +// versionId selects the current version. +func checkObjectLockErr(client *s3.Client, bucket, key, versionId string, expected s3err.S3Error) error { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err := client.GetObjectLegalHold(ctx, &s3.GetObjectLegalHoldInput{ + Bucket: &bucket, + Key: &key, + VersionId: getNonEmptyPtr(versionId), + }) + cancel() + if err := checkApiErr(err, expected); err != nil { + return fmt.Errorf("get object legal hold: %w", err) + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + _, err = client.PutObjectLegalHold(ctx, &s3.PutObjectLegalHoldInput{ + Bucket: &bucket, + Key: &key, + VersionId: getNonEmptyPtr(versionId), + LegalHold: &types.ObjectLockLegalHold{ + Status: types.ObjectLockLegalHoldStatusOff, + }, + }) + cancel() + if err := checkApiErr(err, expected); err != nil { + return fmt.Errorf("put object legal hold: %w", err) + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + _, err = client.GetObjectRetention(ctx, &s3.GetObjectRetentionInput{ + Bucket: &bucket, + Key: &key, + VersionId: getNonEmptyPtr(versionId), + }) + cancel() + if err := checkApiErr(err, expected); err != nil { + return fmt.Errorf("get object retention: %w", err) + } + + rDate := time.Now().Add(time.Hour * 2) + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + _, err = client.PutObjectRetention(ctx, &s3.PutObjectRetentionInput{ + Bucket: &bucket, + Key: &key, + VersionId: getNonEmptyPtr(versionId), + Retention: &types.ObjectLockRetention{ + Mode: types.ObjectLockRetentionModeGovernance, + RetainUntilDate: &rDate, + }, + }) + cancel() + if err := checkApiErr(err, expected); err != nil { + return fmt.Errorf("put object retention: %w", err) + } + + return nil +} + +// checkObjectLock checks that the version versionId of key is under legal +// hold and has a governance retention until rDate. An empty versionId +// selects the current version. +func checkObjectLock(client *s3.Client, bucket, key, versionId string, rDate time.Time) error { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + lHold, err := client.GetObjectLegalHold(ctx, &s3.GetObjectLegalHoldInput{ + Bucket: &bucket, + Key: &key, + VersionId: getNonEmptyPtr(versionId), + }) + cancel() + if err != nil { + return err + } + if lHold.LegalHold == nil || lHold.LegalHold.Status != types.ObjectLockLegalHoldStatusOn { + return fmt.Errorf("expected the legal hold status to be %q, instead got %v", + types.ObjectLockLegalHoldStatusOn, lHold.LegalHold) + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + ret, err := client.GetObjectRetention(ctx, &s3.GetObjectRetentionInput{ + Bucket: &bucket, + Key: &key, + VersionId: getNonEmptyPtr(versionId), + }) + cancel() + if err != nil { + return err + } + if ret.Retention == nil || ret.Retention.Mode != types.ObjectLockRetentionModeGovernance || + ret.Retention.RetainUntilDate == nil || ret.Retention.RetainUntilDate.Unix() != rDate.Unix() { + return fmt.Errorf("expected a %q retention until %v, instead got %+v", + types.ObjectLockRetentionModeGovernance, rDate.Format(time.RFC3339), ret.Retention) + } + + return nil +} + +// checkAndAbortUpload checks that the upload uploadId of key is the only +// multipart upload in the bucket and aborts it +func checkAndAbortUpload(client *s3.Client, bucket, key, uploadId string) error { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + res, err := client.ListMultipartUploads(ctx, &s3.ListMultipartUploadsInput{ + Bucket: &bucket, + }) + cancel() + if err != nil { + return err + } + if len(res.Uploads) != 1 || getString(res.Uploads[0].Key) != key || + getString(res.Uploads[0].UploadId) != uploadId { + return fmt.Errorf("expected the upload %v of %v to be listed, instead got %+v", + uploadId, key, res.Uploads) + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + _, err = client.AbortMultipartUpload(ctx, &s3.AbortMultipartUploadInput{ + Bucket: &bucket, + Key: &key, + UploadId: &uploadId, + }) + cancel() + return err +} diff --git a/tests/integration/versioning.go b/tests/integration/versioning.go index 5af1e0ed..277846f8 100644 --- a/tests/integration/versioning.go +++ b/tests/integration/versioning.go @@ -2016,6 +2016,83 @@ func Versioning_DeleteObject_dir_object_with_children(s *S3Conf) error { }, withVersioning(types.BucketVersioningStatusEnabled)) } +func Versioning_DeleteObject_trailing_slash_counterpart(s *S3Conf) error { + testName := "Versioning_DeleteObject_trailing_slash_counterpart" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + expected := []types.ObjectVersion{} + // deleting the key with or without a trailing slash doesn't delete + // the object or any of its versions + for _, keys := range [][2]string{{"my-dir/", "my-dir"}, {"my-obj", "my-obj/"}} { + obj, other := keys[0], keys[1] + versions, err := createObjVersions(s3client, bucket, obj, 2) + if err != nil { + return err + } + expected = append(expected, versions...) + + for _, versionId := range []*string{versions[0].VersionId, versions[1].VersionId} { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + out, err := s3client.DeleteObject(ctx, &s3.DeleteObjectInput{ + Bucket: &bucket, + Key: &other, + VersionId: versionId, + }) + cancel() + if err != nil { + return fmt.Errorf("%v: %w", other, err) + } + if getString(out.VersionId) != *versionId { + return fmt.Errorf("%v: expected the versionId to be %v, instead got %v", + other, *versionId, getString(out.VersionId)) + } + if out.DeleteMarker != nil && *out.DeleteMarker { + return fmt.Errorf("%v: expected the response DeleteMarker to be false", other) + } + } + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err = s3client.DeleteObject(ctx, &s3.DeleteObjectInput{ + Bucket: &bucket, + Key: &other, + }) + cancel() + if err != nil { + return fmt.Errorf("%v: %w", other, err) + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + res, err := s3client.HeadObject(ctx, &s3.HeadObjectInput{ + Bucket: &bucket, + Key: &obj, + }) + cancel() + if err != nil { + return fmt.Errorf("%v: %w", obj, err) + } + if getString(res.VersionId) != getString(versions[0].VersionId) { + return fmt.Errorf("%v: expected the versionId to be %v, instead got %v", + obj, getString(versions[0].VersionId), getString(res.VersionId)) + } + } + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + res, err := s3client.ListObjectVersions(ctx, &s3.ListObjectVersionsInput{ + Bucket: &bucket, + }) + cancel() + if err != nil { + return err + } + + if !compareVersions(expected, res.Versions) { + return fmt.Errorf("expected the versions to be %v, instead got %v", + expected, res.Versions) + } + + return nil + }, withVersioning(types.BucketVersioningStatusEnabled)) +} + func Versioning_Delete_null_versionId_object(s *S3Conf) error { testName := "Versioning_Delete_null_versionId_object" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { @@ -4196,6 +4273,139 @@ func Versioning_WORM_remove_delete_marker_under_bucket_default_retention(s *S3Co }, withLock()) } +func Versioning_WORM_trailing_slash_counterpart(s *S3Conf) error { + testName := "Versioning_WORM_trailing_slash_counterpart" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + rDate := time.Now().Add(time.Hour).UTC().Truncate(time.Second) + lockedObjs := []objToDelete{} + + for _, keys := range [][2]string{{"my-dir/", "my-dir"}, {"my-obj", "my-obj/"}} { + obj, other := keys[0], keys[1] + res, err := putObjectWithData(objDataLen(obj, 10), &s3.PutObjectInput{ + Bucket: &bucket, + Key: &obj, + ObjectLockLegalHoldStatus: types.ObjectLockLegalHoldStatusOn, + ObjectLockMode: types.ObjectLockModeGovernance, + ObjectLockRetainUntilDate: &rDate, + }, s3client) + if err != nil { + return err + } + versionId := getString(res.res.VersionId) + lockedObjs = append(lockedObjs, objToDelete{ + key: obj, + versionId: versionId, + removeOnlyLeglHold: true, + }) + + // the version belongs to the object, not to the other key + err = checkObjectLockErr(s3client, bucket, other, versionId, s3err.GetAPIError(s3err.ErrNoSuchVersion)) + if err != nil { + return fmt.Errorf("%v: %w", other, err) + } + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err = s3client.DeleteObject(ctx, &s3.DeleteObjectInput{ + Bucket: &bucket, + Key: &other, + VersionId: &versionId, + }) + cancel() + if err != nil { + return fmt.Errorf("%v: %w", other, err) + } + + err = checkObjectLock(s3client, bucket, obj, versionId, rDate) + if err != nil { + return fmt.Errorf("%v: %w", obj, err) + } + } + + return cleanupLockedObjects(s3client, bucket, lockedObjs) + }, withLock()) +} + +func Versioning_WORM_null_version_locked_with_legal_hold(s *S3Conf) error { + testName := "Versioning_WORM_null_version_locked_with_legal_hold" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + objs := []string{"my-obj", "my-dir/"} + // the objects are put before versioning is enabled: their + // current versions are the null versions + _, err := putObjects(s3client, objs, bucket) + if err != nil { + return err + } + + err = putBucketVersioningStatus(s3client, bucket, types.BucketVersioningStatusEnabled) + if err != nil { + return err + } + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err = s3client.PutObjectLockConfiguration(ctx, &s3.PutObjectLockConfigurationInput{ + Bucket: &bucket, + ObjectLockConfiguration: &types.ObjectLockConfiguration{ + ObjectLockEnabled: types.ObjectLockEnabledEnabled, + }, + }) + cancel() + if err != nil { + return err + } + + lockedObjs := []objToDelete{} + err = forEachKey(objs, func(obj string) error { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err := s3client.PutObjectLegalHold(ctx, &s3.PutObjectLegalHoldInput{ + Bucket: &bucket, + Key: &obj, + VersionId: getPtr(nullVersionId), + LegalHold: &types.ObjectLockLegalHold{ + Status: types.ObjectLockLegalHoldStatusOn, + }, + }) + cancel() + if err != nil { + return err + } + lockedObjs = append(lockedObjs, objToDelete{ + key: obj, + versionId: nullVersionId, + removeOnlyLeglHold: true, + }) + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + res, err := s3client.GetObjectLegalHold(ctx, &s3.GetObjectLegalHoldInput{ + Bucket: &bucket, + Key: &obj, + VersionId: getPtr(nullVersionId), + }) + cancel() + if err != nil { + return err + } + if res.LegalHold == nil || res.LegalHold.Status != types.ObjectLockLegalHoldStatusOn { + return fmt.Errorf("expected the legal hold status to be %q, instead got %v", + types.ObjectLockLegalHoldStatusOn, res.LegalHold) + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + _, err = s3client.DeleteObject(ctx, &s3.DeleteObjectInput{ + Bucket: &bucket, + Key: &obj, + VersionId: getPtr(nullVersionId), + }) + cancel() + return checkApiErr(err, s3err.GetAPIError(s3err.ErrObjectLocked)) + }) + if err != nil { + return err + } + + return cleanupLockedObjects(s3client, bucket, lockedObjs) + }) +} + func Versioning_AccessControl_GetObjectVersion(s *S3Conf) error { testName := "Versioning_AccessControl_GetObjectVersion" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { @@ -5032,3 +5242,46 @@ func Versioning_PutGetDeleteObjectTagging_success(s *S3Conf) error { }) }, withVersioning(types.BucketVersioningStatusEnabled)) } + +func Versioning_ObjectTagging_trailing_slash_counterpart(s *S3Conf) error { + testName := "Versioning_ObjectTagging_trailing_slash_counterpart" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + tagSet := []types.Tag{{Key: getPtr("key"), Value: getPtr("value")}} + + for _, keys := range [][2]string{{"my-dir/", "my-dir"}, {"my-obj", "my-obj/"}} { + obj, other := keys[0], keys[1] + res, err := putObjectWithData(objDataLen(obj, 10), &s3.PutObjectInput{ + Bucket: &bucket, + Key: &obj, + Tagging: getPtr("key=value"), + }, s3client) + if err != nil { + return err + } + versionId := getString(res.res.VersionId) + + // the version belongs to the object, not to the other key + err = checkObjectTaggingErr(s3client, bucket, other, versionId, s3err.GetAPIError(s3err.ErrNoSuchVersion)) + if err != nil { + return fmt.Errorf("%v: %w", other, err) + } + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + out, err := s3client.GetObjectTagging(ctx, &s3.GetObjectTaggingInput{ + Bucket: &bucket, + Key: &obj, + VersionId: &versionId, + }) + cancel() + if err != nil { + return fmt.Errorf("%v: %w", obj, err) + } + if !areTagsSame(out.TagSet, tagSet) { + return fmt.Errorf("%v: expected the tag set to be %v, instead got %v", + obj, tagSet, out.TagSet) + } + } + + return nil + }, withVersioning(types.BucketVersioningStatusEnabled)) +} From d204d2e2383ac511d6641e47603a2c6ed5996290 Mon Sep 17 00:00:00 2001 From: niksis02 Date: Sun, 20 Sep 2026 15:35:08 +0400 Subject: [PATCH 2/3] fix: stop posix copying data from a delete marker `CopyObject` and `UploadPartCopy` never looked at the source's `delete-marker` attribute. A delete marker leaves the data file in place at the object path and only flags it, so a copy whose source resolved to a marker opened that file and succeeded, handing back the data of the version the marker had deleted. AWS rejects such a copy: `NoSuchKey` when the marker is the current version of the key, and `InvalidRequest` when the marker is named by version id, the latter regardless of whether it is the latest version. Versions the marker hides stay copyable by version id. Both copy paths now run the resolved source through `checkCopySourceDeleteMarker` once the entry has been validated, returning `NoSuchKey` for an unqualified source and the new `ErrCopySourceDeleteMarker` for one carrying a version id. --- backend/posix/posix.go | 24 +++++ s3err/s3err.go | 6 ++ tests/integration/group-tests.go | 4 + tests/integration/utils.go | 22 +++++ tests/integration/versioning.go | 151 +++++++++++++++++++++++++++++++ 5 files changed, 207 insertions(+) diff --git a/backend/posix/posix.go b/backend/posix/posix.go index a93ad686..b3c7eaaf 100644 --- a/backend/posix/posix.go +++ b/backend/posix/posix.go @@ -1618,6 +1618,24 @@ func (p *Posix) isObjDeleteMarker(bucket, object string) (bool, error) { return true, nil } +// checkCopySourceDeleteMarker rejects a copy whose source resolves to a +// delete marker: the key has no current version when the marker is the +// latest, and a marker named by version id holds no data to copy. +func (p *Posix) checkCopySourceDeleteMarker(bucket, object, versionId string) error { + isDel, err := p.isObjDeleteMarker(bucket, object) + if err != nil { + return err + } + if !isDel { + return nil + } + if versionId != "" { + return s3err.GetAPIError(s3err.ErrCopySourceDeleteMarker) + } + + return s3err.GetAPIError(s3err.ErrNoSuchKey) +} + // Converts the file to object version. Finds all the object versions, // delete markers from the versioning directory and returns func (p *Posix) fileToObjVersions(bucket string) backend.GetVersionsFunc { @@ -4034,6 +4052,9 @@ func (p *Posix) UploadPartCopy(ctx context.Context, upi *s3.UploadPartCopyInput) if strings.HasSuffix(srcObject, "/") != fi.IsDir() { return s3response.CopyPartResult{}, s3err.GetAPIError(s3err.ErrNoSuchKey) } + if err := p.checkCopySourceDeleteMarker(srcBucket, srcObject, srcVersionId); err != nil { + return s3response.CopyPartResult{}, err + } // a directory object holds no data srcSize := fi.Size() if fi.IsDir() { @@ -6237,6 +6258,9 @@ func (p *Posix) CopyObject(ctx context.Context, input s3response.CopyObjectInput if !strings.HasSuffix(srcObject, "/") && fi.IsDir() { return s3response.CopyObjectOutput{}, s3err.GetAPIError(s3err.ErrNoSuchKey) } + if err := p.checkCopySourceDeleteMarker(srcBucket, srcObject, srcVersionId); err != nil { + return s3response.CopyObjectOutput{}, err + } // a directory object holds no data srcSize := fi.Size() var srcBody io.Reader = f diff --git a/s3err/s3err.go b/s3err/s3err.go index 5a02d440..6262d77e 100644 --- a/s3err/s3err.go +++ b/s3err/s3err.go @@ -131,6 +131,7 @@ const ( ErrMissingDateHeader ErrGetUploadsWithKey ErrVersionsWithKey + ErrCopySourceDeleteMarker ErrInvalidRequest ErrAuthNotSetup ErrNotImplemented @@ -453,6 +454,11 @@ var errorCodeResponse = map[ErrorCode]APIError{ Description: "There is no such thing as the ?versions sub-resource for a key", HTTPStatusCode: http.StatusBadRequest, }, + ErrCopySourceDeleteMarker: { + Code: "InvalidRequest", + Description: "The source of a copy request may not specifically refer to a delete marker by version id.", + HTTPStatusCode: http.StatusBadRequest, + }, ErrInvalidRequest: { Code: "InvalidRequest", Description: "Invalid Request.", diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go index c9961db4..cbed13f6 100644 --- a/tests/integration/group-tests.go +++ b/tests/integration/group-tests.go @@ -1957,6 +1957,7 @@ func TestVersioning(ts *TestState) { ts.Run(Versioning_CopyObject_success) ts.Run(Versioning_CopyObject_non_existing_version_id) ts.Run(Versioning_CopyObject_from_an_object_version) + ts.Run(Versioning_CopyObject_from_a_delete_marker) if !ts.conf.windowsTests { ts.Run(Versioning_CopyObject_special_chars) } @@ -2026,6 +2027,7 @@ func TestVersioning(ts *TestState) { ts.Run(Versioning_UploadPartCopy_encoded_versionid_separator_invalid_versionId) ts.Run(Versioning_UploadPartCopy_non_existing_versionId) ts.Run(Versioning_UploadPartCopy_from_an_object_version) + ts.Run(Versioning_UploadPartCopy_from_a_delete_marker) // Object lock configuration ts.Run(Versioning_object_lock_not_enabled_on_bucket_creation) ts.Run(Versioning_Enable_object_lock) @@ -3494,6 +3496,7 @@ func GetIntTests() IntTests { "Versioning_CopyObject_success": Versioning_CopyObject_success, "Versioning_CopyObject_non_existing_version_id": Versioning_CopyObject_non_existing_version_id, "Versioning_CopyObject_from_an_object_version": Versioning_CopyObject_from_an_object_version, + "Versioning_CopyObject_from_a_delete_marker": Versioning_CopyObject_from_a_delete_marker, "Versioning_CopyObject_special_chars": Versioning_CopyObject_special_chars, "Versioning_HeadObject_invalid_versionId": Versioning_HeadObject_invalid_versionId, "Versioning_HeadObject_non_existing_object_version": Versioning_HeadObject_non_existing_object_version, @@ -3550,6 +3553,7 @@ func GetIntTests() IntTests { "Versioning_UploadPartCopy_encoded_versionid_separator_invalid_versionId": Versioning_UploadPartCopy_encoded_versionid_separator_invalid_versionId, "Versioning_UploadPartCopy_non_existing_versionId": Versioning_UploadPartCopy_non_existing_versionId, "Versioning_UploadPartCopy_from_an_object_version": Versioning_UploadPartCopy_from_an_object_version, + "Versioning_UploadPartCopy_from_a_delete_marker": Versioning_UploadPartCopy_from_a_delete_marker, "Versioning_object_lock_not_enabled_on_bucket_creation": Versioning_object_lock_not_enabled_on_bucket_creation, "Versioning_Enable_object_lock": Versioning_Enable_object_lock, "Versioning_status_switch_to_suspended_with_object_lock": Versioning_status_switch_to_suspended_with_object_lock, diff --git a/tests/integration/utils.go b/tests/integration/utils.go index 7f5e2cbb..f849a966 100644 --- a/tests/integration/utils.go +++ b/tests/integration/utils.go @@ -2375,6 +2375,28 @@ func createObjVersions(client *s3.Client, bucket, object string, count int, opts return versions, nil } +// createDeleteMarker deletes object without a version id, making the +// resulting delete marker the current version, and returns its version id. +func createDeleteMarker(client *s3.Client, bucket, object string) (string, error) { + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + out, err := client.DeleteObject(ctx, &s3.DeleteObjectInput{ + Bucket: &bucket, + Key: &object, + }) + cancel() + if err != nil { + return "", err + } + if out.DeleteMarker == nil || !*out.DeleteMarker { + return "", fmt.Errorf("expected a delete marker to be created for %v", object) + } + if getString(out.VersionId) == "" { + return "", fmt.Errorf("expected non empty delete marker versionId for %v", object) + } + + return *out.VersionId, nil +} + // objDataLen returns the data length to upload for key: a directory // object can't hold data func objDataLen(key string, lgth int64) int64 { diff --git a/tests/integration/versioning.go b/tests/integration/versioning.go index 277846f8..45f2491a 100644 --- a/tests/integration/versioning.go +++ b/tests/integration/versioning.go @@ -670,6 +670,78 @@ func Versioning_CopyObject_from_an_object_version(s *S3Conf) error { }, withVersioning(types.BucketVersioningStatusEnabled)) } +// A copy source that resolves to a delete marker is rejected: the key has no +// current version when the marker is the latest, and naming the marker by +// version id is an invalid request. Versions the marker hides stay copyable. +func Versioning_CopyObject_from_a_delete_marker(s *S3Conf) error { + testName := "Versioning_CopyObject_from_a_delete_marker" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + dstBucket, dstObj := getBucketName(), "dst-obj" + if err := setup(s, dstBucket); err != nil { + return err + } + + err := forEachKey([]string{"my-obj", "my-dir/"}, func(srcObj string) error { + srcObjVersions, err := createObjVersions(s3client, bucket, srcObj, 1) + if err != nil { + return err + } + + delMarker, err := createDeleteMarker(s3client, bucket, srcObj) + if err != nil { + return err + } + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err = s3client.CopyObject(ctx, &s3.CopyObjectInput{ + Bucket: &dstBucket, + Key: &dstObj, + CopySource: getPtr(fmt.Sprintf("%v/%v", bucket, srcObj)), + }) + cancel() + if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrNoSuchKey)); err != nil { + return err + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + _, err = s3client.CopyObject(ctx, &s3.CopyObjectInput{ + Bucket: &dstBucket, + Key: &dstObj, + CopySource: getPtr(fmt.Sprintf("%v/%v?versionId=%v", + bucket, srcObj, delMarker)), + }) + cancel() + if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrCopySourceDeleteMarker)); err != nil { + return err + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + out, err := s3client.CopyObject(ctx, &s3.CopyObjectInput{ + Bucket: &dstBucket, + Key: &dstObj, + CopySource: getPtr(fmt.Sprintf("%v/%v?versionId=%v", + bucket, srcObj, getString(srcObjVersions[0].VersionId))), + }) + cancel() + if err != nil { + return err + } + + if getString(out.CopySourceVersionId) != getString(srcObjVersions[0].VersionId) { + return fmt.Errorf("expected the copy-source-version-id to be %v, instead got %v", + getString(srcObjVersions[0].VersionId), getString(out.CopySourceVersionId)) + } + + return nil + }) + if err != nil { + return err + } + + return teardown(s, dstBucket) + }, withVersioning(types.BucketVersioningStatusEnabled)) +} + func Versioning_CopyObject_special_chars(s *S3Conf) error { testName := "Versioning_CopyObject_special_chars" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { @@ -3009,6 +3081,85 @@ func Versioning_UploadPartCopy_from_an_object_version(s *S3Conf) error { }, withVersioning(types.BucketVersioningStatusEnabled)) } +// A copy source that resolves to a delete marker is rejected: the key has no +// current version when the marker is the latest, and naming the marker by +// version id is an invalid request. Versions the marker hides stay copyable. +func Versioning_UploadPartCopy_from_a_delete_marker(s *S3Conf) error { + testName := "Versioning_UploadPartCopy_from_a_delete_marker" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + return forEachKey([]string{"my-obj", "my-dir/"}, func(srcObj string) error { + dstBucket, dstObj := getBucketName(), "dst-obj" + if err := setup(s, dstBucket); err != nil { + return err + } + + srcObjVersions, err := createObjVersions(s3client, bucket, srcObj, 1) + if err != nil { + return err + } + + delMarker, err := createDeleteMarker(s3client, bucket, srcObj) + if err != nil { + return err + } + + mp, err := createMp(s3client, dstBucket, dstObj) + if err != nil { + return err + } + + partNumber := int32(1) + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err = s3client.UploadPartCopy(ctx, &s3.UploadPartCopyInput{ + Bucket: &dstBucket, + Key: &dstObj, + UploadId: mp.UploadId, + PartNumber: &partNumber, + CopySource: getPtr(fmt.Sprintf("%v/%v", bucket, srcObj)), + }) + cancel() + if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrNoSuchKey)); err != nil { + return err + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + _, err = s3client.UploadPartCopy(ctx, &s3.UploadPartCopyInput{ + Bucket: &dstBucket, + Key: &dstObj, + UploadId: mp.UploadId, + PartNumber: &partNumber, + CopySource: getPtr(fmt.Sprintf("%v/%v?versionId=%v", + bucket, srcObj, delMarker)), + }) + cancel() + if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrCopySourceDeleteMarker)); err != nil { + return err + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + out, err := s3client.UploadPartCopy(ctx, &s3.UploadPartCopyInput{ + Bucket: &dstBucket, + Key: &dstObj, + UploadId: mp.UploadId, + PartNumber: &partNumber, + CopySource: getPtr(fmt.Sprintf("%v/%v?versionId=%v", + bucket, srcObj, getString(srcObjVersions[0].VersionId))), + }) + cancel() + if err != nil { + return err + } + + if getString(out.CopySourceVersionId) != getString(srcObjVersions[0].VersionId) { + return fmt.Errorf("expected the copy-source-version-id to be %v, instead got %v", + getString(srcObjVersions[0].VersionId), getString(out.CopySourceVersionId)) + } + + return teardown(s, dstBucket) + }) + }, withVersioning(types.BucketVersioningStatusEnabled)) +} + func Versioning_Enable_object_lock(s *S3Conf) error { testName := "Versioning_Enable_object_lock" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { From ba02c766abf09da68b014366bdc5686211f6101c Mon Sep 17 00:00:00 2001 From: niksis02 Date: Sun, 20 Sep 2026 16:07:40 +0400 Subject: [PATCH 3/3] fix: stop posix rewriting the current version on a copy to itself `CopyObject` treated a destination path equal to the source path as an in-place metadata rewrite, regardless of bucket versioning. On a versioned bucket a self copy therefore edited the current version and returned its existing version id instead of creating a new one. This also defeated object lock: `CheckObjectAccess` skips the retention and legal hold checks for overwrites on version-enabled buckets because an overwrite is expected to create a new version, so a `COMPLIANCE` retained or legal held version had its metadata replaced underneath it. A self copy is now rewritten in place only when the bucket is unversioned, and otherwise goes through the regular copy path, which creates a new version and leaves the one it replaces untouched. The rejection of a self copy that replaces nothing moved out of the in-place branch and is now applied only when the copy source carries no version id. Naming a version explicitly makes the request a regular copy, which AWS accepts even with the `COPY` metadata directive, while versitygw answered `InvalidRequest`. Source tagging for a `COPY` tagging directive is now read before `PutObject` writes the destination rather than after. A self copy replaces the source object's attributes, so the later read returned nothing and the tags were dropped from the new version. --- backend/posix/posix.go | 53 +++-- backend/posix/posix_io_helpers.go | 19 ++ backend/posix/posix_io_helpers_test.go | 79 +++++++ tests/integration/group-tests.go | 6 + tests/integration/versioning.go | 275 +++++++++++++++++++++++++ 5 files changed, 416 insertions(+), 16 deletions(-) create mode 100644 backend/posix/posix_io_helpers_test.go diff --git a/backend/posix/posix.go b/backend/posix/posix.go index b3c7eaaf..165a67f9 100644 --- a/backend/posix/posix.go +++ b/backend/posix/posix.go @@ -6310,11 +6310,18 @@ func (p *Posix) CopyObject(ctx context.Context, input s3response.CopyObjectInput var chType types.ChecksumType dstObjdPath := joinPathWithTrailer(p.BucketPath(dstBucket), dstObject) - if dstObjdPath == objPath { - if input.MetadataDirective == types.MetadataDirectiveCopy { - return s3response.CopyObjectOutput{}, s3err.GetAPIError(s3err.ErrInvalidCopyDest) - } + // A copy of an object onto itself is rejected unless it replaces the + // object metadata. Naming a source version makes it a regular copy. + selfCopy := dstObjdPath == objPath + if selfCopy && srcVersionId == "" && + input.MetadataDirective == types.MetadataDirectiveCopy { + return s3response.CopyObjectOutput{}, s3err.GetAPIError(s3err.ErrInvalidCopyDest) + } + // In a versioned bucket a self copy creates a new version like any other + // write, so only unversioned buckets are rewritten in place. + versioned := p.versioningEnabled() && vStatus != "" + if selfCopy && !versioned { // Delete the object metadata err = p.meta.DeleteAttribute(dstBucket, dstObject, metadataHdr) if err != nil && !errors.Is(err, meta.ErrNoSuchKey) { @@ -6452,6 +6459,14 @@ func (p *Posix) CopyObject(ctx context.Context, input s3response.CopyObjectInput checksums.Algorithm = input.ChecksumAlgorithm } + // A self copy publishes the new version over the source path, which + // on Windows can't be renamed over while the source is still open. + // PutObject reads the body before publishing, so the handle is + // released as soon as the data has been staged. + if selfCopy { + srcBody = &closeOnEOFReader{r: srcBody, c: f} + } + putObjectInput := s3response.PutObjectInput{ Bucket: &dstBucket, Key: &dstObject, @@ -6488,23 +6503,29 @@ func (p *Posix) CopyObject(ctx context.Context, input s3response.CopyObjectInput putObjectInput.Tagging = input.Tagging } - res, err := p.PutObject(withCtxNoSlot(ctx), putObjectInput) - if err != nil { - return s3response.CopyObjectOutput{}, err - } - - // copy the source object tagging after the destination object - // creation, if tagging directive is "COPY" + // read the source tagging before the destination is written, as a + // self copy replaces the source object's metadata + var srcTagging []byte + var hasSrcTagging bool if input.TaggingDirective == types.TaggingDirectiveCopy { tagging, err := p.meta.RetrieveAttribute(nil, srcBucket, srcObject, tagHdr) if err != nil && !errors.Is(err, meta.ErrNoSuchKey) { return s3response.CopyObjectOutput{}, fmt.Errorf("get source object tagging: %w", err) } - if err == nil { - err := p.meta.StoreAttribute(nil, dstBucket, dstObject, tagHdr, tagging) - if err != nil { - return s3response.CopyObjectOutput{}, fmt.Errorf("set destination object tagging: %w", err) - } + srcTagging, hasSrcTagging = tagging, err == nil + } + + res, err := p.PutObject(withCtxNoSlot(ctx), putObjectInput) + if err != nil { + return s3response.CopyObjectOutput{}, err + } + + // the source tagging is stored after the destination object creation, + // if tagging directive is "COPY" + if hasSrcTagging { + err := p.meta.StoreAttribute(nil, dstBucket, dstObject, tagHdr, srcTagging) + if err != nil { + return s3response.CopyObjectOutput{}, fmt.Errorf("set destination object tagging: %w", err) } } diff --git a/backend/posix/posix_io_helpers.go b/backend/posix/posix_io_helpers.go index 62eed318..76d8fd70 100644 --- a/backend/posix/posix_io_helpers.go +++ b/backend/posix/posix_io_helpers.go @@ -16,6 +16,7 @@ package posix import ( "bufio" + "errors" "io" "log" "sync" @@ -45,6 +46,24 @@ func (b *bufferedReadCloser) Close() error { return b.c.Close() } +// closeOnEOFReader closes c once r is drained, for readers whose source has +// to be released before the caller is done with the reader. +type closeOnEOFReader struct { + r io.Reader + c io.Closer + closed bool +} + +func (e *closeOnEOFReader) Read(p []byte) (int, error) { + n, err := e.r.Read(p) + if errors.Is(err, io.EOF) && !e.closed { + e.closed = true + e.c.Close() + } + + return n, err +} + var odirectUnsupportedWarnByOp sync.Map func warnODirectUnsupportedOnce(op string, err error) { diff --git a/backend/posix/posix_io_helpers_test.go b/backend/posix/posix_io_helpers_test.go new file mode 100644 index 00000000..5beb962f --- /dev/null +++ b/backend/posix/posix_io_helpers_test.go @@ -0,0 +1,79 @@ +// Copyright 2026 Versity Software +// This file is licensed under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package posix + +import ( + "bytes" + "io" + "strings" + "testing" +) + +type countingCloser struct { + count int +} + +func (c *countingCloser) Close() error { + c.count++ + return nil +} + +func TestCloseOnEOFReader(t *testing.T) { + c := &countingCloser{} + r := &closeOnEOFReader{r: strings.NewReader("hello"), c: c} + + buf := make([]byte, 2) + n, err := r.Read(buf) + if err != nil { + t.Fatalf("read: %v", err) + } + if n != 2 { + t.Fatalf("expected 2 bytes, got %v", n) + } + if c.count != 0 { + t.Fatalf("expected the source to stay open before EOF, closed %v times", c.count) + } + + data, err := io.ReadAll(r) + if err != nil { + t.Fatalf("read all: %v", err) + } + if !bytes.Equal(data, []byte("llo")) { + t.Fatalf("expected the remaining data to be llo, got %s", data) + } + if c.count != 1 { + t.Fatalf("expected the source to be closed once at EOF, closed %v times", c.count) + } + + // reads past EOF don't close the source again + if _, err := r.Read(buf); err != io.EOF { + t.Fatalf("expected io.EOF, got %v", err) + } + if c.count != 1 { + t.Fatalf("expected the source to be closed once, closed %v times", c.count) + } +} + +func TestCloseOnEOFReaderEmptySource(t *testing.T) { + c := &countingCloser{} + r := &closeOnEOFReader{r: strings.NewReader(""), c: c} + + if _, err := io.ReadAll(r); err != nil { + t.Fatalf("read all: %v", err) + } + if c.count != 1 { + t.Fatalf("expected the source to be closed once at EOF, closed %v times", c.count) + } +} diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go index cbed13f6..6f5d8c79 100644 --- a/tests/integration/group-tests.go +++ b/tests/integration/group-tests.go @@ -1958,6 +1958,8 @@ func TestVersioning(ts *TestState) { ts.Run(Versioning_CopyObject_non_existing_version_id) ts.Run(Versioning_CopyObject_from_an_object_version) ts.Run(Versioning_CopyObject_from_a_delete_marker) + ts.Run(Versioning_CopyObject_to_itself) + ts.Run(Versioning_CopyObject_to_itself_from_the_current_version) if !ts.conf.windowsTests { ts.Run(Versioning_CopyObject_special_chars) } @@ -2056,6 +2058,7 @@ func TestVersioning(ts *TestState) { ts.Run(Versioning_WORM_delete_marker_locked_object_compliance_retention) ts.Run(Versioning_WORM_PutObject_overwrite_locked_object) ts.Run(Versioning_WORM_CopyObject_overwrite_locked_object) + ts.Run(Versioning_WORM_CopyObject_to_itself_locked_object) ts.Run(Versioning_WORM_CompleteMultipartUpload_overwrite_locked_object) if !ts.conf.windowsTests { ts.Run(Versioning_WORM_remove_delete_marker_under_bucket_default_retention) @@ -3497,6 +3500,8 @@ func GetIntTests() IntTests { "Versioning_CopyObject_non_existing_version_id": Versioning_CopyObject_non_existing_version_id, "Versioning_CopyObject_from_an_object_version": Versioning_CopyObject_from_an_object_version, "Versioning_CopyObject_from_a_delete_marker": Versioning_CopyObject_from_a_delete_marker, + "Versioning_CopyObject_to_itself": Versioning_CopyObject_to_itself, + "Versioning_CopyObject_to_itself_from_the_current_version": Versioning_CopyObject_to_itself_from_the_current_version, "Versioning_CopyObject_special_chars": Versioning_CopyObject_special_chars, "Versioning_HeadObject_invalid_versionId": Versioning_HeadObject_invalid_versionId, "Versioning_HeadObject_non_existing_object_version": Versioning_HeadObject_non_existing_object_version, @@ -3578,6 +3583,7 @@ func GetIntTests() IntTests { "Versioning_WORM_delete_marker_locked_object_compliance_retention": Versioning_WORM_delete_marker_locked_object_compliance_retention, "Versioning_WORM_PutObject_overwrite_locked_object": Versioning_WORM_PutObject_overwrite_locked_object, "Versioning_WORM_CopyObject_overwrite_locked_object": Versioning_WORM_CopyObject_overwrite_locked_object, + "Versioning_WORM_CopyObject_to_itself_locked_object": Versioning_WORM_CopyObject_to_itself_locked_object, "Versioning_WORM_CompleteMultipartUpload_overwrite_locked_object": Versioning_WORM_CompleteMultipartUpload_overwrite_locked_object, "Versioning_WORM_remove_delete_marker_under_bucket_default_retention": Versioning_WORM_remove_delete_marker_under_bucket_default_retention, "Versioning_WORM_trailing_slash_counterpart": Versioning_WORM_trailing_slash_counterpart, diff --git a/tests/integration/versioning.go b/tests/integration/versioning.go index 45f2491a..8e71ecd5 100644 --- a/tests/integration/versioning.go +++ b/tests/integration/versioning.go @@ -742,6 +742,182 @@ func Versioning_CopyObject_from_a_delete_marker(s *S3Conf) error { }, withVersioning(types.BucketVersioningStatusEnabled)) } +// A copy of an object onto itself in a versioned bucket is an ordinary +// write: it creates a new version and leaves the one it replaces untouched. +// Without a metadata directive there is nothing to replace, so it's rejected. +func Versioning_CopyObject_to_itself(s *S3Conf) error { + testName := "Versioning_CopyObject_to_itself" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + return forEachKey([]string{"my-obj", "my-dir/"}, func(obj string) error { + // directory objects always carry the directory content-type + srcContentType, dstContentType := "text/plain", "application/json" + if strings.HasSuffix(obj, "/") { + srcContentType, dstContentType = directoryContentType, directoryContentType + } + + srcMeta := map[string]string{"key": "value"} + r, err := putObjectWithData(objDataLen(obj, 1234), &s3.PutObjectInput{ + Bucket: &bucket, + Key: &obj, + ContentType: getPtr("text/plain"), + Metadata: srcMeta, + }, s3client) + if err != nil { + return err + } + + srcVersionId := getString(r.res.VersionId) + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err = s3client.CopyObject(ctx, &s3.CopyObjectInput{ + Bucket: &bucket, + Key: &obj, + CopySource: getPtr(fmt.Sprintf("%v/%v", bucket, obj)), + }) + cancel() + if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrInvalidCopyDest)); err != nil { + return err + } + + dstMeta := map[string]string{"new-key": "new-value"} + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + out, err := s3client.CopyObject(ctx, &s3.CopyObjectInput{ + Bucket: &bucket, + Key: &obj, + CopySource: getPtr(fmt.Sprintf("%v/%v", bucket, obj)), + MetadataDirective: types.MetadataDirectiveReplace, + ContentType: getPtr("application/json"), + Metadata: dstMeta, + }) + cancel() + if err != nil { + return err + } + + dstVersionId := getString(out.VersionId) + if dstVersionId == "" { + return fmt.Errorf("expected non empty versionId") + } + if dstVersionId == srcVersionId { + return fmt.Errorf("expected a new versionId, instead got %v", dstVersionId) + } + + // the replaced version keeps its own metadata + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + res, err := s3client.HeadObject(ctx, &s3.HeadObjectInput{ + Bucket: &bucket, + Key: &obj, + VersionId: &srcVersionId, + }) + cancel() + if err != nil { + return err + } + + if getString(res.ContentType) != srcContentType { + return fmt.Errorf("expected the source version content-type to be %v, instead got %v", + srcContentType, getString(res.ContentType)) + } + if !areMapsSame(res.Metadata, srcMeta) { + return fmt.Errorf("expected the source version metadata to be %v, instead got %v", + srcMeta, res.Metadata) + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + res, err = s3client.HeadObject(ctx, &s3.HeadObjectInput{ + Bucket: &bucket, + Key: &obj, + }) + cancel() + if err != nil { + return err + } + + if getString(res.VersionId) != dstVersionId { + return fmt.Errorf("expected the current versionId to be %v, instead got %v", + dstVersionId, getString(res.VersionId)) + } + if getString(res.ContentType) != dstContentType { + return fmt.Errorf("expected the new version content-type to be %v, instead got %v", + dstContentType, getString(res.ContentType)) + } + if !areMapsSame(res.Metadata, dstMeta) { + return fmt.Errorf("expected the new version metadata to be %v, instead got %v", + dstMeta, res.Metadata) + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + vRes, err := s3client.ListObjectVersions(ctx, &s3.ListObjectVersionsInput{ + Bucket: &bucket, + Prefix: &obj, + }) + cancel() + if err != nil { + return err + } + + if len(vRes.Versions) != 2 { + return fmt.Errorf("expected 2 object versions, instead got %v", len(vRes.Versions)) + } + + return nil + }) + }, withVersioning(types.BucketVersioningStatusEnabled)) +} + +// Naming the current version in the copy source makes a copy onto the same +// key a regular copy, so it is accepted even without a metadata directive. +func Versioning_CopyObject_to_itself_from_the_current_version(s *S3Conf) error { + testName := "Versioning_CopyObject_to_itself_from_the_current_version" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + return forEachKey([]string{"my-obj", "my-dir/"}, func(obj string) error { + versions, err := createObjVersions(s3client, bucket, obj, 1) + if err != nil { + return err + } + + srcVersionId := getString(versions[0].VersionId) + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + out, err := s3client.CopyObject(ctx, &s3.CopyObjectInput{ + Bucket: &bucket, + Key: &obj, + CopySource: getPtr(fmt.Sprintf("%v/%v?versionId=%v", bucket, obj, srcVersionId)), + }) + cancel() + if err != nil { + return err + } + + if getString(out.CopySourceVersionId) != srcVersionId { + return fmt.Errorf("expected the copy-source-version-id to be %v, instead got %v", + srcVersionId, getString(out.CopySourceVersionId)) + } + if getString(out.VersionId) == srcVersionId { + return fmt.Errorf("expected a new versionId, instead got %v", getString(out.VersionId)) + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + res, err := s3client.HeadObject(ctx, &s3.HeadObjectInput{ + Bucket: &bucket, + Key: &obj, + VersionId: &srcVersionId, + }) + cancel() + if err != nil { + return err + } + + if getString(res.VersionId) != srcVersionId { + return fmt.Errorf("expected the source version to remain, instead got %v", + getString(res.VersionId)) + } + + return nil + }) + }, withVersioning(types.BucketVersioningStatusEnabled)) +} + func Versioning_CopyObject_special_chars(s *S3Conf) error { testName := "Versioning_CopyObject_special_chars" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { @@ -4250,6 +4426,105 @@ func Versioning_WORM_CopyObject_overwrite_locked_object(s *S3Conf) error { }, withLock()) } +// A copy of a locked object onto itself creates a new version, leaving the +// locked one and its legal hold in place. +func Versioning_WORM_CopyObject_to_itself_locked_object(s *S3Conf) error { + testName := "Versioning_WORM_CopyObject_to_itself_locked_object" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + return forEachKey([]string{"my-obj", "my-dir/"}, func(obj string) error { + versions, err := createObjVersions(s3client, bucket, obj, 1) + if err != nil { + return err + } + + v := versions[0] + v.IsLatest = getPtr(false) + lockedVersionId := getString(v.VersionId) + + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err = s3client.PutObjectLegalHold(ctx, &s3.PutObjectLegalHoldInput{ + Bucket: &bucket, + Key: &obj, + LegalHold: &types.ObjectLockLegalHold{ + Status: types.ObjectLockLegalHoldStatusOn, + }, + }) + cancel() + if err != nil { + return err + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + copyResult, err := s3client.CopyObject(ctx, &s3.CopyObjectInput{ + Bucket: &bucket, + Key: &obj, + CopySource: getPtr(fmt.Sprintf("%v/%v", bucket, obj)), + MetadataDirective: types.MetadataDirectiveReplace, + ContentType: getPtr("application/json"), + }) + cancel() + if err != nil { + return err + } + + if getString(copyResult.VersionId) == lockedVersionId { + return fmt.Errorf("expected a new versionId, instead got %v", + getString(copyResult.VersionId)) + } + + version := types.ObjectVersion{ + ETag: copyResult.CopyObjectResult.ETag, + IsLatest: getPtr(true), + Key: &obj, + Size: v.Size, + VersionId: copyResult.VersionId, + StorageClass: types.ObjectVersionStorageClassStandard, + ChecksumType: copyResult.CopyObjectResult.ChecksumType, + } + + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + out, err := s3client.ListObjectVersions(ctx, &s3.ListObjectVersionsInput{ + Bucket: &bucket, + Prefix: &obj, + }) + cancel() + if err != nil { + return err + } + + if !compareVersions([]types.ObjectVersion{version, v}, out.Versions) { + return fmt.Errorf("expected the object versions to be %v, instead got %v", + []types.ObjectVersion{version, v}, out.Versions) + } + + // the legal hold stays on the version it was set on + ctx, cancel = context.WithTimeout(context.Background(), shortTimeout) + lhRes, err := s3client.GetObjectLegalHold(ctx, &s3.GetObjectLegalHoldInput{ + Bucket: &bucket, + Key: &obj, + VersionId: &lockedVersionId, + }) + cancel() + if err != nil { + return err + } + + if lhRes.LegalHold.Status != types.ObjectLockLegalHoldStatusOn { + return fmt.Errorf("expected the legal hold status to be %v, instead got %v", + types.ObjectLockLegalHoldStatusOn, lhRes.LegalHold.Status) + } + + return cleanupLockedObjects(s3client, bucket, []objToDelete{ + { + key: obj, + versionId: lockedVersionId, + removeOnlyLeglHold: true, + }, + }) + }) + }, withLock()) +} + func Versioning_WORM_CompleteMultipartUpload_overwrite_locked_object(s *S3Conf) error { testName := "Versioning_WORM_CompleteMultipartUpload_overwrite_locked_object" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {